Skip to content
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.

Sign up for GitHub

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

ref: Plan query optimizations #1131

Merged
merged 2 commits into from
Feb 5, 2025
Merged

ref: Plan query optimizations #1131

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f684472
some query optimizations for payments with test updates
ajay-sentry Feb 5, 2025
7425e16
PR comments
ajay-sentry Feb 5, 2025
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
10 changes: 6 additions & 4 deletions api/internal/owner/serializers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,11 +339,13 @@ def update(self, instance: Owner, validated_data: Dict[str, Any]) -> object:
instance, desired_plan
)

sentry_plans = Plan.objects.filter(
tier__tier_name=TierName.SENTRY.value, is_active=True
).values_list("name", flat=True)
plan = (
Plan.objects.select_related("tier")
.filter(name=desired_plan["value"])
.first()
)

if desired_plan["value"] in sentry_plans:
if plan and plan.tier.tier_name == TierName.SENTRY.value:
current_owner = self.context["view"].request.current_owner
send_sentry_webhook(current_owner, instance)

Expand Down
88 changes: 54 additions & 34 deletions services/billing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,17 +316,33 @@ def get_schedule(self, owner: Owner):
return stripe.SubscriptionSchedule.retrieve(subscription_schedule_id)

@_log_stripe_error
def modify_subscription(self, owner, desired_plan):
def modify_subscription(self, owner: Owner, desired_plan: dict):
desired_plan_info = Plan.objects.filter(name=desired_plan["value"]).first()
if not desired_plan_info:
log.error(
f"Plan {desired_plan['value']} not found",
extra=dict(owner_id=owner.ownerid),
)
return

subscription = stripe.Subscription.retrieve(owner.stripe_subscription_id)
proration_behavior = self._get_proration_params(owner, desired_plan)
proration_behavior = self._get_proration_params(
owner,
desired_plan_info=desired_plan_info,
desired_quantity=desired_plan["quantity"],
)
subscription_schedule_id = subscription.schedule

# proration_behavior indicates whether we immediately invoice a user or not. We only immediately
# invoice a user if the user increases the number of seats or if the plan changes from monthly to yearly.
# An increase in seats and/or plan implies the user is upgrading, hence 'is_upgrading' is a consequence
# of proration_behavior providing an invoice, in this case, != "none"
# TODO: change this to "self._is_upgrading_seats(owner, desired_plan) or self._is_extending_term(owner, desired_plan)"
is_upgrading = True if proration_behavior != "none" else False
is_upgrading = (
True
if proration_behavior != "none" and desired_plan_info.stripe_id
else False
)

# Divide logic bw immediate updates and scheduled updates
# Immediate updates: when user upgrades seats or plan
Expand All @@ -335,15 +351,6 @@ def modify_subscription(self, owner, desired_plan):
# Scheduled updates: when the user decreases seats or plan
# If the user is not in a schedule, create a schedule
# If the user is in a schedule, update the existing schedule

plan = Plan.objects.filter(name=desired_plan["value"]).first()
if not plan or not plan.stripe_id:
log.error(
f"Plan {desired_plan['value']} not found",
extra=dict(owner_id=owner.ownerid),
)
return

if is_upgrading:
if subscription_schedule_id:
log.info(
Expand All @@ -360,7 +367,7 @@ def modify_subscription(self, owner, desired_plan):
items=[
{
"id": subscription["items"]["data"][0]["id"],
"plan": plan.stripe_id,
"plan": desired_plan_info.stripe_id,
"quantity": desired_plan["quantity"],
}
],
Expand Down Expand Up @@ -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,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

just some types

owner: Owner,
subscription: stripe.Subscription,
subscription_schedule_id: str,
desired_plan: dict,
):
current_subscription_start_date = subscription["current_period_start"]
current_subscription_end_date = subscription["current_period_end"]
Expand Down Expand Up @@ -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, desired_quantity: int) -> bool:
"""
Returns `True` if purchasing more seats.
"""
return bool(
owner.plan_user_count and owner.plan_user_count < desired_plan["quantity"]
)
return bool(owner.plan_user_count and owner.plan_user_count < desired_quantity)

def _is_extending_term(self, owner: Owner, desired_plan: dict) -> bool:
def _is_extending_term(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

swap this to just the two plan references

self, current_plan_info: Plan, desired_plan_info: Plan
) -> bool:
"""
Returns `True` if switching from monthly to yearly plan.
"""
current_plan_info = Plan.objects.get(name=owner.plan)
desired_plan_info = Plan.objects.get(name=desired_plan["value"])

return bool(
current_plan_info
Expand All @@ -475,23 +484,24 @@ def _is_extending_term(self, owner: Owner, desired_plan: dict) -> bool:
and desired_plan_info.billing_rate == PlanBillingRate.YEARLY.value
)

def _is_similar_plan(self, owner: Owner, desired_plan: dict) -> bool:
def _is_similar_plan(
self,
owner: Owner,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this one needs all 4 properties, the owner, both plan references, and the quantity

current_plan_info: Plan,
desired_plan_info: Plan,
desired_quantity: int,
) -> bool:
"""
Returns `True` if switching to a plan with similar term and seats.
"""
current_plan_info = Plan.objects.select_related("tier").get(name=owner.plan)
desired_plan_info = Plan.objects.select_related("tier").get(
name=desired_plan["value"]
)

is_same_term = (
current_plan_info
and desired_plan_info
and current_plan_info.billing_rate == desired_plan_info.billing_rate
)

is_same_seats = (
owner.plan_user_count and owner.plan_user_count == desired_plan["quantity"]
owner.plan_user_count and owner.plan_user_count == desired_quantity
)
# If from PRO to TEAM, then not a similar plan
if (
Expand All @@ -508,25 +518,35 @@ def _is_similar_plan(self, owner: Owner, desired_plan: dict) -> bool:

return bool(is_same_term and is_same_seats)

def _get_proration_params(self, owner: Owner, desired_plan: dict) -> str:
def _get_proration_params(
self, owner: Owner, desired_plan_info: Plan, desired_quantity: int
) -> str:
current_plan_info = Plan.objects.select_related("tier").get(name=owner.plan)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

where we're doing the current_plan_info query, the desired_plan one is one level up (saving us an extra query here)

if (
self._is_upgrading_seats(owner, desired_plan)
or self._is_extending_term(owner, desired_plan)
or self._is_similar_plan(owner, desired_plan)
self._is_upgrading_seats(owner=owner, desired_quantity=desired_quantity)
or self._is_extending_term(
current_plan_info=current_plan_info, desired_plan_info=desired_plan_info
)
or self._is_similar_plan(
owner=owner,
current_plan_info=current_plan_info,
desired_plan_info=desired_plan_info,
desired_quantity=desired_quantity,
)
):
return "always_invoice"
else:
return "none"

def _get_success_and_cancel_url(self, owner):
def _get_success_and_cancel_url(self, owner: Owner):
short_services = {"github": "gh", "bitbucket": "bb", "gitlab": "gl"}
base_path = f"/plan/{short_services[owner.service]}/{owner.username}"
success_url = f"{settings.CODECOV_DASHBOARD_URL}{base_path}?success"
cancel_url = f"{settings.CODECOV_DASHBOARD_URL}{base_path}?cancel"
return success_url, cancel_url

@_log_stripe_error
def create_checkout_session(self, owner: Owner, desired_plan):
def create_checkout_session(self, owner: Owner, desired_plan: dict):
success_url, cancel_url = self._get_success_and_cancel_url(owner)
log.info(
"Creating Stripe Checkout Session for owner",
Expand Down
Loading
Loading