-
Notifications
You must be signed in to change notification settings - Fork 2k
feat: plan switch v1 without confirmation #39131
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
|
Size Change: +1.06 kB (+0.03%) Total Size: 3.05 MB
|
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
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.
Looks like flaky test, unrelated change
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.
6 files reviewed, 3 comments
| from_product_key: String(currentPlatformAddon?.type), | ||
| from_plan_key: String(currentPlatformAddon?.plans[0].plan_key), | ||
| to_product_key: addon.type, | ||
| to_plan_key: String(upgradePlan?.plan_key), | ||
| }) |
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.
logic: Potential runtime error if currentPlatformAddon is undefined or plans array is empty. Add null safety checks.
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/scenes/billing/BillingProductAddonActions.tsx
Line: 225:229
Comment:
**logic:** Potential runtime error if currentPlatformAddon is undefined or plans array is empty. Add null safety checks.
How can I resolve this? If you propose a fix, please make it concise.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.
Added check for currentPlatformAddon
| from_product_key: String(currentPlatformAddon?.type), | ||
| from_plan_key: String(currentPlatformAddon?.plans[0].plan_key), | ||
| to_product_key: addon.type, | ||
| to_plan_key: String(upgradePlan?.plan_key), | ||
| }) |
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.
logic: Same issue as downgrade - needs null safety for currentPlatformAddon and plans[0]
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/scenes/billing/BillingProductAddonActions.tsx
Line: 259:263
Comment:
**logic:** Same issue as downgrade - needs null safety for currentPlatformAddon and plans[0]
How can I resolve this? If you propose a fix, please make it concise.| @action(methods=["POST"], detail=False, url_path="subscription/switch-plan") | ||
| def subscription_switch_plan(self, request: Request, *args: Any, **kwargs: Any) -> HttpResponse: | ||
| organization = self._get_org_required() | ||
| billing_manager = self.get_billing_manager() | ||
| res = billing_manager.switch_plan(organization, request.data) | ||
| return Response(res, status=status.HTTP_200_OK) |
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.
logic: Missing error handling for the switch_plan operation. Other billing endpoints in this file (like deactivate, get_invoices) have try-catch blocks to handle billing service errors gracefully and return appropriate error responses.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ee/api/billing.py
Line: 272:277
Comment:
**logic:** Missing error handling for the switch_plan operation. Other billing endpoints in this file (like deactivate, get_invoices) have try-catch blocks to handle billing service errors gracefully and return appropriate error responses.
How can I resolve this? If you propose a fix, please make it concise.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.
If the billing request fails, this will return a 500 error, and the user sees a toast "There was an error switching your plan. Please try again or contact support." This is handled in billingLogic.tsx
So I don’t think we need to expose detailed errors from billing (like validation errors).
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.
Looks good, great work iterating, let's get this and the modal one out so we can test it all together.
| try { | ||
| await api.create('api/billing/subscription/switch-plan', data) | ||
|
|
||
| const productDisplayName = data.to_product_key[0].toUpperCase() + data.to_product_key.slice(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.
Nit: I think we have a util function for this somewhere
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.
Fixed
|
|
||
| return values.billing as BillingType | ||
| } catch (error: any) { | ||
| console.error(error) |
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.
Remove console log and instead capture the error
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.
Oh, thanks, forgot to remove this
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
d2aa4c2 to
b91a944
Compare
Problem
This PR adds a minimal version for plan switch under a feature flag. It allows you to upgrade or downgrade the platform addon.
Confirmation modal and prorated amount calculation will be in a follow-up PR.
This version is planned to be released internally, only for testing.
Changes
How did you test this code?
Locally
switch-subscription-planfeature flag (release toggle) is enabled