Bug 36049 - Rounding prices sometimes leads to incorrect results
Summary: Rounding prices sometimes leads to incorrect results
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: Main
Hardware: All All
: P3 normal (vote)
Assignee: Raphael Straub
QA Contact: Victor Grousset/tuxayo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-02-08 13:44 UTC by Raphael Straub
Modified: 2024-04-10 12:41 UTC (History)
9 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 36049: Fix price rounding and add a unit test (2.32 KB, patch)
2024-02-08 14:32 UTC, Raphael Straub
Details | Diff | Splinter Review
Bug 36049: Fix price rounding and add a unit test (2.37 KB, patch)
2024-02-08 18:56 UTC, David Nind
Details | Diff | Splinter Review
Bug 36049: Add unit test (1.00 KB, patch)
2024-03-03 02:05 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 36049: Fix price rounding (1.70 KB, patch)
2024-03-03 02:05 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 36049: Fix price rounding (1.73 KB, patch)
2024-03-03 02:07 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Raphael Straub 2024-02-08 13:44:41 UTC
Example:
Create an order line in acquisitions with vendor price 18.90 and a discount of 5 % results in a total (tax excl.) of 17.95. The correct value in the database (ecost_tax_excluded) is 17.955000 which should be rounded to 17.96.

The problem is that all financial calculations in Perl and JavaScript are done with binary floating-point arithmetic instead of decimal arithmetic. For a short introduction to this topic see https://floating-point-gui.de.

The solution is to use Math::BigFloat in Perl and bignumber.js (https://github.com/MikeMcl/bignumber.js) or big.js (https://github.com/MikeMcl/big.js) in JavaScript for every financial calculation.
Comment 1 Raphael Straub 2024-02-08 14:32:28 UTC
Created attachment 161918 [details] [review]
Bug 36049: Fix price rounding and add a unit test

This patch only fixes price rounding by using Math::BigFloat.

To test:
1) Create an order line in acquisitions with vendor price 18.90 and a
   discount of 5 %.
2) Verify that the total (tax excl.) in the order table on the basket
   page is 17.95.
3) Apply the patch.
4) Reload the basket page.
5) Verify that the total (tax excl.) in the order table on the basket
   page now is 17.96, which is correct.
6) Run: prove t/Number/Price.t

Sponsored-by: Karlsruhe Institute of Technology (KIT)
Comment 2 David Nind 2024-02-08 18:02:21 UTC
Hi Raphael.

I had a go at testing, however the price doesn't change for me - it stays as 17.95.

David
Comment 3 David Nind 2024-02-08 18:43:30 UTC
(In reply to David Nind from comment #2)
> Hi Raphael.
> 
> I had a go at testing, however the price doesn't change for me - it stays as
> 17.95.
> 
> David

Replying to myself - it worked when I set OrderPriceRounding to round.

Also wondering whether this is the same/similar to bug 35114
Comment 4 David Nind 2024-02-08 18:56:28 UTC
Created attachment 161951 [details] [review]
Bug 36049: Fix price rounding and add a unit test

This patch only fixes price rounding by using Math::BigFloat.

To test:
1) Create an order line in acquisitions with vendor price 18.90 and a
   discount of 5 %.
2) Verify that the total (tax excl.) in the order table on the basket
   page is 17.95.
3) Apply the patch.
4) Reload the basket page.
5) Verify that the total (tax excl.) in the order table on the basket
   page now is 17.96, which is correct.
6) Run: prove t/Number/Price.t

Sponsored-by: Karlsruhe Institute of Technology (KIT)
Signed-off-by: David Nind <david@davidnind.com>
Comment 5 Michaela Sieber 2024-02-09 08:37:37 UTC
(In reply to David Nind from comment #3)
> (In reply to David Nind from comment #2)
> > Hi Raphael.
> > 
> > I had a go at testing, however the price doesn't change for me - it stays as
> > 17.95.
> > 
> > David
> 
> Replying to myself - it worked when I set OrderPriceRounding to round.
> 
> Also wondering whether this is the same/similar to bug 35114


Hey David, 

sorry, we forgot to mention that syspref OrderPriceRounding should be enabled to "Round" to nearest cent.
Comment 6 Raphael Straub 2024-02-09 09:03:46 UTC
(In reply to David Nind from comment #3)
> Also wondering whether this is the same/similar to bug 35114
The similarity to bug 35114 is that both bugs deal with rounding issues. Thats why I added this bug to "See Also".

I think that the patch in bug 35114 only solves part of the problem by rearranging rounding operations and using a nearest rounding mode instead of truncation in JavaScript. Yes, it improves the situation as there are fewer cases with incorrect results, but cases with incorrect results still remain as the underlying problem is not using decimal arithmetic. For example "Math.round(18.90 * (100 - 5)) / 100" in JavaScript evaluates to "17.95" instead of "17.96". This can only be fixed by using a decimal arithmetic package like bignumber.js or big.js.
Comment 7 David Nind 2024-02-10 03:11:20 UTC
Thanks Raphael! That helps. I'll have another go at testing bug 35114.
Comment 8 Victor Grousset/tuxayo 2024-03-03 02:05:56 UTC
Created attachment 162699 [details] [review]
Bug 36049: Add unit test

Sponsored-by: Karlsruhe Institute of Technology (KIT)
Signed-off-by: David Nind <david@davidnind.com>
Comment 9 Victor Grousset/tuxayo 2024-03-03 02:05:59 UTC Comment hidden (obsolete)
Comment 10 Victor Grousset/tuxayo 2024-03-03 02:07:35 UTC
Created attachment 162701 [details] [review]
Bug 36049: Fix price rounding

This patch only fixes price rounding by using Math::BigFloat.

To test:
0) Enable syspref OrderPriceRounding
1) Create an order line in acquisitions with vendor price 18.90 and a
   discount of 5 %.
2) Verify that the total (tax excl.) in the order table on the basket
   page is 17.95.
3) Apply the patch.
4) Reload the basket page.
5) Verify that the total (tax excl.) in the order table on the basket
   page now is 17.96, which is correct.
6) Run: prove t/Number/Price.t

Sponsored-by: Karlsruhe Institute of Technology (KIT)
Signed-off-by: David Nind <david@davidnind.com>
Comment 11 Victor Grousset/tuxayo 2024-03-03 02:36:53 UTC
In basket view, shouldn't "Budgeted cost tax exc." also honor OrderPriceRounding ?
Same about "Budgeted cost tax inc" and "GST".

In the new order form, "Total" (and "Budgeted cost:" depending on previous question) now aren't consistent with the basket view. It's the previous step and the same data should have the same value.

So we would have basket's "total tax exc" honoring well OrderPriceRounding but there is still buggy rounding around. Either for the same data on the previous page or on the same page close to "total tax exc".

--- 

Commit was split to confirm that the test fails without implementation. That shows the test does it's work :)