Summary: | Hold not removed when "trapped" item on hold shelf is checked out to a different patron in the holds queue | ||
---|---|---|---|
Product: | Koha | Reporter: | Chris Cormack <chris> |
Component: | Hold requests | Assignee: | Srdjan Jankovic <srdjan> |
Status: | CLOSED FIXED | QA Contact: | |
Severity: | critical | ||
Priority: | PATCH-Sent (DO NOT USE) | CC: | bchurch, cookie.wolfrom, mcoalwell, mjr, nengard, paul.poulain, smoreland, wizzyrea |
Version: | 3.6 | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | Sponsored | Patch complexity: | --- |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: | ||
Circulation function: | |||
Attachments: |
patch
bug_2830: Remove reserve when checking out if the borrower is not the first one in the reserve queue patch [Signed off] bug_2830: Remove reserve when checking out if the borrower is not the first one in the reserve queue |
Description
Chris Cormack
2010-05-21 00:56:55 UTC
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 |