From da99978777281452b16551ee10e7c86d1578337f Mon Sep 17 00:00:00 2001 From: Srdjan Jankovic Date: Thu, 13 Oct 2011 16:57:41 +1300 Subject: [PATCH] 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 Signed-off-by: Paul Poulain (cherry picked from commit 6e07fd7b004b5cb16ea0bec764993c1e9ffe153c) Signed-off-by: Chris Nighswonger --- C4/Circulation.pm | 42 ++-------------- C4/Items.pm | 2 +- C4/Reserves.pm | 67 +++++++++++++++++++++---- C4/Search.pm | 4 +- C4/XSLT.pm | 2 +- circ/circulation.pl | 2 +- opac/opac-user.pl | 2 +- t/db_dependent/Reserves.t | 6 +-- t/db_dependent/lib/KohaTest/Reserves.pm | 1 + 9 files changed, 72 insertions(+), 56 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 02e13c03a3..53d0b24f6c 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -318,7 +318,7 @@ sub transferbook { # find reserves..... # That'll save a database query. - my ( $resfound, $resrec ) = + my ( $resfound, $resrec, undef ) = CheckReserves( $itemnumber ); if ( $resfound and not $ignoreRs ) { $resrec->{'ResFound'} = $resfound; @@ -868,7 +868,7 @@ sub CanBookBeIssued { } # See if the item is on reserve. - my ( $restype, $res ) = C4::Reserves::CheckReserves( $item->{'itemnumber'} ); + my ( $restype, $res, undef ) = C4::Reserves::CheckReserves( $item->{'itemnumber'} ); if ($restype) { my $resbor = $res->{'borrowernumber'}; my ( $resborrower ) = C4::Members::GetMember( borrowernumber => $resbor ); @@ -982,39 +982,7 @@ sub AddIssue { ); } - # See if the item is on reserve. - my ( $restype, $res ) = - C4::Reserves::CheckReserves( $item->{'itemnumber'} ); - if ($restype) { - my $resbor = $res->{'borrowernumber'}; - if ( $resbor eq $borrower->{'borrowernumber'} ) { - # The item is reserved by the current patron - ModReserveFill($res); - } - elsif ( $restype eq "Waiting" ) { - # warn "Waiting"; - # The item is on reserve and waiting, but has been - # reserved by some other patron. - } - elsif ( $restype eq "Reserved" ) { - # warn "Reserved"; - # The item is reserved by someone else. - if ($cancelreserve) { # cancel reserves on this item - CancelReserve(0, $res->{'itemnumber'}, $res->{'borrowernumber'}); - } - } - if ($cancelreserve) { - CancelReserve($res->{'biblionumber'}, 0, $res->{'borrowernumber'}); - } - else { - # set waiting reserve to first in reserve queue as book isn't waiting now - ModReserve(1, - $res->{'biblionumber'}, - $res->{'borrowernumber'}, - $res->{'branchcode'} - ); - } - } + MoveReserve( $item->{'itemnumber'}, $borrower->{'borrowernumber'}, $cancelreserve ); # Starting process for transfer job (checking transfert and validate it if we have one) my ($datesent) = GetTransfers($item->{'itemnumber'}); @@ -1635,7 +1603,7 @@ sub AddReturn { # find reserves..... # if we don't have a reserve with the status W, we launch the Checkreserves routine - my ($resfound, $resrec) = C4::Reserves::CheckReserves( $item->{'itemnumber'} ); + my ($resfound, $resrec, undef) = C4::Reserves::CheckReserves( $item->{'itemnumber'} ); if ($resfound) { $resrec->{'ResFound'} = $resfound; $messages->{'ResFound'} = $resrec; @@ -2198,7 +2166,7 @@ sub CanBookBeRenewed { $error="too_many"; } - my ( $resfound, $resrec ) = C4::Reserves::CheckReserves($itemnumber); + my ( $resfound, $resrec, undef ) = C4::Reserves::CheckReserves($itemnumber); if ($resfound) { $renewokay = 0; $error="on_reserve" diff --git a/C4/Items.pm b/C4/Items.pm index 0756832232..8802a4c369 100644 --- a/C4/Items.pm +++ b/C4/Items.pm @@ -1229,7 +1229,7 @@ sub GetItemsInfo { $serial = 1; } if ( $datedue eq '' ) { - my ( $restype, $reserves ) = + my ( $restype, $reserves, undef ) = C4::Reserves::CheckReserves( $data->{'itemnumber'} ); # Previous conditional check with if ($restype) is not needed because a true # result for one item will result in subsequent items defaulting to this true diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 4af8a8584c..359bbad97b 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -109,6 +109,7 @@ BEGIN { &ModReserveStatus &ModReserveCancelAll &ModReserveMinusPriority + &MoveReserve &CheckReserves &CanBookBeReserved @@ -523,7 +524,7 @@ sub GetOtherReserves { my ($itemnumber) = @_; my $messages; my $nextreservinfo; - my ( $restype, $checkreserves ) = CheckReserves($itemnumber); + my ( undef, $checkreserves, undef ) = CheckReserves($itemnumber); if ($checkreserves) { my $iteminfo = GetItem($itemnumber); if ( $iteminfo->{'holdingbranch'} ne $checkreserves->{'branchcode'} ) { @@ -738,8 +739,8 @@ sub GetReserveStatus { =head2 CheckReserves - ($status, $reserve) = &CheckReserves($itemnumber); - ($status, $reserve) = &CheckReserves(undef, $barcode); + ($status, $reserve, $all_reserves) = &CheckReserves($itemnumber); + ($status, $reserve, $all_reserves) = &CheckReserves(undef, $barcode); Find a book in the reserves. @@ -804,11 +805,11 @@ sub CheckReserves { # note: we get the itemnumber because we might have started w/ just the barcode. Now we know for sure we have it. my ( $biblio, $bibitem, $notforloan_per_itemtype, $notforloan_per_item, $itemnumber ) = $sth->fetchrow_array; - return ( 0, 0 ) unless $itemnumber; # bail if we got nothing. + return ( '' ) unless $itemnumber; # bail if we got nothing. # if item is not for loan it cannot be reserved either..... # execpt where items.notforloan < 0 : This indicates the item is holdable. - return ( 0, 0 ) if ( $notforloan_per_item > 0 ) or $notforloan_per_itemtype; + return ( '' ) if ( $notforloan_per_item > 0 ) or $notforloan_per_itemtype; # Find this item in the reserves my @reserves = _Findgroupreserve( $bibitem, $biblio, $itemnumber ); @@ -822,7 +823,7 @@ sub CheckReserves { my $priority = 10000000; foreach my $res (@reserves) { if ( $res->{'itemnumber'} == $itemnumber && $res->{'priority'} == 0) { - return ( "Waiting", $res ); # Found it + return ( "Waiting", $res, \@reserves ); # Found it } else { # See if this item is more important than what we've got so far if ( $res->{'priority'} && $res->{'priority'} < $priority ) { @@ -843,11 +844,10 @@ sub CheckReserves { # We return the most important (i.e. next) reservation. if ($highest) { $highest->{'itemnumber'} = $item; - return ( "Reserved", $highest ); - } - else { - return ( 0, 0 ); + return ( "Reserved", $highest, \@reserves ); } + + return ( '' ); } =head2 CancelExpiredReserves @@ -1816,6 +1816,53 @@ sub _ShiftPriorityByDateAndPriority { return $new_priority; # so the caller knows what priority they wind up receiving } +=head2 MoveReserve + + MoveReserve( $itemnumber, $borrowernumber, $cancelreserve ) + +Use when checking out an item to handle reserves +If $cancelreserve boolean is set to true, it will remove existing reserve + +=cut + +sub MoveReserve { + my ( $itemnumber, $borrowernumber, $cancelreserve ) = @_; + + my ( $restype, $res, $all_reserves ) = CheckReserves( $itemnumber ); + return unless $res; + + my $biblionumber = $res->{biblionumber}; + my $biblioitemnumber = $res->{biblioitemnumber}; + + if ($res->{borrowernumber} == $borrowernumber) { + ModReserveFill($res); + } + else { + # warn "Reserved"; + # The item is reserved by someone else. + # Find this item in the reserves + + my $borr_res; + foreach (@$all_reserves) { + $_->{'borrowernumber'} == $borrowernumber or next; + $_->{'biblionumber'} == $biblionumber or next; + + $borr_res = $_; + last; + } + + if ( $borr_res ) { + # The item is reserved by the current patron + ModReserveFill($borr_res); + } + + if ($cancelreserve) { # cancel reserves on this item + CancelReserve(0, $res->{'itemnumber'}, $res->{'borrowernumber'}); + CancelReserve($res->{'biblionumber'}, 0, $res->{'borrowernumber'}); + } + } +} + =head2 MergeHolds MergeHolds($dbh,$to_biblio, $from_biblio); diff --git a/C4/Search.pm b/C4/Search.pm index fdb34790a3..4a8de5cbad 100644 --- a/C4/Search.pm +++ b/C4/Search.pm @@ -1673,7 +1673,7 @@ sub searchResults { my ($transfertfrom, $transfertto); # is item on the reserve shelf? - my $reservestatus = 0; + my $reservestatus = ''; my $reserveitem; unless ($item->{wthdrawn} @@ -1695,7 +1695,7 @@ sub searchResults { # should map transit status to record indexed in Zebra. # ($transfertwhen, $transfertfrom, $transfertto) = C4::Circulation::GetTransfers($item->{itemnumber}); - ($reservestatus, $reserveitem) = C4::Reserves::CheckReserves($item->{itemnumber}); + ($reservestatus, $reserveitem, undef) = C4::Reserves::CheckReserves($item->{itemnumber}); } # item is withdrawn, lost, damaged, not for loan, reserved or in transit diff --git a/C4/XSLT.pm b/C4/XSLT.pm index 8bc400057e..39071d5576 100755 --- a/C4/XSLT.pm +++ b/C4/XSLT.pm @@ -190,7 +190,7 @@ sub buildKohaItemsNamespace { my ( $transfertwhen, $transfertfrom, $transfertto ) = C4::Circulation::GetTransfers($item->{itemnumber}); - my ( $reservestatus, $reserveitem ) = C4::Reserves::CheckReserves($item->{itemnumber}); + my ( $reservestatus, $reserveitem, undef ) = C4::Reserves::CheckReserves($item->{itemnumber}); if ( $itemtypes->{ $item->{itype} }->{notforloan} || $item->{notforloan} || $item->{onloan} || $item->{wthdrawn} || $item->{itemlost} || $item->{damaged} || (defined $transfertwhen && $transfertwhen ne '') || $item->{itemnotforloan} || (defined $reservestatus && $reservestatus eq "Waiting") ){ diff --git a/circ/circulation.pl b/circ/circulation.pl index d8d41788a4..dbcb09beb3 100755 --- a/circ/circulation.pl +++ b/circ/circulation.pl @@ -430,7 +430,7 @@ sub build_issue_data { $it->{'borrowernumber'},$it->{'itemnumber'} ); $it->{"renew_error_${can_renew_error}"} = 1 if defined $can_renew_error; - my ( $restype, $reserves ) = CheckReserves( $it->{'itemnumber'} ); + my ( $restype, $reserves, undef ) = CheckReserves( $it->{'itemnumber'} ); $it->{'can_renew'} = $can_renew; $it->{'can_confirm'} = !$can_renew && !$restype; $it->{'renew_error'} = $restype; diff --git a/opac/opac-user.pl b/opac/opac-user.pl index ca18288b22..f728893b2d 100755 --- a/opac/opac-user.pl +++ b/opac/opac-user.pl @@ -141,7 +141,7 @@ my $canrenew = 0; if ($issues){ foreach my $issue ( sort { $b->{'date_due'} cmp $a->{'date_due'} } @$issues ) { # check for reserves - my ( $restype, $res ) = CheckReserves( $issue->{'itemnumber'} ); + my ( $restype, $res, undef ) = CheckReserves( $issue->{'itemnumber'} ); if ( $restype ) { $issue->{'reserved'} = 1; } diff --git a/t/db_dependent/Reserves.t b/t/db_dependent/Reserves.t index 177febb58a..ca4d42e9bf 100755 --- a/t/db_dependent/Reserves.t +++ b/t/db_dependent/Reserves.t @@ -51,12 +51,12 @@ AddReserve($branch, $borrowernumber, $biblionumber, $constraint, $bibitems, $priority, $notes, $title, $checkitem, $found); -my ($status, $reserve) = CheckReserves($itemnumber, $barcode); +my ($status, $reserve, $all_reserves) = CheckReserves($itemnumber, $barcode); ok($status eq "Reserved", "CheckReserves Test 1"); -($status, $reserve) = CheckReserves($itemnumber); +($status, $reserve, $all_reserves) = CheckReserves($itemnumber); ok($status eq "Reserved", "CheckReserves Test 2"); -($status, $reserve) = CheckReserves(undef, $barcode); +($status, $reserve, $all_reserves) = CheckReserves(undef, $barcode); ok($status eq "Reserved", "CheckReserves Test 3"); diff --git a/t/db_dependent/lib/KohaTest/Reserves.pm b/t/db_dependent/lib/KohaTest/Reserves.pm index 5317029c75..8b05dd0f9e 100644 --- a/t/db_dependent/lib/KohaTest/Reserves.pm +++ b/t/db_dependent/lib/KohaTest/Reserves.pm @@ -29,6 +29,7 @@ sub methods : Test( 1 ) { ModReserveAffect ModReserveCancelAll ModReserveMinusPriority + MoveReserve GetReserveInfo _FixPriority _Findgroupreserve -- 2.39.5