From a57723fc594e92a197da9470959d4067e3d63221 Mon Sep 17 00:00:00 2001 From: Kyle M Hall Date: Fri, 8 Feb 2019 14:46:18 -0500 Subject: [PATCH] Bug 22330: Transfer limits should be respected for placing holds in staff interface and APIs Branch transfer limits are respected for placing holds in the OPAC but nowhere else. This should be remedied. Test Plan: 1) Set up a branch transfer limit from Library A to Library B 2) Verify you cannot set up a hold for an item from Library A for pickup at Library B from the staff interface ( without overriding ) 3) Verify you cannot place that hold via ILS-DI 4) Verify you cannot place that hold via SIP 4) Verify a forced hold from Library A to Library B will not show up in the holds queue Signed-off-by: Liz Rea Signed-off-by: Josef Moravec Signed-off-by: Nick Clemens --- C4/HoldsQueue.pm | 11 +++ C4/ILSDI/Services.pm | 12 ++- C4/Reserves.pm | 40 +++++--- C4/SIP/ILS/Transaction/Hold.pm | 6 ++ .../prog/en/modules/reserve/request.tt | 16 +++- .../prog/js/circ-patron-search-results.js | 14 +-- reserve/request.pl | 6 +- t/db_dependent/Holds/HoldFulfillmentPolicy.t | 20 +++- t/db_dependent/HoldsQueue.t | 74 ++++++++++++++- t/db_dependent/ILSDI_Services.t | 95 ++++++++++++++++++- t/db_dependent/Reserves.t | 14 +++ 11 files changed, 275 insertions(+), 33 deletions(-) diff --git a/C4/HoldsQueue.pm b/C4/HoldsQueue.pm index 1015763dd8..0028d798dc 100755 --- a/C4/HoldsQueue.pm +++ b/C4/HoldsQueue.pm @@ -409,6 +409,8 @@ sub MapItemsToHoldRequests { || ( $item->{holdallowed} == 1 && $item->{homebranch} ne $request->{borrowerbranch} ); + next unless Koha::Items->find( $item->{itemnumber} )->can_be_transferred( { to => scalar Koha::Libraries->find( $request->{branchcode} ) } ); + my $local_holds_priority_item_branchcode = $item->{$LocalHoldsPriorityItemControl}; @@ -463,6 +465,7 @@ sub MapItemsToHoldRequests { and ( !$request->{itemtype} # If hold itemtype is set, item's itemtype must match || $items_by_itemnumber{ $request->{itemnumber} }->{itype} eq $request->{itemtype} ) ) + and Koha::Items->find( $request->{itemnumber} )->can_be_transferred( { to => scalar Koha::Libraries->find( $request->{branchcode} ) } ) ) { @@ -510,6 +513,8 @@ sub MapItemsToHoldRequests { my $holding_branch_items = $items_by_branch{$pickup_branch}; if ( $holding_branch_items ) { foreach my $item (@$holding_branch_items) { + next unless Koha::Items->find( $item->{itemnumber} )->can_be_transferred( { to => scalar Koha::Libraries->find( $request->{branchcode} ) } ); + if ( $request->{borrowerbranch} eq $item->{homebranch} && ( ( $item->{hold_fulfillment_policy} eq 'any' ) # Don't fill item level holds that contravene the hold pickup policy at this time @@ -532,6 +537,7 @@ sub MapItemsToHoldRequests { my $holding_branch_items = $items_by_branch{$holdingbranch}; foreach my $item (@$holding_branch_items) { next if $request->{borrowerbranch} ne $item->{homebranch}; + next unless Koha::Items->find( $item->{itemnumber} )->can_be_transferred( { to => scalar Koha::Libraris->find( $request->{branchcode} ) } ); # Don't fill item level holds that contravene the hold pickup policy at this time next unless $item->{hold_fulfillment_policy} eq 'any' @@ -568,6 +574,7 @@ sub MapItemsToHoldRequests { foreach my $item (@$holding_branch_items) { next if $pickup_branch ne $item->{homebranch}; next if ( $item->{holdallowed} == 1 && $item->{homebranch} ne $request->{borrowerbranch} ); + next unless Koha::Items->find( $item->{itemnumber} )->can_be_transferred( { to => scalar Koha::Libraries->find( $request->{branchcode} ) } ); # Don't fill item level holds that contravene the hold pickup policy at this time next unless $item->{hold_fulfillment_policy} eq 'any' @@ -596,6 +603,8 @@ sub MapItemsToHoldRequests { next unless ( !$request->{itemtype} || $current_item->{itype} eq $request->{itemtype} ); + next unless Koha::Items->find( $current_item->{itemnumber} )->can_be_transferred( { to => scalar Koha::Libraries->find( $request->{branchcode} ) } ); + $itemnumber = $current_item->{itemnumber}; last; # quit this loop as soon as we have a suitable item } @@ -620,6 +629,8 @@ sub MapItemsToHoldRequests { next unless ( !$request->{itemtype} || $item->{itype} eq $request->{itemtype} ); + next unless Koha::Items->find( $item->{itemnumber} )->can_be_transferred( { to => scalar Koha::Libraries->find( $request->{branchcode} ) } ); + $itemnumber = $item->{itemnumber}; $holdingbranch = $branch; last PULL_BRANCHES2; diff --git a/C4/ILSDI/Services.pm b/C4/ILSDI/Services.pm index 2b291b82a4..a1e23cf5de 100644 --- a/C4/ILSDI/Services.pm +++ b/C4/ILSDI/Services.pm @@ -694,6 +694,10 @@ sub HoldTitle { $branch = $patron->branchcode; } + my $destination = Koha::Libraries->find($branch); + return { code => 'libraryNotPickupLocation' } unless $destination->pickup_location; + return { code => 'cannotBeTransferred' } unless $biblio->can_be_transferred({ to => $destination }); + # Add the reserve # $branch, $borrowernumber, $biblionumber, # $constraint, $bibitems, $priority, $resdate, $expdate, $notes, @@ -757,10 +761,6 @@ sub HoldItem { # If the biblio does not match the item, return an error code return { code => 'RecordNotFound' } if $item->biblionumber ne $biblio->biblionumber; - # Check for item disponibility - my $canitembereserved = C4::Reserves::CanItemBeReserved( $borrowernumber, $itemnumber )->{status}; - return { code => $canitembereserved } unless $canitembereserved eq 'OK'; - # Pickup branch management my $branch; if ( $cgi->param('pickup_location') ) { @@ -770,6 +770,10 @@ sub HoldItem { $branch = $patron->branchcode; } + # Check for item disponibility + my $canitembereserved = C4::Reserves::CanItemBeReserved( $borrowernumber, $itemnumber, $branch )->{status}; + return { code => $canitembereserved } unless $canitembereserved eq 'OK'; + # Add the reserve # $branch, $borrowernumber, $biblionumber, # $constraint, $bibitems, $priority, $resdate, $expdate, $notes, diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 3617549582..48b4170a69 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -23,37 +23,38 @@ package C4::Reserves; use strict; #use warnings; FIXME - Bug 2505 -use C4::Context; +use C4::Accounts; use C4::Biblio; -use C4::Members; -use C4::Items; use C4::Circulation; -use C4::Accounts; +use C4::Context; +use C4::Items; +use C4::Members; # for _koha_notify_reserve -use C4::Members::Messaging; -use C4::Members qw(); use C4::Letters; use C4::Log; +use C4::Members qw(); +use C4::Members::Messaging; +use Koha::Account::Lines; use Koha::Biblios; -use Koha::DateUtils; use Koha::Calendar; +use Koha::CirculationRules; use Koha::Database; +use Koha::DateUtils; use Koha::Hold; -use Koha::Old::Hold; use Koha::Holds; -use Koha::Libraries; use Koha::IssuingRules; -use Koha::Items; use Koha::ItemTypes; +use Koha::Items; +use Koha::Libraries; +use Koha::Libraries; +use Koha::Old::Hold; use Koha::Patrons; -use Koha::CirculationRules; -use Koha::Account::Lines; -use List::MoreUtils qw( firstidx any ); use Carp; use Data::Dumper; +use List::MoreUtils qw( firstidx any ); use vars qw(@ISA @EXPORT @EXPORT_OK %EXPORT_TAGS); @@ -482,7 +483,7 @@ sub CanItemBeReserved { return { status => 'libraryNotPickupLocation' }; } unless ($item->can_be_transferred({ to => $destination })) { - return 'cannotBeTransferred'; + return { status => 'cannotBeTransferred' }; } } @@ -801,6 +802,7 @@ sub CheckReserves { next if (($branchitemrule->{'holdallowed'} == 1) && ($branch ne $patron->branchcode)); my $hold_fulfillment_policy = $branchitemrule->{hold_fulfillment_policy}; next if ( ($branchitemrule->{hold_fulfillment_policy} ne 'any') && ($res->{branchcode} ne $item->$hold_fulfillment_policy) ); + next unless $item->can_be_transferred( { to => scalar Koha::Libraries->find( $res->{branchcode} ) } ); $priority = $res->{'priority'}; $highest = $res; last if $local_hold_match; @@ -1140,7 +1142,7 @@ sub ModReserveMinusPriority { =head2 IsAvailableForItemLevelRequest - my $is_available = IsAvailableForItemLevelRequest($item_record,$borrower_record); + my $is_available = IsAvailableForItemLevelRequest( $item_record, $borrower_record, $pickup_branchcode ); Checks whether a given item record is available for an item-level hold request. An item is available if @@ -1165,6 +1167,7 @@ and canreservefromotherbranches. sub IsAvailableForItemLevelRequest { my $item = shift; my $borrower = shift; + my $pickup_branchcode = shift; my $dbh = C4::Context->dbh; # must check the notforloan setting of the itemtype @@ -1187,6 +1190,13 @@ sub IsAvailableForItemLevelRequest { my $on_shelf_holds = Koha::IssuingRules->get_onshelfholds_policy( { item => $item_object, patron => $patron } ); + if ($pickup_branchcode) { + my $destination = Koha::Libraries->find($pickup_branchcode); + return 0 unless $destination; + return 0 unless $destination->pickup_location; + return 0 unless $item_object->can_be_transferred( { to => $destination } ); + } + if ( $on_shelf_holds == 1 ) { return 1; } elsif ( $on_shelf_holds == 2 ) { diff --git a/C4/SIP/ILS/Transaction/Hold.pm b/C4/SIP/ILS/Transaction/Hold.pm index d834516d10..63e1a06dbb 100644 --- a/C4/SIP/ILS/Transaction/Hold.pm +++ b/C4/SIP/ILS/Transaction/Hold.pm @@ -60,6 +60,12 @@ sub do_hold { $self->ok(0); return $self; } + unless ( $item->can_be_transferred( { to => scalar Koha::Libraries->find( $branch ) } ) ) { + $self->screen_msg('Item cannot be transferred.'); + $self->ok(0); + return $self; + } + AddReserve( $branch, $patron->borrowernumber, $item->biblionumber ); # unfortunately no meaningful return value 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 c0c7a07508..fd7571ebff 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt @@ -199,7 +199,7 @@
    • @@ -340,6 +340,8 @@ Patron is from different library [% ELSIF itemloo.not_holdable == 'itemAlreadyOnHold' %] Patron already has hold for this item + [% ELSIF itemloo.not_holdable == 'cannotBeTransferred' %] + Cannot be transferred to pickup library [% ELSE %] [% itemloo.not_holdable | html %] [% END %] @@ -661,6 +663,8 @@ [% INCLUDE 'columns_settings.inc' %] [% Asset.js("js/circ-patron-search-results.js") | $raw %]