Bug 37650 - Fix warn and remove FIXME in circ/returns.pl
Summary: Fix warn and remove FIXME in circ/returns.pl
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor
Assignee: Marcel de Rooy
QA Contact: Nick Clemens (kidclamp)
URL:
Keywords:
Depends on: 38919
Blocks:
  Show dependency treegraph
 
Reported: 2024-08-15 12:38 UTC by Marcel de Rooy
Modified: 2025-02-13 15:20 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
25.05.00
Circulation function:


Attachments
Bug 37650: [22.11] Add lookahead in calling GetOtherReserves in returns.pl (2.71 KB, patch)
2024-08-15 14:25 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 37650: Add lookahead days in CheckReserves call in returns.pl (1.71 KB, patch)
2024-08-19 12:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 37650: Add lookahead days in CheckReserves call in returns.pl (1.49 KB, patch)
2025-01-16 13:25 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 37650: Add lookahead days in CheckReserves call in returns.pl (1.45 KB, patch)
2025-01-16 13:25 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 37650: Add lookahead days in CheckReserves call in returns.pl (1.54 KB, patch)
2025-01-17 08:40 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 37650: (follow-up) Remove FIXME around CheckReserves call (3.90 KB, patch)
2025-01-17 08:40 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 37650: Add lookahead days in CheckReserves call in returns.pl (1.57 KB, patch)
2025-01-21 13:50 UTC, Emily Lamancusa (emlam)
Details | Diff | Splinter Review
Bug 37650: (follow-up) Remove FIXME around CheckReserves call (3.93 KB, patch)
2025-01-21 13:50 UTC, Emily Lamancusa (emlam)
Details | Diff | Splinter Review
Bug 37650: Add lookahead days in CheckReserves call in returns.pl (1.63 KB, patch)
2025-02-07 14:49 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 37650: (follow-up) Remove FIXME around CheckReserves call (3.98 KB, patch)
2025-02-07 14:49 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2024-08-15 12:38:41 UTC
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.
Comment 1 Marcel de Rooy 2024-08-15 12:44:11 UTC
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?
Comment 2 Marcel de Rooy 2024-08-15 13:14:08 UTC
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.
Comment 3 Marcel de Rooy 2024-08-15 13:50:54 UTC
(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
Comment 4 Emily Lamancusa (emlam) 2024-08-15 14:25:24 UTC
(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.
Comment 5 Marcel de Rooy 2024-08-15 14:25:52 UTC
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.
Comment 6 Marcel de Rooy 2024-08-19 12:28:37 UTC
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.
Comment 7 Marcel de Rooy 2024-08-19 12:29:21 UTC
The 22.11 bug is ONLY necessary in case 34972 is not backported !
Comment 8 Marcel de Rooy 2024-08-19 12:30:38 UTC
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>
Comment 9 Marcel de Rooy 2024-08-19 12:31:19 UTC
(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
Comment 10 Victor Grousset/tuxayo 2024-10-07 19:20:14 UTC
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.
Comment 11 Marcel de Rooy 2024-10-08 11:40:03 UTC
(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.
Comment 12 Victor Grousset/tuxayo 2024-10-09 23:17:55 UTC
(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.
Comment 13 Lisette Scheer 2024-10-25 20:02:16 UTC
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
Comment 14 Marcel de Rooy 2025-01-16 13:24:15 UTC
(In reply to Lisette Scheer from comment #13)
> 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

Thx for trying that one. I will remove the 22.11 variant now.
Comment 15 Marcel de Rooy 2025-01-16 13:25:05 UTC Comment hidden (obsolete)
Comment 16 Marcel de Rooy 2025-01-16 13:25:40 UTC
Created attachment 176658 [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.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 17 Marcel de Rooy 2025-01-16 13:26:30 UTC
This was written in August, so line numbers might vary in the meantime..
Comment 18 Marcel de Rooy 2025-01-16 15:32:49 UTC
Blocking this one. Needs another title now.
Comment 19 Marcel de Rooy 2025-01-17 08:40:29 UTC
Created attachment 176706 [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 L186:
  [WARN] Use of uninitialized value in string ne at /usr/share/koha/circ/returns.pl line 186.
With this patch, you should not see it anymore.

NOTE: Before bug 34972 no transfer was created for future holds.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 20 Marcel de Rooy 2025-01-17 08:40:31 UTC
Created attachment 176707 [details] [review]
Bug 37650: (follow-up) Remove FIXME around CheckReserves call

The FIXME correctly stated that the CheckReserves call and follow-up
only should be executed within the cancel hold scope.

NOTE: Actually, I do not see how to trigger it in current code. Since
checking in a waiting hold triggers the NotIssued error. And the
cancel hold button is not available for holds in queue.

Test plan:
Place a hold on an item.
Checkin at pickup location. Confirm. Hold waiting now?
Try again but checkin at another branch. Transfer triggered?

The hard part (see my note). Try to get at the form where you can
cancel the hold (waiting or first in queue).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 21 Marcel de Rooy 2025-01-17 08:43:38 UTC
This bug changed quite a bit over time. Bug 34972 fixed the initial (major) error. This deals with a warn after that. And tries to resolve the FIXME about where to put the CheckReserves call.

But it also triggers the question: How do I actually reach the cancel hold code? See commit message of second follow-up patch.

Emily or Kyle: Any idea here?
Comment 22 Marcel de Rooy 2025-01-17 09:07:32 UTC
Actually the comment is weird now too (after 34972):

        # check if we have other reserves for this document, if we have a result send the message of transfer
        my ( undef, $nextreservinfo, undef ) = CheckReserves( $item, C4::Context->preference('ConfirmFutureHolds') );
        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,
            );

Before 34972 we called GetOtherReserves and it could change stuff, but CheckReserves does not change things. So 'sending the message of transfer' is not true.
Comment 23 Emily Lamancusa (emlam) 2025-01-17 16:28:18 UTC
> NOTE: Actually, I do not see how to trigger it in current code. Since
> checking in a waiting hold triggers the NotIssued error. And the
> cancel hold button is not available for holds in queue.

Yikes, that seems like a bug! There should still be a modal notifying of the already-waiting hold (which was indeed the only way to trigger the cancel-hold code). Opened bug 38919 for it
Comment 24 Emily Lamancusa (emlam) 2025-01-21 13:50:16 UTC
Created attachment 176864 [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 L186:
  [WARN] Use of uninitialized value in string ne at /usr/share/koha/circ/returns.pl line 186.
With this patch, you should not see it anymore.

NOTE: Before bug 34972 no transfer was created for future holds.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 25 Emily Lamancusa (emlam) 2025-01-21 13:50:18 UTC
Created attachment 176865 [details] [review]
Bug 37650: (follow-up) Remove FIXME around CheckReserves call

The FIXME correctly stated that the CheckReserves call and follow-up
only should be executed within the cancel hold scope.

NOTE: Actually, I do not see how to trigger it in current code. Since
checking in a waiting hold triggers the NotIssued error. And the
cancel hold button is not available for holds in queue.

Test plan:
Place a hold on an item.
Checkin at pickup location. Confirm. Hold waiting now?
Try again but checkin at another branch. Transfer triggered?

The hard part (see my note). Try to get at the form where you can
cancel the hold (waiting or first in queue).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 26 Emily Lamancusa (emlam) 2025-01-21 14:06:42 UTC
Testing notes for 2nd patch:
There are two modals that trigger the outer block of code affected by the patch (if ( $query->param('reserve_id') && $op eq 'cud-affect_reserve')):

hold-found1 - modal appears when the checked-in item is already waiting for a hold

hold-found2 - modal appears if the checked-in item can fill a hold, the hold is NOT already waiting (but it may be already in-transit), and there are no recalls on the item

hold-found1 has a button to cancel the hold, which triggers the if($cancel_reserve) block. All other buttons on hold-found1 and hold-found2 would then trigger the else block.

I tested the different buttons on both modals under various circumstances:
- one hold on the bib vs multiple holds on the bib
- holds at the logged-in branch vs another branch
- current vs future holds

I noted that clicking "Ignore" on hold-found1 (when the hold is already waiting) caused the modal to continue popping up until I selected another option; however, this behavior is present on main as well. (Not sure it makes sense to even have an "Ignore" option when the hold is already waiting, but that's for a different bug...)
Comment 27 Marcel de Rooy 2025-01-21 14:25:25 UTC
Thanks, Emily !
Comment 28 Nick Clemens (kidclamp) 2025-02-07 14:49:18 UTC
Created attachment 177629 [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 L186:
  [WARN] Use of uninitialized value in string ne at /usr/share/koha/circ/returns.pl line 186.
With this patch, you should not see it anymore.

NOTE: Before bug 34972 no transfer was created for future holds.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 29 Nick Clemens (kidclamp) 2025-02-07 14:49:20 UTC
Created attachment 177630 [details] [review]
Bug 37650: (follow-up) Remove FIXME around CheckReserves call

The FIXME correctly stated that the CheckReserves call and follow-up
only should be executed within the cancel hold scope.

NOTE: Actually, I do not see how to trigger it in current code. Since
checking in a waiting hold triggers the NotIssued error. And the
cancel hold button is not available for holds in queue.

Test plan:
Place a hold on an item.
Checkin at pickup location. Confirm. Hold waiting now?
Try again but checkin at another branch. Transfer triggered?

The hard part (see my note). Try to get at the form where you can
cancel the hold (waiting or first in queue).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 30 Katrin Fischer 2025-02-13 15:20:27 UTC
Pushed for 25.05!

Well done everyone, thank you!