Add unit test cover for the MessageTemplate::renderMessageTemplate function #19551
Conversation
|
(Standard links)
|
eileenmcnaughton
force-pushed
the
msg_tpl_convert_test
branch
3 times, most recently
from
9957b25
to
dba415b
Compare
eileenmcnaughton
changed the title
| @@ -1234,7 +1234,7 @@ public static function getTokenDetails( | |||
| if (!empty($contactDetails[$contactID]['preferred_communication_method']) | |||
| ) { | |||
| $communicationPreferences = []; | |||
| foreach ($contactDetails[$contactID]['preferred_communication_method'] as $val) { | |||
| foreach ((array) $contactDetails[$contactID]['preferred_communication_method'] as $val) { | |||
There was a problem hiding this comment.
Ensuring it is an array fixed a test fail & makes sense when iterating
| * @throws \CiviCRM_API3_Exception | ||
| */ | ||
| public function getAllContactTokens(): array { | ||
| return [ |
There was a problem hiding this comment.
@totten this & the function below (for domains) provide a full set of token data for the relevant entity. You might be able to extract to a json sample but there are a couple of gotchas around the rows that reference related entities
You can see how each token presents in https://github.com/civicrm/civicrm-core/pull/19551/files#diff-c3c60600f080b49f2a9029b82865098c2a2742fbcedce0ecd731376917d80e1bR162
Note this PR should be merged & I'll rebase #19550 because I think we will need to discuss the slight differences #19550 highlights in the test run & this actually fixes an error in the extraction (& adds tests)
eileenmcnaughton
force-pushed
the
msg_tpl_convert_test
branch
5 times, most recently
from
28b270a
to
41e9e1b
Compare
|
This looks good. However test fails relate: I think the hardcoded token list is a good compromise for the test but note as tokenProcessor preloads and caches values we may need to revisit the list (as currently it's creating contacts etc. on the fly eg. for contact_id). Also be good to get a html style tag in here somewhere as that trips things up unless escaped properly (though we could add that as a follow-up). |
eileenmcnaughton
force-pushed
the
msg_tpl_convert_test
branch
from
41e9e1b
to
ba9ac92
Compare
|
@mattwire hopefully I have fixed the caching bug causing the test fail - perhaps once this is merged you could follow up with a patch of the issue you mentioned - before we consider making any changes to the actual function (ie merging #19950 or similar) I'm not 100% sure what the tokenProcessor caching is that you mention - as long as it uses Civi::cache() or Civi::statics (probably more appropriate for everything except domain) it should be flushed in between tests? |
|
Ok, not strictly a cache but a "preload". Looks like contact tokens may not be using it though - see https://github.com/civicrm/civicrm-core/blob/master/CRM/Activity/Tokens.php#L88 for activities. |
|
@mattwire OK - that will probably not impact how we add test cover I don't think |
…nction This provides cover for the 4 methods - domain tokens - contact tokens - smarty assigns within the function - smarty assigns before the function
eileenmcnaughton
force-pushed
the
msg_tpl_convert_test
branch
from
ba9ac92
to
160e732
Compare
Overview
Test provides full cover for the extracted renderMessageTemplate function covering the 4 methods of token replacement
In the process I identified a typo from the extraction which is tested & fixed in this.
The test supports the further cleanup of this function in #19550 but as it raises a couple of issues on that change it makes sense to merge this first
Before
Test cover incomplete
After
All domain tokens tested for all 3 strings
All contact tokens tested for all 3 strings, including 1 custom field
Assigning a smarty Variable before the function tested
Assigning a smarty variable within the function tested
Technical Details
Comments