Skip to content

Subscriptions: use stripe price instead of relying on plan object #9640

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Oct 10, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions readthedocs/organizations/querysets.py
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ def subscription_trial_plan_ending(self):
date_next_week = date_now + timedelta(days=7)

return self.filter(
subscription__plan__slug=settings.ORG_DEFAULT_SUBSCRIPTION_PLAN_SLUG,
subscription__plan__stripe_id=settings.RTD_ORG_DEFAULT_STRIPE_PRICE,
subscription__status='trialing',
subscription__trial_end_date__lt=date_next_week,
subscription__trial_end_date__gt=date_now,
Expand All @@ -124,7 +124,7 @@ def subscription_trial_plan_ended(self):
date_now = timezone.now()
return self.filter(
~Q(subscription__status='trialing'),
Q(subscription__plan__slug=settings.ORG_DEFAULT_SUBSCRIPTION_PLAN_SLUG),
Q(subscription__plan__stripe_id=settings.RTD_ORG_DEFAULT_STRIPE_PRICE),
Q(subscription__end_date__lt=date_now) | Q(subscription__trial_end_date__lt=date_now),
)

Expand Down
3 changes: 2 additions & 1 deletion readthedocs/settings/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -718,7 +718,8 @@ def DOCKER_LIMITS(self):

# Organization settings
RTD_ALLOW_ORGANIZATIONS = False
ORG_DEFAULT_SUBSCRIPTION_PLAN_SLUG = 'trial-v2-monthly'
RTD_ORG_DEFAULT_STRIPE_PRICE = 'trial-v2-monthly'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the stripe id was the same as the slug :D

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, this was done on purpose when we created these objects on Stripe

RTD_ORG_TRIAL_PERIOD_DAYS = 30

# Elasticsearch settings.
ES_HOSTS = ['search:9200']
Expand Down
28 changes: 6 additions & 22 deletions readthedocs/subscriptions/managers.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,9 @@
from django.conf import settings
from django.db import models
from django.utils import timezone
from djstripe.enums import SubscriptionStatus

from readthedocs.core.history import set_change_reason
from readthedocs.subscriptions.utils import get_or_create_stripe_customer
from readthedocs.subscriptions.utils import get_or_create_stripe_subscription

log = structlog.get_logger(__name__)

Expand All @@ -34,7 +33,10 @@ def get_or_create_default_subscription(self, organization):
return organization.subscription

from readthedocs.subscriptions.models import Plan
plan = Plan.objects.filter(slug=settings.ORG_DEFAULT_SUBSCRIPTION_PLAN_SLUG).first()

plan = Plan.objects.filter(
stripe_id=settings.RTD_ORG_DEFAULT_STRIPE_PRICE
).first()
# This should happen only on development.
if not plan:
log.warning(
Expand All @@ -43,25 +45,7 @@ def get_or_create_default_subscription(self, organization):
)
return None

stripe_customer = get_or_create_stripe_customer(organization)
stripe_subscriptions = stripe_customer.subscriptions.exclude(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

our subscriptions should be correct now, we don't need to use this queryset anymore (excluding canceled subs), if the last subs was canceled we allow the user to buy a new one.

status=SubscriptionStatus.canceled
)
if stripe_subscriptions.count() > 1:
log.warning(
"Customer with more than one active subscription.",
stripe_customer=stripe_customer.id,
)
Comment on lines -50 to -54
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could still leave this warning (on subscriptions/utils.py where this code was moved), but I think it makes more sense to query all of our subs searching for cases like this and decide what to do if any.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm fine trusting that our data is consistent. However, if it's not, how we will detect this? This code at least gave us some hint were to look at when Stripe data was inconsistent.

We added it to detect these cases because we weren't sure which ones were those customers.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have 0 cases of more than one active subs

In [11]: from djstripe import models as djstripe
    ...: from django.db import models
    ...: 
    ...: subquery = models.Subquery(
    ...:     djstripe.Subscription.objects
    ...:     .filter(customer=models.OuterRef("id"), status="active")
    ...:     .annotate(count=models.Count("id"))
    ...:     .values("count")
    ...: )
    ...: 
    ...: q = djstripe.Customer.objects.annotate(active_subs_count=subquery).filter(active_subs_count__gt=1)

In [12]: q.count()
Out[12]: 0

Should we still have this warning?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same on .org

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm merging this, so I can create a new PR with some changes I have, with main as base instead of this branch as base. Let me know if you still think we should log this, I can just open a new PR for that.


stripe_subscription = stripe_subscriptions.last()
if not stripe_subscription:
# TODO: djstripe 2.6.x doesn't return the subscription object
# on subscribe(), but 2.7.x (unreleased) does!
stripe_customer.subscribe(
items=[{"price": plan.stripe_id}],
trial_period_days=plan.trial,
)
stripe_subscription = stripe_customer.subscriptions.latest()
stripe_subscription = get_or_create_stripe_subscription(organization)

return self.create(
plan=plan,
Expand Down
44 changes: 32 additions & 12 deletions readthedocs/subscriptions/signals.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,23 +34,43 @@ def remove_stripe_subscription(sender, instance, using, **kwargs):

# pylint: disable=unused-argument
@receiver(post_save, sender=Organization)
def update_billing_information(sender, instance, created, **kwargs):
"""Update billing email information."""
def update_stripe_customer(sender, instance, created, **kwargs):
"""Update email and metadata attached to the stripe customer."""
if created:
return

organization = instance
log.bind(organization_slug=organization.slug)
# pylint: disable=broad-except
try:
s_customer = stripe.Customer.retrieve(organization.stripe_id)
if s_customer.email != organization.email:
s_customer.email = organization.email
s_customer.save()
except stripe.error.StripeError:
log.exception('Unable to update the Organization billing email on Stripe.')
except Exception:
log.exception('Unknown error when updating Organization billing email on Stripe.')

stripe_customer = organization.stripe_customer
if not stripe_customer:
return

fields_to_update = {}
if organization.email != stripe_customer.email:
fields_to_update["email"] = organization.email

org_metadata = organization.get_stripe_metadata()
current_metadata = stripe_customer.metadata or {}
for key, value in org_metadata.items():
if current_metadata.get(key) != value:
current_metadata.update(org_metadata)
Comment on lines +56 to +58
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the complexity of this for is required. We could just do:

current_metadata.update(org_metadata)
fields_to_update["metadata"] = current_metadata

which will add new fields and update only the fields that have changed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will save one request in case the data hasn't changed, otherwise we will hit the stripe API every time the organization is updated.

fields_to_update["metadata"] = current_metadata
break

if fields_to_update:
# pylint: disable=broad-except
try:
stripe.Customer.modify(
stripe_customer.id,
**fields_to_update,
)
except stripe.error.StripeError:
log.exception("Unable to update the Organization billing email on Stripe.")
except Exception:
log.exception(
"Unknown error when updating Organization billing email on Stripe."
)


# pylint: disable=unused-argument
Expand Down
53 changes: 53 additions & 0 deletions readthedocs/subscriptions/tests/test_signals.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
from unittest import mock

from django.contrib.auth.models import User
from django.test import TestCase
from django_dynamic_fixture import get
from djstripe import models as djstripe

from readthedocs.organizations.models import Organization


class TestSignals(TestCase):
def setUp(self):
email = "[email protected]"
self.user = get(User)
self.stripe_customer = get(
djstripe.Customer,
email=email,
)
self.organization = get(
Organization,
slug="org",
owners=[self.user],
email=email,
stripe_customer=self.stripe_customer,
)
self.stripe_customer.metadata = self.organization.get_stripe_metadata()
self.stripe_customer.save()

@mock.patch("readthedocs.subscriptions.signals.stripe.Customer")
def test_update_organization_email(self, customer):
new_email = "[email protected]"
self.organization.email = new_email
self.organization.save()
customer.modify.assert_called_once_with(
self.stripe_customer.id,
email=new_email,
)

@mock.patch("readthedocs.subscriptions.signals.stripe.Customer")
def test_update_organization_slug(self, customer):
new_slug = "new-org"
self.organization.slug = new_slug
self.organization.save()
new_metadata = self.organization.get_stripe_metadata()
customer.modify.assert_called_once_with(
self.stripe_customer.id,
metadata=new_metadata,
)

@mock.patch("readthedocs.subscriptions.signals.stripe.Customer")
def test_save_organization_no_changes(self, customer):
self.organization.save()
customer.modify.assert_not_called()
8 changes: 6 additions & 2 deletions readthedocs/subscriptions/tests/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,12 @@ 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, slug=settings.ORG_DEFAULT_SUBSCRIPTION_PLAN_SLUG)
self.organization = get(Organization, stripe_id="123", owners=[self.user])
self.plan = get(
Plan,
published=True,
stripe_id=settings.RTD_ORG_DEFAULT_STRIPE_PRICE,
)
self.stripe_subscription = self._create_stripe_subscription(
customer_id=self.organization.stripe_id,
subscription_id="sub_a1b2c3d4",
Expand Down
21 changes: 20 additions & 1 deletion readthedocs/subscriptions/utils.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
"""Utilities to interact with subscriptions and stripe."""

import stripe
import structlog
from django.conf import settings
from djstripe import models as djstripe
from stripe.error import InvalidRequestError

Expand Down Expand Up @@ -68,3 +68,22 @@ def get_or_create_stripe_customer(organization):
log.info("No stripe customer found, creating one.")
return create_stripe_customer(organization)
return stripe_customer


def get_or_create_stripe_subscription(organization):
"""
Get the stripe subscription attached to the organization or create one.

The subscription will be created with the default price and a trial period.
"""
stripe_customer = get_or_create_stripe_customer(organization)
stripe_subscription = stripe_customer.subscriptions.order_by("created").last()
if not stripe_subscription:
# TODO: djstripe 2.6.x doesn't return the subscription object
# on subscribe(), but 2.7.x (unreleased) does!
stripe_customer.subscribe(
items=[{"price": settings.RTD_ORG_DEFAULT_STRIPE_PRICE}],
trial_period_days=settings.RTD_ORG_TRIAL_PERIOD_DAYS,
)
stripe_subscription = stripe_customer.subscriptions.latest()
return stripe_subscription