-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[PM-30460] update storage job to also update database max storage #6803
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6803 +/- ##
==========================================
+ Coverage 54.96% 55.09% +0.12%
==========================================
Files 1930 1934 +4
Lines 85507 85690 +183
Branches 7654 7668 +14
==========================================
+ Hits 47003 47214 +211
+ Misses 36717 36686 -31
- Partials 1787 1790 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
New Issues (1)Checkmarx found the following issues in this Pull Request
|
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.
All the logic looks good, but the addition of new SPROCs need to be represented in the SQL project. See: https://github.com/bitwarden/server/blob/main/src/Sql/dbo/Stored%20Procedures/Organization_ReadById.sql
|
Claude finished @kdenney's task in 4m 35s —— View job Overall Assessment: REQUEST CHANGES This PR adds database MaxStorageGb updates after Stripe reconciliation to fix an oversight where storage changes were only reflected in Stripe. The implementation includes comprehensive test coverage and proper tier detection (Personal vs Organization). However, critical data consistency issues between Stripe and database updates must be addressed. Code Review DetailsCritical Issues
Important Issues
Suggested Improvements
Key Concern: The metadata marking subscriptions as "reconciled" is set during the Stripe update (line 215), but database updates happen afterward. If the database update fails, the subscription is marked as processed and won't be retried, creating permanent inconsistency. This is a critical flaw in a billing reconciliation job where data accuracy is paramount. Recommendation: Address the transaction safety issue before merging. Consider either setting metadata in a separate call after database success, implementing compensating transactions, or validating all prerequisites before modifying Stripe. |
src/Sql/dbo/Billing/Stored Procedures/Organization_ReadByGatewaySubscriptionId.sql
Outdated
Show resolved
Hide resolved
src/Sql/dbo/Billing/Stored Procedures/User_ReadByGatewaySubscriptionId.sql
Outdated
Show resolved
Hide resolved
src/Sql/dbo/Billing/Stored Procedures/User_ReadByGatewaySubscriptionId.sql
Outdated
Show resolved
Hide resolved
mkincaid-bw
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.
Few small DB changes needed.


🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-30460
📔 Objective
Fixes an oversight so that the reconcile storage job also updates the database MaxStorageGb after stripe reconciliation.
⏰ 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