vendure-ecommerce / vendure

The commerce platform with customization in its DNA.
https://www.vendure.io
Other
5.61k stars 993 forks source link

Tax calculations incorrectly rounding #2662

Open exceeded opened 7 months ago

exceeded commented 7 months ago

So, this has only just been brought to my attention but I'm finding that nearly all of the VAT calculations within Vendure are rounding incorrectly.

My products are all set up as "Standard Tax", which is set to 20% in Vendure.

Example order

Product is £38.23 (including VAT) and £31.86 (excluding VAT).

Customer has bought 10 of the product and the price shows as £318.60 excluding VAT and £382.30 including VAT.

If you times £318.60 by 1.2 you get £382.32, not £382.30 as shown in Vendure.

Once I've applied a discount of 6.2% the results are even more strange.

Vendure is showing the correct discount amount but then comes out short on the final result.

Please see the following picture and calculation in Excel. Screenshot 2024-02-02 085759 Screenshot 2024-02-02 091032

I'd expect the final amounts to be correct. I can provide more examples. My feeling is that because the price we enter is the VAT inclusive price, the excluded price has more decimals and then Vendure rounds incorrectly using the extra decimal.

pevey commented 7 months ago

I believe the root of this issue is that the tax calculation strategy is being applied to the proratedUnitPrice, which has already been rounded. This can occur with or without a discount.

One approach could be to not round the prorated unit price, or to create a new property that might be something like rawProratedUnitPrice that is passed to the strategy.

I haven't dug into the discounting math, but a similar early rounding of the unit discounted price may be occurring there.

michaelbromley commented 7 months ago

Thanks for the detailed report.

@pevey is correct that the fundamental issue relates to the order of rounding. By default, Vendure will round monetary values first, and then multiply by the quantity.

The solution for this is that in v2 we introduced the MoneyStrategy which allows you to modify this behaviour.

So for instance if you define a MoneyStrategy like this:

import { VendureConfig, DefaultMoneyStrategy } from '@vendure/core';

class MyMoneyStrategy extends DefaultMoneyStrategy {
    round(value: number, quantity = 1): number {
        return Math.round(value * quantity);
    }
}

export const devConfig: VendureConfig = {
    //...
    entityOptions: {
        moneyStrategy: new MyMoneyStrategy(),
    },
};

With this custom strategy, the tax calculation will look like this: image

This should probably be the default but I didn't want to make such a breaking change in v2, but we can certainly make it more visible in the docs & guides.

exceeded commented 7 months ago

But then surely the version implemented currently is just wrong?

The values we got for VAT are all off, and incorrect.

I have implemented that money strategy and will let you know whether that is a fix for new orders.

pevey commented 7 months ago

This may be US-only, but taxes are required to be calculated and rounded based on the total amount, not individual lines. Are there places where this is not the case? If not, I think it makes sense to make the change, even if it is a breaking one. As is, for most people, the default does not work as expected.

OlivierPineau-qc commented 6 months ago

@exceeded Did that solve your issue ? Currently I am still having 1 cents off issues even with a custom money strategy

michaelbromley commented 1 month ago

The default rounding behaviour will be fixed in v3.1: https://github.com/vendure-ecommerce/vendure/pull/3023

@OlivierPineau-qc can you provide details of the conditions that give and off-by-1 result even with the custom money strategy?