Msg template rendered content api

[eileenmcnaughton]

Overview

This PR adds a new proposed apiv4 api:

MessageTemplate::render

Before

After

Api created...

    $result = \Civi\Api4\MessageTemplate::render()
      ->addMessage(['string' => $html_message, 'format' => 'text/html', 'key' => 'html_string'])
      ->setEntity('Activity')
      ->setEntityIds(['Activity' => $activityIds])
      ->setCheckPermissions(FALSE)
      ->execute();

    $html = [];
    foreach ($result as $htmDetail) {
      $html[] = $htmDetail['html_string'];
    }

Technical Details

In conjunction with looking at #16983 and also the general mess around message templates (and some thoughts I have about using them), I think we should look at an api for them. I think the render needs to be one action but we should probably have other actions like send and pdf or output - where pdf is an option at a later point.

Note that for retrieving un-rendered fieldMessageTemplate::get remains the goto- this new api for getting content parsed for CiviCRM (but not currently Smarty) tokens. It should parse one or more ad hoc strings or, if message_template_id is passed in, retrieve and parse the message template strings (this is not yet implemented).

When I added Contribution.sendConfirmation a long long time ago I feel like it was suggested there could be some security concerns - so this might need a bit of tightening so it doesn't return fields that should not otherwise be visible. At the moment Smarty is just not parsed because we would need to actively set context to smarty for that to work (and that needs more thought).

We will need to agree how this should look....

I chose the MessageTemplate entity as it made sense to me that we would want to be able to passing in the message_template identifier rather than a string, in which case we would also return the fields for the message template (msg_html, msg_text, msg_subject)

Also - it has the functions setEntityIDs and setEntity - note that in future we might want dependent entities - e.g the main entity is membership but there might be a contribution & we might use setEntities along with setRelatedEntityIds to handle that

Note this includes #17161

Comments

Also note I've updated existing tested code to call this api to provide the first round of test cover

@colemanw @totten @mattwire @demerit @Adyun @seamuselee001

[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]

This will need some thought to get this right. I'm not sure that getContent is the right name - from what I can see it is returning a rendered html version of the message template? With what currently exists in the messagetemplate table it could return plaintext or html + rendered or non-rendered. We need to be clear what it should be retrieving and the name and/or parameters should reflect that.

[eileenmcnaughton]

@mattwire "This will need some thought to get this right" - agree. When it comes to api calls we lock in the names & contract so we want to like them.

I'm not sure the distinctions of rendered / non-rendered? It parses each line for tokens - I feel like we would want to separate out 'just parsing tokens' & also parsing 'smarty tokens' - I have a feeling that at this stage you could pass a param & it might parse smarty - which needs to be managed from a security point of view.

At this stage it's fair to say the api 'parses content, replacing non-smarty-tokens'. My vision was that it would also retrieve it based on the message_template

[mattwire]

Hey @eileenmcnaughton so:

I'm not sure the distinctions of rendered / non-rendered?

You may want the contents of a message template pre- or post-token replacement / smarty processing. So "raw" (as saved in DB) or rendered. An example of this is extensions such as EmailAPI/PDFAPI which require a raw copy of the messagetemplate and then do the rendering themselves.
This will also likely apply if I get anywhere with plugging TWIG templating in as an option - as you'd then retrieve a raw template for rendering via TWIG.

Then the content: The messagetemplate table has three renderable fields - msg_subject, msg_text and msg_html. I see that your getContent call is currently rendering all three of them and returning an array. That's probably ok but remember that messagetemplates are used for other things (eg. SMS) where some of those fields are not required/used. The caller should probably know which fields they are interested in but the important thing I think is choosing whether to return the rendered or un-rendered versions.

I like the idea of passing in strings instead of templates but I don't like the two options - html/text as it unnecessarily constrains things and I'm not sure of the need for having two. I imagine in the future you might want to pass other things in - a richtext string or a twig template that renders to a mobile app.

I do think that there should be two separate API functions for getting the unrendered copy of a template and for rendering (replacing tokens / parsing templating language).

[eileenmcnaughton]

@mattwire - I think for non-rendered you would just use MessageTemplate::get

[eileenmcnaughton]

OK so more detail

1. raw vs rendered - I see this as the api for rendered. We don't have a gap on retrieving raw given we have 'get'

2. it is intended that we should parse several strings (as required) in one call in order to load the required data only once (I'm not sure the code does that at this stage but at some point...)

3. 2. text vs html - to be honest that's something the tokens support & I am not quite clear about the difference at the token level. The twig template is a good example. I guess to address that we would have to receive as an array
      ie setAdHocStringToParse(['string' => 'x', 'format' => 'y') instead of the HtmlStringToParse & TextString to parse. Format could be optional, probably defaulting to text.

[eileenmcnaughton]

@colemanw have you got thoughts on how this should look?

[eileenmcnaughton]

I've updated this to use setAdHocStringToParse rather than specified strings.

[eileenmcnaughton]

test this please

[eileenmcnaughton]

Based on feedback from @mattwire and @colemanw I've renamed the action to 'render'. I also switched from setEntities to setEntity and setAdHocStringToParse to addMessage.

Commits not squashed to show this change - but I will once I have more feedback

[eileenmcnaughton]

Actually jenkins couldn't cope with them unmerged..

[colemanw]

If $entity is single-valued then this should probably be as well.

[eileenmcnaughton]

No - one entity - ie Activity - but many ids - results in a row per id returned - that is how it's used in the PdfTemplate code - which might be printing multiple pdfs for multiple activities

[mattwire]

@eileenmcnaughton +1 on naming ::render. This is looking good, I haven't reviewed yet in detail.

Regarding passing entityIDs:

      ->setEntity('Activity')
      ->setEntityIds(['Activity' => $activityIds])

We should do our best to align naming this with the schema param in tokenProcessor, or align the schema param with this. I like the above - not sure if that means we should tweak tokenProcessor again.

Thinking along the lines of the gsoc project related to messagetemplates as well as some work I'm doing around TWIG for templates and this veda-consulting-company/uk.co.vedaconsulting.mosaico#361 ... Perhaps we don't add support initially for parsing smarty etc. but I think it should be part of this API later. That would work well with adding a template_type to the civicrm_msg_template table which would allow this API to choose how to render the template.

[eileenmcnaughton]

@mattwire yeah - re twig / smarty. I'm on the fence as to whether it would be part of this api or another action - ie pass the results to MessageTemplate::renderSmarty or similar

[eileenmcnaughton]

@mattwire also on the TokenProcessor - I find 'schema' a bit confusing - so yeah - maybe changing to above is good - we can still tinker on that. I was hoping to get this merged & then try something on a contribution template & see what that threw up

[totten]

I'm a bit wary about MessageTemplate\Render::setEntity(string $entityType) and MessageTemplate\Render::setEntityIds([string $entityType => int[] $ids]). In "normal" template-engine APIs (like Smarty or Twig or JSP or ERB or whatever), the inputs always have subjective/situational/symbolic names. You might conceivably use data-types to help validate the inputs - but it's not the primary way to identify the inputs.

What about:

$result = \Civi\Api4\MessageTemplate::render()
      ->addMessage(['string' => $html_message, 'format' => 'text/html', 'key' => 'html_string'])
      ->setContext(['activity_id' => 123])
      ->setCheckPermissions(FALSE)
      ->execute();

Then pass that through to the TokenProcessor (perhaps modulo some whitelist/validation/permission checks).

[eileenmcnaughton]

Hmm - I don't have a super strong opinion on that. I find context a bit 'vague' but I suppose that's the point.

But it feels a bit painful to use. If we start with an array of entity IDs it seems like you would quickly wind up writing a wrapper in the calling code o convert [1,6,7,9] to

[['activity_id' =>1],  ['activity_id' =>6],  ['activity_id' =>7],  ['activity_id' =>9]];

At which point you have to wonder why the helper wouldn't be on the api itself....

[mattwire]

@eileenmcnaughton Picking up on what @totten said re context. We need an unambiguous way to pass one or more IDs for different entities for each returned (rendered) template.

For example this kind of thing has always been hard in CiviCRM but is possible using tokenProcessor and emailapi. It is particularly useful when sending notification/informative emails about progress in a case when there is a relationship between case client and coordinator:

• Row1: Contact ID1 = 123, Contact ID2 = 456, Case ID1=12, Activity ID1 = 133, Activity ID2 = 142
• Row1: Contact ID1 = 125, Contact ID2 = 479, Case ID1=13, Activity ID1 = 144, Activity ID2 = 146

That's how tokenProcessor works - you set the "schema" and context for the tokens which can be mapped to a set of entities. I'm not sure how that could work with:

->setEntity('Activity')
->setEntityIds(['Activity' => $activityIds])

but it does work (for a single row) with:

->setContext(['activity_id' => 123])

I can see two ways of doing this:

• Either call render for every "context" - ie. render one template and return one.
• Handle an array of context arrays that map to each returned row.

[eileenmcnaughton]

Right so we have our common scenario where you have a base entity - which is commonly used ie - I want to send a template-based-email to these 50 contributions - and we want to be able to do that simply - so we set the base entity & the ids

$membershipTemplate->setEntityIDs($ids);

We also have a situation where we have a membership with a specific contribution. So at that point we need to pass something more like

foreach ($myArray as $array) {

  $membershipTemplate->addRowIDs(['Membership' => 4, 'Contribution' => 5);
}

That might not be exactly the look - but we want to have a fairly easy way to do it for the normal case & a more nuanced way for the less common complex case.

I guess that latter one could be called 'setContext' - I find that kinda vague & unclear but that's not a show stopper.

I don't see any of the patterns (or the underlying code or templates) having a way to deal with multiple activities and at some point we are getting past the scope of what this api should do

[mattwire]

@eileenmcnaughton Something like that would work well I think - we should mimic tokenProcessor where possible if it makes sense eg. https://lab.civicrm.org/mattwire/emailapi/-/blob/newtokenprocessor/api/v3/Email/Send.php#L275

I don't see any of the patterns (or the underlying code or templates) having a way to deal with multiple activities and at some point we are getting past the scope of what this api should do

TokenProcessor already supports this way of working but not all entities support tokenProcessor yet. It doesn't need to be perfect as we can extend it but let's get it as close as we can. I've done a few live sites for clients with the tokenProcessor and experimental emailapi and I can tell you it's really nice to be able to pass in a bunch of entities and know that all the tokens will render properly!

Even in the first case where it's just a bunch of contributions - they're still linked to a contact ID so you either rely on the contribution being used to map back to the contact so that contact tokens (eg. name) can be rendered or you pass in a contact ID.

[eileenmcnaughton]

@mattwire - OK - I'm kinda losing it on this one then.

I think we want this api to easily support setEntityIDs for the main entity and it should accept an array of ids. I'm open to extending with other methods but I've kind of lost sight of how to resolve this now. I don't want to make it not do what I think it needs to do (accept an array of the main entity ids) in a simple way that doesn't require the calling code to iterate through that array first.

I'm not sure if you are

a) proposing to change this so it no longer does that or
b) proposing further enhancements that could be left for a future round

[mattwire]

@eileenmcnaughton I think we're nearly there.

$membershipTemplate->setEntityIDs($ids);

This feels too limiting to me as we are restricting to a single entity type and are we assuming that it's one entity per template? It would be easier if triggering the API manually but I assume it'll normally be scripted / automatic in which case a bit more verbosity would not matter?
It also feels a bit ambiguous which ID maps to which template.

Do we want to call this API to render multiple templates at once, or should we call it once per set of entities?

To render multiple something like:

$membershipTemplate->addRowIDs(['Membership' => 4, 'Contribution' => [5,3,2]);

and

$membershipTemplate->addRowIDs(['Membership' => 4, 'Contribution' => [5,3,2], ['Membership' => 6, 'Contribution' => [7,8,9]);

The old token stuff doesn't support multiple entities but the new tokenProcessor should be able to and it would be nice to have the ability to send out summaries like the above - eg. "you've made the following contributions for this membership."

We could implement for now just taking the first ID passed for each element (but supporting them coming in as an array or integer).

[eileenmcnaughton]

@mattwire

I just don't see how

$membershipTemplate->addRowIDs(['Membership' => 4, 'Contribution' => [5,3,2], ['Membership' => 6, 'Contribution' => [7,8,9]);

works. But I'm fine it being a second round thing or another api. I still don't think the fact the token processor could support something more complex means we shouldn't have an easy way to do the thing we know we want it to do

[eileenmcnaughton]

I had a talk with @totten today & there is not a clear path forward on this so I'm closing it. Some key points that came out of our discussion

1. It's not currently clear how something like this would be called other than from php (@colemanw ) - we currently have cli and ajax methods for crud v4 APIs but need them for v4. Note that cli methods are pretty painful with apiv4 but @totten has a helper for that that I think should be in core.

2. we also talked about the fact that workflow message templates are currently reliant on quickform for context and at some point we want a path to render these from another form layer. Most variables in most workflow templates could currently be determined from one entity id+entity combo but currently most come from Smarty assignments in the form layer.

3. we covered that the content of a message should be renderable for preview usage.

[eileenmcnaughton]

As an aside this is an example of the sort of thing I'd been thinking to cleanup with a render api

civicrm-core/CRM/Contribute/BAO/ContributionPage.php

Line 265 in d04d887

if (!$returnMessageText && !empty($gIds)) {

You can see that it sends emails from this function - unless the function is really only being called to render the content - in which case it skips that bit....

[eileenmcnaughton]

Further update - it seems that any set method is available in json eg.