From 3ad803cd3ade0ce7e3d0130a18273d2a14d1eb80 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 plans. --- C4/Reserves.pm | 37 ++++--- .../prog/en/modules/reserve/request.tt | 110 +++++++++++++++------ reserve/placerequest.pl | 77 +++++++++------ reserve/request.pl | 48 ++++++--- 4 files changed, 189 insertions(+), 83 deletions(-) diff --git a/C4/Reserves.pm b/C4/Reserves.pm index f8666a3..eca659e 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -117,7 +117,7 @@ BEGIN { &CheckReserves &CanBookBeReserved - &CanItemBeReserved + &CanItemBeReserved &CancelReserve &CancelExpiredReserves @@ -162,8 +162,8 @@ sub AddReserve { undef $expdate; # make reserves.expirationdate default to null rather than '0000-00-00' } if ( C4::Context->preference( 'AllowHoldDateInFuture' ) ) { - # Make room in reserves for this before those of a later reserve date - $priority = _ShiftPriorityByDateAndPriority( $biblionumber, $resdate, $priority ); + # Make room in reserves for this before those of a later reserve date + $priority = _ShiftPriorityByDateAndPriority( $biblionumber, $resdate, $priority ); } my $waitingdate; @@ -365,7 +365,7 @@ sub GetReservesFromItemnumber { WHERE itemnumber=? "; unless ( $all_dates ) { - $query .= " AND reservedate <= CURRENT_DATE()"; + $query .= " AND reservedate <= CURRENT_DATE()"; } my $sth_res = $dbh->prepare($query); $sth_res->execute($itemnumber); @@ -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 ($borrowernumber, $biblionumber, $pickupLibrary) = @_; - my $items = GetItemnumbersForBiblio($biblionumber); + 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::CanItemBeTransferred($pickupLibrary, $item->{holdingbranch}, $item, undef); + return 1 if ($transferOk); + } + else { + return 1 if ( CanItemBeReserved($borrowernumber, $itemNumber) ); + } } return 0; } @@ -842,7 +853,7 @@ sub CheckReserves { my $sth; my $select; if (C4::Context->preference('item-level_itypes')){ - $select = " + $select = " SELECT items.biblionumber, items.biblioitemnumber, itemtypes.notforloan, @@ -854,7 +865,7 @@ sub CheckReserves { "; } else { - $select = " + $select = " SELECT items.biblionumber, items.biblioitemnumber, itemtypes.notforloan, @@ -1950,7 +1961,7 @@ sub _ShiftPriorityByDateAndPriority { $sth = $dbh->prepare( $query ); $sth->execute( $new_priority, $biblio ); while ( my $row = $sth->fetchrow_hashref ) { - $sth_update->execute( $biblio, $row->{borrowernumber}, $row->{reservedate} ); + $sth_update->execute( $biblio, $row->{borrowernumber}, $row->{reservedate} ); } return $new_priority; # so the caller knows what priority they wind up receiving 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..9421e9b 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,12 +385,18 @@ 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 ) %] @@ -476,6 +503,7 @@ function checkMultiHold() { [% 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 %]
Title
    -
  • [% 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 %] @@ -539,11 +572,19 @@ function checkMultiHold() { [% END %]
+ [% 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..6e146b6 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); @@ -98,34 +100,51 @@ 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) = CanItemBeTransferred($branch, $item->{holdingbranch}, $item, undef); + if (! $transferOk) { + $errorMessages .= $errorMsg . ', '; } } - } + else { + #Final check if UseBranchTransferLimits validate for this Biblio. + $transferOk = CanBookBeReserved( $borrowernumber, $biblionumber, $branch ); + } + - if ($multi_hold) { + 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 ($errorMessages) { + print $input->header(); + print "The following branch transfer limits are blocked:\n".$errorMessages; + } + elsif ($multi_hold) { if ($bad_bibs) { $biblionumbers .= $bad_bibs; } @@ -134,8 +153,8 @@ if ($type eq 'str8' && $borrower){ print $input->redirect("request.pl?biblionumber=$biblionumber"); } } elsif ($borrower eq ''){ - print $input->header(); - print "Invalid borrower number please try again"; + print $input->header(); + print "Invalid borrower number please try again"; # Not sure that Dump() does HTML escaping. Use firebug or something to trace # instead. # print $input->Dump; diff --git a/reserve/request.pl b/reserve/request.pl index c411ce9..c3e7446 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 @@ -283,11 +284,15 @@ foreach my $biblionumber (@biblionumbers) { my $num_override = 0; my $hiddencount = 0; + #Collect the amout of items that pass the CanItemBeTransferred-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,11 +316,11 @@ 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}; @@ -390,6 +395,17 @@ foreach my $biblionumber (@biblionumbers) { } } + # UseBranchTransferLimits checking + my ($transferOk, $errorMessage) = CanItemBeTransferred( $pickupBranch, $item->{holdingbranch}, $item, undef ); + if ( ! $transferOk ) { + $item->{cantreserve} = 1 ; + $item->{branchTransferDenied} = 1 ; + } + else { + $branchTransferableItemsCount++; + } + + my $branch = C4::Circulation::_GetCircControlBranch($item, $borrowerinfo); my $branchitemrule = GetBranchItemRule( $branch, $item->{'itype'} ); @@ -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; @@ -499,9 +521,9 @@ foreach my $biblionumber (@biblionumbers) { # get borrowers reserve info my $reserveborrowerinfo = GetMember( borrowernumber => $res->{'borrowernumber'} ); if (C4::Context->preference('HidePatronName')){ - $reserve{'hidename'} = 1; - $reserve{'cardnumber'} = $reserveborrowerinfo->{'cardnumber'}; - } + $reserve{'hidename'} = 1; + $reserve{'cardnumber'} = $reserveborrowerinfo->{'cardnumber'}; + } $reserve{'expirationdate'} = format_date( $res->{'expirationdate'} ) unless ( !defined($res->{'expirationdate'}) || $res->{'expirationdate'} eq '0000-00-00' ); $reserve{'date'} = format_date( $res->{'reservedate'} ); @@ -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