[PM-33901] Implement schedule-aware tax handling#7319
[PM-33901] Implement schedule-aware tax handling#7319cturnbull-bitwarden merged 1 commit intomainfrom
Conversation
cturnbull-bitwarden
added
the
ai-review-vnext
| private async Task EnableAutomaticTaxAsync(Subscription subscription) | ||
| { | ||
| if (featureService.IsEnabled(FeatureFlagKeys.PM32645_DeferPriceMigrationToRenewal)) | ||
| { | ||
| var schedules = await stripeAdapter.ListSubscriptionSchedulesAsync( | ||
| new SubscriptionScheduleListOptions { Customer = subscription.CustomerId }); | ||
|
|
||
| var activeSchedule = schedules.Data.FirstOrDefault(s => | ||
| s.SubscriptionId == subscription.Id && s.Status == SubscriptionScheduleStatus.Active); | ||
|
|
||
| if (activeSchedule != null) | ||
| { | ||
| await stripeAdapter.UpdateSubscriptionScheduleAsync(activeSchedule.Id, | ||
| new SubscriptionScheduleUpdateOptions | ||
| { | ||
| DefaultSettings = new SubscriptionScheduleDefaultSettingsOptions | ||
| { | ||
| AutomaticTax = new SubscriptionScheduleDefaultSettingsAutomaticTaxOptions | ||
| { | ||
| Enabled = true | ||
| } | ||
| } | ||
| }); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| await stripeAdapter.UpdateSubscriptionAsync(subscription.Id, | ||
| new SubscriptionUpdateOptions | ||
| { | ||
| AutomaticTax = new SubscriptionAutomaticTaxOptions { Enabled = true } | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
This shared helper is duplicated in UpdateBillingAddressCommand.EnableAutomaticTaxAsync. I considered extracting it to PriceIncreaseScheduler or a standalone service, but:
- Tax enablement isn't conceptually "price increase scheduling" as it doesn't fit
IPriceIncreaseScheduler's responsibility - A new
ISubscriptionScheduleServicefor a single method felt like over-abstraction - Two call sites with ~10 duplicated lines felt acceptable given the above
If a third schedule-aware call site with this same shape appears, maybe this is worth revisiting. Feel free to disagree!
There was a problem hiding this comment.
Yeah I agree, I think it's good for now but worth revisiting if we see it pop up again.
| { | ||
| try | ||
| { | ||
| await stripeAdapter.UpdateSubscriptionAsync(subscription.Id, |
There was a problem hiding this comment.
Intentionally not schedule-aware. Provider subscriptions manage Teams/Enterprise orgs, which are unaffected by this price migration. PriceIncreaseScheduler.Schedule() explicitly skips providers (returns null), so a provider subscription will never have an active schedule from this epic.
|
Great job! No new security vulnerabilities introduced in this pull request |
cturnbull-bitwarden
removed
the
ai-review-vnext
Make tax-related subscription updates schedule-aware during the ~15-day window between invoice.upcoming and renewal. When a subscription schedule is present and the feature flag is enabled, update default_settings.automatic_tax on the schedule instead of the subscription directly. Modified paths: - UpcomingInvoiceHandler: AlignOrganizationTaxConcernsAsync, AlignPremiumUsersTaxConcernsAsync, new shared EnableAutomaticTaxAsync helper - UpdateBillingAddressCommand: EnableAutomaticTaxAsync, added IFeatureService
cturnbull-bitwarden
force-pushed
the
billing/PM-33901/schedule-aware-tax
branch
from
bd0ada9 to
845658b
Compare
|
sbrown-livefront
left a comment
sbrown-livefront
left a comment
There was a problem hiding this comment.
Thanks for calling those out. This looks good ✅
| private async Task EnableAutomaticTaxAsync(Subscription subscription) | ||
| { | ||
| if (featureService.IsEnabled(FeatureFlagKeys.PM32645_DeferPriceMigrationToRenewal)) | ||
| { | ||
| var schedules = await stripeAdapter.ListSubscriptionSchedulesAsync( | ||
| new SubscriptionScheduleListOptions { Customer = subscription.CustomerId }); | ||
|
|
||
| var activeSchedule = schedules.Data.FirstOrDefault(s => | ||
| s.SubscriptionId == subscription.Id && s.Status == SubscriptionScheduleStatus.Active); | ||
|
|
||
| if (activeSchedule != null) | ||
| { | ||
| await stripeAdapter.UpdateSubscriptionScheduleAsync(activeSchedule.Id, | ||
| new SubscriptionScheduleUpdateOptions | ||
| { | ||
| DefaultSettings = new SubscriptionScheduleDefaultSettingsOptions | ||
| { | ||
| AutomaticTax = new SubscriptionScheduleDefaultSettingsAutomaticTaxOptions | ||
| { | ||
| Enabled = true | ||
| } | ||
| } | ||
| }); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| await stripeAdapter.UpdateSubscriptionAsync(subscription.Id, | ||
| new SubscriptionUpdateOptions | ||
| { | ||
| AutomaticTax = new SubscriptionAutomaticTaxOptions { Enabled = true } | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
Yeah I agree, I think it's good for now but worth revisiting if we see it pop up again.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7319 +/- ##
==========================================
+ Coverage 57.91% 57.93% +0.01%
==========================================
Files 2045 2045
Lines 90235 90278 +43
Branches 8024 8030 +6
==========================================
+ Hits 52263 52304 +41
Misses 36105 36105
- Partials 1867 1869 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
amorask-bitwarden
left a comment
amorask-bitwarden
left a comment
There was a problem hiding this comment.
Disregard previous comment - I read something incorrectly.
3 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-33901
📔 Objective
Make tax-related subscription updates schedule-aware during the ~15-day window between
invoice.upcomingand renewal. When a subscription schedule is present and thePM32645_DeferPriceMigrationToRenewalfeature flag is enabled, updatesdefault_settings.automatic_taxon the schedule instead of the subscription directly.Changes
AlignOrganizationTaxConcernsAsyncandAlignPremiumUsersTaxConcernsAsyncnow delegate to a sharedEnableAutomaticTaxAsynchelper that checks for an active scheduleEnableAutomaticTaxAsyncupdated with the same schedule-aware pattern;IFeatureServiceadded to constructor