-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Subscriptions: remove old code #10642
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
Conversation
c74d879
to
3c24454
Compare
3c24454
to
8b896aa
Compare
readthedocs/organizations/migrations/0013_migrate_locked_subscriptions.py
Outdated
Show resolved
Hide resolved
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.
This looks pretty good! I wasn't able to review everything yet, tests are missing still, but the core logic is 👍🏼 . I left some feedback as well.
I'll come back with fresh eyes for the tests in the following days.
readthedocs/organizations/migrations/0013_migrate_locked_subscriptions.py
Outdated
Show resolved
Hide resolved
queryset = Organization.objects.disable_soon( | ||
days=30, exact=True | ||
).subscription_trial_plan_ended() |
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.
Why do we need both here, disable_soon
and subscription_trial_plan_ended
?
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.
We are automatically disabling expired trial subscriptions only, we don't do that automatically for normal subscriptions (I think we should start doing it automatically as well)
Thanks for considering all my suggestions 👍🏼 |
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.
Went ahead and moved this to be an attribute of the organization instead.
After this is deployed, we can zeroed the migrations and remove the app.
How to deploy this change
Closes #10624