WIP Refactor cancel/failed functions in baseIPN so we don't duplicate code and business logic is separated out

[mattwire]

Overview

In https://lab.civicrm.org/dev/core/issues/927 we identify some undesirable "business logic" related to cancelling of memberships if a contribution is cancelled. This is a refactor of the BaseIPN class to remove duplicate code and separate out the functions which cancel memberships / participants when a contribution is marked cancelled / failed. This does not "fix" 927 but gets us one step closer to having the code responsible in one place!

Next steps could be to share the cancelMembership()/cancelParticipant() functions with CRM_Contribute_BAO_Contribution::cancel()

Before

BaseIPN has almost identical code duplicated for cancelled() and failed() actions.

After

BaseIPN has shared code for cancelled() and failed() actions. Participant and Membership related actions are moved into their own functions.

Technical Details

There should be no change to behaviour here.

Comments

@sluc23 Could anyone from your team review this? @eileenmcnaughton More contribution actions refactoring here... :-)

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

[magnolia61]

Does this PR effect the same logic for failed/cancelled event contributions?

[mattwire]

@magnolia61 No changes to existing logic which does mark participants as cancelled if contribution is cancelled/failed (when triggered via the IPN callback) https://github.com/civicrm/civicrm-core/pull/14397/files#diff-e45a4598f8d66f6a82d08e8d4d22ed81R325

[sluc23]

@mattwire looks good for me. I'm not an IPN expert, but i agree it's the correct path to start organizing better the Contribution code mess 👍

[eileenmcnaughton]

@mattwire so there are different ways to de-fang toxic code.

I would generally advocate for splitting out small functions & a series of refactors smaller than this, in conjunction with unit tests in the first instance.

However, this is tricky because it does a bunch of stuff that seems odd & wrong & paypal specific even though it doesn't live on the paypal class and yet there is a remote possibility other processors use some of it

There are crazy things like

    //add lineitems for recurring payments
    if (!empty($objects['contributionRecur']) && $objects['contributionRecur']->id && $addLineItems) {
      CRM_Contribute_BAO_ContributionRecur::addRecurLineItems($objects['contributionRecur']->id, $contribution);
    }

    //add new soft credit against current $contribution and
    //copy initial contribution custom fields for recurring contributions
    if (!empty($objects['contributionRecur']) && $objects['contributionRecur']->id) {
      CRM_Contribute_BAO_ContributionRecur::addrecurSoftCredit($objects['contributionRecur']->id, $contribution->id);
      CRM_Contribute_BAO_ContributionRecur::copyCustomValues($objects['contributionRecur']->id, $contribution->id);
    }

in there. Which don't make any sense in the context of a a failing contribution and certainly not in the context of our preferred recurring flow being to create a pending contribution using repeattransaction (status = pending) and then update the contribution depending on the result.

My instinct is to

• ring fence the rot in paypal ipn - copy the stuff it uses back into paypal & leave the rest in BaseIPN but throw out deprecation notices if people actually use it and say 'this is is officially old huckery paypal code'
• Write some alternative api functions that we think are actually sensible - ie. Contribution.cancel, Contribution.fail and tell people to use a recommended flow - possibly in new functions on the baseIPN class but leave the ones that exist.

Unless Paypal IPN is the actual focus of what you are trying to achieve here I think messing with it introduces a lot of risk for code that is of very little quality & value in this case.

[mattwire]

@eileenmcnaughton Perhaps this looks more complex that it actually is? All we're actually doing here is merging two identical functions - the only difference is the log message at the start and end. And we extract the two bits of business logic (cancelParticipant() and cancelMembership()). Those are the bits that are important for https://lab.civicrm.org/dev/core/issues/927 and by extracting those elsewhere too we can move them to a single place (eg. the relevant BAO objects) where we can consider how to deactivate them for different use-cases.

I'm planning to do lot's more cleanup as part of the Stripe MIH and implementing Contribution.cancel / Contribution/fail APIs could well be part of that. But we can't deprecate existing code until we have an alternative. And we don't really want to add new code until we understand what it needs to do.

There is also some existing test coverage here: testThatCancellingEventPaymentWillCancelAllAdditionalPendingParticipantsAndCreateCancellationActivities() and testThatFailedEventPaymentWillCancelAllAdditionalPendingParticipantsAndCreateCancellationActivities()

CRM_Contribute_BAO_Contribution::cancel() contains pretty much the same as the two new functions here and the next step would be to extract those two and wrap them all in tests.

[eileenmcnaughton]

This is stale & I'm pretty sure it's because I merrged a recut of this - closing