From f6793697737ec08802cfe9a7f224767e0d0d89fb 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 (cherry picked from commit 0bfe336c7b0a8993411bd45b9e7c66228730f86d) Signed-off-by: Lucas Gass (cherry picked from commit a120656eca586ecd2005421e37fbf8485131ea45) Signed-off-by: Aleisha Amohia --- C4/HoldsQueue.pm | 9 +++++---- C4/Reserves.pm | 6 +++--- .../bug_18958_add_reserve_id_to_hold_fill_targets.perl | 10 ++++++++++ installer/data/mysql/kohastructure.sql | 1 + t/db_dependent/Reserves.t | 4 ++-- 5 files changed, 21 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 5bd3c76600..2a017ed1cb 100755 --- a/C4/HoldsQueue.pm +++ b/C4/HoldsQueue.pm @@ -279,7 +279,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 2fc184a64e..624bdf0df1 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -1597,11 +1597,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 @@ -1632,7 +1632,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..5e0ff9c10c --- /dev/null +++ b/installer/data/mysql/atomicupdate/bug_18958_add_reserve_id_to_hold_fill_targets.perl @@ -0,0 +1,10 @@ +$DBversion = "XXX"; +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" ); + } + + print "Upgrade to $DBversion done (Bug 18958 - Add reserve_id to hold_fill_targets)\n"; + SetVersion( $DBversion ); +} diff --git a/installer/data/mysql/kohastructure.sql b/installer/data/mysql/kohastructure.sql index fd405ccbcf..1b9a045bb8 100644 --- a/installer/data/mysql/kohastructure.sql +++ b/installer/data/mysql/kohastructure.sql @@ -3882,6 +3882,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 70cb530131..e1f5025bb2 100755 --- a/t/db_dependent/Reserves.t +++ b/t/db_dependent/Reserves.t @@ -866,7 +866,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); @@ -949,7 +949,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