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: Remove references to plan and stripe consts #1130

Merged
merged 5 commits into from
Feb 10, 2025
Merged

ref: Remove references to plan and stripe consts #1130

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7218333
remove all old plan representation and stripe id references, fix tests
ajay-sentry Feb 5, 2025
4330f01
ref: Plan query optimizations (#1131)
ajay-sentry Feb 5, 2025
aff49b3
add test
ajay-sentry Feb 5, 2025
a2a853a
robustness for flaky
ajay-sentry Feb 5, 2025
849c411
Merge branch 'main' into Ajay/remove-references-to-old-plan-represent…
ajay-sentry Feb 7, 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
17 changes: 7 additions & 10 deletions api/internal/owner/serializers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
from typing import Any, Dict

from dateutil.relativedelta import relativedelta
from django.conf import settings
from rest_framework import serializers
from rest_framework.exceptions import PermissionDenied
from shared.plan.constants import (
Expand Down Expand Up @@ -217,11 +216,7 @@ class StripeScheduledPhaseSerializer(serializers.Serializer):

def get_plan(self, phase: Dict[str, Any]) -> str:
plan_id = phase["items"][0]["plan"]
stripe_plan_dict = settings.STRIPE_PLAN_IDS
plan_name = list(stripe_plan_dict.keys())[
list(stripe_plan_dict.values()).index(plan_id)
]
marketing_plan_name = Plan.objects.get(name=plan_name).marketing_name
marketing_plan_name = Plan.objects.get(stripe_id=plan_id).marketing_name
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 was so much cleaner to me

Copy link
Contributor

Choose a reason for hiding this comment

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

ya previous way was so annoying

return marketing_plan_name

def get_quantity(self, phase: Dict[str, Any]) -> int:
Expand Down Expand Up @@ -344,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
4 changes: 2 additions & 2 deletions api/internal/tests/views/test_account_viewset.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ def test_retrieve_account_gets_account_fields_when_there_are_scheduled_details(
schedule_params = {
"id": 123,
"start_date": 123689126736,
"stripe_plan_id": "plan_H6P3KZXwmAbqPS",
"stripe_plan_id": "plan_pro_yearly",
"quantity": 6,
}
phases = [
Expand Down Expand Up @@ -330,7 +330,7 @@ def test_retrieve_account_returns_last_phase_when_more_than_one_scheduled_phases
schedule_params = {
"id": 123,
"start_date": 123689126736,
"stripe_plan_id": "plan_H6P3KZXwmAbqPS",
"stripe_plan_id": "plan_pro_yearly",
"quantity": 6,
}
phases = [
Expand Down
24 changes: 12 additions & 12 deletions billing/helpers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ def get_all_admins_for_owners(owners: QuerySet[Owner]):


def mock_all_plans_and_tiers():
TierFactory(tier_name=TierName.BASIC.value)

trial_tier = TierFactory(tier_name=TierName.TRIAL.value)
PlanFactory(
tier=trial_tier,
Expand All @@ -39,18 +41,7 @@ def mock_all_plans_and_tiers():
"Unlimited private repositories",
"Priority Support",
],
)

basic_tier = TierFactory(tier_name=TierName.BASIC.value)
PlanFactory(
name=PlanName.FREE_PLAN_NAME.value,
tier=basic_tier,
marketing_name="Developer",
benefits=[
"Up to 1 user",
"Unlimited public repositories",
"Unlimited private repositories",
],
stripe_id="plan_trial",
)

pro_tier = TierFactory(tier_name=TierName.PRO.value)
Expand All @@ -67,6 +58,7 @@ def mock_all_plans_and_tiers():
billing_rate=BillingRate.MONTHLY.value,
base_unit_price=PlanPrice.MONTHLY.value,
paid_plan=True,
stripe_id="plan_pro",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

mocked out the stripe_ids for us here for easy referencing

)
PlanFactory(
name=PlanName.CODECOV_PRO_YEARLY.value,
Expand All @@ -81,6 +73,7 @@ def mock_all_plans_and_tiers():
billing_rate=BillingRate.ANNUALLY.value,
base_unit_price=PlanPrice.YEARLY.value,
paid_plan=True,
stripe_id="plan_pro_yearly",
)

team_tier = TierFactory(tier_name=TierName.TEAM.value)
Expand All @@ -98,6 +91,7 @@ def mock_all_plans_and_tiers():
base_unit_price=PlanPrice.TEAM_MONTHLY.value,
monthly_uploads_limit=2500,
paid_plan=True,
stripe_id="plan_team_monthly",
)
PlanFactory(
name=PlanName.TEAM_YEARLY.value,
Expand All @@ -113,6 +107,7 @@ def mock_all_plans_and_tiers():
base_unit_price=PlanPrice.TEAM_YEARLY.value,
monthly_uploads_limit=2500,
paid_plan=True,
stripe_id="plan_team_yearly",
)

sentry_tier = TierFactory(tier_name=TierName.SENTRY.value)
Expand All @@ -130,6 +125,7 @@ def mock_all_plans_and_tiers():
"Unlimited private repositories",
"Priority Support",
],
stripe_id="plan_sentry_monthly",
)
PlanFactory(
name=PlanName.SENTRY_YEARLY.value,
Expand All @@ -145,6 +141,7 @@ def mock_all_plans_and_tiers():
"Unlimited private repositories",
"Priority Support",
],
stripe_id="plan_sentry_yearly",
)

enterprise_tier = TierFactory(tier_name=TierName.ENTERPRISE.value)
Expand All @@ -161,6 +158,7 @@ def mock_all_plans_and_tiers():
"Unlimited private repositories",
"Priority Support",
],
stripe_id="plan_enterprise_cloud_monthly",
)
PlanFactory(
name=PlanName.ENTERPRISE_CLOUD_YEARLY.value,
Expand All @@ -175,6 +173,7 @@ def mock_all_plans_and_tiers():
"Unlimited private repositories",
"Priority Support",
],
stripe_id="plan_enterprise_cloud_yearly",
)

PlanFactory(
Expand All @@ -190,4 +189,5 @@ def mock_all_plans_and_tiers():
"Unlimited public repositories",
"Unlimited private repositories",
],
stripe_id="plan_default_free",
)
49 changes: 25 additions & 24 deletions billing/tests/test_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

from billing.helpers import mock_all_plans_and_tiers
from billing.views import StripeWebhookHandler
from codecov_auth.models import Plan

from ..constants import StripeHTTPHeaders

Expand Down Expand Up @@ -721,7 +722,7 @@ def test_customer_subscription_created_early_returns_if_unverified_payment(
"object": {
"id": "sub_123",
"customer": "cus_123",
"plan": {"id": "plan_H6P16wij3lUuxg"},
"plan": {"id": "plan_pro_yearly"},
"metadata": {"obo_organization": self.owner.ownerid},
"quantity": 20,
}
Expand Down Expand Up @@ -776,7 +777,7 @@ def test_customer_subscription_created_does_nothing_if_plan_not_paid_user_plan(
"object": {
"id": "FOEKDCDEQ",
"customer": "sdo050493",
"plan": {"id": "?"},
"plan": {"id": "plan_free"},
"metadata": {"obo_organization": self.owner.ownerid},
"quantity": 20,
}
Expand Down Expand Up @@ -809,7 +810,7 @@ def test_customer_subscription_created_sets_plan_info(
"object": {
"id": stripe_subscription_id,
"customer": stripe_customer_id,
"plan": {"id": "plan_H6P16wij3lUuxg"},
"plan": {"id": "plan_pro_yearly"},
"metadata": {"obo_organization": self.owner.ownerid},
"quantity": quantity,
"status": "active",
Expand Down Expand Up @@ -849,7 +850,7 @@ def test_customer_subscription_created_can_trigger_trial_expiration(
"object": {
"id": stripe_subscription_id,
"customer": stripe_customer_id,
"plan": {"id": "plan_H6P16wij3lUuxg"},
"plan": {"id": "plan_pro_yearly"},
"metadata": {"obo_organization": self.owner.ownerid},
"quantity": quantity,
"default_payment_method": "blabla",
Expand Down Expand Up @@ -882,7 +883,7 @@ def test_customer_subscription_updated_does_not_change_subscription_if_not_paid_
"object": {
"id": self.owner.stripe_subscription_id,
"customer": self.owner.stripe_customer_id,
"plan": {"id": "?"},
"plan": {"id": "plan_free"},
"metadata": {"obo_organization": self.owner.ownerid},
"quantity": 20,
"status": "active",
Expand Down Expand Up @@ -929,7 +930,7 @@ def test_customer_subscription_updated_does_not_change_subscription_if_there_is_
"object": {
"id": self.owner.stripe_subscription_id,
"customer": self.owner.stripe_customer_id,
"plan": {"id": "plan_H6P16wij3lUuxg"},
"plan": {"id": "plan_pro_yearly"},
"metadata": {"obo_organization": self.owner.ownerid},
"quantity": 20,
"status": "active",
Expand Down Expand Up @@ -985,7 +986,7 @@ def test_customer_subscription_updated_sets_free_and_deactivates_all_repos_if_in
"id": self.owner.stripe_subscription_id,
"customer": self.owner.stripe_customer_id,
"plan": {
"id": "plan_H6P16wij3lUuxg",
"id": "plan_pro_yearly",
},
"metadata": {"obo_organization": self.owner.ownerid},
"quantity": 20,
Expand Down Expand Up @@ -1088,7 +1089,7 @@ def test_customer_subscription_updated_sets_free_and_deactivates_all_repos_if_in
"id": self.owner.stripe_subscription_id,
"customer": self.owner.stripe_customer_id,
"plan": {
"id": "plan_H6P16wij3lUuxg",
"id": "plan_pro_yearly",
},
"metadata": {"obo_organization": self.owner.ownerid},
"quantity": 20,
Expand Down Expand Up @@ -1149,7 +1150,7 @@ def test_customer_subscription_updated_sets_fields_on_success(
"object": {
"id": self.owner.stripe_subscription_id,
"customer": self.owner.stripe_customer_id,
"plan": {"id": "plan_H6P16wij3lUuxg"},
"plan": {"id": "plan_pro_yearly"},
"metadata": {"obo_organization": self.owner.ownerid},
"quantity": quantity,
"status": "active",
Expand Down Expand Up @@ -1200,7 +1201,7 @@ def test_customer_subscription_updated_sets_fields_on_success_multiple_owner(
"object": {
"id": self.owner.stripe_subscription_id,
"customer": self.owner.stripe_customer_id,
"plan": {"id": "plan_H6P16wij3lUuxg"},
"plan": {"id": "plan_pro_yearly"},
"metadata": {"obo_organization": self.owner.ownerid},
"quantity": quantity,
"status": "active",
Expand Down Expand Up @@ -1238,7 +1239,7 @@ def test_customer_subscription_updated_logs_error_if_no_matching_owners(
"object": {
"id": "sub_notexist",
"customer": "cus_notexist",
"plan": {"id": "plan_H6P16wij3lUuxg"},
"plan": {"id": "plan_pro_yearly"},
"metadata": {"obo_organization": 1},
"quantity": 8,
"status": "active",
Expand All @@ -1254,7 +1255,7 @@ def test_customer_subscription_updated_logs_error_if_no_matching_owners(
extra={
"stripe_subscription_id": "sub_notexist",
"stripe_customer_id": "cus_notexist",
"plan_id": "plan_H6P16wij3lUuxg",
"plan_id": "plan_pro_yearly",
},
)

Expand All @@ -1267,7 +1268,7 @@ def test_subscription_schedule_released_updates_owner_with_existing_subscription
self.owner.save()

self.new_params = {
"new_plan": "plan_H6P3KZXwmAbqPS",
"new_plan": "plan_pro_yearly",
"new_quantity": 7,
"subscription_id": "sub_123",
}
Expand All @@ -1288,7 +1289,8 @@ def test_subscription_schedule_released_updates_owner_with_existing_subscription
)

self.owner.refresh_from_db()
assert self.owner.plan == settings.STRIPE_PLAN_VALS[self.new_params["new_plan"]]
plan = Plan.objects.get(stripe_id=self.new_params["new_plan"])
assert self.owner.plan == plan.name
assert self.owner.plan_user_count == self.new_params["new_quantity"]

@patch("services.billing.stripe.Subscription.retrieve")
Expand All @@ -1304,7 +1306,7 @@ def test_subscription_schedule_released_updates_multiple_owners_with_existing_su
self.other_owner.save()

self.new_params = {
"new_plan": "plan_H6P3KZXwmAbqPS",
"new_plan": "plan_pro_yearly",
"new_quantity": 7,
"subscription_id": "sub_123",
}
Expand All @@ -1326,12 +1328,11 @@ def test_subscription_schedule_released_updates_multiple_owners_with_existing_su

self.owner.refresh_from_db()
self.other_owner.refresh_from_db()
assert self.owner.plan == settings.STRIPE_PLAN_VALS[self.new_params["new_plan"]]

plan = Plan.objects.get(stripe_id=self.new_params["new_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.

had to change up the assertions a bit here to match

assert self.owner.plan == plan.name
assert self.owner.plan_user_count == self.new_params["new_quantity"]
assert (
self.other_owner.plan
== settings.STRIPE_PLAN_VALS[self.new_params["new_plan"]]
)
assert self.other_owner.plan == plan.name
assert self.other_owner.plan_user_count == self.new_params["new_quantity"]

@patch("logging.Logger.error")
Expand All @@ -1342,7 +1343,7 @@ def test_subscription_schedule_released_logs_error_if_owner_does_not_exist(
log_error_mock,
):
self.new_params = {
"new_plan": "plan_H6P3KZXwmAbqPS",
"new_plan": "plan_pro_yearly",
"new_quantity": 7,
"subscription_id": "sub_notexist",
}
Expand All @@ -1367,7 +1368,7 @@ def test_subscription_schedule_released_logs_error_if_owner_does_not_exist(
extra={
"stripe_subscription_id": "sub_notexist",
"stripe_customer_id": "cus_123",
"plan_id": "plan_H6P3KZXwmAbqPS",
"plan_id": "plan_pro_yearly",
},
)

Expand All @@ -1383,7 +1384,7 @@ def test_subscription_schedule_created_logs_a_new_schedule(
self.owner.save()

self.params = {
"new_plan": "plan_H6P3KZXwmAbqPS",
"new_plan": "plan_pro_yearly",
"new_quantity": 7,
"subscription_id": subscription_id,
}
Expand All @@ -1410,7 +1411,7 @@ def test_subscription_schedule_updated_logs_changes_to_schedule(
original_plan = "users-pr-inappy"
original_quantity = 10
subscription_id = "sub_1K8xfkGlVGuVgOrkxvroyZdH"
new_plan = "plan_H6P3KZXwmAbqPS"
new_plan = "plan_pro_yearly"
new_quantity = 7
self.owner.plan = original_plan
self.owner.plan_user_count = original_quantity
Expand Down
Loading
Loading