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 normal (vote)
Assignee: Lari Taskula
QA Contact: Testopia
URL:
Whiteboard:
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: 2017-06-20 12:18 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


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

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