Bug 25123 - MaxFines does not count the current updating fine
Summary: MaxFines does not count the current updating fine
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Fines and fees (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 25127
  Show dependency treegraph
 
Reported: 2020-04-13 11:38 UTC by Nick Clemens
Modified: 2020-11-30 21:47 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.06


Attachments
Bug 25123: Unit tests (2.39 KB, patch)
2020-04-13 11:40 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25123: Count the current fine when reducing for maxFine (1.42 KB, patch)
2020-04-13 11:40 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25123: Count the current fine when reducing for maxFine (1.33 KB, patch)
2020-04-14 10:17 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25123: Unit tests (2.45 KB, patch)
2020-04-20 07:39 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25123: Count the current fine when reducing for maxFine (1.40 KB, patch)
2020-04-20 07:39 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25123: (QA follow-up) Re-used variable name (1.10 KB, patch)
2020-04-20 07:39 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25123: Unit tests (2.52 KB, patch)
2020-04-20 13:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25123: Count the current fine when reducing for maxFine (1.47 KB, patch)
2020-04-20 13:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25123: (QA follow-up) Re-used variable name (1.17 KB, patch)
2020-04-20 13:06 UTC, Jonathan Druart
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 11:38:02 UTC
If maxFine is set, we total the patrons outstanding fines when making an adjustment, however, we neglect to count the amount of a currently updating fine when doing so.

To test:
1 - Set maxFine to 5
2 - Create an overdue amount of 4.99 for a patron
3 - Set an itemtype to have a fine of $.10 per day
4 - Checkout an item of that type to a patron and backdate the due date so it is overdue
5 - Run fines.pl with -v
6 - Note the fine is reduced from $.10 (or a multiple) to .01
7 - Run it again, a second cent is added
8 - Repeat and note it keeps happening until the amount of the fine is reached, exceeding the maxFine setting
Comment 1 Nick Clemens 2020-04-13 11:40:20 UTC
Created attachment 102812 [details] [review]
Bug 25123: Unit tests
Comment 2 Nick Clemens 2020-04-13 11:40:22 UTC
Created attachment 102813 [details] [review]
Bug 25123: Count the current fine when reducing for maxFine

If maxFine is set, we total the patrons outstanding fines when making an adjustment, however, we neglect to count the amount of a currently updating fine when doing so.

To test:
1 - Set maxFine to 5
2 - Create an overdue amount of 4.99 for a patron
3 - Set an itemtype to have a fine of $.10 per day
4 - Checkout an item of that type to a patron and backdate the due date so it is overdue
5 - Run fines.pl with -v
6 - Note the fine is reduced from $.10 (or a multiple) to .01
7 - Run it again, a second cent is added
8 - Repeat and note it keeps happening until the amount of the fine is reached, exceeding the maxFine setting
9 - Apply patch
10 - Note the fine is now reduced to 0 and nothing is added to account
Comment 3 Martin Renvoize 2020-04-14 10:05:54 UTC
Comment on attachment 102813 [details] [review]
Bug 25123: Count the current fine when reducing for maxFine

Review of attachment 102813 [details] [review]:
-----------------------------------------------------------------

::: C4/Overdues.pm
@@ +555,2 @@
>                  $accountline = $overdue;
>                  next;

I wonder if it might be simpler to just drop the call to 'next' instead of adding the count line?
Comment 4 Nick Clemens 2020-04-14 10:17:29 UTC
Created attachment 102920 [details] [review]
Bug 25123: Count the current fine when reducing for maxFine

If maxFine is set, we total the patrons outstanding fines when making an adjustment, however, we neglect to count the amount of a currently updating fine when doing so.

To test:
1 - Set maxFine to 5
2 - Create an overdue amount of 4.99 for a patron
3 - Set an itemtype to have a fine of $.10 per day
4 - Checkout an item of that type to a patron and backdate the due date so it is overdue
5 - Run fines.pl with -v
6 - Note the fine is reduced from $.10 (or a multiple) to .01
7 - Run it again, a second cent is added
8 - Repeat and note it keeps happening until the amount of the fine is reached, exceeding the maxFine setting
9 - Apply patch
10 - Note the fine is now reduced to 0 and nothing is added to account
Comment 5 Martin Renvoize 2020-04-20 07:39:01 UTC
Created attachment 103232 [details] [review]
Bug 25123: Unit tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2020-04-20 07:39:04 UTC
Created attachment 103233 [details] [review]
Bug 25123: Count the current fine when reducing for maxFine

If maxFine is set, we total the patrons outstanding fines when making an adjustment, however, we neglect to count the amount of a currently updating fine when doing so.

To test:
1 - Set maxFine to 5
2 - Create an overdue amount of 4.99 for a patron
3 - Set an itemtype to have a fine of $.10 per day
4 - Checkout an item of that type to a patron and backdate the due date so it is overdue
5 - Run fines.pl with -v
6 - Note the fine is reduced from $.10 (or a multiple) to .01
7 - Run it again, a second cent is added
8 - Repeat and note it keeps happening until the amount of the fine is reached, exceeding the maxFine setting
9 - Apply patch
10 - Note the fine is now reduced to 0 and nothing is added to account

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize 2020-04-20 07:39:07 UTC
Created attachment 103234 [details] [review]
Bug 25123: (QA follow-up) Re-used variable name

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2020-04-20 07:39:49 UTC
Clear concise code change, tests read well and pass, QA script passes and change works as expected.. Signing off.
Comment 9 Jonathan Druart 2020-04-20 11:37:00 UTC
What about the FIXME a couple of lines above?
Comment 10 Nick Clemens 2020-04-20 12:18:29 UTC
(In reply to Jonathan Druart from comment #9)
> What about the FIXME a couple of lines above?

That behaviour doesn't change, and is still a valid question, how did we get two fines on one issue and how should we resolve? But these patches fix the other case so I don't think we need to address here
Comment 11 Jonathan Druart 2020-04-20 13:06:02 UTC
Created attachment 103294 [details] [review]
Bug 25123: Unit tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 12 Jonathan Druart 2020-04-20 13:06:05 UTC
Created attachment 103295 [details] [review]
Bug 25123: Count the current fine when reducing for maxFine

If maxFine is set, we total the patrons outstanding fines when making an adjustment, however, we neglect to count the amount of a currently updating fine when doing so.

To test:
1 - Set maxFine to 5
2 - Create an overdue amount of 4.99 for a patron
3 - Set an itemtype to have a fine of $.10 per day
4 - Checkout an item of that type to a patron and backdate the due date so it is overdue
5 - Run fines.pl with -v
6 - Note the fine is reduced from $.10 (or a multiple) to .01
7 - Run it again, a second cent is added
8 - Repeat and note it keeps happening until the amount of the fine is reached, exceeding the maxFine setting
9 - Apply patch
10 - Note the fine is now reduced to 0 and nothing is added to account

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 13 Jonathan Druart 2020-04-20 13:06:08 UTC
Created attachment 103296 [details] [review]
Bug 25123: (QA follow-up) Re-used variable name

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 14 Martin Renvoize 2020-04-20 14:39:13 UTC
(In reply to Nick Clemens from comment #10)
> (In reply to Jonathan Druart from comment #9)
> > What about the FIXME a couple of lines above?
> 
> That behaviour doesn't change, and is still a valid question, how did we get
> two fines on one issue and how should we resolve? But these patches fix the
> other case so I don't think we need to address here

Agreed.. when I last dug into this I believe I reached the conclusion that in automated fines code you cannot reach such a state.. but if people are using custom fines or adding things manually we can end up in all sorts of a mess...

I know at least Katrin has her own fines code so I didn't remove it 'just in case', but would like to clean it up at some point.
Comment 15 Katrin Fischer 2020-04-20 17:01:23 UTC
(In reply to Martin Renvoize from comment #14)
> (In reply to Nick Clemens from comment #10)
> > (In reply to Jonathan Druart from comment #9)
> > > What about the FIXME a couple of lines above?
> > 
> > That behaviour doesn't change, and is still a valid question, how did we get
> > two fines on one issue and how should we resolve? But these patches fix the
> > other case so I don't think we need to address here

> I know at least Katrin has her own fines code so I didn't remove it 'just in
> case', but would like to clean it up at some point.

Guilty, but it's not a super bad hack... only a little bit bad. We just do the fine calculation differently, but use the standard methods for triggering and placing the fine (might have to make that work again with all the changes to accounts in last versions? :) I will see when it comes to it)

>  how did we get two fines on one issue and how should we resolve?
Do you mean 2 accrueing fines? Or just fines? In some older DB we didn't do the FU > F change cleanly, so bad data could be an issue. Generally:
Checkout
Gets overdue + fines
Renew
Gets overdue + fines again
Renew...
Comment 16 Martin Renvoize 2020-04-21 11:31:55 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 17 Joy Nelson 2020-05-08 18:47:01 UTC
Backported to 19.11.x for 19.11.06
Comment 18 Lucas Gass 2020-05-14 18:06:35 UTC
Tried to rebase tests for 19.05 but cannot make them pass, skipping this one for 19.05.x, if it is needed please rebase