[PM-33301] Add Functionality for Upgrading Using PayPal#7183
[PM-33301] Add Functionality for Upgrading Using PayPal#7183sbrown-livefront wants to merge 5 commits intomainfrom
Conversation
|
New Issues (2)Checkmarx found the following issues in this Pull Request
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7183 +/- ##
=======================================
Coverage 57.38% 57.38%
=======================================
Files 2031 2031
Lines 89282 89285 +3
Branches 7936 7943 +7
=======================================
+ Hits 51237 51239 +2
- Misses 36204 36205 +1
Partials 1841 1841 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
Claude finished @sbrown-livefront's task in 2m 34s —— View job Review: PM-33301 Add Functionality for Upgrading Using PayPalOverviewThis PR adds PayPal (Braintree) payment support for the Premium-to-Organization upgrade flow, and refactors the upgrade command into smaller helper methods. The refactoring is clean and improves readability. Two new tests cover the PayPal and non-PayPal paths. The overall PayPal integration pattern (setting Findings
SummaryOne bug found: the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| return organization.Id; | ||
| }); | ||
| await braintreeService.PayInvoice(new UserId(organizationId), invoice); |
There was a problem hiding this comment.
❌ CRITICAL: Wrong subscriber type passed to Braintree -- organization payment recorded as user payment
This passes a UserId wrapping the organization's GUID. Since the subscription is being transferred from a User to an Organization (the metadata is updated to set OrganizationId and clear UserId on lines 271-272), the Braintree transaction should be associated with the organization, not a user.
Inside BraintreeService.PayInvoice, the SubscriberId variant determines the Braintree transaction custom field key:
UserIdproduces"user_id"OrganizationIdproduces"organization_id"
Using UserId here means the Braintree transaction metadata will contain user_id:{organizationGuid} instead of organization_id:{organizationGuid}, creating a data inconsistency that will affect payment reconciliation, support lookups, and any downstream systems that key off these Braintree custom fields.
| await braintreeService.PayInvoice(new UserId(organizationId), invoice); | |
| await braintreeService.PayInvoice(new OrganizationId(organizationId), invoice); |
There was a problem hiding this comment.
❌ Confirming above ^
| await _braintreeService.Received(1).PayInvoice( | ||
| Arg.Is<SubscriberId>(id => id.IsT0 && id.AsT0.Value == organizationId), | ||
| mockInvoice); |
There was a problem hiding this comment.
❌ CRITICAL: Test asserts the wrong subscriber type, masking the production bug
id.IsT0 checks for the UserId variant (index 0 in the OneOf<UserId, OrganizationId, ProviderId>). Since the upgrade creates an Organization and the Braintree payment is on behalf of that organization, this should assert IsT1 (the OrganizationId variant) and use AsT1.
This test currently passes because the production code has the same bug (new UserId(organizationId) instead of new OrganizationId(organizationId)), so the assertion matches the buggy behavior.
| await _braintreeService.Received(1).PayInvoice( | |
| Arg.Is<SubscriberId>(id => id.IsT0 && id.AsT0.Value == organizationId), | |
| mockInvoice); | |
| await _braintreeService.Received(1).PayInvoice( | |
| Arg.Is<SubscriberId>(id => id.IsT1 && id.AsT1.Value == organizationId), | |
| mockInvoice); |
|
|
||
| return organization.Id; | ||
| }); | ||
| await braintreeService.PayInvoice(new UserId(organizationId), invoice); |
There was a problem hiding this comment.
❌ Confirming above ^
| return organization.Id; | ||
| }); | ||
|
|
||
| private async Task<(Subscription currentSubscription, List<PremiumPlan> premiumPlans, SubscriptionItem? passwordManagerItem)> GetPremiumPlanAndSubscriptionDetailsAsync(User user) |
There was a problem hiding this comment.
⛏️ The utility of this method is a bit unclear to me. I think 3 straight-forward lines turned into tuple destructuring adds some misdirection.
| }, | ||
| TaxExempt = billingAddress.Country != CountryAbbreviations.UnitedStates ? TaxExempt.Reverse : TaxExempt.None | ||
| }); | ||
|
|
| // Create the Organization entity | ||
| var organization = new Organization | ||
| // if the target plan is non-seat-based, set seats to the base seats of the target plan, otherwise set to 1 | ||
| var initialSeats = isNonSeatBasedPmPlan ? targetPlan.PasswordManager.BaseSeats : 1; |
There was a problem hiding this comment.
❌ This is incorrect at the database-level. While we may only want to purchase 1 Families subscription, a Families organization gets 6 seats.
| MaxStorageGb = targetPlan.PasswordManager.BaseStorageGb, | ||
| UsePolicies = targetPlan.HasPolicies, | ||
| UseMyItems = targetPlan.HasPolicies, // TODO: use the plan property when added (PM-32366) | ||
| UseMyItems = targetPlan.HasMyItems, |
There was a problem hiding this comment.
✅ Thank you for fixing this.
4 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-33301
📔 Objective
This PR adds required functionality to be able to use Paypal as a payment method for upgrading.
It also pull out some of the upgrade command functionality into helper methods to improve readability of the main function.
📸 Screenshots
Upgrading with Existing PayPal Payment Method
Screen.Recording.2026-03-09.at.12.44.23.PM.mov
Upgrading with New Payment Method Changing from PayPal
Screen.Recording.2026-03-09.at.12.48.45.PM.mov