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

dev/core#2851 Remove calls to replaceContributionTokens in contribution pdf letter task #21524

Closed

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Sep 18, 2021

Overview

Remove calls to replaceContributionTokens in pdf letter task (for non-grouped contributions)

Before

Legacy method used to resolve contribution tokens in contribution pdf letter task

After

Token processor used - legacy style still used for 'multiple contribution tokens' - but I have a plan for that

Technical Details

Note the output of cancel_date & thankyou_date is now date style - I've updated the upgrade notes for this. I expect wanting non-formatted (or these tokens at all) would be pretty edge https://lab.civicrm.org/-/ide/project/documentation/docs/sysadmin/tree/case/-/

Comments

This builds on #21525 which also makes minor token changes (added to the same docs PR) - for those we are changing the token syntax not the output so we can block the old syntax & advertise the new

@civibot
Copy link

civibot bot commented Sep 18, 2021

(Standard links)

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

test this please

@eileenmcnaughton eileenmcnaughton force-pushed the cont_render branch 2 times, most recently from c97af7a to ec5ad2e Compare September 19, 2021 01:47
@eileenmcnaughton eileenmcnaughton changed the title Remove calls to replaceContributionTokens Remove calls to replaceContributionTokens in contribution pdf letter task Sep 21, 2021
@eileenmcnaughton eileenmcnaughton changed the title Remove calls to replaceContributionTokens in contribution pdf letter task dev/core#2851 Remove calls to replaceContributionTokens in contribution pdf letter task Sep 21, 2021
@eileenmcnaughton
Copy link
Contributor Author

@demeritcowboy if you have any time to test this - it's probably the one I'm most blocked on at the moment

' . CRM_Utils_System::url('civicrm/event/info', NULL, TRUE) . '&reset=1&id=1
' . CRM_Utils_System::url('civicrm/event/register', NULL, TRUE) . '&reset=1&id=1
' . CRM_Utils_System::url('civicrm/event/info', NULL, TRUE) . '&reset=1&id=1
' . CRM_Utils_System::url('civicrm/event/register', NULL, TRUE) . '&reset=1&id=1
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused by how changing the pdf for contribution thank you letters affects the event urls for scheduled event reminders? I admit I haven't followed it thru yet but maybe you already know the answer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh yeah it is - that line is saying 'this is the html version I'm giving you' - which results in less change to the pdf output

Copy link
Contributor

Choose a reason for hiding this comment

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

&Thanks.

@monishdeb
Copy link
Member

I have tested the patch in my local and looks good to me. Reviewed the patch and the added unit-test capture the change accurately.

@demeritcowboy do you have any concern for merging this PR?

@demeritcowboy
Copy link
Contributor

Thanks @monishdeb the other PR #21652 includes this one as a commit but it's good to have extra testing since there's a billion variations on thank-you letters. So I think if that other one is merged this could be closed.

@eileenmcnaughton
Copy link
Contributor Author

@monishdeb #21521 is still untested....

@monishdeb
Copy link
Member

monishdeb commented Sep 29, 2021

Hmm does that mean

Thanks @monishdeb the other PR #21652 includes this one as a commit but it's good to have extra testing since there's a billion variations on thank-you letters. So I think if that other one is merged this could be closed.

Hmm ok, so I think I'll close this PR as the #21652 will be merged anyway.

@monishdeb #21521 is still untested....
Currently testing it on my local.

@monishdeb
Copy link
Member

@eileenmcnaughton @demeritcowboy I am closing this PR in favor of #21652

@monishdeb monishdeb closed this Sep 29, 2021
@eileenmcnaughton eileenmcnaughton deleted the cont_render branch September 29, 2021 04:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants