Skip to content

[PM-41064] fix: Preserve discounts across subscription schedule rebuilds - #8278

Open
amorask-bitwarden wants to merge 20 commits into
mainfrom
billing/PM-41064/schedule-rebuild-discounting-defects
Open

[PM-41064] fix: Preserve discounts across subscription schedule rebuilds#8278
amorask-bitwarden wants to merge 20 commits into
mainfrom
billing/PM-41064/schedule-rebuild-discounting-defects

Conversation

@amorask-bitwarden

@amorask-bitwarden amorask-bitwarden commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

🎟️ Tracking

Resolves PM-41064.

📔 Objective

Several billing operations rebuild a Stripe subscription schedule wholesale — every phase is replaced, so any discount not re-listed on the rebuilt phases was silently lost. In some paths this also re-minted a consumed one-time coupon or stacked the customer coupon onto the active phase (an over-discount on the current period).

This PR centralizes the discount-carry rules in DiscountExtensions and applies them at every schedule-rebuild site:

  • The active/current phase carries only live subscription discounts, by discount id — never the customer coupon — so a coupon already cascading isn't stacked onto the current period and a consumed one-time coupon isn't re-minted.
  • Future phases carry the customer coupon + live discounts (by id) + preserved/new coupons.
  • Item-level discounts are preserved on rebuilt phase items (by coupon id).
  • Builders return null (omit the field) instead of an empty array, so "no computed discounts" no longer deletes existing ones.
  • A guard rejects subscriptions loaded without the required discount/customer/test-clock expansions instead of silently dropping discounts.

Sites updated: premium storage change, churn-mitigation redeem, annual-upgrade redeem, org subscription/seat update, billing-address update, and the upcoming-invoice / price-increase scheduler that creates the migration schedules.

Verified end-to-end against Stripe test mode across all affected flows.

Note: manual testing surfaced one pre-existing, out-of-scope gap — the migration creation path drops item-level discounts when building the new-plan phase, while annual-upgrade preserves them — tracked separately.

📸 Screenshots

N/A — no UI changes.

Backs the schedule-rebuild discount carry-over fix: live discounts carry by
discount id at subscription and phase scope, coupon id at item scope, and never
an empty array. Callers migrate in later commits; legacy helpers stay for now.
BuildMirroredPhaseOptions re-emitted the current phase's recorded coupon
ids, which re-mints a one-time coupon already consumed on the current
invoice (verified on a Stripe test clock). Carry only what is still live
on the subscription, by discount id, so a consumed coupon carries nothing.
…e active phase

BuildPhaseLevelDiscounts injects the customer's account coupon, which an
active phase's explicit discount would otherwise suppress -- listing it
newly stacks it onto the current period (verified on a Stripe test clock).
Carry only the live discounts on the active phase via a new shared
BuildCurrentPhaseDiscounts helper; keep the full builder for future phases,
which need the customer coupon re-listed to survive their own discounts.
The churn command's current-phase carry now uses the same helper.
…test

The customer coupon reads only from customer.discount.source.coupon; the
discounts.source.coupon expand is unrelated to that assertion.
…d doc

BuildPhaseLevelDiscounts already skips null/empty coupon ids, so drop the
array-spread-Where-Select ceremony at the three PriceIncreaseScheduler call
sites in favor of conditional collection expressions. Tighten the
RequireScheduleDiscountExpansions doc: it detects unexpanded discount stubs
and a missing customer, not an absent discounts field.
…hase

_PreservesPhase1AsIs now sets both a live subscription discount and a
customer coupon, asserting the active phase carries only the live discount
by id -- command-level parity with the premium-storage coverage.
…phase

CreateAndConfigureScheduleAsync built phase 1 (the active period) with the
full BuildPhaseLevelDiscounts, injecting the customer coupon on top of any
live subscription discount and stacking the two. Carry only the live
discounts on the active phase via BuildCurrentPhaseDiscounts, matching the
storage and churn rebuild paths; phase 2 keeps the full builder.
@amorask-bitwarden amorask-bitwarden added the ai-review Request a Claude code review label Aug 27, 2026
@github-actions

github-actions Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Re-reviewed after 3bed1e0 and c194de8, which resolve the open finding from the previous pass: UpcomingInvoiceHandler, UpdateBillingAddressCommand, and UpdateOrganizationSubscriptionCommand now dispatch the active phase to BuildCurrentPhaseDiscounts instead of BuildPhaseLevelDiscounts, so all six rebuild sites match the rule documented on that helper and the customer coupon is no longer stacked onto the already-billing period. Re-traced the active-vs-future dispatch at every site, including migrationPhases[0]/[1] in BuildUpdatedPhases (where phase1IsPostMigration previously wrote an explicit [] and now carries live subscription discounts by id, so a consumed one-time migration coupon is not re-minted), UpdatePremiumStorageCommand.DiscountsForPhase when phase 1 has already ended, and PriceIncreaseScheduler.CreateAndConfigureScheduleAsync. Re-verified the expansion preconditions hold at each entry point — the churn, premium-storage, org-subscription, and upcoming-invoice fetches all include discounts.source.coupon plus a customer expansion; UpdateBillingAddressCommand adds subscriptions.data.discounts.source and assigns the already-fetched customer before the guard; UpdateOrganizationSubscriptionCommand.HasRequiredExpansions now fails a stub-discount or unexpanded-test-clock subscription into a re-fetch; and PriceIncreaseScheduler's two ResolvePhase2… entry points throw their equivalent inline check before BuildCurrentPhaseDiscounts can dereference a null discount stub. Both InvalidOperationException throw sites remain contained (BaseBillingCommand.HandleAsync and the existing try/catch around EnableAutomaticTaxAsync in the webhook handler); the churn command's new set-based BuildCouponFootprint no-op check is equivalent to the old ordered comparison on every case I could construct, and the three flipped tests plus the reworded rationales now agree with the BuildCurrentPhaseDiscounts doc comment rather than contradicting it.

No security, zero-knowledge, or data-exposure concerns; the new and reworded log statements carry only Stripe ids and counts. No dependency manifest, Claude configuration, or skill files in this diff. The c194de8 changes are cosmetic (ternary dispatch, Where in the footprint loop) and behavior-preserving.

No findings at or above the reporting bar.

@amorask-bitwarden amorask-bitwarden added the t:bugfix Change Type - Bugfix label Aug 27, 2026
Comment thread src/Billing/Services/Implementations/UpcomingInvoiceHandler.cs Outdated
Comment thread src/Core/Billing/Extensions/DiscountExtensions.cs Dismissed
Comment thread src/Core/Billing/Extensions/DiscountExtensions.cs Dismissed
Comment thread src/Core/Billing/Extensions/DiscountExtensions.cs Dismissed
@codecov

codecov Bot commented Aug 27, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.12195% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.51%. Comparing base (1c8eebd) to head (c194de8).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/Core/Billing/Extensions/DiscountExtensions.cs 92.94% 0 Missing and 6 partials ⚠️
...tion/Commands/RedeemChurnMitigationOfferCommand.cs 95.34% 0 Missing and 2 partials ⚠️
...eOffer/Commands/RedeemAnnualUpgradeOfferCommand.cs 85.71% 0 Missing and 1 partial ⚠️
...ng/Premium/Commands/UpdatePremiumStorageCommand.cs 94.44% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8278      +/-   ##
==========================================
+ Coverage   63.48%   63.51%   +0.03%     
==========================================
  Files        2430     2430              
  Lines      104928   105004      +76     
  Branches     9505     9521      +16     
==========================================
+ Hits        66614    66694      +80     
+ Misses      36034    36028       -6     
- Partials     2280     2282       +2     

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

…ive phase

The upcoming-invoice handler, billing-address update, and org subscription
update still built the active phase with BuildPhaseLevelDiscounts, injecting
the customer coupon on top of any live subscription discount and stacking the
two. Carry only the live discounts on the active phase via
BuildCurrentPhaseDiscounts, matching the premium-storage, churn, and migration
scheduler paths; future phases keep the full builder. Flip the three tests that
pinned the old carry-everywhere behavior, plus the consumed-milestone case that
also asserted the customer coupon on the active phase.
@amorask-bitwarden
amorask-bitwarden marked this pull request as ready for review August 28, 2026 14:52
@amorask-bitwarden
amorask-bitwarden requested a review from a team as a code owner August 28, 2026 14:52

@cyprain-okeke cyprain-okeke 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.

Looking good, the per-scope carry rules read clearly and the expansion guard is nice

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:bugfix Change Type - Bugfix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants