Bug 37650

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

I don't get a warn at L253, on L182, yes.

Hold starts on date: tomorrow
ConfirmFutureHolds: 3
record level hold
I'm checking it in another library than the pick up one.

Anything I could be missing? I didn't know about the feature and struggled to find  AllowHoldDateInFuture so I'm likely missing something obvious.

(In reply to Victor Grousset/tuxayo from comment #10)
> I don't get a warn at L253, on L182, yes.

The line numbers may not be exact anymore. They were on uninitialized $op iirc, so not that important. L253 includes an $op. 182 does not now?

> Hold starts on date: tomorrow
> ConfirmFutureHolds: 3
> record level hold
> I'm checking it in another library than the pick up one.
> 
> Anything I could be missing? I didn't know about the feature and struggled
> to find  AllowHoldDateInFuture so I'm likely missing something obvious.

First thing coming up, is test with an item level hold.

(In reply to Marcel de Rooy from comment #11)
> The line numbers may not be exact anymore. They were on uninitialized $op
> iirc, so not that important. L253 includes an $op. 182 does not now?

L253 for me would be
if ( $op eq 'cud-dotransfer'){
which is likely the same for you.

182 that I get is
    if ( $userenv_branch ne $nextreservinfo->{'branchcode'} ) {
And it's relevant with the changes line 180 from the patch.



> First thing coming up, is test with an item level hold.

Right, I though of it last time an forgot. But same result here :(
Only war at line 182.




I held 39999000012118 to patron with borrowernumber=5, pickup liberty.
I'm in centerville and check in and confirmed the hold & transfer.

This needs to be rebased. 
Applying: Bug 37650: Add lookahead in calling GetOtherReserves in returns.pl
Using index info to reconstruct a base tree...
M	C4/Reserves.pm
M	circ/returns.pl
Falling back to patching base and 3-way merge...
Auto-merging circ/returns.pl
CONFLICT (content): Merge conflict in circ/returns.pl
Auto-merging C4/Reserves.pm
CONFLICT (content): Merge conflict in C4/Reserves.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 37650: Add lookahead in calling GetOtherReserves in returns.pl