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

Fix rounding error #11006

Closed
wants to merge 1 commit into from
Closed

Fix rounding error #11006

wants to merge 1 commit into from

Conversation

steros
Copy link
Contributor

@steros steros commented Sep 22, 2017

Description

Prices should only rounded to two decimal places in view.

I doubt 2 decimal places is correct here.
If I want a product price of "165" and thus set a base price of "138.6554" with a tax of 19% I get "165.01" because of the rounding to 2 decimal places here.
Setting this to 4 decimal places fixes that issue.

The fact that it is not possible to set a price with 4 decimal places in the backend is another issue that needs to be set separately I suppose.

Manual testing scenarios

  1. set a product price of 138.6554
  2. set tax to 19%
  3. go to checkout cart
  4. final price is 165.01 instead of 165.00

I doubt 2 decimal places is correct here.
If I want a product price of "165" and thus set a base price of "138.6554" with a tax of 19% I get "165.01" because of the rounding to 2 decimal places here.
Setting this to 4 decimal places fixes that issue.

The fact that it is not possible to set a price with 4 decimal places in the backend is another issue that needs to be set separately I suppose.
@orlangur
Copy link
Contributor

The fact that it is not possible to set a price with 4 decimal places in the backend is another issue that needs to be set separately I suppose.

set a product price of 138.6554

Looks a bit contradictory, isn't it?

Some currencies have two decimals, some three, some even zero. Proposed solution may be suitable for your particular needs but in core there should be something more sophisticated covering many scenarios, see #1240 (comment)

Problem in general is tracked as https://github.com/magento/magento2/issues/10532 I believe.

@orlangur orlangur closed this Sep 22, 2017
@orlangur orlangur self-assigned this Sep 22, 2017
@orlangur orlangur added this to the September 2017 milestone Sep 22, 2017
@steros
Copy link
Contributor Author

steros commented Sep 22, 2017

I was going from the point that this is the model not the view. Magento stores prices with 4 decimal places and calculates with them in most places I have seen so far.
The fact that there are currencies with more or less decimals should not be a matter of the model but the view.
There you should be able to set whatever decimals you please.
But the rounding that takes place in the model prevents setting some prices for cases where you need 4 decimals.

@orlangur
Copy link
Contributor

The fact that there are currencies with more or less decimals should not be a matter of the model but the view.

Nope as prices/taxes/etc and corresponding business logic is model responsibility.

The fact that it is not possible to set a price with 4 decimal places in the backend is another issue that needs to be set separately I suppose.

Such changes, if approved, should be done in scope of the same effort. All necessary scenarios needs to be tested in scope of it. Not just take https://travis-ci.org/magento/magento2/jobs/278635465#L897 and change assetions to the new value :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants