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
Created attachment 102812 [details] [review] Bug 25123: Unit tests
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 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?
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
Created attachment 103232 [details] [review] Bug 25123: Unit tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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>
Created attachment 103234 [details] [review] Bug 25123: (QA follow-up) Re-used variable name Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Clear concise code change, tests read well and pass, QA script passes and change works as expected.. Signing off.
What about the FIXME a couple of lines above?
(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
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>
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>
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>
(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.
(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...
Nice work everyone! Pushed to master for 20.05
Backported to 19.11.x for 19.11.06
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