[PM-27117] Sync Stripe Customer details for Organizations and Providers in API & Admin #6679
Conversation
|
Claude finished @amorask-bitwarden's task —— View job Code Review Complete ✅Tasks
SummaryThis PR addresses a bug where Admin Portal and Provider Portal APIs were bypassing the billing service layer, causing outdated information on Stripe invoices. The implementation adds proper Stripe synchronization for organization and provider name/email changes. Overall Quality: Good ⭐⭐⭐⭐ The solution is well-architected with excellent test coverage. However, there is one blocking issue that needs attention before merge. ❌ Blocking Issue30-Character Truncation Removed From Custom FieldsFiles affected:
Previous code (truncated to 30 chars): Value = newDisplayName.Length <= 30
? newDisplayName
: newDisplayName[..30]New code (no truncation): Value = newDisplayNameIssue: According to Stripe's API documentation, custom field values have a maximum of 30 characters. Removing this truncation may cause Stripe API errors for organizations/providers with names longer than 30 characters. Action Required:
🎨 High Priority Recommendations1. Document Error Handling StrategyLocations:
All three controllers use a pattern where database updates succeed even if Stripe sync fails: try
{
await _billingService.UpdateProviderNameAndEmail(provider);
}
catch (Exception ex)
{
_logger.LogError(ex, "...");
TempData["Warning"] = "..."; // Only in Admin Portal
}Concern: This creates eventual consistency issues where the database and Stripe may have different values. Recommendations:
2. Add Stripe API Failure TestsThe test suite covers graceful handling of missing data (null customer IDs, empty names) but doesn't test what happens when Stripe API calls fail. Suggested tests: [Theory, BitAutoData]
public async Task UpdateOrganizationNameAndEmail_WhenStripeThrowsException_PropagatesException(
Organization organization,
SutProvider<OrganizationBillingService> sutProvider)
{
// Arrange
organization.GatewayCustomerId = "cus_test123";
organization.Name = "Test Org";
sutProvider.GetDependency<IStripeAdapter>()
.CustomerUpdateAsync(Arg.Any<string>(), Arg.Any<CustomerUpdateOptions>())
.Throws(new StripeException("API Error"));
// Act & Assert
await Assert.ThrowsAsync<StripeException>(
() => sutProvider.Sut.UpdateOrganizationNameAndEmail(organization));
}👍 Positive ObservationsExcellent Test Coverage
Follows Bitwarden Standards
Clean Architecture
Good RefactoringThe removal of the 📝 Minor Suggestions (Non-blocking)1. Reduce Code DuplicationThe 2. Enhanced Test Coverage
3. Test NamingMinor nitpick: Security Review ✅
Questions for Author
Final VerdictReady to merge after:
Security: ✅ No concerns |
|
Fixed Issues (3)Great job! The following issues were fixed in this Pull Request
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6679 +/- ##
==========================================
- Coverage 53.48% 53.48% -0.01%
==========================================
Files 1918 1918
Lines 85563 85650 +87
Branches 7678 7683 +5
==========================================
+ Hits 45762 45808 +46
- Misses 38030 38071 +41
Partials 1771 1771 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
eliykat
left a comment
eliykat
left a comment
There was a problem hiding this comment.
Nice work - one comment below but it's non-blocking.
| if (!string.IsNullOrEmpty(provider.GatewayCustomerId) && | ||
| (originalName != provider.Name || originalBillingEmail != provider.BillingEmail)) |
There was a problem hiding this comment.
🎨 (non-blocking) This bit of boilerplate is repeated a few times. Is there a way to simplify things for the callers?
- the BillingServices no longer throw if the
GatewayCustomerIdis null. Can we remove that check from the callers? (That has another benefit, which is that callers don't have to know about the significance of this property, which is effectively billing metadata.) - (less sure about this) is there a better way to track / handle the "has the name or billing email changed" logic? It's a shame that cloning objects is so difficult in C#.
There was a problem hiding this comment.
I've removed the Customer ID check from the callers here: 8982bcb
With regards to your second bullet, I didn't see a way to do it that would constitute any meaningful difference. Could use a new DTO to capture previous name / email prior to the database update, but that's the same process with a different flavor.
amorask-bitwarden
dismissed stale reviews from cyprain-okeke and eliykat
via
8982bcb
amorask-bitwarden
dismissed stale reviews from cyprain-okeke and eliykat
via
c827b38
| [BitAutoData("")] | ||
| [BitAutoData((string)null)] | ||
| public async Task UpdateAsync_WhenGatewayCustomerIdIsNullOrEmpty_SkipsBillingUpdate( | ||
| public async Task UpdateAsync_WhenGatewayCustomerIdIsNullOrEmpty_CallsBillingUpdateButHandledGracefully( |
There was a problem hiding this comment.
I would remove this test case, it's no longer relevant. It's not blocking through, I can do it in a separate PR if desired.
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-27117
📔 Objective
Bug Fix: Organization and Provider name/email changes were not being synchronized to Stripe invoice custom fields when updated via Admin Portal or Provider Portal APIs, causing outdated information to appear on customer invoices.
Root Cause: Controllers were bypassing the billing service layer that handles Stripe synchronization.
Solution: Added Stripe synchronization to all update paths:
IOrganizationBillingServicedependency and Stripe sync for name/email changesILoggerdependency and Stripe sync for billing email changesIProviderBillingServiceandILoggerdependencies and Stripe sync for name/email changesKey Changes:
ProviderBillingService.UpdateProviderNameAndEmail()method to sync provider details to StripeOrganizationBillingService.UpdateOrganizationNameAndEmail()to use warning logs instead of exceptions for consistencyImpact: Ensures Stripe invoices always reflect current organization/provider names and contact emails.
📸 Screenshots
Screen.Recording.2025-12-03.at.10.34.20.AM.mov
⏰ 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