Bug 27079

Summary: UpdateFine adds refunds for fines paid off before return
Product: Koha Reporter: Nick Clemens <nick>
Component: Fines and feesAssignee: Martin Renvoize <martin.renvoize>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: andrewfh, jonathan.druart, josef.moravec, kyle, martin.renvoize, tomascohen, victor
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17138
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25127
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.07, 19.11.13
Bug Depends on:    
Bug Blocks: 17138    
Attachments: Bug 27079: Floating point comparison correction
Bug 27079: Unit tests
Bug 27079: Floating point comparison correction
Bug 27079: (QA follow-up) Round both sides for clarity
Bug 27079: Unit tests
Bug 27079: Floating point comparison correction
Bug 27079: (QA follow-up) Round both sides for clarity
Bug 27079: Unit tests
Bug 27079: Floating point comparison correction
Bug 27079: (QA follow-up) Round both sides for clarity
Bug 27079: Floating point comparison correction

Description Nick Clemens 2020-11-23 15:14:31 UTC
The issue seems to be that the amount passed in to UpdateFine fails the comparison to $accountline->amount - one is $1.80 and the other is $1.80000
and comparison fails:

576     if ( $accountline ) {
577         if ( $accountline->amount != $amount ) {

To recreate:
1 - Set a circ rule to have a .15 per day fine
2 - Issue an item of this type to a borrower
3 - Make it 12 days overdue either at checkout or in the db:
    UPDATE issues SET date_due = DATE_SUB(CURDATE(),INTERVAL 12 DAY);
4 - Run the fines job or just put a fine in place:
    INSERT INTO accountlines (issue_id,borrowernumber,itemnumber,amount,debit_type_code,status,amountoutstanding) SELECT issue_id,5,itemnumber,1.80,'OVERDUE','UNRETURNED',1.80 FROM issues WHERE borrowernumber=5;
5 - Pay the fine for the patron
6 - Check in the item
7 - Note that the patron has an 'Overpayment refund' of $0.00
Comment 1 Martin Renvoize 2020-11-25 08:06:21 UTC
Looking at this at the moment... Our dealing with money and floating point are generally a mess. :(
Comment 2 Martin Renvoize 2020-11-25 13:03:49 UTC
Created attachment 113981 [details] [review]
Bug 27079: Floating point comparison correction

We cannot compare calculated values directly as those calculations may
have introduced floating point differences.  We have the
Koha::Number::Price->round function to consistently round the floating
point number to a decimal with 2 points precission for comparison.

Test plan
1/ Set a circ rule to have a .15 per day fine
2/ Issue an item of this type to a borrower (making it 12 days overdue
   by setting the due date to 12 days in the past)
3/ Run the fines job:
4/ Pay the fine for the patron
5/ Check in the item
6/ Note that the patron does not have an 'Overpayment refund' of $0.00
Comment 3 Martin Renvoize 2020-11-25 13:04:25 UTC
Working on a unit test now.
Comment 4 Martin Renvoize 2020-11-25 14:12:27 UTC
Created attachment 113983 [details] [review]
Bug 27079: Unit tests

Add a unit test to check for floating point errors in UpdateFine
relating to catching matching CalcFine amounts compared to DB amount
values.

Test plan
1/ Run the unit test and confirm it fails
2/ Apply the second patch in the series
3/ Run the unit test and confirm it passes
Comment 5 Martin Renvoize 2020-11-25 14:12:30 UTC
Created attachment 113984 [details] [review]
Bug 27079: Floating point comparison correction

We cannot compare calculated values directly as those calculations may
have introduced floating point differences.  We have the
Koha::Number::Price->round function to consistently round the floating
point number to a decimal with 2 points precission for comparison.

Test plan
1/ Set a circ rule to have a .15 per day fine
2/ Issue an item of this type to a borrower (making it 12 days overdue
   by setting the due date to 12 days in the past)
3/ Run the fines job:
4/ Pay the fine for the patron
5/ Check in the item
6/ Note that the patron does not have an 'Overpayment refund' of $0.00
Comment 6 Jonathan Druart 2020-11-25 14:14:16 UTC
Should not we compare the 2 rounded values (to be on the safe side)?
Comment 7 Martin Renvoize 2020-11-25 14:44:11 UTC
The value coming from the database is in effect already rounded.. al be it to 6 decimal places instead of 2..
Comment 8 Martin Renvoize 2020-11-25 14:45:06 UTC
My feeling is.. get this one in as is and work on a much nicer solution to catch cases in a more general way in bug 17138 for the 21.05 cycle.
Comment 9 Jonathan Druart 2020-11-25 14:54:08 UTC
(In reply to Martin Renvoize from comment #7)
> The value coming from the database is in effect already rounded.. al be it
> to 6 decimal places instead of 2..

Yes, that's why I added "to be on the safe side", in case it is modified later (regression proof).

And also for consistency: when you read the line it would read better if you understand directly that you are comparing the same thing :)
Comment 10 Martin Renvoize 2020-11-25 17:17:55 UTC
Created attachment 113997 [details] [review]
Bug 27079: (QA follow-up) Round both sides for clarity
Comment 11 Josef Moravec 2020-11-25 19:39:50 UTC
Created attachment 113999 [details] [review]
Bug 27079: Unit tests

Add a unit test to check for floating point errors in UpdateFine
relating to catching matching CalcFine amounts compared to DB amount
values.

Test plan
1/ Run the unit test and confirm it fails
2/ Apply the second patch in the series
3/ Run the unit test and confirm it passes

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 12 Josef Moravec 2020-11-25 19:39:55 UTC
Created attachment 114000 [details] [review]
Bug 27079: Floating point comparison correction

We cannot compare calculated values directly as those calculations may
have introduced floating point differences.  We have the
Koha::Number::Price->round function to consistently round the floating
point number to a decimal with 2 points precission for comparison.

Test plan
1/ Set a circ rule to have a .15 per day fine
2/ Issue an item of this type to a borrower (making it 12 days overdue
   by setting the due date to 12 days in the past)
3/ Run the fines job:
4/ Pay the fine for the patron
5/ Check in the item
6/ Note that the patron does not have an 'Overpayment refund' of $0.00

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 13 Josef Moravec 2020-11-25 19:39:59 UTC
Created attachment 114001 [details] [review]
Bug 27079: (QA follow-up) Round both sides for clarity

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 14 Victor Grousset/tuxayo 2020-11-26 18:37:56 UTC
Created attachment 114018 [details] [review]
Bug 27079: Unit tests

Add a unit test to check for floating point errors in UpdateFine
relating to catching matching CalcFine amounts compared to DB amount
values.

Test plan
1/ Run the unit test and confirm it fails
2/ Apply the second patch in the series
3/ Run the unit test and confirm it passes

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 15 Victor Grousset/tuxayo 2020-11-26 18:38:00 UTC
Created attachment 114019 [details] [review]
Bug 27079: Floating point comparison correction

We cannot compare calculated values directly as those calculations may
have introduced floating point differences.  We have the
Koha::Number::Price->round function to consistently round the floating
point number to a decimal with 2 points precission for comparison.

Test plan
1/ Set a circ rule to have a .15 per day fine and enable finesMode
syspref.
2/ Issue an item of this type to a borrower (making it 12 days overdue
   by setting the due date to 12 days in the past)
3/ Run the fines job:
4/ Pay the fine for the patron
5/ Check in the item
6/ Note that the patron does not have an 'Overpayment refund' of $0.00

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 16 Victor Grousset/tuxayo 2020-11-26 18:38:03 UTC
Created attachment 114020 [details] [review]
Bug 27079: (QA follow-up) Round both sides for clarity

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 17 Victor Grousset/tuxayo 2020-11-26 18:39:33 UTC
test plan ok
test code looks ok
code ok
QA script ok
Comment 18 Jonathan Druart 2020-11-27 08:38:00 UTC
Created attachment 114029 [details] [review]
Bug 27079: Floating point comparison correction

We cannot compare calculated values directly as those calculations may
have introduced floating point differences.  We have the
Koha::Number::Price->round function to consistently round the floating
point number to a decimal with 2 points precission for comparison.

Test plan
1/ Set a circ rule to have a .15 per day fine and enable finesMode
syspref.
2/ Issue an item of this type to a borrower (making it 12 days overdue
   by setting the due date to 12 days in the past)
3/ Run the fines job:
4/ Pay the fine for the patron
5/ Check in the item
6/ Note that the patron does not have an 'Overpayment refund' of $0.00

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Bug 27079: (QA follow-up) Round both sides for clarity

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 19 Jonathan Druart 2020-11-27 12:03:03 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 20 Andrew Fuerste-Henry 2020-12-14 15:40:14 UTC
Pushed to 20.05.x for 20.05.07
Comment 21 Victor Grousset/tuxayo 2020-12-17 00:25:47 UTC
Backported to 19.11.x branch for 19.11.13