-
Notifications
You must be signed in to change notification settings - Fork 28
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
API: Migrate to Plan / Tier Tables #1099
Changes from 17 commits
947954a
a06b8c8
1f2e63b
5050704
40abd2d
16f7fba
3a5b3af
d26259a
f21a5f1
ba63895
91ea64b
42488fb
1bfc6d8
99a63e1
49f2af8
e2560c5
34b1677
3f7fe6f
4fa07ca
b6ad275
ea7582c
e9662a2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -7,14 +7,12 @@ | |||||
from rest_framework import serializers | ||||||
from rest_framework.exceptions import PermissionDenied | ||||||
from shared.plan.constants import ( | ||||||
PAID_PLANS, | ||||||
SENTRY_PAID_USER_PLAN_REPRESENTATIONS, | ||||||
TEAM_PLAN_MAX_USERS, | ||||||
TEAM_PLAN_REPRESENTATIONS, | ||||||
TierName, | ||||||
) | ||||||
from shared.plan.service import PlanService | ||||||
|
||||||
from codecov_auth.models import Owner | ||||||
from codecov_auth.models import Owner, Plan | ||||||
from services.billing import BillingService | ||||||
from services.sentry import send_user_webhook as send_sentry_webhook | ||||||
|
||||||
|
@@ -137,11 +135,6 @@ def validate_value(self, value: str) -> str: | |||||
plan["value"] for plan in plan_service.available_plans(current_owner) | ||||||
] | ||||||
if value not in plan_values: | ||||||
if value in SENTRY_PAID_USER_PLAN_REPRESENTATIONS: | ||||||
log.warning( | ||||||
"Non-Sentry user attempted to transition to Sentry plan", | ||||||
extra=dict(owner_id=current_owner.pk, plan=value), | ||||||
) | ||||||
raise serializers.ValidationError( | ||||||
f"Invalid value for plan: {value}; must be one of {plan_values}" | ||||||
) | ||||||
|
@@ -154,8 +147,18 @@ def validate(self, plan: Dict[str, Any]) -> Dict[str, Any]: | |||||
detail="You cannot update your plan manually, for help or changes to plan, connect with [email protected]" | ||||||
) | ||||||
|
||||||
active_plans = list( | ||||||
Plan.objects.select_related("tier").filter(paid_plan=True, is_active=True) | ||||||
) | ||||||
nora-codecov marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
active_plan_names = {plan.name for plan in active_plans} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
written from memory so there may be typos, but instead of iterating through, just return the values you want There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's cool that django can keep those sets in memory and do additional filters if needed |
||||||
team_tier_plans = { | ||||||
plan.name | ||||||
for plan in active_plans | ||||||
if plan.tier.tier_name == TierName.TEAM.value | ||||||
} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do a filter on the qset to achieve plan.tier.tier_name == TierName.TEAM.value rather than iterating and checking. then do a values_list to get the names There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated ✔️ Thanks for the suggestion |
||||||
|
||||||
# Validate quantity here because we need access to whole plan object | ||||||
if plan["value"] in PAID_PLANS: | ||||||
if plan["value"] in active_plan_names: | ||||||
if "quantity" not in plan: | ||||||
raise serializers.ValidationError( | ||||||
"Field 'quantity' required for updating to paid plans" | ||||||
|
@@ -184,7 +187,7 @@ def validate(self, plan: Dict[str, Any]) -> Dict[str, Any]: | |||||
"Quantity or plan for paid plan must be different from the existing one" | ||||||
) | ||||||
if ( | ||||||
plan["value"] in TEAM_PLAN_REPRESENTATIONS | ||||||
plan["value"] in team_tier_plans | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought that with the new structuring of plans, instead of doing a comparison like this, you would get the the plan should have these attributes on it, so you don't need to compile a list of other plans and ask is this one in that list - just check the attribute. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep totally on board! Unfortunately the frontend is the thing that sends us these values right now and it's super naive in how it's being done. We could fetch the plan based off the name here maybe and do a similar check, but ultimately its about the same There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||
and plan["quantity"] > TEAM_PLAN_MAX_USERS | ||||||
): | ||||||
raise serializers.ValidationError( | ||||||
|
@@ -219,7 +222,7 @@ def get_plan(self, phase: Dict[str, Any]) -> str: | |||||
plan_name = list(stripe_plan_dict.keys())[ | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can probably be further cleaned up by doing a get on the stripe_id 🤔 |
||||||
list(stripe_plan_dict.values()).index(plan_id) | ||||||
] | ||||||
marketing_plan_name = PAID_PLANS[plan_name].billing_rate | ||||||
marketing_plan_name = Plan.objects.get(name=plan_name).marketing_name | ||||||
return marketing_plan_name | ||||||
|
||||||
def get_quantity(self, phase: Dict[str, Any]) -> int: | ||||||
|
@@ -342,7 +345,11 @@ def update(self, instance: Owner, validated_data: Dict[str, Any]) -> object: | |||||
instance, desired_plan | ||||||
) | ||||||
|
||||||
if desired_plan["value"] in SENTRY_PAID_USER_PLAN_REPRESENTATIONS: | ||||||
sentry_plans = Plan.objects.filter( | ||||||
tier__tier_name=TierName.SENTRY.value, is_active=True | ||||||
).values_list("name", flat=True) | ||||||
|
||||||
if desired_plan["value"] in sentry_plans: | ||||||
current_owner = self.context["view"].request.current_owner | ||||||
send_sentry_webhook(current_owner, instance) | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,92 @@ | ||
interactions: | ||
- request: | ||
body: billing_address_collection=required&payment_method_collection=if_required&client_reference_id=65&success_url=http%3A%2F%2Flocalhost%3A3000%2Fplan%2Fgh%2Fjustin47%3Fsuccess&cancel_url=http%3A%2F%2Flocalhost%3A3000%2Fplan%2Fgh%2Fjustin47%3Fcancel&customer=1000&mode=subscription&line_items[0][price]=price_1OCM0gGlVGuVgOrkWDYEBtSL&line_items[0][quantity]=11&subscription_data[metadata][service]=github&subscription_data[metadata][obo_organization]=65&subscription_data[metadata][username]=justin47&subscription_data[metadata][obo_name]=Kelly+Williams&subscription_data[metadata][obo_email]=christopher27%40lopez-welch.com&subscription_data[metadata][obo]=65&tax_id_collection[enabled]=True&customer_update[name]=auto&customer_update[address]=auto | ||
headers: | ||
Accept: | ||
- '*/*' | ||
Accept-Encoding: | ||
- gzip, deflate | ||
Connection: | ||
- keep-alive | ||
Content-Length: | ||
- '744' | ||
Content-Type: | ||
- application/x-www-form-urlencoded | ||
Idempotency-Key: | ||
- 2e7e12e1-9051-4766-b947-1abe985b5e98 | ||
Stripe-Version: | ||
- 2024-12-18.acacia | ||
User-Agent: | ||
- Stripe/v1 PythonBindings/11.4.1 | ||
X-Stripe-Client-User-Agent: | ||
- '{"bindings_version": "11.4.1", "lang": "python", "publisher": "stripe", "httplib": | ||
"requests", "lang_version": "3.12.8", "platform": "Linux-6.10.14-linuxkit-aarch64-with-glibc2.36", | ||
"uname": "Linux 35c9e7c77efc 6.10.14-linuxkit #1 SMP Fri Nov 29 17:22:03 UTC | ||
2024 aarch64 "}' | ||
method: POST | ||
uri: https://api.stripe.com/v1/checkout/sessions | ||
response: | ||
body: | ||
string: "{\n \"error\": {\n \"code\": \"resource_missing\",\n \"doc_url\": | ||
\"https://stripe.com/docs/error-codes/resource-missing\",\n \"message\": | ||
\"No such customer: '1000'\",\n \"param\": \"customer\",\n \"request_log_url\": | ||
\"https://dashboard.stripe.com/test/logs/req_oevRZUbMiaT1kM?t=1737668618\",\n | ||
\ \"type\": \"invalid_request_error\"\n }\n}\n" | ||
headers: | ||
Access-Control-Allow-Credentials: | ||
- 'true' | ||
Access-Control-Allow-Methods: | ||
- GET, HEAD, PUT, PATCH, POST, DELETE | ||
Access-Control-Allow-Origin: | ||
- '*' | ||
Access-Control-Expose-Headers: | ||
- Request-Id, Stripe-Manage-Version, Stripe-Should-Retry, X-Stripe-External-Auth-Required, | ||
X-Stripe-Privileged-Session-Required | ||
Access-Control-Max-Age: | ||
- '300' | ||
Cache-Control: | ||
- no-cache, no-store | ||
Connection: | ||
- keep-alive | ||
Content-Length: | ||
- '325' | ||
Content-Security-Policy: | ||
- base-uri 'none'; default-src 'none'; form-action 'none'; frame-ancestors 'none'; | ||
img-src 'self'; script-src 'self' 'report-sample'; style-src 'self'; upgrade-insecure-requests; | ||
report-uri https://q.stripe.com/csp-violation?q=ieXMYrsoTw4hsNfwL6RhDPN6zQnVwnAAc0QsFb8i8xtl4N7V94VZzDmgDhKzWxW8mHRRg08-d5GW6oHr | ||
Content-Type: | ||
- application/json | ||
Cross-Origin-Opener-Policy-Report-Only: | ||
- same-origin; report-to="coop" | ||
Date: | ||
- Thu, 23 Jan 2025 21:43:38 GMT | ||
Idempotency-Key: | ||
- 2e7e12e1-9051-4766-b947-1abe985b5e98 | ||
Original-Request: | ||
- req_oevRZUbMiaT1kM | ||
Report-To: | ||
- '{"group":"coop","max_age":8640,"endpoints":[{"url":"https://q.stripe.com/coop-report"}],"include_subdomains":true}' | ||
Reporting-Endpoints: | ||
- coop="https://q.stripe.com/coop-report" | ||
Request-Id: | ||
- req_oevRZUbMiaT1kM | ||
Server: | ||
- nginx | ||
Strict-Transport-Security: | ||
- max-age=63072000; includeSubDomains; preload | ||
Stripe-Version: | ||
- 2024-12-18.acacia | ||
Vary: | ||
- Origin | ||
X-Content-Type-Options: | ||
- nosniff | ||
X-Stripe-Priority-Routing-Enabled: | ||
- 'true' | ||
X-Stripe-Routing-Context-Priority-Tier: | ||
- api-testmode | ||
X-Wc: | ||
- AB | ||
status: | ||
code: 400 | ||
message: Bad Request | ||
version: 1 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,95 @@ | ||
interactions: | ||
- request: | ||
body: billing_address_collection=required&payment_method_collection=if_required&client_reference_id=93&success_url=http%3A%2F%2Flocalhost%3A3000%2Fplan%2Fgh%2Fjocelyn62%3Fsuccess&cancel_url=http%3A%2F%2Flocalhost%3A3000%2Fplan%2Fgh%2Fjocelyn62%3Fcancel&customer=1000&mode=subscription&line_items[0][price]=price_1OCM0gGlVGuVgOrkWDYEBtSL&line_items[0][quantity]=11&subscription_data[metadata][service]=github&subscription_data[metadata][obo_organization]=93&subscription_data[metadata][username]=jocelyn62&subscription_data[metadata][obo_name]=Crystal+Schmitt&subscription_data[metadata][obo_email]=smithamanda%40flowers.biz&subscription_data[metadata][obo]=93&tax_id_collection[enabled]=True&customer_update[name]=auto&customer_update[address]=auto | ||
headers: | ||
Accept: | ||
- '*/*' | ||
Accept-Encoding: | ||
- gzip, deflate | ||
Connection: | ||
- keep-alive | ||
Content-Length: | ||
- '742' | ||
Content-Type: | ||
- application/x-www-form-urlencoded | ||
Idempotency-Key: | ||
- 26000f5a-feb8-4647-ac57-32a5e5ff8729 | ||
Stripe-Version: | ||
- 2024-12-18.acacia | ||
User-Agent: | ||
- Stripe/v1 PythonBindings/11.4.1 | ||
X-Stripe-Client-Telemetry: | ||
- '{"last_request_metrics": {"request_id": "req_AaY8IvHbbSDcvz", "request_duration_ms": | ||
2}}' | ||
X-Stripe-Client-User-Agent: | ||
- '{"bindings_version": "11.4.1", "lang": "python", "publisher": "stripe", "httplib": | ||
"requests", "lang_version": "3.12.8", "platform": "Linux-6.10.14-linuxkit-aarch64-with-glibc2.36", | ||
"uname": "Linux 35c9e7c77efc 6.10.14-linuxkit #1 SMP Fri Nov 29 17:22:03 UTC | ||
2024 aarch64 "}' | ||
method: POST | ||
uri: https://api.stripe.com/v1/checkout/sessions | ||
response: | ||
body: | ||
string: "{\n \"error\": {\n \"code\": \"resource_missing\",\n \"doc_url\": | ||
\"https://stripe.com/docs/error-codes/resource-missing\",\n \"message\": | ||
\"No such customer: '1000'\",\n \"param\": \"customer\",\n \"request_log_url\": | ||
\"https://dashboard.stripe.com/test/logs/req_k8lY68XdxWIFHo?t=1737668619\",\n | ||
\ \"type\": \"invalid_request_error\"\n }\n}\n" | ||
headers: | ||
Access-Control-Allow-Credentials: | ||
- 'true' | ||
Access-Control-Allow-Methods: | ||
- GET, HEAD, PUT, PATCH, POST, DELETE | ||
Access-Control-Allow-Origin: | ||
- '*' | ||
Access-Control-Expose-Headers: | ||
- Request-Id, Stripe-Manage-Version, Stripe-Should-Retry, X-Stripe-External-Auth-Required, | ||
X-Stripe-Privileged-Session-Required | ||
Access-Control-Max-Age: | ||
- '300' | ||
Cache-Control: | ||
- no-cache, no-store | ||
Connection: | ||
- keep-alive | ||
Content-Length: | ||
- '325' | ||
Content-Security-Policy: | ||
- base-uri 'none'; default-src 'none'; form-action 'none'; frame-ancestors 'none'; | ||
img-src 'self'; script-src 'self' 'report-sample'; style-src 'self'; upgrade-insecure-requests; | ||
report-uri https://q.stripe.com/csp-violation?q=1vgSiZ6UPd0qoBXo1Mjsk02GXFGP3M7PXsjua2jiowWQKm8jByxTHbhTeRirsQsrZ7jscQLjXtdCc_sh | ||
Content-Type: | ||
- application/json | ||
Cross-Origin-Opener-Policy-Report-Only: | ||
- same-origin; report-to="coop" | ||
Date: | ||
- Thu, 23 Jan 2025 21:43:39 GMT | ||
Idempotency-Key: | ||
- 26000f5a-feb8-4647-ac57-32a5e5ff8729 | ||
Original-Request: | ||
- req_k8lY68XdxWIFHo | ||
Report-To: | ||
- '{"group":"coop","max_age":8640,"endpoints":[{"url":"https://q.stripe.com/coop-report"}],"include_subdomains":true}' | ||
Reporting-Endpoints: | ||
- coop="https://q.stripe.com/coop-report" | ||
Request-Id: | ||
- req_k8lY68XdxWIFHo | ||
Server: | ||
- nginx | ||
Strict-Transport-Security: | ||
- max-age=63072000; includeSubDomains; preload | ||
Stripe-Version: | ||
- 2024-12-18.acacia | ||
Vary: | ||
- Origin | ||
X-Content-Type-Options: | ||
- nosniff | ||
X-Stripe-Priority-Routing-Enabled: | ||
- 'true' | ||
X-Stripe-Routing-Context-Priority-Tier: | ||
- api-testmode | ||
X-Wc: | ||
- AB | ||
status: | ||
code: 400 | ||
message: Bad Request | ||
version: 1 |
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.
Removed this because it would now just be an extra DB call for a log lol