Bug 17140 - Incorrect rounding in total fines calculations, part 2
Summary: Incorrect rounding in total fines calculations, part 2
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Blou
QA Contact: Josef Moravec
URL:
Keywords:
: 18481 18487 (view as bug list)
Depends on: 15741
Blocks:
  Show dependency treegraph
 
Reported: 2016-08-17 15:36 UTC by Lari Taskula
Modified: 2019-07-10 05:49 UTC (History)
20 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 17140: Test to cover more rounding issues in fines calculations (1.67 KB, patch)
2016-08-17 15:39 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17140: Fix rounding in total fines calculations, part 2 (1.24 KB, patch)
2016-08-18 12:28 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17140: Fix rounding in total fines calculations, part 2 (1.24 KB, patch)
2016-08-18 12:29 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17140: Fix rounding in total fines calculations, part 2 (1.34 KB, patch)
2016-08-19 10:37 UTC, Lari Taskula
Details | Diff | Splinter Review
Illustration of precision problem with calculated rental fees and manual invoices (81.73 KB, image/png)
2016-08-19 15:23 UTC, Marc Véron
Details
Screenshots with some notes added (99.46 KB, image/png)
2016-08-19 15:27 UTC, Marc Véron
Details
Demonstration of List::Util->sum0 vs MySQL sum + 0 (404 bytes, application/x-perl)
2018-11-16 01:02 UTC, Nick Clemens
Details
Bug 17140: fix rounding errors when paying fines (2.00 KB, patch)
2018-12-21 15:41 UTC, Blou
Details | Diff | Splinter Review
Bug 17140: fix rounding errors when paying fines (2.07 KB, patch)
2019-03-04 21:48 UTC, Hayley Mapley
Details | Diff | Splinter Review
Bug 17140: fix rounding errors when paying fines (2.12 KB, patch)
2019-03-05 10:54 UTC, Josef Moravec
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lari Taskula 2016-08-17 15:36:12 UTC
Bug 15741 fixes some rounding issues but not all, for example trying to paying a fine of 1.14 still does not work (also 1.39).

To replicate bug:
1. Go to patron's fines tab, make sure there are no outstanding payments.
2. Via "Create manual invoice" tab, create one fine of 1.14
3. Go to Pay fines tab
4. Click "Pay amount" and confirm.
5. Observe error message "You must pay a value less than or equal to 1.14."
6. Go to Pay fines tab
7. Click "Pay selected" and confirm
8. Observe error message "You must pay a value less than or equal to 1.14."
Comment 1 Lari Taskula 2016-08-17 15:39:27 UTC
Created attachment 54542 [details] [review]
Bug 17140: Test to cover more rounding issues in fines calculations

To test:
1. Run t/db_dependent/Members.t
Comment 2 Lari Taskula 2016-08-18 12:28:08 UTC Comment hidden (obsolete)
Comment 3 Lari Taskula 2016-08-18 12:29:28 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2016-08-18 13:48:49 UTC
I think we should remove the * 1000 / 1000
Comment 5 Lari Taskula 2016-08-19 10:37:41 UTC
Created attachment 54611 [details] [review]
Bug 17140: Fix rounding in total fines calculations, part 2

This patch fixes still occuring rounding issue with total fines calculations.

To test:
1. Go to patron's fines tab, make sure there are no outstanding payments.
2. Via "Create manual invoice" tab, create one fine of 1.14
3. Go to Pay fines tab
4. Click "Pay amount" and confirm.
5. Observe error message "You must pay a value less than or equal to 1.14."
6. Run t/db_dependent/Members.t
7. Observe failure in a test case
8. Apply patch.
9. Repeat step 3 and 4
10. Observe that payment goes through
11. Run t/db_dependent/Members.t
12. Observe test passing
Comment 6 Lari Taskula 2016-08-19 10:45:16 UTC
Is the precision of 2 decimal places enough? I saw it is used in boraccount.pl, pay.pl and moremember.pl and maybe this discussion has been had before? I think there can still be weird cases if you add a fine of 5.005 or anything that has more than 2 decimal places.
Comment 7 Marc Véron 2016-08-19 13:40:55 UTC
In table accountlines, definition of amount is decimal 28,6
http://schema.koha-community.org/tables/accountlines.html

I am able to enter a manual invoice like 0.008 (displays as 0.01) or 0.004 (displays as 0.00)

The table then contains:
mysql> select amount from accountlines where borrowernumber = 13;
+----------+
| amount   |
+----------+
| 0.008000 |
| 0.004000 |
+----------+

The 'Pay fines' tab shows an amount of 0.01 and an amount of 0.00

I payed the first one (0.01), it asked 0.01

mysql> select amount from accountlines where borrowernumber = 13;
+-----------+
| amount    |
+-----------+
|  0.008000 |
|  0.004000 |
| -0.010000 |
+-----------+

The 'Pay fines tab then shows an amount of 0.00
If I want to pay using 'Pay' on the line, it asks for 0.00, but obviously it is not possible to pay the amount.

Same with 'Pay amount'

Now I add manual invoices, let's say 3 times 0.003

mysql> select amount from accountlines where borrowernumber = 13;
+-----------+
| amount    |
+-----------+
|  0.008000 |
|  0.004000 |
| -0.010000 |
|  0.003000 |
|  0.003000 |
|  0.003000 |
+-----------+

The Account tab shows all 0.00 in 'Outstanding' column, but a 'Total due' of 0.01

A similar situation occurs with Rental discounts in Circulation and fines rules (smart-rules.pl), e.g. values like 2.961000

That will lead to confusion... 

What to do? Maybe the following:

 - Define a systemwide decimal precision for data entry of amounts?
 - Maybe depending on the decimal places in syspref CurrencyFormat?
 - Make Rental discounts round to that precision?
 - Allow 'Price' plug-in to override if more precision is necessary, e.g. for calculating / displaying taxes in acquisition? (Koha/Number/Price.pm)
Comment 8 Jonathan Druart 2016-08-19 13:57:53 UTC
Prices should be stored with a big precision (6 is largely enough) and displayed with 2 decimals.
That is how it's done in the acq module and almost everywhere else I think.
Comment 9 Jacek Ablewicz 2016-08-19 15:01:55 UTC
(In reply to Jonathan Druart from comment #8)
> Prices should be stored with a big precision (6 is largely enough) and
> displayed with 2 decimals.

Hm, maybe charges / fines should be treated a bit differently.. When there is a fine like 1.342, and it displays as 1.32, how do you know what exact amount to enter to make account balanced? Doing a math (comparisons etc.) with amounts casted to .2f would solve some kinds of this problems (I think that's like accountants do it [on Discovery Channel]) to solve < .01 difference problems on invoices etc.

Fine calculations in Koha a mostly done in floating-point - it's probably good enough for 99.9(9) of cases (~16 significant decimal digits precision with 64bit floats) as long as you are doing a casting (btw, to .2f or .6f ?) in most important places.

Mysql calculations for decimal(x,y) are allegedly precise, though, and fine calculations in koha are not always floating-point, e.g. in GetMemberIssuesAndFines():

    SELECT SUM(amountoutstanding) FROM accountlines

Storing all fines/charges amounts rounded to .2f and doing most of the math in .2 precision is tempting (but that alone would not be enough to prevent all kinds of such problems) - hopefully nobody is using bitcoin as a main currency in Koha..
Comment 10 Jacek Ablewicz 2016-08-19 15:09:21 UTC
(In reply to Jonathan Druart from comment #4)
> I think we should remove the * 1000 / 1000

+1, it isn't really serving any useful purpose, it's not warranted that the result of the multiplication will be an integer anyway.

BTW, there is another one like that (*100, /100) in GetBorNotifyAcctRecord(). Still with an int() thing, which is definitely incorrect.
Comment 11 Marc Véron 2016-08-19 15:23:00 UTC
Created attachment 54642 [details]
Illustration of precision problem with calculated rental fees and manual invoices

I made some screenshots to explain what I mean.
(Screenshots made with patch applied).

I still think that
- it should not be possible to manually enter values like 1.003 (manual invoices, payment, credit, item rental fees, others?)
- a calculated rental fee (from item rental fee and patron category discount) should be rounded to 2 decimals

Otherwise, after payement, you will have remainders in the database that lead to strange behaviour (see attachment).
Comment 12 Marc Véron 2016-08-19 15:27:29 UTC
Created attachment 54643 [details]
Screenshots with some notes added

Added the same screenshots with some notes on them.

See comment #11
Comment 13 Alex Buckley 2016-12-01 23:09:52 UTC
Running step 4 after applying the patch still throws the error ' You must pay a value less than or equal to 1.14.'. i.e. the payment does not go through after this patch is applied.
Comment 14 Nick Clemens 2017-04-24 13:37:30 UTC
*** Bug 18481 has been marked as a duplicate of this bug. ***
Comment 15 Nick Clemens 2017-04-24 13:38:31 UTC
*** Bug 18487 has been marked as a duplicate of this bug. ***
Comment 16 Brian 2018-11-01 19:52:06 UTC
Has there been any more updates on bug 17140?

Carnegie Stout Team.
Comment 17 Martha Fuerst 2018-11-02 16:40:15 UTC
This is still a problem in 18.05. We have a workaroud (pay the fine in chunks), but if you pick the wrong chunks, it can still be an issue.

Would love a fix.

Thanks!

-Marti Fuerst, Huntsville Madison-County Library System
Comment 18 Barton Chittenden 2018-11-05 19:22:24 UTC
I'm changing the urgency to 'Major' -- this deals with fines, and is an issue that circulation staff will be hitting frequently. ByWater has multiple independent reports of this.
Comment 19 Katrin Fischer 2018-11-05 20:37:20 UTC
I am not sure if Lari is still actively developing on Koha?
Comment 20 Katrin Fischer 2018-11-12 16:59:57 UTC
I confirmed with Lari that he is no longer doing Koha development - I am changing the assignee to default so someone else might pick it up.
Comment 21 Nick Clemens 2018-11-16 01:02:42 UTC
Created attachment 82375 [details]
Demonstration of List::Util->sum0 vs MySQL sum + 0

Run this file to compare the total when summing a list versus getting the sum from the DB and adding 0
Comment 22 Nick Clemens 2018-11-16 01:05:28 UTC
I think this is resolved somewhat by bug 21673 - it changes the method for summing which seems to resolve the issue.

The separate issue of 'partial cent' fines still exists, we should probably display all digits to 6 decimals and allow partial cent payments, but I think 21673 should be considered for backport to resolve rounding issues
Comment 23 Katrin Fischer 2018-11-16 08:09:39 UTC
(In reply to Nick Clemens from comment #22)
> I think this is resolved somewhat by bug 21673 - it changes the method for
> summing which seems to resolve the issue.
> 
> The separate issue of 'partial cent' fines still exists, we should probably
> display all digits to 6 decimals and allow partial cent payments, but I
> think 21673 should be considered for backport to resolve rounding issues

I don't like the idea of displaying 6 decimals really...
Comment 24 Blou 2018-12-20 20:27:06 UTC
6 decimal is ugly.  We should never show technolgies' failings to the user.

Just to confirm that Bug 21673 does not fix this issue.  At least my client's version of it, which is trying to pay a 1.035 fine displayed as 1.04.

I'll assign myself the issue and fix it tomorrow.  IMO, we should solve this with common sense and not an accounting mentality.

For 1.035, we display 1.04, as it should.
Then on payment, we validate if there's a difference of less than 0.01.  If so then
AmountPaid = AmountNeededToBePaid.
Just before the comparison. 

Anyway, accountants HATE those remaining cents at the end of the year.
Comment 25 Blou 2018-12-21 15:41:26 UTC
Created attachment 83467 [details] [review]
Bug 17140: fix rounding errors when paying fines

Whenever a fine (accountlines in DB) has a precision longer than two floating points, it becomes very hard for a user to clear it.
Ex: 1.035 will be displayed as 1.04 or 1.03 depending on the screen.  But entering any of those value in Pay Fine will not clear it.
The user has no way to know the exact value to enter.

This fix makes sure that the intent of the user is met, by matching the EXACT needed sum when the difference is less than 0.01.

TEST
1) Create a 1.035 fine
  - Go to a Patron screen
  - Fines tab on the left
  - Create manual invoice
2) go to Pay fines, click Pay amount on bottom left.
3) It will show 1.03 as Total, and as Collect from patron.  Click confirm.
4) An error message will appear.
5) Apply patch, do again.
Comment 26 Blou 2018-12-21 15:47:19 UTC
This patch does not fix the incongruities of having the 1.035 fine display as 1.04 in some screen and 1.03 in others.

But this simple patch (independant from the others offered, careful when applying), does fix the impossible payment situation in a very elegant way.  I think. :-)

This is the most important.  Trying to fix all the displays for quarter cents, or all input methods if you manage, would still leave clients just migrating with funky data in the situation of not being able to clear the fine.
Comment 27 Hayley Mapley 2019-03-04 01:52:18 UTC
Patch 54542 did not apply cleanly. Marking as Patch doesn't apply.
Comment 28 Blou 2019-03-04 12:52:36 UTC
Hi Hayley,
Have you tried applying only 83467 (the last one), which is separated from the others (see my previous comments)?  Or maybe its solution is not what you're looking for...
Comment 29 Hayley Mapley 2019-03-04 20:13:25 UTC
(In reply to Blou from comment #28)
> Hi Hayley,
> Have you tried applying only 83467 (the last one), which is separated from
> the others (see my previous comments)?  Or maybe its solution is not what
> you're looking for...

Hi Blou,

My bad! I missed your comment. Will try to sign off today.
Comment 30 Hayley Mapley 2019-03-04 21:48:36 UTC
Created attachment 86044 [details] [review]
Bug 17140: fix rounding errors when paying fines

Whenever a fine (accountlines in DB) has a precision longer than two floating points, it becomes very hard for a user to clear it.
Ex: 1.035 will be displayed as 1.04 or 1.03 depending on the screen.  But entering any of those value in Pay Fine will not clear it.
The user has no way to know the exact value to enter.

This fix makes sure that the intent of the user is met, by matching the EXACT needed sum when the difference is less than 0.01.

TEST
1) Create a 1.035 fine
  - Go to a Patron screen
  - Fines tab on the left
  - Create manual invoice
2) go to Pay fines, click Pay amount on bottom left.
3) It will show 1.03 as Total, and as Collect from patron.  Click confirm.
4) An error message will appear.
5) Apply patch, do again.

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>
Comment 31 Josef Moravec 2019-03-05 10:47:34 UTC
I obsoleted first two patches, they do not apply anymore, the updated part of code was refactored and the original issue from comment 1 is fixed.
Comment 32 Josef Moravec 2019-03-05 10:54:57 UTC
Created attachment 86051 [details] [review]
Bug 17140: fix rounding errors when paying fines

Whenever a fine (accountlines in DB) has a precision longer than two floating points, it becomes very hard for a user to clear it.
Ex: 1.035 will be displayed as 1.04 or 1.03 depending on the screen.  But entering any of those value in Pay Fine will not clear it.
The user has no way to know the exact value to enter.

This fix makes sure that the intent of the user is met, by matching the EXACT needed sum when the difference is less than 0.01.

TEST
1) Create a 1.035 fine
  - Go to a Patron screen
  - Fines tab on the left
  - Create manual invoice
2) go to Pay fines, click Pay amount on bottom left.
3) It will show 1.03 as Total, and as Collect from patron.  Click confirm.
4) An error message will appear.
5) Apply patch, do again.

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 33 Josef Moravec 2019-03-05 10:56:56 UTC
I am passing it QA, as it is at least quick fix of long standing issue. It definitely could be done better, but it would need more refactoring and maybe some architectural changes I think.
Comment 34 Jonathan Druart 2019-03-05 13:02:13 UTC
I am not against a quick fix, but things are weird (with or without the patch):

Add a 1.035 fine, you see:
https://snag.gy/hmFX51.jpg

Go to make a payment, you see:
https://snag.gy/0MuCxY.jpg

Click "Pay", you see:
https://snag.gy/bA5O3C.jpg

If we push this as it, we should ask have people to focus on a global solution.
Comment 35 Josef Moravec 2019-03-05 16:08:53 UTC
(In reply to Jonathan Druart from comment #34)
> I am not against a quick fix, but things are weird (with or without the
> patch):
> 
> Add a 1.035 fine, you see:
> https://snag.gy/hmFX51.jpg
> 
> Go to make a payment, you see:
> https://snag.gy/0MuCxY.jpg
> 
> Click "Pay", you see:
> https://snag.gy/bA5O3C.jpg
> 
> If we push this as it, we should ask have people to focus on a global
> solution.

Yes, it is definitely bad, and I thought, that we should push this and work on better general solutions, but thinking about this again: we should not push this, because it silently pay different amount then expected.

The quick fix should be just show warning there is something wrong with some advice probably (like, writeoff an debit when it looks like zero, maybe...) 

Or, if we are able to fix it right until 19.05, then we should do it...
Comment 36 Blou 2019-03-05 16:33:49 UTC
To quote "if we are able to fix it right until 19.05, then we should do it..."

This was opened in 2016, so good luck with that.  It IS a complicated issue to fix right.

The patch allows librarian to move ahead when this occurs whereas a popup will just tell them they are not crazy, but leave them helpless again.

In our production environments, it has occured a few times (to much frustration and bewilderment, and never a complaint since we put the patch on, even though the discremencies in the UI a quite obvious).
Comment 37 Josef Moravec 2019-03-05 17:07:32 UTC
(In reply to Blou from comment #36)
> To quote "if we are able to fix it right until 19.05, then we should do
> it..."
> 
> This was opened in 2016, so good luck with that.  It IS a complicated issue
> to fix right.
> 
> The patch allows librarian to move ahead when this occurs whereas a popup
> will just tell them they are not crazy, but leave them helpless again.
> 
> In our production environments, it has occured a few times (to much
> frustration and bewilderment, and never a complaint since we put the patch
> on, even though the discremencies in the UI a quite obvious).

I understand you....

But your patch could lead to bugs in fine reports, couldn't it?
Comment 38 Blou 2019-03-05 18:21:48 UTC
Interesting.  Showing the extent of what to consider for a complete solution.
That said, I wouldn't call it a "bug".  The report would still work, but have a 0.005 discrepancy somewhere, right?  To which I would oppose a 9$ discrepency because the staff could never enter the payment made by the borrower in the other case.

I'm not particularly attached to this solution, I argue for the sake of the staff users who have encountered the problem, and I believe this is the lesser of two evils (the other one being the status quo).  If Evil is against your policy, that's ok too... :-)