From 111d948a2c5861254d51704180850441a698c8e7 Mon Sep 17 00:00:00 2001
From: Nick Clemens <nick@bywatersolutions.com>
Date: Fri, 28 Jun 2019 12:37:25 +0000
Subject: [PATCH] Bug 14549: Check patrons reserves for any holds on a bib when
 checking out an item

MoveReserve uses CheckReserves to see if the current patron has any holds
on the title they are checking out, however, CheckReserves doesn't return
all holds on a biblio, it returns holds on the item from the holdsqueue
if they exist

This can create a condition where we check holds on an item, find we have it planned for
another borrower, confirm checkout to the current borrower, but don't fill their hold

To test:
  1) Find record
  2) place record level holds for 2 different patrons (record level)
  3) Run holds queue builder, check the queue to confirm an item was selected for patron 1
     Circulation->Holds queue->Library="All"
  4) Check out the item queued for patron with priority 1 to the second patron
  5) You should be asked to confirm, do so
  6) Note the item checks out, but both holds remain
  7) Apply patch
  8) Check in the item
  9) Don't confirm the hold
 10) Check the holds on the record and the holds queue
 11) Patron 1 should be priority 1 with an item selected from the holds queue
 12) Checkout to patron 2 as before
 13) Note the hold for patron 2 is filled this time
 14) Prove -v t/db_dependent/Reserves.t
---
 C4/Reserves.pm | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/C4/Reserves.pm b/C4/Reserves.pm
index 097a5c09d6..cfdfb5ec1d 100644
--- a/C4/Reserves.pm
+++ b/C4/Reserves.pm
@@ -737,11 +737,9 @@ sub CheckReserves {
     }
     # note: we get the itemnumber because we might have started w/ just the barcode.  Now we know for sure we have it.
     my ( $biblio, $bibitem, $notforloan_per_itemtype, $notforloan_per_item, $itemnumber, $damaged, $item_homebranch, $item_holdingbranch ) = $sth->fetchrow_array;
-
     return if ( $damaged && !C4::Context->preference('AllowHoldsOnDamagedItems') );
 
     return unless $itemnumber; # bail if we got nothing.
-
     # if item is not for loan it cannot be reserved either.....
     # except where items.notforloan < 0 :  This indicates the item is holdable.
     return if  ( $notforloan_per_item > 0 ) or $notforloan_per_itemtype;
@@ -979,7 +977,6 @@ sub ModReserveFill {
     my $reserve_id = $res->{'reserve_id'};
 
     my $hold = Koha::Holds->find($reserve_id);
-
     # get the priority on this record....
     my $priority = $hold->priority;
 
@@ -1795,18 +1792,16 @@ sub MoveReserve {
         # The item is reserved by someone else.
         # Find this item in the reserves
 
-        my $borr_res;
-        foreach (@$all_reserves) {
-            $_->{'borrowernumber'} == $borrowernumber or next;
-            $_->{'biblionumber'}   == $biblionumber   or next;
-
-            $borr_res = $_;
-            last;
-        }
+        my $borr_res  = Koha::Holds->search({
+            borrowernumber => $borrowernumber,
+            biblionumber   => $biblionumber,
+        },{
+            order_by       => 'priority'
+        })->next();
 
         if ( $borr_res ) {
             # The item is reserved by the current patron
-            ModReserveFill($borr_res);
+            ModReserveFill($borr_res->unblessed);
         }
 
         if ( $cancelreserve eq 'revert' ) { ## Revert waiting reserve to priority 1
-- 
2.11.0