Bug 14514 - LocalHoldsPriority and the HoldsQueue conflict with each other

It appears that the LocalHoldsPriority feature and the Holds Queue are
fundamentally at odds with each other.

The problem appears to be that both are attempting to choose the best
way to fill holds. When you are using the holds queue and you check in
an item that has been selected by the holds queue builder, that part of
Koha where the LocalHoldsPriority feature lives doesn't get to see all
the holds in order to pick the best one. Instead only the hold selected
by the holds queue builder is returned so to the LocalHoldsPriority
feature, that is only one hold to pick from!

Test Plan:
1) Apply this patch
2) prove t/db_dependent/HoldsQueue.t
3) All tests should pass

Signed-off-by: Barton Chittenden barton@bywatersolutions.com
Signed-off-by: Dani Elder <dani@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>

diff --git a/C4/HoldsQueue.pm b/C4/HoldsQueue.pm
index 1a2de1d..e5f29c7 100755 (executable)
--- a/C4/HoldsQueue.pm
+++ b/C4/HoldsQueue.pm
@@ -388,6 +388,60 @@ sub MapItemsToHoldRequests {
 
     # figure out which item-level requests can be filled
     my $num_items_remaining = scalar(@$available_items);
+
+    # Look for Local Holds Priority matches first
+    if ( C4::Context->preference('LocalHoldsPriority') ) {
+        my $LocalHoldsPriorityPatronControl =
+          C4::Context->preference('LocalHoldsPriorityPatronControl');
+        my $LocalHoldsPriorityItemControl =
+          C4::Context->preference('LocalHoldsPriorityItemControl');
+
+        foreach my $request (@$hold_requests) {
+            last if $num_items_remaining == 0;
+
+            my $local_hold_match;
+            foreach my $item (@$available_items) {
+                next
+                  if ( !$item->{holdallowed} )
+                  || ( $item->{holdallowed} == 1
+                    && $item->{homebranch} ne $request->{borrowerbranch} );
+
+                my $local_holds_priority_item_branchcode =
+                  $item->{$LocalHoldsPriorityItemControl};
+
+                my $local_holds_priority_patron_branchcode =
+                  ( $LocalHoldsPriorityPatronControl eq 'PickupLibrary' )
+                  ? $request->{branchcode}
+                  : ( $LocalHoldsPriorityPatronControl eq 'HomeLibrary' )
+                  ? $request->{borrowerbranch}
+                  : undef;
+
+                $local_hold_match =
+                  $local_holds_priority_item_branchcode eq
+                  $local_holds_priority_patron_branchcode;
+
+                if ($local_hold_match) {
+                    if ( exists $items_by_itemnumber{ $item->{itemnumber} }
+                        and not exists $allocated_items{ $item->{itemnumber} } )
+                    {
+                        $item_map{ $item->{itemnumber} } = {
+                            borrowernumber => $request->{borrowernumber},
+                            biblionumber   => $request->{biblionumber},
+                            holdingbranch  => $item->{holdingbranch},
+                            pickup_branch  => $request->{branchcode}
+                              || $request->{borrowerbranch},
+                            item_level   => 0,
+                            reservedate  => $request->{reservedate},
+                            reservenotes => $request->{reservenotes},
+                        };
+                        $allocated_items{ $item->{itemnumber} }++;
+                        $num_items_remaining--;
+                    }
+                }
+            }
+        }
+    }
+
     foreach my $request (@$hold_requests) {
         last if $num_items_remaining == 0;
 

diff --git a/t/db_dependent/HoldsQueue.t b/t/db_dependent/HoldsQueue.t
index 0416c24..43043d5 100755 (executable)
--- a/t/db_dependent/HoldsQueue.t
+++ b/t/db_dependent/HoldsQueue.t
@@ -8,7 +8,7 @@
 
 use Modern::Perl;
 
-use Test::More tests => 38;
+use Test::More tests => 42;
 use Data::Dumper;
 
 use C4::Calendar;
@@ -62,6 +62,7 @@ my $itemtype = $builder->build({ source => 'Itemtype', value => { notforloan =>
 #Set up the stage
 # Sysprefs and cost matrix
 C4::Context->set_preference('HoldsQueueSkipClosed', 0);
+C4::Context->set_preference('LocalHoldsPriority', 0);
 $dbh->do("UPDATE systempreferences SET value = ? WHERE variable = 'StaticHoldsQueueWeight'", undef,
          join( ',', @other_branches, $borrower_branchcode, $least_cost_branch_code));
 $dbh->do("UPDATE systempreferences SET value = '0' WHERE variable = 'RandomizeHoldsQueueWeight'");
@@ -332,6 +333,85 @@ $holds_queue = $dbh->selectall_arrayref("SELECT * FROM tmp_holdsqueue", { Slice
 is( scalar( @$holds_queue ), 2, "Holds not filled with items from closed libraries" );
 C4::Context->set_preference( 'HoldsQueueSkipClosed', 0 );
 
+## Test LocalHoldsPriority
+C4::Context->set_preference('LocalHoldsPriority', 1);
+
+$dbh->do("DELETE FROM default_circ_rules");
+$dbh->do("INSERT INTO default_circ_rules ( holdallowed ) VALUES ( 2 )");
+$dbh->do("DELETE FROM issues");
+
+# Test homebranch = patron branch
+C4::Context->set_preference('LocalHoldsPriorityPatronControl', 'HomeLibrary');
+C4::Context->set_preference('LocalHoldsPriorityItemControl', 'homebranch');
+C4::Context->clear_syspref_cache();
+$dbh->do("DELETE FROM reserves");
+$sth->execute( $borrower1->{borrowernumber}, $biblionumber, $branchcodes[0], 1 );
+$sth->execute( $borrower2->{borrowernumber}, $biblionumber, $branchcodes[0], 2 );
+$sth->execute( $borrower3->{borrowernumber}, $biblionumber, $branchcodes[0], 3 );
+
+$dbh->do("DELETE FROM items");
+# barcode, homebranch, holdingbranch, itemtype
+$items_insert_sth->execute( $barcode + 4, $branchcodes[2], $branchcodes[0] );
+
+C4::HoldsQueue::CreateQueue();
+$holds_queue = $dbh->selectall_arrayref("SELECT * FROM tmp_holdsqueue", { Slice => {} });
+is( $holds_queue->[0]->{cardnumber}, $borrower3->{cardnumber}, "Holds queue giving priority to patron who's home library matches item's home library");
+
+# Test holdingbranch = patron branch
+C4::Context->set_preference('LocalHoldsPriorityPatronControl', 'HomeLibrary');
+C4::Context->set_preference('LocalHoldsPriorityItemControl', 'holdingbranch');
+C4::Context->clear_syspref_cache();
+$dbh->do("DELETE FROM reserves");
+$sth->execute( $borrower1->{borrowernumber}, $biblionumber, $branchcodes[0], 1 );
+$sth->execute( $borrower2->{borrowernumber}, $biblionumber, $branchcodes[0], 2 );
+$sth->execute( $borrower3->{borrowernumber}, $biblionumber, $branchcodes[0], 3 );
+
+$dbh->do("DELETE FROM items");
+# barcode, homebranch, holdingbranch, itemtype
+$items_insert_sth->execute( $barcode + 4, $branchcodes[0], $branchcodes[2] );
+
+C4::HoldsQueue::CreateQueue();
+$holds_queue = $dbh->selectall_arrayref("SELECT * FROM tmp_holdsqueue", { Slice => {} });
+is( $holds_queue->[0]->{cardnumber}, $borrower3->{cardnumber}, "Holds queue giving priority to patron who's home library matches item's holding library");
+
+# Test holdingbranch = pickup branch
+C4::Context->set_preference('LocalHoldsPriorityPatronControl', 'PickupLibrary');
+C4::Context->set_preference('LocalHoldsPriorityItemControl', 'holdingbranch');
+C4::Context->clear_syspref_cache();
+$dbh->do("DELETE FROM reserves");
+$sth->execute( $borrower1->{borrowernumber}, $biblionumber, $branchcodes[0], 1 );
+$sth->execute( $borrower2->{borrowernumber}, $biblionumber, $branchcodes[0], 2 );
+$sth->execute( $borrower3->{borrowernumber}, $biblionumber, $branchcodes[2], 3 );
+
+$dbh->do("DELETE FROM items");
+# barcode, homebranch, holdingbranch, itemtype
+$items_insert_sth->execute( $barcode + 4, $branchcodes[0], $branchcodes[2] );
+
+C4::HoldsQueue::CreateQueue();
+$holds_queue = $dbh->selectall_arrayref("SELECT * FROM tmp_holdsqueue", { Slice => {} });
+is( $holds_queue->[0]->{cardnumber}, $borrower3->{cardnumber}, "Holds queue giving priority to patron who's home library matches item's holding library");
+
+# Test homebranch = pickup branch
+C4::Context->set_preference('LocalHoldsPriorityPatronControl', 'PickupLibrary');
+C4::Context->set_preference('LocalHoldsPriorityItemControl', 'homebranch');
+C4::Context->clear_syspref_cache();
+$dbh->do("DELETE FROM reserves");
+$sth->execute( $borrower1->{borrowernumber}, $biblionumber, $branchcodes[0], 1 );
+$sth->execute( $borrower2->{borrowernumber}, $biblionumber, $branchcodes[0], 2 );
+$sth->execute( $borrower3->{borrowernumber}, $biblionumber, $branchcodes[2], 3 );
+
+$dbh->do("DELETE FROM items");
+# barcode, homebranch, holdingbranch, itemtype
+$items_insert_sth->execute( $barcode + 4, $branchcodes[2], $branchcodes[0] );
+
+C4::HoldsQueue::CreateQueue();
+$holds_queue = $dbh->selectall_arrayref("SELECT * FROM tmp_holdsqueue", { Slice => {} });
+is( $holds_queue->[0]->{cardnumber}, $borrower3->{cardnumber}, "Holds queue giving priority to patron who's home library matches item's holding library");
+
+C4::Context->set_preference('LocalHoldsPriority', 0);
+## End testing of LocalHoldsPriority
+
+
 # Bug 14297
 $itemtype = $builder->build({ source => 'Itemtype', value => { notforloan => 0 } })->{itemtype};
 $borrowernumber = $borrower3->{borrowernumber};