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

I think we should remove the * 1000 / 1000

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

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.

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)

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.

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

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

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

Created attachment 54643 [details]
Screenshots with some notes added

Added the same screenshots with some notes on them.

See comment #11

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.

*** Bug 18481 has been marked as a duplicate of this bug. ***

*** Bug 18487 has been marked as a duplicate of this bug. ***

Has there been any more updates on bug 17140?

Carnegie Stout Team.

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

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.

I am not sure if Lari is still actively developing on Koha?

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.

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

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

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

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.

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.

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.

Patch 54542 did not apply cleanly. Marking as Patch doesn't apply.

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

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

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>

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.

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>

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.

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.

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

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

(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?

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

We had a client who had a problem that I think is related to this one, but I'm not sure. The accidentally entered 0.251 in the circulation rules fines instead of 0.25. When the patron wanted to pay, it was impossible. There's nothing in the interface that suggests there was a third decimal because the display is truncated (or rounded?) to two decimals. But when going into the database, we noticed there were three decimals.

Here is how to reproduce on master :
(0. In Administration > System preferences, make sure your finesMode syspref is set to Calculate and charge)

1. Go to Administration > Circulation and fines rules and add or modify an existing rule to enter 0.251 in the fines.

2. Save your rule

(2.5 *Notice the fine displayed is only 0.25*, and if you try to edit your rule again, it still displays only 0.25 even though in the database, is is saved as 0.251000; if you click edit, don't click save again because it will save it as two decimals, which is fine IRL, but the test won't work)

3. Check out an item to a patron with a due date in the past (using Checkout settings)

4. Run fines.pl

5. Go to the patron's accounting tab *Notice the fines are rounded/truncated to 2 decimals* (but if you look in the database, they have three decimals)

6. Try to pay the fines that have the three decimals (in master the payment is not blocked whereas in 18.05 there is an error message)

7. Notice the fine is still in the accounts to pay but the amount oustanding is 0, if you go in the db, you will see that there is still some money owed.

19.05 came and went.  So will 19.11...

Created attachment 93553 [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>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 93554 [details] [review]
Bug 17140: Make rounding syspref controlled

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 93678 [details] [review]
Bug 17140: Atomic update file

Created attachment 93715 [details] [review]
Works. Thanks all.

-JesseM


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>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com>

Created attachment 93716 [details] [review]
Bug 17140: Make rounding syspref controlled

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com>

Created attachment 93717 [details] [review]
Bug 17140: Atomic update file

Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com>

Nice work!

Pushed to master for 19.11.00

Pushed to 19.05.x for 19.05.05