Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make tax calculation async #307

Closed
michaelbromley opened this issue Apr 15, 2020 · 3 comments
Closed

Make tax calculation async #307

michaelbromley opened this issue Apr 15, 2020 · 3 comments

Comments

@michaelbromley
Copy link
Member

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.

@michaelbromley
Copy link
Member Author

Spent a bit of time experimenting with this. It's a hard problem:

  • The 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.
  • As mentioned in the original issue, making 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.

@michaelbromley michaelbromley modified the milestones: v0.12.0, v1.0.0 May 7, 2020
@michaelbromley michaelbromley modified the milestones: v1.0.0, v0.18.0 Dec 3, 2020
@michaelbromley
Copy link
Member Author

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:

  1. Calculating tax for the purpose of displaying Product details in list/detail views (this is what the current TaxCalculationStrategy handles)
  2. Calculating tax on OrderItems in an Order. This is the part where you might need to use TaxJar et al, or do some custom lookup like in the quote above. This is the missing piece we do not have yet.

@michaelbromley
Copy link
Member Author

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.

Status quo: summary

So we have 3 distinct things at play regarding the pricing / taxes on products in an order:

  1. TaxCalculationStrategy calculates the price of a ProductVariant based on current Channel & active tax Zone.
  2. PriceCalculationStrategy calculates the price of an OrderItem when adding a ProductVariant to an Order. By default just uses whatever price was calculated by the TaxCalculationStrategy, but can also modify price based on custom logic.
  3. not existing yet: some mechanism to actually calculate the TaxLines on each OrderItem.

Required Changes

  1. 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:

    1. TaxCalculationStrategy -> ProductVariantPriceCalculationStrategy
    2. PriceCalculationStrategy -> OrderItemPriceCalculationStrategy

    We can keep aliases of the old names for now to reduce compatibility issues.

  2. Create a new strategy for calculating TaxLines on OrderItems, which should be named TaxLineCalculationStrategy

  3. The API of 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.

  4. As part of these changes, I revisited the ProductVariant entity and discovered that we can simplify some of the price-related property handling to bring it into line with the way we now handle OrderItems. Consistency across the code base makes it much easier to read and understand.

michaelbromley added a commit that referenced this issue Dec 9, 2020
Relates to #307

BREAKING CHANGE: The TaxCalculationStrategy return value has been simplified - it now only need
return the `price` and `priceIncludesTax` properties. The `ProductVariant` entity has also been
refactored to bring it into line with the corrected tax handling of the OrderItem entity. This
will require a DB migration. See release blog post for details.
michaelbromley added a commit that referenced this issue Dec 9, 2020
Relates to #307.

BREAKING CHANGE: The `TaxCalculationStrategy` has been renamed to
`ProductVariantPriceCalculationStrategy` and moved in the VendureCofig from `taxOptions` to
`catalogOptions` and its API has been simplified.
The `PriceCalculationStrategy` has been renamed to `OrderItemPriceCalculationStrategy`.
michaelbromley added a commit that referenced this issue Dec 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant