Bug 22982 - Paying lost fee does not always remove lost item from checkouts
Summary: Paying lost fee does not always remove lost item from checkouts
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Kyle M Hall
QA Contact: Martin Renvoize
URL:
Keywords:
: 21042 (view as bug list)
Depends on:
Blocks: 24474 22563
  Show dependency treegraph
 
Reported: 2019-05-23 18:12 UTC by Kyle M Hall
Modified: 2020-01-21 20:06 UTC (History)
11 users (show)

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


Attachments
Bug 22982: Paying lost fee does not always remove lost item from checkouts (1.85 KB, patch)
2019-05-23 18:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22982: Add unit tests (5.20 KB, patch)
2019-05-23 18:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22982: Paying lost fee does not always remove lost item from checkouts (1.88 KB, patch)
2019-05-23 18:42 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 22982: Add unit tests (5.23 KB, patch)
2019-05-23 18:42 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 22982: (QA follow-up) Add behavior to Koha::Account::Line::apply and unit test (4.89 KB, patch)
2019-05-28 12:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22982: Paying lost fee does not always remove lost item from checkouts (1.93 KB, patch)
2019-05-31 18:30 UTC, Nadine Pierre
Details | Diff | Splinter Review
Bug 22982: Add unit tests (5.28 KB, patch)
2019-05-31 18:30 UTC, Nadine Pierre
Details | Diff | Splinter Review
Bug 22982: (QA follow-up) Add behavior to Koha::Account::Line::apply and unit test (4.93 KB, patch)
2019-05-31 18:30 UTC, Nadine Pierre
Details | Diff | Splinter Review
Bug 22982: Paying lost fee does not always remove lost item from checkouts (2.00 KB, patch)
2019-06-05 12:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22982: Add unit tests (5.34 KB, patch)
2019-06-05 12:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22982: (QA follow-up) Add behavior to Koha::Account::Line::apply and unit test (4.99 KB, patch)
2019-06-05 12:38 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2019-05-23 18:12:43 UTC
Depending on how a lost fee is paid, it may or may not be removed from the patrons current checkouts. The current expected behavior is for the lost item to be removed from the patrons checkouts when the lost fee is paid in full.

This is due to the subroutine for handling of lost fees being included in the 'pay specific lines' code but not in the 'pay by amount' code.
Comment 1 Kyle M Hall 2019-05-23 18:16:00 UTC
Created attachment 90046 [details] [review]
Bug 22982: Paying lost fee does not always remove lost item from checkouts

Depending on how a lost fee is paid, it may or may not be removed from the patrons current checkouts. The current expected behavior is for the lost item to be removed from the patrons checkouts when the lost fee is paid in full.

This is due to the subroutine for handling of lost fees being included in the 'pay specific lines' code but not in the 'pay by amount' code.

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Koha/Account.t
Comment 2 Kyle M Hall 2019-05-23 18:16:20 UTC
Created attachment 90047 [details] [review]
Bug 22982: Add unit tests
Comment 3 Liz Rea 2019-05-23 18:42:17 UTC
Created attachment 90050 [details] [review]
Bug 22982: Paying lost fee does not always remove lost item from checkouts

Depending on how a lost fee is paid, it may or may not be removed from the patrons current checkouts. The current expected behavior is for the lost item to be removed from the patrons checkouts when the lost fee is paid in full.

This is due to the subroutine for handling of lost fees being included in the 'pay specific lines' code but not in the 'pay by amount' code.

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Koha/Account.t

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 4 Liz Rea 2019-05-23 18:42:27 UTC
Created attachment 90051 [details] [review]
Bug 22982: Add unit tests

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 5 Martin Renvoize 2019-05-24 14:15:14 UTC
This doesn't feel like it'll catch all cases.. especially as some things have already started to move to using the ->apply routines.

I should probably help to fold this into the rest of the accounts tree that's been growing again of late.
Comment 6 Andrew Fuerste-Henry 2019-05-25 17:17:56 UTC
Is this a dupe of https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21042?
Comment 7 Kyle M Hall 2019-05-28 11:36:12 UTC
*** Bug 21042 has been marked as a duplicate of this bug. ***
Comment 8 Kyle M Hall 2019-05-28 12:02:50 UTC
Created attachment 90152 [details] [review]
Bug 22982: (QA follow-up) Add behavior to Koha::Account::Line::apply and unit test
Comment 9 Marcel de Rooy 2019-05-31 08:08:26 UTC
(In reply to Martin Renvoize from comment #5)
> This doesn't feel like it'll catch all cases.. especially as some things
> have already started to move to using the ->apply routines.
> 
> I should probably help to fold this into the rest of the accounts tree
> that's been growing again of late.

Martin: Please have another look too.
I am having some doubts too on cases where $fine->amountoutstanding > $balance_remaining in the first edit. $new_amountoutstanding may not be zero.
Comment 10 Kyle M Hall 2019-05-31 11:27:15 UTC
> I am having some doubts too on cases where $fine->amountoutstanding >
> $balance_remaining in the first edit. $new_amountoutstanding may not be zero.

In cases where $new_amountoutstanding is not zero, we should *not* remove the item from the patron's checkouts. We should only do so once the entire fee is paid off. If you feel otherwise please give specific details that I can act on.
Comment 11 Nadine Pierre 2019-05-31 18:30:31 UTC
Created attachment 90243 [details] [review]
Bug 22982: Paying lost fee does not always remove lost item from checkouts

Depending on how a lost fee is paid, it may or may not be removed from the patrons current checkouts. The current expected behavior is for the lost item to be removed from the patrons checkouts when the lost fee is paid in full.

This is due to the subroutine for handling of lost fees being included in the 'pay specific lines' code but not in the 'pay by amount' code.

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Koha/Account.t

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Nadine Pierre <nadine.pierre@inLibro.com>
Comment 12 Nadine Pierre 2019-05-31 18:30:38 UTC
Created attachment 90244 [details] [review]
Bug 22982: Add unit tests

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Nadine Pierre <nadine.pierre@inLibro.com>
Comment 13 Nadine Pierre 2019-05-31 18:30:43 UTC
Created attachment 90245 [details] [review]
Bug 22982: (QA follow-up) Add behavior to Koha::Account::Line::apply and unit test

Signed-off-by: Nadine Pierre <nadine.pierre@inLibro.com>
Comment 14 Martin Renvoize 2019-06-05 12:38:49 UTC
Created attachment 90327 [details] [review]
Bug 22982: Paying lost fee does not always remove lost item from checkouts

Depending on how a lost fee is paid, it may or may not be removed from the
patrons current checkouts. The current expected behavior is for the lost
item to be removed from the patrons checkouts when the lost fee is paid in
full.

This is due to the subroutine for handling of lost fees being included in
the 'pay specific lines' code but not in the 'pay by amount' code.

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Koha/Account.t

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Nadine Pierre <nadine.pierre@inLibro.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2019-06-05 12:38:52 UTC
Created attachment 90328 [details] [review]
Bug 22982: Add unit tests

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Nadine Pierre <nadine.pierre@inLibro.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2019-06-05 12:38:55 UTC
Created attachment 90329 [details] [review]
Bug 22982: (QA follow-up) Add behavior to Koha::Account::Line::apply and unit test

Signed-off-by: Nadine Pierre <nadine.pierre@inLibro.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Martin Renvoize 2019-06-05 12:39:43 UTC
I'm pretty happy with this one now, it all works for me, passes QA scripts and I can't see any regressions.

Thanks Kyle, Passing QA
Comment 18 Martin Renvoize 2019-06-05 12:41:38 UTC
Note to self.. this will conflict quietly with 22563 when it is pushed.
Comment 19 Martin Renvoize 2019-06-05 12:44:20 UTC
Nice work!

Pushed to master for 19.11.00
Comment 20 Fridolin SOMERS 2019-06-11 13:45:39 UTC
Pushed to 19.05.x for 19.05.01
Comment 21 Aleisha Amohia 2020-01-21 20:03:21 UTC
Hey all, this patch is causing problems for some of our libraries because it's been assumed that lost items with no attached charge should automatically be returned. 

This is not always the desired behaviour - we have one case where the library wants to keep lost items on a patron's record, and another case where a checked out lost item will be renewed, but the amount outstanding is zero, so the item gets returned.

These are edge cases, but the assumptions in this patch do not consider them, and are causing problems. Is there a workaround? Or perhaps a better way of doing this patch that can allow for the cases of 'renewing a lost item' or 'wanting to keep a lost item on the checkouts record'? A solution that avoids doing circulation actions in the accounts modules as this patch does would be ideal.

Aleisha
Comment 22 Aleisha Amohia 2020-01-21 20:06:27 UTC
I have opened Bug 24474 depending on this one to fix the bug that this has caused.