Attachment 108500 Details for Bug 18958 – Bug 18958: Make hold_fill_targets specific to reserves

[patch] Bug 18958: Make hold_fill_targets specific to reserves

Bug-18958-Make-holdfilltargets-specific-to-reserve.patch (text/plain), 7.85 KB, created by Andrew Fuerste-Henry on 2020-08-18 13:03:29 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Andrew Fuerste-Henry

Created: 2020-08-18 13:03:29 UTC

Size: 7.85 KB

patch

obsolete

>From 08f0c72b85b1fbe709dac9346d4dbe28ff9c7bee Mon Sep 17 00:00:00 2001
>From: Nick Clemens <nick@bywatersolutions.com>
>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 <andrew@bywatersolutions.com>
>---
> 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 35c643fa38..4138555adf 100644
>--- a/installer/data/mysql/kohastructure.sql
>+++ b/installer/data/mysql/kohastructure.sql
>@@ -3968,6 +3968,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

Actions: View | Diff | Splinter Review

Attachments on bug 18958: 65104 | 65150 | 65184 | 71164 | 71232 | 72210 | 72211 | 72348 | 72349 | 92159 | 92160 | 93106 | 93107 | 93108 | 93111 | 93112 | 93113 | 93216 | 93217 | 93218 | 107576 | 107577 | 107582 | 107584 | 107585 | 107586 | 108256 | 108274 | 108473 | 108474 | 108497 | 108499 | 108500 | 108936 | 108937 | 109061 | 109222 | 109640 | 109641 | 109642 | 109643 | 109644 | 109645