Skip to content

Change Precisions to 4 to solve #10790 #11634

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

Closed
wants to merge 2 commits into from
Closed

Change Precisions to 4 to solve #10790 #11634

wants to merge 2 commits into from

Conversation

jonashrem
Copy link
Contributor

@jonashrem jonashrem commented Oct 22, 2017

Change Precisions to 4 to solve #10790

Description

I changed the rounding precision in app/code/Magento/Directory/Model/PriceCurrency.php to 4, to avoid issues with rounding in some situations. See #10790

Fixed Issues (if relevant)

solves #10790

Manual testing scenarios

Download the Dump provided here #10790 (comment)
Install Magento and use this dump as Database
Add 2 of each Product A and B to you shopping card
Apply Code "100percent"
Total Order should be 0

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@jonashrem
Copy link
Contributor Author

Can please someone more experienced have a look at this?

After my investigation, I think this is the only way to avoid the issue and as far as I can tell, this works, but it could break functionality somewhere else.

@orlangur
Copy link
Contributor

Hi @jonashrem, although this change may fix some particular scenarios, it may break some other which work correctly now. Something more sophisticated and configurable is needed instead.

Please check #11006 (comment) for more details.

@orlangur orlangur closed this Oct 23, 2017
@orlangur orlangur self-assigned this Oct 23, 2017
@orlangur orlangur added this to the October 2017 milestone Oct 23, 2017
@jonashrem
Copy link
Contributor Author

Hi @orlangur

I see, thank you for the feedback.

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