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-21409 Don't bypass hooks when updating contribution receipt/thankyou date #11257

Conversation

mattwire
Copy link
Contributor

@mattwire mattwire commented Nov 8, 2017

Overview

Use the API instead of directly accessing DAO object. This fixes a "todo" in the code and means that the entity changes will trigger any necessary hooks instead of bypassing them.


@seamuslee001
Copy link
Contributor

Jenkins re test this please

@mlutfy
Copy link
Member

mlutfy commented Nov 15, 2017

jenkins, test this please

capture d ecran de 2017-11-15 16-15-13

(seems like an unrelated fail)

@mlutfy
Copy link
Member

mlutfy commented Nov 16, 2017

jenkins, test this please

@mattwire
Copy link
Contributor Author

Jenkins test this please

@mattwire
Copy link
Contributor Author

@bgm @seamuslee001 Woo! Finally passing, did not see how that could possibly be related to this PR!

}
if ($receipt_update || $thankyou_update) {
$result = civicrm_api3('Contribution', 'create', $contributionParams);
if (empty($result['is_error'])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This if is invalid - there will be an exception thrown rather than an error returned. There are no obvious reasons why an exception would be thrown so I think it's OK not to add any handling for the possibility.

Also - I'm not 100% sure about the ternarys below. I would make it
$receipts = ($receipt_update ? $receipts + 1 : $receipts);

@eileenmcnaughton
Copy link
Contributor

I made some minor comments but overall this change makes sense to me

@mattwire mattwire force-pushed the CRM-21409_contribute_pdf_letter_hooks branch from 5805be4 to fd57820 Compare November 22, 2017 18:27
@mattwire
Copy link
Contributor Author

@eileenmcnaughton Thanks for reviewing. I've updated based on your comments.

@eileenmcnaughton
Copy link
Contributor

test this please

@eileenmcnaughton
Copy link
Contributor

I think this is good to merge now. Re-running tests

@seamuslee001
Copy link
Contributor

@eileenmcnaughton this has passed testing now

@eileenmcnaughton
Copy link
Contributor

Yay

@eileenmcnaughton eileenmcnaughton merged commit f045767 into civicrm:master Nov 23, 2017
sluc23 pushed a commit to ixiam/civicrm-core that referenced this pull request Jan 10, 2018
…df_letter_hooks

CRM-21409 Don't bypass hooks when updating contribution receipt/thankyou date
@mattwire mattwire deleted the CRM-21409_contribute_pdf_letter_hooks branch January 17, 2018 13:12
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.

5 participants