[REF] Retrieve the token if it is available from the Payment Token ta…

[seamuslee001]

…ble if no processor_id is set

Overview

Some Payment Processors (notably IATS) do not store the customer token in the civicrm_contribution_recur.processor_id column but rather in the civicrm_payment_token.token column instead see https://github.com/iATSPayments/com.iatspayments.civicrm/blob/3f72337ca0430fc3aaad94c3aba2271c198b2273/CRM/Core/Payment/iATSServiceACHEFT.php#L266

However this causes the cancel form on contribution recur self service to break because the cancel form is assuming the payment token will always be in civicrm_contribution_recur.processor_id https://github.com/civicrm/civicrm-core/blob/master/CRM/Contribute/Form/CancelSubscription.php#L219

Before

Self-Service Cancel form fails or IATS and possibly other payment processors that store the token in the civicrm_payment_token table rather than in the civicrm_contribution_recur table

After

Cancel form works for IATS and similar

ping @mattwire @eileenmcnaughton @KarinG note that this is a partial alternate to @Edzelopez 's #17445 Note this aims to only fix item 1 from that PR

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

[KarinG]

Thanks @seamuslee001 👍 ping @adixon

PS - civicrm_payment_token was created at a YMCA with poor meals in the Colorado mountains specifically for the purpose of storing payment processor tokens so I think we do want them there.

[eileenmcnaughton]

@seamuslee001 so we have a separate setPaymentToken that is more appropriate than setting the wrong field with it.

I'm kinda inclined to think that since we are already setting contribution recur id it makes sense for the processor to use that to get the token. It is in theory an extra query - but the way you are implementing it is an extra query anyway so the downside isn't that clear

[seamuslee001]

so @eileenmcnaughton my question then is what is the difference between setRecurProcessorID and setPaymentToken ? But also its worth noting that only 1 of the 3 forms seems to be using PropertyBag so far

[eileenmcnaughton]

@seamuslee001 legacy. We were already setting that & the calling functions were already expecting it

[seamuslee001]

So these 2 fields are serving the same purpose right? I'm just a bit confused as to what the end point then is and also I'm not looking to dig in and covert the other forms to property bag and they are still relying on getSubscriptionDetails information

[eileenmcnaughton]

Well fundamentally all the processor needs is the contributionRecurID if everything else is in the DB already.

The reason to give it more would be if it were more efficient query-wise or if is already coded to expect something for legacy reasons or if it's not in the DB.

If we do decide we should give it more then we should set the correct parameter - ie. paymentToken.

I would love to deprecate that silly processor_id field out of existance...

[eileenmcnaughton]

@artfulrobot might have further thoughts - I could make a case for a propertyBag getter that would load DB-retrievable values if not already loaded.

[mattwire]

@seamuslee001 I'm not fully understanding the link with cancelRecurring here and #17445. But civicrm_contribution_recur.processor_id and civicrm_contribution_recur.trxn_id have to be treated as effectively the same thing (processor_id is the only reference that is available in some circumstances so both Stripe and GoCardless (at least) use this to map to the external subscription. A customer payment token is not the same thing and is currently not used by either.

A payment token can be used to initiate a payment using a specific, pre-authorized by the customer, method and does not necessarily need to be tied to a subscription - eg. it could represent a customer credit card.

It might help if you rebase this because the link to cancelsubscription no longer goes to the right place and I can't be sure what you mean :-)

[seamuslee001]

@mattwire the problem is that for IATS and maybe other payment processors (I have not looked our client only uses IATS) the recurring token is stored in the civicrm_payment_token.token field not civicrm_contribution_recur.processor_id

The issue is that when we build the subscriptionDetails object we assume the subscription_id is coming from civicrm_contribution_recur.processor_id https://github.com/civicrm/civicrm-core/blob/master/CRM/Contribute/BAO/ContributionRecur.php#L331 so when someone goes to submit the form after following a link for an IATS payment processor this https://github.com/civicrm/civicrm-core/blob/master/CRM/Contribute/Form/CancelSubscription.php#L221 variable that we are setting the "ReccurProcessorID" to is therefore NULL because we haven't actually queried where IATS is storing the token and subsequently the property bag chucks an exception "Trying to cancel a recurring payment using the self serve link results in an error: InvalidArgumentException: "processorID field has max length of 255"

Eileen is saying that instead of using setPaymentToken for the propertyBag function I think or even not worry about it and assume the processor will get the token from the recur_id but I'm still not 100% sure that is perfect

[seamuslee001]

also have now rebased this PR

[mattwire]

Thanks @seamuslee001 please see #17466 for a cleanup that I think will help.

I think I now understand. IATS is using payment tokens in the way I describe (representing a payment method, eg. a credit card, a bank account) and is not directly related to the subscription - ie. you could use that token on multiple subscriptions/recurs. So it doesn't belong in processor_id(subscription_id).

Historically we didn't actually pass the recur ID into cancel and relied on looking the recur up via subscriptionId (processor_id). But now we do have the recur ID.

Therefore, I think the fix here should be #17466 and then changing https://github.com/civicrm/civicrm-core/blob/master/CRM/Contribute/Form/CancelSubscription.php#L221 to be optional... but..

What is actually being passed into setRecurProcessorID()? Because looking at the code it should accept '' or NULL.

[seamuslee001]

@mattwire my understanding is that the processor_id predates civicrm_payment_token table completely and as such processor_id held the payment token for charging a subscription. Others such as @eileenmcnaughton can probably confirm but that is my understanding

[eileenmcnaughton]

@seamuslee001 ""Trying to cancel a recurring payment using the self serve link results in an error: InvalidArgumentException: "processorID field has max length of 255""

• that was a bug we fixed - that should no longer occur on the rc or master with o without this patch - can you double check?

[mattwire]

that was a bug we fixed - that should no longer occur on the rc or master with o without this patch - can you double check?

That would explain why I couldn't work out why it would happen when looking at the code!
@seamuslee001 For some more background re. various IDs see https://lab.civicrm.org/dev/financial/-/issues/57

One other difference here is whether the recurring contributions are CiviCRM initiated (IATS and some others) or processor initiated (Stripe, GoCardless, Smartdebit etc). In the CiviCRM initiated case you can generally use the (auto-generated) invoice_id on the recur to represent the subscription ID. On processor initiated that field is generally useless and you have to use the processor_id field - that gives you the link to the external subscription.
@artfulrobot @KarinG agree?

Any payment tokens would be handled separately. In fact, civicrm_contribution_recur has a payment_token_id field which is being used (at least by IATs) to map the payment token to the recurring contribution.

[KarinG]

@mattwire my understanding is that the processor_id predates civicrm_payment_token table completely and as such processor_id held the payment token for charging a subscription. Others such as @eileenmcnaughton can probably confirm but that is my understanding

Yep - exactly it. I worked on this with a fellow [forgot his name...] back in 2015/CiviCamp Denver Sprint under Eileen's guidance. When Alan and I first started work on iATS Payments we looked at processor_id and found it was used by many different payment processors in so many different ways - containing many different type of data. So rather than jamming yet another type of data into that processor_id field we decided create our own iATS table to store the tokenized card info i.e. Card on File. Eileen suggested we create a civicrm_payment_token table to ensure not every payment processor that has the ability to tokenize cards would need to create their own table.

[KarinG]

Yes - correct processor_id is different from payment_token in that yes we can also charge that card on file ad-hoc (e.g. Christmas fundraising), but it's also very similar in that payment processors require one or the other to allow for self-serve updates. And I believe that's what @seamuslee001 is trying to enable.

In an abstract way isn't your processor_id a token as well? In the sense that there is a credit card associated with/connected to that processor_id right? I realize it's not a token you can use to pay for other things in that CiviCRM instance. It's more like a reverse token...

[eileenmcnaughton]

I'm closing this as the issue seems to have been an otherwise-fixed error

[KarinG]

@seamuslee001 - fixed how? Perhaps because after Alan ended up playing whack a mole for a bit he ended up adding a hook in the iATS Extension to retrieve the recuring id from the url when on the updatebilling form?

[eileenmcnaughton]

@KarinG my understanding was the underyling issue is that we changed core such that it was erroring per ""Trying to cancel a recurring payment using the self serve link results in an error: InvalidArgumentException: "processorID field has max length of 255""

In this case the processor has the recurring contribution id & it is an extra query for core or the processor to grab the token from that so it makes sense to do it in the extension as required.

[mattwire]

@KarinG @eileenmcnaughton There was an outstanding issue with updateBilling and the recurID until 5.25 - this comment might help: https://lab.civicrm.org/extensions/mjwshared/-/blob/0.8/CRM/Core/Payment/MJWTrait.php#L149

So I think it actually is fixed in CiviCRM 5.25 but you'll need to use a workaround if supporting older versions (I support the last security release upwards (ie 5.24)

[KarinG]

Ok that's great, so CancelSubscription.php now does have both mission critical references (recur_id and subscription_id)

https://github.com/civicrm/civicrm-core/pull/16741/files ->
CRM/Contribute/Form/CancelSubscription.php

+        $propertyBag->setContributionRecurID($this->getSubscriptionDetails()->recur_id);
$propertyBag->setRecurProcessorID($this->getSubscriptionDetails()->subscription_id);

So then we can do something similar for -> CRM/Contribute/Form/UpdateBilling.php as well?

Currently that form only has the processor_id reference:
$processorParams['subscriptionId'] = $this->getSubscriptionDetails()->processor_id;

For the exact same reasons -> that update form also needs:
+ $processorParams['recur_id'] = $this->_subscriptionDetails->recur_id;

@mattwire - I see you edited that
https://github.com/civicrm/civicrm-core/blame/master/CRM/Contribute/Form/UpdateBilling.php#L205 just a few days back -> I don't see the $propertyBag here -> so would we just be adding $processorParams['recur_id']?

[eileenmcnaughton]

@KarinG yes - we should do similar for updateBilling - note the current method name is 'updateSubscriptionBillingInfo' & we should wrap that in a 'do' function - perhaps 'doUpdateRecurring' & then maybe that should be the same wrapper fo ''changeSubscriptionAmount''