From 0bfe336c7b0a8993411bd45b9e7c66228730f86d 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 Signed-off-by: Marcel de Rooy Signed-off-by: Jonathan Druart Signed-off-by: Jonathan Druart --- 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 4950522726..08f8a4eb94 100644 --- 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) @@ -441,6 +441,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}, @@ -720,15 +721,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 8d7dcbc611..2422d4facd 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -1664,11 +1664,11 @@ sub _Findgroupreserve { reserves.non_priority AS non_priority 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 @@ -1700,7 +1700,7 @@ sub _Findgroupreserve { reserves.non_priority AS non_priority 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 9dff2f804b..48d75520a8 100644 --- a/installer/data/mysql/kohastructure.sql +++ b/installer/data/mysql/kohastructure.sql @@ -3980,6 +3980,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 f1cdd065f8..c1c4758c86 100755 --- a/t/db_dependent/Reserves.t +++ b/t/db_dependent/Reserves.t @@ -1023,7 +1023,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); @@ -1149,7 +1149,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.39.5