From 7c7aee054fa8af8e753d7f9391a27efb83f9b52c Mon Sep 17 00:00:00 2001 From: Kyle M Hall Date: Tue, 23 Jul 2013 08:05:22 -0400 Subject: [PATCH] Bug 10628: make sure AutomaticItemReturn doesn't prevent holds queue from filling local holds with local items For some reason, C4::HoldsQueue::MapItemsToHoldRequests used the system preference AutomaticItemReturn to decide if an attempt to fill local holds with local items. No explanation of this behavior is provided. This patch removes this behavior, and also adjusts the calculation of the lead-cost library to always return the pickup library if it is on the list of libraries that could fill the hold -- on the basis that if the item is already at the pickup library, its transport cost is inherently zero. Signed-off-by: Srdjan Signed-off-by: Katrin Fischer Passes QA script and adds unit tests. Tested with some examples and those worked correctly. Signed-off-by: Galen Charlton (cherry picked from commit d46657f8af980524a4351e40abe0b901d1af3e6a) Signed-off-by: Tomas Cohen Arazi Small merge problem in the tests. --- C4/HoldsQueue.pm | 13 ++++++++----- t/db_dependent/HoldsQueue.t | 11 +++++++---- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/C4/HoldsQueue.pm b/C4/HoldsQueue.pm index 2052f096c7..eff3148d40 100755 --- a/C4/HoldsQueue.pm +++ b/C4/HoldsQueue.pm @@ -32,6 +32,7 @@ use C4::Biblio; use C4::Dates qw/format_date/; use List::Util qw(shuffle); +use List::MoreUtils qw(any); use Data::Dumper; use vars qw($VERSION @ISA @EXPORT @EXPORT_OK %EXPORT_TAGS); @@ -354,8 +355,6 @@ sub MapItemsToHoldRequests { return unless scalar(@$hold_requests) > 0; return unless scalar(@$available_items) > 0; - my $automatic_return = C4::Context->preference("AutomaticItemReturn"); - # identify item-level requests my %specific_items_requested = map { $_->{itemnumber} => 1 } grep { defined($_->{itemnumber}) } @@ -419,7 +418,7 @@ sub MapItemsToHoldRequests { my $pickup_branch = $request->{branchcode} || $request->{borrowerbranch}; my ($itemnumber, $holdingbranch); - my $holding_branch_items = $automatic_return ? undef : $items_by_branch{$pickup_branch}; + my $holding_branch_items = $items_by_branch{$pickup_branch}; if ( $holding_branch_items ) { foreach my $item (@$holding_branch_items) { if ( $request->{borrowerbranch} eq $item->{homebranch} ) { @@ -587,10 +586,14 @@ sub least_cost_branch { #$from - arrayref my ($to, $from, $transport_cost_matrix) = @_; -# Nothing really spectacular: supply to branch, a list of potential from branches -# and find the minimum from - to value from the transport_cost_matrix + # Nothing really spectacular: supply to branch, a list of potential from branches + # and find the minimum from - to value from the transport_cost_matrix return $from->[0] if @$from == 1; + # If the pickup library is in the list of libraries to pull from, + # return that library right away, it is obviously the least costly + return ($to) if any { $_ eq $to } @$from; + my ($least_cost, @branch); foreach (@$from) { my $cell = $transport_cost_matrix->{$to}{$_}; diff --git a/t/db_dependent/HoldsQueue.t b/t/db_dependent/HoldsQueue.t index 8de2583552..5c4c74b363 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 => 18; +use Test::More tests => 16; use C4::Branch; use C4::ItemType; @@ -100,7 +100,7 @@ my $priority = 1; # Make a reserve AddReserve ( $borrower_branchcode, $borrowernumber, $biblionumber, $constraint, $bibitems, $priority ); # $resdate, $expdate, $notes, $title, $checkitem, $found -$dbh->do("UPDATE reserves SET reservedate = reservedate - 1"); +$dbh->do("UPDATE reserves SET reservedate = DATE_SUB( reservedate, INTERVAL 1 DAY )"); # Tests my $use_cost_matrix_sth = $dbh->prepare("UPDATE systempreferences SET value = ? WHERE variable = 'UseTransportCostMatrix'"); @@ -121,8 +121,6 @@ $dbh->do("DELETE FROM items WHERE homebranch = '$borrower_branchcode' AND holdin # test_queue will flush C4::Context->set_preference('AutomaticItemReturn', 1); # Not sure how to make this test more difficult - holding branch does not matter -test_queue ('take from holdingbranch AutomaticItemReturn on', 0, $borrower_branchcode, undef); -test_queue ('take from holdingbranch AutomaticItemReturn on', 1, $borrower_branchcode, $least_cost_branch_code); $dbh->do("DELETE FROM tmp_holdsqueue"); $dbh->do("DELETE FROM hold_fill_targets"); @@ -148,6 +146,11 @@ ok( $queue_item && $queue_item->{holdingbranch} eq $least_cost_branch_code, "GetHoldsQueueItems" ) or diag( "Expected item for pick $borrower_branchcode, hold $least_cost_branch_code, got ".Dumper($queue_item) ); +ok( + C4::HoldsQueue::least_cost_branch( 'B', [ 'A', 'B', 'C' ] ) eq 'B', + 'C4::HoldsQueue::least_cost_branch returns the local branch if it is in the list of branches to pull from' +); + # XXX All this tests are for borrower branch pick-up. # Maybe needs expanding to homebranch or holdingbranch pick-up. -- 2.39.5