Bug 40055: Make MoveReserve be passed objects instead of ids

The `MoveReserve` method is called once, in `AddIssue`. The latter has the related `Koha::Item` object already fetched from the DB, but it passed the `itemnumber` which is used by `MoveReserve` to fetch the item from the DB. This should not happen.

To test:
1. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Reserves* \
           t/db_dependent/Koha/Hold* \
           t/db_dependent/Hold* \
           t/db_dependent/Circulation*
=> SUCCESS: Tests pass
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests still pass!
4. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>

C4/Circulation.pm

@@ -1707,7 +1707,7 @@ sub AddIssue {
                 );
             }
 
-            C4::Reserves::MoveReserve( $item_object->itemnumber, $patron->borrowernumber, $cancelreserve );
+            C4::Reserves::MoveReserve( $item_object, $patron, $cancelreserve );
 
             # Starting process for transfer job (checking transfer and validate it if we have one)
             if ( my $transfer = $item_object->get_transfer ) {

C4/Reserves.pm

@@ -2029,25 +2029,36 @@ sub _ShiftPriority {
 
 =head2 MoveReserve
 
-  MoveReserve( $itemnumber, $borrowernumber, $cancelreserve )
+  MoveReserve( $item, $patron, $cancelreserve )
 
 Use when checking out an item to handle reserves
 If $cancelreserve boolean is set to true, it will remove existing reserve
 
+Parameters are:
+
+=over 4
+
+=item B<$item>: a I<Koha::Item> object
+
+=item B<$patron>: a I<Koha::Patron> object
+
+=item B<$cancelreserve>: a boolean telling if the existing hold should be removed
+
+=back
+
 =cut
 
 sub MoveReserve {
-    my ( $itemnumber, $borrowernumber, $cancelreserve ) = @_;
+    my ( $item, $patron, $cancelreserve ) = @_;
 
     $cancelreserve //= 0;
 
     my $lookahead = C4::Context->preference('ConfirmFutureHolds');    #number of days to look for future holds
-    my $item      = Koha::Items->find($itemnumber);
     my ( $restype, $res, undef ) = CheckReserves( $item, $lookahead );
 
-    if ( $res && $res->{borrowernumber} == $borrowernumber ) {
+    if ( $res && $res->{borrowernumber} == $patron->borrowernumber ) {
         my $hold = Koha::Holds->find( $res->{reserve_id} );
-        $hold->fill( { item_id => $itemnumber } );
+        $hold->fill( { item_id => $item->id } );
     } else {
 
         # The item is reserved by someone else.
@@ -2059,24 +2070,23 @@ sub MoveReserve {
                 dateformat => 'iso', dateonly => 1
             }
         );
-        my $borr_res = Koha::Holds->search(
+        my $hold = $patron->holds->search(
             {
-                borrowernumber => $borrowernumber,
-                biblionumber   => $item->biblionumber,
-                reservedate    => { '<=' => $lookahead_date },
-                -or            => [ item_level_hold => 0, itemnumber => $itemnumber ],
+                biblionumber => $item->biblionumber,
+                reservedate  => { '<=' => $lookahead_date },
+                -or          => [ item_level_hold => 0, itemnumber => $item->id ],
             },
             { order_by => 'priority' }
         )->next();
 
-        if ($borr_res) {
+        if ($hold) {
 
             # The item is reserved by the current patron
-            $borr_res->fill( { item_id => $itemnumber } );
+            $hold->fill( { item_id => $item->id } );
         }
 
         if ( $cancelreserve eq 'revert' ) {    ## Revert waiting reserve to priority 1
-            RevertWaitingStatus( { itemnumber => $itemnumber } );
+            RevertWaitingStatus( { itemnumber => $item->id } );
         } elsif ( $cancelreserve eq 'cancel' || $cancelreserve ) {    # cancel reserves on this item
             my $hold = Koha::Holds->find( $res->{reserve_id} );
             $hold->cancel;

t/db_dependent/Holds.t

@@ -2077,7 +2077,7 @@ subtest 'EmailPatronWhenHoldIsPlaced tests' => sub {
         $post_notices_count, $original_notices_count,
         "EmailPatronWhenHoldIsPlaced is disabled so no email is queued"
     );
-    MoveReserve( $item->itemnumber, $borrowernumber, 1 );
+    MoveReserve( $item, $patron, 1 );
 
     $original_notices_count = Koha::Notice::Messages->search(
         {

t/db_dependent/Reserves.t

@@ -699,7 +699,7 @@ AddReserve(
         priority       => 1,
     }
 );
-MoveReserve( $item->itemnumber, $borrowernumber );
+MoveReserve( $item, $patron );
 ($status) = CheckReserves($item);
 is( $status, '', 'MoveReserve filled hold' );
 
@@ -715,7 +715,7 @@ AddReserve(
         itemnumber     => $other_item->id,
     }
 );
-MoveReserve( $item->itemnumber, $borrowernumber );
+MoveReserve( $item, $patron );
 ($status) = CheckReserves($item);
 is( $status, '', 'MoveReserve filled waiting hold' );
 
@@ -731,7 +731,7 @@ AddReserve(
         reservation_date => $resdate,
     }
 );
-MoveReserve( $item->itemnumber, $borrowernumber );
+MoveReserve( $item, $patron );
 ($status) = CheckReserves( $item, 1 );
 is( $status, 'Reserved', 'MoveReserve did not fill future hold' );
 $dbh->do( 'DELETE FROM reserves', undef, ($bibnum) );
@@ -747,7 +747,7 @@ AddReserve(
         reservation_date => $resdate,
     }
 );
-MoveReserve( $item->itemnumber, $borrowernumber );
+MoveReserve( $item, $patron );
 ($status) = CheckReserves( $item, undef, 2 );
 is( $status, '', 'MoveReserve filled future hold now' );
 
@@ -761,7 +761,7 @@ AddReserve(
         reservation_date => $resdate,
     }
 );
-MoveReserve( $item->itemnumber, $borrowernumber );
+MoveReserve( $item, $patron );
 ($status) = CheckReserves( $item, undef, 2 );
 is( $status, '', 'MoveReserve filled future waiting hold now' );
 
@@ -777,7 +777,7 @@ AddReserve(
         reservation_date => $resdate,
     }
 );
-MoveReserve( $item->itemnumber, $borrowernumber );
+MoveReserve( $item, $patron );
 ($status) = CheckReserves( $item, 3 );
 is( $status, 'Reserved', 'MoveReserve did not fill future hold of 3 days' );
 $dbh->do( 'DELETE FROM reserves', undef, ($bibnum) );
@@ -1176,7 +1176,7 @@ subtest 'MoveReserve additional test' => sub {
     );
 
     # The 2nd hold should be filled even if the item is preselected for the first hold
-    MoveReserve( $item_1->itemnumber, $patron_2->borrowernumber );
+    MoveReserve( $item_1, $patron_2 );
     is( $patron_2->holds->count, 0, "The 2nd patrons no longer has a hold" );
     is(
         $patron_2->old_holds->next()->reserve_id, $reserve_2,
@@ -1194,7 +1194,7 @@ subtest 'MoveReserve additional test' => sub {
     );
 
     # The 3rd hold should not be filled as it is an item level hold on a different item
-    MoveReserve( $item_1->itemnumber, $patron_2->borrowernumber );
+    MoveReserve( $item_1, $patron_2 );
     is( $patron_2->holds->count,     1, "The 2nd patron still has a hold" );
     is( $patron_2->old_holds->count, 1, "The 2nd patron has only 1 old holds" );
 
@@ -1202,7 +1202,7 @@ subtest 'MoveReserve additional test' => sub {
     $hold_3->item_level_hold(0)->store();
 
     # The 3rd hold should now be filled as it is a title level hold, even though associated with a different item
-    MoveReserve( $item_1->itemnumber, $patron_2->borrowernumber );
+    MoveReserve( $item_1, $patron_2 );
     is( $patron_2->holds->count,       0, "The 2nd patron no longer has a hold" );
     is( $patron_2->old_holds->count(), 2, "The 2nd patron's hold was filled and moved to old holds" );