From 52437c9df1ae208e67dca440db19f7cde0231d3b Mon Sep 17 00:00:00 2001 From: Olli-Antti Kivilahti Date: Tue, 15 Oct 2013 13:35:01 +0300 Subject: [PATCH] Bug 10993 - Transfer limits should be checked when placing a hold/reservation in OPAC. Depends on bug 11005, 11020 Prevents a user from placing holds on material whose branch transfer is denied. Displays clear messages that items are unavailable and re-checks in the business layer in case of UI blocks failing. Enables choosing another pickup location. Manual tests included. --- .../opac-tmpl/prog/en/modules/opac-reserve.tt | 77 ++++++++++++------ opac/opac-reserve.pl | 91 ++++++++++++++-------- 2 files changed, 115 insertions(+), 53 deletions(-) diff --git a/koha-tmpl/opac-tmpl/prog/en/modules/opac-reserve.tt b/koha-tmpl/opac-tmpl/prog/en/modules/opac-reserve.tt index bf334fa..5e656af 100644 --- a/koha-tmpl/opac-tmpl/prog/en/modules/opac-reserve.tt +++ b/koha-tmpl/opac-tmpl/prog/en/modules/opac-reserve.tt @@ -5,6 +5,7 @@ @@ -270,28 +300,29 @@
ERROR: Internal error: incomplete hold request.
[% END %] [% ELSE %] - [% IF ( none_available ) %] + [% IF ( none_available ) %]
Sorry, none of these items can be placed on hold.
- [% END %] + [% END %] [% END %] - [% UNLESS ( message ) %][% UNLESS ( none_available ) %]

Confirm holds for: - [% FOREACH USER_INF IN USER_INFO %] - [% USER_INF.firstname %] [% USER_INF.surname %] ([% USER_INF.cardnumber %]) - [% END %] -

[% END %] - [% IF (RESERVE_CHARGE) %] -
- There is a charge of [% RESERVE_CHARGE %] for placing this hold -
- [% END %] + [% UNLESS ( message ) %][% IF ( 1 ) %] +

Confirm holds for: + [% FOREACH USER_INF IN USER_INFO %] + [% USER_INF.firstname %] [% USER_INF.surname %] ([% USER_INF.cardnumber %]) + [% END %] +

[% END %] + [% IF (RESERVE_CHARGE) %] +
+ There is a charge of [% RESERVE_CHARGE %] for placing this hold +
+ [% END %]
- - + +
@@ -314,7 +345,7 @@ [% ELSE %] [% END %] - [% bibitemloo.title |html %][% IF ( bibitemloo.subtitle ) %] [% FOREACH subtitl IN bibitemloo.subtitle %][% subtitl.subfield %][% END %][% END %] + [% bibitemloo.title |html %][% IF ( bibitemloo.subtitle ) %] [% FOREACH subtitl IN bibitemloo.subtitle %][% subtitl.subfield %][% END %][% END %] [% IF ( bibitemloo.author ) %], by [% bibitemloo.author %][% END %]

@@ -359,6 +390,10 @@ [% UNLESS ( singleBranchMode ) %] + [% IF ( bibitemloo.suggestAnotherPickupLocation ) %] +
None of the available items can be transferred to your pickup location. You could try another pickup location. +
+ [% END %] [% IF ( bibitemloo.holdable ) %] [% IF ( choose_branch ) %]
  • @@ -398,8 +433,8 @@ [% INCLUDE 'date-format.inc' %]
    • [% END %] - - [% END %] + [% END %][%#End of IF ( bibitemloo.holdable ) %] + [% IF ( bibitemloo.holdable ) %]
  • @@ -440,8 +475,8 @@ checked="checked" /> [% END %] - + [% UNLESS ( bibitemloo.holdable ) %] [% UNLESS ( message ) %] - [% UNLESS ( none_available ) %] [% END %] - [% END %]
    • diff --git a/opac/opac-reserve.pl b/opac/opac-reserve.pl index 4bba9a6..1183347 100755 --- a/opac/opac-reserve.pl +++ b/opac/opac-reserve.pl @@ -91,7 +91,8 @@ if ((! $biblionumbers) && (! $query->param('place_reserve'))) { } # Pass the numbers to the page so they can be fed back -# when the hold is confirmed. TODO: Not necessary? +# when the hold is confirmed. Biblionumbers are also needed if the +# pickup branch changes and the page needs to be reloaded. $template->param( biblionumbers => $biblionumbers ); # Each biblio number is suffixed with '/', e.g. "1/2/3/" @@ -103,12 +104,12 @@ if (($#biblionumbers < 0) && (! $query->param('place_reserve'))) { } # pass the pickup branch along.... -my $branch = $query->param('branch') || $borr->{'branchcode'} || C4::Context->userenv->{branch} || '' ; -($branches->{$branch}) or $branch = ""; # Confirm branch is real -$template->param( branch => $branch ); +my $pickupBranch = $query->param('branch') || $borr->{'branchcode'} || C4::Context->userenv->{branch} || '' ; +($branches->{$pickupBranch}) or $pickupBranch = ""; # Confirm branch is real +$template->param( branch => $pickupBranch ); # make branch selection options... -my $branchloop = GetBranchesLoop($branch); +my $branchloop = GetBranchesLoop($pickupBranch); # Is the person allowed to choose their branch my $OPACChooseBranch = (C4::Context->preference("OPACAllowUserToChooseBranch")) ? 1 : 0; @@ -202,19 +203,19 @@ if ( $query->param('place_reserve') ) { } while (@selectedItems) { - my $biblioNum = shift(@selectedItems); - my $itemNum = shift(@selectedItems); - my $branch = shift(@selectedItems); # i.e., branch code, not name + my $biblioNum = shift(@selectedItems); + my $itemNum = shift(@selectedItems); + my $pickupLocation = shift(@selectedItems); # i.e., branch code, not name, my $canreserve = 0; my $singleBranchMode = C4::Context->preference("singleBranchMode"); if ( $singleBranchMode || !$OPACChooseBranch ) { # single branch mode or disabled user choosing - $branch = $borr->{'branchcode'}; + $pickupLocation = $borr->{'branchcode'}; } -#item may belong to a host biblio, if yes change biblioNum to hosts bilbionumber + #item may belong to a host biblio, if yes change biblioNum to hosts bilbionumber if ( $itemNum ne '' ) { my $hostbiblioNum = GetBiblionumberFromItemnumber($itemNum); if ( $hostbiblioNum ne $biblioNum ) { @@ -235,20 +236,26 @@ if ( $query->param('place_reserve') ) { my $expiration_date = $query->param("expiration_date_$biblioNum"); - # If a specific item was selected and the pickup branch is the same as the - # holdingbranch, force the value $rank and $found. + # If a specific item was selected and the pickup branch is the same as the + # holdingbranch, force the value $rank and $found. my $rank = $biblioData->{rank}; if ( $itemNum ne '' ) { + my $item = GetItem($itemNum); $canreserve = 1 if CanItemBeReserved( $borrowernumber, $itemNum ); $rank = '0' unless C4::Context->preference('ReservesNeedReturns'); - my $item = GetItem($itemNum); - if ( $item->{'holdingbranch'} eq $branch ) { + if ( $item->{'holdingbranch'} eq $pickupLocation ) { $found = 'W' unless C4::Context->preference('ReservesNeedReturns'); } + + # UseBranchTransferLimits checking. + my ($transferOk, $message) = CheckBranchTransferAllowed( $pickupLocation, $item->{'holdingbranch'}, $item, undef ); + if (! $transferOk) { + $canreserve = 0; + } } else { - $canreserve = 1 if CanBookBeReserved( $borrowernumber, $biblioNum ); + $canreserve = 1 if CanBookBeReserved( $borrowernumber, $biblioNum, $pickupLocation ); # Inserts a null into the 'itemnumber' field of 'reserves' table. $itemNum = undef; @@ -264,7 +271,7 @@ if ( $query->param('place_reserve') ) { # Here we actually do the reserveration. Stage 3. if ($canreserve) { AddReserve( - $branch, $borrowernumber, + $pickupLocation, $borrowernumber, $biblioNum, 'a', [$biblioNum], $rank, $startdate, $expiration_date, @@ -398,6 +405,10 @@ foreach my $biblioNum (@biblionumbers) { } } + #Collect the amout of items that pass the CheckBranchTransferAllowed-check. This is needed to tell + # the user if some or all Items cannot be transferred to the pickup location. + my $branchTransferableItemsCount = 0; + $biblioLoopIter{itemLoop} = []; my $numCopiesAvailable = 0; foreach my $itemInfo (@{$biblioData->{itemInfos}}) { @@ -434,10 +445,10 @@ foreach my $biblioNum (@biblionumbers) { my ($reservedate,$reservedfor,$expectedAt) = GetReservesFromItemnumber($itemNum); my $ItemBorrowerReserveInfo = GetMemberDetails( $reservedfor, 0); - # the item could be reserved for this borrower vi a host record, flag this - if ($reservedfor eq $borrowernumber){ - $itemLoopIter->{already_reserved} = 1; - } + # the item could be reserved for this borrower vi a host record, flag this + if ($reservedfor eq $borrowernumber){ + $itemLoopIter->{already_reserved} = 1; + } if ( defined $reservedate ) { $itemLoopIter->{backgroundcolor} = 'reserved'; @@ -480,12 +491,12 @@ foreach my $biblioNum (@biblionumbers) { $itemLoopIter->{nocancel} = 1; } - # if the items belongs to a host record, show link to host record - if ($itemInfo->{biblionumber} ne $biblioNum){ - $biblioLoopIter{hostitemsflag} = 1; - $itemLoopIter->{hostbiblionumber} = $itemInfo->{biblionumber}; - $itemLoopIter->{hosttitle} = GetBiblioData($itemInfo->{biblionumber})->{title}; - } + # if the items belongs to a host record, show link to host record + if ($itemInfo->{biblionumber} ne $biblioNum){ + $biblioLoopIter{hostitemsflag} = 1; + $itemLoopIter->{hostbiblionumber} = $itemInfo->{biblionumber}; + $itemLoopIter->{hosttitle} = GetBiblioData($itemInfo->{biblionumber})->{title}; + } # If there is no loan, return and transfer, we show a checkbox. $itemLoopIter->{notforloan} = $itemLoopIter->{notforloan} || 0; @@ -501,20 +512,32 @@ foreach my $biblioNum (@biblionumbers) { } if (IsAvailableForItemLevelRequest($itemNum) and $policy_holdallowed and CanItemBeReserved($borrowernumber,$itemNum) and ($itemLoopIter->{already_reserved} ne 1)) { - $itemLoopIter->{available} = 1; + + $itemLoopIter->{available} = 1; $numCopiesAvailable++; + + #Check for UseBranchTransferLimit. $numCopiesAvailable is incremented because this Item + # could still be available from another pickup location + my ($transferOk, $errorMsg) = CheckBranchTransferAllowed( $pickupBranch, undef, GetItem($itemNum), undef ); + if (! $transferOk) { + $itemLoopIter->{available} = 0; + $itemLoopIter->{branchTransferBlocked} = 1; + } + else { + $branchTransferableItemsCount++; + } } - # FIXME: move this to a pm + # FIXME: move this to a pm my $dbh = C4::Context->dbh; my $sth2 = $dbh->prepare("SELECT * FROM reserves WHERE borrowernumber=? AND itemnumber=? AND found='W'"); $sth2->execute($itemLoopIter->{ReservedForBorrowernumber}, $itemNum); while (my $wait_hashref = $sth2->fetchrow_hashref) { $itemLoopIter->{waitingdate} = format_date($wait_hashref->{waitingdate}); } - $itemLoopIter->{imageurl} = getitemtypeimagelocation( 'opac', $itemTypes->{ $itemInfo->{itype} }{imageurl} ); + $itemLoopIter->{imageurl} = getitemtypeimagelocation( 'opac', $itemTypes->{ $itemInfo->{itype} }{imageurl} ); - # Show serial enumeration when needed + # Show serial enumeration when needed if ($itemLoopIter->{enumchron}) { $itemdata_enumchron = 1; } @@ -523,6 +546,7 @@ foreach my $biblioNum (@biblionumbers) { } $template->param( itemdata_enumchron => $itemdata_enumchron ); + ## Set the behaviour flags for the template if ($numCopiesAvailable > 0) { $numBibsAvailable++; $biblioLoopIter{bib_available} = 1; @@ -538,13 +562,18 @@ foreach my $biblioNum (@biblionumbers) { $biblioLoopIter{holdable} = undef; $biblioLoopIter{already_patron_possession} = 1; } + if ($branchTransferableItemsCount == 0) { + #We can tell our Borrowers that they can try another pickup location if they don't find what they need. + $biblioLoopIter{suggestAnotherPickupLocation} = 1 ; + } + if( $biblioLoopIter{holdable} ){ $anyholdable++; } push @$biblioLoop, \%biblioLoopIter; } -if ( $numBibsAvailable == 0 || $anyholdable == 0) { +if ( $numBibsAvailable == 0 || $anyholdable == 0 ) { $template->param( none_available => 1 ); } -- 1.8.1.2