Bug 10628: make sure AutomaticItemReturn doesn't prevent holds queue from filling...

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 <srdjan@catalyst.net.nz>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes QA script and adds unit tests.
Tested with some examples and those worked correctly.

Signed-off-by: Galen Charlton <gmc@esilibrary.com>

diff --git a/C4/HoldsQueue.pm b/C4/HoldsQueue.pm
index 094fbe7..ce4594b 100755 (executable)
--- 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);
@@ -361,8 +362,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}) }
@@ -426,7 +425,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} ) {
@@ -594,10 +593,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 b9f5afb..47f9ece 100755 (executable)
--- a/t/db_dependent/HoldsQueue.t
+++ b/t/db_dependent/HoldsQueue.t
@@ -12,7 +12,8 @@ use C4::Context;
 
 use Data::Dumper;
 
-use Test::More tests => 19;
+use Test::More tests => 17;
+
 
 use C4::Branch;
 use C4::ItemType;
@@ -100,7 +101,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 +122,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");
@@ -149,6 +148,11 @@ ok( $queue_item
   or diag( "Expected item for pick $borrower_branchcode, hold $least_cost_branch_code, got ".Dumper($queue_item) );
 ok( exists($queue_item->{itype}), 'item type included in queued items list (bug 5825)' );
 
+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.