dev/core#253: Cancelling or An Error during event registration payment should cancel all additional participates

[omarabuhussein]

Problem

If a user is registering for an event with additional participates, and for some reason the payment failed or the user decided to cancel the payment, then only the main user participant record will be set to "cancelled", the rest of additional participants will stay at "pending incomplete transaction" status.

Solution

All participants statuses should be changed to cancelled and not just the main user, Also there should be an activity created for each participants that indicate the event registration is cancelled similar to what happen if the payment succeeded.

Technical notes

The cancelled() and failed() methods of CRM_Core_Payment_BaseIPN class use an instance of the Participant DAO class to change the status to cancelled which results in only changing the first participant :

https://github.com/civicrm/civicrm-core/blob/master/CRM/Core/Payment/BaseIPN.php#L377
https://github.com/civicrm/civicrm-core/blob/master/CRM/Core/Payment/BaseIPN.php#L280

which should be replaced with either a call to Participant BAO create method, or the Participant API to fix the issue.

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[eileenmcnaughton]

I'm not sure about the meta-issue here (ie. what is right from a user POV - others may have opinions.) Cancelling a PAYMENT doesn't seem to me to necessarily cancel the underlying order

From the code POV - we should move the handling out of the IPN class & consider whether to have a Contribution.cancel api - or whether calling Contribution.create with status of Cancelled should update related entities as described (& we can test this api of course).

@JoeMurray ping

[eileenmcnaughton]

is nothing lost by this removal?

[omarabuhussein]

nope, just the commented out code is removed. but you are welcome to double check :)

[omarabuhussein]

Cancelling a PAYMENT doesn't seem to me to necessarily cancel the underlying order

But the current code already cancel the main participant, so it does make sense to cancel the additional ones which is what this PR do, but if you mean that the even the cancellation for the main participant should not also happen then that's another matter.

From the code POV - we should move the handling out of the IPN class

I already thought of that while working on it, but I was bit busy and doing such change require some time and thinking about a proper design for this and where should this logic be moved, so I decided to stick on the minimal change possible.

& consider whether to have a Contribution.cancel api - or whether calling Contribution.create with status of Cancelled should update related entities as described

hmmm maybe, I'll have to think about both options.

[eileenmcnaughton]

I have been mulling this & I think a Contribution.cancel (& possibly a Contribution.fail) api make sense - mostly because I'm a bit scared to change the existing behaviour of Contribution.create at this stage in the api series.

@JoeMurray thoughts?

[mattwire]

@omarabuhussein I think your fix is correct but I agree with @eileenmcnaughton that we need to push for a little bit of code improvement at the same time. I see that baseIPN->cancel and baseIPN->fail are called in a number of non IPN places.

1. Deprecating and moving those two functions to CRM_Contribute_BAO_Contribution_Utils or similar seems like a good first step.
2. Adding wrappers for Contribution.cancel and Contribution.fail would be a nice second step.

[omarabuhussein]

Thanks both, I have a lot to follow with these days but will try to get to work on this as soon as possible.

[eileenmcnaughton]

Just circling back on this - I agree that calling the api is better than by-passing hooks etc and that this is a fairly small change with additional test coverage. Obviously the additional steps haven't happened but I think the presence of unit tests outweighs that concern at this stage