Bug 25127 - Fines with an amountoutstanding of 0 can be created due to maxFine but cannot be forgiven
Summary: Fines with an amountoutstanding of 0 can be created due to maxFine but cannot...
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Fines and fees (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Martin Renvoize
QA Contact: Testopia
URL:
Keywords:
Depends on: 25123
Blocks: 14825
  Show dependency treegraph
 
Reported: 2020-04-13 12:46 UTC by Nick Clemens
Modified: 2020-05-15 17:27 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.06


Attachments
Bug 25127: Unit tests (2.33 KB, patch)
2020-04-13 12:47 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25127: Correction to tests (3.86 KB, patch)
2020-04-20 08:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25127: Crude float fix (2.98 KB, patch)
2020-04-20 09:32 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25127: Compare with 2 decimals (866 bytes, patch)
2020-04-20 10:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25127: Use Koha::Number::Price->round for comparison (896 bytes, patch)
2020-04-20 11:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25127: Unit tests (2.33 KB, patch)
2020-04-21 06:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25127: Correction to tests (4.36 KB, patch)
2020-04-21 06:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25127: Use Koha::Number::Price->round for comparison (896 bytes, patch)
2020-04-21 06:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25127: Unit tests (2.38 KB, patch)
2020-04-22 00:28 UTC, David Nind
Details | Diff | Splinter Review
Bug 25127: Correction to tests (4.42 KB, patch)
2020-04-22 00:29 UTC, David Nind
Details | Diff | Splinter Review
Bug 25127: Use Koha::Number::Price->round for comparison (944 bytes, patch)
2020-04-22 00:29 UTC, David Nind
Details | Diff | Splinter Review
Bug 25127: Unit tests (2.44 KB, patch)
2020-05-01 12:05 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25127: Correction to tests (4.47 KB, patch)
2020-05-01 12:05 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25127: Use Koha::Number::Price->round for comparison (1001 bytes, patch)
2020-05-01 12:05 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2020-04-13 12:46:44 UTC
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"
Comment 1 Nick Clemens 2020-04-13 12:47:41 UTC
Created attachment 102826 [details] [review]
Bug 25127: Unit tests
Comment 2 Martin Renvoize 2020-04-20 08:30:59 UTC
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?
Comment 3 Martin Renvoize 2020-04-20 08:34:54 UTC
Created attachment 103235 [details] [review]
Bug 25127: Correction to tests
Comment 4 Martin Renvoize 2020-04-20 08:35:14 UTC
Aha.. I think that update to the tests clarifies it
Comment 5 Martin Renvoize 2020-04-20 09:27:48 UTC
Man I hate rounding maths using Floats :(

7.2 (maxfine) - 7.2 (total_amount_other) = 8.88178419700125e-16 ( 😢 )
Comment 6 Martin Renvoize 2020-04-20 09:32:09 UTC
Created attachment 103249 [details] [review]
Bug 25127: Crude float fix
Comment 7 Martin Renvoize 2020-04-20 09:33:15 UTC
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.
Comment 8 Jonathan Druart 2020-04-20 09:40:54 UTC
You usually need to "$integer + 0.0" to compare it with floats. It's what we do in tests.
Comment 9 Martin Renvoize 2020-04-20 09:45:23 UTC
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)
Comment 10 Jonathan Druart 2020-04-20 10:03:36 UTC
Created attachment 103253 [details] [review]
Bug 25127: Compare with 2 decimals
Comment 11 Jonathan Druart 2020-04-20 10:04:11 UTC
(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")
Comment 12 Martin Renvoize 2020-04-20 10:15:07 UTC
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.
Comment 13 Jonathan Druart 2020-04-20 10:28:16 UTC
(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.
Comment 14 Katrin Fischer 2020-04-20 10:34:53 UTC
(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.
Comment 15 Jonathan Druart 2020-04-20 10:43:04 UTC
(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.
Comment 16 Martin Renvoize 2020-04-20 11:11:10 UTC
Created attachment 103276 [details] [review]
Bug 25127: Use Koha::Number::Price->round for comparison
Comment 17 Martin Renvoize 2020-04-20 11:13:22 UTC
Final alternative.. use the existing Koha::Number::Price->round method for the comparison
Comment 18 Jonathan Druart 2020-04-20 11:27:47 UTC
Yes, that's definitely the way to go!
Comment 19 Bernardo Gonzalez Kriegel 2020-04-20 23:00:25 UTC
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?
Comment 20 Martin Renvoize 2020-04-21 06:28:26 UTC
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.
Comment 21 Martin Renvoize 2020-04-21 06:33:56 UTC
Created attachment 103325 [details] [review]
Bug 25127: Unit tests
Comment 22 Martin Renvoize 2020-04-21 06:34:00 UTC
Created attachment 103326 [details] [review]
Bug 25127: Correction to tests
Comment 23 Martin Renvoize 2020-04-21 06:34:03 UTC
Created attachment 103327 [details] [review]
Bug 25127: Use Koha::Number::Price->round for comparison
Comment 24 David Nind 2020-04-22 00:28:59 UTC
Created attachment 103430 [details] [review]
Bug 25127: Unit tests

Signed-off-by: David Nind <david@davidnind.com>
Comment 25 David Nind 2020-04-22 00:29:02 UTC
Created attachment 103431 [details] [review]
Bug 25127: Correction to tests

Signed-off-by: David Nind <david@davidnind.com>
Comment 26 David Nind 2020-04-22 00:29:06 UTC
Created attachment 103432 [details] [review]
Bug 25127: Use Koha::Number::Price->round for comparison

Signed-off-by: David Nind <david@davidnind.com>
Comment 27 David Nind 2020-04-22 00:29:57 UTC
(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!
Comment 28 Nick Clemens 2020-05-01 12:05:13 UTC
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>
Comment 29 Nick Clemens 2020-05-01 12:05:16 UTC
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>
Comment 30 Nick Clemens 2020-05-01 12:05:19 UTC
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>
Comment 31 Martin Renvoize 2020-05-01 14:46:09 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 32 Joy Nelson 2020-05-08 23:00:02 UTC
backported to 19.11.x for 19.11.06
Comment 33 Lucas Gass 2020-05-15 17:27:04 UTC
missing dependencies for 19.05.x, no backport