If an account has been restricted by the long overdue process, and the item causing the restriction is returned by marking it lost (via MarkLostItemsAsReturned), the restriction is not lifted despite 'AutoRemoveOverduesRestriction' being marked to 'Do'. To replicate: 1. Set MarkLostItemsAsReturned to 'from items tab of the catalog module' 2. Set AutoRemoveOverduesRestrictions to 'Do' 3. Set up an overdues restriction in the notice triggers 4. Check out an item and let the overdues process restrict the account 5. Navigate to the moredetail.pl page (items tab) for the overdue item 6. Mark the item lost 7. Return to the account in question - notice the item has been returned, but the restriction remains
Confirmed on master.
Created attachment 99111 [details] [review] Bug 24413: Apply AutoRemoveOverduesRestrictions for lost items It's quite hard to know where this need to be fixed. it can be either MarkIssueReturned or LostItem, depending on the different cases we want to handle. This patch picked MarkIssueReturned, but maybe the similar code in AddReturn needs to be removed then. This patch fixes the original issue report by bug 24413, but is submitted for discussion
(In reply to Jonathan Druart from comment #2) > Created attachment 99111 [details] [review] [review] > Bug 24413: Apply AutoRemoveOverduesRestrictions for lost items > > It's quite hard to know where this need to be fixed. > it can be either MarkIssueReturned or LostItem, depending on the > different cases we want to handle. > > This patch picked MarkIssueReturned, but maybe the similar code in > AddReturn needs to be removed then. > > This patch fixes the original issue report by bug 24413, but is > submitted for discussion Considering MarkIssueReturned and LostItem are not always called, should it not be in _FixAccountForLostAndFound?
It seems that _FixAccountForLostAndFound is called from AddIssue and AddReturn. In our case none of them is called so I am pretty sure it will not fix the issue.
(In reply to Jonathan Druart from comment #4) > It seems that _FixAccountForLostAndFound is called from AddIssue and > AddReturn. In our case none of them is called so I am pretty sure it will > not fix the issue. I see now, this code looks good from that perspective, though it would definitely be preferable to add a new subroutine now, otherwise we have duplicated code. It seems like a fairly trivial adjustment.
(In reply to Kyle M Hall from comment #5) > (In reply to Jonathan Druart from comment #4) > > It seems that _FixAccountForLostAndFound is called from AddIssue and > > AddReturn. In our case none of them is called so I am pretty sure it will > > not fix the issue. > > I see now, this code looks good from that perspective, though it would > definitely be preferable to add a new subroutine now, otherwise we have > duplicated code. It seems like a fairly trivial adjustment. Do not you think we should remove the code from AddReturn (as it calls MarkIssueReturned)?
(In reply to Jonathan Druart from comment #6) > (In reply to Kyle M Hall from comment #5) > > (In reply to Jonathan Druart from comment #4) > > > It seems that _FixAccountForLostAndFound is called from AddIssue and > > > AddReturn. In our case none of them is called so I am pretty sure it will > > > not fix the issue. > > > > I see now, this code looks good from that perspective, though it would > > definitely be preferable to add a new subroutine now, otherwise we have > > duplicated code. It seems like a fairly trivial adjustment. > > Do not you think we should remove the code from AddReturn (as it calls > MarkIssueReturned)? Yes, that makes sense. It's also worth noting that the code block is idempotent so it's not going to hurt to run it multiple times. It should just be kept in one place for code. maintenance purposes.
This change makes sense to me, but I am not able to provide a test plan, as it is hard to predict the side-effects. I would like Jason to test this patch, and QA to make a full review and try to anticipate potential regressions. 2 pairs of QA eyes would be great here.
Created attachment 99369 [details] [review] Bug 24413: Do not remove the restrictions from AddReturn As we are now removing them from MarkIssueReturned they should not be removed from AddReturn as well. Also I think this will fix a regression, if $doreturn is not set (in case the item is withdrawn and BlockReturnOfWithdrawnItems or the item is lost and BlockReturnOfLostItems, and other specific cases).
Hi Jason, Are you able to test this using a sandbox perhaps? Many thanks
The second patch (Do not remove the restrictions from AddReturn) does not apply for me.
Created attachment 102606 [details] [review] Bug 24413: Apply AutoRemoveOverduesRestrictions for lost items It's quite hard to know where this need to be fixed. it can be either MarkIssueReturned or LostItem, depending on the different cases we want to handle. This patch picked MarkIssueReturned, but maybe the similar code in AddReturn needs to be removed then. This patch fixes the original issue report by bug 24413, but is submitted for discussion
Created attachment 102607 [details] [review] Bug 24413: Do not remove the restrictions from AddReturn As we are now removing them from MarkIssueReturned they should not be removed from AddReturn as well. Also I think this will fix a regression, if $doreturn is not set (in case the item is withdrawn and BlockReturnOfWithdrawnItems or the item is lost and BlockReturnOfLostItems, and other specific cases).
I was able to reproduce the issue. But it still happened after applying the patch. Double checked the syspref. Item is lost, the checkout isn't here anymore. The restriction remains. (it was tested in a koha-testing-docker env and restart_all was done)
Created attachment 104843 [details] [review] Bug 24413: Apply AutoRemoveOverduesRestrictions for lost items It's quite hard to know where this need to be fixed. it can be either MarkIssueReturned or LostItem, depending on the different cases we want to handle. This patch picked MarkIssueReturned, but maybe the similar code in AddReturn needs to be removed then. This patch fixes the original issue report by bug 24413, but is submitted for discussion
Created attachment 104844 [details] [review] Bug 24413: Do not remove the restrictions from AddReturn As we are now removing them from MarkIssueReturned they should not be removed from AddReturn as well. Also I think this will fix a regression, if $doreturn is not set (in case the item is withdrawn and BlockReturnOfWithdrawnItems or the item is lost and BlockReturnOfLostItems, and other specific cases).
Created attachment 104845 [details] [review] Bug 24413: Add tests
(In reply to Victor Grousset/tuxayo from comment #14) > I was able to reproduce the issue. > But it still happened after applying the patch. > Double checked the syspref. Item is lost, the checkout isn't here anymore. > The restriction remains. > > (it was tested in a koha-testing-docker env and restart_all was done) Indeed, should be fixed now.
Created attachment 104859 [details] [review] Bug 24413: Apply AutoRemoveOverduesRestrictions for lost items It's quite hard to know where this need to be fixed. it can be either MarkIssueReturned or LostItem, depending on the different cases we want to handle. This patch picked MarkIssueReturned, but maybe the similar code in AddReturn needs to be removed then. == Test plan == 1. Set MarkLostItemsAsReturned to 'from items tab of the catalog module' 2. Set AutoRemoveOverduesRestrictions to 'Do' 3. Set up an overdues restriction in the notice triggers 4. Check out an item and let the overdues process restrict the account 5. Navigate to the moredetail.pl page (items tab) for the overdue item 6. Mark the item lost 7. Return to the account in question - notice the item has been returned, but the restriction remains 8. Clean state: remove restriction + remove item lost status 9. Apply patch 10. Redo the test but this time in addition to the item being returned, the restriction will be lifted. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 104860 [details] [review] Bug 24413: Do not remove the restrictions from AddReturn As we are now removing them from MarkIssueReturned they should not be removed from AddReturn as well. Also I think this will fix a regression, if $doreturn is not set (in case the item is withdrawn and BlockReturnOfWithdrawnItems or the item is lost and BlockReturnOfLostItems, and other specific cases). Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 104861 [details] [review] Bug 24413: Add tests Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
It works! Test plan from here has been copied in the patch and completed.
Created attachment 105205 [details] [review] Bug 24413: Apply AutoRemoveOverduesRestrictions for lost items It's quite hard to know where this need to be fixed. it can be either MarkIssueReturned or LostItem, depending on the different cases we want to handle. This patch picked MarkIssueReturned, but maybe the similar code in AddReturn needs to be removed then. == Test plan == 1. Set MarkLostItemsAsReturned to 'from items tab of the catalog module' 2. Set AutoRemoveOverduesRestrictions to 'Do' 3. Set up an overdues restriction in the notice triggers 4. Check out an item and let the overdues process restrict the account 5. Navigate to the moredetail.pl page (items tab) for the overdue item 6. Mark the item lost 7. Return to the account in question - notice the item has been returned, but the restriction remains 8. Clean state: remove restriction + remove item lost status 9. Apply patch 10. Redo the test but this time in addition to the item being returned, the restriction will be lifted. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 105206 [details] [review] Bug 24413: Do not remove the restrictions from AddReturn As we are now removing them from MarkIssueReturned they should not be removed from AddReturn as well. Also I think this will fix a regression, if $doreturn is not set (in case the item is withdrawn and BlockReturnOfWithdrawnItems or the item is lost and BlockReturnOfLostItems, and other specific cases). Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 105207 [details] [review] Bug 24413: Add tests Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Nice work everyone! Pushed to master for 20.05
Hi subtest 'AutoRemoveOverduesRestrictions' doesn't properly cleanup issues and borrowers tables, issues table is an issue:) it breaks Koha/Biblios.t On today master prove t/db_dependent/Circulation/MarkIssueReturned.t t/db_dependent/Circulation/MarkIssueReturned.t .. ok All tests successful. Files=1, Tests=4, 4 wallclock secs ( 0.02 usr 0.01 sys + 3.44 cusr 0.59 csys = 4.06 CPU) Result: PASS select * from issues; +----------+----------------+------------+---------------------+------------+------------+-----------------+----------+------------+------------------+---------------------+---------------------+-----------------+------+----------+----------+ | issue_id | borrowernumber | itemnumber | date_due | branchcode | returndate | lastreneweddate | renewals | auto_renew | auto_renew_error | timestamp | issuedate | onsite_checkout | note | notedate | noteseen | +----------+----------------+------------+---------------------+------------+------------+-----------------+----------+------------+------------------+---------------------+---------------------+-----------------+------+----------+----------+ | 653 | 2000000449 | 2402 | 2020-05-29 23:59:00 | mF9sTiXrX | NULL | NULL | 0 | 0 | NULL | 2020-05-24 06:45:13 | 2020-05-24 06:45:13 | 0 | NULL | NULL | NULL | +----------+----------------+------------+---------------------+------------+------------+-----------------+----------+------------+------------------+---------------------+---------------------+-----------------+------+----------+----------+ 1 row in set (0.00 sec) prove t/db_dependent/Koha/Biblio.t t/db_dependent/Koha/Biblio.t .. 4/12 # No tests run! # Failed test 'No tests run for subtest "pickup_locations"' # at t/db_dependent/Koha/Biblio.t line 415. DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Cannot delete or update a parent row: a foreign key constraint fails (`koha_kohadev`.`issues`, CONSTRAINT `issues_ibfk_1` FOREIGN KEY (`borrowernumber`) REFERENCES `borrowers` (`borrowernumber`) ON UPDATE CASCADE) [for Statement "DELETE FROM `borrowers` WHERE ( `borrowernumber` = ? )" with ParamValues: 0=2000000449] at /kohadevbox/koha/Koha/Object.pm line 229 # Looks like your test exited with 255 just after 8. t/db_dependent/Koha/Biblio.t .. Dubious, test returned 255 (wstat 65280, 0xff00) Failed 5/12 subtests Test Summary Report ------------------- t/db_dependent/Koha/Biblio.t (Wstat: 65280 Tests: 8 Failed: 1) Failed test: 8 Non-zero exit status: 255 Parse errors: Bad plan. You planned 12 tests but ran 8. Files=1, Tests=8, 2 wallclock secs ( 0.02 usr 0.01 sys + 1.97 cusr 0.20 csys = 2.20 CPU) Result: FAIL mysql> delete from issues; prove t/db_dependent/Koha/Biblio.t t/db_dependent/Koha/Biblio.t .. ok All tests successful. Files=1, Tests=12, 12 wallclock secs ( 0.03 usr 0.00 sys + 8.40 cusr 1.55 csys = 9.98 CPU) Result: PASS
Created attachment 105323 [details] [review] Bug 24413: (follow-up) Add tests Add missing transaction.
Good catch, thanks Didier!
Last patch pushed to master for 20.05.
backported to 19.11.x for 19.11.07
Backported to 19.05.x branch for 19.05.12