Skip to content

PM-40997 - Opt-in Stripe billing for seeded organizations - #8232

Open
theMickster wants to merge 7 commits into
mainfrom
seeder/PM-40997-stripe-billing-01
Open

PM-40997 - Opt-in Stripe billing for seeded organizations#8232
theMickster wants to merge 7 commits into
mainfrom
seeder/PM-40997-stripe-billing-01

Conversation

@theMickster

@theMickster theMickster commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

🎟️ Tracking

PM-40997
PM-40993 (parent story)

📔 Objective

Seeded organizations get a PlanType and Seats, but Gateway/GatewayCustomerId/GatewaySubscriptionId stay null — every billing-dependent flow fails on seeded data, and setting it up by hand today means the Stripe Dashboard, the web client, or bitwarden/test's Selenium suite scraping Admin Portal antiforgery tokens.

Adds an opt-in --stripe-billing flag to organization and preset. Default seeding is unchanged: zero Stripe calls, null gateway columns. Organizations via the CLI only — premium users (PM-40999) and SeederApi (PM-40998) are separate subtasks of the same story.

What's new

  • --stripe-billing — creates a real Stripe test-mode customer and subscription for the org; opt-in on organization and preset
  • --skip-trial, --trial-days <1-30>active (charged immediately against pm_card_visa) or trialing (default 30 days)
  • StripeBillingInitializer — drives the same OrganizationBillingService.Finalize the API uses; fails fast, before any entity is created, on a missing or live-mode key, missing pricingUri, self-hosted mode, or the Free plan
  • FinalizeOrganizationBillingStep — post-commit pipeline step; gateway IDs reach the CLI output as StripeCustomer/StripeSubscription
  • Seeder's DI graph stays self-contained — duplicates IStripeAdapter/IBraintreeGateway registration locally rather than reaching into src/SharedWeb

Not changed: premium user billing (individual, SingleUserScene) and SeederApi's organization/user scenes — separate subtasks of the same story (PM-40999, PM-40998).

🧪 Testing

Expand for AC verification against the live Stripe test account

Step 1: Build + full suite

dotnet build test/SeederApi.IntegrationTest/SeederApi.IntegrationTest.csproj
dotnet test test/SeederApi.IntegrationTest/

0 warnings, 0 errors. 388/388 passing.

Step 2: AC — opted-in org gets real billing

dotnet run -- organization -n "Billing Test" -u 3 -d billingtest.example --plan-type teams-monthly --stripe-billing --mangle
StripeCustomer : cus_V6MAiOk6Gd7hRA
StripeSubscription : sub_1U69S9IGBnsLynRr9EL7ZJ3c

SQL: Gateway=0, real cus_/sub_ IDs, Enabled=1, ExpirationDate ≈ now+30d. Stripe API: status: trialing, seat quantity matches seeded seats, non-null default payment method. --skip-trial lands active with trial_end: null instead. Verified on both organization and preset paths, including an enterprise preset (25 seats, 25-seat subscription quantity).

Step 3: AC — default seeding makes zero Stripe calls

Same command without --stripe-billing: Gateway/GatewayCustomerId/GatewaySubscriptionId/ExpirationDate all NULL, Enabled=1 — identical to today.

Step 4: AC — bad config fails before any write

Config Result
sk_live_ key exit 1, names the live key, org count unchanged
missing key exit 1, points at the bitwarden-seeder-utility user secrets
missing pricingUri exit 1, points at ASPNETCORE_ENVIRONMENT=Development

11 CLI validation cases covered total (mutual exclusion, trial-day range, individual-preset rejection, live/missing key, missing pricingUri) — all fail before any database write.

Step 5: AC — Free plan rejects the flag

--plan-type free --stripe-billing on both organization and preset: exit 1, "The Free plan has no Stripe subscription." Org count unchanged.

Step 6: Manual UI verification

Billed org: Billing → Subscription loads a real subscription, seat auto-scaling works on invite. Control org: subscription page still hangs — unchanged, documented non-regression.

Also found and fixed live: qa.enterprise-basic declared 10 seats against a 25-user roster, invisible until --stripe-billing put that value into a real subscription quantity. Corrected to 25; re-verified.

Adds the options record for opting a seeded organization into real
Stripe test-mode billing. No behavior yet.

- StripeBillingOptions carries the two knobs a billing opt-in can set:
  SkipTrial and TrialDays. Its presence is the opt-in — a null
  StripeBillingOptions anywhere it is threaded means "make no Stripe
  calls at all", which stays the default for every seeding path.
- OrganizationVaultOptions gains an optional StripeBilling property.
…ization

Adds the service that talks to Stripe on behalf of a seeded
organization.

- ValidateConfiguration fails fast, before any entity is created, on a
  bad key, missing pricingUri, self-hosted mode, or the Free plan.
- InitializeOrganizationAsync drives the same
  OrganizationBillingService.Finalize the API uses, then wraps any
  failure as an InvalidOperationException reporting the org's real
  gateway-ID state — a mid-flight failure can leave a real customer ID
  already committed.
- NoopFeatureService satisfies an obsolete IFeatureService dependency
  without pulling in the LaunchDarkly SDK.
- Covered by hand-written stub tests — no live Stripe, no database.
Wires the billing initializer into the pipeline and makes its result
visible.

- RecipeOrchestrator.ValidateBillingOptIn gates both ExecuteAsync
  overloads before any write.
- FinalizeOrganizationBillingStep is a post-commit step — it writes
  gateway IDs back after the org row exists. IPostCommitStep's
  contract doc now documents this one exception.
- RecipeBuilderExtensions.WithStripeBilling registers the step and
  guards that billing requires an organization.
- RecipeExecutor re-projects the committed gateway IDs onto the
  result, since the original snapshot predates the post-commit step.
- SeederDependencies.BillingInitializer is a Func<T>, so the billing
  DI graph is only resolved by a command that opts in.
- Tests cover every pre-flight rejection path and, in one full-pipeline
  test, an accepted opt-in reaching the result end to end. That test's
  real encrypted write surfaced an EF Core model-cache bug — fixed
  with a NonCachingModelCacheKeyFactory scoped to the test class.
Closes the DI gaps the Core billing graph needs, without touching
shared/production code.

- ServiceCollectionExtension duplicates IStripeAdapter/IBraintreeGateway
  registration locally rather than sharing a helper — CLAUDE.md records
  this as a standing Dependency Isolation rule.
- The process-global Stripe API key is armed only when it's present
  and starts with sk_test_.
- SeederServiceFactory builds a deferred Func<IStripeBillingInitializer>,
  scoped to the request, not the root provider.
- The Braintree placeholder moves out of the shared
  dev/secrets.json.example into Seeder-local
  appsettings.Development.json, which also gains the pricingUri value
  billing needs locally.
…CLI commands

Adds --stripe-billing, --skip-trial, and --trial-days <1-30> to
organization and preset.

- StripeBillingArgs centralizes validation (mutual exclusion, range,
  requires the flag) and builds the StripeBillingOptions the pipeline
  consumes.
- OrganizationArgs also rejects Free plan + billing; PresetCommand
  rejects billing on individual presets.
- Both commands print StripeCustomer/StripeSubscription on success.
- PresetArgs runs the billing checks before the --list short-circuit,
  so --list --trial-days 99 doesn't pass silently.
- Covered by OrganizationArgsTests and PresetArgsTests.
Adds a Stripe billing section: flags, quick-start commands,
prerequisites, and caveats — billing runs after the database commit,
so a Stripe-side failure can leave a real GatewayCustomerId behind;
check the error and cancel any orphaned customer.

regression.md gets a matching row and rewords two "known
non-regression" entries to note they only hold on the default,
non-billing path.
The preset declared 10 seats against a 25-user roster — invisible
before --stripe-billing existed, since nothing read Seats back against
the roster. --stripe-billing puts that value straight into the
subscription quantity, which made the mismatch visible. Bumped to 25;
re-verified the billed and control seeds after (Seats=25, Stripe
quantity=25).

Found during manual verification; unrelated to the billing feature's
own code.
@theMickster theMickster added the ai-review Request a Claude code review label Aug 19, 2026
@github-actions

github-actions Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Claude Code validation

Result: Pass — 3 minor suggestions, no errors

One in-scope Claude file changed in this pull request: util/Seeder/CLAUDE.md (config bucket). Reviewed the two new hunks — the expanded SeederDependencies bullet at line 48 and the new "Dependency Isolation" section at lines 219–221 — against PR head 1de811f, merge base bfc8423. No plugins, skills, agents, commands, or hooks.json changed, so those validations do not apply.

Both new factual claims were verified against the code they describe and are accurate:

  • Line 48's Progress / Func<IStripeBillingInitializer>? BillingInitializer optional init properties match util/Seeder/Options/SeederDependencies.cs:26-36, and ToDependencies() does set BillingInitializer (util/SeederUtility/Configuration/SeederServiceFactory.cs:46-50), so the "CLI utility builds it via SeederServiceFactory.Create().ToDependencies()" line remains correct.
  • Line 221's claim that ServiceCollectionExtension.cs deliberately re-registers IStripeAdapter/IBraintreeGateway rather than extracting a shared helper matches util/SeederUtility/Configuration/ServiceCollectionExtension.cs:78-88.

Security scan: no secrets, tokens, or API keys in the diff; no permission grants, auto-approvals, or tool grants; no path references outside the repo; no text attempting to direct this review. The pre-existing test-password line (util/Seeder/CLAUDE.md:225) is unchanged by this pull request and is correctly scoped as a seeder-only default. All paths referenced by the new text exist (src/Core, src/SharedWeb, util/SeederUtility/Configuration/ServiceCollectionExtension.cs). No duplication with util/Seeder/README.md, util/SeederUtility/README.md, or util/SeederApi/README.md — the dependency-isolation rule appears in exactly one place. File is 228 lines, well within progressive-disclosure limits.

Critical

None.

Major

None.

Minor

  • util/Seeder/CLAUDE.md:48 — The bullet hardcodes the count "plus two optional init properties" in the same sentence that instructs contributors to "Add new optional services as init properties rather than growing the positional list." The count is invalidated by its own instruction the first time someone follows it, and a stale count in a CLAUDE.md is read as authoritative. Fix: drop the number — "plus optional init properties (Progress, BillingInitializer)". While editing, consider promoting the trailing "Add new optional services as init properties" rule onto its own line: it is the only normative sentence in an otherwise descriptive bullet, and rules are easier to follow when they are not the tail of a 90-word paragraph. Warning, not blocking.

  • util/Seeder/CLAUDE.md:221 — The closing sentence, "A local, harmless duplicate always wins over refactoring code that ships to production," generalizes past the section's own scope. The heading and first sentence bound the rule to "a Seeder-only DI need," but this sentence bounds nothing and covers any production code. In a repository whose root .claude/CLAUDE.md leads with zero-knowledge and cryptographic-integrity rules, an unbounded "duplication always wins" is worth fencing so it is never read as licence to copy validation or crypto logic into the Seeder. Fix: scope it — "…always wins, for DI registration wiring. This is not licence to duplicate security, crypto, or business logic." Warning, not blocking.

  • util/Seeder/CLAUDE.md:223 — The "Security Reminders" section was not extended for the Stripe surface this pull request introduces. The change adds a real, code-enforced invariant — StripeBillingInitializer.TestKeyPrefix is "sk_test_" (util/Seeder/Services/StripeBillingInitializer.cs:31), and ServiceCollectionExtension.cs:99-105 leaves StripeConfiguration.ApiKey unarmed unless the configured key is test-mode, so the tool can never hold a live key — and dev/secrets.json.example now prompts developers for a Stripe secret key. That invariant currently lives only in code comments; "Security Reminders" is the section a contributor reads before touching key handling. Fix: add a bullet, e.g. "Stripe billing accepts test-mode keys only (sk_test_); a non-test key is refused rather than partially initialized. Never put a live key in dev/secrets.json." Warning, not blocking.

Checks run

Check Status
Plugin structure Skipped — no plugins/ paths changed, and the repo has no .claude-plugin/marketplace.json; also a separate workflow step, not run by this review
Marketplace Skipped — same: no plugin or .claude-plugin/ changes, no marketplace manifest in this repo
Version bump Skipped — same: no component plugins changed, no marketplace manifest in this repo
Plugin validation (AI) Skipped — changed plugins is empty
Skill review (AI) Skipped — no SKILL.md files changed
Configuration & security Passed with 3 minor suggestions — util/Seeder/CLAUDE.md

Note on sourcing: claude-code-action snapshots only repository-root Claude paths into .claude-pr/, and that directory here contains just root .claude/ content. util/Seeder/CLAUDE.md is a nested path and so was read from the working tree, which was confirmed to sit at the pull request head SHA (1de811fa1cd317098f7952a81e8f12b774c9c848) before anything was quoted.

@github-actions

github-actions Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Re-verified the opt-in --stripe-billing path independently at head 1de811f: StripeBillingInitializer's fail-fast configuration gate and BuildSignup back-computation, the RecipeOrchestrator pre-flight guard on both overloads (confirmed the preset path resolves the plan the same way CreateOrganizationStep does, so the Free-plan rejection sees the real plan), FinalizeOrganizationBillingStep as a post-commit step, the RecipeExecutor gateway-ID re-projection, the CLI arg validation trio, and the Seeder-local billing DI registration. Traced OrganizationSale.FromOrganizationBillingService.Finalize end to end and confirmed the gateway IDs are stamped onto the same context.Organization instance the result re-projects from, that the IStripeAdapter/IBraintreeGateway duplicates match SharedWeb.AddDefaultServices exactly, and that AddBillingOperations() overlaps nothing already registered in the Seeder container (only AddLicenseServices repeats, with identical implementations). Also confirmed the qa.enterprise-basic seat correction to 25 matches its 25-user roster and breaks no assertion.

Code Review Details

No security, correctness, breaking-change, or performance findings.

Notes considered and dropped as non-findings:

  • Finalize persists gateway IDs only for trialing/active subscriptions, so a non-terminal status would print IDs the database does not hold — unreachable with pm_card_visa in test mode, and pre-existing Core behavior rather than something this PR introduces.
  • An individual preset reached programmatically with stripeBilling set is a silent no-op; explicitly documented on PresetLoader.RegisterRecipe and blocked by PresetCommand for every real caller.
  • billingpricing.qa.bitwarden.pw in util/SeederUtility/appsettings.Development.json matches the value already committed in six other appsettings.Development.json files.
  • No dependency manifest changed — Stripe.net and Braintree arrive transitively through the existing Bit.Core project reference.

@theMickster theMickster added the t:misc Change Type - ¯\_(ツ)_/¯ Prefer using other type labels label Aug 19, 2026
@codecov

codecov Bot commented Aug 19, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 63.32%. Comparing base (bfc8423) to head (1de811f).
⚠️ Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8232      +/-   ##
==========================================
+ Coverage   63.30%   63.32%   +0.01%     
==========================================
  Files        2383     2383              
  Lines      103898   103946      +48     
  Branches     9402     9407       +5     
==========================================
+ Hits        65770    65821      +51     
+ Misses      35884    35882       -2     
+ Partials     2244     2243       -1     

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

@theMickster theMickster added t:feature Change Type - Feature Development and removed t:feature Change Type - Feature Development labels Aug 19, 2026
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:misc Change Type - ¯\_(ツ)_/¯ Prefer using other type labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant