Feat(medusa): tax inclusive pricing in shipping method tax

[pKorsholm]

What

• consider wether or not a price is tax inclusive for shipping methods

How

• extend getShippingOptionPricing with fields price_includes_tax and tax_amount to resulting pricedShippingOption
  • handle tax inclusive price calculations accordingly
• extend strategies/tax-calculations to handle shipping option taxes according to if includes_tax is set on the shipping option

Testing

• unit testing tax calculation service
• integration testing:
  • listing shipping options admin and store

Fixes CORE-374

[changeset-bot]

⚠️ No Changeset found

Latest commit: 75f46fa

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[adrien2p]

few comments :)

[adrien2p]

if you grab the util I have created here we can simplify it as follow

    const includesTax = const includesTax =
      this.featureFlagRouter.isFeatureEnabled(
        TaxInclusivePricingFeatureFlag.key
      ) && shippingOption.includes_tax

    const taxAmount = calculatePriceTaxAmount({ price, taxRate: rate, includesTax })
    const totalInclTax = price + (!shippingOption.includes_tax ? taxAmount : 0)

    return {
      ...shippingOption,
      price_incl_tax: totalInclTax,
      tax_rates: shippingOptionRates,
      tax_amount: taxAmount,
      price_includes_tax: includesTax,
    }

wdyt?

[pKorsholm]

I opted for the following, not setting the price_includes_tax if not applicable by featureflag but otherwise I agree (though I changed the ternary a bit) 😄

   const includesTax =
      this.featureFlagRouter.isFeatureEnabled(
        TaxInclusivePricingFeatureFlag.key
      ) && shippingOption.includes_tax

    const taxAmount = calculatePriceTaxAmount({
      taxRate: rate,
      price,
      includesTax,
    })
    const totalInclTax = includesTax ? price : price + taxAmount

    const result: PricedShippingOption = {
      ...shippingOption,
      price_incl_tax: totalInclTax,
      tax_rates: shippingOptionRates,
      tax_amount: taxAmount,
    }

    if (
      this.featureFlagRouter.isFeatureEnabled(
        TaxInclusivePricingFeatureFlag.key
      )
    ) {
      result.price_includes_tax = shippingOption.includes_tax
    }

[adrien2p]

Perfect 👍 i am good with that too, it was mainly the rest

[adrien2p]

it seams that in the totals service the method getShippingTotal return in fact the subtotal of the shipping.
we can see line 342 in the totals, that the tax rate is applied on the sum including the shipping total. the price of the shipping method can include the taxes and therefore they would be applied twice in that context.

I think that this method should only return the amount without taxes like the getSubTotal method does now. then in other cases, line 372 we are calling the calculate method from the strategy which will return the tax amount (taking into account the inclusivity or not which is fine) which I think confirmed the hypotheses above.

wdyt?
@srindom can you confirm this hypothesis

[adrien2p]

Ok @pKorsholm confirmed by @carlos-r-l-rodrigues pr 😅 i think there is some overlap on your pr's both 😂

[srindom]

Nice work! Think @adrien2p has a point about handling taxes in the totals service although I don't think it necessarily has to be handled in this PR. Wdyt?

[srindom]

thought: I think we would benefit from making the util pure by not importing the feature flag, but instead just making includesTax handle the calculation regardless of what feature flags are enabled. The responsibility to call it correctly will then fall onto the callee. Wdyt?

[srindom]

I know that this is from a previous PR, but just didn't think of it before now :D

[adrien2p]

I totally agree, the flag should be part of the condition passed to the includesTax options to the util ^^ i ll make a pr tomorrow to change it where it is used so that it does not impact the dependent pr’s 😀 is that ok?

[adrien2p]

praise: 😂

[adrien2p]

I totally agree, the flag should be part of the condition passed to the includesTax options to the util ^^ i ll make a pr tomorrow to change it where it is used so that it does not impact the dependent pr’s 😀 is that ok?

[pKorsholm]

Nice spotting @adrien2p of the taxes in the totals service. I think it’s not necessary to fix in this pr as @srindom states, but important to keep in mind.

[srindom]

LGTM - @pKorsholm feel free to merge :)

@adrien2p make sure to add the fix to the util.

The totals fix should maybe be part of CORE-376 cc: @carlos-r-l-rodrigues

[adrien2p]

LGTM - @pKorsholm feel free to merge :)

@adrien2p make sure to add the fix to the util.

The totals fix should maybe be part of CORE-376 cc: @carlos-r-l-rodrigues

I ve done the PR 👍 i have seen after my message that @carlos-r-l-rodrigues included the changes. I ll review it in time.