From 43418ae9f088f023b1d398ab30faaebb9c00cbf5 Mon Sep 17 00:00:00 2001 From: Kyle M Hall Date: Thu, 4 Feb 2016 19:41:57 +0000 Subject: [PATCH] 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 Signed-off-by: Martin Renvoize Signed-off-by: Brendan Gallagher --- C4/HoldsQueue.pm | 54 ++++++++++++++++++++++++ t/db_dependent/HoldsQueue.t | 82 ++++++++++++++++++++++++++++++++++++- 2 files changed, 135 insertions(+), 1 deletion(-) diff --git a/C4/HoldsQueue.pm b/C4/HoldsQueue.pm index 1a2de1de81..e5f29c72bc 100755 --- 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 0416c243df..43043d5a52 100755 --- 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}; -- 2.39.5