Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from Jul 24, 2018

Conversation

@mattwire
Copy link
Contributor

commented Jul 1, 2018

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

This comment has been minimized.

Copy link

commented Jul 1, 2018

(Standard links)

@eileenmcnaughton

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2018

@mattwire you can rebase this now

@davejenx

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2018

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

@eileenmcnaughton

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2018

@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 mattwire force-pushed the mattwire:paypalipn_fixes branch from da0d054 to 3abfa37 Jul 3, 2018

@mattwire 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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 6, 2018

@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.

@mattwire mattwire force-pushed the mattwire:paypalipn_fixes branch from 64f2b88 to 6a8bc5b Jul 7, 2018

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

This comment has been minimized.

Copy link
@pradpnayak

pradpnayak Jul 9, 2018

Contributor

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

This comment has been minimized.

Copy link
@JoeMurray

JoeMurray Jul 9, 2018

Contributor

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

This comment has been minimized.

Copy link
@mattwire

mattwire Jul 10, 2018

Author Contributor

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

@mattwire mattwire force-pushed the mattwire:paypalipn_fixes branch from 6a8bc5b to 8d3af43 Jul 10, 2018

$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'];

This comment has been minimized.

Copy link
@pradpnayak

pradpnayak Jul 13, 2018

Contributor

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

This comment has been minimized.

Copy link
@mattwire

mattwire Jul 14, 2018

Author Contributor

@pradpnayak I think it probably works either way?

@pradpnayak

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2018

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

Test case: Without patch

  1. Created Payment Processor Username : test1@paypal.com
  2. Added recurring contribution $10 using test1@paypal.com
  3. Updated username of Payment processor : test2@paypal.com
  4. 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.

beforepatch

beforepatch-1

Test case: With patch

  1. Created Payment Processor Username : test1@paypal.com
  2. Added recurring contribution $10 using test1@paypal.com
  3. Updated username of Payment processor : test2@paypal.com
  4. 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.
afterpatch

afterpatch1

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.

$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 comment has been minimized.

Copy link
@eileenmcnaughton

eileenmcnaughton Jul 16, 2018

Contributor

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

This comment has been minimized.

Copy link
@mattwire

mattwire Jul 23, 2018

Author Contributor

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

@mattwire mattwire force-pushed the mattwire:paypalipn_fixes branch from 8d3af43 to 5c6e4a1 Jul 23, 2018

@mattwire mattwire force-pushed the mattwire:paypalipn_fixes branch from 5c6e4a1 to 9f68fe6 Jul 23, 2018

@mattwire

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2018

@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

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2018

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

@pradpnayak

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2018

Good to merge!!

Thanks @eileenmcnaughton and @mattwire

@eileenmcnaughton

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2018

@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

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2018

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

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

1 check passed

default Build finished.
Details

@mattwire mattwire deleted the mattwire:paypalipn_fixes branch Sep 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.