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 <andrew@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
(cherry picked from commit 0bfe336c7b)

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>

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});
     }
 }
 

C4/Reserves.pm

@@ -1655,11 +1655,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
@@ -1690,7 +1690,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

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");
+}

installer/data/mysql/kohastructure.sql

@@ -3900,6 +3900,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`)

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);
@@ -1152,7 +1152,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' );
 
 };