[PM-19147] Automatic Tax Improvements #5510
Conversation
|
Great job, no security vulnerabilities found in this Pull Request |
src/Core/Billing/Services/Implementations/AutomaticTax/IndividualAutomaticTaxStrategy.cs
Outdated
Show resolved
Hide resolved
src/Core/Billing/Services/Implementations/AutomaticTax/IndividualAutomaticTaxStrategy.cs
Outdated
Show resolved
Hide resolved
src/Core/Billing/Services/Implementations/AutomaticTax/OrganizationAutomaticTaxStrategy.cs
Outdated
Show resolved
Hide resolved
|
|
||
| ArgumentNullException.ThrowIfNull(options); | ||
|
|
||
| if (subscription.AutomaticTax.Enabled == ShouldEnable(subscription.Customer)) |
There was a problem hiding this comment.
❓ Is this part necessary? Seems like line 31 would handle everything correctly - setting in the case it should set and unsetting in the case it should unset.
|
|
||
| public Task<SubscriptionUpdateOptions> GetUpdateOptionsAsync(Subscription subscription) | ||
| { | ||
| if (subscription.AutomaticTax.Enabled == ShouldEnable(subscription.Customer)) |
There was a problem hiding this comment.
❓ Same question here.
src/Core/Billing/Services/Implementations/AutomaticTax/OrganizationAutomaticTaxStrategy.cs
Outdated
Show resolved
Hide resolved
| @@ -605,6 +602,8 @@ public async Task<Subscription> SetupSubscription( | |||
| ProrationBehavior = StripeConstants.ProrationBehavior.CreateProrations | |||
| }; | |||
|
|
|||
| await organizationAutomaticTaxStrategy.SetCreateOptionsAsync(subscriptionCreateOptions, customer); | |||
There was a problem hiding this comment.
❌ It looks strange to me to be calling something named for organizations for providers. My suggestion is that we align your new tax strategies with our actual domain use cases:
PersonalUseTaxStrategy: Premium users and family organizationsBusinessUseTaxStrategy: Non-family organizations and providers
| sub.Id == subscriber.GatewaySubscriptionId && | ||
| !sub.AutomaticTax.Enabled) && | ||
| customer.HasTaxLocationVerified()) | ||
| customer.Subscriptions.Any(sub => sub.Id == subscriber.GatewaySubscriptionId)) |
There was a problem hiding this comment.
⛏️ You can leave this change if you want, but this code path is no longer ever invoked. This is a byproduct of necessary cleanup I haven't gotten around to.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5510 +/- ##
==========================================
+ Coverage 44.52% 44.55% +0.03%
==========================================
Files 1538 1543 +5
Lines 70582 70615 +33
Branches 6316 6317 +1
==========================================
+ Hits 31426 31463 +37
+ Misses 37810 37807 -3
+ Partials 1346 1345 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-19147
📔 Objective
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes