Closed michaelbromley closed 3 years ago
Spent a bit of time experimenting with this. It's a hard problem:
TaxCalculationStrategy.calculate()
method is called many times. E.g. invoking the products
query and selecting variants
will call it once for each variant. That could be hundreds of calls in a single query. So obviously it is not feasible to e.g. call out to a 3rd party service for each of these calls.TaxCalculationStrategy.calculate()
async has a large knock-on effect to many code paths. This issue need quite a bit of thought and design to get right. The goal is to enable more powerful tax calculations using async user code, which could e.g. call out to a 3rd party tax service (e.g. https://www.taxjar.com/) or perform some other lookup e.g. in a DB or file system.
Since the tax on any given ProductVariant depends on the Customer's Zone (as determined by the TaxZoneStrategy
), and that Zone will vary from customer to customer, it cannot be known until the point that a given customer views the ProductVariant.
However, we do store per-Channel prices for each variant, and each Channel has a defaultTaxZone
. Therefore, it would be feasible to pre-compute and store the "default price with tax" and thereby only call TaxCalculationStrategy.calculate()
once any time a ProductVariant is created/updated and use that default value from that point forward.
This would mean that simply querying ProductVariants would not invoke TaxCalculationStrategy.calculate()
.
Then at some stage of the checkout, e.g. when moving to ArrangingPayment
, we could re-calculate all taxes for the cart, because it's okay if this step is a little slower.
Adding in a real-world use case that came up on Slack:
Hi Michael, we explored the use of categories and zones. However, many of the existing tax calculations our team does relies on an external API that requires an address and part number. If we were to use the tax rates and categories, we would need to make one for every postal code (which is not really feasible). The above has also helped me realize that the tax calculator does not have access to the address information. This seems like it could be more of a challenge, since this really only makes sense with respect to an order. In addition, we don't really plan to show any tax related pricing until the checkout process. So in a sense, we really only care about calculating tax once (during the checkout phase). (edited)
I think we need to distinguish between 2 separate concerns here:
TaxCalculationStrategy
handles)After the recent overhaul of tax handling (#573) I have a much clearer understanding of each part and I've concluded that the TaxCalculationStrategy is incorrectly named.
It is not really calculating taxes - it is calculating the price of a ProductVariant. Its job can be stated as:
Decide whether the listPrice of a ProductVariant should be inclusive of tax or not, based on the current Channel and active tax Zone.
That's all it does.
So we have 3 distinct things at play regarding the pricing / taxes on products in an order:
We need to rename the existing strategies and create a new one to accurately reflect their roles in the pricing lifecycle and eliminate confusion caused by ambiguity:
ProductVariantPriceCalculationStrategy
OrderItemPriceCalculationStrategy
We can keep aliases of the old names for now to reduce compatibility issues.
TaxLineCalculationStrategy
ProductVariantPriceCalculationStrategy
can actually be simplified to only return two properties: price
& priceIncludesTax
. This is all the data we need to derive the net & gross prices.
Is your feature request related to a problem? Please describe. Currently the
TaxCalculationStrategy.calculate()
method is sync. This is very limiting because it prevents the use of 3rd-party tax calculation service APIs and even custom local calculation methods which e.g. require a database lookup.Describe the solution you'd like Make the method async - able to return a result or a Promise of the result. This change will have a knock-on effect on several code paths related to calculating order totals, as the introduction of an async step then will require the entire order calculation logic to go async.