Bug 24413

Summary: MarkLostItemsAsReturned functionality does not lift restrictions caused by long overdues
Product: Koha Reporter: Jason Robb <jrobb>
Component: CirculationAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: aleisha, andrewfh, didier.gautheron, gmcharlt, jonathan.druart, kyle.m.hall, kyle, martin.renvoize, nick, victor
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.07, 19.05.12
Bug Depends on: 2720    
Bug Blocks:    
Attachments: Bug 24413: Apply AutoRemoveOverduesRestrictions for lost items
Bug 24413: Do not remove the restrictions from AddReturn
Bug 24413: Apply AutoRemoveOverduesRestrictions for lost items
Bug 24413: Do not remove the restrictions from AddReturn
Bug 24413: Apply AutoRemoveOverduesRestrictions for lost items
Bug 24413: Do not remove the restrictions from AddReturn
Bug 24413: Add tests
Bug 24413: Apply AutoRemoveOverduesRestrictions for lost items
Bug 24413: Do not remove the restrictions from AddReturn
Bug 24413: Add tests
Bug 24413: Apply AutoRemoveOverduesRestrictions for lost items
Bug 24413: Do not remove the restrictions from AddReturn
Bug 24413: Add tests
Bug 24413: (follow-up) Add tests

Description Jason Robb 2020-01-13 16:53:47 UTC
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
Comment 1 Jonathan Druart 2020-02-17 15:17:45 UTC
Confirmed on master.
Comment 2 Jonathan Druart 2020-02-17 15:31:16 UTC
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
Comment 3 Kyle M Hall 2020-02-18 12:44:04 UTC
(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?
Comment 4 Jonathan Druart 2020-02-18 13:23:36 UTC
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.
Comment 5 Kyle M Hall 2020-02-18 13:30:58 UTC
(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.
Comment 6 Jonathan Druart 2020-02-18 14:07:17 UTC
(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)?
Comment 7 Kyle M Hall 2020-02-18 14:45:33 UTC
(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.
Comment 8 Jonathan Druart 2020-02-21 11:41:44 UTC
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.
Comment 9 Jonathan Druart 2020-02-21 11:45:41 UTC
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).
Comment 10 Martin Renvoize 2020-04-08 08:23:06 UTC
Hi Jason, 

Are you able to test this using a sandbox perhaps?

Many thanks
Comment 11 Andrew Fuerste-Henry 2020-04-08 15:24:20 UTC
The second patch (Do not remove the restrictions from AddReturn) does not apply for me.
Comment 12 Jonathan Druart 2020-04-09 09:57:03 UTC
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
Comment 13 Jonathan Druart 2020-04-09 09:57:07 UTC
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).
Comment 14 Victor Grousset/tuxayo 2020-05-12 23:08:54 UTC
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)
Comment 15 Jonathan Druart 2020-05-13 13:19:29 UTC
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
Comment 16 Jonathan Druart 2020-05-13 13:19:33 UTC
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).
Comment 17 Jonathan Druart 2020-05-13 13:19:36 UTC
Created attachment 104845 [details] [review]
Bug 24413: Add tests
Comment 18 Jonathan Druart 2020-05-13 13:19:57 UTC
(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.
Comment 19 Victor Grousset/tuxayo 2020-05-14 02:42:49 UTC
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>
Comment 20 Victor Grousset/tuxayo 2020-05-14 02:42:53 UTC
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>
Comment 21 Victor Grousset/tuxayo 2020-05-14 02:42:57 UTC
Created attachment 104861 [details] [review]
Bug 24413: Add tests

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 22 Victor Grousset/tuxayo 2020-05-14 02:43:44 UTC
It works!

Test plan from here has been copied in the patch and completed.
Comment 23 Katrin Fischer 2020-05-21 14:30:15 UTC
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>
Comment 24 Katrin Fischer 2020-05-21 14:30:19 UTC
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>
Comment 25 Katrin Fischer 2020-05-21 14:30:24 UTC
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>
Comment 26 Martin Renvoize 2020-05-22 08:33:46 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 27 Didier Gautheron 2020-05-24 06:48:21 UTC
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
Comment 28 Jonathan Druart 2020-05-25 08:15:02 UTC
Created attachment 105323 [details] [review]
Bug 24413: (follow-up) Add tests

Add missing transaction.
Comment 29 Jonathan Druart 2020-05-25 08:17:01 UTC
Good catch, thanks Didier!
Comment 30 Jonathan Druart 2020-05-25 14:00:51 UTC
Last patch pushed to master for 20.05.
Comment 31 Aleisha Amohia 2020-06-11 23:46:12 UTC
backported to 19.11.x for 19.11.07
Comment 32 Victor Grousset/tuxayo 2020-06-12 19:18:15 UTC
Backported to 19.05.x branch for 19.05.12