Bug 26627 - Print and confirming a hold can cause an infinite loop
Summary: Print and confirming a hold can cause an infinite loop
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on: 25907 25969 26485
Blocks:
  Show dependency treegraph
 
Reported: 2020-10-07 13:00 UTC by Nick Clemens
Modified: 2020-11-17 20:43 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.06


Attachments
Bug 26627: Print and confirming a hold can cause an infinite loop (1.10 KB, patch)
2020-10-07 15:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26627: Print and confirming a hold can cause an infinite loop (1.22 KB, patch)
2020-10-14 16:35 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 26627: Print and confirming a hold can cause an infinite loop (1.29 KB, patch)
2020-10-15 22:14 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 26627: (QA follow-up) Remove barcode input by id (1.62 KB, patch)
2020-11-05 11:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26627: (QA follow-up) Remove only the loop causing input field (2.33 KB, patch)
2020-11-06 11:18 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 26627: (QA follow-up) Remove barcode input by id (1.68 KB, patch)
2020-11-06 13:27 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26627: (QA follow-up) Remove barcode input by id (1.81 KB, patch)
2020-11-06 13:34 UTC, Joonas Kylmälä
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2020-10-07 13:00:26 UTC
See bug 25907 and comment 21 from bug 25969

To recreate:
1 - Place a hold for pickup at library A
2 - Confirm the hold and print slip
3 - Checkin the item again
3 - Confirm the hold and print slip
4 - The modal returns, do it again
5 - and again
6 - and again
Comment 1 Kyle M Hall 2020-10-07 15:03:14 UTC
Created attachment 111337 [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!
Comment 2 Jonathan Druart 2020-10-08 12:30:25 UTC
Nick, why did you link this bug report with bug 26485?
Comment 3 Nick Clemens 2020-10-08 13:14:24 UTC
(In reply to Jonathan Druart from comment #2)
> Nick, why did you link this bug report with bug 26485?

I tested them all together, without the dependencies I was still seeing issues
Comment 4 Séverine Queune 2020-10-14 16:35:11 UTC
Created attachment 111635 [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: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 5 Katrin Fischer 2020-10-15 22:14:41 UTC
Created attachment 111805 [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: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 6 Katrin Fischer 2020-10-15 22:15:45 UTC
Interesting bug! Note: it has to be "print slip..." with just confirm the loop doesn't happen.
Comment 7 Jonathan Druart 2020-10-25 23:07:53 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 8 Joonas Kylmälä 2020-11-05 10:33:50 UTC
The fix here seems to have created another problem, it causes the button "Print slip, transfer, and confirm" to not do anything when clicked. In Javascript console I see

Uncaught TypeError: this.form.barcode is undefined
/cgi-bin/koha/circ/returns.pl:31317
Comment 9 Kyle M Hall 2020-11-05 11:41:22 UTC
Created attachment 113093 [details] [review]
Bug 26627: (QA follow-up) Remove barcode input by id
Comment 10 Magnus Enger 2020-11-06 10:16:06 UTC
It looks like the first patch is "Pushed to master", but nothin has been done about the second patch from Kyle. Should it go on a separate bug, or should the status here be reset to "Needs signoff"? Joonas: Have you tested if the second patch solves your problem? 

BTW: This needs to be backported to 20.05.x, I think.
Comment 11 Joonas Kylmälä 2020-11-06 10:34:06 UTC
I just tested and the QA follow-up here didn't solve the issue, in browser console I get:

document.getElementById(...) is null

So I'm creating first a hold to item and select a pickup location to something other than where the item is currently, then I check-in the item and I want to confirm the hold and print slip & transfer but this happens. Let me see if I will find a fix for this.
Comment 12 Joonas Kylmälä 2020-11-06 10:35:29 UTC
My guess is that this would work if the hold was item level, but I made biblio level hold meaning there is no specific barcode attached to the hold, thus the error.
Comment 13 Joonas Kylmälä 2020-11-06 11:18:46 UTC
Created attachment 113237 [details] [review]
Bug 26627: (QA follow-up) Remove only the loop causing input field

The loop reported originally happened only in this one specific form
which this commit now removes the input field from. There is no
need to remove the barcode from all the forms (even ones that don't
have it). Because the JavaScript tried to remove the barcode input
field from other forms as well we got following error when clicking
"Print slip, transfer, and confirm" for a hold that needed transfer:

Uncaught TypeError: this.form.barcode is undefined

Test plan:
1) Create biblio level hold that needs a transfer
2) Check-in the item and click "Print slip, transfer, and confirm"
3) Notice nothing happens, apply patch & restart, notice the slip will
be now printed
4) Follow the test plan from commit a71596fb39
   (Bug 26627: Print and confirming a hold can cause an infinite loop)

Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 14 Joonas Kylmälä 2020-11-06 11:25:11 UTC
I re-did now basically the original patch by removing the unneeded HTML input directly from the HTML code instead of using Javascript to do it. Asking now for someone else to sign-off this as well.
Comment 15 Kyle M Hall 2020-11-06 13:21:09 UTC
(In reply to Joonas Kylmälä from comment #14)
> I re-did now basically the original patch by removing the unneeded HTML
> input directly from the HTML code instead of using Javascript to do it.
> Asking now for someone else to sign-off this as well.

This patch has effectively undone bug 25907 and would create a regression.
Comment 16 Kyle M Hall 2020-11-06 13:27:03 UTC
Created attachment 113249 [details] [review]
Bug 26627: (QA follow-up) Remove barcode input by id
Comment 17 Kyle M Hall 2020-11-06 13:27:52 UTC
I think this latest version should solve all the problems! Would you mind testing? Thanks!
Comment 18 Joonas Kylmälä 2020-11-06 13:29:25 UTC
(In reply to Kyle M Hall from comment #16)
> Created attachment 113249 [details] [review] [review]
> Bug 26627: (QA follow-up) Remove barcode input by id

Thanks, I was just commenting the following and your patch seems to be what I wanted:

> I think we should have separate forms for each one of the actions (confirm, 
> print and confirm, cancel). Though it would be nice to have a fast fix for this 
> without huge possibility of regression so maybe adding and if check whether the
> HTML element this.form.barcode exists would be good enough solution for now.

I will test now your patch and sign-it off.
Comment 19 Joonas Kylmälä 2020-11-06 13:34:57 UTC
Created attachment 113250 [details] [review]
Bug 26627: (QA follow-up) Remove barcode input by id

Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 20 Joonas Kylmälä 2020-11-06 13:36:04 UTC
Works now as expected, signing-off.
Comment 21 Jonathan Druart 2020-11-06 14:56:58 UTC
(In reply to Joonas Kylmälä from comment #19)
> Created attachment 113250 [details] [review] [review]
> Bug 26627: (QA follow-up) Remove barcode input by id
> 
> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>

Patch pushed to master for 20.11
Comment 22 Lucas Gass 2020-11-13 20:44:13 UTC
backported to 20.05.x for 20.05.06
Comment 23 Aleisha Amohia 2020-11-17 04:40:58 UTC
missing dependencies, not backported to 19.11.x