gitea
/
Koha-clms
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
This commit is contained in:
Emily Lamancusa 2024-03-19 14:14:27 -04:00 committed by Katrin Fischer
parent d71f527202
commit dc00e55a32
No known key found for this signature in database
GPG Key ID: 0EF6E2C03357A834
3 changed files with 36 additions and 65 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

66
C4/Reserves.pm
View File

@ -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 );
@ -1298,7 +1248,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
@ -1306,14 +1256,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} );
}

27
circ/returns.pl
View File

@ -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 );
}
# check if we have other reserves for this document, if we have a return send the message of transfer
my ( $messages, $nextreservinfo ) = GetOtherReserves($itemnumber);
my $patron = Koha::Patrons->find( $nextreservinfo );
if ( $messages->{'transfert'} ) {
if ($diffBranchSend) {
ModItemTransfer( $itemnumber, $userenv_branch, $diffBranchSend, 'Reserve' );
}
}
# 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->{'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,

8
t/db_dependent/Circulation.t
View File

@ -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 );
@ -6269,8 +6269,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;