To recreate: 1 - Set maxFine to 7.50 2 - Manually create some overdues for a patron in the amounts of: 6.99, .1, .1, .1, .1, .1, .01 (create a manual debits then 'UPDATE accountlines SET debit_type_code = "OVERDUE";' 3 - Set fines for an item type to .1 4 - Checkout an item of that type to patron and backdate so it is overdue 5 - Run fines.pl 6 - Check the DB, they have a new fine of "0" 7 - Checkin the item, checking 'forgive fines' option 8 - ISE "Debit amount passed is not positive"
Created attachment 102826 [details] [review] Bug 25127: Unit tests
I'm confused.. are your libraries literally doing step 2? By skipping UpdateFine for the test data you're ending up with 'fines' without an associated issue.. which shouldn't be possible. If I correct the test to either add the issue_id in or use UpdateFine (which results in the same thing) then I cannot replicate the problem?
Created attachment 103235 [details] [review] Bug 25127: Correction to tests
Aha.. I think that update to the tests clarifies it
Man I hate rounding maths using Floats :( 7.2 (maxfine) - 7.2 (total_amount_other) = 8.88178419700125e-16 ( [U+1F622] )
Created attachment 103249 [details] [review] Bug 25127: Crude float fix
Not entirely happy with my fix, but it's the best I've come up with to date and does appear to work for all existing test cases.
You usually need to "$integer + 0.0" to compare it with floats. It's what we do in tests.
Long term, we should really switch away from `decimal(28,6)` to `int` or `smallint` at the database level and then set a rounding routine for anywhere we end up with parts of a penny/cent/etc. (Probably we want to stick to 'round to half even' - https://en.wikipedia.org/wiki/Rounding#Round_half_to_even too)
Created attachment 103253 [details] [review] Bug 25127: Compare with 2 decimals
(In reply to Jonathan Druart from comment #10) > Created attachment 103253 [details] [review] [review] > Bug 25127: Compare with 2 decimals This is what I meant. (alternative patch for "Crude float fix")
Using sprintf isn't a bad solution.. it limits us to 2 currencies using only 2 decimal places though.. I'd be interested to know if there are any Koha users out there actually using 3 or more decimals. Also.. using sprintf does the half round by even thing I was talking about in my earlier comment.
(In reply to Martin Renvoize from comment #12) > Using sprintf isn't a bad solution.. it limits us to 2 currencies using only > 2 decimal places though.. I'd be interested to know if there are any Koha > users out there actually using 3 or more decimals. > > Also.. using sprintf does the half round by even thing I was talking about > in my earlier comment. I think your *100/100 trick will only work for 2 decimals as well. Koha cannot work with more than 2 decimals right now.
(In reply to Martin Renvoize from comment #12) > Using sprintf isn't a bad solution.. it limits us to 2 currencies using only > 2 decimal places though.. I'd be interested to know if there are any Koha > users out there actually using 3 or more decimals. I remember looking it up - it appears there are some currencies where more decimals are valid (column E): https://en.wikipedia.org/wiki/ISO_4217 We might have to treat this as configurable.
(In reply to Katrin Fischer from comment #14) > We might have to treat this as configurable. We are supporting 2 decimals, it's hardcoded all over our codebase I think.
Created attachment 103276 [details] [review] Bug 25127: Use Koha::Number::Price->round for comparison
Final alternative.. use the existing Koha::Number::Price->round method for the comparison
Yes, that's definitely the way to go!
Hi, what is the test plan? patch 3 and 4 are exclusive, seems to me that 4 is preferred. On top of 25123 and patches 1, 2 and 4, the test pass. Is anything more to test?
I've now obsoleted patch three as discussed. Test plan wise, the Unit tests effectively emulate the plan in Nicks first comment. It's never a bad idea to give it a go manually as a double check that the unit tests are doing what they're supposed to.
Created attachment 103325 [details] [review] Bug 25127: Unit tests
Created attachment 103326 [details] [review] Bug 25127: Correction to tests
Created attachment 103327 [details] [review] Bug 25127: Use Koha::Number::Price->round for comparison
Created attachment 103430 [details] [review] Bug 25127: Unit tests Signed-off-by: David Nind <david@davidnind.com>
Created attachment 103431 [details] [review] Bug 25127: Correction to tests Signed-off-by: David Nind <david@davidnind.com>
Created attachment 103432 [details] [review] Bug 25127: Use Koha::Number::Price->round for comparison Signed-off-by: David Nind <david@davidnind.com>
(In reply to Martin Renvoize from comment #20) > I've now obsoleted patch three as discussed. > > Test plan wise, the Unit tests effectively emulate the plan in Nicks first > comment. It's never a bad idea to give it a go manually as a double check > that the unit tests are doing what they're supposed to. I applied the patch and ran prove t/db_dependent/Overdues.t Hope that is sufficient for testing!
Created attachment 104078 [details] [review] Bug 25127: Unit tests Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 104079 [details] [review] Bug 25127: Correction to tests Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 104080 [details] [review] Bug 25127: Use Koha::Number::Price->round for comparison Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Nice work everyone! Pushed to master for 20.05
backported to 19.11.x for 19.11.06
missing dependencies for 19.05.x, no backport