Create contribution before taking payment, per contribution page workflow

[mattwire]

Overview

When registering for an event the payment processor is called before the contribution is created. This is inconsistent with the contribution page workflow which creates the contribution before calling the payment processor.

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

"Simple" refactor that removes $result parameter which is not required, swaps two blocks of code around and stops the event code trying to do things with contributions that should be handled by the payment processor (eg. saving of trxn_id).

Comments

Further refactor could be to extract some of this code into separate functions but this seems like a logical first step as it puts things into the right order and variables are used later in the function.

[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).

[JoeMurray]

NB: worth investigating what happens to bookkeeping transactions with this change. There is a default setting that causes contributions to always record a bookkeeping entry for revenue going into Accounts Receivable. When set to not do this, then this PR should probably cause the contribution status should be set to In progress (NOT Pending==pay later) and if a payment for the full amount is processed immediately and successfully would need to be changed to Completed. Partial payments would also need to be handled similarly.

[mattwire]

@JoeMurray Thanks for flagging some potential issues. I'm waiting for tests to run to see where we have test coverage of this but will need to investigate the areas you suggest.

[MegaphoneJon]

One other issue might be folks who have the fairly common CiviRules configuration wherein the trigger is "Contribution is created" and the condition is "Status is Complete". If this creates a contribution with a status that's NOT complete, then changes it on update, it might cause the rule to stop working.

[mattwire]

@MegaphoneJon How common do you think that configuration actually is? As we already create a pending contribution for the contribution page workflow and subsequently mark it as completed if it succeeds. This PR tries to change the event workflow to match

[MegaphoneJon]

@mattwire good question! I decided to do a bit of research on whether this was an issue at all. I got a bit stuck - I was CiviRules overwrites contribution params set in hook_post with database data for this scenario, but my read of the code suggests that this would NOT fire on new contributions from contribution pages, which I know it does.

@jaapjansma Does this code change pose a concern for you?

[jaapjansma]

The code you see does not alter any actual post data.
What it does? CiviRules has a Trigger Data object which holds data which could then be used within a condition and action.
The Trigger Data object gets the data from the post hook and with contributions the data within the trigger data is enriched with the actual data within the database.

So I dont see any concern here.

[mattwire]

Note that the membership flow also works this way and is a complete mess. This one (event) looks pretty straightforward to resolve though :-)

[eileenmcnaughton]

I think this is on the right track - the backoffice participant form ALSO calls processConfirm so I figured rather than remove pending handling we should just not permit it from this form as a first step - trying #13837 to see if it gets through tests - that seems like the minimum possible change to me

[mattwire]

Closing in favour of #13837