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

Standardise our 2 pledge templates to use tokens, add tests #21847

Merged
merged 3 commits into from
Oct 23, 2021

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Standardise our 2 pledge templates to use tokens, add tests

Before

These 2 templates are still calling the legacy getTokenDetails which renders hooks (as well as using the tokent processor). $contact and $domain are assigned to the template and to some degree used

After

legacy getTokenDetails not longer called, templates updated to use tokens

Technical Details

Consistent with past template changes the upgrade script should upgrade any sites where the templates are not customised and notifty them if their templates are customised.

We have recently augmented this with a status check - which doesn't reply on them responding to the upgrade script

Comments

I think this is an appropriate level of effort for the pledge templates. My suspicion based on gitlab & github activity and general chat is that they are barely used, ditto pledge code in general

@civibot
Copy link

civibot bot commented Oct 16, 2021

(Standard links)

@demeritcowboy
Copy link
Contributor

Should I laugh or cry about the expectation that a civi component should have more bugs than it does.

On the topic of bugs though, the email I get for the receipt comes thru as the literal text Array(), but at first glance I don't think this PR touches receipts, so I'll check if that might be something else going on.

@@ -1,4 +1,4 @@
{assign var="greeting" value="{contact.email_greeting}"}{if $greeting}<p>{$greeting},</p>{/if}
{assign var="greeting" value="{contact.email_greeting_display}"}{if $greeting}<p>{$greeting},</p>{/if}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is preexisting but if updating anyway can you remove the <p></p> from this text version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

use Civi\Api4\Email;

/**
* Include dataProvider for tests
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy/paste probably.

@demeritcowboy
Copy link
Contributor

I think my problem with the receipt email is something else. So except for the <p></p> and a couple minor notes this seems ok.

@eileenmcnaughton
Copy link
Contributor Author

Thanks @demeritcowboy I've pushed this up - ideally as I go through templates I'd add classes to support them like in #21611 - but I didn't do that on this one since there isn't a pledge token class yet & I wasn't sure how hard it would be to get review (that one seems a bit stalled). But for the participant ones which also use this old pattern I might have a go

@demeritcowboy
Copy link
Contributor

Great, thanks. Github doesn't seem to have noticed the push? Maybe a temp glitch?

@eileenmcnaughton
Copy link
Contributor Author

@demeritcowboy fixed now - after re-checking the branch name...

@eileenmcnaughton
Copy link
Contributor Author

The test fails were

  1. a fail that related to the requested template fix
  2. 9 fails that appear related but are actually happening now without this PR merged

@eileenmcnaughton
Copy link
Contributor Author

I also put in the empty workflow message templates in that last force-push - https://github.com/civicrm/civicrm-core/compare/c8af61776e1b5dde005610f7ac1707135c223785..358171fba9d6dc164fc8153dbf98f30a95067e8b

Eventually we will add these for all templates and they will specify the entities to render tokens for. However, these 2 are really just placeholders. I'm still on the fence about going that bit further & adding a pledge token class & migrating the pledge tokens to it - but suspect I'd be better to put the effort into the more used messages

@eileenmcnaughton
Copy link
Contributor Author

failures are now fixed in master - test this please

@demeritcowboy
Copy link
Contributor

I've merged but I don't think {domain.phone} is correct. I missed it the first time since it's blank in the demo data. I'll make a lab ticket.

@demeritcowboy
Copy link
Contributor

Clearing cache fixed the phone. I guess domain info is cached even after saving the org info form? Anyway it seems ok.

@eileenmcnaughton eileenmcnaughton deleted the pledge2 branch October 23, 2021 22:41
@eileenmcnaughton
Copy link
Contributor Author

@demeritcowboy I doubt that value changes much in 'real' use so probably not a biggie if the cache is a bit aggressive

@demeritcowboy
Copy link
Contributor

Ooh that sounds pretty smart!

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.

2 participants