From 01931a255ae40a27665e749c515a9d0aabe38174 Mon Sep 17 00:00:00 2001 From: Olli-Antti Kivilahti Date: Tue, 15 Oct 2013 13:46:40 +0300 Subject: [PATCH] Bug 11024 - Transfer limits should be respected in the build_holds_queue.pl -script. Depends on bug 11005 Adds the UseBranchTransferLimits-functionality to HoldsQueue.pm Includes unit tests. --- C4/HoldsQueue.pm | 53 +++- .../HoldsQueue_UseBranchTransferLimits.t | 326 +++++++++++++++++++++ 2 files changed, 371 insertions(+), 8 deletions(-) create mode 100755 t/db_dependent/HoldsQueue_UseBranchTransferLimits.t diff --git a/C4/HoldsQueue.pm b/C4/HoldsQueue.pm index 7062cbe..dfbc0dd 100755 --- a/C4/HoldsQueue.pm +++ b/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 @@ -425,6 +438,9 @@ sub MapItemsToHoldRequests { 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,20 +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; } + # 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::CanItemBeTransferred($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 { diff --git a/t/db_dependent/HoldsQueue_UseBranchTransferLimits.t b/t/db_dependent/HoldsQueue_UseBranchTransferLimits.t new file mode 100755 index 0000000..2612a6a --- /dev/null +++ b/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(); \ No newline at end of file -- 1.8.1.2