-
-
Notifications
You must be signed in to change notification settings - Fork 825
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
Update Payment Notification to use greeting, remove text to 'Please print this confirmation for your records. #13655
Conversation
(Standard links)
|
@@ -160,7 +160,7 @@ | |||
</td> | |||
</tr> | |||
{/if} | |||
{if $contributeMode eq'direct' and !$isAmountzero} | |||
{if $credit_card_number} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we would never have a card number here if it's not been used for a payment so we don't need contributeMode or to check the amount
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree
@@ -147,7 +147,7 @@ | |||
<tr> | |||
<td> | |||
<table style="border: 1px solid #999; margin: 1em 0em 1em; border-collapse: collapse; width:100%;"> | |||
{if $contributeMode eq 'direct' and !$isAmountzero} | |||
{if $billingName || $address} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are assigned in the processCreditCard code. I would have liked to load them for the db but we don't retain an address per payment - only per contribution - probably we should
These changes are consistent with other changes. I also switched the parameters for when to include extra detail to deprecate contributeMode
46ce020
to
1e477c5
Compare
@@ -169,6 +169,8 @@ protected static function loadRelatedEntities($id) { | |||
$contactID = self::getPaymentContactID($contributionID); | |||
list($displayName, $email) = CRM_Contact_BAO_Contact_Location::getEmailDetails($contactID); | |||
$entities['contact'] = ['id' => $contactID, 'display_name' => $displayName, 'email' => $email]; | |||
$contact = civicrm_api3('Contact', 'getsingle', ['id' => $contactID, 'return' => 'email_greeting']); | |||
$entities['contact']['email_greeting'] = $contact['email_greeting_display']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems $contact is not used anywhere below so can we change it to:
$entities['contact']['email_greeting'] = civicrm_api3('Contact', 'getvalue', ['id' => $contactID, 'return' => 'email_greeting_display']);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@monishdeb nope we can't - if it's empty getvalue will throw an error
// These 2 rows are temporarily added for sequencing of adding commits. They won't be needed when we | ||
// switch to Payment.send_confirmation api | ||
$contact = civicrm_api3('Contact', 'getsingle', ['id' => $this->_contactId, 'return' => 'email_greeting']); | ||
$this->assign('emailGreeting', $contact['email_greeting_display']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as below!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these 2 lines will go next pr
@@ -12,7 +12,8 @@ | |||
{capture assign=emptyBlockStyle }style="padding: 10px; border-bottom: 1px solid #999;background-color: #f7f7f7;"{/capture} | |||
{capture assign=emptyBlockValueStyle }style="padding: 10px; border-bottom: 1px solid #999;"{/capture} | |||
|
|||
<p>Dear {$contactDisplayName}</p> | |||
{if $emailGreeting}<p>{$emailGreeting},</p>{/if} | |||
|
|||
<center> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree
Tested working fine. Also has an extended UT to assert billing address and card details |
@magnolia61 indeed! having already set the upgrade script on this it's good to tweak that at the same time - are you able to to test the revisted tpl in #13669 |
When I upgrade my dev site to the current master a fatal error get's thrown on the |
@magnolia61 is there any more info? That update script hasn't changed so it's a bit weird |
This is from the CiviCRM log: [debug_info] |
Solved it! For some reason a former upgrade did not work well and left me with a double entry in the civicrm_option_value table for the payment_or_refund_notification template. When I removed the redundant ones, the upgrade worked like a charm. |
thanks - & phew |
Overview
Update Payment notification to use the value stored in email_greeting rather than display name (this has been done in other templates).
Also remove text to 'Please print this confirmation for your records.'
Before
Addressed by display name - eg. 'Dear Mr. Anthony Anderson II,'
Sentence Please print this confirmation for your records is present
After
Addressed by email greeting per site config & like other templates eg. 'Dear Anthony,'
Sentence Please print this confirmation for your records has gone
Technical Details
These changes are consistent with other changes. I also switched the parameters for when to include extra detail to deprecate contributeMode
Comments
Resolving inconsistency on greeting as pre-requisite for #13649
@magnolia61 I think this is probably one you'll be please to see