Skip to content

Instantly share code, notes, and snippets.

@mickadoo
Last active March 29, 2018 11:10
Show Gist options
  • Save mickadoo/8b23e5fc10d8f6d94f52525139fdcfe6 to your computer and use it in GitHub Desktop.
Save mickadoo/8b23e5fc10d8f6d94f52525139fdcfe6 to your computer and use it in GitHub Desktop.
Discussion on the current meaning of `is_reserved` and `is_locked` in CiviCRM. Suggestions for improvements.

DAO Definitions

These definitions are from the docblock comment on the DAO properties. A quick search for public $is_reserved showed 10 entities with this property, but I will focus on these three for now.

Option Group
  • is_reserved: Is this a predefined system option group (i.e. it can not be deleted)?
  • is_locked: A lock to remove the ability to add new options via the UI.
Option Value
  • is_reserved: Is this a predefined system object?
Custom Group
  • is_reserved: Is this a reserved Custom Group?

In Reality

Type Editable via UI Editable via API
Reserved Custom Group No, does not appear in civicrm/admin/custom/group Yes, could delete
Locked Option Group Yes**, but addding new options is disabled. Non-reserved option values can be deleted Yes, could delete
Reserved Option Group Yes**, options can be added and deleted Yes, could delete
Reserved Option Value Yes, but option to delete doesn't appear Yes, could delete

** I can't find any way to delete any option group via the UI

Real Definitions

  • is_reserved: For custom groups it means "hide it from editing in the UI". For option values it means "hide delete option in the UI". For option groups it means nothing.
  • is_locked: For options groups it means "hide the option to add a new option value"

Goals

  • As an extension developer I want to ensure some critical option groups and custom groups will not be deleted. I still want to be able to edit or delete them via the API (for example in hook_civicrm_uninstall).
  • As an organization we want to have a consistent language with the same rules being applied each time a concept such as is_reserved is used.

Suggestions

  • is_reserved means it will be uneditable from the UI. Attempts to modify via API will fail. Modification is possible, but only after changing is_reserved to false.
  • is_locked is only for groups containing child items. It means no items can be added or removed.
  • is_reserved is ignored on child items if the parent is deleted. For example, deleting a non-reserved option_group will also delete reserved option_values in it.

Problems

  • While it would be simpler to say is_reserved means uneditable, there are some properties would be very helpful to edit, even if the entity was reserved. For example "Title" and "Descrption" for option_group, "Weight", "Description" and "Label" for option_value or "Title" and "Weight" for custom_group. We might consider allowing edits to just these properties for reserved entities. That said, it would be much easier to stick to the logic of disallowing any edit to reserved entities.
  • There could be some confusion if we hide reserved items. We might decide to show reserved items, but in uneditable form.
@colemanw
Copy link

The "reserved" and "locked" flags in the context of option groups and custom groups are a little funny because one must possess "administer civicrm" permission in order to edit those entities in the first place. So by definition, anyone with permission to edit an option group has permission to do literally anything in the system. The flag is therefore more of a suggestion not to tamper with something rather than a deterrent.

That said, I'm in favor of consistency, and think your suggestions would be an improvement.

I'm not sure about disallowing edits via the api for reserved items, but allowing edits to the is_reserved flag via the api, that seems slightly silly, especially if we are allowing edits to certain fields via the UI.

What do you mean in your last note about hiding reserved items? I don't think we do that currently.

@mickadoo
Copy link
Author

The "reserved" and "locked" flags in the context of option groups and custom groups are a little funny because one must possess "administer civicrm" permission in order to edit those entities in the first place... The flag is therefore more of a suggestion not to tamper with something rather than a deterrent.

For CustomGroups the flag is important because it hides the reserved groups from the edit screen, regardless of user permissions.

There could be some confusion if we hide reserved items. We might decide to show reserved items, but in uneditable form. (clarification)

Reserved CustomGroups are hidden from the UI (civicrm/admin/custom/group) right now. While I like how this prevents them from being edited, I think it's a bit confusing if someone knows these CustomGroups do exist, but just can't see them. In my mind it would be clearer to show them, but just don't allow them to be deleted.

I'm not sure about disallowing edits via the api for reserved items, but allowing edits to the is_reserved flag via the api, that seems slightly silly

That's true, it might be a step too far. It's like a safeguard to protect against unintended edits, but it doesn't stop the user in the end.

What I'd like it a broad definition of what it means to be "is_reserved" and "is_locked", for example these questions might be used to make a definition. I'm not talking about permissions here. Like you said, you need the "Administer CiviCRM" permission to edit these anyway, this definition would be for all users, regardless of permission.

Question UI API
Can I edit it?
Can I edit all identifier properties, e.g. name?
Can I edit critical properties, e.g. extends for Custom Group?
Can I edit non-critical fields not used as identifiers, e.g. label, description?
Can I delete it?
Can I add new child elements to it? E.g. adding new option to an option group?
Can I remove existing child elements?
Will it be hidden in the UI? n/a
If not hidden, will it be displayed as frozen / uneditable? n/a

We might also consider the same list of questions to define is_locked

@mickadoo
Copy link
Author

@colemanw @totten I thought it was a funny coincidence that while we're discussing this issue someone in CiviHR made a PR to delete reserved relationship types, which would probably lead to a while world of trouble compucorp/civihr#2554

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment