Bug 25969 - Checking in a found hold at a different branch then confirming the hold causes internal server error
Summary: Checking in a found hold at a different branch then confirming the hold cause...
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
: 26386 (view as bug list)
Depends on:
Blocks: 26485 26627
  Show dependency treegraph
 
Reported: 2020-07-09 18:24 UTC by Kyle M Hall
Modified: 2020-11-06 14:47 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:
20.11.00


Attachments
Bug 25969: Checking in a found hold at a different branch then confirming the hold causes internal server error (1.40 KB, patch)
2020-07-09 18:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25969: Checking in a found hold at a different branch then confirming the hold causes internal server error (1.38 KB, patch)
2020-07-10 15:51 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25969: Checking in a found hold at a different branch then confirming the hold causes internal server error (2.57 KB, patch)
2020-08-03 11:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25969: Don't set itemnumber as reserve->itemnumber (2.41 KB, patch)
2020-09-04 16:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25969: Checking in a found hold at a different branch then confirming the hold causes internal server error (2.62 KB, patch)
2020-09-08 15:15 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 25969: Don't set itemnumber as reserve->itemnumber (2.47 KB, patch)
2020-09-08 15:15 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 25969: Checking in a found hold at a different branch then confirming the hold causes internal server error (2.67 KB, patch)
2020-09-12 22:57 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25969: Don't set itemnumber as reserve->itemnumber (2.53 KB, patch)
2020-09-12 22:57 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25969: (follow-up) If not coming from hold modal find item by barcode (1008 bytes, patch)
2020-10-07 12:25 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26627: Print and confirming a hold can cause an infinite loop (1.19 KB, patch)
2020-10-07 15:36 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2020-07-09 18:24:48 UTC
If a record level hold is filled and waiting at Library A, and the item is checked in at Library B, AddReturn() calls RevertWaitingStatus(), which unsets found, and itemnumber ( if the hold was record level ). AddReturn() then retuns the message 'ResFound', which triggers the hold fulfillment dialog, which passes the reserve_id back to circulation.pl.

Then, circulation.pl attempts to get the biblio via the itemnumber, which the hold doesn't have, causing an ISE.
Comment 1 Kyle M Hall 2020-07-09 18:35:45 UTC
So the root cause is that we are not passing the itemnumber *for the scanned barcode* back to the form, making itemnumber undef, so the Item object is empty, so calling the biblio method causes an ISE.
Comment 2 Kyle M Hall 2020-07-09 18:39:55 UTC Comment hidden (obsolete)
Comment 3 Nick Clemens 2020-07-10 15:51:22 UTC Comment hidden (obsolete)
Comment 4 Andrew Fuerste-Henry 2020-07-31 18:39:17 UTC
I'm still getting an error with the patch applied. "Can't call method "biblio" on an undefined value at /kohadevbox/koha/circ/returns.pl line 148"
Comment 5 Kyle M Hall 2020-08-03 11:03:58 UTC
Created attachment 107697 [details] [review]
Bug 25969: Checking in a found hold at a different branch then confirming the hold causes internal server error

If a record level hold is filled and waiting at Library A, and the item
is checked in at Library B, and an attempt is made to re-fill the hold
with the item, Koha will return an ISE.

Test Plan:
1) Place a hold for Library A, at Library A, for pickup at Library A
2) Check in the item at Library A and fill the hold so it is waiting at Library A
3) Log in as Library B, check in the same barcode
4) Note the request to fill the hold with the item
5) Choose to fill the hold
6) Note you get an internal server error
7) Apply this patch
8) Restart all the things!
9) Repeat steps 1-5
10) No ISE!
Comment 6 Sally 2020-08-03 15:30:03 UTC
This patch works if you check in using the the 'check in' function in the main search bar.

However, if you use the check in at: cgi-bin/koha/circ/returns.pl - then you still get the same ISE as before the patch is applied:

Can't call method "biblio" on an undefined value at /kohadevbox/koha/circ/returns.pl line 148
in (eval) at /kohadevbox/koha/circ/returns.pl line 148
  145:     my $cancel_reserve = $query->pa
Comment 7 Nick Clemens 2020-09-03 10:13:43 UTC
(In reply to Sally from comment #6)
> This patch works if you check in using the the 'check in' function in the
> main search bar.
> 
> However, if you use the check in at: cgi-bin/koha/circ/returns.pl - then you
> still get the same ISE as before the patch is applied:
> 
> Can't call method "biblio" on an undefined value at
> /kohadevbox/koha/circ/returns.pl line 148
> in (eval) at /kohadevbox/koha/circ/returns.pl line 148
>   145:     my $cancel_reserve = $query->pa

I cannot recreate this, can you test again and explain the steps?
Comment 8 Andrew Fuerste-Henry 2020-09-03 16:59:05 UTC
I'm also getting an error. 

To recreate:
- Log in at Library A
- Place hold on item at Library A, for pickup at Library A
- Check item in at Library A
- Click "Confirm hold"
- Set library to Library B
- Check item in again
- Click "Confirm hold and transfer"
- get error: Can't call method "biblio" on an undefined value at /kohadevbox/koha/circ/returns.pl line 147
Comment 9 Nick Clemens 2020-09-04 16:22:05 UTC
Created attachment 109678 [details] [review]
Bug 25969: Don't set itemnumber as reserve->itemnumber

When a hold is checked in at the wrong destination we revert the found status
For a title level hold this means we return it to a title level hold, with no itemnumber

When we checkin the item we find the hold, but won't set the itemnumber in the hold until it is confirmed

In the script we were setting the itemnumber param, but then setting it again using the reserve itemnumber.
In all conditions before the itemnumber has been set, we don't need to set it again, especially when we do
so incorrectly

To test:
- Log in at Library A
- Place hold on item at Library A, for pickup at Library A
- Check item in at Library A
- Click "Confirm hold"
- Set library to Library B
- Check item in again (try with both the box on reutrns.pl and the box in the header)
- Click "Confirm hold and transfer"
- get error: Can't call method "biblio" on an undefined value at /kohadevbox/koha/circ/returns.pl line 147
- apply patch
- repeat
- success!
- confirm trappings of holds works as normally otherwise too
Comment 10 Nick Clemens 2020-09-04 16:22:15 UTC
*** Bug 26386 has been marked as a duplicate of this bug. ***
Comment 11 Andrew Fuerste-Henry 2020-09-08 15:15:12 UTC
Created attachment 109755 [details] [review]
Bug 25969: Checking in a found hold at a different branch then confirming the hold causes internal server error

If a record level hold is filled and waiting at Library A, and the item
is checked in at Library B, and an attempt is made to re-fill the hold
with the item, Koha will return an ISE.

Test Plan:
1) Place a hold for Library A, at Library A, for pickup at Library A
2) Check in the item at Library A and fill the hold so it is waiting at Library A
3) Log in as Library B, check in the same barcode
4) Note the request to fill the hold with the item
5) Choose to fill the hold
6) Note you get an internal server error
7) Apply this patch
8) Restart all the things!
9) Repeat steps 1-5
10) No ISE!

Signed-off-by: Daniel Gaghan <daniel.gaghan@pueblolibrary.org>
Comment 12 Andrew Fuerste-Henry 2020-09-08 15:15:16 UTC
Created attachment 109756 [details] [review]
Bug 25969: Don't set itemnumber as reserve->itemnumber

When a hold is checked in at the wrong destination we revert the found status
For a title level hold this means we return it to a title level hold, with no itemnumber

When we checkin the item we find the hold, but won't set the itemnumber in the hold until it is confirmed

In the script we were setting the itemnumber param, but then setting it again using the reserve itemnumber.
In all conditions before the itemnumber has been set, we don't need to set it again, especially when we do
so incorrectly

To test:
- Log in at Library A
- Place hold on item at Library A, for pickup at Library A
- Check item in at Library A
- Click "Confirm hold"
- Set library to Library B
- Check item in again (try with both the box on reutrns.pl and the box in the header)
- Click "Confirm hold and transfer"
- get error: Can't call method "biblio" on an undefined value at /kohadevbox/koha/circ/returns.pl line 147
- apply patch
- repeat
- success!
- confirm trappings of holds works as normally otherwise too

Signed-off-by: Daniel Gaghan <daniel.gaghan@pueblolibrary.org>
Comment 13 Katrin Fischer 2020-09-12 22:57:29 UTC
Created attachment 110006 [details] [review]
Bug 25969: Checking in a found hold at a different branch then confirming the hold causes internal server error

If a record level hold is filled and waiting at Library A, and the item
is checked in at Library B, and an attempt is made to re-fill the hold
with the item, Koha will return an ISE.

Test Plan:
1) Place a hold for Library A, at Library A, for pickup at Library A
2) Check in the item at Library A and fill the hold so it is waiting at Library A
3) Log in as Library B, check in the same barcode
4) Note the request to fill the hold with the item
5) Choose to fill the hold
6) Note you get an internal server error
7) Apply this patch
8) Restart all the things!
9) Repeat steps 1-5
10) No ISE!

Signed-off-by: Daniel Gaghan <daniel.gaghan@pueblolibrary.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 14 Katrin Fischer 2020-09-12 22:57:35 UTC
Created attachment 110007 [details] [review]
Bug 25969: Don't set itemnumber as reserve->itemnumber

When a hold is checked in at the wrong destination we revert the found status
For a title level hold this means we return it to a title level hold, with no itemnumber

When we checkin the item we find the hold, but won't set the itemnumber in the hold until it is confirmed

In the script we were setting the itemnumber param, but then setting it again using the reserve itemnumber.
In all conditions before the itemnumber has been set, we don't need to set it again, especially when we do
so incorrectly

To test:
- Log in at Library A
- Place hold on item at Library A, for pickup at Library A
- Check item in at Library A
- Click "Confirm hold"
- Set library to Library B
- Check item in again (try with both the box on reutrns.pl and the box in the header)
- Click "Confirm hold and transfer"
- get error: Can't call method "biblio" on an undefined value at /kohadevbox/koha/circ/returns.pl line 147
- apply patch
- repeat
- success!
- confirm trappings of holds works as normally otherwise too

Signed-off-by: Daniel Gaghan <daniel.gaghan@pueblolibrary.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 15 Jonathan Druart 2020-09-18 09:02:08 UTC
Kyle, Nick, I have a follow-up patches for this, but I prefer to not push it as a RM follow-up but separated instead. Can you have a look at bug 26485 please?
Comment 16 Jonathan Druart 2020-09-18 09:53:40 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 17 Joonas Kylmälä 2020-10-01 09:57:55 UTC
The patch "Bug 25969: Don't set itemnumber as reserve->itemnumber" here breaks printing hold slip in returns.pl page if you try to print it more than once. So if you return the hold first once, it prints the slip fine and goes to waiting state, then if you check in the item another time and try to print the slip the Internal server error comes.
Comment 18 Jonathan Druart 2020-10-01 10:04:24 UTC
(In reply to Joonas Kylmälä from comment #17)
> The patch "Bug 25969: Don't set itemnumber as reserve->itemnumber" here
> breaks printing hold slip in returns.pl page if you try to print it more
> than once. So if you return the hold first once, it prints the slip fine and
> goes to waiting state, then if you check in the item another time and try to
> print the slip the Internal server error comes.

Nick or Kyle, can you have a look at this ASAP please?
Comment 19 Nick Clemens 2020-10-07 12:25:09 UTC
Created attachment 111328 [details] [review]
Bug 25969: (follow-up) If not coming from hold modal find item by barcode
Comment 20 Nick Clemens 2020-10-07 12:26:00 UTC
(In reply to Nick Clemens from comment #19)
> Created attachment 111328 [details] [review] [review]
> Bug 25969: (follow-up) If not coming from hold modal find item by barcode

This fixes the ISE, but if you click "Print slip and confirm" the modal launches again, and again, and again...

Pinging Kyle
Comment 21 Stefan Berndtsson 2020-10-07 12:33:55 UTC
(In reply to Nick Clemens from comment #20)
> (In reply to Nick Clemens from comment #19)
> > Created attachment 111328 [details] [review] [review] [review]
> > Bug 25969: (follow-up) If not coming from hold modal find item by barcode
> 
> This fixes the ISE, but if you click "Print slip and confirm" the modal
> launches again, and again, and again...
> 
> Pinging Kyle

That repeat is due to the change from Bug 25907 that adds the barcode to the modal request. This makes the system think there is another book being returned after the currently processed one. Since it's the same one it just keeps repeating.

I tried commenting out the barcode part from 25907 and that works (when itemnumber is provided), but the functionality from 25907 of course stops working properly then.
Comment 22 Nick Clemens 2020-10-07 15:36:50 UTC
Created attachment 111344 [details] [review]
Bug 26627: Print and confirming a hold can cause an infinite loop

Test plan:
1) Place a hold for pickup at library A
2) Confirm the hold and print slip
3) Checkin the item again
4) Confirm the hold and print slip
5) The modal returns, do it again
6) Apply this patch
7) Reload the page
8) Repeat steps 1-3
9) No loop!

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 23 Joonas Kylmälä 2020-10-23 11:01:50 UTC
(In reply to Joonas Kylmälä from comment #17)
> The patch "Bug 25969: Don't set itemnumber as reserve->itemnumber" here
> breaks printing hold slip in returns.pl page if you try to print it more
> than once. So if you return the hold first once, it prints the slip fine and
> goes to waiting state, then if you check in the item another time and try to
> print the slip the Internal server error comes.

This issue doesn't happen anymore with the latest master version. I'm not exactly sure which commit did the fix. So the patch "Bug 25969: (follow-up) If not coming from hold modal find item by barcode" is not at least required anymore to fix this, I don't know if it would be useful in other cases.
Comment 24 Jonathan Druart 2020-11-06 14:46:59 UTC
Comment on attachment 111328 [details] [review]
Bug 25969: (follow-up) If not coming from hold modal find item by barcode

This has not been pushed.