[PM-18028] Attempting to enable automatic tax on customer with invalid location #5374
Conversation
This reverts commit 5562ca9
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5374 +/- ##
==========================================
+ Coverage 44.20% 44.25% +0.05%
==========================================
Files 1494 1501 +7
Lines 69024 69312 +288
Branches 6224 6293 +69
==========================================
+ Hits 30510 30677 +167
- Misses 37194 37308 +114
- Partials 1320 1327 +7 ☔ View full report in Codecov by Sentry. |
|
Great job, no security vulnerabilities found in this Pull Request |
| return false; | ||
| } | ||
|
|
||
| options.DefaultTaxRates = []; |
There was a problem hiding this comment.
⛏️
In general, I would opt for immutable operations over mutable ones like this.
Not to say that there's never a time we should mutate input params. While this encapsulates the logic to ensure that SubscriptionCreateOptions always has its DefaultTaxRates set to [], which is a good thing, it also introduces a side effect that is hidden from the caller, namely conditionally setting DefaultTaxRates and AutomaticTax on the input parameter options by reference.
Without the engineer knowing ahead of time that this side effect exists, it would be reasonable to assume that they might try to also set these values elsewhere depending on the return value of this method. This also introduces some coupling given that this side effect behavior is tightly governed by the external state, which we could argue makes it a bit harder to decouple later on should we choose.
@amorask-bitwarden has done a good job of setting up a reusable pattern to borrow from, by creating .From extensions to handle this kind of immutable mapping logic. Take OrganizationMetadataResponse as an example.
Again, this is a nit, just wanted to leave it as a thought
| /// </summary> | ||
| /// <param name="customer"></param> | ||
| /// <returns></returns> | ||
| public static bool HasTaxLocationVerified(this Customer customer) => |
There was a problem hiding this comment.
🎉 nice, I like this
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-18028
📔 Objective
Enabling automatic tax for customers without a country set or with tax rates causes issues.
📸 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