[REF] Add test for existing Participant batch update cancel and fix to not call BaseIPN->cancelled

[eileenmcnaughton]

Overview

Minor cleanup to avoid complicated calling a complicated function when a simple one will do

Before

$baseIPN->cancelled($objects, $transaction, $input);

After

civicrm_api3('Contribution', 'create', ['id' => $contributionId, 'contribution_status_id' => 'Cancelled']);

Technical Details

I dug into what is 'achieved' by calling BaseIPN->cancelled here and everything except the bit where the contribution
is updated to cancelled is actually bypassed so a simple api call suffices.

I also discovered the cancellation of the contribution is highly conditional and arguably illogical. I will separately
log a gitlab to discuss whether it still makes sense, but I wouldn't want that discussion to derail this
no-change cleanup

Comments

This cleans up one of the 'outer edges' of the BaseIPN flow. Cleaning the actual functions has been a bit blocked by them being called from more places than really makes sense.

Note there is quite a bit more follow up could be done

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

[mattwire]

@eileenmcnaughton A good cleanup. Couple of questions:

• Was the original code doing anything with memberships (setting them to cancelled?) - does the call to Contribution.create still do that? The same question for participants.
• Contribution.create is slow and I'm always nervous calling it for that reason (I've seen numerous performance issues that can be traced back to it). For that reason I try to add as many of the "skip" parameters as I can when calling - eg. https://lab.civicrm.org/extensions/mjwshared/-/blob/master/CRM/Core/Payment/MJWTrait.php#L465-468

[eileenmcnaughton]

@mattwire this is probably one of those PRs that requires a second pair of eyes on the code rather than a github review but specifically to why I concluded that the membership & participant records are not updated in that function

1. this is an event update task & is only accessible from there - so memberships are not relevant. It actually only is called for events with a source 'Online Event Registration'
2. all handling for both participant and membership records was wrapped in the IF that I removed that ensured it was not done from this flow

On the performance - our standard/ recommended practice at this stage is definitely to call Contribution.create. It might be worth adding a gitlab to explore those performance issues. I'm actually going to spend some time over the next week tracking where we can improve our processing through reducing / improving queries so might have something on that over the next little bit.

Specifically on this form we don't expect it to be a high-usage or high-volume work flow. I could make a case for skipRecentView but I could also make the case against it (the choices the code makes about cancelling contributions are a bit arguable so more visibility is better).

TBH I'm a bit alarmed to see you using is_post_payment_create since we deliberately didn't advertise it as it's intended as an internal param. You know the workings of the code well enough to know what it does but let's not use it anywhere outside it's original purpose outside your code - because I don't want people thinking it's supported other than as an internal param which is part of our transition towards Payment.create

[mattwire]

Thanks for clarifying.

Re. contribution.create - I've found it adds around 1.5s per call in "normal" circumstances so generally avoid for any batch stuff (it was particularly an issue eg. with UK giftaid where I switched to using the customvalue API instead (as we were updating custom values).

Re. is_post_payment_create we don't currently have a simple way to update certain parameters (such as trxn_id) without triggering all the Contribution.create logic - that parameter bypasses the lineitems stuff.