TaxZoneStrategy does not allow for taxZone determination by Order data

[martijnvdbrug]

Describe the bug
TaxZoneStrategy does not allow for taxZone determination by Order data, because the final call to determineTaxZone is without order. Without the order argument, you'd have to resort back to a default zone, resulting in always having the defaultTaxZone on your order.

For example, when we try to determine taxZone by shippingAddress with the following code:

import { Channel, Logger, Order, RequestContext, Zone } from '@vendure/core';
import { TaxZoneStrategy } from '@vendure/core/dist/config/tax/tax-zone-strategy';

export class ShippingBasedTaxZoneStrategy implements TaxZoneStrategy {

  determineTaxZone(
    ctx: RequestContext,
    zones: Zone[],
    channel: Channel,
    order?: Order
  ): Zone {

    const zone = zones.find(zone =>
      zone.members?.find(member =>
        member.code === order?.shippingAddress?.countryCode
      )
    );

    Logger.error(`ZONE ${zone?.name}`);

    if (zone) {
      return zone;
    }

    return channel.defaultTaxZone;
  }
}

To Reproduce
Steps to reproduce the behavior:

1. Use the strategy above as TaxZoneStrategy
2. Place and fulfill an order.
3. In the logs you will see that the final log statement prints undefined because there is no order given, so the taxZone will always be the channel.defaultTaxZone

Expected behavior

1. Ultimately I would like to always have the order object, OR:
2. Alternatively, I would like to be able to return undefined from determineTaxZone, meaning the taxZone should not be changed

Environment (please complete the following information):

• @vendure/core version: 1.1.5
• Nodejs version 14
• Database (mysql/postgres etc): mysql

Additional context
Slack conversation: https://vendure-ecommerce.slack.com/archives/CKYMF0ZTJ/p1629468594214100

[martijnvdbrug]

Another option that would make it more extensible is making the function async. The function is being called alot of times, so this would have a performance impact (depending on the implementation). Could we then limit the calls to determineTaxZone, or is there a clear reason why it's called so many times?

[martijnvdbrug]

I just noticed that determineTaxZone is used for multiple purposes (correct me if I'm wrong):

1. Determine taxzone of returned products, seperated from order context. For example via getProducts in the shop-api
2. Determine taxzone of an order.

We could also seperate these into 2 functions:

1. determineDisplayTaxZone(ctx: RequestContext, zones: Zone[], channel: Channel) for product display
2. determineOrderTaxZone(ctx: RequestContext, zones: Zone[], channel: Channel, order: Order) for orders

This probably has some implications I didn't foresee, but it's food for thought 👍

[michaelbromley]

Hi Martin,

Thanks for your suggestions on this. Some thoughts:

1. The determineTaxZone() method is only called from 2 locations:

   • ProductVariantService.applyChannelPriceAndTax()
   • OrderCalculator.applyPriceAdjustments()

   and both of those are async methods. So it would be rather trivial to add the possibility of a Promise return type to it without needing to make lots of changes to calling code. As you point out, this could have a perf impact due to the number of times it gets called.

2. It gets called a lot of times because whenever we want to return ProductVariants, it is called in order to display the prices correctly. In reality we can reduce the number of calls by caching per request. We already have a RequestContextCache we can use to optimize this part.

3. Splitting into 2 calls, display / order, does conceptually make sense. It would simplify your strategy code since you would no longer need to check whether the order arg is defined. I will do a bit of investigation to see whether this would be feasible. In this case, the current determineTaxZone() would be deprecated and later removed.