Skip to content

Subscriptions: use djstripe events to mail owners #9661

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 6 commits into from
Nov 7, 2022

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Oct 13, 2022

Instead of trying to replace the existing queries, I'm making use of djstripe events/webhooks.

  • Instead of a daily task to email users with a canceled subscription,
    we use a webhook to email users the exact time the subscription changes to "canceled" (this is the moment the subs ends, not when a user cancels a subscription, since it will remain active till the end of the cicle).
  • Instead of querying organizations with subs in trialing, we query the subs directly,
    and get the organization from them.

With this, we don't depend on having just one subs per-organization, and also stop relying on our models a little less.
The queries that are used to disable orgs aren't migrated yet since they query canceled subscriptions (#9652 (comment)).


📚 Documentation previews 📚

@stsewd stsewd force-pushed the use-stripe-webhooks-to-email-users branch 4 times, most recently from f3b962d to 417450f Compare October 14, 2022 00:48
@stsewd stsewd force-pushed the use-stripe-webhooks-to-email-users branch from 417450f to 53558fa Compare October 14, 2022 00:52
@stsewd stsewd marked this pull request as ready for review October 14, 2022 01:04
@stsewd stsewd requested a review from a team as a code owner October 14, 2022 01:04
@stsewd stsewd requested a review from humitos October 14, 2022 01:04
self.object = context_object
self.request = request
self.request = request or HttpRequest()
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 were requiring passing this parameter, and always using HttpRequest when we didn't have it, so looks like a good default. Doesn't look like we are using it for making decisions.

Comment on lines +2 to +5
Querysets for djstripe models.

Since djstripe is a third-party app,
these are injected at runtime at readthedocs/subscriptions/apps.py.
Copy link
Member Author

Choose a reason for hiding this comment

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

this can also be a collection of helpers, but using a queryset we don't need to import those and just use the model directly :)

Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

Looks good! This is pretty close to get merged.

Since we are changing how most of our email notification works, we will need to QA this closely when deploying these changes to make sure they keep continue working correctly. In particular that we are handling the Stripe webhooks correctly and also the Celery tasks.

@stsewd stsewd merged commit ffbac81 into main Nov 7, 2022
@stsewd stsewd deleted the use-stripe-webhooks-to-email-users branch November 7, 2022 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants