From e7e51fb37cc3c67bde1534a1d3382c16a0e6084c Mon Sep 17 00:00:00 2001 From: Nick Clemens Date: Fri, 14 Aug 2020 11:06:40 +0000 Subject: [PATCH] Bug 18958: Make hold_fill_targets specific to reserves After looking at Marcel's comments, the problem is in our matching to hold_fill_targets - rather than adjusting to find filled/waiting holds we could ensure that hold_fill_targets only refers to the specific hold it is intended to This patch is clearer, if slightly less performant than last (we now return all the reserves and have to find the 'highest') Test Plan: 1 - Create and use a patron that can place multiple record level holds per record 2 - Create a record with X items, each at a different library 3 - Place X 'Next available' holds on the record for the patron using the 'Holds to place' box 4 - perl misc/cronjobs/holds/build_holdsqueue.pl 5 - Check in LibraryA's copy as LibraryA and confirm the hold 6 - Revisit request.pl for the record, notice the next hold in line is now item-specific 7 - Checkout the item to the patron, notice the remaining hold is marked waiting 8 - Attempt to place another hold for your patron, notice that it requires an item-specific hold 8 - Apply this patch 9 - Repeat steps 1-5 10 - Revisit request.pl for the record, notice the next hold in line has *not* become item-specific 11 - Checkout the item to the patron, ensure the first hold is filled and the second remains record level 12 - Repeat whole test plan without building holds queue to confirm holds are still treated correctly Signed-off-by: Andrew Fuerste-Henry --- C4/HoldsQueue.pm | 9 +++++---- C4/Reserves.pm | 6 +++--- .../bug_18958_add_reserve_id_to_hold_fill_targets.perl | 9 +++++++++ installer/data/mysql/kohastructure.sql | 1 + t/db_dependent/Reserves.t | 4 ++-- 5 files changed, 20 insertions(+), 9 deletions(-) create mode 100644 installer/data/mysql/atomicupdate/bug_18958_add_reserve_id_to_hold_fill_targets.perl diff --git a/C4/HoldsQueue.pm b/C4/HoldsQueue.pm index 5938d35d2a..5bda36db9e 100755 --- a/C4/HoldsQueue.pm +++ b/C4/HoldsQueue.pm @@ -276,7 +276,7 @@ sub GetPendingHoldRequestsForBib { my $dbh = C4::Context->dbh; - my $request_query = "SELECT biblionumber, borrowernumber, itemnumber, priority, reserves.branchcode, + my $request_query = "SELECT biblionumber, borrowernumber, itemnumber, priority, reserve_id, reserves.branchcode, reservedate, reservenotes, borrowers.branchcode AS borrowerbranch, itemtype, item_level_hold FROM reserves JOIN borrowers USING (borrowernumber) @@ -438,6 +438,7 @@ sub MapItemsToHoldRequests { holdingbranch => $item->{holdingbranch}, pickup_branch => $request->{branchcode} || $request->{borrowerbranch}, + reserve_id => $request->{reserve_id}, item_level => $request->{item_level_hold}, reservedate => $request->{reservedate}, reservenotes => $request->{reservenotes}, @@ -717,15 +718,15 @@ sub AddToHoldTargetMap { my $dbh = C4::Context->dbh; my $insert_sql = q( - INSERT INTO hold_fill_targets (borrowernumber, biblionumber, itemnumber, source_branchcode, item_level_request) - VALUES (?, ?, ?, ?, ?) + INSERT INTO hold_fill_targets (borrowernumber, biblionumber, itemnumber, source_branchcode, item_level_request, reserve_id) + VALUES (?, ?, ?, ?, ?, ?) ); my $sth_insert = $dbh->prepare($insert_sql); foreach my $itemnumber (keys %$item_map) { my $mapped_item = $item_map->{$itemnumber}; $sth_insert->execute($mapped_item->{borrowernumber}, $mapped_item->{biblionumber}, $itemnumber, - $mapped_item->{holdingbranch}, $mapped_item->{item_level}); + $mapped_item->{holdingbranch}, $mapped_item->{item_level}, $mapped_item->{reserve_id}); } } diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 950d29f587..9c2049745a 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -1652,11 +1652,11 @@ sub _Findgroupreserve { reserves.itemtype AS itemtype FROM reserves JOIN biblioitems USING (biblionumber) - JOIN hold_fill_targets USING (biblionumber, borrowernumber, itemnumber) + JOIN hold_fill_targets USING (reserve_id) WHERE found IS NULL AND priority > 0 AND item_level_request = 1 - AND itemnumber = ? + AND hold_fill_targets.itemnumber = ? AND reservedate <= DATE_ADD(NOW(),INTERVAL ? DAY) AND suspend = 0 ORDER BY priority @@ -1687,7 +1687,7 @@ sub _Findgroupreserve { reserves.itemtype AS itemtype FROM reserves JOIN biblioitems USING (biblionumber) - JOIN hold_fill_targets USING (biblionumber, borrowernumber) + JOIN hold_fill_targets USING (reserve_id) WHERE found IS NULL AND priority > 0 AND item_level_request = 0 diff --git a/installer/data/mysql/atomicupdate/bug_18958_add_reserve_id_to_hold_fill_targets.perl b/installer/data/mysql/atomicupdate/bug_18958_add_reserve_id_to_hold_fill_targets.perl new file mode 100644 index 0000000000..4849771c51 --- /dev/null +++ b/installer/data/mysql/atomicupdate/bug_18958_add_reserve_id_to_hold_fill_targets.perl @@ -0,0 +1,9 @@ +$DBversion = 'XXX'; # will be replaced by the RM +if( CheckVersion( $DBversion ) ) { + + if( !column_exists( 'hold_fill_targets', 'reserve_id' ) ) { + $dbh->do( "ALTER TABLE hold_fill_targets ADD COLUMN reserve_id int(11) DEFAULT NULL AFTER item_level_request" ); + } + + NewVersion( $DBversion, 18958, "Add reserve_id to hold_fill_targets"); +} diff --git a/installer/data/mysql/kohastructure.sql b/installer/data/mysql/kohastructure.sql index 27bb117b31..4a4d6de675 100644 --- a/installer/data/mysql/kohastructure.sql +++ b/installer/data/mysql/kohastructure.sql @@ -3970,6 +3970,7 @@ CREATE TABLE `hold_fill_targets` ( `itemnumber` int(11) NOT NULL, `source_branchcode` varchar(10) default NULL, `item_level_request` tinyint(4) NOT NULL default 0, + `reserve_id` int(11) DEFAULT NULL PRIMARY KEY `itemnumber` (`itemnumber`), KEY `bib_branch` (`biblionumber`, `source_branchcode`), CONSTRAINT `hold_fill_targets_ibfk_1` FOREIGN KEY (`borrowernumber`) diff --git a/t/db_dependent/Reserves.t b/t/db_dependent/Reserves.t index 7b8650689a..d9ab89b99a 100755 --- a/t/db_dependent/Reserves.t +++ b/t/db_dependent/Reserves.t @@ -1026,7 +1026,7 @@ subtest 'MoveReserve additional test' => sub { is($patron_2->holds->next()->reserve_id, $reserve_2, "The 2nd patron has a hold"); # Fake the holds queue - $dbh->do(q{INSERT INTO hold_fill_targets VALUES (?, ?, ?, ?, ?)},undef,($patron_1->borrowernumber,$biblio->biblionumber,$item_1->itemnumber,$item_1->homebranch,0)); + $dbh->do(q{INSERT INTO hold_fill_targets VALUES (?, ?, ?, ?, ?,?)},undef,($patron_1->borrowernumber,$biblio->biblionumber,$item_1->itemnumber,$item_1->homebranch,0,$reserve_1)); # The 2nd hold should be filed even if the item is preselected for the first hold MoveReserve($item_1->itemnumber,$patron_2->borrowernumber); @@ -1139,7 +1139,7 @@ subtest 'CheckReserves additional test' => sub { is( $status, 'Reserved', "We found a reserve" ); is( $matched_reserve->{reserve_id}, $reserve1->reserve_id, "We got the Transit reserve"); - is( scalar @$possible_reserves, 1, 'We only get the one matched'); + is( scalar @$possible_reserves, 2, 'We do get both reserves'); }; -- 2.11.0