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

Floating point overflows in checkout totals fixed #18185

Merged
merged 8 commits into from Jan 27, 2019

Conversation

@jayankaghosh
Copy link
Contributor

commented Sep 21, 2018

Description

Round the total amount to 4 decimal places in \Magento\Quote\Model\Quote\Address\Total::setTotalAmount, to avoid floating point overflows

Fixed Issues (if relevant)

  1. #18027: Cart Total is NaN in some circumstances
  2. ...

Manual testing scenarios

  1. Put two items in the cart, in our case this is one product at 59,80€ incl. Tax and one product at 9,49€ incl. Tax, Shipping is free
  2. Apply a coupon code (cart price rule) with 100% discount
  3. This usually results in a floating point overflow and the grand total coming as 1.4210854715202e-14 instead of 0
  4. After this fix, the grand total should come 0
@magento-engcom-team

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2018

Hi @jayankaghosh. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento-engcom-team give me test instance - deploy test instance based on PR changes
  • @magento-engcom-team give me $VERSION instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

@jayankaghosh

This comment has been minimized.

Copy link
Contributor Author

commented Sep 21, 2018

@magento-engcom-team give me 2.2.5 instance

@magento-engcom-team

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2018

Hi @jayankaghosh. Thank you for your request. I'm working on Magento 2.2.5 instance for you

@magento-engcom-team

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2018

Hi @jayankaghosh, here is your Magento instance.
Admin access: https://i-18185-2-2-5.engcom.dev.magento.com/admin
Login: admin Password: 123123q
Instance will be terminated in up to 3 hours.

@jayankaghosh

This comment has been minimized.

Copy link
Contributor Author

commented Sep 21, 2018

@magento-engcom-team give me test instance

@magento-engcom-team

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2018

Hi @jayankaghosh. Thank you for your request. I'm working on Magento instance for you

@magento-engcom-team

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2018

Hi @jayankaghosh, here is your new Magento instance.
Admin access: https://pr-18185.engcom.dev.magento.com/admin
Login: admin Password: 123123q

@jayankaghosh

This comment has been minimized.

Copy link
Contributor Author

commented Sep 21, 2018

@magento-engcom-team tested on the test instance and worked like a charm
screenshot from 2018-09-22 00-16-41

jayankaghosh added some commits Sep 21, 2018

@sidolov

This comment has been minimized.

Copy link
Contributor

commented Sep 28, 2018

Hi @jayankaghosh , please, take a look at failed tests, looks like failures related to your changes.
Thanks!

@josefbehr josefbehr self-assigned this Oct 6, 2018

@sivaschenko
Copy link
Contributor

left a comment

Please remove unnecessary comments

@@ -54,6 +55,9 @@ public function __construct(
*/
public function setTotalAmount($code, $amount)
{
/* (Fixes issue #18027) Round the total amount to 4 decimal places, to avoid floating point overflows */

This comment has been minimized.

Copy link
@sivaschenko

sivaschenko Nov 14, 2018

Contributor

These comments are not necessary for final codebase, please remove them from both functions

@sivaschenko

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2018

Hi, @jayankaghosh , thanks for your contribution, can you please remove the unnecessary comments as mentioned in code review and fix the static tests build:

app/code/Magento/Quote/Model/Quote/Address/Total.php
10 | ERROR | [x] Short description should not be in multi lines
177 | ERROR | [x] There must be exactly one blank line between lines
@jayankaghosh

This comment has been minimized.

Copy link
Contributor Author

commented Nov 14, 2018

Hi, @jayankaghosh , thanks for your contribution, can you please remove the unnecessary comments as mentioned in code review and fix the static tests build:

app/code/Magento/Quote/Model/Quote/Address/Total.php
10 | ERROR | [x] Short description should not be in multi lines
177 | ERROR | [x] There must be exactly one blank line between lines

I will remove the unnecessary comments from the methods. Not sure what I can do for the Short description issue

@magento-engcom-team

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2018

Hi @sivaschenko, thank you for the review.
ENGCOM-3464 has been created to process this Pull Request

@magento-engcom-team magento-engcom-team merged commit 1d8d568 into magento:2.3-develop Jan 27, 2019

3 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@contribution-survey

This comment has been minimized.

Copy link

commented Jan 27, 2019

Hi @jayankaghosh, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

@magento-engcom-team

This comment has been minimized.

Copy link
Contributor

commented Jan 27, 2019

Hi @jayankaghosh. Thank you for your contribution.
We will aim to release these changes as part of 2.3.1.
Please check the release notes for final confirmation.

magento-engcom-team pushed a commit that referenced this pull request Jan 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.