dev/core#2747 Reconcile remaining fields between scheduled reminders and legacy tokens #21046
Conversation
|
(Standard links)
|
| '{contribution.check_number}' => ts('Check Number'), | ||
| '{contribution.campaign}' => ts('Contribution Campaign'), | ||
| // @todo - we shouldn't need to include custom fields here - | ||
| // remove, with test. |
There was a problem hiding this comment.
The removed lines are from the function that determins the tokens that show up here.
Per the comments I thought the custom field line was also redundant & UI testing should it to be true - I wound up UI testing rather than unit
testing that custom fields are still advertised
as the test uses rollback & adding custom fields would break that.
| foreach ($fields as $field) { | ||
| $allFields[$field['name']] = $field['title']; | ||
| } | ||
| // $this->assertEquals($realLegacyTokens, $allFields); |
There was a problem hiding this comment.
This is commented out because it is not true yet & I want to discuss whether it should be TRUE
eileenmcnaughton
force-pushed
the
camp
branch
2 times, most recently
from
2b489f3 to
727d9d5
Compare
|
I've opened https://lab.civicrm.org/dev/core/-/issues/2745 to discuss the remaining fields |
eileenmcnaughton
changed the title
This adds the last fields that were in legacy tokens but not scheduled reminders and now the same code is deriving the list for each. I wound up UI testing rather than unit testing that custom fields are still advertised without this line as the test uses rollback & adding custom fields would break that.
|
As we've discussed re-adding pseudoconstants to campaign (with pre-fetch safeguards) I've ripped out all the special handling from here & it should be mergeable once the pseudoconstant is merged #21093 |
| '{contribution.cancel_reason}' => ts('Contribution Cancel Reason'), | ||
| '{contribution.amount_level}' => ts('Amount Level'), | ||
| '{contribution.check_number}' => ts('Check Number'), | ||
| '{contribution.campaign}' => ts('Contribution Campaign'), |
There was a problem hiding this comment.
Note I manually tested that this token still works in the original legacy flow - although it is no longer advertised and it never worked in the scheduled reminders flow. This change just stops it from being advertised
|
This looks fine and makes sense to me and I like the usage of pseudoconstants style for the fks in tokens |
Overview
This adds the last fields that were in legacy tokens but not scheduled reminders and now
the same code is deriving the list for each.
Before
The following fields are resolved by legacy tokens (send letter) but not by token processor (scheduled reminders) - from running the test in this PR before the change
After
Those fields are added - note that for campaign I have it as a pseudo-pseudo-constant. As with the 'true' pseudoconstants the ability to add label, id or machine name is offered. However, we have in-class caching since the pseudoconstant functions won't work with it.
Technical Details
We need a new metadata thing 'pseudo-pseudo-constant'
Comments
With this merged the 2 classes are in sync but there is a question as to why we have a white list of token fields - here is the test-derived comparison of the fields for which we offer tokens vs the full list for contributions. (the ones in green are missing from our offering). The question is then 'why not make them ALL available' - the main field which I think would increase is contact_id because it would probably be exposed via other token classes (like contact) - @colemanw thoughts