-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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__) | ||
|
||
|
@@ -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( | ||
|
@@ -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( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have 0 cases of more than one active subs
Should we still have this warning? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same on .org There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
stsewd marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
which will add new fields and update only the fields that have changed. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
stsewd marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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." | ||
stsewd marked this conversation as resolved.
Show resolved
Hide resolved
|
||
) | ||
|
||
|
||
# pylint: disable=unused-argument | ||
|
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() |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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