[Koha-patches] [PATCH] Bug 10628 - AutomaticItemReturn prevents holds queue from filling local holds with local items

[Srdjan]

From: Kyle M Hall <kyle at bywatersolutions.com>

For some reason, C4::HoldsQueue::MapItemsToHoldRequests uses the system
preference AutomaticItemReturn to decide if an attempt to fill local
holds with local items. No explanation of this behavior is provided.

Signed-off-by: Srdjan <srdjan at catalyst.net.nz>
---
 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 2052f09..eff3148 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 8de2583..5c4c74b 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.
 
-- 
1.8.1.2