Description
Nick Clemens (kidclamp)
2020-11-23 15:14:31 UTC
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 |