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

Some price rounding issue #795

Closed
Traumflug opened this issue Dec 13, 2018 · 6 comments

Comments

@Traumflug
Copy link
Contributor

commented Dec 13, 2018

Lots of description, still unclear which rounding types (back office -> Preferences -> General) get used:

https://forum.thirtybees.com/topic/2349/wrong-tax-calculation

@getdatakick

This comment has been minimized.

Copy link
Contributor

commented Dec 13, 2018

Reprosteps:

  1. Preferences > General, set

    • Round mode = Round up away from zero
    • Round Type = Round on each item
    • Number of decimals = 2
  2. Edit some product A and set

    • Pre-tax retail price = 9.55
    • Tax Rule = any rule with 22% tax
  3. Edit customer group Visitor, and add 5% discount

  4. Go to front office, and add product A to cart, quantity = 24

  5. Go to Check out

Expected results:

  • Unit price = 9.07 (9.55 - 5% == 9.0725, round(9.0725) = 9.07)
  • Line total = 217.68 (9.07 * 24 == 217.68)
  • Total Tax = 47.89 (22% of 217.68)

Actual result

  • Unit price = 9.07
  • Line total = 217.74 (9.0725 * 24 == 217.74)
  • Total Tax = 48 (24 * round(22% of 9.0725) = 24 * round(1.99595) = 24 * 2 = 48)

Screenshot:

image

@getdatakick

This comment has been minimized.

Copy link
Contributor

commented Dec 13, 2018

The first problem - Line total - can be fixed by using correct rounding precision here. But first we should investigate why @firstred changed it in the first place (commit 50081cd).

For the second problem I haven't found a solution yet.

@guestisp

This comment has been minimized.

Copy link
Contributor

commented Dec 17, 2018

The first problem - Line total - can be fixed by using correct rounding precision here. But first we should investigate why @firstred changed it in the first place (commit 50081cd).

Exactly. Why it was hard-coded to 6 regardless what is choosen from the backoffice ?

@Traumflug

This comment has been minimized.

Copy link
Contributor Author

commented Feb 12, 2019

Update:

Over the last two days I reviewed this rounding business. There were lots of hardcoded values rather than usage of constants. Problem with the rounded line was that it rounded to the wrong precision. PS used the same value for _PS_PRICE_DISPLAY_PRECISION_ and _PS_PRICE_COMPUTE_PRECISION_, which led to a number of confusions without being noticed. This part should be done and will get committed before too long.

The tax thing is more tricky. Products store their prices always with high precision. Which means, taxes have to get re-calculated after rounding, depending on what gets displayed. Currently taxes get fetched from the product and rounded after that.

Next task will be a review of orders. They apparently have their own calculations, which isn't ideal. Turning a cart into an order should take prices from the cart as-is, to avoid any potential for deviations from the cart.

@Traumflug

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2019

A cigarette and a tomato juice later, there was suddenly this:

grafik

✌️

Traumflug added a commit that referenced this issue Feb 14, 2019
Essentially, it was just a matter of applying the right rounding
precision. In PS history, computation and display precision used
to be the same, so it was also mixed up in many places.

This should partially fix issues #795 and #553. Displayed tax
amount still wrong.
Traumflug added a commit that referenced this issue Feb 14, 2019
This should fix remaining parts of issue #795.

The option to kind of re-engineer the tax rate by comparing a
price with and without tax was choosen, because applying taxes is
a pretty complex business and the last thing we need is another
place implementing it. Computation precision should be sufficient.
@Traumflug

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2019

All commits on branch 1.0.x now, this issue should be solved.

@Traumflug Traumflug closed this Feb 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.