(dev/mail#83) Introduce WorkflowMessage APIs with CaseActivity example

[totten]

Overview

This introduces APIs for handling WorkflowMessages and, to facilitate testing, it up-converts an example workflow (case_activity).

This is the last big extract from #20975.

Before

The great void.

After

WorkflowMessage.get API: Lookup the list of supported workflow-messages (names/classes/descriptions/etc).

$msgs = \Civi\Api4\WorkflowMessage::get()->execute()->indexBy('name');
assertEquals("When an activity is created in a case, the "case_activity" email is sent....", 
  $msgs['case_activity']['description']);

WorkflowMessage.getTemplateFields() API: Lookup the fields that are required for rendering a message.

$msgFields = Civi\Api4\WorkflowMessage::getTemplateFields()
  ->setWorkflow("case_activity")
  ->execute()->indexBy("name");
assertEquals('Integer', $msgFields['contactId']['data_type']);
assertEquals('Contact', $msgFields['contactId']['fk_entity']);
assertEquals('Integer', $msgFields['clientId']['data_type']);
assertEquals('String', $msgFields['idHash']['data_type']);

WorkflowMessage.render API: Allow one to render a message, performing any filtering/data-loading required
for the given message template.

$rendered = \Civi\Api4\WorkflowMessage()
  ->setWorkflow('case_activity')
  // Data to be loaded into the template. These are the published `modelProps`.
  ->setValues(['contactId' => 123, 'clientId' => 456, ...])
  // Optionally, specify the message template. If omitted, search for the default MessageTemplate for this workflow.
  ->setMessgeTemplate(['msg_text' => 'Hello {contact.display_name}'])
  ->execute();
assertEquals('Hello Mr. Example', $rendered->first()['msg_text']);

Note that the render() can autoload data. Consequently, there are a few security constraints:

• If you input an FK field (eg contactId=>123), then it calls CoreUtil::checkAccessDelegated() to see if you're allowed to read that entity.
• You're not allowed to call render() remotely unless you have some administrativy privileges (e.g. render templates or edit system workflow message templates.

ExampleData.get API: Lookup example data that can be used when previewing/rendering the workflow-message.

$example = \Civi\Api4\ExampleData::get()
  ->addWhere('name', '=', 'workflow/foo/bar')
  ->addSelect(['name', 'data'])
  ->execute();

[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).

[mattwire]

@totten I realise some of this is "inherited" code but I have concerns about the use of client_id:

• It's ambiguous and does not exist anywhere in our database schema.
• It does not clearly indicate that it should be a contact_id whereas eg. client_contact_id would.
• There can be multiple "clients" on a case.
• In this case as we are talking about case_activity are we really referring to the case "client/contact_id" or the activity target?

I'm very happy with the concept of this PR by the way!

[totten]

Case-Activity Message

@mattwire Right, at a high level preface -- I also dislike some specifics in the current contract for the case_activity message. (For example - it'd make more sense to me if the caller supplied activityId and caseId rather than $activityTypeName and $activityFields...)

The content in CaseActivity.php is just aiming to describe the existing expectations for the message, base on reviewing the $params and $params['tplParams'] generated by CRM_Case_BAO_Case::sendActivityCopy().

For client_id, some thoughts/observations:

• client_id looks to correspond to civicrm_case_contact.contact_id.
• I did a couple rounds of "Find Usage" (eg sendTemplate.*case_activity.* <= sendActivityCopy() <= sendToAssignee() <= ...). As near as I can tell, the only time when client_id is supplied is here. There are 3-4 other calls to sendToAssignee(), but I don't think they give client_id.
• When reading to assess the multiplicity of Case:Contact, I find contradictory information. The schema looks like N:N (stored as join-table civicrm_case_contact). But in the "Manage Case" UI, it behaves like N:1. (You assign one client when opening the case, and then it's immutable.) To my eye, this N:1 seems like the true multiplicity for the case-client relationship. And a {case_}client_id is a valid representation of N:1.
• In "Manage Case UI", you can assign other contacts in other roles (not the "Client"). But these are actually civicrm_relationship records.

General

But I'd like to shift your attention away from the specifics of the CaseActivity message and toward the process at higher level. We have a start-point where there are 30+ messages where inputs+outputs are all-over-the-place (incl weird/questionable elements). We want an end-point where the inputs are clear, pithy, consistent. Getting from here-to-there requires a mix of plays like:

1. Assess the message that exists. Figure out who initiates it, what data they provide. Figure out what's in the templates, what data they expect.
2. Add more/better inputs+outputs (eg $tokenContext['activityId'] which enables the {activity.*} tokens). Update corresponding callers or templates.
3. Phase-out silly inputs+outputs (eg $tplParams['activityTypeName']) with some mix of alias/bridge/migration. Update corresponding callers or templates.

To my thinking, it's useful to assess and provide the class as the declaration/documentation on the current-status. This makes it easier to plot other changes. From this perspective, the work can be broken into more bite-sized chunks, and the standard for the first revision is fairly modest.

But there is a bit of contradiction - this PR is also adding WorkflowMessage APIs. APIs imply support, maintenance, and higher upfront standard.

I think the contradiction goes away if we indicate different stability levels on different artifacts, eg

• The APIv4 surface (WorkflowMessage.get, WorkflowMessage.render, etc) should be held to the normal standard for an API. These APIs should be unit-tested and remain stable/backward-compatible even as we incrementally revise the descriptions for specific msgs (like case_activity). This provides the basis for editing/administrative UIs.
• The payloads - the specific messages (case_activity, contribution_recurring_edit, participant_cancelled, etc) - are different artifacts. I'd say that core messages presumptively continue their current support-level (ie we may change implementation of the callers - but we're conscientious about message-template compat), but I'd be open to the idea that some messages will be held to stricter standard.

So this means we need to communicate this support level a little more clearly. For example, I could imagine:

/**
 * @support public
 */
class GenericWorkflowMessage implements WorkflowMessageInterface { ... }

// or

/**
 * @support internal
 */
class CaseActivity extends GenericWorkflowMessage { ... }

$ cv api4 WorkflowMessage.get +s name,description,support -T

+---------------+-----------------------------------------------------------------------------+----------+
| name          | description                                                                 | support  |
+---------------+-----------------------------------------------------------------------------+----------+
| generic       | Generic base-class for describing the inputs for a workflow email template. | public   |
| case_activity | When an activity is created in a case, the "case_activity" email is sent.   | internal |
+---------------+-----------------------------------------------------------------------------+----------+

(Not sure the best wording for the support flag - but the main thing is to flag each with a different level.)

[mattwire]

@totten It's actually a setting in CiviCase Settings - "Allow Multiple Case Clients". I flagged it because it's an area we've got into a mess with before (some code expecting a single integer and other code expecting an array). That particular issue may have been fixed by now.

I understand that it's not the main point of this PR and I don't want to hold it up over this one point as we're initially just trying to replicate "like for like" but in a better way and with a better framework going forward. I also like the smarty->token aliasing PR as that provides a neat way to transition some of that.

So I'd say, if we can be clear about the level of support - ie. we're not locking in client_id forever - perhaps with the support field you describe + some code comments to help the next person then I'm happy to see this progress without "fixing" the client_id bit for now.

[totten]

@mattwire Thanks for pointing out the multi-client mode. I'd never used that option.

Added the @support field.

Pushed up some more docblocks on clientId and contactId.

[eileenmcnaughton]

@totten - we discussed this commit yesterday & specifically the slightly odd name - ie WorkflowMessageExample::get() and you suggested that @colemanw would be the only one likely to have another opinion

Looking at this PR is strikes me that an entity of 'SampleData::get()` would potentially be useful - since in this specific example the sampleData is just a contact and not tied to the workflow message

However, I suspect you feel that we DO need sample data specific to the message and that perhaps it could call a more generic sampleData in future.

I would probably choose WorkflowMessageSampleData as the entity after thinking about it - but I think @colemanw should have the last work on,

Based on my delving to date I would merge this commit if it were in it's own PR & we had a +1 from @colemanw on the naming

[totten]

@eileenmcnaughton I actually think a SampleData entity might make more sense. I had originally written as WorkflowMessageExample, then a couple days later kicked myself when I realized 80% would be the same for a general ExampleData. The only thing is that the file-naming convention (and therefore loader/scanner) would merit a rethink - because the current convention is tied to the WorkflowMessage classes (e.g. CRM/Contribution/WorkflowMessage/RecurringEdit/foo.ex.php).

(EDIT) Re-stating: Overall, I'm on the fence about whether it's better as WorkflowMessageExample or SampleData. The WorkflowMessageExample has the advantage of being written at this point - but I think if you and coleman believe in a generic sample-data makes more sense, then I'll be game for it.

[eileenmcnaughton]

OK - let's see if @colemanw pipes up....

[eileenmcnaughton]

@totten I went through the process of adding sample data for the 2 entities that I am testing this with - going htrough the process does bring me out on the side of SampleData::get()

My other thought was that it would be able to specify simpy the entity and get something that represents a very common set of fields - in the case of contact 'alex' in the case of 'contribution' - a Completed contribution like the data I put up. My recurring example is set up to use lots of fields - but that meant choosing a cancelled status - which seems like it isn't the 'normal' scenario

#21346

[colemanw]

+1 to what @eileenmcnaughton said. Maybe it's as simple as renaming the API to SampleData now and then adding non non-workflow-msg functionality in the future.

[colemanw]

Oh I get it. You serialize the value in the get action and then rely on the API output formatter to unserialize it back into an array.
That seems a bit silly. Why not just declare data_type => Array here?

[totten]

Right, I originally just had it return an array, but (IIRC) the explorer was happier if you coerced it to look more like MySQL data (ie with a serialization format). In a MySQL context, data_type=>Array seems nonsensical... but if APIv4 actually works with , then that's cool. I can give that a try...

[colemanw]

+1 to what @eileenmcnaughton said. Maybe it's as simple as renaming the API to SampleData now and then adding non non-workflow-msg functionality in the future.

[eileenmcnaughton]

@totten as you know I've been struggling with the addition of the case activity template for the same reasons @mattwire has and I think there is a better option than the 'it exists but we don't really support it' without losing the 'we really want it to provide the test cover' - in my testing simply moving those classes into the test folder c08b2fe allowed all the test to run without us having to figure out how we want this class to look like right now.

I've been focussing on a very simple template - (the recurring edit)[https://github.com//pull/21356/commits/c08b2fe4fee988d5526c1664df952d2e0464869f] - which basically requires

• recurring contribution tokens
• contact tokens
• one smarty variable that could be calculate from the recurring contribution id
  and so far it seems to be working pretty well so I definitely thing the code that does the heavy lifting here is mergeable - although it might be better to finalise the sample data as an api first given there is quite a bit of feedback on it

[eileenmcnaughton]

I don't want to bring it into scope for getting this merged but I've been digging into these 2 lines. It seems they basically do the same thing with subtle differences - since both actually 'replaceContactTokens' in the string.

Despite the name the way we use replaceGreetingTokens is not to replace greeting tokens - it is to replace contact tokens (generally).

The differences seem to be:

• the extent to which they replace 'blank' tokens - the first line DOES the second doesn't,
• the extent to which they attempt to optimise by early exiting if tokens are resolved and
• the extent to which they attempt to load the missing values in the contact

Test cover is good (#21387) & this function & replaceGreetingTokens are the only 2 places still calling replaceContactTokens so I think the goal would be to consolidate the replacment on this class & call this class from replaceGreetingTokens & deprecate replaceContactTokens.

[eileenmcnaughton]

With those last commits I think this is mergeable once tests pass

[seamuslee001]

This seems all fine to me