CRM-20184 Missing email_greeting in Workflow Templates

[magnolia61]

Overview

The token {contact.email_greeting} was introduced to give site admins control over the type and formality of the greeting. This token was only introduced in a few system workflow templates. This PR adds it to the others. Also it adds the display name to the email subject to be clearer

Before

Even though {contact.email_greeting} was introduced the workflow templates did not work as expected.

After

All (actually all but some) of the templates now have the {contact.email_greeting} for both the html and the text version. Also the display name is added to the subject to clearer discern between mails.

Technical Details

templates were changes in xml/templates/message_templates/ and I added them to the upgrade script.

Comments

---

• CRM-20184: Some System Workflow templates still miss {contact.email_greeting}

[magnolia61]

Hi @twomice & @sunilpawar, I saw you have been involved in issues regarding greeting tokens for civicrm in the past, would any one of you like to review my PR to make the system workflow messages consistent in the use of the {contact.email_greeting} token? The PR provides an upgrade script. Thanks!

[twomice]

Jenkins test this please.

[twomice]

Thanks @magnolia61 for the PR.

1. In general I like the idea of using email_greeting instead of display_name in emailed message templates.
2. Adding display_name to the subject line should probably be discussed separately on its own merits. It may not be a slam-dunk "yes" from all concerned parties; either way, I recommend putting it in a separate issue and separate PR. (That will also reduce the sheer size of this PR, which is daunting.)
3. As far as I can tell, you're taking the right approach to changing templates in the upgrade scripts, but it's pretty hard (not your fault) to see what's changed, since the diffs are just "+" lines on new files. Not sure what to do about that, but also not sure I'll have time to work out the differences.
4. Automated tests failed somewhere. You'll need to look at those results and fix whatever is failing. The test results are no longer available (I think it's because the PR is 14 days old now), so I've issued a request (my previous comment to Jenkins) to start those tests again. When the tests are done, if there's a failure, you can click the "Details" link to find what failed and clues about why.

[jaapjansma]

This PR looks good to me. I agree with @twomice that the display name should be a separate PR ideally. But as it is already here I would rather review it is as mergeable because it is not a big deal.
@magnolia61 I do think you have to make sure the automated tests will pass. I do think that will include change the test as the tests which are failing are looking up whether a certain text exists in the email send out. (And it doesnt as you have changed it).

[magnolia61]

@jaapjansma Would that be as easy as replacing 'Dear Mr. Anthony Anderson II' with 'Dear' in:

$mut->checkAllMailLog(array(
'Membership Type: General',
'Mr. Anthony Anderson II" [email protected]',
'Amount: $ 200.00',
'Membership Start Date:',
'Supporter Profile',
'First Name: Anthony',
'Last Name: Anderson',
'Email Address: [email protected]',
'This membership will be automatically renewed every',
'Dear Mr. Anthony Anderson II',
'Thanks for your auto renew membership sign-up',
));

[jaapjansma]

I am not sure whether that is enough it depends on what the test exactly is testing. Maybe @eileenmcnaughton can help with this?

[eileenmcnaughton]

@magnolia61 It does seem like replacing 'Dear Mr. Anthony Anderson II', with the greeting value makes sense - although hopefully it's a 'real' value like Dear Anthony not just Dear - might need to ensure that in the test set up

[magnolia61]

Anyone? Is the failing test related? I cant figure out how it would be related

[jaapjansma]

I dont think it is related.

[eileenmcnaughton]

No - I think not

[eileenmcnaughton]

I don't feel like I can review a PR with so many changes - it might need to be broken up - if you want to put a PR for a single template I can take a look at that (that might help find any issues too)

[magnolia61]

ehm. I'll try. BTW to upgrade system message templates, are the files in xml/templates/message_templates necessary or are they generated automatically. I included them in this PR.

[eileenmcnaughton]

@magnolia61 I'm not that sure - perhaps @monishdeb @jitendrapurohit
or @yashodha can advise as they have done this sort of update before

[magnolia61]

@eileenmcnaughton Shall I change this PR temporarily to be only for 1 template, or create a new one?

[eileenmcnaughton]

@magnolia61 using this PR keeps the comments for us which is nice - start with the template you care most about!

[magnolia61]

@eileenmcnaughton I narrowed it down to 3 templates for review, and will add the other templates after initial review.

[homotechsual]

I have concerns with this specific change: CRM/Upgrade/5.1.beta1.msg_template/message_templates/contribution_online_receipt_subject.tpl hardcoding the contact name/title into an email subject would be restrictive and would force CiviCRM users/admins to "unhardcode" this in order to achieve the current behaviour.

[eileenmcnaughton]

I'm closing this in favour of #12296 (which excludes the email subjects change & uses new upgrade logic)