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#1255 - fix display of email address on pay later contribution #15314

Merged
merged 2 commits into from
Jan 14, 2020

Conversation

jitendrapurohit
Copy link
Contributor

Overview

Fix for email address token on the confirmation message not working for a pay later contribution

Before

Email address is not displayed on the Thankyou screen. To replicate -

  • Add a profile with an email(email-primary) field.
  • Include this profile on a contribution page.
  • Submit the page with pay later and notice the below confirmation message on the thankyou page-

image

The second line ends with "has been sent to .". This should be displaying an email address of the user.

After

The confirmation message displays an email address correctly.

image

Comments

Gitlab link - https://lab.civicrm.org/dev/core/issues/1255

@civibot
Copy link

civibot bot commented Sep 16, 2019

(Standard links)

@yashodha
Copy link
Contributor

Jenkins test this please

Comment on lines 577 to 603
if ($this->_emailExists && empty($this->_params["email-{$this->_bltID}"])) {
foreach ($this->_params as $key => $val) {
if (substr($key, 0, 6) == 'email-') {
$this->assign('email', $this->_params[$key]);
}
}
}
else {
$this->assign('email', CRM_Utils_Array::value("email-{$this->_bltID}", $this->_params));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@jitendrapurohit Can you extract this bit of code into it's own function and provide a comment explaining why we have to do it this way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@yashodha
Copy link
Contributor

@jitendrapurohit You might want to move the code in its own method as suggested by Matt.

@yashodha
Copy link
Contributor

yashodha commented Nov 6, 2019

@jitendrapurohit Will you able to extract this code as a separate method and we can merge this bug fix :)

@jitendrapurohit
Copy link
Contributor Author

I've updated the PR. Thanks @yashodha @mattwire

@jitendrapurohit
Copy link
Contributor Author

Has anyone tested this @mattwire @yashodha @eileenmcnaughton? If yes, can we merge?

@demeritcowboy
Copy link
Contributor

FYI it looks like someone on stackexchange tested it and said it worked: https://civicrm.stackexchange.com/questions/34284/an-email-confirmation-with-these-payment-instructions-has-been-sent-to

@seamuslee001
Copy link
Contributor

Looks good merging as per the positive feedback on stack exchange

@seamuslee001 seamuslee001 merged commit 3454932 into civicrm:master Jan 14, 2020
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.

5 participants