Skip to content
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-11728] Upgrade free organizations without Stripe Sources API #4757

Merged

Conversation

amorask-bitwarden
Copy link
Contributor

@amorask-bitwarden amorask-bitwarden commented Sep 10, 2024

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-11728

📔 Objective

This PR does some refactoring of the previously implemented OrganizationSubscriptionPurchase and methods in the OrganizationBillingService to support the purchasing of subscriptions for organizations where customers already exist.

Additionally, it adds support for upgrading free organizations using our new purchase process by:
A.) Adding an OrganizationBillingService.UpdatePaymentMethod method that will create a customer for an organization trying to update their payment method if one does not exist already (this is the flow used when organizations upgrade from free to paid)
B.) Piping the free organization purchase through the same, improved purchase process we use when purchasing a new organization.

All other subscription update logic remains untouched.

Notes

The commit chain is ordered and each commit note contains a detailed description of the changes within that specific commit.

Related Clients PR: bitwarden/clients#10976

📸 Screenshots

Upgrading a free organization with SM to a teams organization using a credit card:

Screen.Recording.2024-09-10.at.2.09.59.PM.mov

Upgrading a teams organization with SM to an enterprise organization while changing the payment method to a bank account:

Screen.Recording.2024-09-10.at.2.18.21.PM.mov

Upgrading a free organization without SM to a families organization using PayPal:

Screen.Recording.2024-09-10.at.2.21.14.PM.mov

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 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

This commit moves the IsFromSecretsManagerTrial flag from the OrganizationUpgrade to the OrganizationSignup because it will only be passed in on organization creation. Additionally, it removes the nullable boolean 'provider' flag passed to OrganizationService.SignUpAsync and instead adds that boolean flag to the OrganizationSignup which seems more appropriate.
While I'm trying to ingrain a singular model that can be used to purchase or upgrade organizations, I disliked my previously implemented OrganizationSubscriptionPurchase for being a little too wordy and specific. This sale class aligns more closely with the work we need to complete against Stripe and also uses a private constructor so that it can only be created and utilized via an Organiztion and either OrganizationSignup or OrganizationUpgrade object.
This commit renames the OrganizationBillingService.PurchaseSubscription to Finalize and passes it the OrganizationSale object. It also updates the method so that, if the organization already has a customer, it retrieves that customer instead of automatically trying to create one which we'll need for upgraded free organizations.
This commit adds an UpdatePaymentMethod to the OrganizationBillingService that will check if a customer exists for the organization and if not, creates one with the updated payment source and tax information. Then, in the UpgradeOrganizationPlanCommand, we can use the OrganizationUpgrade to get an OrganizationSale and finalize it, which will create a subscription using the customer created as part of the payment method update that takes place right before it on the client-side. Additionally, it adds some tax ID backfill logic to SubscriberService.UpdateTaxInformation
Copy link

codecov bot commented Sep 10, 2024

Codecov Report

Attention: Patch coverage is 10.62802% with 185 lines in your changes missing coverage. Please review.

Project coverage is 41.72%. Comparing base (3f11274) to head (f562767).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/Core/Billing/Models/Sales/OrganizationSale.cs 0.00% 69 Missing ⚠️
...ices/Implementations/OrganizationBillingService.cs 0.00% 68 Missing ⚠️
src/Core/Billing/Models/TaxInformation.cs 0.00% 15 Missing ⚠️
...ling/Services/Implementations/SubscriberService.cs 21.42% 9 Missing and 2 partials ⚠️
src/Core/Billing/Models/Sales/SubscriptionSetup.cs 0.00% 8 Missing ⚠️
...ionSubscriptions/UpgradeOrganizationPlanCommand.cs 66.66% 4 Missing and 1 partial ⚠️
...le/Services/Implementations/OrganizationService.cs 57.14% 2 Missing and 1 partial ⚠️
src/Core/Billing/Models/Sales/CustomerSetup.cs 0.00% 3 Missing ⚠️
...ole/Controllers/ProviderOrganizationsController.cs 0.00% 1 Missing ⚠️
...lling/Controllers/OrganizationBillingController.cs 0.00% 1 Missing ⚠️
... and 1 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4757      +/-   ##
==========================================
- Coverage   41.74%   41.72%   -0.03%     
==========================================
  Files        1304     1306       +2     
  Lines       61841    61904      +63     
  Branches     5694     5704      +10     
==========================================
+ Hits        25818    25828      +10     
- Misses      34835    34885      +50     
- Partials     1188     1191       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

Logo
Checkmarx One – Scan Summary & Detailscc3cef6d-0e43-45dd-92f2-e449100334a1

No New Or Fixed Issues Found

Copy link
Member

@eliykat eliykat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AC changes look good, I particularly like moving the provider parameter inside the Signup object.

@amorask-bitwarden amorask-bitwarden merged commit 68b421f into main Sep 11, 2024
51 checks passed
@amorask-bitwarden amorask-bitwarden deleted the billing/PM-11728/change-plan-dialog-setup-intents branch September 11, 2024 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants