-
-
Notifications
You must be signed in to change notification settings - Fork 825
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix another instance of double token rendering #21788
Conversation
(Standard links)
|
$contactDetails = []; | ||
$contactIds[] = $participant->contact_id; | ||
list($currentContactDetails) = CRM_Utils_Token::getTokenDetails($contactIds, NULL, | ||
FALSE, FALSE, NULL, [], 'CRM_Event_BAO_Participant'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems significant to me that (1) old code called CRM_Utils_Token::getTokenDetails()
but didn't do anything to indicate returnProperties
or the active-token-list. So it'd always return the same fields. (2) The subsequent references to $contactDetails
only u se display_name
and email
.
So the change here looks like a proper simplification.
if ($toEmail) { | ||
//take a receipt from as event else domain. | ||
$receiptFrom = $domainValues['name'] . ' <' . $domainValues['email'] . '>'; | ||
$receiptFrom = CRM_Core_BAO_Domain::getNameAndEmail(FALSE, TRUE); | ||
$receiptFrom = reset($receiptFrom); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, so this removes the last remaining reference to $domainValues
(in this function) - making all the 15 SLOC silliness above unnecessary. 👍
$form->transferParticipantRegistration($toContactId, $participantId); | ||
$mut->checkAllMailLog(['Default Domain Name Anthony']); | ||
$mut->clearMessages(); | ||
$this->revertTemplateToReservedTemplate('event_online_receipt', 'html'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test seems like a good idea and clever placement. 👍
Style-wise, some feedback but non-blocking reactions:
- This particular test-class declares
useTransaction(TRUE)
, so that should cleanup automatically. But if it were a test that needed manual cleanup, then (as a standard/pro-forma thing) it'd merit atry/finally
block. (When a leak from test A causes tests B+C to fail, it can be tricky to recognize the causation.) - Some of the method signatures are... quirky to me. But
CiviUnitTestCase
is internal surface-area, (and it's already got other things that seem quirky to my sensibility), so 🤷
Looks good - simpler/cleaner data-loading. Test passes. |
Overview
Fix another instance of double token rendering
Before
getTokenDetails
duplicates token resolutionAfter
Rely on token processor
Technical Details
Comments