dev/membership#13 Add handling for missing MembershipPayment record.

[eileenmcnaughton]

Overview

We have an intermittent regression on extending memberships off paypal payments. This adds debug messages and handling / tests for the scenario I suspect may be in play.

In short it used to be that the paypal request membershipID would override a lack of identifiable membership ids later on. This causes the paypal id to have that dominance again

Before

If the code could not determine the correct membership none would be extended.

After

The paypal request sets the membership id to be extended

Technical Details

The 'decision' the code makes about how to create recurring transactions is all dependent on the 'template transaction'

• if that is not correct the follow on payments will not be.

This PR allows the membershipID input from paypal to be passed through and used. This will override any
intended changes - but that just restores previous behaviour and we do not have good handling for these

Comments

@MegaphoneJon @Stoob can you test

https://lab.civicrm.org/dev/membership/issues/13

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

This whole section only adds debug

[eileenmcnaughton]

This is where the change is - it allows an input membership id to have this effect

[mattwire]

Also, if we are going to add membership specific stuff into repeattransaction can we abstract it to it's own function as we know we have to do more refactoring/abstraction in this area eg. #14397

[mattwire]

@eileenmcnaughton I'm not completely comfortable with this - it feels like we may be adding complexity to repeattransaction to handle a PayPalIPN specific issue? If the fundamental issue is that the membership ID record does not have the recurring_contribution_id recorded then can't we fix that instead and allow the code to continue behaving as before?

[eileenmcnaughton]

@mattwire repeattransaction should handle creating membership payment records. The ideal flow is that repeattransaction is called with contribution_status_id =2 (pending) and it sets up all the records correctly and then Payment.create is called to add the payment - which in turn calls completetransaction if it completes the transaction.

For data reasons unknown membership payment records are not being set up in some cases & it seems reasonable that the calling function CAN specify which membership the repeated transaction can be linked to.

In paypal's case we DO have some information we can pass in to specify it. We don't know if this bug only affects paypal

[mattwire]

repeattransaction should handle creating membership payment records. The ideal flow is that repeattransaction is called with contribution_status_id =2 (pending) and it sets up all the records correctly and then Payment.create is called to add the payment - which in turn calls completetransaction if it completes the transaction.

@eileenmcnaughton The main issue I have is that creating a membershippayment doesn't feel like something that repeattransaction should need to know anything about - it just happens in this case that the "bug" shows when going through that route. But what we actually want to do is say "Hey, we're creating a contribution and it's for this Membership/Participant" - so how about putting the MembershipPayment (and ParticipantPayment if we're passed a participant ID) into Contribution.create API instead? And then just passing the parameter through in repeattransaction?

[eileenmcnaughton]

@mattwire My understanding of repeattransaction is that the goal of it IS to create a contribution with all it's 'assets' - based off a template.

It does do it via Contribution.create in some cases (in my view that is a bad thing & contribution create should only manage the contribution - wrappers like Order.create & repeattransaction should be the only things to create related entities) - but I forgive the 'how' if it's tested

[mattwire]

It does do it via Contribution.create in some cases (in my view that is a bad thing & contribution create should only manage the contribution - wrappers like Order.create & repeattransaction should be the only things to create related entities) - but I forgive the 'how' if it's tested

@eileenmcnaughton Makes sense. So how about creating a separate function (eg. createRelatedEntities()) that is called from repeatTransaction where you've added the MembershipPayment line? That function should eventually be used to create all related entities based on the params passed from repeattransaction (ie. ParticipantPayment, MembershipPayment, anything else..) but could start with just MembershipPayment to fix the current issue. Then that function could later be called by other wrappers such as Order.create.

As an aside, it's an absolute nightmare working with multiple entities via the API - have you ever tried creating a participant record which has an associated contribution via the API :-( So at least this may help a bit with the membership side.

[eileenmcnaughton]

Hmm -so the api for creating multiple entities via the api is Order.create. I'm not sold on the function here because I think it implies things that we still need to work through - but I'll amp up the code comments. Note this still needs at least some testing by @MegaphoneJon or @Stoob

[eileenmcnaughton]

@mattwire I've done a heavy update on the comments

[mattwire]

@eileenmcnaughton Happy with this now. If we can get someone to review/test then it can be merged

[eileenmcnaughton]

@mattwire thanks we've been struggling to get a tester :-( @MegaphoneJon this should be pretty narrow & safe & have lots of debug if you ARE able to try it

[aydun]

@mattwire I've done a heavy update on the comments

@eileenmcnaughton Big yay for instructive comments! :-)

[eileenmcnaughton]

@aydun - Matt does prod me at times...

[MegaphoneJon]

Hi @eileenmcnaughton - unfortunately this doesn't resolve the bug. Here's the output of my log file when I replay an IPN using your NotificationLog.retry API:

Sep 03 22:08:43  [info] $GET = Array
(
)


Sep 03 22:08:43  [info] $POST = Array
(
)


Sep 03 22:08:43  [warning] Unreliable method used to get payment_processor_id for PayPal Pro IPN - this will cause problems if you have more than one instance                                                                               

Sep 03 22:08:43  [info] Contribution record updated successfully

Sep 03 22:08:43  [info] Success: Database updated

[eileenmcnaughton]

@MegaphoneJon I was on the fence about how well it would help with the bug but I expected more debug info! TBH I'm not quite sure where that info would wind up (from civi::log()->debug) but maybe the system_log table?

[MegaphoneJon]

Nothing in civicrm_system_log I'm afraid. I thought Civi::log()->debug was similar to CRM_Core_Error::debug_var() but I haven't switched over yet so I'm not sure.

[eileenmcnaughton]

Hmm - as long as membershipID is passed in this should be hit

   Civi::log()->debug('PayPalIPN: Received payment for membership ' . (int) $membershipID
        . '. Original contribution was ' . (int) $contributionID . '. The template for this contribution is '
        . $templateContribution['id'] . ' it is linked to ' . $membershipPayment['count']
        . 'payments for this membership. It has ' . $lineItems['count'] . ' line items linked to  this membership.'
        . '  it is  expected the original contribution will be linked by both entities to the membership.'
      );

[seamuslee001]

Added merge on pass following discussion in product maintenance. We feel this is conservative enough and better in than not but doing it before the next RC so we have another month to test it out

[eileenmcnaughton]

Fail unrelated - also present on other tests today

[mattwire]

@eileenmcnaughton This has broken Paypal standard for non-recurring payments (which are confirmed via IPN) as we are requiring a recurring contribution ID which does not exist.

[mattwire]

A quick patch for this that fixed it on the site where I found this:
In CRM/Core/Payment/PayPalIPN.php:

      if (!empty($contributionRecurID)) {
        $templateContribution = CRM_Contribute_BAO_ContributionRecur::getTemplateContribution($contributionRecurID);
      }
      else {
        $templateContribution['id'] = $contributionID;
      }