Bug 20844 - Reset a hold when it is missing after allocation
Summary: Reset a hold when it is missing after allocation
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Aleisha Amohia
QA Contact: Martin Renvoize
URL:
Keywords: Manual
Depends on: 9834 21732 21754
Blocks:
  Show dependency treegraph
 
Reported: 2018-05-31 07:07 UTC by Hugh Rundle
Modified: 2019-06-14 17:53 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Who signed the patch off:
Text to go in the release notes:
This enhancement gives librarians easier control over waiting holds when items are marked as lost. When an item with a waiting item-level hold on it is marked as lost the librarian is given the option to retain the hold or to cancel it. When a lost item has an allocated waiting bib-level hold on it then if the 'LostBibLevelHoldsRevert' syspref is enabled then the allocated hold is reverted to an unallocated bib-level hold.
Version(s) released in:


Attachments
Bug 20844 - Revert hold when setting an item to lost (26.22 KB, patch)
2018-09-23 21:25 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 20844: Revert hold when setting an item to lost (26.22 KB, patch)
2018-10-17 21:30 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 20844: Fixed error when cancelling hold from cataloguing/additem.pl (2.71 KB, patch)
2018-10-17 21:31 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 20844: Fixed error when cancelling hold from cataloguing/additem.pl (2.71 KB, patch)
2018-10-19 00:39 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 20844: Added tests of the RevertWaitingStatus and RevertItemLevelHoldToBibLevelHold subroutines in C4/Reserves.pm (6.39 KB, patch)
2018-10-19 00:40 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 20844: Revert hold when setting an item to lost (29.41 KB, patch)
2018-10-24 01:57 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 20844: Revert hold when setting an item to lost (29.45 KB, patch)
2018-10-29 19:37 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 20844: Revert hold when setting an item to lost (29.50 KB, patch)
2018-10-30 14:38 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 20844: (follow-up) Fix style of dialogs (5.55 KB, patch)
2018-10-30 14:38 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 20844: (follow-up) Fix style of dialogs (5.64 KB, patch)
2018-10-30 21:29 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 20844: Revert hold when setting an item to lost (28.13 KB, patch)
2018-10-31 10:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20844: (follow-up) Fix style of dialogs (5.64 KB, patch)
2018-10-31 10:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20844: Revert hold when setting an item to lost (28.20 KB, patch)
2018-10-31 12:15 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20844: (follow-up) Fix style of dialogs (5.70 KB, patch)
2018-10-31 12:15 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20884: (QA follow-up) Remove Data::Dumper (1.04 KB, patch)
2018-10-31 12:15 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 Hugh Rundle 2018-05-31 07:07:48 UTC
There currently is no simple way to 'deallocate' a hold if it has gone missing after it is allocated to a patron but before they collect it.

This enhancement would work as follows:

1. item is allocated
2. item cannot be located when patron tries to collect it
3. staff member marks item as Lost
4. if hold was originally a bib-level hold, hold reverts from being specific to the missing item, back to a bib-level hold with priority 1.
5. if hold was originally item-level, staff member gets an alert asking whether to cancel or retain the hold
6. (optionally?) a 'lost hold' notice is queued to be sent to the patron

This bug is related to the original description of https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19287#c32 and also to https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15679

To do it properly, it may require a couple of new settings, something like:

LostBibLevelHoldsRevert
SendLostHoldNotices
Comment 1 Katrin Fischer 2018-05-31 15:23:41 UTC
Hi Hugh, 
so this woudl be to 'reset' a hold that has been waiting already? Does this happen often? It sounds like someone would be stealing from the waiting holds shelf.
Comment 2 Hugh Rundle 2018-05-31 22:50:58 UTC
(In reply to Katrin Fischer from comment #1)
> Hi Hugh, 
> so this woudl be to 'reset' a hold that has been waiting already? Does this
> happen often? It sounds like someone would be stealing from the waiting
> holds shelf.

Hi Katrin

Yes, this is specifically for holds that are waiting already. It could be missing because someone has stolen it (rare, but certainly happens sometimes in libraries that have open shelves for waiting holds), or it may simply have been mislaid by staff between returning it and putting it on a waiting holds shelf (possible in a busy library). It's certainly more common that may initially seem likely.

The other thing I've just realised would need to be either part of this enhancement, or paired with it perhaps as a different bug, is holds that go missing when they're in transit. i.e. the hold 'allocates' to a specific item when returned, but the pickup is at a different library. The notification won't be sent until it is returned at the pickup point, but it might go missing whilst in transit: in that case (which is more common) much the same process is needed, in that there needs to be a simple way to reverse the allocation since that item has gone missing, but there's probably no need to send a notification saying it got lost since there never would have been one saying it was ready to collect.
Comment 3 Alex Buckley 2018-09-23 21:25:34 UTC Comment hidden (obsolete)
Comment 4 Alex Buckley 2018-10-16 00:11:18 UTC
Any chance we can move this one along with testing?
Comment 5 Owen Leonard 2018-10-16 14:53:53 UTC
This seems to be working perfectly except for one thing: I can't cancel a hold from the add item screen. I get an error, Can't call method "cancel" on an undefined value at /home/vagrant/kohaclone/cataloguing/additem.pl line 741.

The patch does need to be fixed up according to QA tool warnings.
Comment 6 Alex Buckley 2018-10-17 21:30:34 UTC Comment hidden (obsolete)
Comment 7 Alex Buckley 2018-10-17 21:31:32 UTC Comment hidden (obsolete)
Comment 8 Alex Buckley 2018-10-17 21:33:44 UTC
Hi Owen

Thanks for testing. I have corrected the commit message in the first patch, and have just attached a second path which corrects the issue you saw trying to cancel a hold on the cataloguing/additem.pl interface. 

This second patch also corrects the qa warning on C4/Circulation.pm

Can you please test again?

Cheers,
Alex
Comment 9 Alex Buckley 2018-10-19 00:39:30 UTC Comment hidden (obsolete)
Comment 10 Alex Buckley 2018-10-19 00:40:00 UTC Comment hidden (obsolete)
Comment 11 Alex Buckley 2018-10-19 00:41:57 UTC
Hi all

I've added unit tests for the two new subroutines I added to C4/Reserves.pm which were RevertWaitingStatus() and RevertItemLevelHoldToBibLevelHold().

Also all three patches pass the QA test tool now.
Comment 12 Michal Denar 2018-10-19 06:25:10 UTC
Hi,

on kohadevbox I get:

kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove t/db_dependent/Holds/RevertWaitingStatus.t
t/db_dependent/Holds/RevertWaitingStatus.t .. DBD::mysql::st execute failed: Unknown column 'originalholdtype' in 'field list' [for Statement "INSERT INTO `reserves` ( `biblionumber`, `borrowernumber`, `branchcode`, `expirationdate`, `found`, `itemnumber`, `itemtype`, `originalholdtype`, `priority`, `reservedate`, `reservenotes`, `waitingdate`) VALUES ( ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ? )" with ParamValues: 0='439', 1='53', 2='YbGPVdI', 3=undef, 4=undef, 5=undef, 6=undef, 7='B', 8=undef, 9='2018-10-19', 10='', 11=undef] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1836.
Can't locate object method "rethrow" via package "DBD::mysql::st execute failed: Unknown column 'originalholdtype' in 'field list' [for Statement "INSERT INTO `reserves` ( `biblionumber`, `borrowernumber`, `branchcode`, `expirationdate`, `found`, `itemnumber`, `itemtype`, `originalholdtype`, `priority`, `reservedate`, `reservenotes`, `waitingdate`) VALUES ( ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ? )" with ParamValues: 0='439', 1='53', 2='YbGPVdI', 3=undef, 4=undef, 5=undef, 6=undef, 7='B', 8=undef, 9='2018-10-19', 10='', 11=undef] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1836.
" (perhaps you forgot to load "DBD::mysql::st execute failed: Unknown column 'originalholdtype' in 'field list' [for Statement "INSERT INTO `reserves` ( `biblionumber`, `borrowernumber`, `branchcode`, `expirationdate`, `found`, `itemnumber`, `itemtype`, `originalholdtype`, `priority`, `reservedate`, `reservenotes`, `waitingdate`) VALUES ( ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ? )" with ParamValues: 0='439', 1='53', 2='YbGPVdI', 3=undef, 4=undef, 5=undef, 6=undef, 7='B', 8=undef, 9='2018-10-19', 10='', 11=undef] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1836.
"?) at /home/vagrant/kohaclone/Koha/Object.pm line 148.
# Looks like your test exited with 11 before it could output anything.
t/db_dependent/Holds/RevertWaitingStatus.t .. Dubious, test returned 11 (wstat 2816, 0xb00)
Failed 12/12 subtests
Comment 13 Alex Buckley 2018-10-24 01:57:13 UTC Comment hidden (obsolete)
Comment 14 Alex Buckley 2018-10-24 02:16:53 UTC
Hi Michael

Thanks for testing. Can you please try testing now using the test plan in the patch I have just attached.
Comment 15 Alex Buckley 2018-10-24 02:17:52 UTC
Apologies that should be 'Hi Michal'
Comment 16 Alex Buckley 2018-10-28 21:54:46 UTC
With the feature freeze coming up very soon are we able to get this moving along with testing?
Comment 17 Alex Buckley 2018-10-29 19:37:28 UTC Comment hidden (obsolete)
Comment 18 Owen Leonard 2018-10-30 14:38:36 UTC Comment hidden (obsolete)
Comment 19 Owen Leonard 2018-10-30 14:38:39 UTC Comment hidden (obsolete)
Comment 20 Alex Buckley 2018-10-30 21:29:40 UTC Comment hidden (obsolete)
Comment 21 Alex Buckley 2018-10-30 21:38:23 UTC
Thanks Owen for testing my patch I have tested your follow-up and the dialogs look correct so I have signed off on it.
Comment 22 Martin Renvoize 2018-10-31 10:56:14 UTC Comment hidden (obsolete)
Comment 23 Martin Renvoize 2018-10-31 10:56:18 UTC Comment hidden (obsolete)
Comment 24 Martin Renvoize 2018-10-31 10:57:21 UTC
I've split this into two distinct bugs.. one for the enhancement details here and another for the included LostItem change which cleans up transfers as I count that as it's own bug.
Comment 25 Martin Renvoize 2018-10-31 12:15:51 UTC
Created attachment 81710 [details] [review]
Bug 20844: Revert hold when setting an item to lost

If an item is marked as 'Lost' then this patch introduces the following
new functionality:

1. Check if there is a bib-level or item-level hold on the item with the
status of 'W' (waiting). This is an allocated hold which is not yet
checked out to the requesting patron.

1.a. If the hold was originally a bib-level reserve (reserves.originalholdtype='B') then
do the following:

* Revert the waiting status of the reserve
* Set the reserves.itemnumber=NULL so the hold is reset to a bib-level
hold

1.b. If the hold was originally (and still is) a item-level hold
(reserves.originalholdtype='I') then
display a alert dialog box on the additem.pl interface asking the
librarian to select one of two options:

* Cancel the hold - This deletes the hold altogether
* Retain the hold - This does not change the hold. Therefore the
requester will still have a hold on a lost item

NOTE: Previously an allocated bib-level hold was identical to a item-level hold in the database.
As the first available item on the bib record had been allocated to the
requester and so an itemnumber had been set in the record in the reserves table.

However this patch introduces a atomicupdate which adds the new column
'originalholdtype' to the reserves table. This allows us to track if the
reserve was originally a bib level hold and treat it differently when
the allocated item is being marked as 'Lost'

Test plan:
1. Place a bib-level hold on a biblio
2. Check out an item from the biblio to a different patron
3. Query the reserves db table for biblio you placed the bib-level
hold and notice it has no 'itemnumber' or 'status' value
4. Return the item and confirm the hold in the modal box that is loaded
5. Query the reserves db table and notice the itemnumber field is now
filled with the returned item's itemnumber and the status is 'W'
6. Set the item to 'Lost' either by clicking on Edit->Edit items from
the detail.pl page
OR
clicking on the Items tab on the left side of the detail.pl page
7. Notice the waiting item-level hold still exists after the item is
marked as 'lost'

8. Repeat steps 1-7 but this time place an item-level hold on an item in
step 1 and then check that same item out in step 2. Notice in the
repeated step 7 that the hold remains after marking the item as 'Lost'
9. Apply patch and run ./updatedatabase.pl from the koha-shell in the
installer/data/mysql/ directory

9. Repeat steps 1-6 placing an bib-level hold and then query the
database and notice the bib-level hold which was changed to an allocated
waiting item-level hold when the item was returned and the hold
confirmed is now reverted to a bib-level hold again. This is controlled
by the LostBibLevelHoldsRevert syspref.

If this syspref is not enabled then the allocated item-level (originally
bib-level hold) will remain after the item is marked as lost.

10. Repeat 1-6 but this time place an item-level hold on the item.
Notice when you mark the item as 'Lost' a yellow warning box is
displayed asking if you want to 'Retain hold' or 'Cancel hold'. Select
'Retain hold' and notice the item-level hold remains now the item is marked as
lost.

11. Repeat step 10 but choose 'Cancel hold' option and notice the hold
is deleted now.

12. Now enable the new 'LostItemCancelOutstandingTransfers' syspref.
This will now cancel any outstanding transfers on the item when it is
marked as lost.

13. Find a item which is in transfer, i.e. find an item with the text in
the 'Status' field of the table in detail.pl that indicates it is in
transfer

14. Now mark that item as 'Lost' and now notice the transfer is
cancelled

15. Go into koha-shell from the Koha home dir:
sudo koha-shell <instancename>

16. Run the RevertWaitingStatus.t test file:
prove t/db_dependent/Holds/RevertWaitingStatus.t

Sponsored-By: Brimbank Library, Australia

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 26 Martin Renvoize 2018-10-31 12:15:55 UTC
Created attachment 81711 [details] [review]
Bug 20844: (follow-up) Fix style of dialogs

This patch modifies the add item and item detail templates so that
the hold cancellation dialogs match other similar dialogs in Koha.

To test, apply the patch and perform step 10 or the original test plan.
Confirm that the confirmation dialogs look correct.

Confirmation dialogs look correct
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 27 Martin Renvoize 2018-10-31 12:15:59 UTC
Created attachment 81712 [details] [review]
Bug 20884: (QA follow-up) Remove Data::Dumper

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 28 Martin Renvoize 2018-10-31 12:17:38 UTC
I'm afraid I'm not confident enough thought has been given to this regarding all the different ways an item can be marked as lost.

/tools/batchMod.pl, circ/pendingreserves.pl and mist/cronjobs/longoverdue.pl all call LostItem and would need some level of handling for the new workflow which require user input.
Comment 29 Alex Buckley 2018-11-01 11:54:16 UTC
Hi Martin

Thanks for your feedback.

I have tested and looked through the code calls in /circ/pendingreserves.pl, /tools/batchMod.pl and misc/cronjobs/longoverdue.pl

I have noticed they are not consistent in their current handling of the reserves on lost items. 

* /circ/pendingreserves.pl - This cancels non-waiting item level reserves when item is marked as lost. Doesn't show allocated (waiting) holds.

* /tools/batchMod.pl - Doesn’t cancel item-level or allocated bib-level reserves.

* /misc/cronjob/longoverdue.pl - Doesn't cancel non-waiting (un-allocated) reserves

It would be good to have a consistent approach throughout Koha for dealing with reserves on lost items. 

A question do you think it would be best to split this patch up so it is only dealing with allocated (waiting) holds. This would alter the behavior of the patches on the bug report which check for reserve type of 'waiting' or 'reserved'. 

Then we would only need to add reserve handling to /tools/batchMod.pl as longoverdue.pl and pendingreserves.pl don't touch waiting reserves. 

Amending longoverdue.pl and pendingreserves.pl could go in another bug report building on this to handle un-allocated holds, just to prevent too much scope creep on this bug report?

Cheers,
Alex
Comment 30 Aleisha Amohia 2019-02-20 23:17:42 UTC
(In reply to Alex Buckley from comment #29)
> 
> It would be good to have a consistent approach throughout Koha for dealing
> with reserves on lost items. 
> 
> A question do you think it would be best to split this patch up so it is
> only dealing with allocated (waiting) holds. This would alter the behavior
> of the patches on the bug report which check for reserve type of 'waiting'
> or 'reserved'. 
> 
> Then we would only need to add reserve handling to /tools/batchMod.pl as
> longoverdue.pl and pendingreserves.pl don't touch waiting reserves. 
> 
> Amending longoverdue.pl and pendingreserves.pl could go in another bug
> report building on this to handle un-allocated holds, just to prevent too
> much scope creep on this bug report?
> 

It would be great to have some feedback on this so we know what the next steps are.
Comment 31 Nick Clemens 2019-06-14 17:53:29 UTC
9834 allows for manual reversion of hold to take original state of hold (item level vs next available) 

This should be adapted to use that code, or maybe that fix satisfies the requirements here?