From 8b8ec7468286cd53e62737aaa6fbe944bf2b987c Mon Sep 17 00:00:00 2001 From: Olli-Antti Kivilahti Date: Tue, 15 Oct 2013 13:16:05 +0300 Subject: [PATCH] Bug 11020 - Transfer limits should be checked when placing a hold/reservation in Staff client Extended bug 11005. Extended C4/Reserves.pm to support checking BranchTransferLimits when requested. Extension does in no way disturb existing functionality. Modified the templates to show branch transfer blocks to the user. Modified the business layer to re-check branch transfer limits in case user fools the POST-request. Created manual test cases. --- C4/Reserves.pm | 25 +++-- .../prog/en/modules/reserve/request.tt | 110 +++++++++++++++------ reserve/placerequest.pl | 79 +++++++++------ reserve/request.pl | 40 ++++++-- 4 files changed, 180 insertions(+), 74 deletions(-) diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 8fe78db..98d3de0 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -409,22 +409,33 @@ sub GetReservesFromBorrowernumber { #------------------------------------------------------------------------------------- =head2 CanBookBeReserved - $error = &CanBookBeReserved($borrowernumber, $biblionumber) + $error = &CanBookBeReserved($borrowernumber, $biblionumber, $pickupLibrary) + +C<$pickupLibrary> OPTIONAL, undef OR the branchcode of the item transfer destination. =cut sub CanBookBeReserved{ - my ($borrowernumber, $biblionumber) = @_; - - my $items = GetItemnumbersForBiblio($biblionumber); + my ($borrowernumber, $biblionumber, $pickupLibrary) = @_; + + my $itemNumbers = GetItemnumbersForBiblio($biblionumber); #get items linked via host records my @hostitems = get_hostitemnumbers_of($biblionumber); if (@hostitems){ - push (@$items,@hostitems); + push (@$itemNumbers,@hostitems); } - foreach my $item (@$items){ - return 1 if CanItemBeReserved($borrowernumber, $item); + foreach my $itemNumber (@$itemNumbers){ + + if (defined $pickupLibrary) { + return 0 if (! CanItemBeReserved($borrowernumber, $itemNumber) ); + my $item = GetItem($itemNumber); + my ($transferOk, $errorMsg) = C4::Circulation::CheckBranchTransferAllowed($pickupLibrary, $item->{holdingbranch}, $item, undef); + return 1 if ($transferOk); + } + else { + return 1 if ( CanItemBeReserved($borrowernumber, $itemNumber) ); + } } return 0; } diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt index 7f7d05f..accf5fe 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt @@ -130,6 +130,19 @@ function checkMultiHold() { }); $('#hold-request-form').preventDoubleFormSubmit(); + $('#reloadPickupLocationForm').preventDoubleFormSubmit(); + + //Setting the trigger to reload branchTransferLimits + $('#pickup').change(function() { + var newSelection = $(this).val(); + + //Make sure the pickup location is changed to the first select as well. + // The branch selection uses only the first given pickuplocations branch value. + $('#reloadPickupLocationForm > input[name="pickup"]').val(newSelection); + + $('#reloadPickupLocationForm').submit(); + }); + [% UNLESS ( borrowernumber || borrower_list ) %] [% IF ( CircAutocompl ) %] @@ -352,10 +365,15 @@ function checkMultiHold() { [% END %] - [% UNLESS ( multi_hold ) %] + + + + [% UNLESS ( multi_hold ) %]
[% IF ( borrowernumber ) %] - [% IF ( override_required ) %] + [% IF ( bookBranchTransferDenied ) %] + + [% ELSIF ( override_required ) %] [% ELSIF ( none_available ) %] @@ -367,11 +385,17 @@ function checkMultiHold() { [% FOREACH bibitemloo IN bibitemloop %]
    [% UNLESS ( item_level_itypes ) %] -
  1. Item type: [% bibitemloo.description %]
  2. +
  3. Item type: [% bibitemloo.description %]
  4. [% END %] - [% IF ( bibitemloo.publicationyear ) %]
  5. Publication year: [% bibitemloo.publicationyear %]
  6. [% END %] + [% IF ( bibitemloo.publicationyear ) %] +
  7. Publication year: [% bibitemloo.publicationyear %]
  8. + [% END %]
+ + [% IF ( bibitemloo.suggestAnotherPickupLocation ) %] +
None of the available items can be transferred to your pickup location. You could try another pickup location.
+ [% END %] @@ -396,12 +420,15 @@ function checkMultiHold() { [% IF ( item_level_itypes ) %] @@ -475,6 +502,7 @@ function checkMultiHold() { [% END %] [% ELSE %] +
Place a hold on a specific copy
[% IF ( itemloo.available ) %] + [% ELSIF ( itemloo.branchTransferDenied ) %] + + Cannot transfer this item to your pickup location [% ELSIF ( itemloo.override ) %] - Requires override of hold policy + Requires override of hold policy [% ELSE %] - Cannot be put on hold + Cannot be put on hold [% END %]
@@ -493,7 +521,12 @@ function checkMultiHold() { [% END %] [% END %]
    -
  • [% biblioloo.title |html %]
  • +
  • + [% IF ( biblioloo.suggestAnotherPickupLocation ) %] + Cannot transfer this item to your pickup location + [% END %] + [% biblioloo.title |html %] +
  • [% IF ( biblioloo.publicationyear ) %]
  • Publication year: [% biblioloo.publicationyear %]
  • [% END %] @@ -538,12 +571,20 @@ function checkMultiHold() {
+ + [% IF ( bookBranchTransferDenied ) %] +
+ Some books cannot be transferred to your pickup location. Please try another pickup location, or reserve fewer items at once. +
+ [% END %] [% END %]
[% IF ( borrowernumber ) %] - [% IF ( override_required ) %] + [% IF ( bookBranchTransferDenied ) %] + + [% ELSIF ( override_required ) %] [% ELSIF ( none_available ) %] @@ -600,26 +641,17 @@ function checkMultiHold() { @@ -774,4 +806,26 @@ function checkMultiHold() { +[% IF ( multi_hold ) %] +
+ + + + + + + + [% FOREACH biblioloo IN biblioloop %] + + + [% END %] +
+[% ELSE %] +
+ + + +
+[% END %] + [% INCLUDE 'intranet-bottom.inc' %] diff --git a/reserve/placerequest.pl b/reserve/placerequest.pl index 3fe459c..27b6a6b 100755 --- a/reserve/placerequest.pl +++ b/reserve/placerequest.pl @@ -59,6 +59,9 @@ my $multi_hold = $input->param('multi_hold'); my $biblionumbers = $multi_hold ? $input->param('biblionumbers') : ($biblionumber . '/'); my $bad_bibs = $input->param('bad_bibs'); +my $transferOk; # Used to check if this reservation passes UseBranchTransferLimits check. +my $errorMessages; # Used to concatenate generated error messages from multiple biblios. + my %bibinfos = (); my @biblionumbers = split '/', $biblionumbers; foreach my $bibnum (@biblionumbers) { @@ -67,12 +70,11 @@ foreach my $bibnum (@biblionumbers) { $bibinfo{rank} = $input->param("rank_$bibnum"); $bibinfos{$bibnum} = \%bibinfo; } - my $found; # if we have an item selectionned, and the pickup branch is the same as the holdingbranch # of the document, we force the value $rank and $found . -if ($checkitem ne ''){ +if (defined $checkitem && $checkitem ne ''){ $rank[0] = '0' unless C4::Context->preference('ReservesNeedReturns'); my $item = $checkitem; $item = GetItem($item); @@ -97,35 +99,52 @@ if ($type eq 'str8' && $borrower){ } } my $const; - - if ($checkitem ne ''){ - my $item = GetItem($checkitem); - if ($item->{'biblionumber'} ne $biblionumber) { - $biblionumber = $item->{'biblionumber'}; - } - } - - - - if ($multi_hold) { - my $bibinfo = $bibinfos{$biblionumber}; - AddReserve($branch,$borrower->{'borrowernumber'},$biblionumber,'a',[$biblionumber], - $bibinfo->{rank},$startdate,$expirationdate,$notes,$bibinfo->{title},$checkitem,$found); - } else { - if ($input->param('request') eq 'any'){ - # place a request on 1st available - AddReserve($branch,$borrower->{'borrowernumber'},$biblionumber,'a',\@realbi,$rank[0],$startdate,$expirationdate,$notes,$title,$checkitem,$found); - } elsif ($reqbib[0] ne ''){ - # FIXME : elsif probably never reached, (see top of the script) - # place a request on a given item - AddReserve($branch,$borrower->{'borrowernumber'},$biblionumber,'o',\@reqbib,$rank[0],$startdate,$expirationdate,$notes,$title,$checkitem, $found); - } else { - AddReserve($branch,$borrower->{'borrowernumber'},$biblionumber,'a',\@realbi,$rank[0],$startdate,$expirationdate,$notes,$title,$checkitem, $found); - } - } + + if (defined $checkitem && $checkitem ne ''){ + my $item = GetItem($checkitem); + + #Making sure the biblionumber is present. Pretty robust! And possibly scary? + if ($item->{'biblionumber'} ne $biblionumber) { + $biblionumber = $item->{'biblionumber'}; + } + my $errorMsg; + ($transferOk, $errorMsg) = CheckBranchTransferAllowed($branch, $item->{holdingbranch}, $item, undef); + if (! $transferOk) { + $errorMessages .= $errorMsg . ', '; + } + } + else { + #Final check if UseBranchTransferLimits validate for this Biblio. + $transferOk = CanBookBeReserved( $borrowernumber, $biblionumber, $branch ); + } + + + if ($transferOk) { + + if ($multi_hold) { + my $bibinfo = $bibinfos{$biblionumber}; + AddReserve($branch,$borrower->{'borrowernumber'},$biblionumber,'a',[$biblionumber], + $bibinfo->{rank},$startdate,$expirationdate,$notes,$bibinfo->{title},$checkitem,$found); + } + else { + if ($input->param('request') eq 'any'){ + # place a request on 1st available + AddReserve($branch,$borrower->{'borrowernumber'},$biblionumber,'a',\@realbi,$rank[0],$startdate,$expirationdate,$notes,$title,$checkitem,$found); + } elsif ($reqbib[0] ne ''){ + # FIXME : elsif probably never reached, (see top of the script) + # place a request on a given item + AddReserve($branch,$borrower->{'borrowernumber'},$biblionumber,'o',\@reqbib,$rank[0],$startdate,$expirationdate,$notes,$title,$checkitem, $found); + } else { + AddReserve($branch,$borrower->{'borrowernumber'},$biblionumber,'a',\@realbi,$rank[0],$startdate,$expirationdate,$notes,$title,$checkitem, $found); + } + } + } } - - if ($multi_hold) { + if ($errorMessages) { + print $input->header(); + print "The following branch transfer limits are blocked:\n".$errorMessages; + } + elsif ($multi_hold) { if ($bad_bibs) { $biblionumbers .= $bad_bibs; } diff --git a/reserve/request.pl b/reserve/request.pl index c411ce9..d3c6651 100755 --- a/reserve/request.pl +++ b/reserve/request.pl @@ -69,6 +69,7 @@ my $userbranch = ''; if (C4::Context->userenv && C4::Context->userenv->{'branch'}) { $userbranch = C4::Context->userenv->{'branch'}; } +my $pickupBranch = $input->param('pickup') ? $input->param('pickup') : $userbranch; # Select borrowers infos @@ -282,12 +283,16 @@ foreach my $biblionumber (@biblionumbers) { my $num_available = 0; my $num_override = 0; my $hiddencount = 0; + + #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; $biblioitem->{description} = $itemtypes->{ $biblioitem->{itemtype} }{description}; - if($biblioitem->{biblioitemnumber} ne $biblionumber){ - $biblioitem->{hostitemsflag}=1; - } + if($biblioitem->{biblioitemnumber} ne $biblionumber){ + $biblioitem->{hostitemsflag}=1; + } $biblioloopiter{description} = $biblioitem->{description}; $biblioloopiter{itypename} = $biblioitem->{description}; $biblioloopiter{imageurl} = @@ -311,10 +316,10 @@ foreach my $biblionumber (@biblionumbers) { $branches->{ $item->{holdingbranch} }{branchname}; } - if($item->{biblionumber} ne $biblionumber){ - $item->{hostitemsflag}=1; - $item->{hosttitle} = GetBiblioData($item->{biblionumber})->{title}; - } + if($item->{biblionumber} ne $biblionumber){ + $item->{hostitemsflag}=1; + $item->{hosttitle} = GetBiblioData($item->{biblionumber})->{title}; + } # add information $item->{itemcallnumber} = $item->{itemcallnumber}; @@ -389,6 +394,17 @@ foreach my $biblionumber (@biblionumbers) { } } } + + # UseBranchTransferLimits checking + my ($transferOk, $errorMessage) = CheckBranchTransferAllowed( $pickupBranch, $item->{holdingbranch}, $item, undef ); + if ( ! $transferOk ) { + $item->{cantreserve} = 1 ; + $item->{branchTransferDenied} = 1 ; + } + else { + $branchTransferableItemsCount++; + } + my $branch = C4::Circulation::_GetCircControlBranch($item, $borrowerinfo); @@ -417,7 +433,7 @@ foreach my $biblionumber (@biblionumbers) { } elsif ( C4::Context->preference('AllowHoldPolicyOverride') ) { -# If AllowHoldPolicyOverride is set, it should override EVERY restriction, not just branch item rules + # If AllowHoldPolicyOverride is set, it should override EVERY restriction, not just branch item rules $item->{override} = 1; $num_override++; } @@ -446,6 +462,12 @@ foreach my $biblionumber (@biblionumbers) { $biblioloopiter{warn} = 1; $biblioloopiter{none_avail} = 1; } + if ( $branchTransferableItemsCount == 0) { + $biblioloopiter{suggestAnotherPickupLocation} = 1; + $biblioitem->{suggestAnotherPickupLocation} = 1; + $template->param( bookBranchTransferDenied => 1 ); + } + $template->param( hiddencount => $hiddencount); push @bibitemloop, $biblioitem; @@ -538,7 +560,7 @@ foreach my $biblionumber (@biblionumbers) { my $time = time(); $template->param( - branchloop => GetBranchesLoop($userbranch), + branchloop => GetBranchesLoop($pickupBranch), time => $time, fixedRank => $fixedRank, ); -- 1.8.1.2