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

Catch Payment Processor Exception if thrown when registering via bac… #14930

Conversation

seamuslee001
Copy link
Contributor

…k office participant registration

Overview

This catches any Exception thrown by the Payment Processor as it doesn't appear to be caught anywhere else and then sets a standard style status message

Before

Payment Processor Exception not caught

After

Payment Processor Exception caught

ping @eileenmcnaughton

@civibot
Copy link

civibot bot commented Jul 30, 2019

(Standard links)

@civibot civibot bot added the master label Jul 30, 2019
@seamuslee001 seamuslee001 force-pushed the backoffice_participant_payment_processor_error branch 2 times, most recently from e6651bd to 91b84a8 Compare July 30, 2019 22:06
@mattwire
Copy link
Contributor

@seamuslee001 There seems to be quite a bit of inconsistency in how we handle the call to doPayment() - I do agree with catching the exception, but looking at other points in the code I think the right thing to do is call CRM_Contribute_BAO_Contribution::failPayment() and then call CRM_Core_Session::singleton()->setStatus($e->getMessage()); (ie without adding any extra text).

@seamuslee001
Copy link
Contributor Author

@mattwire i'm not sure if we have created a contribution at this point in this code so not sure if we can call failPayment() but will update on the other part then.

@eileenmcnaughton
Copy link
Contributor

We should have created a contribution & I'm pretty sure on this form we have - the front end event form is still a gap

@seamuslee001
Copy link
Contributor Author

@eileenmcnaughton @mattwire having done a bit of debugging i think the contribution is actually getting created at L1292 after we have done the payment when we call https://github.com/civicrm/civicrm-core/blob/master/CRM/Event/Form/Registration/Confirm.php#L960

@eileenmcnaughton
Copy link
Contributor

ug - I've started digging into this event stuff due to the paypal checkout flow issue - but so far have only put up minor cleanups like #14917

@mattwire
Copy link
Contributor

:-( @seamuslee001 Ok, can you add the failPayment in commented if it definitely does not with a link in the comments to https://lab.civicrm.org/dev/financial/issues/53 or if it makes sense wrap failPayment in an if (!empty($contributionID)) { failPayment } with the same comment? At least we can work towards consistency - even if we can't quite get there yet!

… office participant registration

Make changes as per Matt's and Eileen's comments
@seamuslee001 seamuslee001 force-pushed the backoffice_participant_payment_processor_error branch from 91b84a8 to 46366f1 Compare July 31, 2019 22:08
@seamuslee001
Copy link
Contributor Author

@mattwire @eileenmcnaughton ok added comments and made changes now

@eileenmcnaughton
Copy link
Contributor

OK - I wrote a unit test (will push up separately) & stepped through this in it & agree it makes sense as a change. I'd rather see us doing the redirect in the postProcess rather than submit & fix the order of events - but this does fix a problem where the code intended to be called on a failed payment isn't

@eileenmcnaughton eileenmcnaughton merged commit c3be407 into civicrm:master Aug 1, 2019
@eileenmcnaughton eileenmcnaughton deleted the backoffice_participant_payment_processor_error branch August 1, 2019 22:29
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Aug 1, 2019
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Aug 2, 2019
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Aug 2, 2019
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Aug 2, 2019
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.

3 participants