[PM-29611] Decouple License from Subscription Response #6768
Conversation
|
New Issues (5)Checkmarx found the following issues in this Pull Request
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6768 +/- ##
==========================================
- Coverage 54.84% 54.83% -0.01%
==========================================
Files 1920 1923 +3
Lines 85255 85285 +30
Branches 7633 7634 +1
==========================================
+ Hits 46757 46770 +13
- Misses 36718 36734 +16
- Partials 1780 1781 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| throw new UnauthorizedAccessException(); | ||
| } | ||
|
|
||
| var license = await userService.GenerateLicenseAsync(user); |
There was a problem hiding this comment.
⛏️ Please create a query for this logic. We'd like to keep our only controller-level dependencies for these controllers to be commands and queries. It can go in Core/Billing/Licenses like the response model.
| /// you want to expose Milestone 2 discount information to the client. | ||
| /// The discount will only be included if it matches the specific Milestone 2 coupon ID. | ||
| /// </param> | ||
| public SubscriptionResponseModel(User user, SubscriptionInfo subscription, UserLicense license, bool includeMilestone2Discount = false) |
There was a problem hiding this comment.
❌ Again, we're removing something from the existing flow without flagging the change. If the FF that toggles the new Premium subscription page is not active, where is the license going to come from?
There was a problem hiding this comment.
See comment on the AccountsController
cyprain-okeke
force-pushed
the
pm-29611-decouple-license-from-subscription-response
branch
from
628d8c2 to
7148bf2
Compare
| // Check if the new premium subscription page feature flag is enabled | ||
| // When enabled, clients should use the separate /license endpoint | ||
| // When disabled, include license in subscription response for backward compatibility | ||
| var useNewPremiumPage = featureService.IsEnabled(FeatureFlagKeys.PM24996ImplementUpgradeFromFreeDialog); |
There was a problem hiding this comment.
❌ There does not need to be any changes to the existing AccountsController or the SubscriptionResponseModel. This is a net-new addition of an endpoint that will service the redesigned Premium Subscription page. Once we enable the flag for the redesign, the new endpoints will be hit instead of these and at that point, we can make these changes.
| /// you want to expose Milestone 2 discount information to the client. | ||
| /// The discount will only be included if it matches the specific Milestone 2 coupon ID. | ||
| /// </param> | ||
| public SubscriptionResponseModel(User user, SubscriptionInfo subscription, UserLicense license, bool includeMilestone2Discount = false) |
There was a problem hiding this comment.
See comment on the AccountsController
| public async Task<IResult> GetLicenseAsync( | ||
| [BindNever] User user) | ||
| { | ||
| if (user == null) |
There was a problem hiding this comment.
❌ This check is not necessary when using InjectUser.
cyprain-okeke
deleted the
pm-29611-decouple-license-from-subscription-response
branch
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-29611
📔 Objective
Refactor the subscription response model to decouple license information from subscription data, implementing proper separation of concerns between billing and licensing functionality.
Code changes
Backend Changes
New Files:
- src/Api/Models/Response/LicenseResponseModel.cs- New response model for license data- test/Api.Test/Billing/Controllers/VNext/AccountBillingVNextControllerTests.cs- Unit tests for new license endpointModified Files:
- src/Api/Models/Response/SubscriptionResponseModel.cs:- Removed License property
- Removed Expiration property
- Removed license-related constructor parameters
- Maintained all subscription and discount functionality
- src/Api/Billing/Controllers/VNext/AccountBillingVNextController.cs:- Added GET /account/billing/vnext/license endpoint
- Added IUserService and ILicensingService dependencies
- src/Api/Billing/Controllers/AccountsController.cs:- Updated GetSubscriptionAsync to remove license generation
- Removed ILicensingService dependency
Test Updates:
Breaking Changes
Migration Path:
📸 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