Bugzilla – Attachment 6329 Details for
Bug 2830
Hold not removed when "trapped" item on hold shelf is checked out to a different patron in the holds queue
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
[Signed off] bug_2830: Remove reserve when checking out if the borrower is not the first one in the reserve queue
Signed-off-bug2830-Remove-reserve-when-checking-ou.patch (text/plain), 13.54 KB, created by
Liz Rea
on 2011-11-17 16:57:54 UTC
(
hide
)
Description:
[Signed off] bug_2830: Remove reserve when checking out if the borrower is not the first one in the reserve queue
Filename:
MIME Type:
Creator:
Liz Rea
Created:
2011-11-17 16:57:54 UTC
Size:
13.54 KB
patch
obsolete
>From 8002b8fa053e7cdd17a48fec868aeaa1a70bfdde Mon Sep 17 00:00:00 2001 >From: Srdjan Jankovic <srdjan@catalyst.net.nz> >Date: Thu, 13 Oct 2011 16:57:41 +1300 >Subject: [PATCH] [Signed off] bug_2830: Remove reserve when checking out if the borrower is not the first one in the reserve queue >Content-Type: text/plain; charset="utf-8" > >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> >--- > 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 9f81773..12f0898 100644 >--- a/C4/Circulation.pm >+++ b/C4/Circulation.pm >@@ -319,7 +319,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; >@@ -869,7 +869,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 ); >@@ -983,39 +983,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'}); >@@ -1636,7 +1604,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; >@@ -2199,7 +2167,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 0b91b65..6429251 100644 >--- a/C4/Items.pm >+++ b/C4/Items.pm >@@ -1228,7 +1228,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 4af8a85..3259a68 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 98db887..428cb26 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 8bc4000..39071d5 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 efb87da..5c239ce 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 03c9e81..6952a85 100755 >--- a/opac/opac-user.pl >+++ b/opac/opac-user.pl >@@ -139,7 +139,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 177febb..ca4d42e 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 5317029..8b05dd0 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 >-- >1.7.2.5
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 2830
:
5469
|
5479
|
5871
| 6329