Price with tax incorrect while using BigInt strategy

[daniel-lxs]

Describe the bug
The property of ProductVariant priceWithTax is not being correctly calculated while using BigInt strategy for prices, priceWithTax is a concatenation of the price of the product variant and the calculated tax percentage of that price.

To Reproduce
Steps to reproduce the behavior:

1. Import and use BigIntMoneyStrategy
2. Add Tax value on the Vendure admin dashbboard
3. Go to product variants to check the prices
4. See error on the price presented by the dashboard

Expected behavior
The price with tax should be the sum of the numbers and not the concatenation of the 2 values as if they were a string.

Environment:

• @vendure/core : v2.1.1
• Node: v18
• Database: mysql

Additional context

[michaelbromley]

Hi,

I am not able to reproduce this so far. Can you clarify what you mean by this step?

Add Tax value on the Vendure admin dashbboard

[alebak]

Hi @michaelbromley, @daniel-lxs indicates is that you add or adjust the value on the tax page. In Colombia the tax is 19%.

So the second step was to adjust that value:

Once the tax has been adjusted. When checking the product and its variants, this price with tax appears in the admin-ui:

This product costs $40,000 COP, expressed in cents is 4000000 and the tax of this product, expressed in cents, is 760000.

What we assume is that Vendure in GraphQL is concatenating these values 4000000 + 760000 = 4000000760000

On the other hand, when we enter a variant to check the tax, the admin-ui correctly calculates the price with tax. We assume that this calculation is based on the price and the calculation is done correctly by the frontend (Angular):

And in the order it is shown like this:

[michaelbromley]

I've spent some more time trying to reproduce this:

• Used MySQL to match your environment
• Configured server to use BigIntMoneyStrategy
• Set default channel defaultCurrencyCode to COP
• changed standard tax rate to several different values
• tried with both tax-inclusive & exclusive pricing in the default channel settings

I'm not seeing the error. So looking at the code paths involved in calculating the tax-inclusive price of a variant, we can look at the ProductPriceApplicator class:

vendure/packages/core/src/service/helpers/product-price-applicator/product-price-applicator.ts

Lines 88 to 94 in fee995c

	const { price, priceIncludesTax } = await productVariantPriceCalculationStrategy.calculate({
	inputPrice: channelPrice.price,
	taxCategory: variant.taxCategory,
	productVariant: variant,
	activeTaxZone,
	ctx,
	});

This is making a call to the configured ProductVariantPriceCalculationStrategy. Do you have a custom one defined?

[daniel-lxs]

Hi, thank you for taking a look at this. We are not using a custom ProductVariantPriceCalculationStrategy or any other catalogOptions on our vendure. Is there any more information we can provide to hopefully help you figure it out?
We are using a number of plugins that we developed but we haven't changed anything related to variant price calculation.

[michaelbromley]

Are you able to run the server locally in debug mode and set a breakpoint in the product-price-applicator.js file in your node_modules, and then step through and see where the price gets concatenated?

[daniel-lxs]

Hello, the problem was happening on a deployed docker container, I tried replicating the problem but couldn't, I had a feeling that the image we were using was the problem, we changed from node:18-alpine to 21-alpine3.18 and that seems to have fixed the issue, maybe something with that image was causing the problem but I am not sure.
I think for that for some reason the Math.round(value * quantity); on the bigint-money-strategy.ts file was adding the numbers instead of multiplying them.
I also noticed a difference with the default-money-strategy.ts, in this file the rounding is done like Math.round(value) * quantity.

[michaelbromley]

Wow that's bizarre! I really cannot begin to understand what might have caused this, that was fixed by changing the docker image 😱

The reason the default strategy uses a different rounding method is to keep backwards compatibility with Vendure v1.x behaviour. Both approaches could be considered valid.

I'm going to close this now since we don't have a reliable reproduction. If it ever re-occurs and you have any other insight into related changes, we can reopen.

[michaelbromley]

Reproduction: https://github.com/carathorys/money-strategy-error

[daniel-lxs]

The amounts shown in the reproduction and the ones we had are the result of the sum of the two numbers instead of the product: Math.round(num1 + num2).
Also I would try and change the version of node used on the docker image and see if this error only happens with versions < 18.

[michaelbromley]

OK, thanks to the reproduction repo from @carathorys, I was able to get to the bottom of this:

It only happens when the custom MoneyStrategy is set via a plugin's configuration function. When directly setting the strategy in the VendureConfig object, it will work.

The reason that it breaks is that:

1. During bootstrap, in the preBootstrapConfig() function which is responsible for assembling the final config object, we have a call to setMoneyStrategy:

   vendure/packages/core/src/bootstrap.ts

   Line 153 in 9b6dce6

   setMoneyStrategy(moneyStrategy, entities);

2. However, this call is made before the line that collects all the config settings from plugins:

   vendure/packages/core/src/bootstrap.ts

   Line 159 in 9b6dce6

   config = await runPluginConfigurations(config);

3. For this reason, when setMoneyStrategy() is called, the custom strategy defined in the plugin is not yet known to Vendure. The setMoneyStrategy() function is responsible for setting up the the TypeORM column definition for all money columns, including (importantly for this issue) the transformer functions
4. So when storing as a bigint, the value is returned from the DB as a string type like '12345'. The transformer.from function of the BigIntMoneyStrategy should then coerce that numeric string into a number.
5. However, due to the ordering issue outlined in 1 & 2 above, the transformers are never registered. Thus all addition operations are performed on strings, not numbers, resulting in the concatenation behaviour.

The solution in the referenced commit is to switch the order of operations in the bootstrap sequence to ensure we have collected all the plugin configs before we set the money strategy.