Update cancelSubscription form to use updated methodology #16501

eileenmcnaughton · 2020-02-10T06:44:42Z

Overview

Updates our methodology on the cancelSubscription form (the only for that cancels recurrings)

Before

input params are a string passed as a reference and an array
function is called cancelSubscription, which is inconsistent with our other 'action functions'
function returns a bool - which is inconsistent with our other 'action functions'
processors need to implement cancelSubscription for recurrings to be cancellable. If they support more than one processor & one supports cancel recurring & another doesn't this gets messy quickly as supportsCancelRecurring checks for the function existing so we recommend that both are implemented.

After

a) input params are a property bag
b) function is called doCancelRecur - in line with our other 'action' functions.
c) function throws a PaymentProcessorException on error or returns a result array with additional information (the message) which is consistent with other action functions
d) processors can still implement cancelRecurring with no change. Or they can implement supportsCancelRecurring, and also doCancelRecurring
e) we have a path towards separating whether a checkbox should be displayed to 'send cancel request' to the processor and whether to permit cancelling (separate into 2 supports statements or rename the existing one)

Existing processors do not need to be altered although some deprecation may occur in future.

Technical Details

This came up after eileenmcnaughton/nz.co.fuzion.omnipaymultiprocessor#149 was
logged & I thought it was time to pick this up again.

There is further cleanup to be done & I have deliberately left that out in order to keep this tightly related to the
decisions that need to be made.

This creates a new function more in line with our standards it starts with 'do', accepts a propertyBag,
expects a PaymentProcessorException to be thrown if there is a fail.
does not require PaymentProcessors to implement cancelSubscription. A common scenario is that a processor
supports recurrings being cancelled but does not need to do anything as updating the contribution
recur record is enough to achieve that. In this case processors should implement supportsCancelRecurring
but they do not need to implement this function. (Currently they are recommended to implement supportsCancelRecurring
in keeping with our standardised 'supports' pattern but the non-standard cancelSubscription is also required.
ensures that the only place in the core code that calls this passes the PropertyBag

However, this also introduces the parameter 'subscriptionId' to the PropertyBag. This is one we 'left for it's own discussion'
because - we all hate it.

The options as I see it are

subscriptionId - advantage is this is what is already in use & we will need some transitioning if we change it. Disadvantage is that it only really relates to paypal apart from that....
processor_id - advantage is this i s in the contribution_recur table, disadvantage is that it's a really silly & confusing name for it
recur_profile_id, recur_gateway_id, other we come up with - advantage is we could give it a meaningful name, disadvantage is yet
another terminology in the mix & we might be opening a whole new can of worms.....

At this stage I'm leaning to 1 on the basis that it's a baby step that complies with existing code & doesn't open up a whole
new scope leap. But - open.....

Comments

@artfulrobot @mattwire

civibot · 2020-02-10T06:44:45Z

(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 · 2020-02-10T07:09:18Z

CRM/Contribute/Form/CancelSubscription.php

@@ -187,7 +188,7 @@ public function setDefaultValues() {
   * Process the form submission.
   */
  public function postProcess() {
-    $status = $message = NULL;
+    $message = NULL;


Actually message can go too in the cleanup round.

CRM/Core/Payment.php

mattwire · 2020-02-10T10:45:09Z

Civi/Payment/PropertyBag.php

@@ -65,6 +65,7 @@ class PropertyBag implements \ArrayAccess {
    'frequency_interval'          => 'recurFrequencyInterval',
    'recurFrequencyUnit'          => TRUE,
    'frequency_unit'              => 'recurFrequencyUnit',
+    'subscriptionId'              => TRUE,


@eileenmcnaughton @artfulrobot At the very least subscriptionId needs to be mapped to a more standard name (eg. subscriptionID. See https://lab.civicrm.org/dev/financial/issues/82#note_27029 and https://lab.civicrm.org/dev/financial/issues/57#note_19168 where this field has been discussed in detail

I think we've basically agreed that civicrm_contribution_recur.processor_id == subscriptionID.

We should update the descriptions in the database as a separate PR as they are currently:

processor_id: Possibly needed to store a unique identifier for this recurring payment order - if this is available from the processor??

trxn_id: unique transaction id. may be processor id, bank id + trans id, or account number + check number... depending on payment_method

CRM/Core/Payment.php

eileenmcnaughton · 2020-02-10T22:57:00Z

@mattwire yes - per the PR template we've discussed subscriptionId - it never felt like we resolved it :-(

I've updated this to use subscriptionID rather than subscriptionId for now - I guess I didn't cover that option in the comments on this PR. After looking at how this works with @artfulrobot's implementation I see that it's pretty easy to handle the changing of the name - so I'm now leaning towards switching to the DB field name (getRecurProcessorID) & just upping the comment block.. I don't have an appetite for renaming the field in the DB

eileenmcnaughton · 2020-02-10T22:57:53Z

ohh we HAVE recurProcessorID - I obviously didn't spot it because it didn't occur to me it would be called that. OK - I'll switch!

eileenmcnaughton · 2020-02-10T23:01:39Z

OK - that works without adding any properties to the class - just a mapping for processors not yet switched over

mattwire · 2020-02-10T23:41:47Z

Civi/Payment/PropertyBag.php

@@ -65,6 +65,7 @@ class PropertyBag implements \ArrayAccess {
    'frequency_interval'          => 'recurFrequencyInterval',
    'recurFrequencyUnit'          => TRUE,
    'frequency_unit'              => 'recurFrequencyUnit',
+    'subscriptionId'              => 'recurProcessorID',


Great :-) Nice and simple

Yep the magic is in how Rich implemented it

/** * Implements ArrayAccess::offsetGet * * @param mixed $offset * @return mixed */ public function offsetGet ($offset) { $prop = $this->handleLegacyPropNames($offset); return $this->get($prop, 'default'); }

a) use properyBag and b) call doCancelRecur - this separates adds a new function more in line with our other 'do' functions. This came up after eileenmcnaughton/nz.co.fuzion.omnipaymultiprocessor#149 was logged & I thought it was time to pick this up again. There is further cleanup to be done & I have deliberately left that out in order to keep this tightly related to the decisions that need to be made. 1) This creates a new function more in line with our standards it starts with 'do', accepts a propertyBag, expects a PaymentProcessorException to be thrown if there is a fail. 2) does not require PaymentProcessors to implement cancelSubscription. A common scenario is that a processor supports recurrings being cancelled but does not need to do anything as updating the contribution recur record is enough to achieve that. In this case processors should implement supportsCancelRecurring but they do not need to implement this function. (Currently they are recommended to implement supportsCancelRecurring in keeping with our standardised 'supports' pattern but the non-standard cancelSubscription is also required. 3) ensures that the only place in the core code that calls this passes the PropertyBag However, this also introduces the parameter 'subscriptionId' to the PropertyBag. This is one we 'left for it's own discussion' because - we all hate it. The options as I see it are 1) subscriptionId - advantage is this is what is already in use & we will need some transitioning if we change it. Disadvantage is that it only really relates to paypal apart from that.... 2) processor_id - advantage is this i s in the contribution_recur table, disadvantage is that it's a really silly & confusing name for it 3) recur_profile_id, recur_gateway_id, other we come up with - advantage is we could give it a meaningful name, disadvantage is yet another terminology in the mix & we might be opening a whole new can of worms..... At this stage I'm leaning to 1 on the basis that it's a baby step that complies with existing code & doesn't open up a whole new scope leap. But - open.....

eileenmcnaughton · 2020-02-11T02:56:48Z

test this please

artfulrobot · 2020-02-11T09:18:14Z

I'm at home looking after my youngest who's off school following a (non emergency) op. Reading this was a rollercoaster ride:

"Oh no!" subscriptionId! I give up!
Ah, good to have links to previous discussion
Nooooo don't use subscriptionId!
Oooh, yes!
Yes!
YES!

Good work both, this looks sensible to me.

I wonder, as there aren't too many cancelSubscription implementations in core to change, whether it would be worth updating those to properly use the property bag instead of them assuming an array?

eileenmcnaughton · 2020-02-11T09:33:29Z

test this please

eileenmcnaughton · 2020-02-11T09:34:19Z

@artfulrobot we could probably clean up the 2 places in core - testing is alway a pain :-( - I would leave that as cleanup / follow up (as with the form clean up)

artfulrobot · 2020-02-11T09:48:15Z

@eileenmcnaughton sounds good

mattwire · 2020-02-11T10:39:19Z

From a code POV I'm happy for this to be merged. However it still needs to be r-run. If someone else gets there first that would be great, otherwise I'll probably look at integrating this with Stripe and test it next week.

eileenmcnaughton · 2020-02-11T19:48:21Z

Thanks @mattwire

eileenmcnaughton · 2020-02-17T21:42:04Z

@mattwire @artfulrobot did either of you get a chance to r-run this?

mattwire · 2020-03-06T08:08:03Z

CRM/Contribute/Form/CancelSubscription.php

@@ -207,17 +208,16 @@ public function postProcess() {
      // civicrm_contribution_recur.processor_id
      $cancelParams = ['subscriptionId' => $this->_subscriptionDetails->subscription_id];
      try {
-        $cancelSubscription = $this->_paymentProcessorObj->cancelSubscription($message, $cancelParams);
+        $propertyBag = new PropertyBag();
+        $propertyBag->setRecurProcessorID($this->_subscriptionDetails->subscription_id);


@eileenmcnaughton Please add in here: $propertyBag->setContributionRecurID($this->_subscriptionDetails->recur_id); so we have the recurring contribution ID available as well.

OK - let's worry about adding new parameters in a follow up - the goal of this PR is just to switch with parity

mattwire · 2020-03-06T08:08:51Z

CRM/Core/Payment.php

+  public function doCancelRecurring(PropertyBag $propertyBag) {
+    if (method_exists($this, 'cancelSubscription')) {
+      $message = NULL;
+      if ($this->cancelSubscription($message, $propertyBag)) {


Before this PR cancelSubscription gets the param "subscriptionId", afterwards it does not. This breaks most current cancelSubscription implementations so we can't merge as is :-(

@mattwire so we stopped passing inn

$cancelParams = ['subscriptionId' => $this->_subscriptionDetails->subscription_id]; `` But instead we have

$propertyBag->setRecurProcessorID($this->_subscriptionDetails->subscription_id);

So subscripttonId should still be available - did you test & find it missing?

mattwire · 2020-03-06T08:14:14Z

@eileenmcnaughton @artfulrobot I've added two comments. One to set the recur ID property (which will make Stripe work whether or not subscriptionId is set as we've been checking that first for ages.

The other comment currently blocking - we want to get rid of subscriptionId but this PR stops it being available in cancelSubscription (only recurProcessorID is available) and means that all existing cancelSubscription implementations will break (including Stripe). @artfulrobot I don't know if there's any way that propertyBag could silently handle that for now and somehow output a deprecation notice when subscriptionId is accessed?

By the way, this is why Stripe and other processors using mjwshared will continue to work if we pass in contributionRecurID and drop subscriptionId because we've been shipping this code for ages: https://lab.civicrm.org/extensions/mjwshared/-/blob/master/CRM/Core/Payment/MJWTrait.php#L162-187

eileenmcnaughton · 2020-03-06T08:21:02Z

@mattwire recurProcessorID IS subscriptionId - see

civicrm-core/Civi/Payment/PropertyBag.php

Line 68 in 11ec1f9

'subscriptionId' => 'recurProcessorID',

mattwire · 2020-03-06T08:46:07Z

@mattwire recurProcessorID IS subscriptionId - ..

Yes, but it doesn't work. At least not with the code I've linked to above. Actually it's also not retrieving the contributionRecurID when I add it in. On further testing it turns out this is because using CRM_Utils_Array::value does not work with propertyBag - if I access the properties directly (eg. $params['subscriptionId'] it does work.

So I'm not sure what to suggest here. Obviously I can update the stripe/mjwshared extension code to not use CRM_Utils_Array::value but it's a pretty nasty breaking change for all existing users. Just looking at https://stats.civicrm.org/json/ext/mjwshared.json shows there are over 800 sites that would need to upgrade their extension versions and we're not very good at communicating that core needs a certain extension version (we can do it the other way around).

mattwire · 2020-03-06T08:47:02Z

The alternative might be to enhance CRM_Utils_Array::value to work with ArrayAccess and propertyBag - @artfulrobot ?

eileenmcnaughton · 2020-03-08T22:24:59Z

Just linking #16699 which is doing what @mattwire suggested ^^ (although the deprecation notice in there is flushing out a few things to work through before we can merge).

Big ups to @mattwire for doing an r-run on this with Stripe pre-merge & flushing out the array::value issue as I expect it would have caused problems

eileenmcnaughton · 2020-03-10T20:49:18Z

@mattwire we merged the dependency so this should work for you now

mattwire · 2020-03-10T21:08:31Z

Now working :-)

civibot bot added the master label Feb 10, 2020

eileenmcnaughton force-pushed the cancel_recur branch from a9aa915 to 3d79288 Compare February 10, 2020 07:02

eileenmcnaughton mentioned this pull request Feb 10, 2020

Cannot cancel recurring contribution eileenmcnaughton/nz.co.fuzion.omnipaymultiprocessor#149

Open

eileenmcnaughton commented Feb 10, 2020

View reviewed changes

CRM/Core/Payment.php Outdated Show resolved Hide resolved

mattwire reviewed Feb 10, 2020

View reviewed changes

CRM/Core/Payment.php Outdated Show resolved Hide resolved

eileenmcnaughton force-pushed the cancel_recur branch 3 times, most recently from f11e304 to c32e091 Compare February 10, 2020 22:50

eileenmcnaughton force-pushed the cancel_recur branch from c32e091 to 2096393 Compare February 10, 2020 23:00

mattwire reviewed Feb 10, 2020

View reviewed changes

eileenmcnaughton force-pushed the cancel_recur branch from 2096393 to 11ec1f9 Compare February 10, 2020 23:47

mattwire reviewed Mar 6, 2020

View reviewed changes

mattwire added the needs-work-not-review-ready Submitter required to take action, will be closed after 1-2 weeks in this state label Mar 6, 2020

eileenmcnaughton removed the needs-work-not-review-ready Submitter required to take action, will be closed after 1-2 weeks in this state label Mar 10, 2020

mattwire merged commit 2ae6888 into civicrm:master Mar 10, 2020

mattwire mentioned this pull request Mar 10, 2020

REF Switch to getter on cancelSubscription form #16740

Merged

eileenmcnaughton deleted the cancel_recur branch March 11, 2020 01:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update cancelSubscription form to use updated methodology #16501

Update cancelSubscription form to use updated methodology #16501

eileenmcnaughton commented Feb 10, 2020 •

edited

Loading

civibot bot commented Feb 10, 2020

eileenmcnaughton Feb 10, 2020

mattwire Feb 10, 2020

mattwire Feb 10, 2020 •

edited

Loading

eileenmcnaughton commented Feb 10, 2020

eileenmcnaughton commented Feb 10, 2020

eileenmcnaughton commented Feb 10, 2020

mattwire Feb 10, 2020

eileenmcnaughton Feb 11, 2020

eileenmcnaughton commented Feb 11, 2020

artfulrobot commented Feb 11, 2020

eileenmcnaughton commented Feb 11, 2020

eileenmcnaughton commented Feb 11, 2020

artfulrobot commented Feb 11, 2020

mattwire commented Feb 11, 2020

eileenmcnaughton commented Feb 11, 2020

eileenmcnaughton commented Feb 17, 2020

mattwire Mar 6, 2020

eileenmcnaughton Mar 6, 2020

mattwire Mar 6, 2020

eileenmcnaughton Mar 6, 2020

mattwire commented Mar 6, 2020

eileenmcnaughton commented Mar 6, 2020

mattwire commented Mar 6, 2020

mattwire commented Mar 6, 2020 •

edited

Loading

eileenmcnaughton commented Mar 8, 2020

eileenmcnaughton commented Mar 10, 2020

mattwire commented Mar 10, 2020

Update cancelSubscription form to use updated methodology #16501

Update cancelSubscription form to use updated methodology #16501

Conversation

eileenmcnaughton commented Feb 10, 2020 • edited Loading

Overview

Before

After

Technical Details

Comments

civibot bot commented Feb 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattwire Feb 10, 2020 • edited Loading

Choose a reason for hiding this comment

eileenmcnaughton commented Feb 10, 2020

eileenmcnaughton commented Feb 10, 2020

eileenmcnaughton commented Feb 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eileenmcnaughton commented Feb 11, 2020

artfulrobot commented Feb 11, 2020

eileenmcnaughton commented Feb 11, 2020

eileenmcnaughton commented Feb 11, 2020

artfulrobot commented Feb 11, 2020

mattwire commented Feb 11, 2020

eileenmcnaughton commented Feb 11, 2020

eileenmcnaughton commented Feb 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattwire commented Mar 6, 2020

eileenmcnaughton commented Mar 6, 2020

mattwire commented Mar 6, 2020

mattwire commented Mar 6, 2020 • edited Loading

eileenmcnaughton commented Mar 8, 2020

eileenmcnaughton commented Mar 10, 2020

mattwire commented Mar 10, 2020

eileenmcnaughton commented Feb 10, 2020 •

edited

Loading

mattwire Feb 10, 2020 •

edited

Loading

mattwire commented Mar 6, 2020 •

edited

Loading