-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[PM-29598] Create Subscription Upgrade Endpoint #6787
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
[PM-29598] Create Subscription Upgrade Endpoint #6787
Conversation
|
New Issues (2)Checkmarx found the following issues in this Pull Request
|
| public class UpgradePremiumToOrganizationRequest | ||
| { | ||
| [Required] | ||
| public required PlanType PlanType { get; set; } |
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.
❌ Our newer client-side code is not operating off of PlanType. Let's send Tier and Cadence like we do here: https://github.com/bitwarden/server/blob/main/src/Api/Billing/Models/Requests/Organizations/OrganizationSubscriptionPurchaseRequest.cs
| public required PlanType PlanType { get; set; } | ||
|
|
||
| [Range(1, int.MaxValue)] | ||
| public int Seats { get; set; } |
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.
❌ When upgrading from Premium, we're only going to have 1 seat. The designs do not support seat increases at the moment.
| [Range(1, int.MaxValue)] | ||
| public int Seats { get; set; } | ||
|
|
||
| public bool PremiumAccess { get; set; } = false; |
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.
❓ What current plan still has premium access?
| public bool PremiumAccess { get; set; } = false; | ||
|
|
||
| [Range(0, 99)] | ||
| public int Storage { get; set; } = 0; |
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.
❌ This can be removed as the designs also don't support storage selection in the upgrade modal.
| [Range(0, 99)] | ||
| public int Storage { get; set; } = 0; | ||
|
|
||
| public DateTime? TrialEndDate { get; set; } |
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.
❓ Why are we passing this in?
| if (!user.Premium) | ||
| { | ||
| return new BadRequest("User does not have an active Premium subscription."); | ||
| } | ||
|
|
||
| if (string.IsNullOrEmpty(user.GatewaySubscriptionId)) | ||
| { | ||
| return new BadRequest("User does not have a Stripe subscription."); | ||
| } |
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 (user is { Premium: true, GatewaySubscriptionId: not null and not "" })
| // Fetch the current Premium subscription from Stripe | ||
| var currentSubscription = await subscriberService.GetSubscriptionOrThrow(user, new SubscriptionGetOptions | ||
| { | ||
| Expand = ["items.data.price"] |
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.
| } | ||
|
|
||
| // Fetch the current Premium subscription from Stripe | ||
| var currentSubscription = await subscriberService.GetSubscriptionOrThrow(user, new SubscriptionGetOptions |
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 this is the only use of the SubscriberService you can use StripeAdapter since that will automatically throw an exception if the subscription does not exist.
| user.PremiumExpirationDate = trialEndDate ?? currentSubscription.GetCurrentPeriodEnd(); | ||
| user.RevisionDate = DateTime.UtcNow; | ||
| await userService.SaveUserAsync(user); |
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.
❌ This results in a User record being attached to an Organization subscription, which would not be renderable or usable on the client. The idea is to create an Organization record and attach the modified subscription to that Organization while removing the subscription from the User.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6787 +/- ##
==========================================
+ Coverage 55.19% 55.28% +0.08%
==========================================
Files 1937 1939 +2
Lines 86026 86224 +198
Branches 7702 7705 +3
==========================================
+ Hits 47481 47665 +184
- Misses 36754 36767 +13
- Partials 1791 1792 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
sbrown-livefront
left a comment
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.
A few questions and changes
src/Api/Billing/Controllers/VNext/AccountBillingVNextController.cs
Outdated
Show resolved
Hide resolved
src/Core/Billing/Premium/Commands/UpgradePremiumToOrganizationCommand.cs
Outdated
Show resolved
Hide resolved
|
|
||
| namespace Bit.Api.Billing.Models.Requests.Premium; | ||
|
|
||
| public class UpgradePremiumToOrganizationRequest |
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.
❓ Would we not include seats in the request(even though we assume it will be 1 for now)?
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.
@sbrown-livefront I recommended not doing so - since the business requirements are for a 1-seat upgrade, I feel like that would primarily just give the client extra, unnecessary responsibility.
| var subscriptionItemOptions = new List<SubscriptionItemOptions>(); | ||
|
|
||
| // Mark existing Premium subscription items for deletion | ||
| foreach (var item in currentSubscription.Items.Data) |
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.
❌ We may need to be more precise in identifying subscription items by narrowing our search specifically to the items we want to delete related to premium (plan id, etc.) I'm not sure if there are some items we'd want to retain.
| }; | ||
|
|
||
| // Apply trial period if specified | ||
| if (trialEndDate.HasValue) |
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.
❓ Shouldn't this date be calculated using the TrialPeriodDays property of the target plan?
src/Core/Billing/Premium/Commands/UpgradePremiumToOrganizationCommand.cs
Outdated
Show resolved
Hide resolved
|
|
||
| // Assert | ||
| Assert.True(result.IsT1); | ||
| var badRequest = result.AsT1; |
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.
📝 Alex made some great helper methods that are more descriptive here. This is out of scope for this PR, but I wonder if we could clean up command logic a bit with it.
src/Api/Billing/Models/Requests/Premium/UpgradePremiumToOrganizationRequest.cs
Outdated
Show resolved
Hide resolved
src/Api/Billing/Controllers/VNext/AccountBillingVNextController.cs
Outdated
Show resolved
Hide resolved
src/Core/Billing/Premium/Commands/UpgradePremiumToOrganizationCommand.cs
Outdated
Show resolved
Hide resolved
src/Core/Billing/Premium/Commands/UpgradePremiumToOrganizationCommand.cs
Outdated
Show resolved
Hide resolved
src/Core/Billing/Premium/Commands/UpgradePremiumToOrganizationCommand.cs
Outdated
Show resolved
Hide resolved
src/Core/Billing/Premium/Commands/UpgradePremiumToOrganizationCommand.cs
Outdated
Show resolved
Hide resolved
src/Core/Billing/Premium/Commands/UpgradePremiumToOrganizationCommand.cs
Outdated
Show resolved
Hide resolved
src/Core/Billing/Premium/Commands/UpgradePremiumToOrganizationCommand.cs
Outdated
Show resolved
Hide resolved
src/Core/Billing/Premium/Commands/UpgradePremiumToOrganizationCommand.cs
Show resolved
Hide resolved
src/Core/Billing/Premium/Commands/UpgradePremiumToOrganizationCommand.cs
Outdated
Show resolved
Hide resolved
src/Core/Billing/Premium/Commands/UpgradePremiumToOrganizationCommand.cs
Outdated
Show resolved
Hide resolved
src/Core/Billing/Premium/Commands/UpgradePremiumToOrganizationCommand.cs
Outdated
Show resolved
Hide resolved
src/Core/Billing/Premium/Commands/UpgradePremiumToOrganizationCommand.cs
Outdated
Show resolved
Hide resolved
src/Core/Billing/Premium/Commands/UpgradePremiumToOrganizationCommand.cs
Show resolved
Hide resolved
src/Core/Billing/Premium/Commands/UpgradePremiumToOrganizationCommand.cs
Show resolved
Hide resolved
src/Core/Billing/Premium/Commands/UpgradePremiumToOrganizationCommand.cs
Outdated
Show resolved
Hide resolved
| UseCustomPermissions = targetPlan.HasCustomPermissions, | ||
| UseScim = targetPlan.HasScim, | ||
| Plan = targetPlan.Name, | ||
| Gateway = null, |
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.
Last thing: This needs to be Stripe in order for the Admin Portal links to the Customer and Subscription to work.
| var currentSubscription = await stripeAdapter.GetSubscriptionAsync(user.GatewaySubscriptionId); | ||
|
|
||
| // Get the premium plan to identify which price IDs to delete | ||
| var premiumPlan = await pricingClient.GetAvailablePremiumPlan(); |
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.
❌ I missed this, but what happens if the User is not on the currently available premium plan?
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.
This is an excellent catch
| var premiumPlans = await pricingClient.ListPremiumPlans(); | ||
| var availablePremiumPlan = premiumPlans.FirstOrDefault(p => p.Available); | ||
|
|
||
| if (availablePremiumPlan == null) |
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.
I don't think this is necessary. Really what we're trying to do here is find the specific Premium plan the user is on. We do that by looking for the subscription item for Password Manager that matches one of the premium plans. Then, when we have that subscription item, we can delete it because it will be transitioning to an organization. Let me know if this makes sense. I think you did something similar on your recent storage command you can reference.
| Metadata = new Dictionary<string, string> | ||
| { | ||
| [StripeConstants.MetadataKeys.OrganizationId] = organizationId.ToString(), | ||
| [StripeConstants.MetadataKeys.PreviousPremiumPriceId] = availablePremiumPlan.Seat.StripePriceId, |
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.
This is what I mean: The user might not actually have been on this plan. We need the plan the user was on from the premium plans that come back from the Pricing service.
src/Core/Billing/Premium/Commands/UpgradePremiumToOrganizationCommand.cs
Show resolved
Hide resolved
sbrown-livefront
left a comment
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.
✅


🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-29598
📔 Objective
Objective: Create a backend endpoint to process the upgrade from a Premium subscription to a new Organization plan, ensuring the existing subscription is modified rather than creating a new one.
Given a user has an active Premium subscription
When they submit a request to upgrade to an organization
Then the system updates their Stripe subscription, applies a trial, and adds reversion metadata.
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes