Bug 36049

Summary: Rounding prices sometimes leads to incorrect results
Product: Koha Reporter: Raphael Straub <raphael.straub>
Component: AcquisitionsAssignee: Raphael Straub <raphael.straub>
Status: Pushed to oldoldstable --- QA Contact: Victor Grousset/tuxayo <victor>
Severity: normal    
Priority: P3 CC: clemens.tubach, david, dcook, fridolin.somers, jonathan.druart, kebliss, lucas, matt.blenkinsop, michaela.sieber, mtj, thibaud.guillot, tomascohen, victor
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35114
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37938
Change sponsored?: Sponsored Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
This fixes the values and totals shown for orders when rounding prices using the OrderPriceRounding system preference. Example: vendor price for an item is 18.90 and the discount is 5%, the total would show as 17.95 instead of 17.96.
Version(s) released in:
24.11.00,24.05.06,23.11.11
Circulation function:
Bug Depends on:    
Bug Blocks: 37937    
Attachments: Bug 36049: Fix price rounding and add a unit test
Bug 36049: Fix price rounding and add a unit test
Bug 36049: Add unit test
Bug 36049: Fix price rounding
Bug 36049: Fix price rounding
Bug 36049: Add unit test
Bug 36049: Fix price rounding

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 :)
Comment 12 Katrin Fischer 2024-09-02 15:48:08 UTC
(In reply to Victor Grousset/tuxayo from comment #11)
> In basket view, shouldn't "Budgeted cost tax exc." also honor
> OrderPriceRounding ?
> Same about "Budgeted cost tax inc" and "GST".

If it doesn't honor it right now, then this seems a bit out of scope here and would be better suited for a separate bug for discussion/decision.

> 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.

I believe that this calculation is done with JavaScript only and might require a different kind of fix. It might be also not following the system preference?

> 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".

Can you explain this one a bit differently?
Comment 13 Victor Grousset/tuxayo 2024-09-17 01:03:07 UTC
(In reply to Katrin Fischer from comment #12)
> (In reply to Victor Grousset/tuxayo from comment #11)
> > In basket view, shouldn't "Budgeted cost tax exc." also honor
> > OrderPriceRounding ?
> > Same about "Budgeted cost tax inc" and "GST".
> 
> If it doesn't honor it right now, then this seems a bit out of scope here
> and would be better suited for a separate bug for discussion/decision.

Ok. It introduces an inconsistency (17.95 & 17.96 in the same page) but now that it's known and deemed fine for this bug. Not blocking then.


> > 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.
> 
> I believe that this calculation is done with JavaScript only and might
> require a different kind of fix. It might be also not following the system
> preference?

Ok, same as above, not a issue for here then :)
 
> > 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".
> 
> Can you explain this one a bit differently?

That was a sum up of the two previous points. But failed since it actually doesn't help and adds confusion ^^"
Comment 14 Victor Grousset/tuxayo 2024-09-17 01:03:38 UTC
Created attachment 171598 [details] [review]
Bug 36049: Add unit test

Sponsored-by: Karlsruhe Institute of Technology (KIT)
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 15 Victor Grousset/tuxayo 2024-09-17 01:03:41 UTC
Created attachment 171599 [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>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 16 Victor Grousset/tuxayo 2024-09-17 01:05:48 UTC
Works, makes sense, QA script happy, code looks good, passing QA :)
Comment 17 Katrin Fischer 2024-09-17 07:14:11 UTC
I'll have another look before pushing this as you are in principle right about adding inconsistencies. Maybe Raphael can weigh in here as he might have a better idea having looked at the code more closely?
Comment 18 Raphael Straub 2024-09-18 07:27:06 UTC
(In reply to Katrin Fischer from comment #17)
> I'll have another look before pushing this as you are in principle right
> about adding inconsistencies. Maybe Raphael can weigh in here as he might
> have a better idea having looked at the code more closely?

To fix the whole problem, we would have to change every financial calculation in Koha (in Perl and JavaScript, the database does it right) to use a decimal datatype. I think it is not feasible to do it all at once. So, we should identify isolated parts of Koha that could be fixed independently without introducing new inconsistencies and fix those parts separately.

The patch in this bug fixes a rounding function that is used in several parts of Koha and would introduce at least one inconsistency in the acquisition module (see bug 37937). I would not push this patch until we think that no new inconsistencies are introduced by it.

I wrote this bug report mainly to point out that there is a problem with financial calculations in Koha and to provide an example of how this problem can be solved.
Comment 19 Martin Renvoize (ashimema) 2024-09-30 14:33:57 UTC
Thanks for all the hard work!

Pushed to main for the next 24.11.00 release as RM Assistant
Comment 20 Lucas Gass (lukeg) 2024-11-18 22:19:41 UTC
Backported to 24.05.x for upcoming 24.05.06
Comment 21 Fridolin Somers 2024-12-06 11:28:31 UTC
Math::BigFloat is not a new dependancy ?
Comment 22 Raphael Straub 2024-12-10 11:01:27 UTC
(In reply to Fridolin Somers from comment #21)
> Math::BigFloat is not a new dependancy ?
I think it is a new dependency as it is not yet in Koha's cpanfile. Math::BigFloat is installed because it seems to be a dependency of some other Perl module that Koha depends on. On Debian/Ubuntu Math::BigFloat is part of the perl-modules-* package.

Nevertheless, to ensure that Math::BigFloat is always installed with Koha it should be declared as a direct dependency. Is it sufficient to add it to cpanfile?
Comment 23 David Cook 2024-12-10 22:44:09 UTC
(In reply to Raphael Straub from comment #22)
> (In reply to Fridolin Somers from comment #21)
> > Math::BigFloat is not a new dependancy ?
> I think it is a new dependency as it is not yet in Koha's cpanfile.
> Math::BigFloat is installed because it seems to be a dependency of some
> other Perl module that Koha depends on. On Debian/Ubuntu Math::BigFloat is
> part of the perl-modules-* package.
> 
> Nevertheless, to ensure that Math::BigFloat is always installed with Koha it
> should be declared as a direct dependency. Is it sufficient to add it to
> cpanfile?

It looks like Math::BigFloat is and has for a very long time been a part of the Perl core: https://perldoc.perl.org/Math::BigFloat

In theory, I'm not opposed to listing it as a dependency.

I think that debian/list-deps should auto-detect that it's part of perl-modules-5* and not add it to the debian/control.
Comment 24 Fridolin Somers 2024-12-11 13:21:20 UTC
Pushed to 23.11.x for 23.11.11
Comment 25 Victor Grousset/tuxayo 2024-12-19 10:26:06 UTC
(In reply to David Cook from comment #23)
> It looks like Math::BigFloat is and has for a very long time been a part of
> the Perl core: https://perldoc.perl.org/Math::BigFloat

Looks like it's at least 34 years old so we are good 🦕

https://github.com/Perl/perl5/blob/perl-3.044/lib/bigfloat.pl