@@ -, +, @@
build_holds_queue.pl -script.
---
C4/HoldsQueue.pm | 56 +++-
.../HoldsQueue_UseBranchTransferLimits.t | 326 +++++++++++++++++++++
2 files changed, 372 insertions(+), 10 deletions(-)
create mode 100755 t/db_dependent/HoldsQueue_UseBranchTransferLimits.t
--- a/C4/HoldsQueue.pm
+++ a/C4/HoldsQueue.pm
@@ -90,7 +90,7 @@ sub UpdateTransportCostMatrix {
my $sth = $dbh->prepare("INSERT INTO transport_cost (frombranch, tobranch, cost, disable_transfer) VALUES (?, ?, ?, ?)");
- $dbh->do("TRUNCATE TABLE transport_cost");
+ $dbh->do("DELETE FROM transport_cost"); #Deleting is slower than TRUNCATE, but using TRUNCATE screws up $dbh->{AutoCommit} in unit tests
foreach (@$records) {
my $cost = $_->{cost};
my $from = $_->{frombranch};
@@ -306,7 +306,7 @@ sub GetItemsAvailableToFillHoldRequestsForBib {
my ($biblionumber, $branches_to_use) = @_;
my $dbh = C4::Context->dbh;
- my $items_query = "SELECT itemnumber, homebranch, holdingbranch, itemtypes.itemtype AS itype
+ my $items_query = "SELECT itemnumber, homebranch, holdingbranch, ccode, itemtypes.itemtype AS itype
FROM items ";
if (C4::Context->preference('item-level_itypes')) {
@@ -386,7 +386,19 @@ sub MapItemsToHoldRequests {
# fill it if possible; if not skip it
if (exists $items_by_itemnumber{$request->{itemnumber}} and
not exists $allocated_items{$request->{itemnumber}}) {
- $item_map{$request->{itemnumber}} = {
+
+ #Dereferencing variables for sanitys sake, and because it is faster when repeatedly called.
+ my $holdingBranch = $items_by_itemnumber{$request->{itemnumber}}->{holdingbranch};
+ my $pickupBranch = $request->{branchcode} || $request->{borrowerbranch};
+
+ my ($transferOk, $errorMsg) = CheckBranchTransferAllowed( $pickupBranch, $holdingBranch, $items_by_itemnumber{$request->{itemnumber}}, undef);
+ if (! $transferOk) {
+ #UseBranchTransfer check failed, gosh damn how can this be! It should be blocked in the UI for item level holds!
+ warn "HoldsQueue.pm:> Branch transfer failed for pickup branch $pickupBranch, holding branch $holdingBranch, itemnumber $request->{itemnumber}\n" .
+ "This should be impossible because branch transfer limits for item level holds must be checked when placing holds!";
+ }
+ else {
+ $item_map{$request->{itemnumber}} = {
borrowernumber => $request->{borrowernumber},
biblionumber => $request->{biblionumber},
holdingbranch => $items_by_itemnumber{$request->{itemnumber}}->{holdingbranch},
@@ -394,9 +406,10 @@ sub MapItemsToHoldRequests {
item_level => 1,
reservedate => $request->{reservedate},
reservenotes => $request->{reservenotes},
- };
- $allocated_items{$request->{itemnumber}}++;
- $num_items_remaining--;
+ };
+ $allocated_items{$request->{itemnumber}}++;
+ $num_items_remaining--;
+ }
}
} else {
# it's title-level request that will take up one item
@@ -424,7 +437,10 @@ sub MapItemsToHoldRequests {
# look for local match first
my $pickup_branch = $request->{branchcode} || $request->{borrowerbranch};
my ($itemnumber, $holdingbranch);
-
+
+ # Used to detect if no Items can be transferred to the pickup location.
+ my $branchTransfersAllowedCount = 1; #Default this to 1. It is decremented when branchTransfers fail.
+
my $holding_branch_items = $items_by_branch{$pickup_branch};
if ( $holding_branch_items ) {
foreach my $item (@$holding_branch_items) {
@@ -442,21 +458,41 @@ sub MapItemsToHoldRequests {
if ( $holdingbranch ) {
my $holding_branch_items = $items_by_branch{$holdingbranch};
+ $branchTransfersAllowedCount = @$holding_branch_items;
+ # Making sure to put preference to the Item, which is originally from the Borrowers homebranch.
foreach my $item (@$holding_branch_items) {
next if $request->{borrowerbranch} ne $item->{homebranch};
$itemnumber = $item->{itemnumber};
last;
}
- $itemnumber ||= $holding_branch_items->[0]->{itemnumber};
+ # Checking if the Item can be Transferred to the pickup library
+ if (! $itemnumber && C4::Context->preference("UseBranchTransferLimits") == 1 ) {
+ foreach my $item (@$holding_branch_items) {
+ my ($transferOk, $errorMsg) = C4::Circulation::CheckBranchTransferAllowed($pickup_branch, $item->{holdingbranch}, $item, undef);
+ if (! $transferOk) {
+ $branchTransfersAllowedCount--;
+ next;
+ }
+
+ $itemnumber = $item->{itemnumber};
+ last;
+ }
+ }
+ # If one is not using UseBranchTransferLimits, then revert to the default setting.
+ elsif (! $itemnumber) {
+ # FIXME Choosing the Item based on the first available, even if it could be on hold.
+ # Problematic since holds tend to stack on this first item then.
+ $itemnumber ||= $holding_branch_items->[0]->{itemnumber};
+ }
}
else {
warn "No transport costs for $pickup_branch";
}
}
- unless ($itemnumber) {
- # not found yet, fall back to basics
+ if ((! $itemnumber) && $branchTransfersAllowedCount > 0) {
+ # not found yet and branchTransfers are not blocking, fall back to basics
if ($branches_to_use) {
$pull_branches = $branches_to_use;
} else {
--- a/t/db_dependent/HoldsQueue_UseBranchTransferLimits.t
+++ a/t/db_dependent/HoldsQueue_UseBranchTransferLimits.t
@@ -0,0 +1,326 @@
+#!/usr/bin/perl
+
+=head TESTING PLAN
+
+PRECONDITIONS|
+-------------+
+UseBranchTranferLimits itemtype DVD
+IPT -> CPL -> FFL -> IPT
+no transfers into LPL
+
+TransportCostMatrix in use
+Allow all, cost 1
+
+-> pickup branch IPT <-
+-> Borrowers homebranch LPL <-
+
+
+Biblio1 Items
+Item barcode holdingbranch transferOK? itemtype
+cpl11 CPL 0 DVD
+cpl12 CPL 1 BK
+ffl11 FFL 1 DVD
+ffl12 FFL 1 BK
+
+Biblio2 Items
+Item barcode holdingbranch transferOK? itemtype
+cpl21 CPL 0 DVD
+cpl22 CPL 0 DVD
+
+Biblio3 Items
+Item barcode holdingbranch transferOK? itemtype
+ffl31 FFL 1 DVD
+ffl32 FFL 1 BK
+
+---------------------
+--- HoldsQueue.pm ---
+---------------------
+
+CASE1: Title-level multi_hold on all Biblios, pickup location IPT
+
+Expected:
+Hold for Biblio1 gets satisfied with Items cpl12, ipt11, ipt12 or ffl1.
+Hold for Biblio2 fails to satisfy, no messages given.
+Hold for Biblio3 is satisfied with ffl31.
+
+
+CASE2: Item-level holds, pickup location IPT
+
+Expected:
+Item to hold hold satisfied?
+cpl1 0
+cpl12 1
+ipt11 1
+ipt12 1
+ffl1 1
+cpl21 0
+cpl22 0
+cpl23 1
+ffl31 1
+ffl32 1
+
+
+=cut
+
+use Modern::Perl;
+
+use C4::Context;
+
+use Test::More tests => 9;
+
+use C4::Circulation;
+use C4::Record;
+use C4::HoldsQueue;
+use C4::Reserves;
+use C4::HoldsQueue;
+
+# Start transaction
+my $dbh = C4::Context->dbh;
+$dbh->{AutoCommit} = 0;
+$dbh->{RaiseError} = 1;
+
+
+##########################xxxxxxxxxxxxxxxxxxxxxxxxx###########################
+### xxxxxxxxxxxxxxxxxxxxxxxxxx Set up the stage xxxxxxxxxxxxxxxxxxxxxxxxxx ###
+##########################xxxxxxxxxxxxxxxxxxxxxxxxx###########################
+
+## Set branches
+my $desiredBranches = ['IPT', 'CPL', 'FFL', 'LPL'];
+my $branches = C4::Branch::GetBranches;
+
+foreach my $branchCode (@$desiredBranches) {
+ if (exists $branches->{$branchCode}) {
+ #Alls well and the library is here yay!
+ }
+ else {
+ #le fuu! who is not using the default libraries?
+ my $branchwhatever = C4::SQLHelper::InsertInTable("branches",{branchcode=>"$branchCode",branchname=>"Yet another branch",city=>" ",zipcode=>" "},1);
+ print $branchwhatever;
+ }
+}
+
+my %data = (
+ cardnumber => 'CARDNUMBER42',
+ firstname => 'my firstname',
+ surname => 'my surname',
+ categorycode => 'S',
+ branchcode => 'LPL',
+);
+
+my $borrowernumber = C4::Members::AddMember(%data);
+my $borrower = C4::Members::GetMember( borrowernumber => $borrowernumber );
+
+
+## Itemtypes
+my $itemTypes = { map {$_->{itemtype} => 1} C4::ItemType->all() };
+if (! (exists $itemTypes->{'BK'})) {
+ $dbh->do("INSERT INTO `kohadata`.`itemtypes` (`itemtype`, `description`, `rentalcharge`, `notforloan`, `imageurl`, `summary`, `checkinmsg`, `checkinmsgtype`) VALUES ('BK', NULL, '0', NULL, NULL, NULL, NULL, 'message');");
+}
+if (! (exists $itemTypes->{'DVD'})) {
+ $dbh->do("INSERT INTO `kohadata`.`itemtypes` (`itemtype`, `description`, `rentalcharge`, `notforloan`, `imageurl`, `summary`, `checkinmsg`, `checkinmsgtype`) VALUES ('DVD', NULL, '0', NULL, NULL, NULL, NULL, 'message');");
+}
+
+
+
+## Add example Bibliographic records
+
+my ( $biblioNumber1, $biblioNumber2, $biblioNumber3, $discardBiblioitemnumber);
+my $bibFramework = ''; #Using the default bibliographic framework.
+
+my $marcxml=qq(
+
+ 00000cim a22000004a 4500
+ ss||||j|||||||
+ uuuu xxk|||||||||||||||||eng|c
+
+ BIBLIO1
+
+
+);
+my $record=C4::Record::marcxml2marc($marcxml);
+( $biblioNumber1, $discardBiblioitemnumber ) = C4::Biblio::AddBiblio( $record, $bibFramework, { defer_marc_save => 1 } );
+
+$marcxml=qq(
+
+ 00000cim a22000004a 4500
+ ss||||j|||||||
+ uuuu xxk|||||||||||||||||eng|c
+
+ BIBLIO2
+
+
+);
+$record=C4::Record::marcxml2marc($marcxml);
+( $biblioNumber2, $discardBiblioitemnumber ) = C4::Biblio::AddBiblio( $record, $bibFramework, { defer_marc_save => 1 } );
+
+$marcxml=qq(
+
+ 00000cim a22000004a 4500
+ ss||||j|||||||
+ uuuu xxk|||||||||||||||||eng|c
+
+ BIBLIO3
+
+
+);
+$record=C4::Record::marcxml2marc($marcxml);
+( $biblioNumber3, $discardBiblioitemnumber ) = C4::Biblio::AddBiblio( $record, $bibFramework, { defer_marc_save => 1 } );
+
+## Add items to the mix
+my ($discardThis, $discardThat); # Not needing biblionumber or biblioitemnumber for the added items
+my ($cpl11, $cpl12, $ffl11, $ffl12, $cpl21, $cpl22, $ffl31, $ffl32);
+
+($discardThis, $discardThat, $cpl11) = C4::Items::AddItem({ barcode => 'cpl11', homebranch => 'CPL', holdingbranch => 'CPL', itype => 'DVD', notforloan => '0' }, $biblioNumber1);
+($discardThis, $discardThat, $cpl12) = C4::Items::AddItem({ barcode => 'cpl12', homebranch => 'CPL', holdingbranch => 'CPL', itype => 'BK' , notforloan => '0' }, $biblioNumber1);
+($discardThis, $discardThat, $ffl11) = C4::Items::AddItem({ barcode => 'ffl11', homebranch => 'FFL', holdingbranch => 'FFL', itype => 'DVD', notforloan => '0' }, $biblioNumber1);
+($discardThis, $discardThat, $ffl12) = C4::Items::AddItem({ barcode => 'ffl12', homebranch => 'FFL', holdingbranch => 'FFL', itype => 'BK' , notforloan => '0' }, $biblioNumber1);
+
+($discardThis, $discardThat, $cpl21) = C4::Items::AddItem({ barcode => 'cpl21', homebranch => 'CPL', holdingbranch => 'CPL', itype => 'DVD', notforloan => '0' }, $biblioNumber2);
+($discardThis, $discardThat, $cpl22) = C4::Items::AddItem({ barcode => 'cpl22', homebranch => 'CPL', holdingbranch => 'CPL', itype => 'DVD', notforloan => '0' }, $biblioNumber2);
+
+($discardThis, $discardThat, $ffl31) = C4::Items::AddItem({ barcode => 'ffl31', homebranch => 'FFL', holdingbranch => 'FFL', itype => 'DVD', notforloan => '0' }, $biblioNumber3);
+($discardThis, $discardThat, $ffl32) = C4::Items::AddItem({ barcode => 'ffl32', homebranch => 'FFL', holdingbranch => 'FFL', itype => 'BK' , notforloan => '0' }, $biblioNumber3);
+
+
+## Sysprefs and cost matrix
+C4::Context->set_preference('UseTransportCostMatrix', 1);
+C4::Context->set_preference('item-level_itypes', 1);
+C4::Context->set_preference("BranchTransferLimitsType", 'itemtype');
+C4::Context->set_preference('StaticHoldsQueueWeight', 0);
+C4::Context->set_preference('RandomizeHoldsQueueWeight', 0);
+
+
+C4::HoldsQueue::UpdateTransportCostMatrix( [
+ {frombranch => 'IPT', tobranch => 'CPL', cost => 1, disable_transfer => 0},
+ {frombranch => 'IPT', tobranch => 'FFL', cost => 1, disable_transfer => 0},
+ {frombranch => 'IPT', tobranch => 'LPL', cost => 1, disable_transfer => 0},
+ {frombranch => 'CPL', tobranch => 'FFL', cost => 1, disable_transfer => 0},
+ {frombranch => 'CPL', tobranch => 'IPT', cost => 0.2, disable_transfer => 0},
+ {frombranch => 'CPL', tobranch => 'LPL', cost => 1, disable_transfer => 0},
+ {frombranch => 'FFL', tobranch => 'IPT', cost => 1, disable_transfer => 0},
+ {frombranch => 'FFL', tobranch => 'CPL', cost => 1, disable_transfer => 0},
+ {frombranch => 'FFL', tobranch => 'LPL', cost => 1, disable_transfer => 0},
+] );
+
+
+C4::Circulation::DeleteBranchTransferLimits( 'IPT' );
+C4::Circulation::DeleteBranchTransferLimits( 'CPL' );
+C4::Circulation::DeleteBranchTransferLimits( 'FFL' );
+C4::Circulation::DeleteBranchTransferLimits( 'LPL' );
+# CreateBranchTransferLimit( to , from, code ); Block these transfers!
+C4::Circulation::CreateBranchTransferLimit( 'IPT', 'CPL', 'DVD' );
+C4::Circulation::CreateBranchTransferLimit( 'CPL', 'FFL', 'DVD' );
+C4::Circulation::CreateBranchTransferLimit( 'FFL', 'IPT', 'DVD' );
+C4::Circulation::CreateBranchTransferLimit( 'LPL', 'IPT', 'DVD' );
+C4::Circulation::CreateBranchTransferLimit( 'LPL', 'CPL', 'DVD' );
+C4::Circulation::CreateBranchTransferLimit( 'LPL', 'FFL', 'DVD' );
+
+
+#############################xxxxxxxxxxxxxxxxxxxxxxxxx###########################
+### xxxxxxxxxxxxxxxxxxxxxxxxxx Start running tests xxxxxxxxxxxxxxxxxxxxxxxxxx ###
+#############################xxxxxxxxxxxxxxxxxxxxxxxxx###########################
+
+
+ #############
+### CASE1: Title-level multi_hold on all Biblios, pickup location IPT ###
+ #############
+
+## Remove existing reserves, makes debugging easier
+$dbh->do("DELETE FROM reserves");
+
+C4::Reserves::AddReserve ( 'IPT', $borrowernumber, $biblioNumber1, 'a', '', '1' );
+C4::Reserves::AddReserve ( 'IPT', $borrowernumber, $biblioNumber2, 'a', '', '1' );
+C4::Reserves::AddReserve ( 'IPT', $borrowernumber, $biblioNumber3, 'a', '', '1' );
+
+$dbh->do("UPDATE reserves SET reservedate = DATE_SUB( reservedate, INTERVAL 1 DAY )");
+
+## Item cpl11 is a DVD and transferring it from CPL to IPT is strictly forbidden.
+## Transport matrix favours CPL -> IPT transports, so we should get either the BK or DVD.
+my $testOk = 0;
+my $branches_to_use = undef; #Testing only with Transport cost matrix since it is the future!
+my $transport_cost_matrix = C4::HoldsQueue::TransportCostMatrix();
+my $hold_requests = C4::HoldsQueue::GetPendingHoldRequestsForBib($biblioNumber1);
+my $available_items = C4::HoldsQueue::GetItemsAvailableToFillHoldRequestsForBib($biblioNumber1, $branches_to_use);
+my $item_map = C4::HoldsQueue::MapItemsToHoldRequests($hold_requests, $available_items, $branches_to_use, $transport_cost_matrix);
+my $rib = (C4::Items::GetItem( (keys %$item_map)[0] ))->{barcode}; #Reserved Item's barcode
+$testOk = 1 if ($rib eq 'cpl12');
+is($testOk, 1, 'Title-level hold, mixed material, Biblio1');
+=head
+## Should fail, requesting DVD's from a transfer-denied branch.
+$testOk = 0;
+$branches_to_use = undef; #Testing only with Transport cost matrix since it is the future!
+$transport_cost_matrix = C4::HoldsQueue::TransportCostMatrix();
+$hold_requests = C4::HoldsQueue::GetPendingHoldRequestsForBib($biblioNumber2);
+$available_items = C4::HoldsQueue::GetItemsAvailableToFillHoldRequestsForBib($biblioNumber2, $branches_to_use);
+$item_map = C4::HoldsQueue::MapItemsToHoldRequests($hold_requests, $available_items, $branches_to_use, $transport_cost_matrix);
+$testOk = %$item_map; #This time we should have no results
+is($testOk, 0, 'Title-level hold, transfer denied, Biblio2');
+
+## Should succeed, no transfer blocks
+$testOk = 0;
+$branches_to_use = undef; #Testing only with Transport cost matrix since it is the future!
+$transport_cost_matrix = C4::HoldsQueue::TransportCostMatrix();
+$hold_requests = C4::HoldsQueue::GetPendingHoldRequestsForBib($biblioNumber3);
+$available_items = C4::HoldsQueue::GetItemsAvailableToFillHoldRequestsForBib($biblioNumber3, $branches_to_use);
+$item_map = C4::HoldsQueue::MapItemsToHoldRequests($hold_requests, $available_items, $branches_to_use, $transport_cost_matrix);
+$testOk = keys %$item_map; #This time any result will do
+is($testOk, 1, 'Title-level hold, transfer allowed, Biblio3');
+=cut
+
+
+
+ #############
+### CASE2: Item-level holds, pickup location IPT ###
+ #############
+
+## Remove existing reserves, makes debugging easier
+$dbh->do("DELETE FROM reserves");
+
+#$branch, $borrowernumber, $biblionumber, $constraint, $bibitems, $priority, $resdate, $expdate, $notes, $title, $checkitem, $found
+C4::Reserves::AddReserve ( 'IPT', $borrowernumber, $biblioNumber1, undef, undef, '1', undef, undef, undef, undef, $cpl11, undef );
+C4::Reserves::AddReserve ( 'IPT', $borrowernumber, $biblioNumber1, undef, undef, '1', undef, undef, undef, undef, $cpl12, undef );
+C4::Reserves::AddReserve ( 'IPT', $borrowernumber, $biblioNumber1, undef, undef, '1', undef, undef, undef, undef, $ffl11, undef );
+C4::Reserves::AddReserve ( 'IPT', $borrowernumber, $biblioNumber1, undef, undef, '1', undef, undef, undef, undef, $ffl12, undef );
+
+C4::Reserves::AddReserve ( 'IPT', $borrowernumber, $biblioNumber2, undef, undef, '1', undef, undef, undef, undef, $cpl21, undef );
+C4::Reserves::AddReserve ( 'IPT', $borrowernumber, $biblioNumber2, undef, undef, '1', undef, undef, undef, undef, $cpl22, undef );
+
+C4::Reserves::AddReserve ( 'IPT', $borrowernumber, $biblioNumber3, undef, undef, '1', undef, undef, undef, undef, $ffl31, undef );
+C4::Reserves::AddReserve ( 'IPT', $borrowernumber, $biblioNumber3, undef, undef, '1', undef, undef, undef, undef, $ffl32, undef );
+
+$dbh->do("UPDATE reserves SET reservedate = DATE_SUB( reservedate, INTERVAL 1 DAY )");
+
+## Transfer of cpl11 should be blocked, other 3 Items are mapped
+$testOk = 0;
+$branches_to_use = undef; #Testing only with Transport cost matrix since it is the future!
+$transport_cost_matrix = C4::HoldsQueue::TransportCostMatrix();
+$hold_requests = C4::HoldsQueue::GetPendingHoldRequestsForBib($biblioNumber1);
+$available_items = C4::HoldsQueue::GetItemsAvailableToFillHoldRequestsForBib($biblioNumber1, $branches_to_use);
+$item_map = C4::HoldsQueue::MapItemsToHoldRequests($hold_requests, $available_items, $branches_to_use, $transport_cost_matrix);
+is( (exists $item_map->{$cpl11}) , '', 'Item cpl11 transfer denied');
+is( (exists $item_map->{$cpl12}) , 1, 'Item cpl12 transfer allowed');
+is( (exists $item_map->{$ffl11}) , 1, 'Item cpl11 transfer allowed');
+is( (exists $item_map->{$ffl12}) , 1, 'Item cpl11 transfer allowed');
+
+## All items should fail their transfer
+$testOk = 0;
+$branches_to_use = undef; #Testing only with Transport cost matrix since it is the future!
+$transport_cost_matrix = C4::HoldsQueue::TransportCostMatrix();
+$hold_requests = C4::HoldsQueue::GetPendingHoldRequestsForBib($biblioNumber2);
+$available_items = C4::HoldsQueue::GetItemsAvailableToFillHoldRequestsForBib($biblioNumber2, $branches_to_use);
+$item_map = C4::HoldsQueue::MapItemsToHoldRequests($hold_requests, $available_items, $branches_to_use, $transport_cost_matrix);
+is( (exists $item_map->{$cpl21}) , '', 'Item cpl21 transfer denied');
+is( (exists $item_map->{$cpl22}) , '', 'Item cpl22 transfer denied');
+
+## Should succeed, no transfer blocks
+$testOk = 0;
+$branches_to_use = undef; #Testing only with Transport cost matrix since it is the future!
+$transport_cost_matrix = C4::HoldsQueue::TransportCostMatrix();
+$hold_requests = C4::HoldsQueue::GetPendingHoldRequestsForBib($biblioNumber3);
+$available_items = C4::HoldsQueue::GetItemsAvailableToFillHoldRequestsForBib($biblioNumber3, $branches_to_use);
+$item_map = C4::HoldsQueue::MapItemsToHoldRequests($hold_requests, $available_items, $branches_to_use, $transport_cost_matrix);
+is( (exists $item_map->{$ffl31}) , 1, 'Item ffl31 transfer allowed');
+is( (exists $item_map->{$ffl32}) , 1, 'Item ffl32 transfer allowed');
+
+# Cleanup
+$dbh->rollback();
--