From 63b060e4db395e33a55c71589f7a2cb55ccb7bd2 Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi Date: Mon, 2 Jun 2025 18:09:10 -0300 Subject: [PATCH] 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 --- C4/Circulation.pm | 2 +- C4/Reserves.pm | 36 +++++++++++++++++++++++------------- t/db_dependent/Holds.t | 2 +- t/db_dependent/Reserves.t | 18 +++++++++--------- 4 files changed, 34 insertions(+), 24 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 733177a552..bf93c32a59 100644 --- a/C4/Circulation.pm +++ b/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 ) { diff --git a/C4/Reserves.pm b/C4/Reserves.pm index cadd4d413e..852768c330 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -2028,25 +2028,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 object + +=item B<$patron>: a I 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->id ) { 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. @@ -2058,24 +2069,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; diff --git a/t/db_dependent/Holds.t b/t/db_dependent/Holds.t index b4c98d9393..5157b1121e 100755 --- a/t/db_dependent/Holds.t +++ b/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( { diff --git a/t/db_dependent/Reserves.t b/t/db_dependent/Reserves.t index b66706dcfc..97cd34aa3b 100755 --- a/t/db_dependent/Reserves.t +++ b/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" ); -- 2.39.5