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

CRM-21424 Set receipt date when downloading pdf receipt #11289

Merged
merged 3 commits into from
Nov 18, 2017
Merged

CRM-21424 Set receipt date when downloading pdf receipt #11289

merged 3 commits into from
Nov 18, 2017

Conversation

kainuk
Copy link
Contributor

@kainuk kainuk commented Nov 16, 2017

Overview

Set receipt date when downloading pdf receipt.

Before

Does not set de receipt date when downloading a receipt pdf.

After

Does set the receipt date when downloading the receipt pdf.

Technical Details

https://issues.civicrm.org/jira/browse/CRM-21424 contains an example to reproduce

Comments

issue was introduced with the fix for CRM-18166.


@eileenmcnaughton
Copy link
Contributor

test this please

@eileenmcnaughton eileenmcnaughton added the merge ready PR will be merged after a few days if there are no objections label Nov 16, 2017
@eileenmcnaughton
Copy link
Contributor

This makes sense to me - if there is an input option we should respect it & the parameter does not seem like it would be set other than deliberately

@@ -4722,7 +4722,7 @@ public static function sendMail(&$input, &$ids, $contributionID, &$values,
$values['contribution_status'] = CRM_Core_PseudoConstant::getLabel('CRM_Contribute_BAO_Contribution', 'contribution_status_id', $contribution->contribution_status_id);
$return = $contribution->composeMessageArray($input, $ids, $values, $returnMessageText);
// Contribution ID should really always be set. But ?
Copy link
Member

Choose a reason for hiding this comment

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

This comment doesn't make sense to me. Maybe it's no longer relevant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment is obsolete - on line 4714 the existence of the contribution is tested, and this test fails when the id is not set.

@JoeMurray
Copy link
Contributor

The issue is worthy.

  1. I would want to ensure that re-downloading does not update receipt date field.

  2. Can we get a unit test to lock in fixed behaviour?

Thanks for this good work.

@colemanw colemanw removed the merge ready PR will be merged after a few days if there are no objections label Nov 17, 2017
@kainuk
Copy link
Contributor Author

kainuk commented Nov 17, 2017

@JoeMurray In the download screen the user has the option to prevent the update of the receipt field (stand behaviour is an update, the user can disable it). I think that is correct because only the user knows of the first downloaded pdf really was sent out).
crm-21424

@mlutfy
Copy link
Member

mlutfy commented Nov 18, 2017

jenkins, test this please

@eileenmcnaughton
Copy link
Contributor

Looks like issues have been addressed & the change looks good

@eileenmcnaughton eileenmcnaughton merged commit ee65bcd into civicrm:master Nov 18, 2017
sluc23 pushed a commit to ixiam/civicrm-core that referenced this pull request Jan 10, 2018
CRM-21424 Set receipt date when downloading pdf receipt
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.

6 participants