---- 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
Created attachment 5469 [details] [review] patch
Created attachment 5479 [details] [review] 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. One comment, if a reserve that already has a waiting item is filled by an item with a different barcode, the original reserve is deleted. Cool. It might be nice to alert the librarian that there might be an item on the hold shelf. i.e. "I just deleted a reserve that was in waiting status, go get it off the hold shelf and send me to someone else or reshelve it" That's probably a different ENH bug though. :) Signed-off-by: Liz Rea <lrea@nekls.org> http://bugs.koha-community.org/show_bug.cgi?id=2380
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.
nope, the warning was not there before. I'll file another bug for the suggested slight enhancement. :)
Comment on attachment 5469 [details] [review] patch obsoleted by liz signoff
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.
Created attachment 5871 [details] [review] patch There's no inconsistency in return() - undef will be assigned to any missing value, and that is what is expected - if nothing is reserved then curent reserve record should be undefined as well as all reserves array. Also, I find it much more inconsistent to return 0 for strings and hashrefs, so I changed that. I've added an undef to CheckReserves() calls to indicate that we know the interface, but I'm not sure how useful is
I will try to walk through the signoff again tomorrow (10-26)
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>
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