dev/financial#27 Paypal_Standard recurring IPNs don't work #12387

mattwire · 2018-07-01T20:17:33Z

Overview

Currently you can setup recurring payments using the Paypal Standard processor, but after the first IPN is processed subsequent IPNs do not get processed because the IPN code does not fully implement code to do so and fails to identify the correct payment processor in CiviCRM if the "business" email does not match.

By replacing the call to completetransaction with one to repeattransaction we have working recurring IPNs for paypal standard.

See: https://lab.civicrm.org/dev/financial/issues/27

"business" email does not necessarily match the "Merchant Account Email" defined for the payment processor. Currently this causes the IPN to silently fail as it cannot identify the payment processor. Fix: A new function getPayPalPaymentProcessorID() based on the one used by PayPalPro IPN is implemented (a future improvement could be to make this a shared function).
"receive_date" is being set to the date/time the IPN was processed instead of using the "payment_date" specified in the IPN response. Fix: We now use the "payment_date" if it is available.
Subsequent contributions not being created. On the client site I tested this on NONE of the subsequent transactions were being processed - I assumed this was because we are using CompleteTransaction function instead of calling the API Contribution.repeattransaction. It may, in part have been caused by the bad lookup for the payment processor (see above). However, on further testing and looking at the testIPNPaymentRecurSuccess() test, whilst I can see that the test "Passes" I can also see that it is not creating the subsequent contributions correctly (they have a different payment_instrument and the total_amount is different for both contributions because it is not using the value passed by the mock IPN response. Fix: I've updated this PR to include improvements to the unit test to make sure we capture those issues - it should pass when run with the other changes in this PR but fail if the other changes are not applied.

Before

Paypal Standard recurring IPNs do not work properly

After

Paypal Standard recurring IPNs work properly.

Technical Details

Switch to calling the internal function completetransaction to the API repeattransaction which is the "correct" thing to do.

Comments

This has been tested on a live site, and has been used with nz.co.fuzion.notificationlog to replay six months of IPNs that were received by CiviCRM but not acted on.

civibot · 2018-07-01T20:17:35Z

(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 · 2018-07-02T00:48:22Z

@mattwire you can rebase this now

davejenx · 2018-07-02T11:29:27Z

@mattwire Looks like this includes changes from #12386, presumably that's what Eileen refers to re rebasing. Will be easier to review then.

eileenmcnaughton · 2018-07-03T01:54:14Z

@mattwire I think your issue description needs some nuance - I don't think all IPNs are broken - and the tests in CRM_Core_Payment_PayPalIPNTest suggest some are tested as working - so I think there is some extra detail to the scenario - we should try to reproduce in the a test.

mattwire · 2018-07-06T16:37:17Z

@eileenmcnaughton @davejenx @pradpnayak Thanks for taking a look at this and commenting. I've taken a look at the unit test that @eileenmcnaughton found and identified some issues with it which are now included in this PR. Also, below is a much more detailed description of the problems with the PayPal_Standard IPN and the fixes required. @pradpnayak Could you take another look for me please?

"business" email does not necessarily match the "Merchant Account Email" defined for the payment processor. Currently this causes the IPN to silently fail as it cannot identify the payment processor. Fix: A new function getPayPalPaymentProcessorID() based on the one used by PayPalPro IPN is implemented (a future improvement could be to make this a shared function).
"receive_date" is being set to the date/time the IPN was processed instead of using the "payment_date" specified in the IPN response. Fix: We now use the "payment_date" if it is available.
Subsequent contributions not being created. On the client site I tested this on NONE of the subsequent transactions were being processed - I assumed this was because we are using CompleteTransaction function instead of calling the API Contribution.repeattransaction. It may, in part have been caused by the bad lookup for the payment processor (see above). However, on further testing and looking at the testIPNPaymentRecurSuccess() test, whilst I can see that the test "Passes" I can also see that it is not creating the subsequent contributions correctly (they have a different payment_instrument and the total_amount is different for both contributions because it is not using the value passed by the mock IPN response. Fix: I've updated this PR to include improvements to the unit test to make sure we capture those issues - it should pass when run with the other changes in this PR but fail if the other changes are not applied.

Note that PayPal Pro IPN and most other payment processors do already call the repeattransaction API when creating subsequent contributions for a recurring transaction. That is the correct thing for a payment processor to be doing to ensure it's behaviour is consistent.

pradpnayak · 2018-07-09T16:18:06Z

tests/phpunit/CiviTest/CiviUnitTestCase.php

+    $contributionParams = array_merge([
+        'total_amount' => '200',
+        'invoice_id' => $this->_invoiceID,
+        'financial_type_id' => 1,


can you change this to 'financial_type_id' => 'Donation',

Wouldn't it be more appropriate to use something like CRM_Core_Pseduoconstant::value(‘CRM_Contribute_BAO_Contribution’, ‘financial_type_id’, 'Donation’)

Done. @JoeMurray Normally yes. In this case we can use API "magic" so can set 'Donation' directly and the API will work it out.

pradpnayak · 2018-07-13T09:10:02Z

CRM/Core/Payment/PayPalIPN.php

-      $objects['contribution'] = &$contribution;
+      // In future moving to create pending & then complete, but this OK for now.
+      // Also consider accepting 'Failed' like other processors.
+      $input['contribution_status_id'] = $contributionStatuses['Completed'];


Can we use $input['contribution_status_id'] = 'Completed' ?

@pradpnayak I think it probably works either way?

pradpnayak · 2018-07-16T09:29:24Z

@mattwire I had setup two instances with CiviCRM 5.3.0 one with current state patch and without patch.

Test case: Without patch

Created Payment Processor Username : test1@paypal.com
Added recurring contribution $10 using test1@paypal.com
Updated username of Payment processor : test2@paypal.com
Added recurring contribution $15 using test1@paypal.com

Expected result : Record subsequent recurring contribution
Actual result: Recurring contribution was recorded from ipn call back in both cases(#2 and #4) and additional transaction is created for payment instrument change.

Test case: With patch

Created Payment Processor Username : test1@paypal.com
Added recurring contribution $10 using test1@paypal.com
Updated username of Payment processor : test2@paypal.com
Added recurring contribution $15 using test1@paypal.com

Expected result : Record subsequent recurring contribution
Actual result: Recurring contribution was recorded from ipn call back in both cases(#2 and #4) and no additional transaction is created, received date set using payment date from ipn.

I couldn't replicate the issue through paypal ipn callback. However when i tried to use fake ipn call i could replicate the issue i.e subsequent payment were not created for $10 as it was failing silently before applying patch and worked after applying patch. The ipn callback fails when i use civicrm/extern/ipn.php as notification url and current version of civicrm uses civicrm/payment/ipn/1? as notification url from paypal where 1 is a payment processor Id thats the reason i couldn't replicate the issue. @mattwire your client's paypal must be using older version of notification url which doesn't pass payment processor id and the code have to rely on business email or payment processor id from contribution recur.

Note: Patch works in both cases using old(civicrm/extern/ipn.php) and new(civicrm/payment/ipn/1) notification URL.

eileenmcnaughton · 2018-07-16T09:47:20Z

CRM/Core/Payment/PayPalProIPN.php

+          $input['original_contribution_id'] = $ids['contribution'];
+          $input['contribution_recur_id'] = $ids['contributionRecur'];
+          // We don't want to send out email receipts for repeat contributions.
+          $input['is_email_receipt'] = FALSE;


this looks like a change in behaviour - and while I think many would prefer it I don't think a change to outgoing emails should be this PR as they need their own discussion

Sure, I've removed that as it's not critical to this PR.

…eters for readability to match PayPalIPN

mattwire · 2018-07-23T17:10:29Z

@eileenmcnaughton @pradpnayak I've removed the is_email_receipt parameter as that's not critical to this PR. @pradpnayak Can you confirm if you are happy this should be merged now?

eileenmcnaughton · 2018-07-23T20:06:00Z

I looked and the code looks sensible & within scope now. Am happy to merge when @pradpnayak is

pradpnayak · 2018-07-23T21:14:06Z

Good to merge!!

Thanks @eileenmcnaughton and @mattwire

eileenmcnaughton · 2018-07-23T21:27:55Z

@Stoob pinging you as I now you use Paypal std - am merging this but perhaps you could make a point of testing the rc that gets cut on the 1st of next month

eileenmcnaughton · 2018-07-23T21:31:19Z

I've sent this email to the dev list

Matt has done some paypal std fixes #12387 which have been reviewed by Pradeep. I'm happy with the review & am proceeding to merge but am hoping that users of Paypal std on this list will make a special point of testing next month's rc (cut first Wed of Aug) since I know a bug in this area is a big issue

civicrm-builder added the master label Jul 1, 2018

mattwire mentioned this pull request Jul 1, 2018

NFC code cleanup for AuthNet, Paypal, PaypalPro IPNs #12386

Merged

mattwire force-pushed the paypalipn_fixes branch from da0d054 to 3abfa37 Compare July 3, 2018 09:06

mattwire changed the title ~~dev/financial#27 Paypal recurring IPNs don't work~~ dev/financial#27 Paypal_Standard recurring IPNs don't work Jul 3, 2018

mattwire force-pushed the paypalipn_fixes branch from 64f2b88 to 6a8bc5b Compare July 7, 2018 10:44

pradpnayak reviewed Jul 9, 2018

View reviewed changes

mattwire force-pushed the paypalipn_fixes branch from 6a8bc5b to 8d3af43 Compare July 10, 2018 16:35

pradpnayak reviewed Jul 13, 2018

View reviewed changes

eileenmcnaughton reviewed Jul 16, 2018

View reviewed changes

mattwire force-pushed the paypalipn_fixes branch from 8d3af43 to 5c6e4a1 Compare July 23, 2018 17:05

mattwire and others added 3 commits July 23, 2018 18:08

Fix Paypal recurring IPNs

f5bc4a2

Don't hardcode contribution_status_id in PayPalProIPN, re-order param…

b392485

…eters for readability to match PayPalIPN

Improve PayPalIPN unit tests

9f68fe6

mattwire force-pushed the paypalipn_fixes branch from 5c6e4a1 to 9f68fe6 Compare July 23, 2018 17:08

eileenmcnaughton added the merge on pass label Jul 23, 2018

eileenmcnaughton merged commit c099182 into civicrm:master Jul 24, 2018

mattwire deleted the paypalipn_fixes branch September 25, 2018 11:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dev/financial#27 Paypal_Standard recurring IPNs don't work #12387

dev/financial#27 Paypal_Standard recurring IPNs don't work #12387

mattwire commented Jul 1, 2018 •

edited

Loading

civibot bot commented Jul 1, 2018

eileenmcnaughton commented Jul 2, 2018

davejenx commented Jul 2, 2018

eileenmcnaughton commented Jul 3, 2018

mattwire commented Jul 6, 2018 •

edited

Loading

pradpnayak Jul 9, 2018

JoeMurray Jul 9, 2018

mattwire Jul 10, 2018

pradpnayak Jul 13, 2018

mattwire Jul 14, 2018

pradpnayak commented Jul 16, 2018

eileenmcnaughton Jul 16, 2018

mattwire Jul 23, 2018

mattwire commented Jul 23, 2018

eileenmcnaughton commented Jul 23, 2018

pradpnayak commented Jul 23, 2018

eileenmcnaughton commented Jul 23, 2018

eileenmcnaughton commented Jul 23, 2018

dev/financial#27 Paypal_Standard recurring IPNs don't work #12387

dev/financial#27 Paypal_Standard recurring IPNs don't work #12387

Conversation

mattwire commented Jul 1, 2018 • edited Loading

Overview

Before

After

Technical Details

Comments

civibot bot commented Jul 1, 2018

eileenmcnaughton commented Jul 2, 2018

davejenx commented Jul 2, 2018

eileenmcnaughton commented Jul 3, 2018

mattwire commented Jul 6, 2018 • edited Loading

pradpnayak Jul 9, 2018

Choose a reason for hiding this comment

JoeMurray Jul 9, 2018

Choose a reason for hiding this comment

mattwire Jul 10, 2018

Choose a reason for hiding this comment

pradpnayak Jul 13, 2018

Choose a reason for hiding this comment

mattwire Jul 14, 2018

Choose a reason for hiding this comment

pradpnayak commented Jul 16, 2018

eileenmcnaughton Jul 16, 2018

Choose a reason for hiding this comment

mattwire Jul 23, 2018

Choose a reason for hiding this comment

mattwire commented Jul 23, 2018

eileenmcnaughton commented Jul 23, 2018

pradpnayak commented Jul 23, 2018

eileenmcnaughton commented Jul 23, 2018

eileenmcnaughton commented Jul 23, 2018

mattwire commented Jul 1, 2018 •

edited

Loading

mattwire commented Jul 6, 2018 •

edited

Loading