Create contribution before taking payment, per contribution page workflow

[eileenmcnaughton]

Overview

Also exploring this https://github.com/civicrm/civicrm-core/pull/13763/files - diff between this & @mattwire is that I figure removing the $pending parameter from CRM_Event_Form_Registration_Confirm::processContribution affects 2 forms so I thought I'd try limiting the scope by just passing it as TRUE from one form

Before

No record of contribution if payment processor fails.

After

Contribution is created before payment processor is called, so there is always a contribution record even if payment fails. Contribution ID is available to the payment processor and we can update it with our own parameters, rather than passing them back to the event handling code which subsequently saves them on the contribution.

Technical Details

If the PR introduces noteworthy technical changes, please describe them here. Provide code snippets if necessary

Comments

Anything else you would like the reviewer to note

[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]

@mattwire this seems 'simple' but I guess we'll need to think about manual & unit testing - I guess the riskier areas are around waitlists, mutliple participants & also what emails go out

[eileenmcnaughton]

test this please

[eileenmcnaughton]

@jitendrapurohit @monishdeb @pradpnayak @seamuslee001 can I ask you to try testing this. It's a simple & important change (switches events to the create pending & then confirm flow) but the events code is pretty nasty. I'd like to merge it shortly after 5.13 is cut

[eileenmcnaughton]

@mattwire what is your thinking? Wait til the rc is cut & merge so it gets the longest rc period?

[mattwire]

@eileenmcnaughton So the RC will be 5.13? which will be ESR? in which case let's leave it until the RC is cut as I think we are good with this change, but it does have more potential for regressions than some and would be better not in the ESR.

[eileenmcnaughton]

@mattwire I don't think avoiding putting higher risk changes in the ESR makes sense. Our best stability comes from when many people upgrade so it could be an argument in FAVOUR of the ESR.
That would be fairer in a way since I think ESR is not fully costing the direct costs so we shouldn't also be protecting them from the indirect costs.

However, the goal of waiting is that we would have a longer period between merge & release

[mattwire]

Merging now so we are in the next RC (5.14)

[eileenmcnaughton]

thanks @mattwire I guess it would be good to look at extending tests on events now