Bug 24146 - Paying Accruing Fines prior to return causes another accruing fine when returned
Summary: Paying Accruing Fines prior to return causes another accruing fine when returned
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: rel_19_05_candidate
Depends on:
Blocks: 24186 24408
  Show dependency treegraph
 
Reported: 2019-12-02 18:44 UTC by Kelly McElligott
Modified: 2020-02-13 18:04 UTC (History)
10 users (show)

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


Attachments
Bug 24146: Add test cases (5.79 KB, patch)
2019-12-04 14:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24146: Add test cases (7.47 KB, patch)
2019-12-05 09:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24146: Increment existing fine (1.01 KB, patch)
2019-12-05 10:17 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24146: Add test cases (8.43 KB, patch)
2019-12-06 15:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24146: Increment existing fine (1.01 KB, patch)
2019-12-06 15:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24146: Corrections to UpdateFine logic concerning maxFine (1.98 KB, patch)
2019-12-06 15:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24146: Illustrate increasing and decreasing fine (2.58 KB, patch)
2019-12-06 15:35 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24146: Add test cases (8.49 KB, patch)
2019-12-06 16:23 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24146: Increment existing fine (1.07 KB, patch)
2019-12-06 16:23 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24146: Corrections to UpdateFine logic concerning maxFine (2.03 KB, patch)
2019-12-06 16:23 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24146: Illustrate increasing and decreasing fine (2.63 KB, patch)
2019-12-06 16:23 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24146: Add test cases (8.56 KB, patch)
2020-01-13 11:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24146: Increment existing fine (1.14 KB, patch)
2020-01-13 11:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24146: Corrections to UpdateFine logic concerning maxFine (2.10 KB, patch)
2020-01-13 11:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24146: Illustrate increasing and decreasing fine (2.71 KB, patch)
2020-01-13 11:58 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 Kelly McElligott 2019-12-02 18:44:36 UTC
In 19.05, when an accruing fine is paid prior to checking in the item, the fine will get re-accessed when the item is returned. 

This is caused by bug 22521 looking at the diff to this bug 


Lines 520-565 sub UpdateFine {


it looks like we have changed the way Koha is checking for these fines and therefore reassessing a fine as there isn't one there anymore, as it has been paid.
Comment 1 Nick Clemens 2019-12-02 19:22: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?
Comment 2 Martin Renvoize 2019-12-04 07:56:33 UTC
(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`.
Comment 3 Martin Renvoize 2019-12-04 07:58:55 UTC
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.
Comment 4 Martin Renvoize 2019-12-04 14:26:50 UTC
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.
Comment 5 Kyle M Hall 2019-12-04 15:44:29 UTC
(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?
Comment 6 Martin Renvoize 2019-12-05 09:53:34 UTC
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.
Comment 7 Martin Renvoize 2019-12-05 10:17:14 UTC
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.
Comment 8 Martin Renvoize 2019-12-05 10:19:33 UTC
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.
Comment 9 Martin Renvoize 2019-12-06 15:13:00 UTC
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.
Comment 10 Martin Renvoize 2019-12-06 15:13:03 UTC
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.
Comment 11 Martin Renvoize 2019-12-06 15:13:06 UTC
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.
Comment 12 Nick Clemens 2019-12-06 15:35:47 UTC
Created attachment 96068 [details] [review]
Bug 24146: Illustrate increasing and decreasing fine
Comment 13 Nick Clemens 2019-12-06 16:23:26 UTC
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>
Comment 14 Nick Clemens 2019-12-06 16:23:29 UTC
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>
Comment 15 Nick Clemens 2019-12-06 16:23:32 UTC
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>
Comment 16 Nick Clemens 2019-12-06 16:23:34 UTC
Created attachment 96072 [details] [review]
Bug 24146: Illustrate increasing and decreasing fine

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 17 Jonathan Druart 2020-01-13 11:58:09 UTC
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>
Comment 18 Jonathan Druart 2020-01-13 11:58:12 UTC
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>
Comment 19 Jonathan Druart 2020-01-13 11:58:16 UTC
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>
Comment 20 Jonathan Druart 2020-01-13 11:58:20 UTC
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>
Comment 21 Martin Renvoize 2020-01-13 13:41:08 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 22 Joy Nelson 2020-01-31 21:32:07 UTC
Pushed to 19.11.x branch for 19.11.03
Comment 23 Hugo Agud 2020-02-06 03:20:07 UTC
just one question will be this bug backported to 19.05?
Comment 24 Martha Fuerst 2020-02-12 17:20:39 UTC
Seconding the request to backport this to 19.05.

-Marti Fuerst
Huntsville-Madison County Public Library
Comment 25 Lisette Scheer 2020-02-12 17:21:37 UTC
(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
Comment 26 George Williams (NEKLS) 2020-02-12 17:22:23 UTC
If this could be backported to 19.05, we'd appreciate it.

George
Comment 27 Lucas Gass 2020-02-13 17:51:52 UTC
i cannot make this patchset both work and pass tests in 19.05.x, no backport
Comment 28 Lucas Gass 2020-02-13 18:04:41 UTC
if this needs to be in 19.05.x please rebase so it will pass unit tests