This was a bug introduced by Bug 22982. "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."
Are you intending to work on this Aleisha.. just checking before I look at it myself.
It feels like yet another value to add to the `MarkLostItemsAsReturned` system preference to me.
Hm, if a library doesn't charge a lost fee and somene renews an item marked lost - would it still return it? I think the renewal part is what worries me most here and I think we shoudl never return an item on renewal. On paying the fine off, I can see why someone would want it and that seems ok to be pref driven.
(In reply to Martin Renvoize from comment #1) > Are you intending to work on this Aleisha.. just checking before I look at > it myself. Thanks for checking Martin, feel free to work on this yourself.
Created attachment 98392 [details] [review] Bug 24474: Add tests This does the following using the interface: - Unselect all the options from MarkLostItemsAsReturned - Check an item out - Create a manual invoice LOST for the checked in item - Pay the fine (Make a payment> Pay > Confirm) - Renew the item => Item is checked in (NOT EXPECTED)
Created attachment 98393 [details] [review] Bug 24474: A try This does not work
I gave a try to this, wrote the test then try to fix it. But my understanding of the code leads me to abandon. I do not understand why there are 3 calls to ReturnLostItem: Koha/Account.pm: C4::Circulation::ReturnLostItem( $self->{patron_id}, $fine->itemnumber ) unless $checkout; Koha/Account.pm: C4::Circulation::ReturnLostItem( $self->{patron_id}, $fine->itemnumber ) unless $checkout; Koha/Account/Line.pm: C4::Circulation::ReturnLostItem( $self->borrowernumber, $debit->itemnumber ); That seems wrong to me. So I guess my test will not cover all the cases. Hope it could help as a start anyway.
(In reply to Aleisha Amohia from comment #0) > This was a bug introduced by Bug 22982. > > "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. I don't understand what you mean by this.. the code in bug 22982 only applies to LOST charges.. I'm not sure I can see how items are being returned when no lost item charge is attached in the first place? > 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. So, this is where there are LOST charges being applied right? Instead of automatically returning the item on lost charge payment we want to allow for configurable state changes instead.. or am I misunderstanding the problem/request? > > 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."
(In reply to Jonathan Druart from comment #7) > I gave a try to this, wrote the test then try to fix it. > > But my understanding of the code leads me to abandon. I do not understand > why there are 3 calls to ReturnLostItem: > > Koha/Account.pm: C4::Circulation::ReturnLostItem( > $self->{patron_id}, $fine->itemnumber ) unless $checkout; > Koha/Account.pm: C4::Circulation::ReturnLostItem( > $self->{patron_id}, $fine->itemnumber ) unless $checkout; > Koha/Account/Line.pm: C4::Circulation::ReturnLostItem( > $self->borrowernumber, $debit->itemnumber ); > > That seems wrong to me. > > So I guess my test will not cover all the cases. Hope it could help as a > start anyway. Not sure I understand what you tried here Jonathan.. looking like you're simply adding a second check to see if the item is still checked out before returning it.. I think we need clarification on the requested behaviour before continuing here.
From what I understand: - the LOST charge is applied, but the library decided to not auto-return the item (config wise) - then renewing the item returns it I'll have to give this a go in testing, but I think we need to verify and try stuff here. Would be great to get a step by step by Aleisha if possible.
.. or maybe they just don't charge a fee. (no lost charge). I really wish our lost handling options were less confusing and more in one spot.
WhenLostChargeReplacementFee DefaultLongOverdueChargeValue MarkLostItemsAsReturned Are the ones I am thinking of.
Thats right, you have an item that is marked lost. But the library wants that to stay on the persons card. Not be removed, so it is still out to them. Then if you renew that item, * then that makes it no longer lost - correct * removes the charge - correct - Now the code goes on, there is a paid charge for lost, I will return it - not correct IE when you renew a lost item, the code in the accounts now returns it, because it has a paid for charge. I feel like having circulation actions inside the accounts module is a bad thing anyway, but certainly creates unexpected (and wrong) behaviour in this case
Thanks for the clarifications guys.. it's a real action at a distance issue I see.. hmm, I agree it would be best to strip out the behaviour from the accounts code and push it up a level into the controllers really. I'll have a go at that when I'm fresh tomorrow.. unless anyone else wants to give it a punt..
Comment on attachment 98393 [details] [review] Bug 24474: A try That really did not make sense! :)
(In reply to Chris Cormack from comment #13) > Then if you renew that item, > * then that makes it no longer lost - correct > * removes the charge - correct > - Now the code goes on, there is a paid charge for lost, I will return it > - not correct Martin, I think these steps are what my tests are doing and that they could be use to prove that a fix is correct.
Created attachment 98436 [details] [review] Bug 24474: Drop `ReturnLostItem` from ::Account classes This patche removes the ReturnLostItem calls from Koha::Account->pay and Koha::Account::Line->apply methods.
Created attachment 98437 [details] [review] Bug 24474: Add ReturnLostItem handling for SIP Payments This patch adds a call to ReturnLostItem for Lost Item payments taken via SIP machines.
Created attachment 98438 [details] [review] Bug 24474: Add ReturnLostItem handling for staff client payments This patch adds a call to ReturnLostItem for Lost Item payments taken via the staff client.
Created attachment 98439 [details] [review] Bug 24474: Add ReturnLostItem handling for paypal payments This patch adds a call to ReturnLostItem for Lost Item payments taken via OPAC online payments through PayPal.
This is still a work in progress.. I need to add some further tests selenium tests as this is moving code out of tested objects and into the controllers where really it should belong.. I really wish our controller layer was lighter and more clearly tested.. paycollect, for example, is a real mess at this point and I'm sure must contain untested functionality. I'll get writing tests and hopefully get this patchset through before swinging back around for a refactoring effort there.
OK.. having another go.. I don't like that I was moving the logic out of a well testing Object and into untested and hard to test messy controllers... bare with me.
Created attachment 98460 [details] [review] Bug 24474: Add `onpayment` option to `MarkLostItemsReturned`
Created attachment 98485 [details] [review] Bug 24474: Add tests This does the following using the interface: - Unselect all the options from MarkLostItemsAsReturned except for 'onpayment' - Check an item out - Create a manual invoice LOST for the checked in item - Renew the item - Check that the item was not returned
Created attachment 98486 [details] [review] Bug 24474: Add `onpayment` option to `MarkLostItemsReturned`
Created attachment 98487 [details] [review] Bug 24474: Filter out LOST_FOUND
Probably not QA ready, but certainly in good enough shape for some testing please.
For whatever reason I can't seem to get the 'onpayment' option to show up for MarkLostItemsAsReturned. (Applied patch, ran database update, restart_all). For the tests I get this fail: root@50d5e409ced0:koha(bz24474)$ prove t/db_dependent/Circulation.t t/db_dependent/Circulation.t .. 30/47 # Failed test 'Should have a single 10 days overdue fine and lost charge' # at t/db_dependent/Circulation.t line 2344. # got: '27' # expected: '17' # Looks like you failed 1 test of 3. # Failed test 'bug 22877' # at t/db_dependent/Circulation.t line 2347. # Looks like you failed 1 test of 5. t/db_dependent/Circulation.t .. 31/47 # Failed test 'AddReturn | is_overdue' # at t/db_dependent/Circulation.t line 2347. DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`account_offsets`, CONSTRAINT `account_offsets_ibfk_t` FOREIGN KEY (`type`) REFERENCES `account_offset_types` (`type`) ON DELETE CASCADE ON UPDATE CASCADE) [for Statement "INSERT INTO `account_offsets` ( `amount`, `credit_id`, `type`) VALUES ( ?, ?, ? )" with ParamValues: 0=-99, 1='25', 2='Lost Item Found'] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1836. DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`account_offsets`, CONSTRAINT `account_offsets_ibfk_t` FOREIGN KEY (`type`) REFERENCES `account_offset_types` (`type`) ON DELETE CASCADE ON UPDATE CASCADE) [for Statement "INSERT INTO `account_offsets` ( `amount`, `credit_id`, `type`) VALUES ( ?, ?, ? )" with ParamValues: 0=-99, 1='28', 2='Lost Item Found'] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1836. DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`account_offsets`, CONSTRAINT `account_offsets_ibfk_t` FOREIGN KEY (`type`) REFERENCES `account_offset_types` (`type`) ON DELETE CASCADE ON UPDATE CASCADE) [for Statement "INSERT INTO `account_offsets` ( `amount`, `credit_id`, `type`) VALUES ( ?, ?, ? )" with ParamValues: 0=-74, 1='33', 2='Lost Item Found'] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1836. DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`account_offsets`, CONSTRAINT `account_offsets_ibfk_t` FOREIGN KEY (`type`) REFERENCES `account_offset_types` (`type`) ON DELETE CASCADE ON UPDATE CASCADE) [for Statement "INSERT INTO `account_offsets` ( `amount`, `credit_id`, `type`) VALUES ( ?, ?, ? )" with ParamValues: 0=-100, 1='37', 2='Lost Item Found'] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1836. t/db_dependent/Circulation.t .. 45/47 # Looks like you planned 2 tests but ran 1. t/db_dependent/Circulation.t .. 47/47 # Failed test 'Do not return on renewal (LOST charge)' # at t/db_dependent/Circulation.t line 3689. # Looks like you failed 2 tests of 47. t/db_dependent/Circulation.t .. Dubious, test returned 2 (wstat 512, 0x200) Failed 2/47 subtests Test Summary Report ------------------- t/db_dependent/Circulation.t (Wstat: 512 Tests: 47 Failed: 2) Failed tests: 31, 47 Non-zero exit status: 2 Files=1, Tests=47, 29 wallclock secs ( 0.09 usr 0.00 sys + 23.87 cusr 2.77 csys = 26.73 CPU) Result: FAIL
Created attachment 98645 [details] [review] Bug 24474: Add tests This does the following using the interface: - Unselect all the options from MarkLostItemsAsReturned except for 'onpayment' - Check an item out - Create a manual invoice LOST for the checked in item - Renew the item - Check that the item was not returned
Created attachment 98646 [details] [review] Bug 24474: Add `onpayment` option to `MarkLostItemsReturned`
Created attachment 98647 [details] [review] Bug 24474: Filter out LOST_FOUND
Created attachment 98648 [details] [review] Bug 24474: (follow-up) Add onpayment to sysprefs file This patch adds the required line to the circulation.pref file to dispaly the onpayment option in the MarkLostItemReturned preference
(In reply to David Nind from comment #28) > For whatever reason I can't seem to get the 'onpayment' option to show up > for MarkLostItemsAsReturned. (Applied patch, ran database update, > restart_all). Doh! I missed a file when committing.. that should now be resolved. The test failure we're actually in the dependancy bug.. I've resolved them there and rebased this patchset upon them now. Cheers for testing :)
Created attachment 98673 [details] [review] Bug 24474: Add tests This does the following using the interface: - Unselect all the options from MarkLostItemsAsReturned except for 'onpayment' - Check an item out - Create a manual invoice LOST for the checked in item - Renew the item - Check that the item was not returned Signed-off-by: David Nind <david@davidnind.com>
Created attachment 98674 [details] [review] Bug 24474: Add `onpayment` option to `MarkLostItemsReturned` Signed-off-by: David Nind <david@davidnind.com>
Created attachment 98675 [details] [review] Bug 24474: Filter out LOST_FOUND Signed-off-by: David Nind <david@davidnind.com>
Created attachment 98676 [details] [review] Bug 24474: (follow-up) Add onpayment to sysprefs file This patch adds the required line to the circulation.pref file to dispaly the onpayment option in the MarkLostItemReturned preference Signed-off-by: David Nind <david@davidnind.com>
Now works as outlined in the test plan! I re-ran the tests after signing off and one test fails: root@314245d82a59:koha(bz24474)$ prove t/db_dependent/Circulation.t t/db_dependent/Circulation.t .. 30/47 # Failed test 'Should have a single 10 days overdue fine and lost charge' # at t/db_dependent/Circulation.t line 2344. # got: '27' # expected: '17' # Looks like you failed 1 test of 3. # Failed test 'bug 22877' # at t/db_dependent/Circulation.t line 2347. # Looks like you failed 1 test of 5. t/db_dependent/Circulation.t .. 31/47 # Failed test 'AddReturn | is_overdue' # at t/db_dependent/Circulation.t line 2347. t/db_dependent/Circulation.t .. 47/47 # Looks like you failed 1 test of 47. t/db_dependent/Circulation.t .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/47 subtests Test Summary Report ------------------- t/db_dependent/Circulation.t (Wstat: 256 Tests: 47 Failed: 1) Failed test: 31 Non-zero exit status: 1 Files=1, Tests=47, 29 wallclock secs ( 0.08 usr 0.00 sys + 23.90 cusr 2.79 csys = 26.77 CPU) Result: FAIL
Hmm, I can't seem to replicate that test failure.. lets see how it goes in the QA run.
Martin, what about the 2 occurrences in Koha::Account->pay?
(In reply to Jonathan Druart from comment #40) > Martin, what about the 2 occurrences in Koha::Account->pay? I think I traced it back and found that it's only actually occurring via a call to apply (i.e the call inside _FixAccountForLostAndFound), but it's probably not a bad idea to match the code in the two locations. I'll add a followup.
Created attachment 98923 [details] [review] Bug 24474: (follow-up) Add LOST_FOUND logic to Koha::Account::pay This patch adds the same updated logic as found in Koha::Account::Line->apply to handle LOST_FOUND credits being applied against their associated LOST debit. Currently a 'LOST_FOUND' credit is never created via the pay method and so the additional test will always pass, but the addition here accounts for possible future uses.
As promised.
Switching to Needs Signoff as I would like Aleisha to have a look at Martin's patches. However regression tests are missing, but that does not block human testing :)
Created attachment 100229 [details] [review] Bug 24474: Add tests This does the following using the interface: - Unselect all the options from MarkLostItemsAsReturned except for 'onpayment' - Check an item out - Create a manual invoice LOST for the checked in item - Renew the item - Check that the item was not returned Signed-off-by: David Nind <david@davidnind.com>
Created attachment 100230 [details] [review] Bug 24474: Add `onpayment` option to `MarkLostItemsReturned` Signed-off-by: David Nind <david@davidnind.com>
Created attachment 100231 [details] [review] Bug 24474: Filter out LOST_FOUND Signed-off-by: David Nind <david@davidnind.com>
Created attachment 100232 [details] [review] Bug 24474: (follow-up) Add onpayment to sysprefs file This patch adds the required line to the circulation.pref file to dispaly the onpayment option in the MarkLostItemReturned preference Signed-off-by: David Nind <david@davidnind.com>
Created attachment 100233 [details] [review] Bug 24474: (follow-up) Add LOST_FOUND logic to Koha::Account::pay This patch adds the same updated logic as found in Koha::Account::Line->apply to handle LOST_FOUND credits being applied against their associated LOST debit. Currently a 'LOST_FOUND' credit is never created via the pay method and so the additional test will always pass, but the addition here accounts for possible future uses.
Rebased again.. please test this Aleisha
Another few weeks passed.. quickly thinking this isn't 'critical' at this point. :(
Everything works as per the test plan. However, there is still a failed test as before. Is there a system preference or setting related to test 31? I haven't failed QA, but I haven't signed it off yet either. root@8ca270a5fd09:koha(bz24474)$ prove t/db_dependent/Circulation.t t/db_dependent/Circulation.t .. 30/47 # Failed test 'Should have a single 10 days overdue fine and lost charge' # at t/db_dependent/Circulation.t line 2430. # got: '27' # expected: '17' # Looks like you failed 1 test of 3. # Failed test 'bug 22877' # at t/db_dependent/Circulation.t line 2433. # Looks like you failed 1 test of 5. t/db_dependent/Circulation.t .. 31/47 # Failed test 'AddReturn | is_overdue' # at t/db_dependent/Circulation.t line 2433. t/db_dependent/Circulation.t .. 47/47 # Looks like you failed 1 test of 47. t/db_dependent/Circulation.t .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/47 subtests Test Summary Report ------------------- t/db_dependent/Circulation.t (Wstat: 256 Tests: 47 Failed: 1) Failed test: 31 Non-zero exit status: 1 Files=1, Tests=47, 30 wallclock secs ( 0.09 usr 0.00 sys + 24.44 cusr 2.49 csys = 27.02 CPU) Result: FAIL
How are you running the tests David? They're all passing for me within koha-testing-docker.. it feels like perhaps you have some data in your database that the tests are not expecting.. I'll take a look at trying to improve them to prevent such failures.
Hmm.. I can't see any obvious reason for this :(
I figured out what has happened: 1) If I leave MarkLostItemsAsReturned with only the 'when receiving payment for the item' option selected (as per the test plan) then the tests fail. 2) If I change MarkLostItemsAsReturned back to the default (with all options selected) then the tests pass. If this is fine, then I will sign-off.
That's really helpful David, thankyou. The tests really aught to take account of that.. I'll take a look.
Created attachment 102104 [details] [review] Bug 24474: (QA follow-up) Fix failing test The test was assuming default data and would fail if you ran through the test plan (and disabled all but `onpayment` for the 'MarkLostItemsAsReturned' preference) on the data before running the included unit tests.
Created attachment 102105 [details] [review] Bug 24474: Add tests This does the following using the interface: - Unselect all the options from MarkLostItemsAsReturned except for 'onpayment' - Check an item out - Create a manual invoice LOST for the checked in item - Renew the item - Check that the item was not returned Signed-off-by: David Nind <david@davidnind.com>
Created attachment 102106 [details] [review] Bug 24474: Add `onpayment` option to `MarkLostItemsReturned` Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 102107 [details] [review] Bug 24474: Filter out LOST_FOUND Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 102108 [details] [review] Bug 24474: (follow-up) Add onpayment to sysprefs file This patch adds the required line to the circulation.pref file to dispaly the onpayment option in the MarkLostItemReturned preference Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 102109 [details] [review] Bug 24474: (follow-up) Add LOST_FOUND logic to Koha::Account::pay This patch adds the same updated logic as found in Koha::Account::Line->apply to handle LOST_FOUND credits being applied against their associated LOST debit. Currently a 'LOST_FOUND' credit is never created via the pay method and so the additional test will always pass, but the addition here accounts for possible future uses. Signed-off-by: David Nind <david@davidnind.com>
Created attachment 102110 [details] [review] Bug 24474: (QA follow-up) Fix failing test The test was assuming default data and would fail if you ran through the test plan (and disabled all but `onpayment` for the 'MarkLostItemsAsReturned' preference) on the data before running the included unit tests. Signed-off-by: David Nind <david@davidnind.com>
Created attachment 102132 [details] [review] Bug 24474: Add tests This does the following using the interface: - Unselect all the options from MarkLostItemsAsReturned except for 'onpayment' - Check an item out - Create a manual invoice LOST for the checked in item - Renew the item - Check that the item was not returned Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 102133 [details] [review] Bug 24474: Add tests This does the following using the interface: - Unselect all the options from MarkLostItemsAsReturned except for 'onpayment' - Check an item out - Create a manual invoice LOST for the checked in item - Renew the item - Check that the item was not returned Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 102134 [details] [review] Bug 24474: Add `onpayment` option to `MarkLostItemsReturned` Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 102135 [details] [review] Bug 24474: Filter out LOST_FOUND Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 102136 [details] [review] Bug 24474: (follow-up) Add onpayment to sysprefs file This patch adds the required line to the circulation.pref file to dispaly the onpayment option in the MarkLostItemReturned preference Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 102137 [details] [review] Bug 24474: (follow-up) Add LOST_FOUND logic to Koha::Account::pay This patch adds the same updated logic as found in Koha::Account::Line->apply to handle LOST_FOUND credits being applied against their associated LOST debit. Currently a 'LOST_FOUND' credit is never created via the pay method and so the additional test will always pass, but the addition here accounts for possible future uses. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 102138 [details] [review] Bug 24474: (QA follow-up) Fix failing test The test was assuming default data and would fail if you ran through the test plan (and disabled all but `onpayment` for the 'MarkLostItemsAsReturned' preference) on the data before running the included unit tests. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Nice work everyone! Pushed to master for 20.05
does not apply cleanly to 19.11.x please rebase if needed.
Also - dependencies lacking in 19.11.x - not backported