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

[REF] dev/core#2790 dev/core#2814 Start migration to MessageTemplate::render #21335

Merged
merged 1 commit into from
Sep 6, 2021

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Sep 1, 2021

Overview

[REF] dev/core#2790 Start migration to MessageTemplate::render

Before

long chunk of copy & paste used to replace most functions

After

MessageTemplate::render function used

Technical Details

  • the test cover should mean we can switch to using the api once the api is available if this is merged first
  • I've left the case tokens for later - hence they are before the smarty handling
  • we know the contact tokens have been reconciled
  • I want to remove this chunk as a follow up -
    list($contact) = CRM_Utils_Token::getTokenDetails($params,
    $returnProperties,
    $skipOnHold,
    $skipDeceased,
    NULL,
    $messageToken,
    'CRM_Contact_Form_Task_PDFLetterCommon'
    );
    if (civicrm_error($contact)) {
    $notSent[] = $contactId;
    continue;
    }
    - however it is still achieving 2 things for now
    1. possibly maybe filtering out deceased & on hold - I have my doubts but havent tested yet - that should be done before the rendering starts IMHO
    2. retrieving the contact to assign to the template IF smarty is enabled
  • @totten - this last point throws up a variant of what we have been discussing - an annoying variant - note I don't see it as blocking on merging this - just something for next PR

Comments

@civibot
Copy link

civibot bot commented Sep 1, 2021

(Standard links)

@civibot civibot bot added the master label Sep 1, 2021
@eileenmcnaughton
Copy link
Contributor Author

@demeritcowboy I'm gonna clean this rendering up before moving to the trait. I have some thoughts about cleaning up the case part too but this change should be small & safe IMHO

@eileenmcnaughton
Copy link
Contributor Author

eileenmcnaughton commented Sep 2, 2021

Also - it starts to add the test cover for tokens here

@eileenmcnaughton eileenmcnaughton changed the title [REF] dev/core#2790 Start migration to MessageTemplate::render [REF] dev/core#2790 dev/core#2814 Start migration to MessageTemplate::render Sep 3, 2021
@colemanw
Copy link
Member

colemanw commented Sep 6, 2021

Refactoring looks good and tests look solid.

@colemanw colemanw merged commit 14d70e1 into civicrm:master Sep 6, 2021
@colemanw colemanw deleted the pdft branch September 6, 2021 01:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants