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

Do not round pre-tax amounts #901

Merged
merged 1 commit into from Feb 24, 2016
Merged

Do not round pre-tax amounts #901

merged 1 commit into from Feb 24, 2016

Conversation

mamhoff
Copy link
Contributor

@mamhoff mamhoff commented Feb 24, 2016

When rounding the pre tax amount for VAT calculations,
we run into rounding errors on creating the adjustment.

Example: We want the tax amount for $16.00 at 19% VAT
16.00 / 1.19 = 13.4453 ~ 13.45 --> 2.55 VAT
13.45 * 1.19 = 16.0055 ~ 16.01 --> 2.56 VAT (wrong wrong wrong)

Not rounding the pre tax amount fixes this rounding error.

As usual with rounding, there will still be cases where this does
not work, depending on how many digits our database stores. They
will be much rarer, though.

There is a failing test for this, but it's pending for a different
reason - in the taxation integration spec, we have the aforementioned
rounding error, but it doesn't show up because shipping rates are not
tax adjusted because of a missing contains? check.

When rounding the pre tax amount for VAT calculations,
we run into rounding errors on creating the adjustment.

Example: We want the tax amount for $16.00 at 19% VAT
16.00 / 1.19 = 13.4453 ~ 13.45 --> 2.55 VAT
13.45 * 1.19 = 16.0055 ~ 16.01 --> 2.56 VAT (wrong wrong wrong)

Not rounding the pre tax amount fixes this rounding error.

As usual with rounding, there will still be cases where this does
not work, depending on how many digits our database stores. They
will be much rarer, though.

There is a failing test for this, but it's pending for a different
reason (in the taxation integration spec, we have the aforementioned
rounding error, but it doesn't show up because shipping rates are not
tax adjusted because of a missing `contains?` check.
@tvdeyen
Copy link
Member

tvdeyen commented Feb 24, 2016

👍

2 similar comments
@Senjai
Copy link
Contributor

Senjai commented Feb 24, 2016

👍

@jhawthorn
Copy link
Contributor

👍

@cbrunsdon
Copy link
Contributor

Yea, I had a chat with @jhawthorn about this IRL and I'm cool with it too. As hawth points out it more affects euro customers than it does NA, and I definitely want to start tracking more things unrounded so I am 👍.

Thanks for pulling this bit out @mamhoff

This should probably make it into a changelog entry though.

cbrunsdon added a commit that referenced this pull request Feb 24, 2016
@cbrunsdon cbrunsdon merged commit 6faf904 into solidusio:master Feb 24, 2016
@mamhoff mamhoff mentioned this pull request Mar 16, 2016
23 tasks
@mamhoff mamhoff deleted the do-not-round-pre-tax-amount branch May 24, 2016 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants