ref: Plan query optimizations #1131
Conversation
🔍 Existing Issues For ReviewYour pull request is modifying functions with the following pre-existing issues: 📄 File: api/internal/owner/serializers.py
Did you find this useful? React with a 👍 or 👎 |
| @@ -404,7 +411,11 @@ def modify_subscription(self, owner, desired_plan): | |||
| ) | |||
|
|
|||
| def _modify_subscription_schedule( | |||
| self, owner: Owner, subscription, subscription_schedule_id, desired_plan | |||
| self, | |||
services/billing.py
Outdated
| @@ -453,20 +464,18 @@ def _modify_subscription_schedule( | |||
| metadata=self._get_checkout_session_and_subscription_metadata(owner), | |||
| ) | |||
|
|
|||
| def _is_upgrading_seats(self, owner: Owner, desired_plan: dict) -> bool: | |||
| def _is_upgrading_seats(self, owner: Owner, quantity: int) -> bool: | |||
There was a problem hiding this comment.
swap this to just quantity
|
|
||
| def _is_extending_term(self, owner: Owner, desired_plan: dict) -> bool: | ||
| def _is_extending_term( |
There was a problem hiding this comment.
swap this to just the two plan references
| def _is_similar_plan(self, owner: Owner, desired_plan: dict) -> bool: | ||
| def _is_similar_plan( | ||
| self, | ||
| owner: Owner, |
There was a problem hiding this comment.
this one needs all 4 properties, the owner, both plan references, and the quantity
| def _get_proration_params( | ||
| self, owner: Owner, desired_plan_info: Plan, quantity: int | ||
| ) -> str: | ||
| current_plan_info = Plan.objects.select_related("tier").get(name=owner.plan) |
There was a problem hiding this comment.
where we're doing the current_plan_info query, the desired_plan one is one level up (saving us an extra query here)
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## Ajay/remove-references-to-old-plan-representations #1131 +/- ##
===================================================================================
Coverage 96.05% 96.06%
===================================================================================
Files 837 837
Lines 19768 19765 -3
===================================================================================
- Hits 18988 18987 -1
+ Misses 780 778 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. 📢 Thoughts on this report? Let us know! |
|
✅ All tests successful. No failed tests were found. 📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
services/billing.py
Outdated
| proration_behavior = self._get_proration_params( | ||
| owner, | ||
| desired_plan_info=desired_plan_info, | ||
| quantity=desired_plan["quantity"], |
There was a problem hiding this comment.
Nit: would change this to "desired_quantity"
There was a problem hiding this comment.
fair point, I'll make the update in the other PR
There was a problem hiding this comment.
I lied fixed in7425e16
There was a problem hiding this comment.
Lgtm! I'm wondering if there's an easy way to cache these generic values during runtime. Not a huge fan of adding many DB queries per subscription/any time we want plan data that's just going to be static.
I wonder if we could have longterm a redis cache that caches all plan data from the DB for like a day and then we wouldn't be pinging the DB any time we tell it "gimme the basic plan, gimme the pro year plan" etc (this is different than your own plan). And we could make it fancier by invalidating the cache if we see a change in the plan/tier database or something
ajay-sentry
merged commit 4330f01
into
Ajay/remove-references-to-old-plan-representations
Purpose/Motivation
Some more plan optimizations for the update_plan helper and when determining if we want to fire the sentry webhook when moving to a sentry plan. Consequently updates the tests.
The update_plan helper change is the main one, mostly just consolidating the 2 queries to a single one and passing the relevant params down that are needed
This should be merged into the remove reference PR before merging that one, but can also be done separately
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.