Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 3249: make casetype a managed entity #23313

Merged
merged 2 commits into from
May 4, 2022
Merged

Issue 3249: make casetype a managed entity #23313

merged 2 commits into from
May 4, 2022

Conversation

herbdool
Copy link
Contributor

@civibot
Copy link

civibot bot commented Apr 27, 2022

(Standard links)

@civibot civibot bot added the master label Apr 27, 2022
@demeritcowboy
Copy link
Contributor

Just noting

* Get a list of managed-entities representing auto-generated case-types
and I'm not sure off the top of my head how this would or wouldn't interact with that. Currently the way you export case types is to take the contents of the xml field and put it in a file in custom_templates/CRM/Case/xml/configuration/. But if they can play nice together with another export then that shouldn't be a problem.

@colemanw
Copy link
Member

I think the word "managed" is used to mean two different things here.

  1. "managed" case types are loaded from xml files in a bespoke way.
  2. "managed" entities are more generalized and work with any api (that uses the trait).

If the latter works well for case types, then I would say we deprecate the former.

@herbdool
Copy link
Contributor Author

I pasted an example here https://lab.civicrm.org/dev/core/-/issues/3429#note_74121.

I haven't tested adding it to an extension, but I may soon, and I wasn't aware of the old approach.

@herbdool
Copy link
Contributor Author

Perhaps trait CiviCaseSaveTrait needs to be updated too? It opens a case by using CRM_Case_XMLProcessor_Process(). Or would this managed entity basically create a new XML from the code?

@colemanw
Copy link
Member

@herbdool no I don't think other updates will be needed.
CiviCase types do not require an xml file, it's optional as the configuration can be stored in the database. I think this change is a good one and gets us farther away from that old system.

@colemanw
Copy link
Member

I'll let @totten weigh in here, but IMO we ought to deprecate hook_civicrm_caseTypes. See #23320

@herbdool
Copy link
Contributor Author

herbdool commented Apr 28, 2022

I tested my PR now and it seems to work, though darn slow.

I created a test extension and added an mgd.php file with:

return [
  [
    'name' => 'CaseType_test',
    'entity' => 'CaseType',
    'cleanup' => 'unused',
    'update' => 'unmodified',
    'params' => [
      'version' => 4,
      'values' => [
        'name' => 'test',
        'title' => 'Test',
        'description' => NULL,
        'is_active' => TRUE,
        'is_reserved' => NULL,
        'weight' => 1,
        'definition' => [
          'restrictActivityAsgmtToCmsUser' => 0,
          'activityAsgmtGrps' => [],
          'activityTypes' => [
            [
              'name' => 'Open Case',
              'max_instances' => '1',
            ],
            [
              'name' => 'Email',
            ],
            [
              'name' => 'Follow up',
            ],
            [
              'name' => 'Meeting',
            ],
            [
              'name' => 'Phone Call',
            ],
          ],
          'activitySets' => [
            [
              'name' => 'standard_timeline',
              'label' => 'Standard Timeline',
              'timeline' => 1,
              'activityTypes' => [
                [
                  'name' => 'Open Case',
                  'status' => 'Completed',
                  'label' => 'Open Case',
                  'default_assignee_type' => '1',
                ],
              ],
            ],
          ],
          'timelineActivityTypes' => [
            [
              'name' => 'Open Case',
              'status' => 'Completed',
              'label' => 'Open Case',
              'default_assignee_type' => '1',
            ],
          ],
          'caseRoles' => [
            [
              'name' => 'Case Coordinator',
              'creator' => '1',
              'manager' => '1',
            ],
          ],
        ],
      ],
    ],
  ],
];

Relies only on existing activity types. I enabled it via cv (on local dev) and it took over 3 minutes and most of my RAM. But it worked.

Uninstalling the extension is much faster but it doesn't remove the case type.

I also tested the same extension with only a mgd.php file for 1 contact type. That was fast (and disabling the extension removes the contact type). When I test creating 1 contact type with the case type, the contact type fails to appear. So something might be getting caught in a loop, but I haven't looked under the hood.

@totten
Copy link
Member

totten commented Apr 30, 2022

I'll let @totten weigh in here, but IMO we ought to deprecate hook_civicrm_caseTypes.

Yes, from a high level, 👍 I also like hook_managed(...CaseType...) more than hook_caseTypes:

  • Improved (good)
    • The generic-ness of hook_managed means you get more utility from understanding it.
    • hook_managed has more options vis-a-vis lifecycle, cleanup, etc.
  • Similar (good)
    • It still provides the ability to view/edit as code (and to track changes in SCM).
  • Ambivalent
    • When writing tooling that manipulates files (*.xml or *.mgd.php), there are some trade-offs between the formats, but I don't think those trade-offs matter much for how we use CaseType.
  • Comment
    • Like anything, hook_managed has limits. (Ex: Don't push 100,000 records through hook_managed...) But at first glance, I'm not super-concerned...

Re: #23320, I have issues with that PR at this time (being discussed over there). But please don't interpret that as a general obstacle to dev/core#3429. They can certainly coexist, and (for deprecation/transition) there's plenty of tasks.

IMHO, the transition would look something like this:

  • Near term
    • Do some testing around extension lifecycle/upgrades - eg if you upgrade an extension (getting a new CaseType definition that adds, edits, removes some ActivityType), then... do the interdependent things update properly? What happens if you've edited the ActivityType locally or added another ActivityType? Do policies like cleanup=>unmodified or cleanup=>unused actually work? (*I expect these to be somewhat sensible, since each part seems to have satisfactory precedent; but I encourage playing with it during QA, since this is a bit unusual and it's important to the story of "managed CaseType".)
    • File an issue about transitioning the hooks. (dev/core#3429 seems very focused on adding hook_managed(...CaseType...).)
  • Mid-term
  • Long-term
    • Remove hook_caseTypes aka mixins/case-xml from core
    • Remove hook_caseTypes aka mixins/case-xml from civix

@colemanw colemanw merged commit 1d39b83 into civicrm:master May 4, 2022
@colemanw
Copy link
Member

colemanw commented May 4, 2022

All that to say, yes we should merge this PR :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants