Bugzilla – Attachment 166024 Details for
Bug 34972
Canceling a waiting hold from the holds over tab can make the next hold unfillable
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 34972: Remove GetOtherReserves
Bug-34972-Remove-GetOtherReserves.patch (text/plain), 12.71 KB, created by
Emily Lamancusa (emlam)
on 2024-05-01 20:54:29 UTC
(
hide
)
Description:
Bug 34972: Remove GetOtherReserves
Filename:
MIME Type:
Creator:
Emily Lamancusa (emlam)
Created:
2024-05-01 20:54:29 UTC
Size:
12.71 KB
patch
obsolete
>From 9e637ae042c676a60dfd482559e25ac2e92decc2 Mon Sep 17 00:00:00 2001 >From: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> >Date: Tue, 19 Mar 2024 14:14:27 -0400 >Subject: [PATCH] Bug 34972: Remove GetOtherReserves > >GetOtherReserves attempts to set the waiting/transit status for the next >hold on the list when applicable, but in practice it either leaves the >hold state unchanged, or sets the itemnumber without setting the found >status (erroneously converting bib-level holds to item-level holds). > >The latter situation only occurs when the user has been prompted to >confirm, cancel, or revert the hold, and is able to ignore the prompt. >In those situations, the hold's state should not change. > >GetOtherReserves does not need to change the hold state, and it does not >do so correctly. Besides that, it does not do much other than call >CheckReserves, and is only used in 3 places. > >This patch removes GetOtherReserves, and refactors returns.pl and >C4::Reserves::ModReserveCancelAll to call CheckReserves directly instead. > >To test: >1. Place 2 bib-level holds for 2 different patrons (Patron A and Patron > B) on the same record, both for pickup at the logged-in library >2. Check in an item from that record to fill Patron A's hold >3. Set the hold's expiration date to yesterday by accessing the database > in the command line: > - In a ktd shell prompt, open the db client with koha-mysql kohadev > - UPDATE reserves > SET expirationdate = DATE_SUB(CURDATE(), INTERVAL 1 DAY) > WHERE borrowernumber = <Patron A's borrowernumber> >4. Go to Circulation > Holds Awaiting Pickup, and find the hold in the > "holds waiting past their expiration date" tab >5. Click the "Cancel hold" button in the Actions column next to the hold > (do not check in the book) >6. Return to the bib record and look at Patron B's hold >--> Note that Patron B's hold is now an item-level hold and does not > have a waiting status > >7. Cancel Patron B's hold >8. Place 2 new holds on the record: one for Patron A at the logged-in > library, and one for Patron B at a different library >9. Check in an item to fill Patron A's hold >10. Repeat steps 3-5 to expire and cancel Patron A's hold >11. Return to the Holds tab of the bib record and look at Patron B's hold >--> Note that Patron B's hold is now an item-level hold, and there is no > "Revert transit status" button > >12. Place 2 bib-level holds for 2 different patrons (Patron A and Patron > B) on the same record, both for pickup at the logged-in library >13. Check in an item from that record to fill Patron A's hold >14. Check in the same item again. A modal will pop up, saying that the > hold is already waiting >15. In the modal, choose a cancellation reason and click "Cancel hold" >--> A new modal will pop up to fill Patron B's hold >16. Click "Ignore" on the modal for Patron B's hold >17. Return to the bib record and look at Patron B's hold >--> Note that Patron B's hold is now an item-level hold and does not > have a waiting status > >18. Apply patch >19. Repeat steps 1-6 >--> Note that Patron B's hold is still a bib-level/"next available" hold >20. Repeat steps 7-11 >--> Note that Patron B's hold is still a bib-level/"next available" hold >21. Repeat steps 12-17 >--> Note that Patron B's hold is still a bib-level/"next available" hold > >Make sure correct behavior is unchanged: > >22. Cancel Patron B's hold >23. Place 2 new holds on the record: one for Patron A at the logged-in > library, and one for Patron B at a different library >24. Check in an item from that record to fill Patron A's hold >25. Check in the same item again. A modal will pop up, saying that the > hold is already waiting >26. In the modal, choose a cancellation reason and click "Cancel hold" >--> A new modal will pop up to fill Patron B's hold >27. Click "Print slip, transfer, and confirm" on the modal for Patron B's hold >--> Confirm that the information on the slip is correct >--> Confirm that the hold is correctly put in transit > >22. Set HoldsAutoFill and HoldsAutoFillPrintSlip to "Do" > >23. Place a bib-level hold for the logged-in library >24. Check in an item from that bib >--> Confirm the information on the slip is correct >--> Confirm the hold is correctly assigned and set to waiting >25. Place a bib-level hold for a different library >26. Check in an item from that bib >--> Confirm the information on the slip is correct >--> Confirm the hold is correctly put in transit >27. Change the logged-in branch to match the hold pickup location >28. Check the item in >--> Confirm the information on the slip is correct >--> Confirm the hold is correctly assigned and set to waiting > >29. Repeat steps 22-26 >--> Confirm a correct hold slip pops up for Patron B's hold >--> Confirm that Patron B's hold is correctly put in transit >30. Cancel Patron B's hold >31. Place 2 bib-level holds for 2 different patrons (Patron A and Patron > B) on the same record, both for pickup at the logged-in library >33. Repeat steps 24-26 >--> Confirm a correct hold slip pops up for Patron B's hold >--> Confirm Patron B's hold is correctly set to Waiting > >34. Prove t/db_dependent/Circulation.t >35. Prove t/db_dependent/Koha/Holds.t >--> Tests pass >--- > C4/Reserves.pm | 66 +++++++----------------------------- > circ/returns.pl | 25 +++++++++----- > t/db_dependent/Circulation.t | 8 +++-- > 3 files changed, 35 insertions(+), 64 deletions(-) > >diff --git a/C4/Reserves.pm b/C4/Reserves.pm >index 97374d9514..6c23d3a732 100644 >--- a/C4/Reserves.pm >+++ b/C4/Reserves.pm >@@ -106,7 +106,6 @@ BEGIN { > > GetReserveStatus > >- GetOtherReserves > ChargeReserveFee > GetReserveFee > >@@ -714,55 +713,6 @@ sub CanReserveBeCanceledFromOpac { > return $reserve->is_cancelable_from_opac; > } > >-=head2 GetOtherReserves >- >- ($messages,$nextreservinfo)=$GetOtherReserves(itemnumber); >- >-Check queued list of this document and check if this document must be transferred >- >-=cut >- >-sub GetOtherReserves { >- my ($itemnumber) = @_; >- my $messages; >- my $nextreservinfo; >- my $item = Koha::Items->find($itemnumber); >- my ( undef, $checkreserves, undef ) = CheckReserves($item); >- if ($checkreserves) { >- if ( $item->holdingbranch ne $checkreserves->{'branchcode'} ) { >- $messages->{'transfert'} = $checkreserves->{'branchcode'}; >- #minus priorities of others reservs >- ModReserveMinusPriority( >- $itemnumber, >- $checkreserves->{'reserve_id'}, >- ); >- >- #launch the subroutine dotransfer >- C4::Items::ModItemTransfer( >- $itemnumber, >- $item->holdingbranch, >- $checkreserves->{'branchcode'}, >- 'Reserve' >- ), >- ; >- } >- >- #step 2b : case of a reservation on the same branch, set the waiting status >- else { >- $messages->{'waiting'} = 1; >- ModReserveMinusPriority( >- $itemnumber, >- $checkreserves->{'reserve_id'}, >- ); >- ModReserveStatus($itemnumber,'W'); >- } >- >- $nextreservinfo = $checkreserves; >- } >- >- return ( $messages, $nextreservinfo ); >-} >- > =head2 ChargeReserveFee > > $fee = ChargeReserveFee( $borrowernumber, $fee, $title ); >@@ -1293,7 +1243,7 @@ sub ModReserveAffect { > > ($messages,$nextreservinfo) = &ModReserveCancelAll($itemnumber,$borrowernumber,$reason); > >-function to cancel reserv,check other reserves, and transfer document if it's necessary >+function to cancel reserve and check other reserves > > =cut > >@@ -1301,14 +1251,24 @@ sub ModReserveCancelAll { > my $messages; > my $nextreservinfo; > my ( $itemnumber, $borrowernumber, $cancellation_reason ) = @_; >+ my $item = Koha::Items->find($itemnumber); > > #step 1 : cancel the reservation > my $holds = Koha::Holds->search({ itemnumber => $itemnumber, borrowernumber => $borrowernumber }); > return unless $holds->count; > $holds->next->cancel({ cancellation_reason => $cancellation_reason }); > >- #step 2 launch the subroutine of the others reserves >- ( $messages, $nextreservinfo ) = GetOtherReserves($itemnumber); >+ #step 2 check for other reserves on this item >+ ( undef, $nextreservinfo, undef ) = CheckReserves($item); >+ >+ if ($nextreservinfo) { >+ if( $item->holdingbranch ne $nextreservinfo->{'branchcode'} ) { >+ $messages->{'transfert'} = $nextreservinfo->{'branchcode'}; >+ } >+ else { >+ $messages->{'waiting'} = 1; >+ } >+ } > > return ( $messages, $nextreservinfo->{borrowernumber} ); > } >diff --git a/circ/returns.pl b/circ/returns.pl >index 6614237e47..08587a17d4 100755 >--- a/circ/returns.pl >+++ b/circ/returns.pl >@@ -41,7 +41,7 @@ use C4::Items qw( ModItemTransfer ); > use C4::Members::Messaging; > use C4::Members; > use C4::Output qw( output_html_with_http_headers ); >-use C4::Reserves qw( ModReserve ModReserveAffect GetOtherReserves ); >+use C4::Reserves qw( ModReserve ModReserveAffect CheckReserves ); > use C4::RotatingCollections; > use Koha::AuthorisedValues; > use Koha::BiblioFrameworks; >@@ -162,15 +162,21 @@ if ( $query->param('reserve_id') && $op eq 'cud-affect_reserve') { > } # FIXME else? > } else { > my $diffBranchSend = ($userenv_branch ne $diffBranchReturned) ? $diffBranchReturned : undef; >+ > # diffBranchSend tells ModReserveAffect whether document is expected in this library or not, > # i.e., whether to apply waiting status > ModReserveAffect( $itemnumber, $borrowernumber, $diffBranchSend, $reserve_id, $desk_id ); >+ >+ if ($diffBranchSend) { >+ ModItemTransfer( $itemnumber, $userenv_branch, $diffBranchSend, 'Reserve' ); >+ } > } >-# check if we have other reserves for this document, if we have a return send the message of transfer >- my ( $messages, $nextreservinfo ) = GetOtherReserves($itemnumber); >+ # check if we have other reserves for this document, if we have a result send the message of transfer >+ # FIXME do we need to do this if we didn't take the cancel_reserve branch above? >+ my ( undef, $nextreservinfo, undef ) = CheckReserves($item); > >- my $patron = Koha::Patrons->find( $nextreservinfo ); >- if ( $messages->{'transfert'} ) { >+ my $patron = Koha::Patrons->find( $nextreservinfo->{'borrowernumber'} ); >+ if ( $userenv_branch ne $nextreservinfo->{'branchcode'} ) { > $template->param( > itemtitle => $biblio->title, > itembiblionumber => $biblio->biblionumber, >@@ -595,15 +601,18 @@ if ( $messages->{'ResFound'} ) { > > my $diffBranchSend = !$branchCheck ? $reserve->{branchcode} : undef; > ModReserveAffect( $itemnumber, $reserve->{borrowernumber}, $diffBranchSend, $reserve->{reserve_id}, $desk_id ); >- my ( $messages, $nextreservinfo ) = GetOtherReserves($reserve->{itemnumber}); >+ >+ if ($diffBranchSend) { >+ ModItemTransfer( $itemnumber, $item->holdingbranch, $reserve->{branchcode}, 'Reserve' ); >+ } > > $template->param( > hold_auto_filled => 1, > print_slip => C4::Context->preference('HoldsAutoFillPrintSlip'), >- reserve_id => $nextreservinfo->{reserve_id}, >+ reserve_id => $reserve->{reserve_id}, > ); > >- if ( $messages->{'transfert'} ) { >+ if ($diffBranchSend) { > $template->param( > itemtitle => $biblio->title, > itembiblionumber => $biblio->biblionumber, >diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t >index 307db1d8a9..5170744e6e 100755 >--- a/t/db_dependent/Circulation.t >+++ b/t/db_dependent/Circulation.t >@@ -37,7 +37,7 @@ use C4::Circulation qw( AddIssue AddReturn CanBookBeRenewed GetIssuingCharges Ad > use C4::Biblio; > use C4::Items qw( ModItemTransfer ); > use C4::Log; >-use C4::Reserves qw( AddReserve ModReserve ModReserveCancelAll ModReserveAffect CheckReserves GetOtherReserves ); >+use C4::Reserves qw( AddReserve ModReserve ModReserveCancelAll ModReserveAffect CheckReserves ); > use C4::Overdues qw( CalcFine UpdateFine get_chargeable_units ); > use C4::Members::Messaging qw( SetMessagingPreference ); > use Koha::DateUtils qw( dt_from_string output_pref ); >@@ -5976,8 +5976,10 @@ subtest 'Checkout should correctly terminate a transfer' => sub { > ModItemTransfer( $item->itemnumber, $library_1->branchcode, > $library_2->branchcode, 'Manual' ); > ModReserveAffect( $item->itemnumber, undef, $do_transfer, $reserve_id ); >- GetOtherReserves( $item->itemnumber ) >- ; # To put the Reason, it's what does returns.pl... >+ ModItemTransfer( >+ $item->itemnumber, $library_1->branchcode, >+ $library_2->branchcode, 'Reserve' >+ ); # To put the Reason, it's what does returns.pl... > my $hold = Koha::Holds->find($reserve_id); > is( $hold->found, 'T', 'Hold is in transit' ); > my $transfer = $item->get_transfer; >-- >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 34972
:
160775
|
160776
|
163471
|
163472
|
163473
|
163728
|
163729
|
163730
|
164392
|
164393
|
166024
|
166028
|
166029
|
166100
|
166101
|
166102
|
166103