allow to cancel subscription through CiviCRM #241

sunilpawar · 2017-10-24T16:53:54Z

#240 allow to cancel subscription through CiviCRM

h-c-c · 2017-10-24T17:09:43Z

Interesting! So does this get called when the cancel button is clicked on a Recurring Contribution?

sunilpawar · 2017-10-24T17:28:36Z

yes, on clicking on 'cancel' link , pop will open , it has options (e.g notify user) like other payment processor have.

h-c-c · 2017-10-24T20:40:08Z

I haven't tested this, but I feel like we'd be causing an error once the webhook receives the 'customer.subscription.deleted' event because that code currently cancels the recurring contribution.

In this case the recurring contribution would already be canceled.

Have you tested this and/or are you seeing such an error?

Thanks

sunilpawar · 2017-10-25T06:20:18Z

https://github.com/drastik/com.drastikbydesign.stripe/blob/4.7-dev/CRM/Stripe/Page/Webhook.php#L335

https://github.com/civicrm/civicrm-core/blob/master/CRM/Contribute/BAO/ContributionRecur.php#L253

recurring cancel api check status is not cancelled before going further. so there would not be any issue.

sunilpawar · 2017-11-21T06:21:27Z

Hi, what's your opinion on my last comment.?

drastik · 2017-11-21T15:22:09Z

Is this something that currently does not work with the current codebase?
(cancelling a recurring contribution in Civi UI)

h-c-c · 2017-11-21T16:48:50Z

@sunilpawar I was having difficulty understanding your sentence. But I think I have it now.

recurring cancel api check status is not cancelled before going further. so there would not be any issue.

With the API explorer, I just tried canceling an already-canceled recurring contribution with seemingly no ill effects.

@sunilpawar Have you have a chance to test your code?

@drastik currently the way we're doing it is cancel in the Stripe UI and the event triggers the Recurring Contribution cancelation in Civi.

sunilpawar · 2017-11-21T16:53:02Z

@h-c-c , yes i have tested this on live and test mode.

mattwire · 2017-11-24T11:15:52Z

CRM/Core/Payment/Stripe.php

+    \Stripe\Stripe::setApiKey($this->_paymentProcessor['user_name']);
+    $subscription = \Stripe\Subscription::retrieve($params['subscriptionId']);
+    $subscription->cancel();
+    return TRUE;


Looks correct.

mattwire · 2017-11-24T11:15:56Z

CRM/Core/Payment/Stripe.php

@@ -817,6 +817,10 @@ public function doRecurPayment(&$params, $amount, $stripe_customer) {
        VALUES (%1, %2, %3, %4, %5, '{$this->_islive}')", $query_params);
    }

+    // update recur processor_id with subscriptionId
+    if ($subscription_id && $recuring_contribution_id) {
+      CRM_Core_DAO::setFieldValue('CRM_Contribute_DAO_ContributionRecur', $recuring_contribution_id, 'processor_id', $subscription_id);


Can you use the Contributionrecur.create API here instead of accessing the DAO object directly? It's less likely to break in the future if we use the API.

@mattwire , used api instead of DAO function

h-c-c · 2017-11-25T03:25:01Z

@sunilpawar I just gave this a spin. When I attempt to cancel a subscription that is already canceled through the Stripe UI, it throughly locks up my civi session. Maybe we should check to see if the subscription is active before canceling it, and maybe give feedback that it doesn't exist if that's the case, and just cancel the recurring contribution.

If we proceed like this, then there's probably no need for the "Send cancellation request to Stripe?" option.

What do you think?

Thanks!

sunilpawar · 2017-11-25T09:34:34Z

@h-c-c
added try catch block to handled any exception.
Tested for cancelling already cancelled subscription.

is there way to check subscription is active, because retrieve cancelled subscription using api throwing error message 'No such subscription: sub_xxxxxxx'

drastik · 2017-11-26T16:51:29Z

We should init Stripe once in a constructor, that way we don't call these require_once() 's within functions/methods.

h-c-c · 2017-12-09T17:53:27Z

@sunilpawar I'm against this radio button choice for "Send cancellation request to Stripe?". I'm trying to think of a valid use case for letting the user get their systems out of sync and I really can't think of good reason.

Am I missing something? Seems like we should always cancel the subscription in Stripe and simply delete the recurring contribution if it's somehow already gone.

What do you think? Thanks!

MegaphoneJon · 2017-12-15T16:18:35Z

The "send cancellation request" is part of core CiviCRM.

The use case is that you can cancel recurring payments on the payment processor side, which doesn't update CiviCRM. It's not uncommon for folks to do so, not realizing there's a more correct way. I see this a lot when people switch from having a PayPal "Donate" button to using Civi - their staff are used to cancelling recurring contributions from within PayPal itself. I don't think this should block a merge.

h-c-c · 2018-01-24T20:25:31Z

Thanks @MegaphoneJon, good to know! I understand the scenario, but not the use case. I think a better approach would be to always try to cancel on the payprocessor side and catch silently, with the assumption that we never want an active subscription without a a matching active recurring contribution. My feeling is, if we don't have to give users a way to shoot themselves in the foot, then let's not.

I guess I was more specifically asking, is there a valid use case for having a canceled recurring contribution and active subscription. I see none at this time. Leaving the radio buttons seems like a potential pitfall, much less so if "yes" was set as the default option. It's a little detail, but ultimately a kindness I feel.

@sunilpawar what if we take this approach?

-break up the initialization and cancel in to two different try statements.
-Catch hard on the initialization
-Then catch soft for the retrieve and cancel...don't do anything for the exception.

That should hand the already-canceled situation I think.

MegaphoneJon · 2018-01-24T20:48:04Z

Hi @h-c-c - the use cases I've encountered are:

Someone erroneously canceled the payment on the payment processor side;
The payment processor account is no longer valid.

There's also "testing", but I think that can be handled more elegantly in other ways.

sunilpawar · 2018-01-26T07:28:24Z

@h-c-c This is back-end option for Admin/Support team who perform these operation and its safe (not every have access to payment processor site to cancel recurring payment if requested by end user). That's why these feature available through API.

CiviCRM have similar feature for other Payment processor (AuthorizeNet, PayPal Pro)

h-c-c · 2018-01-26T09:55:17Z

ha! I'm pretty sure that means it's not safe. That is after all, the point of @MegaphoneJon 's use case....that admin people make errors.

What do think about the proposed solution to your question?

is there way to check subscription is active, because retrieve cancelled subscription using api throwing error message 'No such subscription: sub_xxxxxxx'

jmcclelland · 2018-04-10T17:19:07Z

I think this is an important feature and I would really like to see it added. It seems like the only outstanding issue is h-c-c's suggestion about how to handle the already canceled subscription.

I would only add one more thing... the civicrm_contribution_recur table does not have the subscription id (aka processor_id) in it. That means any attempt to cancel a recurring contribution that was made before this patch will fail.

I would suggest two steps:

Alter the cancelSubscription function to check for the presence of the subscription id in params and fail gracefully if it's not there.
I think we would need an upgrade script that populates this field from the civicrm_stripe_subscription table. Something like:

UPDATE civicrm_contribution_recur cr JOIN civicrm_stripe_subscriptions ss ON cr.id = ss.contribution_recur_id SET cr.processor_id = ss.subscription_id WHERE cr.processor_id IS NULL;

MegaphoneJon · 2018-04-10T17:24:49Z

I'd only add that the extension should gracefully handle the situation where a subscription is already canceled from within Stripe. With the PayPal processor, pressing the "Cancel Recurring Contribution" button hangs CiviCRM, and leaves a message in the CiviCRM log.

h-c-c · 2018-04-19T20:53:43Z

@sunilpawar To deal with the already canceled situation, apparently we can see if $subscription->ended_at is set. Found here:
stripe/stripe-php#368

Probably a good idea to keep the try/catches in there anyway.

sunilpawar · 2018-05-04T05:31:03Z

I will add this, Thank @h-c-c for link.

petzsch · 2018-08-31T15:00:01Z

Is this PR still moving forward?The feature would be highly appreciated. 👍

allow to cancel subscription through CiviCRM

9e82c12

mattwire suggested changes Nov 24, 2017

View reviewed changes

used api instead for DAO function for updating processor_id value

98f06ca

added try catch, to handled invalid request

c6e2501

scardinius pushed a commit to WeMoveEU/com.drastikbydesign.stripe that referenced this pull request Nov 18, 2020

Fix drastik#241 - Incorrect Form Validation for checkboxes on profiles

fefb07c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow to cancel subscription through CiviCRM #241

allow to cancel subscription through CiviCRM #241

sunilpawar commented Oct 24, 2017

h-c-c commented Oct 24, 2017

sunilpawar commented Oct 24, 2017

h-c-c commented Oct 24, 2017

sunilpawar commented Oct 25, 2017

sunilpawar commented Nov 21, 2017

drastik commented Nov 21, 2017

h-c-c commented Nov 21, 2017

sunilpawar commented Nov 21, 2017

mattwire Nov 24, 2017

mattwire Nov 24, 2017

sunilpawar Nov 24, 2017

h-c-c commented Nov 25, 2017

sunilpawar commented Nov 25, 2017

drastik commented Nov 26, 2017

h-c-c commented Dec 9, 2017

MegaphoneJon commented Dec 15, 2017

h-c-c commented Jan 24, 2018

MegaphoneJon commented Jan 24, 2018

sunilpawar commented Jan 26, 2018

h-c-c commented Jan 26, 2018

jmcclelland commented Apr 10, 2018

MegaphoneJon commented Apr 10, 2018

h-c-c commented Apr 19, 2018

sunilpawar commented May 4, 2018

petzsch commented Aug 31, 2018

allow to cancel subscription through CiviCRM #241

Are you sure you want to change the base?

allow to cancel subscription through CiviCRM #241

Conversation

sunilpawar commented Oct 24, 2017

h-c-c commented Oct 24, 2017

sunilpawar commented Oct 24, 2017

h-c-c commented Oct 24, 2017

sunilpawar commented Oct 25, 2017

sunilpawar commented Nov 21, 2017

drastik commented Nov 21, 2017

h-c-c commented Nov 21, 2017

sunilpawar commented Nov 21, 2017

mattwire Nov 24, 2017

Choose a reason for hiding this comment

mattwire Nov 24, 2017

Choose a reason for hiding this comment

sunilpawar Nov 24, 2017

Choose a reason for hiding this comment

h-c-c commented Nov 25, 2017

sunilpawar commented Nov 25, 2017

drastik commented Nov 26, 2017

h-c-c commented Dec 9, 2017

MegaphoneJon commented Dec 15, 2017

h-c-c commented Jan 24, 2018

MegaphoneJon commented Jan 24, 2018

sunilpawar commented Jan 26, 2018

h-c-c commented Jan 26, 2018

jmcclelland commented Apr 10, 2018

MegaphoneJon commented Apr 10, 2018

h-c-c commented Apr 19, 2018

sunilpawar commented May 4, 2018

petzsch commented Aug 31, 2018