Bugzilla – Attachment 182999 Details for
Bug 40055
C4::Reserves::MoveReserve should be passed objects
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 40055: Make MoveReserve be passed objects instead of ids
Bug-40055-Make-MoveReserve-be-passed-objects-inste.patch (text/plain), 8.18 KB, created by
Jonathan Druart
on 2025-06-05 13:14:03 UTC
(
hide
)
Description:
Bug 40055: Make MoveReserve be passed objects instead of ids
Filename:
MIME Type:
Creator:
Jonathan Druart
Created:
2025-06-05 13:14:03 UTC
Size:
8.18 KB
patch
obsolete
>From 79e081f60999b92839d9dadfd8b9f097141d43bc Mon Sep 17 00:00:00 2001 >From: Tomas Cohen Arazi <tomascohen@theke.io> >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 <david@davidnind.com> > >Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> >--- > 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 733177a5527..bf93c32a590 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 cadd4d413e0..7718b4cddbe 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<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. >@@ -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 b4c98d93934..5157b1121ef 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 b66706dcfc2..97cd34aa3bc 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.34.1
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 40055
:
182915
|
182953
|
182989
| 182999