Skip to content

[PM-25501] Register NoopBusinessUnitConverter in OSS DI container#7245

Open
cturnbull-bitwarden wants to merge 1 commit intomainfrom
billing/PM-25501-NoopBusinessUnitConverter
Open

[PM-25501] Register NoopBusinessUnitConverter in OSS DI container#7245
cturnbull-bitwarden wants to merge 1 commit intomainfrom
billing/PM-25501-NoopBusinessUnitConverter

Conversation

@cturnbull-bitwarden
Copy link
Contributor

🎟️ Tracking

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

📔 Objective

Fixes a runtime InvalidOperationException when the server is compiled with the OSS flag. OrganizationBillingController requires IBusinessUnitConverter, but the concrete BusinessUnitConverter is only registered in AddCommercialCoreServices(). In OSS builds, AddOosServices() is called instead, leaving the interface unregistered in the DI container.

This adds a NoopBusinessUnitConverter (following the existing noop pattern, e.g. NoopProviderService) and registers it in AddOosServices(). All methods throw NotImplementedException since the business unit conversion endpoints are gated behind [SelfHosted(NotSelfHostedOnly = true)] and should never be reached in OSS builds.

@cturnbull-bitwarden cturnbull-bitwarden added the ai-review Request a Claude code review label Mar 18, 2026
@claude
Copy link
Contributor

claude bot commented Mar 18, 2026

Claude finished @cturnbull-bitwarden's task in 1m 22s —— View job


Code Review: Clean PR

  • Gather context and review PR changes
  • Analyze code for issues, security concerns, and best practices
  • Post review findings

Summary

This is a clean, well-structured PR. No issues found.

The two changes follow established patterns in the codebase:

  1. NoopBusinessUnitConverter.cs — Follows the existing noop pattern (e.g., NoopProviderService) by throwing NotImplementedException for all interface methods. This is appropriate since the endpoints are gated behind [SelfHosted(NotSelfHostedOnly = true)] and should never be reached in OSS builds.

  2. ServiceCollectionExtensions.cs — Registers the noop implementation in AddOosServices() using AddTransient, consistent with the commercial registration lifetime.

No bugs, security issues, or improvements to flag. ✅


@cturnbull-bitwarden cturnbull-bitwarden removed the ai-review Request a Claude code review label Mar 18, 2026
@cturnbull-bitwarden cturnbull-bitwarden marked this pull request as ready for review March 18, 2026 16:31
@cturnbull-bitwarden cturnbull-bitwarden requested a review from a team as a code owner March 18, 2026 16:31
@sonarqubecloud
Copy link

@github-actions
Copy link
Contributor

Logo
Checkmarx One – Scan Summary & Detailsf3d5202a-3620-490e-acbb-59a8e1e93e22

Great job! No new security vulnerabilities introduced in this pull request

@codecov
Copy link

codecov bot commented Mar 18, 2026

Codecov Report

❌ Patch coverage is 0% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.67%. Comparing base (0cba609) to head (2b6cb1f).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...s/NoopImplementations/NoopBusinessUnitConverter.cs 0.00% 4 Missing ⚠️
...SharedWeb/Utilities/ServiceCollectionExtensions.cs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7245      +/-   ##
==========================================
- Coverage   57.68%   57.67%   -0.01%     
==========================================
  Files        2035     2036       +1     
  Lines       89645    89650       +5     
  Branches     7993     7993              
==========================================
  Hits        51710    51710              
- Misses      36071    36076       +5     
  Partials     1864     1864              

☔ 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.

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.

1 participant