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

Allow processors (inc. paypal rest) to not support cancel recurring #210

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

michaelmcandrew
Copy link
Contributor

The current PayPal Checkout implementation uses reference transactions to deliver recurring payments.

Therefore, no action is needed on the Payment processor side to cancel a recurring transaction.

So I wanted to set CRM_Core_Payment_OmnipayMultiProcessor::supportsCancelRecurring() to return FALSE.

Currently, with it set to TRUE, cancelling a recurring payment is broken for anoymous users. Setting it to FALSE fixes the issue.

I added a meta data field to define this to PayPal Checkout.

I didn't want to add it to other payment processors but since the default for undefined meta data is false, I thought it might be a good idea to be able to default the default when none exists.

To do this, I added another param to ::getProcessorTypeMetadata() to allow it to return other values (TRUE in this case).

I also added a commit to comply with civicrm coding standards. The interesting stuff is in this commit: 11bcd79.

Interested to know what you think about this approach.

@civibot civibot bot added the master label Sep 22, 2021
@mattwire
Copy link
Contributor

@michaelmcandrew I don't think this is the right way to do it - you still want to be able to "cancel" the subscription in CiviCRM but you don't want to notify the payment processor - you should set supportsCancelRecurringNotifyOptional() to FALSE. See https://docs.civicrm.org/dev/en/latest/extensions/payment-processors/create/#supportscancelrecurringnotifyoptional and https://docs.civicrm.org/dev/en/latest/extensions/payment-processors/create/#docancelrecurring-function

@eileenmcnaughton
Copy link
Owner

What @mattwire says seems right - if that works then do that....

note I think the behaviour for Paypal rest on recurrings is the same as other processors in Omnipay (ie it's supported but should not 'do' anything) so you don't have to implement it in a processor specific way

@michaelmcandrew
Copy link
Contributor Author

Thanks for the pointers, both!

You were right, @mattwire. After a bit more testing we realised that the fix I suggested above resulted in the cancel this subscription link being removed from the receipts.

On further investigation, it looks like CRM_Core_Payment::doCancelRecurring() is expecting CRM_Core_Payment_OmnipayMultiProcessor::cancelSubscription() to return true to avoid throwing an exception.

But it returns void, which I think is ultimately the source of the errors we were experiencing (and this is likely omnipay wide).

From my reading of CRM_Core_Payment::doCancelRecurring(), the best solution here would be to remove the empty CRM_Core_Payment_OmnipayMultiProcessor::cancelSubscription(). Let me know what you think and I will update the patch.

Also @eileenmcnaughton - do any of the omnipay processors communicate with the payment processor to cancel recurring contributions. My reading of the code suggested that they do not, and if that is the case, then I think we should probably set
supportsCancelRecurringNotifyOptional() to FALSE to avoid the suggestion that we will notify the processor - what do you think?

@eileenmcnaughton
Copy link
Owner

do any of the omnipay processors communicate with the payment processor to cancel recurring contributions. -
Answer = no

the best solution here would be to remove the empty CRM_Core_Payment_OmnipayMultiProcessor::cancelSubscription().
Answer - yeah I feel like we worked to make that function override no longer required so that sounds right

@eileenmcnaughton eileenmcnaughton force-pushed the master branch 3 times, most recently from 8f31154 to 679df0f Compare December 21, 2023 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants