Description
Kelly McElligott
2019-12-02 18:44:36 UTC
Specifically: 538 "SELECT * FROM accountlines 539 WHERE borrowernumber=? AND 540 (( accounttype IN ('F','M') AND amountoutstanding<>0 ) OR 541 accounttype = 'FU' )" 542 ); To: 523 my $overdues = Koha::Account::Lines->search( 524 { 525 borrowernumber => $borrowernumber, 526 accounttype => [ 'OVERDUE', 'M' ], 527 amountoutstanding => { '<>' => 0 } 528 } In the first it is a closed fine more than 0 OR an accruing fine After the patch it must be greater than 0, so if someone pays off an accruing fine it will not be found. Could we possibly just search by issue_id? (In reply to Nick Clemens from comment #1) > > Could we possibly just search by issue_id? It's a little more complicated than that :(. We need to walk through all fines on the users account so we may honour `MaxFine`. The fix does appear to be trivial, but I think I'm going to submit a series of tests first to ensure I'm understanding the problem fully, bare with me whilst I write those. Created attachment 95992 [details] [review] Bug 24146: Add test cases This patch adds test cases for both checking amountoutstanding values are handled correctly and also that the paid down accruing fines do not add additional fines on increment. Test plan: Read the changeset and asses whether the changes/additional tests are correct as per the expected behaviour. (In reply to Martin Renvoize from comment #4) > Created attachment 95992 [details] [review] [review] > Bug 24146: Add test cases > > This patch adds test cases for both checking amountoutstanding values > are handled correctly and also that the paid down accruing fines do not > add additional fines on increment. > > Test plan: > Read the changeset and asses whether the changes/additional tests are > correct as per the expected behaviour. Do we need to take renewals into account? We already have the issue id, do we need the renewal iteration as well to ensure we target the correct paid fine? Created attachment 96005 [details] [review] Bug 24146: Add test cases This patch adds test cases for both checking amountoutstanding values are handled correctly and also that the paid down accruing fines do not add additional fines on increment. Test plan: Read the changeset and asses whether the changes/additional tests are correct as per the expected behaviour. Created attachment 96006 [details] [review] Bug 24146: Increment existing fine We should increment an existing fine even if it has been fully paid off whilst it is still incrementing and not create a new fine. Right, I've commented the tests a bit further and I think it shows we're testing the renewal case. The second patch fixes the error reported here, but the enhanced tests still do not fully pass... but I'm not entirely sure the tests are correct. The question is how should MaxFine act in the case of partially paid fines. Setting to NSO to get some more eyes. Created attachment 96065 [details] [review] Bug 24146: Add test cases This patch adds test cases for both checking amountoutstanding values are handled correctly and also that the paid down accruing fines do not add additional fines on increment. Test plan: Read the changeset and asses whether the changes/additional tests are correct as per the expected behaviour. Created attachment 96066 [details] [review] Bug 24146: Increment existing fine We should increment an existing fine even if it has been fully paid off whilst it is still incrementing and not create a new fine. Created attachment 96067 [details] [review] Bug 24146: Corrections to UpdateFine logic concerning maxFine The `maxFine` system preference actually refers to the amount of outsanding debt (in fines) a patron may have at a given time. This patch corrects the functionality of UpdateFine such that it properly respects that counter given the payment of accruing fines scenario. Created attachment 96068 [details] [review] Bug 24146: Illustrate increasing and decreasing fine Created attachment 96069 [details] [review] Bug 24146: Add test cases This patch adds test cases for both checking amountoutstanding values are handled correctly and also that the paid down accruing fines do not add additional fines on increment. Test plan: Read the changeset and asses whether the changes/additional tests are correct as per the expected behaviour. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 96070 [details] [review] Bug 24146: Increment existing fine We should increment an existing fine even if it has been fully paid off whilst it is still incrementing and not create a new fine. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 96071 [details] [review] Bug 24146: Corrections to UpdateFine logic concerning maxFine The `maxFine` system preference actually refers to the amount of outsanding debt (in fines) a patron may have at a given time. This patch corrects the functionality of UpdateFine such that it properly respects that counter given the payment of accruing fines scenario. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 96072 [details] [review] Bug 24146: Illustrate increasing and decreasing fine Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 97304 [details] [review] Bug 24146: Add test cases This patch adds test cases for both checking amountoutstanding values are handled correctly and also that the paid down accruing fines do not add additional fines on increment. Test plan: Read the changeset and asses whether the changes/additional tests are correct as per the expected behaviour. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 97305 [details] [review] Bug 24146: Increment existing fine We should increment an existing fine even if it has been fully paid off whilst it is still incrementing and not create a new fine. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 97306 [details] [review] Bug 24146: Corrections to UpdateFine logic concerning maxFine The `maxFine` system preference actually refers to the amount of outsanding debt (in fines) a patron may have at a given time. This patch corrects the functionality of UpdateFine such that it properly respects that counter given the payment of accruing fines scenario. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 97307 [details] [review] Bug 24146: Illustrate increasing and decreasing fine Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Nice work everyone! Pushed to master for 20.05 Pushed to 19.11.x branch for 19.11.03 just one question will be this bug backported to 19.05? Seconding the request to backport this to 19.05. -Marti Fuerst Huntsville-Madison County Public Library (In reply to Martha Fuerst from comment #24) > Seconding the request to backport this to 19.05. > > -Marti Fuerst > Huntsville-Madison County Public Library +1 If this could be backported to 19.05, we'd appreciate it. George i cannot make this patchset both work and pass tests in 19.05.x, no backport if this needs to be in 19.05.x please rebase so it will pass unit tests |