Skip to content

[PM-39925] feat: scaffold the Bit.Subscription.* and Bit.Invoicing libraries for invoice preview - #8208

Open
kdenney wants to merge 24 commits into
mainfrom
billing/PM-39925/invoice-preview-scaffolding
Open

[PM-39925] feat: scaffold the Bit.Subscription.* and Bit.Invoicing libraries for invoice preview#8208
kdenney wants to merge 24 commits into
mainfrom
billing/PM-39925/invoice-preview-scaffolding

Conversation

@kdenney

@kdenney kdenney commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

🎟️ Tracking

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

📔 Objective

Sets up the library skeleton for the preview-driven cart work. There is no user-facing behavior yet: the endpoint groups map no routes, and everything sits behind a feature flag that defaults off.

  • Adds the PM36631_PreviewDrivenCart feature flag.
  • Scaffolds three libraries under src/Libraries/: Bit.Invoicing (platform tier, which will own the invoice-preview projection) plus two feature-tier endpoint-group shells, Bit.Subscriptions.User and Bit.Subscriptions.Organization.
  • Registers the feature libraries in the API host cloud-only (inside a !globalSettings.SelfHosted block). The host owns the route prefixes: the user group mounts at /account/billing/subscription/premium and the organization group at /organizations/{organizationId:guid}/billing/subscription.
  • Each group attaches its shared cross-cutting chain (tags, the Application authorization policy, exception handling, and the PM36631_PreviewDrivenCart gate) to an empty group but maps no endpoints yet; those arrive in later slices. Only an authenticated caller is required at the group level, and each future handler will perform its own organization billing authorization check.
  • Adds the new projects to the solution, a README per library, and metadata-based endpoint tests that assert the group conventions against a throwaway probe endpoint.
  • Un-ignores the Subscriptions.User directory that the stock *.user gitignore pattern was silently hiding.

All three libraries reference Core as a documented deviation from the target billing-library contract (pending Bit.Integrations.Billing, per ADR-0032); each library's README records what it takes from Core.

This branch is stacked on the Stripe SDK upgrade work (PM-36225), which is its base and should merge first.


Stack created with GitHub Stacks CLIGive Feedback 💬

@kdenney kdenney added t:feature Change Type - Feature Development ai-review Request a Claude code review labels Aug 13, 2026
@github-actions

github-actions Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Re-reviewed at the current head. The only change since the last pass is a47db11 — it deletes .claude/rules/billing-library-comment-discipline.md and folds a general "Comment discipline" subsection into .claude/skills/writing-server-code/SKILL.md, which resolves the review thread on that rule; no reference to the deleted rule file remains anywhere in the repo, and the skill's frontmatter is unchanged. The library scaffolding is otherwise unchanged and still has nil runtime blast radius: both endpoint groups map zero routes behind WithTags / WithGroupName("internal") / RequireAuthorization(Policies.Application) / WithBasicExceptionHandling() / RequireFeature(PM36631_PreviewDrivenCart), with the flag defaulting off. Service registration is unconditional in src/Api/Startup.cs per earlier feedback, while AddOpenApiEndpointDataSource and the UseEndpoints mapping stay cloud-only — and since AddOpenApiEndpointDataSource short-circuits unless swaggerGen=true (src/HttpExtensions/EndpointDataSourceServiceCollectionExtensions.cs:22-28) there is no duplicate route source at runtime, while dev/generate_openapi_files.ps1 sets swaggerGen without self-host settings so the spec path still sees the groups.

Code Review Details

No new findings this pass.

Re-verified since the previous review:

  • The new .csproj files match the shape of the existing libraries under src/Libraries/ (FrameworkReference on Microsoft.AspNetCore.App, InternalsVisibleTo for the paired test project, Bitwarden.Server.Sdk imported only where the [FlagKeyCollection] generator is needed).
  • The six new CODEOWNERS entries all assign @bitwarden/team-billing-dev, and no later pattern in the file re-matches those paths, so last-match-wins produces the intended owner.
  • The .gitignore negation !src/Libraries/Subscriptions.User/ sits after *.user and before the [Bb]in/ / [Oo]bj/ patterns, so the library sources are tracked while build output stays ignored.
  • pm-36631-preview-driven-cart is defined only in InvoicingFeatureFlags — no duplicate key in Core's FeatureFlagKeys.

Dependency review: no manifest version changes. The new .csproj files add only the shared $(XUnitVersion)-style test template already used across test/Libraries/; the lock file churn in src/Api, util/SqlServerEFScaffold, and the test projects is project-reference metadata from the new ProjectReference entries. No net-new external dependency, so no AppSec approval applies.

Comment thread test/Libraries/Invoicing.Test/Invoicing.Test.csproj
@codecov

codecov Bot commented Aug 13, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 63.31%. Comparing base (ac309aa) to head (a47db11).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8208      +/-   ##
==========================================
+ Coverage   63.29%   63.31%   +0.01%     
==========================================
  Files        2401     2406       +5     
  Lines      104043   104091      +48     
  Branches     9426     9429       +3     
==========================================
+ Hits        65857    65904      +47     
- Misses      35930    35931       +1     
  Partials     2256     2256              

☔ View full report in Codecov by Harness.
📢 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.

@kdenney
kdenney marked this pull request as ready for review August 14, 2026 03:37
@kdenney
kdenney requested a review from a team as a code owner August 14, 2026 03:37
@kdenney
kdenney requested review from a team and amorask-bitwarden and removed request for a team August 14, 2026 03:37
An error occurred while trying to automatically change base from billing/pm-36225/upgrade-stripe-sdk-to-51.1.0 to main August 14, 2026 15:10
@kdenney
kdenney force-pushed the billing/PM-39925/invoice-preview-scaffolding branch 2 times, most recently from c0c45fd to 3922c6e Compare August 17, 2026 18:19
@kdenney
kdenney requested review from a team as code owners August 17, 2026 18:19
@kdenney
kdenney requested review from mzieniukbw and sven-bitwarden and removed request for a team, mzieniukbw and sven-bitwarden August 17, 2026 18:19
@kdenney
kdenney changed the base branch from billing/pm-36225/upgrade-stripe-sdk-to-51.1.0 to main August 17, 2026 18:25
@kdenney
kdenney force-pushed the billing/PM-39925/invoice-preview-scaffolding branch from 3922c6e to dfbff32 Compare August 18, 2026 14:36

@SaintPatrck SaintPatrck left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⛏️ PR description mentions new CLAUDE.md files in every library, but I don't see any new CLAUDE.md files, or modifications to existing ones.


# Library comment discipline

Public API gets `///` doc comments — terse, but they state the contract LIBRARY.md requires:

@SaintPatrck SaintPatrck Aug 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⛏️ ❓ Pointing Claude to LIBRARY.md here is going to pull the entire file into context every time a matching path is touched, even if it's only reading a file in the path. LIBRARY.md contains more than comment-specific details, so the rule name is misleading.

This rule reads like it's relevant when Claude is writing-server-code, where LIBRARY.md is already referenced. Did you observe behavior that warranted duplicating the instructions and reference outside of writing-server-code?

@kdenney kdenney Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah originally I had per-library CLAUDE.md files and each had this comment discipline rule inside but a claude reviewer suggested I remove the duplication and add it to a rule file that covers all of billing team's libraries.

The comment rule was essentially our team's attempt at forcing claude to stop writing verbose comments all throughout code that is already self-explanatory. I don't see any mentions of code comments in writing-server-code but I'm happy to relocate this or rethink how best to achieve this if you have any suggestions. Thanks for the feedback!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

writing-server-code is probably a better place for this. Library Shape is already referenced in the skill. If Claude isn't invoking writing-server-code, not reading LIBRARY.md, or not producing the expected results when it is invoked, we should try remediating that first.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

writing-server-code doesn't really say much about the library shape itself; it links out to LIBRARY.md for that. Buuuuut you could make the argument that all server code should follow this sort of comment discipline rule, not just library code. 😄 To that end, I'll add a comment discipline section to writing-server-code that is not library specific, but let me know if you think I should take a different approach. Thanks!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in a47db11

kdenney added 24 commits August 21, 2026 11:14
Includes the automatic packages.lock.json updates for Api and Api.Test
reflecting the new project-reference graph.
…grade

The rebase onto the stripe-upgrade branch brought in a newer main where
Core swapped its direct Serilog.Extensions.Logging.File dependency for the
new SerilogFileLogging project and the internal project version moved from
2026.7.2 to 2026.8.0. The lock files that this branch had regenerated at
the old base were left stale, so `dotnet restore --locked-mode` (the CI
gate) failed with NU1004. Regenerate the ten affected lock files to record
the SerilogFileLogging project edge and the 2026.8.0 versions.
…nd add test placeholder

Minimal API endpoint groups don't get ApiExplorerGroupConvention, so without an explicit group name their future endpoints would emit into the published Public API spec (api.public.json). Set WithGroupName("internal") on both subscription groups to match the legacy PreviewInvoiceController, and assert it in the metadata tests.

Also add the placeholder test the repo convention expects so the otherwise-empty Invoicing.Test assembly doesn't fail the test run.
…in CODEOWNERS

The substring globs (**/*subscription*, **/*invoice*) don't match the
capitalized library paths, so the new src/Libraries/Invoicing and
src/Libraries/Subscriptions.{Organization,User} directories (and their
tests) were ownerless. Add explicit entries, following the
src/Libraries/OrganizationAuthorization precedent.
… a rule

The three new libraries each carried a byte-identical "Comment discipline" section in their own CLAUDE.md, so the rule lived in triplicate with nothing keeping the copies in sync and a fresh copy due for every future library.

Replace them with a single .claude/rules/billing-library-comment-discipline.md scoped via `paths:` to exactly the three new libraries (Invoicing, Subscriptions.Organization, Subscriptions.User) — matching the existing auth-tokenables and database-* rules — and delete the per-library CLAUDE.md files. The wording now states the contract LIBRARY.md requires so "terse" cannot be read as "skip the contract."

Addresses the AI validation note on #8208.
…in Bit.Invoicing

Address review feedback on PR #8208: the feature libraries no longer take
a Core dependency for the feature flag key.

- Add InvoicingFeatureFlags ([FlagKeyCollection]) in Bit.Invoicing holding
  PM36631_PreviewDrivenCart, and register it as a known flag from
  AddInvoicing via AddKnownFeatureFlags.
- Reference Bit.Invoicing's key from both subscription endpoint groups and
  remove the constant from Core's FeatureFlagKeys.
- Reference the Bitwarden.Server.Sdk meta-SDK in Bit.Invoicing so the
  [FlagKeyCollection] source generator runs; this also aligns the library
  lock files' platform package versions up to Core's baseline.
- Trim the FeatureFlagKeys row from the subscription library README debt
  tables (Policies.Application remains) and note the ownership in Invoicing.
- Replace the Invoicing.Test placeholder with a test asserting AddInvoicing
  registers the flag as a known feature flag.
…tionally in the API host

Address review feedback on PR #8208: guarding service registration behind
!SelfHosted risks a generic DI resolution failure. Register
AddUserSubscriptions/AddOrganizationSubscriptions unconditionally (they are
TryAdd singletons) and keep only the endpoint mapping and OpenAPI data
source cloud-only, so the environment decision lives at routing.
…kill

Replace the billing-scoped .claude/rules/billing-library-comment-discipline.md
with a general "Comment discipline" subsection in the writing-server-code skill.

The standalone rule fired on every touch of a matching library path, including
reads, and duplicated guidance the skill already covers. Folding it into the
skill removes the duplication and the misleading rule name, and applies the
guidance to all server code rather than only the billing libraries.

Addresses review feedback on #8208.
@kdenney
kdenney dismissed stale reviews from justindbaur and amorask-bitwarden via a47db11 August 21, 2026 16:14
@kdenney
kdenney force-pushed the billing/PM-39925/invoice-preview-scaffolding branch from 225030d to a47db11 Compare August 21, 2026 16:14
@kdenney
kdenney requested a review from SaintPatrck August 21, 2026 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request a Claude code review t:feature Change Type - Feature Development

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants