Bug 2830 - Hold not removed when "trapped" item on hold shelf is checked out to a different patron in the holds queue
Summary: Hold not removed when "trapped" item on hold shelf is checked out to a differ...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: 3.6
Hardware: All All
: PATCH-Sent (DO NOT USE) critical (vote)
Target Milestone: Oldversions
Assignee: Srdjan Jankovic
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-11-28 08:20 UTC by Chris Cormack
Modified: 2018-12-16 19:42 UTC (History)
9 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
patch (5.58 KB, patch)
2011-09-20 03:58 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_2830: Remove reserve when checking out if the borrower is not the first one in the reserve queue (6.81 KB, patch)
2011-09-20 15:54 UTC, Liz Rea
Details | Diff | Splinter Review
patch (12.74 KB, patch)
2011-10-13 04:10 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
[Signed off] bug_2830: Remove reserve when checking out if the borrower is not the first one in the reserve queue (13.54 KB, patch)
2011-11-17 16:57 UTC, Liz Rea
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Cormack 2010-05-21 00:56:55 UTC


---- Reported by bchurch@ptfs.com 2008-11-28 08:20:15 ----

Although it is not normal procedure, "trapped" items waiting on the hold are sometimes checked out to a different patron in the holds queue. For example, a "trapped" item is waiting on the hold shelf for Patron A. If this item is checked out to Patron C who has hold priority #2, the hold for Patron C is not removed from the holds queue. The hold for Patron C should be considered as filled and removed from the holds queue.

Here are the steps for recreating this problem:
1. Place three holds on a title.
2. Check out an item to Patron A who has priority #1 so it is "trapped" for Patron A. The hold status is "waiting".
3. Check out this "trapped" item to Patron C, who now has the #2 priority hold in the holds queue.
4. Ignore the system message that the item is being held for Patron A and check out the item to Patron C.
5. The hold for Patron A is re-inserted as priority #1 in the holds queue, but the hold for Patron C is not removed from holds queue.



---- Additional Comments From bchurch@ptfs.com 2008-12-01 16:13:53 ----

Step 2 Should read "Check in" not "check out" an item.



---- Additional Comments From bchurch@ptfs.com 2008-12-02 09:38:09 ----

When the hold for Patron A is re-inserted in the holds queue, the hold becomes a "copy specific" hold instead of an "any copy" hold. If you look at the normal view for the title in the staff client, the item appears as being both checked out to Patron C and on hold for Patron A.



---- Additional Comments From joe.atzberger@liblime.com 2009-01-27 11:26:20 ----

Increased severity since this compromises holds as a feature.  Koha presents the availability of adjusting queue priority, without any warning of the negative behavior resulting from attempting it.



--- Bug imported by chris@bigballofwax.co.nz 2010-05-21 00:56 UTC  ---

This bug was previously known as _bug_ 2830 at http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=2830

Actual time not defined. Setting to 0.0
The original reporter of this bug does not have
   an account here. Reassigning to the person who moved
   it here: chris@bigballofwax.co.nz.
   Previous reporter was bchurch@ptfs.com.
CC member 1goodlibrarian@gmail.com does not have an account here
CC member adrea@moablibrary.org does not have an account here
CC member arm@hanover.ca does not have an account here
CC member mjr@ttllp.co.uk does not have an account here

Comment 1 Srdjan Jankovic 2011-09-20 03:58:52 UTC Comment hidden (obsolete)
Comment 2 Liz Rea 2011-09-20 15:54:12 UTC Comment hidden (obsolete)
Comment 3 Srdjan Jankovic 2011-09-21 00:12:41 UTC
If the warning was there before then I'll have to reinstate it. Otherwise a new bug is preferred because this one has been tested, and if I make other changes we would need to retest it.
Comment 4 Liz Rea 2011-09-26 14:22:43 UTC
nope, the warning was not there before. I'll file another bug for the suggested slight enhancement. :)
Comment 5 Paul Poulain 2011-10-12 09:37:58 UTC
Comment on attachment 5469 [details] [review]
patch

obsoleted by liz signoff
Comment 6 Paul Poulain 2011-10-12 09:51:51 UTC
QA comment:
you've updated the CheckReserves sub and added a returned parameter. Fair.

But :
* there are some places where you don't return the new parameter:
    return ( 0, 0 ) unless $itemnumber; # bail if we got nothing.
 for example
* there are places where you don't retrieve the parameter:
C4/Circulation.pm:    my ( $restype, $res ) = C4::Reserves::CheckReserves( $item->{'itemnumber'} );

Even if the new parameters is useless in this context, it must be returned for consistency.

I also see in Circulation.pm that one can now read:
# See if the item is on reserve.
MoveReserve( $item->{'itemnumber'}, $borrower->{'borrowernumber'}, $cancelreserve );
The comment is not really related to the function name. Either the function name is wrong, or the comment is wrong ("see if the item is on reserve & mark it filled if it's the case", for example)

Marking failed QA.
Comment 7 Srdjan Jankovic 2011-10-13 04:10:39 UTC Comment hidden (obsolete)
Comment 8 Liz Rea 2011-10-25 21:17:29 UTC
I will try to walk through the signoff again tomorrow (10-26)
Comment 9 Liz Rea 2011-11-17 16:57:54 UTC
Created attachment 6329 [details] [review]
[Signed off] bug_2830: Remove reserve when checking out if the borrower is not the first one in the reserve queue

To test:
Create 4 holds on a bib, for patrons A, B, C, and D,

Check in the item to mark hold as waiting for patron A
Check out the item to patron B -> reserve for patron B should be removed
Check in the item to mark hold as waiting for patron A
Check out the item to Patron A, hold should complete normally
Check in the item to mark hold as waiting for patron C
Check out the item to patron D -> reserve for patron D should be removed.
Check in the item to mark hold as waiting for patron C
Check out the item to patron C, hold should complete normally
Check in the item -> there should be no more reserves.

We also tested:
Created 4 holds on a bib with two items, for patrons A, B, C, and D

All worked as expected.

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 10 Paul Poulain 2011-12-16 16:42:45 UTC
QA comment:
prove t/00-testcritic.t 
t/00-testcritic.t .. ok       

changes the API of checkReserves and adds a sub MoveReserve that could be usefull in other circumstances, it's a good new sub

I also made some tests with basic holds, and couldn't spot any difference.

prove t/00-testcritic.t 
t/00-testcritic.t .. ok       

passed QA

patch pushed, please test