dev/financial#14 Fix enotices & inability to cancel recurring on paypal express

[eileenmcnaughton]

Overview

Enotices when doing a recurring contribution using paypal express. Also apply changes to permit cancelling paypal express contributions

Before

After

notices fixed

Technical Details

For background - Paypal Express was not originally supported for recurring. It kind of snuck
in the back door since it was actually not possible to block in when configuring paypal pro
and people kept adding little fixes since they assumed it was broken rather than deliberately
missing functionality. Somewhere along the line this function was probably copied & extracted
from the main function - but it still contained a bunch of lines not
required for paypal express. I took a look at what data is actually available in this function
and removed reference to unavailable fields.

This is from https://developer.paypal.com/docs/classic/api/merchant/CreateRecurringPaymentsProfile_API_Operation_NVP/ and from testing and from https://lab.civicrm.org/dev/financial/issues/14

I also added the 2 one line changes from the reporter here https://lab.civicrm.org/dev/financial/issues/15 - I couldn't really test this but I could confirm that from a code POV they made sense and were simply removing blocks on Paypal Express.

Comments

The one are where I differed from logic from @carbar1103
  is the trxn_id - I retained the profile id which I think is consistent with use for other processsors, but needs testing - which I requested on the gitlab issue

I tested the flow of doing a recurring contribution from a contribution page - which I think is the only relevant flow.

Note that this is more or less a reviewer's commit as all changes were suggested by the reporter in gitlab - there is one minor difference - on the trxn_id - so if the originally reviewer retests & confirms I will merge based on that

[seamuslee001]

@eileenmcnaughton i'm guessing that in this case these are not applicable?

[eileenmcnaughton]

Right - this code chunk is actually called server to server - it's not the piece that redirects the user

[seamuslee001]

ok happy with that then

[cartbar]

Hello.

I have tried it, but it is failing at line 576, because the trxn_id is not set. You need to change the code so that it only looks for the transaction if the trxn_id is set.

from
if ($this->transactionExists($input['trxn_id'])) {

to something like
if ( (!empty($input['trxn_id']) && ($this->transactionExists($input['trxn_id'])) ) {

Regards,

Carl

[eileenmcnaughton]

@cartbar updated to avoid that exception

[cartbar]

Hello.

Unfortunately I am now getting a different error. The problem is with line 564:

$input['invoice'] = self::getValue('i', FALSE);

This is putting null into $input['invoice'], because the function that is supposed to be populating it - setInvoiceData() - doesn't. I think this has to be changed to

$input['invoice'] = $this->retrieve('invoice', 'String');

Not sure why this didn't happen during my initial testing (possibly because I only had a single contribution record).

Regards,

Carl

[eileenmcnaughton]

hmm - what is the content of the parameters posted? I guess it's not the same format as elsewhere?

[cartbar]

Hello

Sorry, I was debugging the wrong notification ("Recurring billing agreement expired", which also isn't working, but that is for another time). The "recurring_payment_profile_created" notification failed at line 582:

$result = civicrm_api3('contribution', 'getsingle', array('invoice_id' => $input['invoice']));

This is because it isn't looking for a test record. This needs to be changed to

$result = civicrm_api3('contribution', 'getsingle', array('invoice_id' => $i

nput['invoice'], 'contribution_test' => $contributionRecur['is_test']));

This assumes that you change the code to retrieve the 'is_test' field when getting the contribution_recur record.

Regards,

Carl

[eileenmcnaughton]

OK - I've updated to

$result = civicrm_api3('contribution', 'getsingle', ['invoice_id' => $input['invoice'], 'contribution_test' => '']);

which should remove the default to test being 1

[cartbar]

Hello

Moved a bit further - it is now failing at line 601:

$paymentProcessorID = self::getPayPalPaymentProcessorID();

This is because getPayPalPaymentProcessorID() always looks for a live processor (i.e. is_test = 0)

Regards,

Carl

[eileenmcnaughton]

Hmm - what is the ipn url you are using ? is it like civicrm/payment/ipn/x where x is the processor id?

[eileenmcnaughton]

(I assume it's returning the live processor id but it's failing - in validate?)

[cartbar]

Hello

The url is ".../sites/all/modules/civicrm/extern/ipn.php"

However, I do not believe that to be the problem. The code for getPayPalPaymentProcessorID() is

  public function getPayPalPaymentProcessorID() {
    // This is an unreliable method as there could be more than one instance.
    // Recommended approach is to use the civicrm/payment/ipn/xx url where xx is the payment
    // processor id & the handleNotification function (which should call the completetransaction api & by-pass this
    // entirely). The only thing the IPN class should really do is extract data from the request, validate it
    // & call completetransaction or call fail? (which may not exist yet).
    $paymentProcessorTypeID = CRM_Core_DAO::getFieldValue('CRM_Financial_DAO_PaymentProcessorType',
      'PayPal', 'id', 'name'
    );
    return (int) civicrm_api3('PaymentProcessor', 'getvalue', array(
      'is_test' => 0,
      'options' => array('limit' => 1),
      'payment_processor_type_id' => $paymentProcessorTypeID,
      'return' => 'id',
    ));

  }

As you can see it is specifically searching for a live payment processor. Given that I am using a test payment processor, this fails to find any payment processor. The way I fixed this was to change

    $paymentProcessorID = self::getPayPalPaymentProcessorID();

to

    $paymentProcessorID = $contributionRecur['payment_processor_id'];

(having previously changed the code that populates $contributionRecur to retrieve the payment processor id)

Regards,

Carl

[eileenmcnaughton]

OK - I can accept that change - pushed

[cartbar]

Hello,

You also need to change around line 566:

     $contributionRecur = civicrm_api3('contribution_recur', 'getsingle',
 array(
           'return' => 'contact_id, id',
           'invoice_id' => $input['invoice'],
         ));

to

     $contributionRecur = civicrm_api3('contribution_recur', 'getsingle',
 array(
           'return' => 'contact_id, id, payment_processor_id',
           'invoice_id' => $input['invoice'],
         ));

otherwise it won't get populated.

Regards,

Carl

[eileenmcnaughton]

@cartbar ok made that change too

[cartbar]

Hello.

It is all working as expected. Thanks for your hard work.

Do you know when these changes will be officially released?

Regards,

Carl

[eileenmcnaughton]

@cartbar if we get this merged now then it will be in the 5.3 release candidate (available for download from first Wed of next month) and in 5.3.0 which will be officially released on the first Wed of the month after

@seamuslee001 @colemanw @mlutfy @monishdeb - are one of you able to merge this?

It's a bit of an odd collaboration because this is code that has very low usage & while it has been clear from code comments for a long time we haven't really had anyone to test. Pretty much every line in this final commit has gone through the following process:

• Carl suggested the line to change & how
• I put up either the change he suggested or one that I thought was preferable after reading the code
• in pretty much all the cases where I deviated Carl helped me to understand why his suggestion was actually right :-)
• Carl re-tested & identified further things to change
• loop restarts

So, this is actually a patch that represents reviewed code changes in my mind - but I feels it needs a separate merger

[mlutfy]

At a glance, looks good. Ideally, because PayPal is pretty sensitive (low test coverage, regression risk), I would feel more comfortable with a second production user to confirm the bug/fix, but I agree that this is something that has probably not been used much, or in an area that is hard to debug, that most people probably just work around it.

We have ~ 15 days before the new RC is cut? Can we wait 1-2 days to see if anyone else chimes in?

[eileenmcnaughton]

@mlutfy I know @jmcclelland did use paypal express at one point - perhaps not now.

I think Paypal more broadly has good test coverage - but paypal express does not & has low usage too.

[eileenmcnaughton]

Actually @mattwire I've pulled your fix into this - so we have one clear patch set for @cartbar to test (we had some version confusion earlier so this seems good)

[eileenmcnaughton]

@cartbar I took the data you supplied & added a test. Hopefully this will work

@Stoob I think you use paypal express too? Perhaps you can test this out too?

[cartbar]

Hello.

I have done one test setting up a recurring contribution. The recurring contribution appears to be set up correctly, but the initial collection isn't made. I've done a diff of the code and can't see anything obvious, so I will have another look when I get a moment. However, it would help me if you could describe how that payment should happen (I am assuming that it is triggered by the first IPN being received, since the second IPN is usually generated a few minutes after the first).

Regards,

Carl

[cartbar]

Hello.

Something odd is going on with PayPal. The payment notifications have come through hours after the agreement was set up.

I will continue my testing and let you know how it goes

Regards,

Carl

[mattwire]

@eileenmcnaughton Could you rename this PR to "dev/financial#14 Fix enotices & inability to cancel recurring on paypal express"

[mlutfy]

😎

[cartbar]

Hello

PayPalImpl.php needs a few changes to allow amount to be changed:

692c692,693
<     if (!$this->isPayPalType($this::PAYPAL_PRO)) {
---
>     if ( (!$this->isPayPalType($this::PAYPAL_PRO)) &&
>          (!$this->isPayPalType($this::PAYPAL_EXPRESS)) ) {
830c831,832
<     if ($this->isPayPalType($this::PAYPAL_PRO)) {
---
>     if ( ($this->isPayPalType($this::PAYPAL_PRO)) ||
>          ($this->isPayPalType($this::PAYPAL_EXPRESS)) ) {

Also, when I go to .../index.php?q=civicrm/contribute/updaterecur&reset=1&coid=211&cs=07a38d79ac9511e474870bbd0cd3b393_1526559486_inf as an anonymous user, the browser reports an error trying to access .../index.php?q=civicrm/custom&type=ContributionRecur&entityID=79&qf=86c61170a3a93e460697cf94b4a06552_4261&cgcount=1&snippet=json.

This url appears to be being created by a javascript function called CRM.buildCustomData in CRM/Contribute/Form/UpdateSubscription.tpl. I have no idea what it is trying to do, but it doesn't seem to stop the page from working

Regards,

Carl

[eileenmcnaughton]

@mattwire just pointing out I had to revert this change to make it work (the fact things like this can sneak in is why I try to get each PR down to as limited a change set as possible, otherwise it's too easy to miss something seemingly innocuous)

[eileenmcnaughton]

@cartbar as I understand it paypal IPNs can be delayed on recurring. I think there should be one to confirm the subscription was set up & then another to indicate that a payment has been taken

Can we keep the update recurring out of scope until we have fixed the original scope of this issue - ie. once we have resolved the e-notices & fixed the ability to cancel with no regression will be soon enough to look at the ability to alter the subscription (OTOH if we are actually causing that to regress through this it will be harder to ringfence)

[mattwire]

@cartbar Have you had chance to test this yet?

[eileenmcnaughton]

@cartbar It's also worth noting that the next rc is cut the first Wed of the month. I would be good to get this in for then - but if we can't then at least we should aim to merge the parts of it that are fully agreed - ie I could split off the commit that fixes the e-notices & we could get that merged. I think the more that is in the rc / the nightly tarball (both available from https://download.civicrm.org/latest ) the easier it will be to test the last bits

[cartbar]

Hello.

I used the URL in the "Receipt - Make a donation" - .../index.php?q=civicrm/contribute/unsubscribe&reset=1&coid=211&cs=07a38d79ac9511e474870bbd0cd3b393_1526559486_inf and I got the following error:

Civi\Payment\Exception\PaymentProcessorException: Profile Id is missing from the request Profile Id is missing from the request in /var/www/html/SOTS/sites/all/modules/civicrm/CRM/Core/Payment/PayPalImpl.php on line 731

Exception trace

| Function | Location

0 | CRM_Core_Payment_PayPalImpl->invokeAPI(Array) | /var/www/html/SOTS/sites/all/modules/civicrm/CRM/Core/Payment/PayPalImpl.php:731
1 | CRM_Core_Payment_PayPalImpl->cancelSubscription(null, Array) | /var/www/html/SOTS/sites/all/modules/civicrm/CRM/Contribute/Form/CancelSubscription.php:213
2 | CRM_Contribute_Form_CancelSubscription->postProcess() | /var/www/html/SOTS/sites/all/modules/civicrm/CRM/Core/Form.php:447
3 | CRM_Core_Form->mainProcess() | /var/www/html/SOTS/sites/all/modules/civicrm/CRM/Core/QuickForm/Action/Submit.php:74
4 | CRM_Core_QuickForm_Action_Submit->perform(Object(CRM_Contribute_Form_CancelSubscription), 'submit') | /var/www/html/SOTS/sites/all/modules/civicrm/packages/HTML/QuickForm/Controller.php:203
5 | HTML_QuickForm_Controller->handle(Object(CRM_Contribute_Form_CancelSubscription), 'submit') | /var/www/html/SOTS/sites/all/modules/civicrm/packages/HTML/QuickForm/Page.php:103
6 | HTML_QuickForm_Page->handle('submit') | /var/www/html/SOTS/sites/all/modules/civicrm/CRM/Core/Controller.php:351
7 | CRM_Core_Controller->run() | /var/www/html/SOTS/sites/all/modules/civicrm/CRM/Utils/Wrapper.php:113
8 | CRM_Utils_Wrapper->run('CRM_Contribute_F…', 'Cancel Subscript…', null) | /var/www/html/SOTS/sites/all/modules/civicrm/CRM/Core/Invoke.php:283
9 | CRM_Core_Invoke::runItem(Array) | /var/www/html/SOTS/sites/all/modules/civicrm/CRM/Core/Invoke.php:84
10 | CRM_Core_Invoke::_invoke(Array) | /var/www/html/SOTS/sites/all/modules/civicrm/CRM/Core/Invoke.php:52
11 | CRM_Core_Invoke::invoke(Array) | /var/www/html/SOTS/sites/all/modules/civicrm/drupal/civicrm.module:445
12 | civicrm_invoke('contribute', 'unsubscribe') | unknown:unknown
13 | call_user_func_array('civicrm_invoke', Array) | /var/www/html/SOTS/includes/menu.inc:527
14 | menu_execute_active_handler() | /var/www/html/SOTS/index.php:21
15 | {main} |  

Regards,

Carl

[eileenmcnaughton]

@cartbar that implies that there is nothing in the civicrm_contribution_recur.processor_id field

[eileenmcnaughton]

test this please

[cartbar]

Hello

I am holiday for a few days so won't be able to test this until I am back.

Regards,

Carl

[cartbar]

Hello.

I have tested cancelling a recurring contribution using the links in both the "Recurring Contribution Notification" and "Receipt" emails. In both cases it works as expected.

I was going to test cancelling from within the website (as an Admin user), but for the life of me I can't figure out how to do it.

Regards,

Carl

[eileenmcnaughton]

@mlutfy @seamuslee001 RC is cut - shall we merge this now?

[seamuslee001]

I'm satisfied that there has been enough testing of this PR that its ok to merge in