[PM-31645] Implement Swiss Tax Logic#7186
Conversation
|
New Issues (2)Checkmarx found the following issues in this Pull Request
|
|
@sbrown-livefront You've got some failing tests on here. Wanna take care of those before I review? |
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7186 +/- ##
==========================================
+ Coverage 57.38% 57.51% +0.12%
==========================================
Files 2031 2032 +1
Lines 89282 89299 +17
Branches 7936 7946 +10
==========================================
+ Hits 51237 51356 +119
+ Misses 36204 36088 -116
- Partials 1841 1855 +14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Claude finished @sbrown-livefront's task in 4m 41s —— View job Pull Request Review: PM-31645 Implement Swiss Tax LogicSummaryThis PR centralizes tax exemption logic into a new The centralization approach is well-designed -- the Findings
Test CoverageTest coverage is comprehensive. The PR adds unit tests for:
Codecov reports 75% patch coverage with 27 uncovered lines, which is reasonable for this type of change. Notes
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| if (isBusinessUseSubscriber) | ||
| { | ||
| var determinedTaxExemptStatus = TaxHelpers.DetermineTaxExemptStatus(customer.Address.Country, customer.TaxExempt); |
There was a problem hiding this comment.
customer.Address.Country will throw if Address is null.
The original code used pattern matching (case { Address.Country: ... }), which is inherently null-safe. The refactored code extracts customer.Address.Country directly before the switch, reintroducing the NRE risk that commit 5c17069c0 fixed in OrganizationBillingService.cs (where customer.Address?.Country is correctly used).
The same issue exists in 3 other files:
src/Core/Billing/Services/Implementations/StripePaymentService.cs:126--sub.Customer.Address.Countrysrc/Billing/Services/Implementations/UpcomingInvoiceHandler.cs:165--customer.Address.Countrysrc/Billing/Services/Implementations/UpcomingInvoiceHandler.cs:458--customer.Address.Country
All four call sites should use the null-conditional operator, consistent with the fix already applied in OrganizationBillingService.cs.
| var determinedTaxExemptStatus = TaxHelpers.DetermineTaxExemptStatus(customer.Address.Country, customer.TaxExempt); | |
| var determinedTaxExemptStatus = TaxHelpers.DetermineTaxExemptStatus(customer.Address?.Country, customer.TaxExempt); |
There was a problem hiding this comment.
I know by this point it's a virtual guarantee that the customer will have an Address, but since it's nullable in Stripe, this is probably worth addressing to be safe.
amorask-bitwarden
left a comment
amorask-bitwarden
left a comment
There was a problem hiding this comment.
The logic looks good to me - I think Claude's NRE is easy enough to add, but there's also a couple missed spots:
UpgradePremiumToOrganizationCommand:205
UpdateOrganizationSubscriptionCommand:162 (New)
We're also doing similar, but not exactly the same, operations in a few different places. I don't think we need to fix that here, but might be worth filing a Tech Debt ticket to resolve it if possible.
|
|
||
| if (isBusinessUseSubscriber) | ||
| { | ||
| var determinedTaxExemptStatus = TaxHelpers.DetermineTaxExemptStatus(customer.Address.Country, customer.TaxExempt); |
There was a problem hiding this comment.
I know by this point it's a virtual guarantee that the customer will have an Address, but since it's nullable in Stripe, this is probably worth addressing to be safe.
4 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-31645
📔 Objective
This pull request refactors tax exemption logic across billing-related services and validation models to centralize country determination . The main change is replacing direct country checks (previously hardcoded for the United States) with calls to the new
TaxHelpersutility. Tax exemption status is now consistently determined usingTaxHelpers.DetermineTaxExemptStatus, ensuring correct handling for various countries and scenarios.Bit.Core.Billing.Tax.Utilitiesimports throughout affected files to support the new utility methods.📸 Screenshots
Trigger Billing Warnings for Required TaxId When Updating Organization Billing Address
Screen.Recording.2026-03-06.at.5.12.32.PM.mov
No Billing Warnings for Swiss Organization When Updating Organization Billing Address
Screen.Recording.2026-03-06.at.5.14.29.PM.mov
Update Tax Exempt Status When Updating Organization Billing Address
Screen.Recording.2026-03-06.at.5.21.47.PM.mov
Update Tax Exempt Status When Upgrading Organization
Screen.Recording.2026-03-06.at.5.25.01.PM.mov
Trigger Billing Warnings for Required TaxId When Updating Provider Billing Address
Screen.Recording.2026-03-06.at.5.28.51.PM.mov
Create Client Organization With the Correct Tax Exempt Status (Provider Tax Exempt Status)
Screen.Recording.2026-03-06.at.5.41.18.PM.mov
Upcoming Invoice Handler Retains Manually Set Tax Exempt Status Swiss Organization (Works the Same for Providers)
Screen.Recording.2026-03-09.at.2.31.22.PM.mov
Upcoming Invoice Handler Correctly Sets Tax Exempt Status For International Organization (Works the Same for Providers)
Screen.Recording.2026-03-09.at.2.35.39.PM.mov