Skip to content

[PM-34866][PM-34865] Fix EnableAutomaticTaxAsync to update schedule phases#7437

Merged
connerbw merged 5 commits intomainfrom
billing/pm-34866/fix-schedule-aware-tax-reenable
Apr 10, 2026
Merged

[PM-34866][PM-34865] Fix EnableAutomaticTaxAsync to update schedule phases#7437
connerbw merged 5 commits intomainfrom
billing/pm-34866/fix-schedule-aware-tax-reenable

Conversation

@connerbw
Copy link
Copy Markdown
Contributor

@connerbw connerbw commented Apr 10, 2026

🎟️ Tracking

📔 Objective

Fixes two defects in EnableAutomaticTaxAsync (in both UpcomingInvoiceHandler and UpdateBillingAddressCommand):

PM-34866: Tax remains disabled when automatic tax is set to false during Phase 1. The method only updated DefaultSettings.AutomaticTax on the schedule, which does not propagate to the active phase or the underlying subscription.

PM-34865: One-time migration discount (milestone-3/milestone-2c) incorrectly reapplied on invoice.upcoming when tax is disabled during Phase 1.

Fix

Updates EnableAutomaticTaxAsync to echo all schedule phases with AutomaticTax.Enabled = true set on each phase — matching the pattern used by every other schedule-aware method in the codebase (SubscriberService.CancelSubscription, UpdatePremiumStorageCommand, ReinstateSubscriptionCommand, etc.).

Key details:

  • Completed phases are skipped (Stripe rejects updates to past phases)
  • One-time migration discounts on already-active phases are cleared to prevent Stripe from reapplying consumed coupons (same pattern as UpdatePremiumStorageCommand)
  • Phase items, dates, and proration behavior are preserved unchanged
  • Test clock FrozenTime is used for phase filtering to ensure correct behavior during QA testing with Stripe test clocks

📸 Screenshots

N/A — no UI changes.

@connerbw connerbw added the ai-review Request a Claude code review label Apr 10, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 10, 2026

Bitwarden Claude Code Review

Overall Assessment: APPROVE

This PR fixes EnableAutomaticTaxAsync in both UpcomingInvoiceHandler and UpdateBillingAddressCommand to echo subscription schedule phases with AutomaticTax.Enabled = true on each phase, rather than only updating DefaultSettings. The implementation correctly follows the established schedule-aware pattern used by UpdatePremiumStorageCommand and other billing commands. The Stripe API Expand lists are updated to include subscriptions.data.test_clock so that FrozenTime is available for phase filtering. Test coverage is thorough, validating the two-phase case, the completed-phase skip with consumed-discount clearing, and the no-schedule fallback path.

Code Review Details

No findings. The phase filtering logic, discount consumption handling, test clock support, and item/date preservation are all consistent with existing codebase patterns. The previous review finding about using subscription.TestClock?.FrozenTime instead of bare DateTime.UtcNow has been addressed in both files.

Comment thread src/Billing/Services/Implementations/UpcomingInvoiceHandler.cs Outdated
@connerbw connerbw removed the ai-review Request a Claude code review label Apr 10, 2026
@connerbw connerbw marked this pull request as ready for review April 10, 2026 13:42
@connerbw connerbw requested a review from a team as a code owner April 10, 2026 13:42
@github-actions
Copy link
Copy Markdown
Contributor

Logo
Checkmarx One – Scan Summary & Details2186b374-4df3-4e23-ba80-6e4ab95a9648

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

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 10, 2026

Codecov Report

❌ Patch coverage is 89.85507% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 58.48%. Comparing base (52ff4a7) to head (9331a7b).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...ng/Payment/Commands/UpdateBillingAddressCommand.cs 85.71% 2 Missing and 3 partials ⚠️
...Services/Implementations/UpcomingInvoiceHandler.cs 94.11% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7437      +/-   ##
==========================================
+ Coverage   58.46%   58.48%   +0.02%     
==========================================
  Files        2066     2066              
  Lines       91071    91135      +64     
  Branches     8100     8110      +10     
==========================================
+ Hits        53247    53303      +56     
- Misses      35922    35925       +3     
- Partials     1902     1907       +5     

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

@connerbw connerbw changed the title [PM-34866] Fix EnableAutomaticTaxAsync to update schedule phases [PM-34866, PM-34865] Fix EnableAutomaticTaxAsync to update schedule phases Apr 10, 2026
@connerbw connerbw changed the title [PM-34866, PM-34865] Fix EnableAutomaticTaxAsync to update schedule phases [PM-34866][PM-34865] Fix EnableAutomaticTaxAsync to update schedule phases Apr 10, 2026
@connerbw connerbw added the ai-review Request a Claude code review label Apr 10, 2026
@connerbw connerbw enabled auto-merge (squash) April 10, 2026 15:37
@sonarqubecloud
Copy link
Copy Markdown

@connerbw connerbw merged commit aad805a into main Apr 10, 2026
38 of 40 checks passed
@connerbw connerbw deleted the billing/pm-34866/fix-schedule-aware-tax-reenable branch April 10, 2026 15:50
connerbw added a commit that referenced this pull request Apr 10, 2026
…hases (#7437)

* [PM-34866] Fix EnableAutomaticTaxAsync to update schedule phases

* Use test clock frozen time for phase filtering

* Expand test_clock on customer subscription fetches
@djsmith85 djsmith85 added the t:bugfix Change Type - Bugfix label May 6, 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:bugfix Change Type - Bugfix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants