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

[AC-2678] Enterprise to Families Sponsorship Bugs #4118

Merged
merged 3 commits into from
Jun 3, 2024

Conversation

cturnbull-bitwarden
Copy link
Contributor

@cturnbull-bitwarden cturnbull-bitwarden commented May 23, 2024

Type of change

- [X] Bug fix
- [ ] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

This PR resolves two bugs:

  1. Depending on how fast the server could process the request, redeeming a sponsorship would sometimes fail. This was due to our manually setting prorationDate on the Stripe SubscriptionUpdateOptions object to the value of DateTime.UtcNow. Occasionally, the DateTime.UtcNow value would be before the lower bound of the Stripe subscriptions billing cycle, causing Stripe to throw an exception since you cannot have a proration set to before the subscription began.

    After some investigation and testing, we found that prorationDate was always being set to DateTime.UtcNow throughout the app. By removing all references to prorationDate we're effectively setting it to null on the SubscriptionUpdateOptions. When Stripe receives an update to a subscription with a proration behavior, but no proration date set, then they set it to "now" on their end, ensuring that it will never be before the billing cycle begins.

    A potential reason you might want to set prorationDate would be to apply exactly the same proration that was previewed with the upcoming invoice endpoint. It can also be used to implement custom proration logic, such as prorating by day instead of by second, by providing the time that you wish to use for proration calculations. We have no need for this logic, so it's being culled.

  2. After an organization was successfully sponsored if the sponsoring enterprise organization is then deleted, or the sponsorship relationship otherwise removed, the free families price would never be updated to be full price again.

    When we receive the invoice.upcoming webhook event from Stripe, we check if the invoice is for a sponsored organization. If it is for a sponsored organization, and the sponsoring enterprise organization is deleted or the sponsorship relationship otherwise removed, then we update the subscription to be a full priced family plan. This was broken since the webhook was looking to see if the subscription's ID matched our hard-coded IDs for a sponsored price, instead of checking the price IDs on the subscription itself.

Code changes

Changes around removing prorationDate

  • bitwarden_license\src\Sso\Controllers\AccountController.cs
  • src\Core\AdminConsole\Services\IOrganizationService.cs
  • src\Core\AdminConsole\Services\Implementations\OrganizationService.cs
  • src\Core\Models\Business\SecretsManagerSubscriptionUpdate.cs
  • src\Core\OrganizationFeatures\OrganizationSubscriptions\UpdateSecretsManagerSubscriptionCommand.cs
  • src\Core\Services\IPaymentService.cs
  • src\Core\Services\Implementations\StripePaymentService.cs

Change to update organization to full-priced Family plan

  • src\Billing\Controllers\StripeController.cs

Before you submit

  • Please check for formatting errors (dotnet format --verify-no-changes) (required)
  • If making database changes - make sure you also update Entity Framework queries and/or migrations
  • Please add unit tests where it makes sense to do so (encouraged but not required)
  • If this change requires a documentation update - notify the documentation team
  • If this change has particular deployment requirements - notify the DevOps team

Copy link

codecov bot commented May 23, 2024

Codecov Report

Attention: Patch coverage is 13.88889% with 31 lines in your changes are missing coverage. Please review.

Project coverage is 39.10%. Comparing base (0691017) to head (65767f5).
Report is 2 commits behind head on main.

Files Patch % Lines
...e/Services/Implementations/StripePaymentService.cs 0.00% 23 Missing ⚠️
...le/Services/Implementations/OrganizationService.cs 37.50% 5 Missing ⚠️
src/Billing/Controllers/StripeController.cs 0.00% 3 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4118   +/-   ##
=======================================
  Coverage   39.10%   39.10%           
=======================================
  Files        1203     1203           
  Lines       58179    58178    -1     
  Branches     5355     5354    -1     
=======================================
  Hits        22748    22748           
  Misses      34360    34360           
+ Partials     1071     1070    -1     

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

Copy link
Contributor

github-actions bot commented May 23, 2024

Logo
Checkmarx One – Scan Summary & Detailsdfe0d318-e460-46a9-b9b3-9c2b42422e9e

No New Or Fixed Issues Found

@cturnbull-bitwarden cturnbull-bitwarden merged commit 395d6e8 into main Jun 3, 2024
49 checks passed
@cturnbull-bitwarden cturnbull-bitwarden deleted the billing/AC-2678/sponsorship-bugs branch June 3, 2024 17:18
withinfocus pushed a commit that referenced this pull request Jul 9, 2024
* Removed prorationDate as it wasn't used, and wasn't needed

* Fixed logic to detect if a subscription was sponsored

* Moved OrganizationSponsorshipsController.cs to Billing folder
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.

5 participants