From d862611855d4cb72873293cec0131020b7b3385c Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 16 Aug 2023 18:29:34 -0500 Subject: [PATCH 01/13] Subscriptions: remove old code We are now fully using dj-stripe for subscriptions, so we can our code for that. I'm not fully removing the app yet, so we can do a remove the old tables with zero downtime. --- readthedocs/organizations/models.py | 20 +-- readthedocs/organizations/signals.py | 12 ++ readthedocs/subscriptions/event_handlers.py | 72 ++++---- readthedocs/subscriptions/managers.py | 161 ------------------ readthedocs/subscriptions/models.py | 2 - readthedocs/subscriptions/signals.py | 26 +-- readthedocs/subscriptions/tasks.py | 10 +- .../subscription_stats_email.txt | 2 +- .../tests/test_event_handlers.py | 101 +++-------- .../subscriptions/tests/test_models.py | 138 --------------- readthedocs/subscriptions/tests/test_views.py | 51 +----- readthedocs/subscriptions/utils.py | 45 +---- 12 files changed, 97 insertions(+), 543 deletions(-) delete mode 100644 readthedocs/subscriptions/managers.py delete mode 100644 readthedocs/subscriptions/tests/test_models.py diff --git a/readthedocs/organizations/models.py b/readthedocs/organizations/models.py index f201e96704b..3ec41fd09db 100644 --- a/readthedocs/organizations/models.py +++ b/readthedocs/organizations/models.py @@ -13,6 +13,8 @@ from readthedocs.core.permissions import AdminPermission from readthedocs.core.utils import slugify +from readthedocs.subscriptions.utils import get_or_create_stripe_subscription + from . import constants from .managers import TeamManager, TeamMemberManager from .querysets import OrganizationQuerySet @@ -23,11 +25,7 @@ class Organization(models.Model): - """ - Organization model. - - stripe_id: Customer id from Stripe API - """ + """Organization model.""" # Auto fields pub_date = models.DateTimeField(_('Publication date'), auto_now_add=True) @@ -128,19 +126,11 @@ def __str__(self): return self.name def get_or_create_stripe_subscription(self): - # TODO: remove this once we don't depend on our Subscription models. - from readthedocs.subscriptions.models import Subscription - - subscription = Subscription.objects.get_or_create_default_subscription(self) - if not subscription: - # This only happens during development. - log.warning("No default subscription created.") - return None - return self.get_stripe_subscription() + return get_or_create_stripe_subscription(self) def get_stripe_subscription(self): # Active subscriptions take precedence over non-active subscriptions, - # otherwise we return the must recently created subscription. + # otherwise we return the most recently created subscription. active_subscription = self.stripe_customer.subscriptions.filter( status=SubscriptionStatus.active ).first() diff --git a/readthedocs/organizations/signals.py b/readthedocs/organizations/signals.py index f4b1912cb6e..e0d1ad1ac16 100644 --- a/readthedocs/organizations/signals.py +++ b/readthedocs/organizations/signals.py @@ -10,6 +10,7 @@ from readthedocs.builds.models import Build from readthedocs.builds.signals import build_complete from readthedocs.organizations.models import Organization, Team, TeamMember +from readthedocs.payments.utils import delete_customer from readthedocs.projects.models import Project from .tasks import ( @@ -37,6 +38,7 @@ def remove_organization_completely(sender, instance, using, **kwargs): This includes: + - Stripe customer - Projects - Versions - Builds (deleted on cascade) @@ -46,6 +48,16 @@ def remove_organization_completely(sender, instance, using, **kwargs): - Artifacts (HTML, PDF, etc) """ organization = instance + + stripe_customer = organization.stripe_customer + if stripe_customer: + log.info( + "Removing Stripe customer", + organization_slug=organization.slug, + stripe_customer_id=stripe_customer.id, + ) + delete_customer(stripe_customer.id) + log.info("Removing organization completely", organization_slug=organization.slug) # ``Project`` has a ManyToMany relationship with ``Organization``. We need diff --git a/readthedocs/subscriptions/event_handlers.py b/readthedocs/subscriptions/event_handlers.py index 7f0de1aa285..0781d256ce2 100644 --- a/readthedocs/subscriptions/event_handlers.py +++ b/readthedocs/subscriptions/event_handlers.py @@ -12,7 +12,6 @@ from readthedocs.organizations.models import Organization from readthedocs.payments.utils import cancel_subscription as cancel_stripe_subscription -from readthedocs.subscriptions.models import Subscription from readthedocs.subscriptions.notifications import ( SubscriptionEndedNotification, SubscriptionRequiredNotification, @@ -37,8 +36,18 @@ def decorator(func): return decorator -def _update_subscription_from_stripe(rtd_subscription, stripe_subscription_id): - """Update the RTD subscription object given the new stripe subscription object.""" +@handler("customer.subscription.updated", "customer.subscription.deleted") +def update_subscription(event): + """ + Cancel trial subscriptions if their trial has ended. + + We need to cancel these subscriptions manually, + otherwise Stripe will keep them active. + + If the organization attached to the subscription is disabled, + and the subscription is active, we re-enable the organization. + """ + stripe_subscription_id = event.data["object"]["id"] log.bind(stripe_subscription_id=stripe_subscription_id) stripe_subscription = djstripe.Subscription.objects.filter( id=stripe_subscription_id @@ -47,18 +56,6 @@ def _update_subscription_from_stripe(rtd_subscription, stripe_subscription_id): log.info("Stripe subscription not found.") return - previous_subscription_id = rtd_subscription.stripe_id - Subscription.objects.update_from_stripe( - rtd_subscription=rtd_subscription, - stripe_subscription=stripe_subscription, - ) - log.info( - "Subscription updated.", - previous_stripe_subscription_id=previous_subscription_id, - stripe_subscription_status=stripe_subscription.status, - ) - - # Cancel the trial subscription if its trial has ended. trial_ended = ( stripe_subscription.trial_end and stripe_subscription.trial_end < timezone.now() ) @@ -75,26 +72,18 @@ def _update_subscription_from_stripe(rtd_subscription, stripe_subscription_id): ) cancel_stripe_subscription(stripe_subscription.id) - -@handler("customer.subscription.updated", "customer.subscription.deleted") -def update_subscription(event): - """Update the RTD subscription object with the updates from the Stripe subscription.""" - stripe_subscription_id = event.data["object"]["id"] - rtd_subscription = Subscription.objects.filter( - stripe_id=stripe_subscription_id - ).first() - if not rtd_subscription: - log.info( - "Stripe subscription isn't attached to a RTD object.", - stripe_subscription_id=stripe_subscription_id, - ) + organization = getattr(stripe_subscription.customer, "rtd_organization", None) + if not organization: + log.error("Subscription isn't attached to an organization") return - _update_subscription_from_stripe( - rtd_subscription=rtd_subscription, - stripe_subscription_id=stripe_subscription_id, - ) - + if ( + stripe_subscription.status == SubscriptionStatus.active + and organization.disabled + ): + log.info("Re-enabling organization.", organization_slug=organization.slug) + organization.disabled = False + organization.save() @handler("customer.subscription.deleted") def subscription_canceled(event): @@ -146,31 +135,34 @@ def checkout_completed(event): Stripe checkout will create a new subscription, so we need to replace the older one with the new one. + + If the organization attached to the customer is disabled, + we re-enable it, since the user just subscribed to a plan. """ customer_id = event.data["object"]["customer"] + log.bind(stripe_customer_id=customer_id) organization = Organization.objects.filter(stripe_customer__id=customer_id).first() if not organization: log.error( "Customer isn't attached to an organization.", - stripe_customer_id=customer_id, ) return stripe_subscription_id = event.data["object"]["subscription"] + log.bind(stripe_subscription_id=stripe_subscription_id) stripe_subscription = djstripe.Subscription.objects.filter( id=stripe_subscription_id ).first() if not stripe_subscription: log.info("Stripe subscription not found.") return - organization.stripe_subscription = stripe_subscription - organization.save() - _update_subscription_from_stripe( - rtd_subscription=organization.subscription, - stripe_subscription_id=stripe_subscription_id, - ) + if organization.disabled: + log.info("Re-enabling organization.", organization_slug=organization.slug) + organization.disabled = False + organization.stripe_subscription = stripe_subscription + organization.save() @handler("customer.updated") def customer_updated_event(event): diff --git a/readthedocs/subscriptions/managers.py b/readthedocs/subscriptions/managers.py deleted file mode 100644 index f616f61a4e0..00000000000 --- a/readthedocs/subscriptions/managers.py +++ /dev/null @@ -1,161 +0,0 @@ -"""Subscriptions managers.""" - -import structlog -from django.conf import settings -from django.db import models - -from readthedocs.core.history import set_change_reason -from readthedocs.subscriptions.utils import get_or_create_stripe_subscription - -log = structlog.get_logger(__name__) - - -class SubscriptionManager(models.Manager): - - """Model manager for Subscriptions.""" - - def get_or_create_default_subscription(self, organization): - """ - Get or create a trialing subscription for `organization`. - - If the organization doesn't have a subscription attached, - the following steps are executed. - - - If the organization doesn't have a stripe customer, one is created. - - A new stripe subscription is created using the default plan. - - A new subscription object is created in our database - with the information from the stripe subscription. - """ - if hasattr(organization, 'subscription'): - return organization.subscription - - from readthedocs.subscriptions.models import Plan - - plan = Plan.objects.filter( - stripe_id=settings.RTD_ORG_DEFAULT_STRIPE_SUBSCRIPTION_PRICE - ).first() - # This should happen only on development. - if not plan: - log.warning( - 'No default plan found, not creating a subscription.', - organization_slug=organization.slug, - ) - return None - - stripe_subscription = get_or_create_stripe_subscription(organization) - - return self.create( - plan=plan, - organization=organization, - stripe_id=stripe_subscription.id, - status=stripe_subscription.status, - start_date=stripe_subscription.start_date, - end_date=stripe_subscription.current_period_end, - trial_end_date=stripe_subscription.trial_end, - ) - - def update_from_stripe(self, *, rtd_subscription, stripe_subscription): - """ - Update the RTD subscription object with the information of the stripe subscription. - - :param subscription: Subscription object to update. - :param stripe_subscription: Stripe subscription object from API - :type stripe_subscription: stripe.Subscription - """ - # Documentation doesn't say what will be this value once the - # subscription is ``canceled``. I'm assuming that ``current_period_end`` - # will have the same value than ``ended_at`` - # https://stripe.com/docs/api/subscriptions/object?lang=python#subscription_object-current_period_end - start_date = stripe_subscription.current_period_start - end_date = stripe_subscription.current_period_end - log.bind(stripe_subscription=stripe_subscription.id) - - rtd_subscription.status = stripe_subscription.status - - # This should only happen if an existing user creates a new subscription, - # after their previous subscription was cancelled. - if stripe_subscription.id != rtd_subscription.stripe_id: - log.info( - 'Replacing stripe subscription.', - old_stripe_subscription=rtd_subscription.stripe_id, - new_stripe_subscription=stripe_subscription.id, - ) - rtd_subscription.stripe_id = stripe_subscription.id - - # Update trial end date if it's present - trial_end_date = stripe_subscription.trial_end - if trial_end_date: - rtd_subscription.trial_end_date = trial_end_date - - # Update the plan in case it was changed from the Portal. - # This mostly just updates the UI now that we're using the Stripe Portal. - # A miss here just won't update the UI, but this shouldn't happen for most users. - # NOTE: Previously we were using stripe_subscription.plan, - # but that attribute is deprecated, and it's null if the subscription has more than - # one item, we have a couple of subscriptions that have more than - # one item, so we use the first that is found in our DB. - for stripe_item in stripe_subscription.items.prefetch_related("price").all(): - plan = self._get_plan(stripe_item.price) - if plan: - rtd_subscription.plan = plan - break - else: - log.error("Plan not found, skipping plan update.") - - if stripe_subscription.status == "active" and end_date: - # Save latest active date (end_date) to notify owners about their subscription - # is ending and disable this organization after N days of unpaid. We check for - # ``active`` here because Stripe will continue sending updates for the - # subscription, with a new ``end_date``, even after the subscription enters - # an unpaid state. - rtd_subscription.end_date = end_date - - elif stripe_subscription.status == 'past_due' and start_date: - # When Stripe marks the subscription as ``past_due``, - # it means the usage of RTD service for the current period/month was not paid at all. - # At this point, we need to update our ``end_date`` to the last period the customer paid - # (which is the start date of the current ``past_due`` period --it could be the end date - # of the trial or the end date of the last paid period). - rtd_subscription.end_date = start_date - - klass = self.__class__.__name__ - change_reason = f'origin=stripe-subscription class={klass}' - - # Ensure that the organization is in the correct state. - # We want to always ensure the organization is never disabled - # if the subscription is valid. - organization = rtd_subscription.organization - if stripe_subscription.status == 'active' and organization.disabled: - log.warning( - 'Re-enabling organization with valid subscription.', - organization_slug=organization.slug, - stripe_subscription=rtd_subscription.id, - ) - organization.disabled = False - set_change_reason(organization, change_reason) - organization.save() - - set_change_reason(rtd_subscription, change_reason) - rtd_subscription.save() - return rtd_subscription - - def _get_plan(self, stripe_price): - from readthedocs.subscriptions.models import Plan - - try: - plan = ( - Plan.objects - # Exclude "custom" here, as we historically reused Stripe plan - # id for custom plans. We don't have a better attribute to - # filter on here. - .exclude(slug__contains="custom") - .exclude(name__icontains="Custom") - .get(stripe_id=stripe_price.id) - ) - return plan - except (Plan.DoesNotExist, Plan.MultipleObjectsReturned): - log.info( - "Plan lookup failed.", - stripe_price=stripe_price.id, - ) - return None diff --git a/readthedocs/subscriptions/models.py b/readthedocs/subscriptions/models.py index af65a608e61..84010f0340c 100644 --- a/readthedocs/subscriptions/models.py +++ b/readthedocs/subscriptions/models.py @@ -11,7 +11,6 @@ from readthedocs.core.utils import slugify from readthedocs.organizations.models import Organization from readthedocs.subscriptions.constants import FEATURE_TYPES -from readthedocs.subscriptions.managers import SubscriptionManager class Plan(models.Model): @@ -165,7 +164,6 @@ class Subscription(models.Model): null=True, ) - objects = SubscriptionManager() history = ExtraHistoricalRecords() class Meta: diff --git a/readthedocs/subscriptions/signals.py b/readthedocs/subscriptions/signals.py index a173deb5d97..49e2f56df44 100644 --- a/readthedocs/subscriptions/signals.py +++ b/readthedocs/subscriptions/signals.py @@ -2,38 +2,14 @@ import stripe import structlog -from django.db.models.signals import post_save, pre_delete +from django.db.models.signals import post_save from django.dispatch import receiver from readthedocs.organizations.models import Organization -from readthedocs.payments.utils import cancel_subscription -from readthedocs.subscriptions.models import Subscription log = structlog.get_logger(__name__) -# pylint: disable=unused-argument -@receiver(pre_delete, sender=Subscription) -def remove_stripe_subscription(sender, instance, using, **kwargs): - """Remove Stripe subscription on Subscription delete.""" - subscription = instance - try: - log.bind(organization_slug=subscription.organization.slug) - if subscription.stripe_id: - log.info("Removing organization Stripe subscription.") - cancel_subscription(subscription.stripe_id) - else: - log.exception( - "Can't remove Stripe subscription. Organization didn't have ID." - ) - - except Organization.DoesNotExist: - log.exception( - "Subscription has no organization. No subscription to cancel.", - subscription_id=subscription.pk, - ) - - # pylint: disable=unused-argument @receiver(post_save, sender=Organization) def update_stripe_customer(sender, instance, created, **kwargs): diff --git a/readthedocs/subscriptions/tasks.py b/readthedocs/subscriptions/tasks.py index 0adaf0c48b5..c8fa8ba10ee 100644 --- a/readthedocs/subscriptions/tasks.py +++ b/readthedocs/subscriptions/tasks.py @@ -5,12 +5,12 @@ from django.contrib.auth.models import User from django.db.models import Count from django.utils import timezone +from djstripe import models as djstripe from readthedocs.builds.models import Build from readthedocs.core.utils import send_email from readthedocs.organizations.models import Organization from readthedocs.projects.models import Domain, Project -from readthedocs.subscriptions.models import Subscription from readthedocs.subscriptions.notifications import ( OrganizationDisabledNotification, TrialEndingNotification, @@ -94,11 +94,11 @@ def weekly_subscription_stats_email(recipients=None): organizations_to_disable = Organization.objects.disable_soon(days=30).count() users = User.objects.filter(date_joined__gte=last_week).count() subscriptions = ( - Subscription.objects.filter( - trial_end_date__gte=last_week, - trial_end_date__lte=yesterday, + djstripe.Subscription.objects.filter( + created__gte=last_week, + created__lte=yesterday, ) - .values("status", "plan__name") + .values("status", "items__price__product__name") .annotate(total_status=Count("status")) .order_by("status") ) diff --git a/readthedocs/subscriptions/templates/subscriptions/notifications/subscription_stats_email.txt b/readthedocs/subscriptions/templates/subscriptions/notifications/subscription_stats_email.txt index 9800e0b41bf..84d0abb21d8 100644 --- a/readthedocs/subscriptions/templates/subscriptions/notifications/subscription_stats_email.txt +++ b/readthedocs/subscriptions/templates/subscriptions/notifications/subscription_stats_email.txt @@ -10,7 +10,7 @@ These are the stats of the past week: * Users: {{ users }} * Subscriptions: {% for subscription in subscriptions %} - * {{ subscription.plan__name }}/{{ subscription.status}}: {{ subscription.total_status }} + * {{ subscription.items__price__product__name }}/{{ subscription.status}}: {{ subscription.total_status }} {% endfor %} diff --git a/readthedocs/subscriptions/tests/test_event_handlers.py b/readthedocs/subscriptions/tests/test_event_handlers.py index b32956dc19b..0e52f76f4c4 100644 --- a/readthedocs/subscriptions/tests/test_event_handlers.py +++ b/readthedocs/subscriptions/tests/test_event_handlers.py @@ -10,7 +10,6 @@ from readthedocs.organizations.models import Organization from readthedocs.subscriptions import event_handlers -from readthedocs.subscriptions.models import Plan, Subscription @override_settings( @@ -26,9 +25,9 @@ def setUp(self): self.organization = get( Organization, slug="org", email="test@example.com", owners=[self.user] ) - get(Plan, stripe_id="trialing", slug="trialing") - def test_subscription_updated_event(self): + @mock.patch("readthedocs.subscriptions.event_handlers.cancel_stripe_subscription") + def test_subscription_updated_event(self, cancel_subscription_mock): """Test handled event.""" start_date = timezone.now() end_date = timezone.now() + timezone.timedelta(days=30) @@ -49,25 +48,22 @@ def test_subscription_updated_event(self): } }, ) - subscription = get( - Subscription, - stripe_id=stripe_subscription.id, - status=SubscriptionStatus.trialing, - ) event_handlers.update_subscription(event=event) - - subscription.refresh_from_db() - self.assertEqual(subscription.status, SubscriptionStatus.active) - self.assertEqual(subscription.trial_end_date, end_date) - self.assertEqual(subscription.end_date, end_date) + cancel_subscription_mock.assert_not_called() def test_reenabled_organization_on_subscription_updated_event(self): """Organization is re-enabled when subscription is active.""" + customer = get(djstripe.Customer, id="cus_KMiHJXFHpLkcRP") + self.organization.stripe_customer = customer + self.organization.disabled = True + self.organization.save() + start_date = timezone.now() end_date = timezone.now() + timezone.timedelta(days=30) stripe_subscription = get( djstripe.Subscription, id="sub_9LtsU02uvjO6Ed", + customer=customer, status=SubscriptionStatus.active, current_period_start=start_date, current_period_end=end_date, @@ -83,31 +79,22 @@ def test_reenabled_organization_on_subscription_updated_event(self): }, ) - organization = get(Organization, disabled=True) - subscription = get( - Subscription, - organization=organization, - stripe_id=stripe_subscription.id, - status=SubscriptionStatus.canceled, - ) - self.assertTrue(organization.disabled) - + self.assertTrue(self.organization.disabled) event_handlers.update_subscription(event=event) + self.organization.refresh_from_db() + self.assertFalse(self.organization.disabled) - subscription.refresh_from_db() - organization.refresh_from_db() - self.assertEqual(subscription.status, SubscriptionStatus.active) - self.assertEqual(subscription.trial_end_date, end_date) - self.assertEqual(subscription.end_date, end_date) - self.assertFalse(organization.disabled) + def test_subscription_checkout_completed_event(self): + customer = get(djstripe.Customer, id="cus_KMiHJXFHpLkcRP") + self.organization.stripe_customer = customer + self.organization.save() - def test_subscription_deleted_event(self): start_date = timezone.now() end_date = timezone.now() + timezone.timedelta(days=30) stripe_subscription = get( djstripe.Subscription, id="sub_9LtsU02uvjO6Ed", - status=SubscriptionStatus.canceled, + status=SubscriptionStatus.active, current_period_start=start_date, current_period_end=end_date, trial_end=end_date, @@ -116,27 +103,24 @@ def test_subscription_deleted_event(self): djstripe.Event, data={ "object": { - "id": "sub_9LtsU02uvjO6Ed", - "object": "subscription", + "id": "cs_test_a1UpM7pDdpXqqgZC6lQDC2HRMo5d1wW9fNX0ZiBCm6vRqTgZJZx6brwNan", + "object": "checkout.session", + "customer": customer.id, + "subscription": stripe_subscription.id, } }, ) - subscription = get( - Subscription, - organization=self.organization, - stripe_id=stripe_subscription.id, - status=SubscriptionStatus.active, - ) - event_handlers.update_subscription(event=event) + self.assertIsNone(self.organization.stripe_subscription) + event_handlers.checkout_completed(event=event) - subscription.refresh_from_db() - self.assertEqual(subscription.stripe_id, stripe_subscription.id) - self.assertEqual(subscription.status, SubscriptionStatus.canceled) + self.organization.refresh_from_db() + self.assertEqual(self.organization.stripe_subscription, stripe_subscription) - def test_subscription_checkout_completed_event(self): + def test_reenable_organization_on_subscription_checkout_completed_event(self): customer = get(djstripe.Customer, id="cus_KMiHJXFHpLkcRP") self.organization.stripe_customer = customer + self.organization.disabled = True self.organization.save() start_date = timezone.now() @@ -161,21 +145,12 @@ def test_subscription_checkout_completed_event(self): }, ) - subscription = get( - Subscription, - organization=self.organization, - stripe_id=None, - status=SubscriptionStatus.canceled, - ) - self.assertIsNone(self.organization.stripe_subscription) event_handlers.checkout_completed(event=event) - subscription.refresh_from_db() self.organization.refresh_from_db() - self.assertEqual(subscription.stripe_id, stripe_subscription.id) - self.assertEqual(subscription.status, SubscriptionStatus.active) self.assertEqual(self.organization.stripe_subscription, stripe_subscription) + self.assertFalse(self.organization.disabled) @mock.patch("readthedocs.subscriptions.event_handlers.cancel_stripe_subscription") def test_cancel_trial_subscription_after_trial_has_ended( @@ -208,18 +183,8 @@ def test_cancel_trial_subscription_after_trial_has_ended( } }, ) - subscription = get( - Subscription, - organization=self.organization, - stripe_id=stripe_subscription.id, - status=SubscriptionStatus.active, - ) event_handlers.update_subscription(event=event) - - subscription.refresh_from_db() - self.assertEqual(subscription.status, SubscriptionStatus.active) - self.assertTrue(subscription.is_trial_ended) cancel_subscription_mock.assert_called_once_with(stripe_subscription.id) @mock.patch("readthedocs.subscriptions.event_handlers.cancel_stripe_subscription") @@ -253,18 +218,8 @@ def test_dont_cancel_normal_subscription_after_trial_has_ended( } }, ) - subscription = get( - Subscription, - organization=self.organization, - stripe_id=stripe_subscription.id, - status=SubscriptionStatus.active, - ) event_handlers.update_subscription(event=event) - - subscription.refresh_from_db() - self.assertEqual(subscription.status, SubscriptionStatus.active) - self.assertTrue(subscription.is_trial_ended) cancel_subscription_mock.assert_not_called() def test_customer_updated_event(self): diff --git a/readthedocs/subscriptions/tests/test_models.py b/readthedocs/subscriptions/tests/test_models.py deleted file mode 100644 index fc27ebd2666..00000000000 --- a/readthedocs/subscriptions/tests/test_models.py +++ /dev/null @@ -1,138 +0,0 @@ -import django_dynamic_fixture as fixture -from django.test import TestCase, override_settings -from django.utils import timezone -from django_dynamic_fixture import get -from django_dynamic_fixture.ddf import BadDataError -from djstripe import models as djstripe -from djstripe.enums import SubscriptionStatus - -from readthedocs.organizations.models import Organization -from readthedocs.subscriptions.models import Plan, Subscription - - -@override_settings(RTD_ALLOW_ORGANIZATIONS=True) -class SubscriptionTests(TestCase): - - def setUp(self): - fixture.get( - Plan, - stripe_id='basic', - ) - fixture.get( - Plan, - stripe_id='advanced', - ) - - def test_create(self): - """Subscription creation.""" - org = fixture.get(Organization) - subscription = fixture.get(Subscription, organization=org) - self.assertIsNotNone(subscription) - self.assertIsNotNone(org.subscription) - - def test_double_create(self): - """Subscription creation.""" - org = fixture.get(Organization) - fixture.get(Subscription, organization=org) - with self.assertRaises(BadDataError): - fixture.get(Subscription, organization=org) - - def test_delete(self): - """Subscription delete doesn't cascade.""" - org = fixture.get(Organization) - subscription = fixture.get(Subscription, organization=org) - self.assertIsNotNone(subscription) - self.assertIsNotNone(org.subscription) - - Subscription.objects.filter(organization=org).delete() - org = Organization.objects.get(slug=org.slug) - self.assertIsNotNone(org) - - def test_update(self): - """Test update from stripe.""" - start_date = timezone.now() - end_date = timezone.now() + timezone.timedelta(days=30) - stripe_subscription = get( - djstripe.Subscription, - id="sub_foo", - status=SubscriptionStatus.active, - current_period_start=start_date, - current_period_end=end_date, - trial_end=end_date, - ) - price = get(djstripe.Price, id="advanced") - get( - djstripe.SubscriptionItem, - id="si_KOcEsHCktPUedU", - price=price, - quantity=1, - subscription=stripe_subscription, - ) - - subscription = fixture.get( - Subscription, - stripe_id=stripe_subscription.id, - status=SubscriptionStatus.trialing, - ) - Subscription.objects.update_from_stripe( - rtd_subscription=subscription, - stripe_subscription=stripe_subscription, - ) - subscription.refresh_from_db() - self.assertEqual(subscription.status, 'active') - self.assertEqual( - subscription.end_date, - end_date, - ) - self.assertEqual( - subscription.trial_end_date, - end_date, - ) - - # Cancel event - stripe_subscription.status = SubscriptionStatus.unpaid - stripe_subscription.save() - Subscription.objects.update_from_stripe( - rtd_subscription=subscription, - stripe_subscription=stripe_subscription, - ) - subscription.refresh_from_db() - self.assertEqual(subscription.status, 'unpaid') - self.assertEqual( - subscription.trial_end_date, - end_date, - ) - - def test_replace_subscription(self): - """Test update from stripe.""" - start_date = timezone.now() - end_date = timezone.now() + timezone.timedelta(days=30) - stripe_subscription = get( - djstripe.Subscription, - id="sub_bar", - status=SubscriptionStatus.active, - current_period_start=start_date, - current_period_end=end_date, - trial_end=end_date, - ) - price = get(djstripe.Price, id="advanced") - get( - djstripe.SubscriptionItem, - id="si_KOcEsHCktPUedU", - price=price, - quantity=1, - subscription=stripe_subscription, - ) - - subscription = fixture.get( - Subscription, - stripe_id='sub_foo', - status=SubscriptionStatus.trialing, - ) - Subscription.objects.update_from_stripe( - rtd_subscription=subscription, - stripe_subscription=stripe_subscription, - ) - subscription.refresh_from_db() - self.assertEqual(subscription.stripe_id, 'sub_bar') - self.assertEqual(subscription.status, 'active') diff --git a/readthedocs/subscriptions/tests/test_views.py b/readthedocs/subscriptions/tests/test_views.py index afee5b6339b..9088d03a624 100644 --- a/readthedocs/subscriptions/tests/test_views.py +++ b/readthedocs/subscriptions/tests/test_views.py @@ -15,7 +15,6 @@ TYPE_CONCURRENT_BUILDS, TYPE_PRIVATE_DOCS, ) -from readthedocs.subscriptions.models import Plan, Subscription from readthedocs.subscriptions.products import RTDProduct, RTDProductFeature @@ -51,11 +50,6 @@ class SubscriptionViewTests(TestCase): def setUp(self): self.user = get(User) self.organization = get(Organization, stripe_id="123", owners=[self.user]) - self.plan = get( - Plan, - published=True, - stripe_id=settings.RTD_ORG_DEFAULT_STRIPE_SUBSCRIPTION_PRICE, - ) self.stripe_product = get( djstripe.Product, id="prod_a1b2c3", @@ -85,12 +79,6 @@ def setUp(self): self.organization.stripe_customer = self.stripe_customer self.organization.stripe_subscription = self.stripe_subscription self.organization.save() - self.subscription = get( - Subscription, - organization=self.organization, - plan=self.plan, - status='active', - ) self.client.force_login(self.user) def _create_stripe_subscription( @@ -163,85 +151,62 @@ def test_manage_subscription(self, mock_request): fetch_redirect_response=False, ) - @mock.patch( - "readthedocs.subscriptions.utils.stripe.Customer.modify", new=mock.MagicMock - ) - @mock.patch("readthedocs.subscriptions.utils.djstripe.Customer._get_or_retrieve") @mock.patch("readthedocs.subscriptions.utils.stripe.Customer.create") - def test_user_without_subscription( - self, customer_create_mock, customer_retrieve_mock - ): + def test_user_without_subscription(self, customer_create_mock): stripe_subscription = self._create_stripe_subscription() stripe_customer = stripe_subscription.customer stripe_customer.subscribe = mock.MagicMock() stripe_customer.subscribe.return_value = stripe_subscription - customer_retrieve_mock.return_value = stripe_customer self.organization.refresh_from_db() - self.organization.stripe_customer = None + self.organization.stripe_customer = stripe_customer self.organization.stripe_subscription = None self.organization.save() - self.subscription.delete() - self.assertFalse(hasattr(self.organization, 'subscription')) - self.assertIsNone(self.organization.stripe_customer) + self.assertFalse(hasattr(self.organization, "subscription")) self.assertIsNone(self.organization.stripe_subscription) - resp = self.client.get(reverse('subscription_detail', args=[self.organization.slug])) + resp = self.client.get( + reverse("subscription_detail", args=[self.organization.slug]) + ) self.assertEqual(resp.status_code, 200) self.organization.refresh_from_db() - subscription = self.organization.subscription - self.assertEqual(subscription.status, 'active') - self.assertEqual(subscription.stripe_id, 'sub_a1b2c3') self.assertEqual(self.organization.stripe_customer, stripe_customer) self.assertEqual(self.organization.stripe_subscription, stripe_subscription) - customer_retrieve_mock.assert_called_once() customer_create_mock.assert_not_called() @mock.patch( "readthedocs.subscriptions.utils.djstripe.Customer.sync_from_stripe_data" ) - @mock.patch("readthedocs.subscriptions.utils.djstripe.Customer._get_or_retrieve") @mock.patch("readthedocs.subscriptions.utils.stripe.Customer.create") def test_user_without_subscription_and_customer( - self, customer_create_mock, customer_retrieve_mock, sync_from_stripe_data_mock + self, customer_create_mock, sync_from_stripe_data_mock ): stripe_subscription = self._create_stripe_subscription() stripe_customer = stripe_subscription.customer stripe_customer.subscribe = mock.MagicMock() stripe_customer.subscribe.return_value = stripe_subscription - customer_retrieve_mock.return_value = None sync_from_stripe_data_mock.return_value = stripe_customer - # When stripe_id is None, a new customer is created. - self.organization.stripe_id = None + # When stripe_customer is None, a new customer is created. self.organization.stripe_customer = None self.organization.stripe_subscription = None self.organization.save() - self.subscription.delete() self.organization.refresh_from_db() self.assertFalse(hasattr(self.organization, 'subscription')) - self.assertIsNone(self.organization.stripe_id) self.assertIsNone(self.organization.stripe_customer) self.assertIsNone(self.organization.stripe_subscription) - customer_retrieve_mock.reset_mock() resp = self.client.get(reverse('subscription_detail', args=[self.organization.slug])) self.assertEqual(resp.status_code, 200) self.organization.refresh_from_db() - subscription = self.organization.subscription - self.assertEqual(subscription.status, 'active') - self.assertEqual(subscription.stripe_id, 'sub_a1b2c3') self.assertEqual(self.organization.stripe_id, 'cus_a1b2c3') self.assertEqual(self.organization.stripe_customer, stripe_customer) self.assertEqual(self.organization.stripe_subscription, stripe_subscription) customer_create_mock.assert_called_once() - customer_retrieve_mock.assert_not_called() def test_user_with_canceled_subscription(self): - self.subscription.status = 'canceled' self.stripe_subscription.status = SubscriptionStatus.canceled self.stripe_subscription.save() - self.subscription.save() resp = self.client.get(reverse('subscription_detail', args=[self.organization.slug])) self.assertEqual(resp.status_code, 200) self.assertEqual(resp.context["stripe_subscription"], self.stripe_subscription) diff --git a/readthedocs/subscriptions/utils.py b/readthedocs/subscriptions/utils.py index a9929be7203..2d0e984e5c8 100644 --- a/readthedocs/subscriptions/utils.py +++ b/readthedocs/subscriptions/utils.py @@ -3,7 +3,6 @@ import structlog from django.conf import settings from djstripe import models as djstripe -from stripe.error import InvalidRequestError log = structlog.get_logger(__name__) @@ -17,13 +16,6 @@ def create_stripe_customer(organization): metadata=organization.get_stripe_metadata(), ) stripe_customer = djstripe.Customer.sync_from_stripe_data(stripe_data) - if organization.stripe_id: - log.warning( - "Overriding existing stripe customer.", - organization_slug=organization.slug, - previous_stripe_customer=organization.stripe_id, - stripe_customer=stripe_customer.id, - ) organization.stripe_customer = stripe_customer organization.save() return stripe_customer @@ -34,40 +26,13 @@ def get_or_create_stripe_customer(organization): Retrieve the stripe customer or create a new one. If `organization.stripe_customer` is `None`, a new customer is created. - Not all models are migrated to use djstripe yet, - so we retrieve the customer from the stripe_id attribute if the model has one. """ - log.bind( - organization_slug=organization.slug, - stripe_customer=organization.stripe_id, - ) + log.bind(organization_slug=organization.slug) stripe_customer = organization.stripe_customer - if not stripe_customer: - stripe_customer = None - if organization.stripe_id: - try: - # TODO: Don't fully trust on djstripe yet, - # the customer may not be in our DB yet. - stripe_customer = djstripe.Customer._get_or_retrieve( - organization.stripe_id - ) - except InvalidRequestError as exc: - if exc.code == "resource_missing": - log.info("Invalid stripe customer, creating new one.") - - if stripe_customer: - metadata = stripe_customer.metadata or {} - metadata.update(organization.get_stripe_metadata()) - stripe.Customer.modify( - stripe_customer.id, - metadata=metadata, - ) - organization.stripe_customer = stripe_customer - organization.save() - else: - log.info("No stripe customer found, creating one.") - return create_stripe_customer(organization) - return stripe_customer + if stripe_customer: + return stripe_customer + log.info("No stripe customer found, creating one.") + return create_stripe_customer(organization) def get_or_create_stripe_subscription(organization): From 9dd281eb53aedb8d9d5035b28f5cbc12b2f054c6 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 21 Aug 2023 16:24:00 -0500 Subject: [PATCH 02/13] Tests --- .../0012_add_organization_never_disable.py | 32 +++ .../0013_migrate_locked_subscriptions.py | 22 ++ readthedocs/organizations/models.py | 8 + readthedocs/organizations/querysets.py | 107 ++++---- .../organizations/tests/test_querysets.py | 197 ++++++++++++++- readthedocs/subscriptions/tasks.py | 4 +- .../subscriptions/tests/test_daily_email.py | 129 ---------- readthedocs/subscriptions/tests/test_tasks.py | 232 ++++++++++++++++++ 8 files changed, 538 insertions(+), 193 deletions(-) create mode 100644 readthedocs/organizations/migrations/0012_add_organization_never_disable.py create mode 100644 readthedocs/organizations/migrations/0013_migrate_locked_subscriptions.py delete mode 100644 readthedocs/subscriptions/tests/test_daily_email.py create mode 100644 readthedocs/subscriptions/tests/test_tasks.py diff --git a/readthedocs/organizations/migrations/0012_add_organization_never_disable.py b/readthedocs/organizations/migrations/0012_add_organization_never_disable.py new file mode 100644 index 00000000000..6b74972e4dc --- /dev/null +++ b/readthedocs/organizations/migrations/0012_add_organization_never_disable.py @@ -0,0 +1,32 @@ +# Generated by Django 4.2.4 on 2023-08-17 22:58 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("organizations", "0011_add_stripe_subscription_field"), + ] + + operations = [ + migrations.AddField( + model_name="historicalorganization", + name="never_disable", + field=models.BooleanField( + default=False, + help_text="Never disable this organization, even if its subscription ends", + null=True, + verbose_name="Never disable", + ), + ), + migrations.AddField( + model_name="organization", + name="never_disable", + field=models.BooleanField( + default=False, + help_text="Never disable this organization, even if its subscription ends", + null=True, + verbose_name="Never disable", + ), + ), + ] diff --git a/readthedocs/organizations/migrations/0013_migrate_locked_subscriptions.py b/readthedocs/organizations/migrations/0013_migrate_locked_subscriptions.py new file mode 100644 index 00000000000..faed7883e82 --- /dev/null +++ b/readthedocs/organizations/migrations/0013_migrate_locked_subscriptions.py @@ -0,0 +1,22 @@ +# Generated by Django 4.2.4 on 2023-08-21 20:28 + +from django.db import migrations + +def forwards_func(apps, schema_editor): + """Migrate locked subscriptions to never_disable organizations.""" + Subscription = apps.get_model("subscriptions", "Subscription") + locked_subscriptions = Subscription.objects.filter(locked=True).select_related('organization') + for subscription in locked_subscriptions: + subscription.organization.never_disable = True + subscription.organization.save() + + +class Migration(migrations.Migration): + + dependencies = [ + ('organizations', '0012_add_organization_never_disable'), + ] + + operations = [ + migrations.RunPython(forwards_func), + ] diff --git a/readthedocs/organizations/models.py b/readthedocs/organizations/models.py index 3ec41fd09db..9f98f48ef30 100644 --- a/readthedocs/organizations/models.py +++ b/readthedocs/organizations/models.py @@ -73,6 +73,13 @@ class Organization(models.Model): blank=True, null=True, ) + never_disable = models.BooleanField( + _("Never disable"), + help_text="Never disable this organization, even if its subscription ends", + # TODO: remove after migration + null=True, + default=False, + ) disabled = models.BooleanField( _('Disabled'), help_text='Docs and builds are disabled for this organization', @@ -89,6 +96,7 @@ class Organization(models.Model): blank=True, ) + # TODO: This field can be removed, we are now using stripe_customer instead. stripe_id = models.CharField( _('Stripe customer ID'), max_length=100, diff --git a/readthedocs/organizations/querysets.py b/readthedocs/organizations/querysets.py index 73e722550f6..2230f084e87 100644 --- a/readthedocs/organizations/querysets.py +++ b/readthedocs/organizations/querysets.py @@ -6,6 +6,7 @@ from django.db import models from django.db.models import Count, Q from django.utils import timezone +from djstripe.enums import InvoiceStatus, SubscriptionStatus from readthedocs.core.utils.extend import SettingsOverrideObject from readthedocs.subscriptions.constants import DISABLE_AFTER_DAYS @@ -38,62 +39,16 @@ def created_days_ago(self, days, field='pub_date'): query_filter[field + '__day'] = when.day return self.filter(**query_filter) - # TODO: once we are settled into the Trial Plan, merge this method with the - # following one (``subscription_trial_ended``). def subscription_trial_plan_ended(self): """ Organizations with subscriptions to Trial Plan ended. Trial Plan in Stripe has a 30-day trial set up. After that period ends, - the subscription goes to ``active`` and we know that the trial ended. - - It also checks that ``end_date`` or ``trial_end_date`` are in the past. - """ - date_now = timezone.now() - return self.filter( - ~Q(subscription__status="trialing"), - Q( - subscription__plan__stripe_id=settings.RTD_ORG_DEFAULT_STRIPE_SUBSCRIPTION_PRICE - ), - Q(subscription__end_date__lt=date_now) - | Q(subscription__trial_end_date__lt=date_now), - ) - - def subscription_trial_ended(self): + the subscription is canceled. """ - Organizations with subscriptions in trial ended state. - - Filter for organization subscription past the trial date, or - organizations older than 30 days old - """ - date_now = timezone.now() return self.filter( - Q( - ( - Q( - subscription__status='trialing', - ) | Q( - subscription__status__exact='', - ) - ), - subscription__trial_end_date__lt=date_now, - ) | Q( - subscription__isnull=True, - pub_date__lt=date_now - timedelta(days=30), - ), - ) - - def subscription_ended(self): - """ - Organization with paid subscriptions ended. - - Filter for organization with paid subscriptions that have - ended (canceled, past_due or unpaid) and they didn't renew them yet. - - https://stripe.com/docs/api#subscription_object-status - """ - return self.filter( - subscription__status__in=['canceled', 'past_due', 'unpaid'], + stripe_subscription__status=SubscriptionStatus.canceled, + stripe_subscription__items__price__id=settings.RTD_ORG_DEFAULT_STRIPE_SUBSCRIPTION_PRICE, ) def disable_soon(self, days, exact=False): @@ -103,6 +58,9 @@ def disable_soon(self, days, exact=False): This will return organizations that the paid/trial subscription has ended ``days`` ago. + Organizations to be disabled are which their subscription has been canceled, + or hasn't been paid for ``days``. + :param days: Days after the subscription has ended :param exact: Make the ``days`` date to match exactly that day after the subscription has ended (useful to send emails only once) @@ -112,20 +70,45 @@ def disable_soon(self, days, exact=False): if exact: # We use ``__date`` here since the field is a DateTimeField - trial_filter = {'subscription__trial_end_date__date': end_date} - paid_filter = {'subscription__end_date__date': end_date} + subscription_ended = self.filter( + Q( + stripe_subscription__status=SubscriptionStatus.canceled, + stripe_subscription__ended_at__date=end_date, + ) + | Q( + stripe_subscription__status__in=[ + SubscriptionStatus.past_due, + SubscriptionStatus.incomplete, + SubscriptionStatus.unpaid, + ], + stripe_subscription__latest_invoice__due_date__date=end_date, + stripe_subscription__latest_invoice__status=InvoiceStatus.open, + ) + ) else: - trial_filter = {'subscription__trial_end_date__lt': end_date} - paid_filter = {'subscription__end_date__lt': end_date} - - trial_ended = self.subscription_trial_ended().filter(**trial_filter) - paid_ended = self.subscription_ended().filter(**paid_filter) - - # Exclude organizations with custom plans (locked=True) - orgs = (trial_ended | paid_ended).exclude(subscription__locked=True) - - # Exclude organizations that are already disabled - orgs = orgs.exclude(disabled=True) + subscription_ended = self.filter( + Q( + stripe_subscription__status=SubscriptionStatus.canceled, + stripe_subscription__ended_at__lt=end_date, + ) + | Q( + stripe_subscription__status__in=[ + SubscriptionStatus.past_due, + SubscriptionStatus.incomplete, + SubscriptionStatus.unpaid, + ], + stripe_subscription__latest_invoice__due_date__date__lt=end_date, + stripe_subscription__latest_invoice__status=InvoiceStatus.open, + ) + ) + + orgs = ( + subscription_ended + # Exclude organizations that can't be disabled. + .exclude(never_disable=True) + # Exclude organizations that are already disabled + .exclude(disabled=True) + ) return orgs.distinct() diff --git a/readthedocs/organizations/tests/test_querysets.py b/readthedocs/organizations/tests/test_querysets.py index cfeb52c4e80..1d48fcacc59 100644 --- a/readthedocs/organizations/tests/test_querysets.py +++ b/readthedocs/organizations/tests/test_querysets.py @@ -1,10 +1,19 @@ +from datetime import timedelta + from django.contrib.auth.models import User -from django.test import TestCase +from django.test import TestCase, override_settings +from django.utils import timezone from django_dynamic_fixture import get +from djstripe import models as djstripe +from djstripe.enums import InvoiceStatus, SubscriptionStatus from readthedocs.organizations.models import Organization +@override_settings( + RTD_ALLOW_ORGANIZATIONS=True, + RTD_ORG_DEFAULT_STRIPE_SUBSCRIPTION_PRICE="trialing", +) class TestOrganizationQuerysets(TestCase): def test_only_owner(self): user = get(User) @@ -22,3 +31,189 @@ def test_only_owner(self): self.assertEqual( {org_three}, set(Organization.objects.single_owner(another_user)) ) + + def test_organizations_with_trial_subscription_plan_ended(self): + price = get(djstripe.Price, id="trialing") + + stripe_subscription1 = get( + djstripe.Subscription, + status=SubscriptionStatus.active, + customer=get(djstripe.Customer), + ) + get( + djstripe.SubscriptionItem, + price=price, + quantity=1, + subscription=stripe_subscription1, + ) + + org1 = get( + Organization, + stripe_subscription=stripe_subscription1, + stripe_customer=stripe_subscription1.customer, + ) + + stripe_subscription2 = get( + djstripe.Subscription, + status=SubscriptionStatus.canceled, + customer=get(djstripe.Customer), + ) + get( + djstripe.SubscriptionItem, + price=price, + quantity=1, + subscription=stripe_subscription2, + ) + org2 = get( + Organization, + stripe_subscription=stripe_subscription2, + stripe_customer=stripe_subscription2.customer, + ) + + self.assertEqual( + list(Organization.objects.subscription_trial_plan_ended()), [org2] + ) + + def test_organizations_to_be_disabled(self): + subscription1 = get( + djstripe.Subscription, + customer=get(djstripe.Customer), + status=SubscriptionStatus.active, + ) + organization_active = get( + Organization, + stripe_subscription=subscription1, + stripe_customer=subscription1.customer, + ) + + subscription2 = get( + djstripe.Subscription, + customer=get(djstripe.Customer), + status=SubscriptionStatus.canceled, + ended_at=timezone.now() - timedelta(days=30), + ) + organization_canceled_30_days_ago = get( + Organization, + stripe_subscription=subscription2, + stripe_customer=subscription2.customer, + ) + + subscription3 = get( + djstripe.Subscription, + customer=get(djstripe.Customer), + status=SubscriptionStatus.canceled, + ended_at=timezone.now(), + ) + organization_canceled_now = get( + Organization, + stripe_subscription=subscription3, + stripe_customer=subscription3.customer, + ) + + subscription4 = get( + djstripe.Subscription, + customer=get(djstripe.Customer), + status=SubscriptionStatus.canceled, + ended_at=timezone.now() - timedelta(days=35), + ) + organization_canceled_35_days_ago = get( + Organization, + stripe_subscription=subscription4, + stripe_customer=subscription4.customer, + ) + + latest_invoice1 = get( + djstripe.Invoice, + due_date=timezone.now() + timedelta(days=30), + status=InvoiceStatus.open, + ) + subscription5 = get( + djstripe.Subscription, + customer=get(djstripe.Customer), + status=SubscriptionStatus.past_due, + latest_invoice=latest_invoice1, + ) + organization_past_due_in_30_days = get( + Organization, + stripe_subscription=subscription5, + stripe_customer=subscription5.customer, + ) + + latest_invoice2 = get( + djstripe.Invoice, + due_date=timezone.now() - timedelta(days=30), + status=InvoiceStatus.open, + ) + subscription6 = get( + djstripe.Subscription, + customer=get(djstripe.Customer), + status=SubscriptionStatus.past_due, + latest_invoice=latest_invoice2, + ) + organization_past_due_30_days_ago = get( + Organization, + stripe_subscription=subscription6, + stripe_customer=subscription6.customer, + ) + + latest_invoice3 = get( + djstripe.Invoice, + due_date=timezone.now() - timedelta(days=35), + status=InvoiceStatus.open, + ) + subscription7 = get( + djstripe.Subscription, + customer=get(djstripe.Customer), + status=SubscriptionStatus.past_due, + latest_invoice=latest_invoice3, + ) + organization_past_due_35_days_ago = get( + Organization, + stripe_subscription=subscription7, + stripe_customer=subscription7.customer, + ) + + self.assertEqual( + set(Organization.objects.disable_soon(days=30, exact=False)), + {organization_canceled_35_days_ago, organization_past_due_35_days_ago}, + ) + + self.assertEqual( + set(Organization.objects.disable_soon(days=20, exact=False)), + { + organization_canceled_30_days_ago, + organization_canceled_35_days_ago, + organization_past_due_35_days_ago, + organization_past_due_30_days_ago, + }, + ) + + self.assertEqual( + set(Organization.objects.disable_soon(days=30, exact=True)), + {organization_canceled_30_days_ago, organization_past_due_30_days_ago}, + ) + + self.assertEqual( + set(Organization.objects.disable_soon(days=35, exact=True)), + {organization_canceled_35_days_ago, organization_past_due_35_days_ago}, + ) + + self.assertEqual( + set(Organization.objects.disable_soon(days=20, exact=True)), + set(), + ) + + organization_past_due_30_days_ago.disabled = True + organization_past_due_30_days_ago.save() + self.assertEqual( + set(Organization.objects.disable_soon(days=30, exact=False)), + {organization_canceled_35_days_ago, organization_past_due_35_days_ago}, + ) + + organization_past_due_30_days_ago.disabled = False + organization_past_due_30_days_ago.never_disable = True + organization_past_due_30_days_ago.save() + self.assertEqual( + set(Organization.objects.disable_soon(days=30, exact=False)), + {organization_canceled_35_days_ago, organization_past_due_35_days_ago}, + ) diff --git a/readthedocs/subscriptions/tasks.py b/readthedocs/subscriptions/tasks.py index c8fa8ba10ee..e843a5c09be 100644 --- a/readthedocs/subscriptions/tasks.py +++ b/readthedocs/subscriptions/tasks.py @@ -62,7 +62,9 @@ def daily_email(): @app.task(queue="web") def disable_organization_expired_trials(): """Daily task to disable organization with expired Trial Plans.""" - queryset = Organization.objects.subscription_trial_plan_ended() + queryset = Organization.objects.disable_soon( + days=30, exact=True + ).subscription_trial_plan_ended() for organization in queryset: log.info( diff --git a/readthedocs/subscriptions/tests/test_daily_email.py b/readthedocs/subscriptions/tests/test_daily_email.py deleted file mode 100644 index b30c99f74cb..00000000000 --- a/readthedocs/subscriptions/tests/test_daily_email.py +++ /dev/null @@ -1,129 +0,0 @@ -from datetime import timedelta -from unittest import mock - -import django_dynamic_fixture as fixture -from django.contrib.auth.models import User -from django.test import TestCase -from django.utils import timezone -from django_dynamic_fixture import get -from djstripe import models as djstripe -from djstripe.enums import SubscriptionStatus - -from readthedocs.organizations.models import Organization, OrganizationOwner -from readthedocs.subscriptions.models import Subscription -from readthedocs.subscriptions.tasks import daily_email - - -@mock.patch("readthedocs.notifications.backends.send_email") -@mock.patch("readthedocs.notifications.storages.FallbackUniqueStorage") -class DailyEmailTests(TestCase): - def test_trial_ending(self, mock_storage_class, mock_send_email): - """Trial ending daily email.""" - now = timezone.now() - - owner1 = get(User) - org1 = get(Organization, owners=[owner1]) - customer1 = get(djstripe.Customer) - org1.stripe_customer = customer1 - org1.save() - get( - djstripe.Subscription, - status=SubscriptionStatus.trialing, - trial_start=now, - trial_end=now + timedelta(days=7), - created=now - timedelta(days=24), - customer=customer1, - ) - - owner2 = get(User) - org2 = fixture.get(Organization, owners=[owner2]) - customer2 = get(djstripe.Customer) - org2.stripe_customer = customer2 - org2.save() - get( - djstripe.Subscription, - status=SubscriptionStatus.trialing, - trial_start=now, - trial_end=now + timedelta(days=7), - created=now - timedelta(days=25), - ) - - mock_storage = mock.Mock() - mock_storage_class.return_value = mock_storage - - daily_email() - - self.assertEqual(mock_storage.add.call_count, 1) - mock_storage.add.assert_has_calls( - [ - mock.call( - message=mock.ANY, - extra_tags="", - level=31, - user=owner1, - ), - ] - ) - self.assertEqual(mock_send_email.call_count, 1) - mock_send_email.assert_has_calls( - [ - mock.call( - subject="Your trial is ending soon", - recipient=owner1.email, - template=mock.ANY, - template_html=mock.ANY, - context=mock.ANY, - ), - ] - ) - - def test_organization_disabled(self, mock_storage_class, mock_send_email): - """Subscription ended ``DISABLE_AFTER_DAYS`` days ago daily email.""" - sub1 = fixture.get( - Subscription, - status="past_due", - end_date=timezone.now() - timedelta(days=30), - ) - owner1 = fixture.get( - OrganizationOwner, - organization=sub1.organization, - ) - - sub2 = fixture.get( - Subscription, - status="past_due", - end_date=timezone.now() - timedelta(days=35), - ) - owner2 = fixture.get( - OrganizationOwner, - organization=sub2.organization, - ) - - mock_storage = mock.Mock() - mock_storage_class.return_value = mock_storage - - daily_email() - - self.assertEqual(mock_storage.add.call_count, 1) - mock_storage.add.assert_has_calls( - [ - mock.call( - message=mock.ANY, - extra_tags="", - level=31, - user=owner1.owner, - ), - ] - ) - self.assertEqual(mock_send_email.call_count, 1) - mock_send_email.assert_has_calls( - [ - mock.call( - subject="Your Read the Docs organization will be disabled soon", - recipient=owner1.owner.email, - template=mock.ANY, - template_html=mock.ANY, - context=mock.ANY, - ), - ] - ) diff --git a/readthedocs/subscriptions/tests/test_tasks.py b/readthedocs/subscriptions/tests/test_tasks.py new file mode 100644 index 00000000000..13c71050f19 --- /dev/null +++ b/readthedocs/subscriptions/tests/test_tasks.py @@ -0,0 +1,232 @@ +from datetime import timedelta +from unittest import mock + +import django_dynamic_fixture as fixture +from django.contrib.auth.models import User +from django.test import TestCase, override_settings +from django.utils import timezone +from django_dynamic_fixture import get +from djstripe import models as djstripe +from djstripe.enums import InvoiceStatus, SubscriptionStatus + +from readthedocs.organizations.models import Organization +from readthedocs.subscriptions.tasks import ( + daily_email, + disable_organization_expired_trials, +) + + +@override_settings( + RTD_ALLOW_ORGANIZATIONS=True, + RTD_ORG_DEFAULT_STRIPE_SUBSCRIPTION_PRICE="trialing", +) +@mock.patch("readthedocs.notifications.backends.send_email") +@mock.patch("readthedocs.notifications.storages.FallbackUniqueStorage") +class DailyEmailTests(TestCase): + def test_trial_ending(self, mock_storage_class, mock_send_email): + """Trial ending daily email.""" + now = timezone.now() + + owner1 = get(User) + org1 = get(Organization, owners=[owner1]) + customer1 = get(djstripe.Customer) + org1.stripe_customer = customer1 + org1.save() + get( + djstripe.Subscription, + status=SubscriptionStatus.trialing, + trial_start=now, + trial_end=now + timedelta(days=7), + created=now - timedelta(days=24), + customer=customer1, + ) + + owner2 = get(User) + org2 = fixture.get(Organization, owners=[owner2]) + customer2 = get(djstripe.Customer) + org2.stripe_customer = customer2 + org2.save() + get( + djstripe.Subscription, + status=SubscriptionStatus.trialing, + trial_start=now, + trial_end=now + timedelta(days=7), + created=now - timedelta(days=25), + ) + + mock_storage = mock.Mock() + mock_storage_class.return_value = mock_storage + + daily_email() + + self.assertEqual(mock_storage.add.call_count, 1) + mock_storage.add.assert_has_calls( + [ + mock.call( + message=mock.ANY, + extra_tags="", + level=31, + user=owner1, + ), + ] + ) + self.assertEqual(mock_send_email.call_count, 1) + mock_send_email.assert_has_calls( + [ + mock.call( + subject="Your trial is ending soon", + recipient=owner1.email, + template=mock.ANY, + template_html=mock.ANY, + context=mock.ANY, + ), + ] + ) + + def test_organizations_to_be_disable_email( + self, mock_storage_class, mock_send_email + ): + """Subscription ended ``DISABLE_AFTER_DAYS`` days ago daily email.""" + customer1 = get(djstripe.Customer) + latest_invoice1 = get( + djstripe.Invoice, + due_date=timezone.now() - timedelta(days=30), + status=InvoiceStatus.open, + ) + sub1 = get( + djstripe.Subscription, + status=SubscriptionStatus.past_due, + latest_invoice=latest_invoice1, + customer=customer1, + ) + owner1 = get(User) + get( + Organization, + owners=[owner1], + stripe_customer=customer1, + stripe_subscription=sub1, + ) + + customer2 = get(djstripe.Customer) + latest_invoice2 = get( + djstripe.Invoice, + due_date=timezone.now() - timedelta(days=35), + status=InvoiceStatus.open, + ) + sub2 = get( + djstripe.Subscription, + status=SubscriptionStatus.past_due, + latest_invoice=latest_invoice2, + customer=customer2, + ) + owner2 = get(User) + get( + Organization, + owners=[owner2], + stripe_customer=customer2, + stripe_subscription=sub2, + ) + + mock_storage = mock.Mock() + mock_storage_class.return_value = mock_storage + + daily_email() + + self.assertEqual(mock_storage.add.call_count, 1) + mock_storage.add.assert_has_calls( + [ + mock.call( + message=mock.ANY, + extra_tags="", + level=31, + user=owner1, + ), + ] + ) + self.assertEqual(mock_send_email.call_count, 1) + mock_send_email.assert_has_calls( + [ + mock.call( + subject="Your Read the Docs organization will be disabled soon", + recipient=owner1.email, + template=mock.ANY, + template_html=mock.ANY, + context=mock.ANY, + ), + ] + ) + + +@override_settings( + RTD_ALLOW_ORGANIZATIONS=True, + RTD_ORG_DEFAULT_STRIPE_SUBSCRIPTION_PRICE="trialing", +) +class SubscriptionTasksTests(TestCase): + def test_disable_organizations_with_expired_trial(self): + price = get(djstripe.Price, id="trialing") + + # Active trial subscription + subscription1 = get( + djstripe.Subscription, + status=SubscriptionStatus.active, + customer=get(djstripe.Customer), + ) + get( + djstripe.SubscriptionItem, + price=price, + quantity=1, + subscription=subscription1, + ) + organization_with_active_subscription = get( + Organization, + stripe_subscription=subscription1, + stripe_customer=subscription1.customer, + ) + + # Canceled trial subscription + subscription2 = get( + djstripe.Subscription, + status=SubscriptionStatus.canceled, + customer=get(djstripe.Customer), + ended_at=timezone.now() - timedelta(days=30), + ) + get( + djstripe.SubscriptionItem, + price=price, + quantity=1, + subscription=subscription2, + ) + organization_with_canceled_trial_subscription = get( + Organization, + stripe_subscription=subscription2, + stripe_customer=subscription2.customer, + ) + + # Canceled subscription + subscription3 = get( + djstripe.Subscription, + status=SubscriptionStatus.canceled, + customer=get(djstripe.Customer), + ended_at=timezone.now() - timedelta(days=30), + ) + get( + djstripe.SubscriptionItem, + quantity=1, + subscription=subscription3, + ) + organization_with_canceled_subscription = get( + Organization, + stripe_subscription=subscription3, + stripe_customer=subscription3.customer, + ) + + disable_organization_expired_trials() + + organization_with_active_subscription.refresh_from_db() + self.assertFalse(organization_with_active_subscription.disabled) + + organization_with_canceled_trial_subscription.refresh_from_db() + self.assertTrue(organization_with_canceled_trial_subscription.disabled) + + organization_with_canceled_subscription.refresh_from_db() + self.assertFalse(organization_with_canceled_subscription.disabled) From f95a0f331e4b3bff26aea440274a5cc3da0cdab5 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 21 Aug 2023 16:36:56 -0500 Subject: [PATCH 03/13] Format --- .../migrations/0013_migrate_locked_subscriptions.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/readthedocs/organizations/migrations/0013_migrate_locked_subscriptions.py b/readthedocs/organizations/migrations/0013_migrate_locked_subscriptions.py index faed7883e82..a5c7f260838 100644 --- a/readthedocs/organizations/migrations/0013_migrate_locked_subscriptions.py +++ b/readthedocs/organizations/migrations/0013_migrate_locked_subscriptions.py @@ -2,19 +2,21 @@ from django.db import migrations + def forwards_func(apps, schema_editor): """Migrate locked subscriptions to never_disable organizations.""" Subscription = apps.get_model("subscriptions", "Subscription") - locked_subscriptions = Subscription.objects.filter(locked=True).select_related('organization') + locked_subscriptions = Subscription.objects.filter(locked=True).select_related( + "organization" + ) for subscription in locked_subscriptions: subscription.organization.never_disable = True subscription.organization.save() class Migration(migrations.Migration): - dependencies = [ - ('organizations', '0012_add_organization_never_disable'), + ("organizations", "0012_add_organization_never_disable"), ] operations = [ From 3d0ca9cdf988f04b59cfab970213859d3cca8e2c Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 21 Aug 2023 17:50:49 -0500 Subject: [PATCH 04/13] Fix migration --- .../migrations/0013_migrate_locked_subscriptions.py | 1 + 1 file changed, 1 insertion(+) diff --git a/readthedocs/organizations/migrations/0013_migrate_locked_subscriptions.py b/readthedocs/organizations/migrations/0013_migrate_locked_subscriptions.py index a5c7f260838..fe77895c599 100644 --- a/readthedocs/organizations/migrations/0013_migrate_locked_subscriptions.py +++ b/readthedocs/organizations/migrations/0013_migrate_locked_subscriptions.py @@ -17,6 +17,7 @@ def forwards_func(apps, schema_editor): class Migration(migrations.Migration): dependencies = [ ("organizations", "0012_add_organization_never_disable"), + ("subscriptions", "0002_alter_planfeature_feature_type"), ] operations = [ From 0bd7208c8541c6f81e3129669f94bc64643d71bf Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 21 Aug 2023 18:57:54 -0500 Subject: [PATCH 05/13] Fixes --- readthedocs/organizations/querysets.py | 34 +++++++++++++++----------- readthedocs/organizations/signals.py | 8 +++--- 2 files changed, 25 insertions(+), 17 deletions(-) diff --git a/readthedocs/organizations/querysets.py b/readthedocs/organizations/querysets.py index 2230f084e87..1396cb57b38 100644 --- a/readthedocs/organizations/querysets.py +++ b/readthedocs/organizations/querysets.py @@ -51,14 +51,11 @@ def subscription_trial_plan_ended(self): stripe_subscription__items__price__id=settings.RTD_ORG_DEFAULT_STRIPE_SUBSCRIPTION_PRICE, ) - def disable_soon(self, days, exact=False): + def subscription_ended(self, days, exact=False): """ - Filter organizations that will eventually be marked as disabled. + Filter organizations which their subscription has ended. - This will return organizations that the paid/trial subscription has - ended ``days`` ago. - - Organizations to be disabled are which their subscription has been canceled, + This will return organizations which their subscription has been canceled, or hasn't been paid for ``days``. :param days: Days after the subscription has ended @@ -102,16 +99,27 @@ def disable_soon(self, days, exact=False): ) ) - orgs = ( - subscription_ended + return subscription_ended.distinct() + + def disable_soon(self, days, exact=False): + """ + Filter organizations that will eventually be marked as disabled. + + These are organizations which their subscription has ended, + excluding organizations that can't be disabled, or are already disabled. + + :param days: Days after the subscription has ended + :param exact: Make the ``days`` date to match exactly that day after the + subscription has ended (useful to send emails only once) + """ + return ( + self.subscription_ended(days=days, exact=exact) # Exclude organizations that can't be disabled. .exclude(never_disable=True) # Exclude organizations that are already disabled .exclude(disabled=True) ) - return orgs.distinct() - def clean_artifacts(self): """ Filter organizations which their artifacts can be cleaned up. @@ -120,13 +128,11 @@ def clean_artifacts(self): are disabled and their artifacts weren't cleaned already. We should be safe to cleanup all their artifacts at this point. """ - end_date = timezone.now().date() - timedelta(days=3 * DISABLE_AFTER_DAYS) - orgs = self.filter( + return self.subscription_ended(days=3 * DISABLE_AFTER_DAYS, exact=True).filter( disabled=True, - subscription__end_date__lt=end_date, artifacts_cleaned=False, ) - return orgs.distinct() + def single_owner(self, user): """Returns organizations where `user` is the only owner.""" diff --git a/readthedocs/organizations/signals.py b/readthedocs/organizations/signals.py index e0d1ad1ac16..3dad30b69ae 100644 --- a/readthedocs/organizations/signals.py +++ b/readthedocs/organizations/signals.py @@ -1,5 +1,6 @@ """Organization signals.""" +from djstripe.enums import SubscriptionStatus import structlog from allauth.account.signals import user_signed_up from django.db.models import Count @@ -10,7 +11,7 @@ from readthedocs.builds.models import Build from readthedocs.builds.signals import build_complete from readthedocs.organizations.models import Organization, Team, TeamMember -from readthedocs.payments.utils import delete_customer +from readthedocs.payments.utils import cancel_subscription from readthedocs.projects.models import Project from .tasks import ( @@ -52,11 +53,12 @@ def remove_organization_completely(sender, instance, using, **kwargs): stripe_customer = organization.stripe_customer if stripe_customer: log.info( - "Removing Stripe customer", + "Canceling subscriptions", organization_slug=organization.slug, stripe_customer_id=stripe_customer.id, ) - delete_customer(stripe_customer.id) + for subscription in stripe_customer.subscriptions.exclude(status=SubscriptionStatus.canceled): + cancel_subscription(subscription.id) log.info("Removing organization completely", organization_slug=organization.slug) From 8b896aad2c1089dffcbdb7f1631ef7b8971c2462 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 21 Aug 2023 19:22:30 -0500 Subject: [PATCH 06/13] Format --- readthedocs/organizations/signals.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/readthedocs/organizations/signals.py b/readthedocs/organizations/signals.py index 3dad30b69ae..165a62b049b 100644 --- a/readthedocs/organizations/signals.py +++ b/readthedocs/organizations/signals.py @@ -1,11 +1,11 @@ """Organization signals.""" -from djstripe.enums import SubscriptionStatus import structlog from allauth.account.signals import user_signed_up from django.db.models import Count from django.db.models.signals import pre_delete from django.dispatch import receiver +from djstripe.enums import SubscriptionStatus from readthedocs.builds.constants import BUILD_FINAL_STATES from readthedocs.builds.models import Build @@ -57,7 +57,9 @@ def remove_organization_completely(sender, instance, using, **kwargs): organization_slug=organization.slug, stripe_customer_id=stripe_customer.id, ) - for subscription in stripe_customer.subscriptions.exclude(status=SubscriptionStatus.canceled): + for subscription in stripe_customer.subscriptions.exclude( + status=SubscriptionStatus.canceled + ): cancel_subscription(subscription.id) log.info("Removing organization completely", organization_slug=organization.slug) From 8e02420b4f5e73af67f9e9b104b6925449e8229d Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 21 Aug 2023 19:48:08 -0500 Subject: [PATCH 07/13] Don't create subscription when checking for feature --- readthedocs/subscriptions/products.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/subscriptions/products.py b/readthedocs/subscriptions/products.py index af27da70cd0..09e022ae073 100644 --- a/readthedocs/subscriptions/products.py +++ b/readthedocs/subscriptions/products.py @@ -134,7 +134,7 @@ def get_feature(obj, feature_type) -> RTDProductFeature: available_stripe_products_id = [ product.stripe_id for product in get_products_with_feature(feature_type) ] - stripe_subscription = organization.get_or_create_stripe_subscription() + stripe_subscription = organization.stripe_subscription if stripe_subscription: subscription_items = stripe_subscription.items.filter( price__product__id__in=available_stripe_products_id From 2a38f29421f6ad09223d4efe3b8825337250a411 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 21 Aug 2023 20:06:45 -0500 Subject: [PATCH 08/13] Include requirements in cache key --- .circleci/config.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 6a6176f6d72..7d6985e70bb 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -45,10 +45,11 @@ jobs: - run: git submodule update --init # Recipe: https://pre-commit.com/#managing-ci-caches - run: - name: Combine pre-commit config and python versions for caching + name: Combine pre-commit config, python version and testing requirements for caching command: | cp common/pre-commit-config.yaml pre-commit-cache-key.txt python --version --version >> pre-commit-cache-key.txt + cat requirements/testing.txt >> pre-commit-cache-key.txt - restore_cache: keys: - pre-commit-cache-{{ checksum "pre-commit-cache-key.txt" }} From 6ec0c22d1ecda7c54ddf95157a9d0dd1ffc21931 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 22 Aug 2023 15:36:01 -0500 Subject: [PATCH 09/13] Check for created subscriptions --- readthedocs/organizations/models.py | 16 +++- readthedocs/subscriptions/event_handlers.py | 76 ++++++++++++++++++- .../tests/test_event_handlers.py | 43 +++++++++-- 3 files changed, 122 insertions(+), 13 deletions(-) diff --git a/readthedocs/organizations/models.py b/readthedocs/organizations/models.py index 9f98f48ef30..1c20ac097ba 100644 --- a/readthedocs/organizations/models.py +++ b/readthedocs/organizations/models.py @@ -139,11 +139,19 @@ def get_or_create_stripe_subscription(self): def get_stripe_subscription(self): # Active subscriptions take precedence over non-active subscriptions, # otherwise we return the most recently created subscription. - active_subscription = self.stripe_customer.subscriptions.filter( + active_subscriptions = self.stripe_customer.subscriptions.filter( status=SubscriptionStatus.active - ).first() - if active_subscription: - return active_subscription + ) + if active_subscriptions: + if active_subscriptions.count() > 1: + # NOTE: this should never happen, unless we manually + # created another subscription for the user or if there + # is a bug in our code. + log.exception( + "Organization has more than one active subscription", + organization_slug=self.slug, + ) + return active_subscriptions.order_by("created").last() return self.stripe_customer.subscriptions.order_by("created").last() def get_absolute_url(self): diff --git a/readthedocs/subscriptions/event_handlers.py b/readthedocs/subscriptions/event_handlers.py index 0781d256ce2..31cd94ed034 100644 --- a/readthedocs/subscriptions/event_handlers.py +++ b/readthedocs/subscriptions/event_handlers.py @@ -36,16 +36,63 @@ def decorator(func): return decorator +@handler("customer.subscription.created") +def subscription_created_event(event): + """ + Handle the creation of a new subscription. + + When a new subscription is created, the latest subscription + of the organization is updated to point to this new subscription. + + If the organization attached to the customer is disabled, + we re-enable it, since the user just subscribed to a plan. + """ + stripe_subscription_id = event.data["object"]["id"] + log.bind(stripe_subscription_id=stripe_subscription_id) + + stripe_subscription = djstripe.Subscription.objects.filter( + id=stripe_subscription_id + ).first() + if not stripe_subscription: + log.info("Stripe subscription not found.") + return + + organization = getattr(stripe_subscription.customer, "rtd_organization", None) + if not organization: + log.error("Subscription isn't attached to an organization") + return + + if organization.disabled: + log.info("Re-enabling organization.", organization_slug=organization.slug) + organization.disabled = False + + old_subscription_id = ( + organization.stripe_subscription.id + if organization.stripe_subscription + else None + ) + log.info( + "Attaching new subscription to organization.", + organization_slug=organization.slug, + old_stripe_subscription_id=old_subscription_id, + ) + organization.stripe_subscription = stripe_subscription + organization.save() + + @handler("customer.subscription.updated", "customer.subscription.deleted") -def update_subscription(event): +def subscription_updated_event(event): """ - Cancel trial subscriptions if their trial has ended. + Handle subscription updates. - We need to cancel these subscriptions manually, + We need to cancel trial subscriptions manually when their trial period ends, otherwise Stripe will keep them active. If the organization attached to the subscription is disabled, - and the subscription is active, we re-enable the organization. + and the subscription is now active, we re-enable the organization. + + We also re-evaluate the latest subscription attached to the organization, + it case it changed. """ stripe_subscription_id = event.data["object"]["id"] log.bind(stripe_subscription_id=stripe_subscription_id) @@ -71,20 +118,41 @@ def update_subscription(event): "Trial ended, canceling subscription.", ) cancel_stripe_subscription(stripe_subscription.id) + return organization = getattr(stripe_subscription.customer, "rtd_organization", None) if not organization: log.error("Subscription isn't attached to an organization") return + save_org = False if ( stripe_subscription.status == SubscriptionStatus.active and organization.disabled ): log.info("Re-enabling organization.", organization_slug=organization.slug) organization.disabled = False + save_org = True + + new_stripe_subscription = organization.get_stripe_subscription() + if organization.stripe_subscription != new_stripe_subscription: + old_subscription_id = ( + organization.stripe_subscription.id + if organization.stripe_subscription + else None + ) + log.info( + "Attaching new subscription to organization.", + organization_slug=organization.slug, + old_stripe_subscription_id=old_subscription_id, + ) + organization.stripe_subscription = stripe_subscription + save_org = True + + if save_org: organization.save() + @handler("customer.subscription.deleted") def subscription_canceled(event): """ diff --git a/readthedocs/subscriptions/tests/test_event_handlers.py b/readthedocs/subscriptions/tests/test_event_handlers.py index 0e52f76f4c4..dc691b4fa83 100644 --- a/readthedocs/subscriptions/tests/test_event_handlers.py +++ b/readthedocs/subscriptions/tests/test_event_handlers.py @@ -26,6 +26,39 @@ def setUp(self): Organization, slug="org", email="test@example.com", owners=[self.user] ) + def test_subscription_created_event(self): + customer = get(djstripe.Customer, id="cus_KMiHJXFHpLkcRP") + self.organization.stripe_customer = customer + self.organization.save() + + start_date = timezone.now() + end_date = timezone.now() + timezone.timedelta(days=30) + stripe_subscription = get( + djstripe.Subscription, + id="sub_9LtsU02uvjO6Ed", + status=SubscriptionStatus.active, + current_period_start=start_date, + current_period_end=end_date, + trial_end=end_date, + customer=customer, + ) + event = get( + djstripe.Event, + data={ + "object": { + "id": stripe_subscription.id, + "object": "subscription", + "customer": customer.id, + } + }, + ) + + self.assertIsNone(self.organization.stripe_subscription) + event_handlers.subscription_created_event(event=event) + + self.organization.refresh_from_db() + self.assertEqual(self.organization.stripe_subscription, stripe_subscription) + @mock.patch("readthedocs.subscriptions.event_handlers.cancel_stripe_subscription") def test_subscription_updated_event(self, cancel_subscription_mock): """Test handled event.""" @@ -48,10 +81,10 @@ def test_subscription_updated_event(self, cancel_subscription_mock): } }, ) - event_handlers.update_subscription(event=event) + event_handlers.subscription_updated_event(event=event) cancel_subscription_mock.assert_not_called() - def test_reenabled_organization_on_subscription_updated_event(self): + def test_reenable_organization_on_subscription_updated_event(self): """Organization is re-enabled when subscription is active.""" customer = get(djstripe.Customer, id="cus_KMiHJXFHpLkcRP") self.organization.stripe_customer = customer @@ -80,7 +113,7 @@ def test_reenabled_organization_on_subscription_updated_event(self): ) self.assertTrue(self.organization.disabled) - event_handlers.update_subscription(event=event) + event_handlers.subscription_updated_event(event=event) self.organization.refresh_from_db() self.assertFalse(self.organization.disabled) @@ -184,7 +217,7 @@ def test_cancel_trial_subscription_after_trial_has_ended( }, ) - event_handlers.update_subscription(event=event) + event_handlers.subscription_updated_event(event=event) cancel_subscription_mock.assert_called_once_with(stripe_subscription.id) @mock.patch("readthedocs.subscriptions.event_handlers.cancel_stripe_subscription") @@ -219,7 +252,7 @@ def test_dont_cancel_normal_subscription_after_trial_has_ended( }, ) - event_handlers.update_subscription(event=event) + event_handlers.subscription_updated_event(event=event) cancel_subscription_mock.assert_not_called() def test_customer_updated_event(self): From ee1688613c923852b62a91bdd3401f434c9f8ec1 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 24 Aug 2023 12:48:48 -0500 Subject: [PATCH 10/13] Updates from review --- readthedocs/organizations/models.py | 5 ----- readthedocs/organizations/querysets.py | 2 +- readthedocs/subscriptions/event_handlers.py | 8 ++------ readthedocs/subscriptions/views.py | 7 +++++-- 4 files changed, 8 insertions(+), 14 deletions(-) diff --git a/readthedocs/organizations/models.py b/readthedocs/organizations/models.py index 1c20ac097ba..491124a79e1 100644 --- a/readthedocs/organizations/models.py +++ b/readthedocs/organizations/models.py @@ -13,8 +13,6 @@ from readthedocs.core.permissions import AdminPermission from readthedocs.core.utils import slugify -from readthedocs.subscriptions.utils import get_or_create_stripe_subscription - from . import constants from .managers import TeamManager, TeamMemberManager from .querysets import OrganizationQuerySet @@ -133,9 +131,6 @@ class Meta: def __str__(self): return self.name - def get_or_create_stripe_subscription(self): - return get_or_create_stripe_subscription(self) - def get_stripe_subscription(self): # Active subscriptions take precedence over non-active subscriptions, # otherwise we return the most recently created subscription. diff --git a/readthedocs/organizations/querysets.py b/readthedocs/organizations/querysets.py index 1396cb57b38..decb0a6272d 100644 --- a/readthedocs/organizations/querysets.py +++ b/readthedocs/organizations/querysets.py @@ -128,7 +128,7 @@ def clean_artifacts(self): are disabled and their artifacts weren't cleaned already. We should be safe to cleanup all their artifacts at this point. """ - return self.subscription_ended(days=3 * DISABLE_AFTER_DAYS, exact=True).filter( + return self.subscription_ended(days=3 * DISABLE_AFTER_DAYS, exact=False).filter( disabled=True, artifacts_cleaned=False, ) diff --git a/readthedocs/subscriptions/event_handlers.py b/readthedocs/subscriptions/event_handlers.py index 31cd94ed034..cb9517c6e77 100644 --- a/readthedocs/subscriptions/event_handlers.py +++ b/readthedocs/subscriptions/event_handlers.py @@ -92,7 +92,7 @@ def subscription_updated_event(event): and the subscription is now active, we re-enable the organization. We also re-evaluate the latest subscription attached to the organization, - it case it changed. + in case it changed. """ stripe_subscription_id = event.data["object"]["id"] log.bind(stripe_subscription_id=stripe_subscription_id) @@ -125,14 +125,12 @@ def subscription_updated_event(event): log.error("Subscription isn't attached to an organization") return - save_org = False if ( stripe_subscription.status == SubscriptionStatus.active and organization.disabled ): log.info("Re-enabling organization.", organization_slug=organization.slug) organization.disabled = False - save_org = True new_stripe_subscription = organization.get_stripe_subscription() if organization.stripe_subscription != new_stripe_subscription: @@ -147,10 +145,8 @@ def subscription_updated_event(event): old_stripe_subscription_id=old_subscription_id, ) organization.stripe_subscription = stripe_subscription - save_org = True - if save_org: - organization.save() + organization.save() @handler("customer.subscription.deleted") diff --git a/readthedocs/subscriptions/views.py b/readthedocs/subscriptions/views.py index c70e5a62625..31680d57dcd 100644 --- a/readthedocs/subscriptions/views.py +++ b/readthedocs/subscriptions/views.py @@ -16,7 +16,10 @@ from readthedocs.organizations.views.base import OrganizationMixin from readthedocs.subscriptions.forms import PlanForm from readthedocs.subscriptions.products import get_product -from readthedocs.subscriptions.utils import get_or_create_stripe_customer +from readthedocs.subscriptions.utils import ( + get_or_create_stripe_customer, + get_or_create_stripe_subscription, +) log = structlog.get_logger(__name__) @@ -108,7 +111,7 @@ def get_object(self): We retry the operation when the user visits the subscription page. """ org = self.get_organization() - return org.get_or_create_stripe_subscription() + return get_or_create_stripe_subscription(org) def get_context_data(self, **kwargs): context = super().get_context_data(**kwargs) From da64d82a34afdc286236919b9372a813a9fd561e Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 24 Aug 2023 13:17:22 -0500 Subject: [PATCH 11/13] Merge subscription.created and checkout.completed events --- readthedocs/subscriptions/event_handlers.py | 36 ---------------- .../tests/test_event_handlers.py | 42 ++----------------- 2 files changed, 4 insertions(+), 74 deletions(-) diff --git a/readthedocs/subscriptions/event_handlers.py b/readthedocs/subscriptions/event_handlers.py index cb9517c6e77..cdeec2c7338 100644 --- a/readthedocs/subscriptions/event_handlers.py +++ b/readthedocs/subscriptions/event_handlers.py @@ -192,42 +192,6 @@ def subscription_canceled(event): log.info("Notification sent.", recipient=owner) -@handler("checkout.session.completed") -def checkout_completed(event): - """ - Handle the creation of a new subscription via Stripe Checkout. - - Stripe checkout will create a new subscription, - so we need to replace the older one with the new one. - - If the organization attached to the customer is disabled, - we re-enable it, since the user just subscribed to a plan. - """ - customer_id = event.data["object"]["customer"] - log.bind(stripe_customer_id=customer_id) - organization = Organization.objects.filter(stripe_customer__id=customer_id).first() - if not organization: - log.error( - "Customer isn't attached to an organization.", - ) - return - - stripe_subscription_id = event.data["object"]["subscription"] - log.bind(stripe_subscription_id=stripe_subscription_id) - stripe_subscription = djstripe.Subscription.objects.filter( - id=stripe_subscription_id - ).first() - if not stripe_subscription: - log.info("Stripe subscription not found.") - return - - if organization.disabled: - log.info("Re-enabling organization.", organization_slug=organization.slug) - organization.disabled = False - - organization.stripe_subscription = stripe_subscription - organization.save() - @handler("customer.updated") def customer_updated_event(event): """Update the organization with the new information from the stripe customer.""" diff --git a/readthedocs/subscriptions/tests/test_event_handlers.py b/readthedocs/subscriptions/tests/test_event_handlers.py index dc691b4fa83..f8712087a7e 100644 --- a/readthedocs/subscriptions/tests/test_event_handlers.py +++ b/readthedocs/subscriptions/tests/test_event_handlers.py @@ -117,40 +117,7 @@ def test_reenable_organization_on_subscription_updated_event(self): self.organization.refresh_from_db() self.assertFalse(self.organization.disabled) - def test_subscription_checkout_completed_event(self): - customer = get(djstripe.Customer, id="cus_KMiHJXFHpLkcRP") - self.organization.stripe_customer = customer - self.organization.save() - - start_date = timezone.now() - end_date = timezone.now() + timezone.timedelta(days=30) - stripe_subscription = get( - djstripe.Subscription, - id="sub_9LtsU02uvjO6Ed", - status=SubscriptionStatus.active, - current_period_start=start_date, - current_period_end=end_date, - trial_end=end_date, - ) - event = get( - djstripe.Event, - data={ - "object": { - "id": "cs_test_a1UpM7pDdpXqqgZC6lQDC2HRMo5d1wW9fNX0ZiBCm6vRqTgZJZx6brwNan", - "object": "checkout.session", - "customer": customer.id, - "subscription": stripe_subscription.id, - } - }, - ) - - self.assertIsNone(self.organization.stripe_subscription) - event_handlers.checkout_completed(event=event) - - self.organization.refresh_from_db() - self.assertEqual(self.organization.stripe_subscription, stripe_subscription) - - def test_reenable_organization_on_subscription_checkout_completed_event(self): + def test_reenable_organization_on_subscription_created_event(self): customer = get(djstripe.Customer, id="cus_KMiHJXFHpLkcRP") self.organization.stripe_customer = customer self.organization.disabled = True @@ -170,16 +137,15 @@ def test_reenable_organization_on_subscription_checkout_completed_event(self): djstripe.Event, data={ "object": { - "id": "cs_test_a1UpM7pDdpXqqgZC6lQDC2HRMo5d1wW9fNX0ZiBCm6vRqTgZJZx6brwNan", - "object": "checkout.session", + "id": stripe_subscription.id, + "object": "subscription", "customer": customer.id, - "subscription": stripe_subscription.id, } }, ) self.assertIsNone(self.organization.stripe_subscription) - event_handlers.checkout_completed(event=event) + event_handlers.subscription_created_event(event=event) self.organization.refresh_from_db() self.assertEqual(self.organization.stripe_subscription, stripe_subscription) From 057b35c4b7eeda5d2de3ede8e39b2841b5dfecb1 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 24 Aug 2023 13:19:04 -0500 Subject: [PATCH 12/13] Don't include data migration, make this a deploy step instead --- .../0013_migrate_locked_subscriptions.py | 25 ------------------- 1 file changed, 25 deletions(-) delete mode 100644 readthedocs/organizations/migrations/0013_migrate_locked_subscriptions.py diff --git a/readthedocs/organizations/migrations/0013_migrate_locked_subscriptions.py b/readthedocs/organizations/migrations/0013_migrate_locked_subscriptions.py deleted file mode 100644 index fe77895c599..00000000000 --- a/readthedocs/organizations/migrations/0013_migrate_locked_subscriptions.py +++ /dev/null @@ -1,25 +0,0 @@ -# Generated by Django 4.2.4 on 2023-08-21 20:28 - -from django.db import migrations - - -def forwards_func(apps, schema_editor): - """Migrate locked subscriptions to never_disable organizations.""" - Subscription = apps.get_model("subscriptions", "Subscription") - locked_subscriptions = Subscription.objects.filter(locked=True).select_related( - "organization" - ) - for subscription in locked_subscriptions: - subscription.organization.never_disable = True - subscription.organization.save() - - -class Migration(migrations.Migration): - dependencies = [ - ("organizations", "0012_add_organization_never_disable"), - ("subscriptions", "0002_alter_planfeature_feature_type"), - ] - - operations = [ - migrations.RunPython(forwards_func), - ] From cdc0c5620cad3eec613764c410fb35c931c2963e Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 24 Aug 2023 16:28:34 -0500 Subject: [PATCH 13/13] Fix test --- readthedocs/subscriptions/tests/test_event_handlers.py | 1 + 1 file changed, 1 insertion(+) diff --git a/readthedocs/subscriptions/tests/test_event_handlers.py b/readthedocs/subscriptions/tests/test_event_handlers.py index f8712087a7e..738cfbb2c04 100644 --- a/readthedocs/subscriptions/tests/test_event_handlers.py +++ b/readthedocs/subscriptions/tests/test_event_handlers.py @@ -128,6 +128,7 @@ def test_reenable_organization_on_subscription_created_event(self): stripe_subscription = get( djstripe.Subscription, id="sub_9LtsU02uvjO6Ed", + customer=customer, status=SubscriptionStatus.active, current_period_start=start_date, current_period_end=end_date,