Skip to content
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

Populate Template Params for Offline Event Confirmation Receipts #22878

Closed
wants to merge 2 commits into from

Conversation

alifrumin
Copy link
Contributor

@alifrumin alifrumin commented Mar 2, 2022

Overview

This populates the event information in the tplParams variable for Offline Event Confirmation Receipts.

Before

Event information not available in hook_civicrm_altermailparams for offline event confirmation receipts.

After

Event information available in hook_civicrm_altermailparams for offline event confirmation receipts.

Technical Details

Borrows from

@civibot
Copy link

civibot bot commented Mar 2, 2022

(Standard links)

@civibot civibot bot added the master label Mar 2, 2022
@alifrumin alifrumin closed this Mar 2, 2022
@alifrumin alifrumin reopened this Mar 2, 2022
$sendTemplateParams = [
'groupName' => 'msg_tpl_workflow_event',
'valueName' => 'event_offline_receipt',
'contactId' => $contactID,
'isTest' => !empty($this->_defaultValues['is_test']),
'PDFFilename' => ts('confirmation') . '.pdf',
'tplParams' => $tplParams,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alifrumin Here's a fun bit since v5.42 - the sendTemplate() function accepts an argument tokenContext, as in:

$sendTemplateParams = [
  ...
  'tokenContext' => ['eventId' => 123, 'participantId' => 456],
];

The upshot of using tokenContext is:

  • It enables Civi-style tokens (eg {event.description} or {participant.status_id:label}) which match tokens used elsewhere (eg Scheduled Reminders).
  • It doesn't need CRM_Event_BAO_Event::retrieve -- it will autoload data that is actually used.
    • (Ex: If the template says Go to {event.title} at {event.location}, then it effectively runs SELECT title, location FROM civicrm_event WHERE id = $eventId)

(There's another possible level to this - where we declare that event_offline_receipt expects parameters X,Y,Z. That makes it easier to provide admin tools - eg token-pickers and previews. I don't know if that's in-scope for what you're trying to address.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll let @alifrumin confirm but I did want to comment on this that I think it's not about the message template it's about alterMailParams. This is similar to the earlier ticket regarding contact id. In hook_alterMailParams, you don't have access to anything except what is passed to Mail::send(), which is really only guaranteed to be the message text and the recipient email address (which might match on several contacts so isn't an identifier), and you don't have tokenContext available even when it's from a message template (unless I'm missing how to get at it).

But altermailparams can be called from so many different paths, not just message templates, and so what variables make sense at any given time is going to be different (it may not even have a related contact id). So it would be nice to handle in a clean and documented/unit-tested way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, the list of fields (contactId vs contactId+eventId vs tplParams['event']['title']) represents a contract that should (ideally) be documented. That contract impacts a few different parties - eg (a) the code that fires event_offline_receipt, (b) the admin editing event_offline_receipt, (c) any extensions that hook into event_offline_receipt, and (d) any tests or previews involving event_offline_receipt.

If one just wants to make the {event.*} tokens functional, then it's not strictly necessary to define the contract. You just need to pass along $sendTemplateParams['tokenContext']['eventId'].

But if we're going to document the contract and use it for hooks, then I'd suggest two more pieces:

@alifrumin
Copy link
Contributor Author

Thanks guys!

I know there have been a bunch of token updates lately that I need to catch up on.

@totten is this what you had in mind? I just want to be able to access the event id from altermailparams hook. I don't care what its called so this works for me. I was mirroring how its done for the event_online_receipt.. which gets to how there is still a lack of consistency here but I think that is out of my current scope.

@demeritcowboy
Copy link
Contributor

Are you able to add a unit test so it doesn't get broken when more token stuff gets moved around? Maybe something similar to CRM/Event/Form/ParticipantTest::testParticipantOfflineReceipt(), but with the alterMailParams hook implemented checking that event id is present, similar to CRM/Contribute/Form/Task/PDFLetterCommonTest.

@alifrumin
Copy link
Contributor Author

That’s a good idea! I will work on that next week.

@demeritcowboy
Copy link
Contributor

Cool. Have made a ticket regarding the above suggestions for the variant of alterMailParams: https://lab.civicrm.org/dev/core/-/issues/3103

@eileenmcnaughton
Copy link
Contributor

eileenmcnaughton commented Mar 8, 2022

This #22904 should create a situation where the parameters available within sendMessageTemplate are standardised & their output in the template tested for tokens and having standard id fields assigned to smarty

@alifrumin
Copy link
Contributor Author

I'm good with #22904 instead of this.. especially since @eileenmcnaughton wrote tests and cleaned up other stuff.

@demeritcowboy
Copy link
Contributor

I think the other PR might make it available to alterMailParams although I don't immediately see where there's a test that ensures alterMailParams will always receive it (because it's mostly about tokens not the hook). Such a test could always be done separately, if someone gets bored and is looking for something to do.

@eileenmcnaughton
Copy link
Contributor

@demeritcowboy yeah - I think the other would solve this - but I haven't specifically checked that it does as it is also generally a standardisation we want to do

@alifrumin alifrumin closed this Mar 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants