ActionSchedule - Pass real batches into TokenProcessor. Simplify CRM_Activity_Tokens. #21088
Conversation
This try/catch block purports to catch any exceptions of type `TokenException`. However, if you grep the source tree, you will find that `TokenException` is never thrown.
Background: `ActionSchedule::sendMailings()` fetches a batch of pending reminders (per some specific schedule/rule). Then it composes and sends a message for each. Before: For each item in the batch, it makes a new `TokenProcessor`. This means that the `TokenProcessor` only sees one pending reminder. Therefore, the `TokenProcessor` cannot meaningfully fetch batched data. After: It creates one `TokenProcessor` and adds rows for each pending reminder. This means that `TokenProcessor` can fetch batched data.# On branch master-action-batch Comments: This part of a longer plot to simplify the `CRM_*_Tokens`. Currently, `CRM_*_Tokens` implements duplicate prefetch mechanisms -- the general-purpose `prefetch()` and the special-purpose `alterActionScheduleQuery()`. This patch makes it possible to use `prefetch()` for all the real work of prefetching (and phasing-out `alterActionScheduleQuery()`).
This will help us to consolidate the prefetching logic in `CRM_*_Tokens`. Currently, `CRM_*_Token::alterActionScheduleQuery()` updates the query to select all fields for the entity. This is regardless of how many fields there are, whether they are needed, etc. This is also prone to naming conflicts. With this patch, `CRM_*_Token::alterActionScheduleQuery()` only needs to select one field (`tokenContext_fooId`). This will then propagate to the `TokenProcessor` which can do its own optimized data-loading.
…tionSearchResult`. Before: There are two distinct ways in which `CRM_Activity_Tokens` can be called, eg * For scheduled reminders, it's given `$row->context[actionSearchResult]`. The query is inspected for data. * For everything else, it's given `$row->context[activityId]`. The value is After: `CRM_Activity_Tokens` always receives activities as `$row->context[activityId]`. Comment: There are pre-existing tests for activity-based reminders and tokens in `CRM_Core_BAO_ActionScheduleTest` and `CRM_Activity_ActionMappingTest`.
|
(Standard links)
|
| } | ||
|
|
||
| foreach ($dao->toArray() as $key => $value) { | ||
| if (preg_match('/^tokenContext_(.*)/', $key, $m)) { |
There was a problem hiding this comment.
So really this is the meaningful change in here - I think some comments around it would help. The fact this still passes tests means it must still work for contribution tokens - but I'm quite sure how
There was a problem hiding this comment.
@eileenmcnaughton Agree about comments. Added PR for that: #21098
For contribution tokens, this PR doesn't actually change anything. CRM_Contribution_Tokens::alterActionScheduleQuery() still does the same prefetch that it did before. That'll be the subsequent PR.
For this PR, I only touched CRM_Activity_Tokens::alterActionScheduleQuery() because that was the simplest one - a way to demonstrate the mechanism.
|
@totten so in theory if we use this with the Event tokens (I don't know if they are tested) then in a multilingual database we would get (before and after) different titles and different event type values for 2 rows where the contact's preferred language is french in the first row & english in the second row ? I'm confortable that we have tests on action schedule rendering at least one field in each of the entities that we handle in the scheduled reminders - but I haven't found any tests (yet) that check that we render per the contact's language |
|
I've realised that it would be possible to add multilingual testing to the contribution tokens test - we could test the db translation through the psuedoconstants & db-style if we extend it support contribution_status_id.thank_you_text - which is a format we've been discussing |
|
@mattwire did you r-run the multilingual aspects before merging? |
|
@eileenmcnaughton I didn't test the multilingual side because my understanding was that that is something we'd like to get working but has not been implemented until now? |
|
@mattwire I was assuming the references to locale in this PR & in the 'depends' pr https://github.com/civicrm/civicrm-core/pull/21085/files#diff-73feaf674c8b9ad9c02bb58a7bc1d05a2bf23a4cdc87ed281b51257ac773bf84R279-R281 meant something |
Hmm, right, multilingual is more complicated than just We have two prefetching patterns on the table (
For that goal, I think this PR leaves us in basically the same place as before -- maybe slightly better. I don't think it sets us back. Prefetching multilingual-subjective data with But I think that requirement is a bit ucky regardless. You'd either have to skip prefetching (ie load all data separately for every recipient), or you need a bigger data-structure for the cache (where it tracks locales as well as entity-ids and field-names). So, for example, one might edit |
|
OK - that makes sense for the db fetches. I would have either written a test or r-run with some location testing to test it before merging but since it's already merged I will let it be |
Overview
When sending scheduled reminders, the method
ActionSchedule::sendMailings()fetches a batch of pending reminders then composes+delivers a message for each. This PR changes the batch mechanics with an aim toward future simplifications inCRM_*_Tokens.(Depends: #21085. Extracted from #21079.)
Before
For each item in the batch, it makes a new and separate instance of
TokenProcessor. This means that theTokenProcessoronly sees one pending reminder at a time. Thus, a method likeCRM_Activity_Tokens::prefetch()can only prefetch one record at a time. In the context of scheduled reminders,prefetch()cannot meaningfully fetch batched data.(This is why several variants of
CRM_*_Tokensimplement duplicate data-loads. eg One general-purpose data-load uses$context['activityId']+prefetch(); but an auxiliary data-load is needed for ActionSchedules. It uses$context['actionSearchResult']+alterActionScheduleQuery().)After
It creates one
TokenProcessorand adds multiple rows (one row for each pending reminder). This means a method likeCRM_Activity_Tokens::prefetch()can see a full batch. In the context of scheduled reminders,TokenProcessorcan meaningfully fetch batched data.(This means that we don't need duplicate data-loads. The variants based on
$context['actionSearchResult']+alterActionScheduleQuery()can become redundant.)Comments
To demonstrate that the change works, the PR also updates
CRM_Activity_Tokensto rely on the new mechanics and to pare-downactionSearchResult.There is a fair amount of test-coverage for scheduled-reminders and tokens in
CRM_Core_BAO_ActionScheduleTestandCRM_Activity_ActionMappingTest. The correctness of the PR mostly rests on the existing tests.