[PM-24616] refactor stripe adapter

[kdenney]

🎟️ Tracking

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

📔 Objective

This is a BEEEP initiative. Main goal was to move and cleanup the StripeAdapter. Changes made:

1. Moved StripeAdapter into Core.Billing.Services.
2. Renamed adapter methods to be consistent with how we usually name methods.
3. Renamed StripePaymentService and StripeSyncService and associated interfaces and test files.
4. Other various cleanup and refactorings in these files.

⏰ 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

[github-actions]

Checkmarx One – Scan Summary & Details – 1c1b8d2b-fe3f-4ac7-95f4-8e7fb1338af4

New Issues (1)

Checkmarx found the following issues in this Pull Request

Severity	Issue	Source File / Package	Checkmarx Insight
	CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 427	details Method at line 427 of /src/Api/Auth/Controllers/AccountsController.cs gets a parameter from a user request from model. This parameter value flow... ID: qB2Oh4bvPkc2tAsuqY%2B%2FiPKPcCE%3D Attack Vector

[claude]

Claude finished @kdenney's task —— View job

---

Code Review Complete

• Check status of previously identified issues (all fixed ✓)
• Review recent commits and changes
• Analyze StripeAdapter and related billing services
• Check for security implications in payment/billing code
• Review areas with low test coverage
• Provide comprehensive review feedback

---

Summary of Changes Since Last Review

All three issues from my previous review have been successfully resolved:

1. ✅ Removed redundant async/await in CreateCustomerBalanceTransactionAsync (StripeAdapter.cs:74-76)
2. ✅ Removed redundant async/await in ListPricesAsync (StripeAdapter.cs:180-181)
3. ✅ Removed unused GetProviderSubscriptionAsync method that wasn't in the interface
4. ✅ Fixed method name: ListCustomerPaymentMethods → ListCustomerPaymentMethodsAsync

The recent commits (92e9163c8 and f47bc55a7) implemented these fixes along with additional formatting improvements.

---

Critical Issues

None found. The refactoring addresses previous feedback comprehensively.

---

Suggested Improvements

1. 📝 Inconsistent Nullable Directives Format

File: src/Core/Billing/Services/Implementations/StripeAdapter.cs:1-3

The nullable disable directive has an extra blank line that's inconsistent with other files in the same refactoring:

// Current:
// FIXME: Update this file to be null safe and then delete the line below

#nullable disable

// Should be:
// FIXME: Update this file to be null safe and then delete the line below
#nullable disable

Compare with: src/Core/Billing/Services/Implementations/StripePaymentService.cs:1-3 which has the standard format.

Rationale

Consistency in formatting across the codebase makes it easier to apply automated fixes later when addressing the nullable reference type FIXMEs.

---

2. 🤔 Service Registration Change Needs Verification

File: src/SharedWeb/Utilities/ServiceCollectionExtensions.cs:243

The service registration changed from:

services.AddScoped<IPaymentService, StripePaymentService>();

to:

services.AddScoped<IStripePaymentService, StripePaymentService>();

Questions:

• Was IPaymentService intentionally deleted/renamed?
• Are there any consumers that might still expect IPaymentService to be available in the DI container?
• This appears to be a breaking change for any code that injects IPaymentService

Evidence

I verified that no code currently references IPaymentService in this branch, but this could be a breaking change for:

• External plugins/extensions
• Code in other repositories that depend on this service registration
• Any dynamic/reflection-based DI resolution

If this is intentional as part of the BEEEP initiative to separate Stripe-specific services, consider:

1. Adding a migration note in the PR description
2. Checking if IPaymentService should be kept as an alias/base interface for backward compatibility

---

3. ⚠️ Low Test Coverage in Critical Payment Areas

Codecov Report: 44.25% patch coverage with 160 missing lines

The following critical files have concerning test coverage:

File	Missing Lines	Coverage
StripeAdapter.cs	63	21.25%
StripePaymentService.cs	58	4.91%
PremiumUserBillingService.cs	7	0.00%

Recommendation:

While this is a refactoring PR (primarily renaming and moving), the low coverage is concerning for payment/billing code. Consider:

1. High Priority: Add unit tests for StripeAdapter methods that weren't previously tested
2. Medium Priority: Improve coverage for error paths in StripePaymentService (especially around Stripe exception handling)
3. Future Task: Add integration tests for the payment flow end-to-end

Why This Matters

Payment and billing code requires exceptional test coverage because:

• Financial correctness is critical (SOC 2, HIPAA compliance requirements)
• Stripe API changes can break integrations silently
• Refactoring without adequate tests increases regression risk
• The codebase has #nullable disable, making it more prone to null reference issues

Even though this is "just a refactoring," the act of moving code is a good opportunity to add tests for previously untested paths.

---

4. 🌱 Consider Future Null Safety Improvements

Multiple files in this PR have #nullable disable FIXMEs:

• StripeAdapter.cs
• StripePaymentService.cs
• StripeSyncService.cs (no FIXME, but implicitly nullable)
• PremiumUserBillingService.cs
• IStripePaymentService.cs
• IStripeAdapter.cs

Suggestion: While not required for this PR, consider filing a follow-up issue to address nullable reference types in the billing services, as this would:

• Align with ADR-0024 (Use .NET nullable reference types)
• Reduce null-reference exceptions in critical payment paths
• Improve code quality and maintainability

---

Good Practices Observed

👍 Consistent method naming - All adapter methods now follow the {Verb}{Noun}Async pattern
👍 Proper namespace organization - Services moved to appropriate Billing.Services namespace
👍 Interface segregation - IStripeAdapter cleanly separates Stripe API concerns
👍 Test updates included - Test files were properly renamed and updated alongside production code
👍 No functional changes - This is a true refactoring with minimal behavior changes
👍 Expression body usage - Simple pass-through methods use concise expression bodies

---

Action Items for Author

Required Before Merge:

• Verify the IPaymentService → IStripePaymentService change won't break any consumers (internal or external)
• Confirm CI tests pass completely (they currently pass, but coverage is flagged)

Recommended (Optional):

• Fix formatting inconsistency in StripeAdapter.cs nullable directive (lines 1-3)
• Consider adding issue for improving test coverage in payment services
• Consider adding issue for addressing nullable reference type FIXMEs in billing services

---

Overall Assessment

This is a well-executed refactoring that successfully:

• ✅ Moves StripeAdapter to the correct namespace (Core.Billing.Services)
• ✅ Establishes consistent naming conventions across all Stripe-related methods
• ✅ Addresses all feedback from previous review
• ✅ Updates all call sites across 100 files without introducing compilation errors
• ✅ Maintains backward compatibility in behavior (aside from the noted DI registration change)

The main concerns are around test coverage and the service registration change, which should be verified but don't block the refactoring itself.

Recommendation: Approve after confirming the IPaymentService → IStripePaymentService change is intentional and documented.

---

[codecov]

Codecov Report

❌ Patch coverage is 44.09722% with 161 lines in your changes missing coverage. Please review.
✅ Project coverage is 52.04%. Comparing base (d40d705) to head (92e9163).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
.../Billing/Services/Implementations/StripeAdapter.cs	21.25%	63 Missing ⚠️
...g/Services/Implementations/StripePaymentService.cs	4.91%	58 Missing ⚠️
...vices/Implementations/PremiumUserBillingService.cs	0.00%	7 Missing ⚠️
...ling/Services/Implementations/SubscriberService.cs	80.55%	6 Missing and 1 partial ⚠️
...ganizations/Services/OrganizationBillingService.cs	14.28%	6 Missing ⚠️
...lling/Providers/Services/ProviderBillingService.cs	64.28%	5 Missing ⚠️
...in/AdminConsole/Controllers/ProvidersController.cs	0.00%	2 Missing ⚠️
src/Api/Billing/Controllers/StripeController.cs	0.00%	2 Missing ⚠️
...nds/CreatePremiumCloudHostedSubscriptionCommand.cs	60.00%	2 Missing ⚠️
...ling/Services/Implementations/StripeSyncService.cs	0.00%	2 Missing ⚠️
... and 6 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6527      +/-   ##
==========================================
+ Coverage   51.89%   52.04%   +0.15%     
==========================================
  Files        1905     1906       +1     
  Lines       84232    84221      -11     
  Branches     7509     7516       +7     
==========================================
+ Hits        43708    43837     +129     
+ Misses      38821    38679     -142     
- Partials     1703     1705       +2     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.