Found on 22.11 The code in returns.pl contains a call to GetOtherReserves but does not pass lookahead days. Causing CheckReserves to find no reserves, skipping creation of a new transfer. Easy fix forthcoming Note that it took me hours to find this bug. I love returns.pl, its template and the associated modules ;) PS Bug 34972 is a game changer here on master.
FOR MASTER: my ( undef, $nextreservinfo, undef ) = CheckReserves($item); @Emily: Inrelation to bug 34972, we should pass lookahead_days here as well. Or default in the sub to the pref ConfirmFutureHolds?
A related problem in transferstoreceive btw: # we check if we have a reserv for this transfer my $holds = $item->current_holds; This wont find a future hold (e.g. for tomorrow) either.
(In reply to Marcel de Rooy from comment #2) > A related problem in transferstoreceive btw: > > # we check if we have a reserv for this transfer > my $holds = $item->current_holds; > > This wont find a future hold (e.g. for tomorrow) either. Moving this to a new report. The whole idea behind current_holds is a bit rigorous.. Most calls might need some adjustment? Bug 37651
(In reply to Marcel de Rooy from comment #1) > FOR MASTER: > my ( undef, $nextreservinfo, undef ) = CheckReserves($item); > > @Emily: Inrelation to bug 34972, we should pass lookahead_days here as well. > Or default in the sub to the pref ConfirmFutureHolds? Ooh, good catch. I think we need to pass it. C4::Circulation::transferbook calls CheckReserves with no lookahead on purpose, so we need to preserve the behavior for that case.
Created attachment 170384 [details] [review] Bug 37650: [22.11] Add lookahead in calling GetOtherReserves in returns.pl Test plan: Add a future hold for an item on another branch. Check that item in at the home branch. Without this patch, you can confirm the hold but a transfer will not be made. With this patch, the transfer should be created.
Testing on master shows that the bug does not occur anymore after bug 34972. The call to CheckReserves seems to be wrong but this part of code does not seem to make any difference: if ( $userenv_branch ne $nextreservinfo->{'branchcode'} ) { my $patron = Koha::Patrons->find( $nextreservinfo->{'borrowernumber'} ); $template->param( itemtitle => $biblio->title, itembiblionumber => $biblio->biblionumber, iteminfo => $biblio->author, patron => $patron, diffbranch => 1, ); } If you confirmed a future hold, returns moves the hold to transit and creates a transfer now. The following call to CheckReserves will not find this hold, normally nothing. So $nextreserveinfo is expected to be undef. There is no additional display after the modal is gone. Just a checkin box. Warnings in the log confirm the uninitialized value: [2024/08/19 12:02:34] [WARN] Use of uninitialized value in string ne at /usr/share/koha/circ/returns.pl line 182. The master patch should resolve that warning now. And remove an additional one too on L253: [2024/08/19 12:04:01] [WARN] Use of uninitialized value $op in string eq at /usr/share/koha/circ/returns.pl line 253.
The 22.11 bug is ONLY necessary in case 34972 is not backported !
Created attachment 170466 [details] [review] Bug 37650: Add lookahead days in CheckReserves call in returns.pl Test plan: Enable future holds. Put a positive value in ConfirmFutureHolds. Add a hold for tomorrow and pickup at B. Check item in at A. Confirm hold. Without this patch you should see a warn on L182 and L253. With this patch, they should be gone. NOTE: Before bug 34972 no transfer was created for future holds. See also the 22.11 patch on Bugzilla. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Marcel de Rooy from comment #2) > A related problem in transferstoreceive btw: > > # we check if we have a reserv for this transfer > my $holds = $item->current_holds; > > This wont find a future hold (e.g. for tomorrow) either. See bug 37651