|
Description
Kyle M Hall (khall)
2019-05-23 18:12:43 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 Created attachment 90047 [details] [review] Bug 22982: Add unit tests 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> Created attachment 90051 [details] [review] Bug 22982: Add unit tests Signed-off-by: Liz Rea <wizzyrea@gmail.com> 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. Is this a dupe of https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21042? *** Bug 21042 has been marked as a duplicate of this bug. *** Created attachment 90152 [details] [review] Bug 22982: (QA follow-up) Add behavior to Koha::Account::Line::apply and unit test (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. > 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.
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> 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> 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> 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> 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> 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> 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 Note to self.. this will conflict quietly with 22563 when it is pushed. Nice work! Pushed to master for 19.11.00 Pushed to 19.05.x for 19.05.01 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 I have opened Bug 24474 depending on this one to fix the bug that this has caused. |