Skip to content

Make rounding more accurate when prices are entered more than 2dp #29318

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.

Sign up for GitHub

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

Jump to bottom
Merged
jonathansadowski merged 17 commits into from
May 12, 2021
Merged

Make rounding more accurate when prices are entered more than 2dp #29318

jonathansadowski merged 17 commits into from
May 12, 2021

Conversation

@vedanshujain
Copy link
Contributor

@vedanshujain vedanshujain commented Mar 10, 2021 •
edited
Loading

All Submissions:

Changes proposed in this Pull Request:

This PR introduces the following changes:

  1. Remove rounding from many places, where the values would be already rounded depending upon settings. I have added inline comments when applicable, but feel free to point in case anything can be explained more.
  2. Added tax rounding before displaying the prices to the user.
  3. Unit tests for the above changes.
  4. Fixed some older unit tests as they were failing new tests when all tests are run.

Closes #24184 #28292.

How to test the changes in this Pull Request:

I have tried to cover most things via unit tests but some example configurations to verify

From 24184 first comment

  1. Set the rounding to subtotal level, and price entered exclusive taxes from WooCommerce > Settings > Tax.
  2. Add a tax rate of 20%.
  3. Add a product for price 30.825. Add it to cart.
  4. Checkout the cart and check that prices are 36.99 with this patch. Without this patch they would be 37.

From #28292

  1. Set the rounding to subtotal level, and price entered inclusive taxes from WooCommerce > Settings > Tax.
  2. Add a tax rate of 23%.
  3. Set the decimals displayed to 0 from WooCommerce > Settings > General
  4. Add a product for 301.90909. Add it to cart.
  5. Checkout the cart and check that prices everywhere is 302. Without this patch they would be 301.

Edit: Updated testing instruction with correct configuration of inclusive/exclusive setting.

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully run tests with your changes locally?

Changelog entry

Fix - Removed rounding at several places to better support precision when prices are entered more than 2dp.

@vedanshujain vedanshujain force-pushed the fix/rounding branch 2 times, most recently from fc72bd8 to 0c46b71 Compare March 10, 2021 09:48
@vedanshujain vedanshujain changed the title Fix/rounding Make rounding more accurate when prices are entered more than 2dp Mar 10, 2021
@vedanshujain vedanshujain marked this pull request as ready for review March 10, 2021 14:07
@vedanshujain vedanshujain requested review from a team and ObliviousHarmony and removed request for a team March 16, 2021 12:27
@wpniall wpniall mentioned this pull request Mar 26, 2021
Closed
@ObliviousHarmony ObliviousHarmony requested review from jonathansadowski and removed request for ObliviousHarmony April 2, 2021 19:15
jonathansadowski
jonathansadowski reviewed Apr 8, 2021
View reviewed changes
Copy link
Contributor

@jonathansadowski jonathansadowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested this, and went through the tests you added / updated to make sure that they made sense to me, and everything works well. Nice job.

I added a question about the changes that remove wc_format_decimal from the WC_Cart methods that were changed. It seems like a breaking change to me. I'm not sure if we should introduce new methods without the rounding, or if perhaps that change isn't breaking in some way I'm missing at first glance.

includes/class-wc-cart.php Outdated
*/
public function set_subtotal( $value ) {
$this->totals['subtotal'] = wc_format_decimal( $value, wc_get_price_decimals() );
$this->totals['subtotal'] = $value;
Copy link
Contributor

@jonathansadowski jonathansadowski Apr 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would happen if somebody was relying on the wc_format_decimal behavior up until now?

Copy link
Contributor Author

@vedanshujain vedanshujain May 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch. We did a similar change sometime back, and while no issues have been reported so far because of it, there could have been.

For now, I have restored the formatting changes, but have removed rounding, so it's backward compatible.

vedanshujain added 16 commits May 7, 2021 14:30
@vedanshujain
Remove rounding from individual item discount taxes.
43822b7 
Rounding here conflicts with round at subtotal settings becase we would round at line item level irrespective of settings.
@vedanshujain
Remove rounding from subtotals as they would already be rounded depen…
973bba3 
…ding upon settings.
@vedanshujain
Remove rounding from lines which are not final totals.
b156e32
@vedanshujain
Modify unit test to accomodate pre-rounding total.
a0fbff3
@vedanshujain
Add tests to make sure discounts are rounded properly in cart.
2930057
@vedanshujain
Add test for when entered price has more precision than displayed price.
c834dfb
@vedanshujain
Add test for when product prices has more precision than is displayed.
162588c
@vedanshujain
Add expected deprecated call so that we can call setUp for this testc…
f5056d3 
…ase.
@vedanshujain
Use constant instead of magic numbers.
30cbf46
@vedanshujain
Add expected deprecate calls to make tests work with parent::setUp.
49ed94a
@vedanshujain
Round shipping and fees tax seperately since they are entered excl ta…
a0d6cea 
…xes.

We round up or round down depending upon if prices are entered inclusive or exclusive taxes, however since shipping and fees are always entered exclusive of taxes, they should always be rounded down.
@vedanshujain
Round tax amount appropriately before displaying.
833193b 
We have special method to round taxes which may round up or round down depending upon settings. This method should be used instead of default rounding in formatting funtions.
@vedanshujain
Add clear cart call to prevent side effects from tests.
6c2f301
@vedanshujain
Add code comments for confusing changes.
18411d2
@vedanshujain
Restore formatting but without rounding for backward compat.
b487e36
@vedanshujain
Use edit context to get unformatted price.
f53e9dd
@vedanshujain
Copy link
Contributor Author

vedanshujain commented May 7, 2021

@jonathansadowski sorry for the delay here, this is ready for another round. I have restored the compatibility changes but without rounding in b487e36.

jonathansadowski
jonathansadowski approved these changes May 12, 2021
View reviewed changes
Copy link
Contributor

@jonathansadowski jonathansadowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating this. Tests well for me. Just noting that the inclusive/exclusive setting appears to be backward in the test-plan as it's written, but I figured it out from looking at the issues you linked to. Nice work!

@jonathansadowski jonathansadowski merged commit 692ddaf into trunk May 12, 2021
@jonathansadowski jonathansadowski deleted the fix/rounding branch May 12, 2021 22:43
@woocommercebot woocommercebot added release: add changelog Mark all PRs that have not had their changelog entries added. [auto] release: add testing instructions PRs that have not had testing instructions added to the wiki. [auto] labels May 12, 2021
@github-actions github-actions bot added this to the 5.4.0 milestone May 12, 2021
@ObliviousHarmony ObliviousHarmony added changelog added and removed release: add changelog Mark all PRs that have not had their changelog entries added. [auto] labels May 14, 2021
@juliaamosova juliaamosova added testing instructions added and removed release: add testing instructions PRs that have not had testing instructions added to the wiki. [auto] labels May 16, 2021
@vedanshujain vedanshujain mentioned this pull request May 17, 2021
Closed
@juliaamosova juliaamosova mentioned this pull request May 28, 2021
Closed
3 tasks
@lsinger lsinger mentioned this pull request Aug 8, 2022
Closed
3 tasks
@JanMaranti JanMaranti mentioned this pull request Aug 23, 2022
Closed
5 tasks
@harrowmykel harrowmykel mentioned this pull request Jul 5, 2025
Merged
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@jonathansadowski jonathansadowski jonathansadowski approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

5.4.0

Development

Successfully merging this pull request may close these issues.

Incorrect rounding when product price is entered more than 2dp

6 participants

@vedanshujain @jonathansadowski @ObliviousHarmony @juliaamosova @woocommercebot