Repository: dj-stripe/dj-stripe
Issue: subscription.cancel() does not guard against the subscription already being canceled

If the subscription does not exist, the exception isn't caught. This probably should be caught. It would happen if an attempt to cancel is made on stale data.

Changed files:
djstripe/stripe_objects.py, tests/test_subscription.py

diff --git a/djstripe/stripe_objects.py b/djstripe/stripe_objects.py
index d083e97d..20c5f8b4 100644
--- a/djstripe/stripe_objects.py
+++ b/djstripe/stripe_objects.py
@@ -1907,7 +1907,21 @@ Fields not implemented:
 
         """
 
-        return self._api_delete(at_period_end=at_period_end)
+        try:
+            stripe_subscription = self._api_delete(at_period_end=at_period_end)
+        except InvalidRequestError as exc:
+            if "No such subscription:" in str(exc):
+                # cancel() works by deleting the subscription. The object still
+                # exists in Stripe however, and can still be retrieved.
+                # If the subscription was already canceled (status=canceled),
+                # that api_retrieve() call will fail with "No such subscription".
+                # However, this may also happen if the subscription legitimately
+                # does not exist, in which case the following line will re-raise.
+                stripe_subscription = self.api_retrieve()
+            else:
+                six.reraise(*sys.exc_info())
+
+        return stripe_subscription
 
     def reactivate(self):
         """
diff --git a/tests/test_subscription.py b/tests/test_subscription.py
index 80651670..cc95d2b1 100644
--- a/tests/test_subscription.py
+++ b/tests/test_subscription.py
@@ -13,9 +13,13 @@ from django.contrib.auth import get_user_model
 from django.test import TestCase
 from django.utils import timezone
 from mock import patch
+from stripe.error import InvalidRequestError
 
 from djstripe.models import Customer, Subscription, Plan
-from tests import FAKE_SUBSCRIPTION, FAKE_PLAN, FAKE_CUSTOMER, FAKE_PLAN_II, datetime_to_unix
+from tests import (
+    datetime_to_unix, FAKE_CUSTOMER, FAKE_PLAN, FAKE_PLAN_II,
+    FAKE_SUBSCRIPTION, FAKE_SUBSCRIPTION_CANCELED
+)
 
 
 class SubscriptionTest(TestCase):
@@ -300,3 +304,25 @@ class SubscriptionTest(TestCase):
         subscription_reactivate_fake = deepcopy(FAKE_SUBSCRIPTION)
         reactivated_subscription = Subscription.sync_from_stripe_data(subscription_reactivate_fake)
         self.assertEqual(reactivated_subscription.cancel_at_period_end, False)
+
+    @patch("djstripe.stripe_objects.StripeSubscription._api_delete")
+    @patch("stripe.Subscription.retrieve", return_value=deepcopy(FAKE_SUBSCRIPTION_CANCELED))
+    def test_cancel_already_canceled(self, subscription_retrieve_mock, subscription_delete_mock):
+        subscription_delete_mock.side_effect = InvalidRequestError("No such subscription: sub_xxxx", "blah")
+
+        subscription_fake = deepcopy(FAKE_SUBSCRIPTION)
+        subscription = Subscription.sync_from_stripe_data(subscription_fake)
+
+        self.assertEqual(Subscription.objects.filter(status="canceled").count(), 0)
+        subscription.cancel()
+        self.assertEqual(Subscription.objects.filter(status="canceled").count(), 1)
+
+    @patch("djstripe.stripe_objects.StripeSubscription._api_delete")
+    def test_cancel_error_in_cancel(self, subscription_delete_mock):
+        subscription_delete_mock.side_effect = InvalidRequestError("Unexpected error", "blah")
+
+        subscription_fake = deepcopy(FAKE_SUBSCRIPTION)
+        subscription = Subscription.sync_from_stripe_data(subscription_fake)
+
+        with self.assertRaises(InvalidRequestError):
+            subscription.cancel()