ManagedEntities - Track modification and auto-update

[colemanw]

Overview

Keeps track of user modifications to managed entities, and provides a new auto-update mode to prevent overwriting user-modified entities.

This new mode only works for those entities who have opted in to the v4 ManagedEntity trait; so far this is only SavedSearch and SearchDisplay, but others could easily be added.

Before

Each managed entity may specify two policies:

• update policy: When an upgraded extension provides updates to a managed entity (e.g. new search criteria for a SavedSearch), the options for how to manage this change were limited to:

  • always: force-update the savedSearch on-site with the new one from the extension, overwriting any user modifications
  • never: updates to the managed entity in the extension would be ignored

  The former is good for getting new functionality from extensions; the latter is respectful of user-customizations. Neither is perfect.

• cleanup policy: Similarly, when an extension is uninstalled, the options for deleting managed entities are:

  • always: delete it no matter what
  • never: leave it lying around for the user to deal with
  • unused: check fk references before deleting

  The unused mode is preferable but was broken for SearchDisplays, and for SavedSearches it failed to take Afforms into account.

After

All previous modes still work. The unused cleanup mode has been fixed for SearchDisplays and now accounts for usage by Afforms. Additionally, the following update mode is available (but only for compliant v4 entities):

• The new mode unmodified will be respectful of user changes and only pull in updates from upgraded managed files if the local copy is unmodified.

Comments

This gives SavedSearches, SearchDisplays, etc. the ability to behave like packaged Afforms, with a packaged copy that will always be used unless the user modifies it and creates the equivalent of a local copy, which can then be reverted. If the record is never manually edited then it will be treated like a packaged entity and aggressively managed, but once it is touched by the user it will be left alone. If the extension is uninstalled it will be removed if not in use by Afforms, Smart Groups, etc.

Technical Details

This implements hook_civicrm_post() to propagate updates to the civicrm_managed record:

• When a local copy of an entity is modified, update the timestamp for the civicrm_managed record.
• When a local copy of an entity is deleted, delete the civicrm_managed record.

It adds a new extra APIv4 field to the ManagedEntity trait: local_modified_date which can be used in UIs the same way as Afform's has_local field.

To track usage by Afforms, the Afform extension now implements hook_civicrm_referenceCounts() to check useages by SavedSearches and SearchDisplays.

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[totten]

1. If you use attempt to use the new policy on an entity that doesn't support it, what's expected to happen? Does it effectively use some other mode? Or does it raise an error?

2. Why does it delete the civicrm_managed record when the local entity is deleted? I guess my first interpretation of that scenario is that the user chooses not to have the record. (Ex: The user installs an extension which happens to include a ReportTemplate/ReportInstance; they delete the ReportInstance because they're not interested.) With this patch (where it deletes the civicrm_managed record), doesn't it mean that the ReportInstance will get recreated during the next reconciliation?

[colemanw]

1. If you use attempt to use the new policy on an entity that doesn't support it, what's expected to happen? Does it effectively use some other mode? Or does it raise an error?

The former. Because the modification timestamp is only updated for supported entities, for all others it will remain null; which in practice would cause the mode to act like "always".

2. Why does it delete the civicrm_managed record when the local entity is deleted? I guess my first interpretation of that scenario is that the user chooses not to have the record. (Ex: The user installs an extension which happens to include a ReportTemplate/ReportInstance; they delete the ReportInstance because they're not interested.) With this patch (where it deletes the civicrm_managed record), doesn't it mean that the ReportInstance will get recreated during the next reconciliation?

Yes, it would have that effect. IMO that's not a bad thing, after all these are managed entities, so I would expect them to reappear if deleted. Arguably they shouldn't do so if the policy is "never" but I'm not sure that's important enough to block this PR. For your reading pleasure, here is the writeup I did when I first submitted this PR with only the first commit. These details felt less important in the big picture once this PR evolved to what it is now, so I edited them out of the description, but the revision history is preserved for posterity:

Overview

Ensures managed records are always cleared out when an entity is deleted. This prevents perceived bugs caused by orphaned records hanging around in the civicrm_managed table.

To minimize risk and improve performance, this only targets those entities who have opted in to the v4 ManagedEntity trait.

Before

• Install an extension which contains a v4 managed entity (e.g. a SavedSearch) using cleanup: never.
• Uninstall the extension.
• Manually delete the managed SavedSearch.
• Reinstall the extension.
• Expected: the SavedSearch will reappear when re-installing the extension.
• Actual: it does not.

After
After uninstalling extension and manually deleteing its entities, one can re-install the extension and the entities will be reinstalled as well.

Technical Details
The problem is that entries in the civicrm_managed table were only ever deleted when an entity is deleted pragmatically by CRM_Core_ManagedEntities::reconcile. IMO they also ought to be deleted when an entity is manually deleted, so as not to leave orphaned entries lying around in the civicrm_managed table, which give the unexpected behavior described above when trying to re-install an extension and get your entities back.

This solves it by using hook_civicrm_post().

Note: in the original version of this commit I did not restrict it by entity type, and to get tests passing I had to fix OptionValue::delete which was previously not calling hooks. That's not strictly necessary in the current version with scope limited to only v4 ManagedEntity types, but I think OptionValue will eventually get opted-in, so IMO it's best for it to call hooks properly.

[totten]

If you use attempt to use the new policy on an entity that doesn't support it, what's expected to happen? Does it effectively use some other mode? Or does it raise an error?

The former. Because the modification timestamp is only updated for supported entities, for all others it will remain null; which in practice would cause the mode to act like "always".

Would this be a sensible place to emit a warning if someone has mixed unmodified with an entity that lacks modification-tracking?

I like that it's able to execute without crashing. 👍 It seems support would roll out incrementally (entity-by-entity/version-by-version) - so we'd expect various permutations {extension-version x managed-entity-policy x core-version x ...} over time. If a particular configuration declares a policy that can't be met, then a warning seems appropriate...

[totten]

I really like the idea of update=>unmodified. I think that addresses a big gap in how updates are propagated.

Thanks for adding the explanation about the first commit. I'm a little ambivalent about scenarios that involve cleanup=>never (ie to me, that mode seems innately leaky and caveat emptor). But maybe that's looking at the wrong thing to look at - since the overarching goal seems to be more about cleanup=>unmodified... So I guess the real concern is a scenario involving that?

Which has prompted me to think about cleanup=>unmodified more... I guess I'm not sure when to use it. My interpretation depends on which subsystem I consider first:

• Afform POV: The cleanup=>unmodified policy sounds like an accurate description of what happens if you use afform to go through the process of (a) install extension with bundled form, (b) edit/fork form, (c) remove extension. You are left with some content in [civicrm.files]/ang, so (in that sense) modified content is protected from cleanup. So this is like a straight port. (But, for me, the afform behavior was mostly incidental. At the time, my goal was to have file-storage, and I didn't care/consider this edge of the lifecycle.)
• Managed Entity POV: I can't think of a use-case where I would prefer cleanup=>umodified. Whether the user has modified the record or not, it may still be in use. It's the usages (the foreign references) that create unintuitive knock-on effects that one has to consider.

Getting too abstract... Maybe better to think about an example... Suppose we have an extension top_donor_analysis that defines a SavedSearch+SearchDisplay called TopDonors. As an admin, I might do some combination of:

1. Customize the TopDonors SearchDisplay.
2. Pull the TopDonors SearchDisplay into a standalone form.
3. Pull the TopDonors SearchDisplay into a dashlet.
4. Create a smart-group that references the TopDonors SavedSearch.

If the admin has only (1) customized the TopDonors, then I think it's appropriate for the uninstall to delete TopDonors (customized or not). Destroying TopDonors is certainly less destructive than DROP TABLE civicrm_mosaico_template, which we'd regard as normal for uninstallation. Maybe if they've derived a new artifact per (2)/(3)/(4), then it's analogous to a activity_type/payment_processor_type scenario (where deleting would have have cascade effect into destroying things outside the extension)? But even then I'm not sure. (If you uninstall top_donor_analysis extension, it makes sense for the TopDonor dashlet to go away, right?)

None of this is meant as a hard block on cleanup=>unmodified. But let me phrase it as question: when it comes time to r-document this, do you think you'll be able to explain an example where cleanup=>unmodified is a good policy?

[colemanw]

do you think you'll be able to explain an example where cleanup=>unmodified is a good policy?

I guess the question is, good compared to what? For search displays, it's the best choice available because the others are:

• always: deletes potentially in-use search displays, no go
• never: ok except it leaves stuff lying around during uninstall
• unused: sounds nice but is currently broken for SearchDisplays and would probably result in a hard-crash because
  a. It uses civicrm_api3(SearchDiplay, getRefCount) and SearchDisplay doesn't have a v3 API.
  b. We can't switch it to use v4 because APIv4 doesn't have a getRefCount action.
  c. If it did have such an action, we would need to add some extra code specifically for the SearchDisplay.getRefCount action to take Afforms into account; they would have to be counted separately.

So unmodified is a step-up from never but a step down from a hypothetical unused. Do you think we should try to implement that instead?

Here's another consideration: user visibility. Up until now, managed entities came and went in the shadows, it was mysterious or completely unknown to users. The Afform admin UI started to change that concept (albeit with file-based entities) and now I've implemented something very similar to the SearchKit UI. This is fast-moving code across some merged & in-progress PRs so just as a recap, that UI will show the user:

• A separate tab for packaged searches
• The name of the extension it was packaged in
• Whether and when it was overridden locally
• A revert button

Because of the user-visibility, I feel like having it work the way Afforms work is a mitigating factor which makes unmodified an acceptable option. But I agree with you that implementing unused would be a better outcome.

[totten]

sounds nice but is currently broken for SearchDisplays... v3...v4...getRefCount... implementing unused would be a better outcome...

OK, that all makes sense. 👍 getRefCount is not a trivial (15 min) thing. Let's go ahead with adding cleanup=>unmodified for APIv4 entities. In the docs for hook_managed, we should just explain this situation.

[colemanw]

@totten I was able to fix getRefCount to work with SearchDisplays, and your pointer about a hook gave me the hint I needed to get it working with Afforms. I have removed cleanup mode unmodified (and updated the PR description to omit it) because unused is better now that it's working. Added tests and I think this PR is good to go!

[totten]

Updates look pretty good. The test coverage seems pretty detailed and appropriate. 👍

Merging