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
Looking at this at the moment... Our dealing with money and floating point are generally a mess. :(
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
Working on a unit test now.
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
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
Should not we compare the 2 rounded values (to be on the safe side)?
The value coming from the database is in effect already rounded.. al be it to 6 decimal places instead of 2..
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.
(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 :)
Created attachment 113997 [details] [review] Bug 27079: (QA follow-up) Round both sides for clarity
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>
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>
Created attachment 114001 [details] [review] Bug 27079: (QA follow-up) Round both sides for clarity Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
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>
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>
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>
test plan ok test code looks ok code ok QA script ok
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>
Pushed to master for 20.11, thanks to everybody involved!
Pushed to 20.05.x for 20.05.07
Backported to 19.11.x branch for 19.11.13