From 79660377470e8d6ba9057eb59645886410852c92 Mon Sep 17 00:00:00 2001 From: Kyle M Hall Date: Fri, 15 Nov 2013 08:29:23 -0800 Subject: [PATCH] Bug 11258: fix another case where holds queue made transfer requests that contradict the library holds policy This patch fixes a problem where the holds queue generator was making requests where the pickup library is the same as the item's library but not the patron's branch, even if there is a "Default holds policy by item type" rule that states this item can only fill holds for patrons of the same library as the item. Test Plan: 1) Create a test record with 2 items with different itemtypes 2) Set the Default holds policy by item type for the first item to "From any library" 3) Set the Default holds policy by item type for the second item to "From home library" 4) Place a record level hold for a patron from another library, but for pickup at the same library as the item is from 5) Rebuild the holds queue 6) View the holds queue, note the item is listed, though this patron cannot place a hold on this item 7) Apply this patch 8) Repeat step 5, note the hold is no longer in the queue Signed-off-by: Liz Rea automated tests pass, functional tests pass. Bug replicated, eradicated by patch. Signed-off-by: Katrin Fischer I finally managed to reproduce this, patch works as described. Passes tests and QA script, provided tests fail without patch, but succeed with the patch. Signed-off-by: Galen Charlton --- C4/HoldsQueue.pm | 4 ++-- t/db_dependent/HoldsQueue.t | 7 ++++--- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/C4/HoldsQueue.pm b/C4/HoldsQueue.pm index 63f159ae52..0d645bdfdd 100755 --- a/C4/HoldsQueue.pm +++ b/C4/HoldsQueue.pm @@ -432,7 +432,6 @@ sub MapItemsToHoldRequests { } } $holdingbranch = $pickup_branch; - $itemnumber ||= $holding_branch_items->[0]->{itemnumber}; } elsif ($transport_cost_matrix) { $pull_branches = [keys %items_by_branch]; @@ -467,6 +466,7 @@ sub MapItemsToHoldRequests { $holdingbranch ||= $branch; foreach my $item (@$holding_branch_items) { next if $pickup_branch ne $item->{homebranch}; + next if ( $item->{holdallowed} == 1 && $item->{homebranch} ne $request->{borrowerbranch} ); $itemnumber = $item->{itemnumber}; $holdingbranch = $branch; @@ -476,7 +476,7 @@ sub MapItemsToHoldRequests { unless ( $itemnumber ) { foreach my $current_item ( @{ $items_by_branch{$holdingbranch} } ) { - if ( $holdingbranch && ( $current_item->{holdallowed} == 2 || $pickup_branch eq $current_item->{homebranch} ) ) { + if ( $holdingbranch && ( $current_item->{holdallowed} == 2 || $request->{borrowerbranch} eq $current_item->{homebranch} ) ) { $itemnumber = $current_item->{itemnumber}; last; # quit this loop as soon as we have a suitable item } diff --git a/t/db_dependent/HoldsQueue.t b/t/db_dependent/HoldsQueue.t index 92bd843baa..0a96143779 100755 --- a/t/db_dependent/HoldsQueue.t +++ b/t/db_dependent/HoldsQueue.t @@ -12,7 +12,7 @@ use C4::Context; use Data::Dumper; -use Test::More tests => 20; +use Test::More tests => 21; use C4::Branch; @@ -287,8 +287,9 @@ $dbh->do("DELETE FROM default_circ_rules"); $dbh->do("INSERT INTO default_circ_rules ( holdallowed ) VALUES ( 1 )"); C4::HoldsQueue::CreateQueue(); $holds_queue = $dbh->selectall_arrayref("SELECT * FROM tmp_holdsqueue", { Slice => {} }); -ok( @$holds_queue == 1, "Holds queue filling correct number for default holds policy 'from home library'" ); -ok( $holds_queue->[0]->{cardnumber} eq 'CARDNUMBER1', "Holds queue filling correct hold for default holds policy 'from home library'"); +ok( @$holds_queue == 2, "Holds queue filling correct number for default holds policy 'from home library'" ); +ok( $holds_queue->[0]->{cardnumber} eq 'CARDNUMBER1', "Holds queue filling 1st correct hold for default holds policy 'from home library'"); +ok( $holds_queue->[1]->{cardnumber} eq 'CARDNUMBER2', "Holds queue filling 2nd correct hold for default holds policy 'from home library'"); $dbh->do("DELETE FROM default_circ_rules"); $dbh->do("INSERT INTO default_circ_rules ( holdallowed ) VALUES ( 2 )"); -- 2.39.5