Bug 37192 - Can't print label from the item editor
Summary: Can't print label from the item editor
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Label/patron card printing (show other bugs)
Version: Main
Hardware: All All
: P5 - low major
Assignee: Lucas Gass (lukeg)
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords: regression
: 37267 37297 (view as bug list)
Depends on: 36192
Blocks:
  Show dependency treegraph
 
Reported: 2024-06-26 02:36 UTC by Phil Ringnalda
Modified: 2024-10-07 19:18 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This fixes a 500 error that occurs when attempting to print a label for an item in the staff interface (from the record details page > Edit > Edit items > Actions > Print label (for a specific item). The label batch editor now opens (as expected).
Version(s) released in:
24.11.00,24.05.04
Circulation function:


Attachments
Bug 37192: Make label printing from the item editor work by accepting the op add (1.72 KB, patch)
2024-06-26 03:28 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 37192: Make label printing from the item editor work by accepting the op add (1.77 KB, patch)
2024-06-26 14:46 UTC, Eric Garcia
Details | Diff | Splinter Review
Bug 37192: Add ability to pass data-new_tab to form-submit.js and open in new tab (877 bytes, patch)
2024-07-11 16:14 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 37192: Adjust template for additem.tt (2.67 KB, patch)
2024-07-11 16:14 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 37192: Add ability to pass data-new_tab to form-submit.js and open in new tab (932 bytes, patch)
2024-07-11 17:01 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 37192: Adjust template for additem.tt (2.72 KB, patch)
2024-07-11 17:01 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 37192: Add ability to pass data-new_tab to form-submit.js and open in new tab (999 bytes, patch)
2024-07-17 15:17 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 37192: Adjust template for additem.tt (2.79 KB, patch)
2024-07-17 15:17 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Phil Ringnalda 2024-06-26 02:36:55 UTC
Steps to reproduce:

1. Search the catalog for anything with results, click Edit items on any bib with 
   an existing item.
2. Click the Actions menu at the left of any item, select Print label

That 500 error you just saw happened because the Print label action opens a popup at /cgi-bin/koha/labels/label-edit-batch.pl?op=add&number_type=itemnumber&number_list=633 which used to create a new label batch with that item in it, but now falls through the code past the "$op eq cud-add" because other parts of the label batch UI want to POST things to add so the op changed to cud-add, and the attempt to add winds up in code that thinks you are editing an existing batch, and makes crashing assumptions about what will be defined.

Given that we don't believe that creating a label batch is worthy of CSRF protection, and we don't believe that removing an item from an existing batch is worthy of CSRF protection, I would argue that we should just make the if for add be elsif ($op eq 'cud-add' || $op eq 'add') rather than go through all the gyrations necessary to open a popup and have it immediately and silently POST a form just to add an item to a newly created batch.
Comment 1 Phil Ringnalda 2024-06-26 03:28:17 UTC
Created attachment 168117 [details] [review]
Bug 37192: Make label printing from the item editor work by accepting the op add

Some parts of the UI for adding items to a label batch (and there's lots of
different UI to do it) want to POST items to add to a batch, so CSRF
protection switched the op to add an item to cud-add. However, the item
editor needs to create a batch of one from its Actions menu by just opening
a popup, so it wants to GET ?op=add. Accepting both ops will keep all the
variety of UIs happy.

Test plan:
1. Search the catalog for something that will return results, like Perl
2. On any bib that has at least one item, click Edit items
3. In the Actions menu to the left of any item, choose Print label
4. That opened a popup with a 500 error rather than a label batch editor
5. Apply patch, restart_all
6. Repeat step 3, but this time you'll get a label batch editor with your
   item in it.
Comment 2 Eric Garcia 2024-06-26 14:46:37 UTC
Created attachment 168151 [details] [review]
Bug 37192: Make label printing from the item editor work by accepting the op add

Some parts of the UI for adding items to a label batch (and there's lots of
different UI to do it) want to POST items to add to a batch, so CSRF
protection switched the op to add an item to cud-add. However, the item
editor needs to create a batch of one from its Actions menu by just opening
a popup, so it wants to GET ?op=add. Accepting both ops will keep all the
variety of UIs happy.

Test plan:
1. Search the catalog for something that will return results, like Perl
2. On any bib that has at least one item, click Edit items
3. In the Actions menu to the left of any item, choose Print label
4. That opened a popup with a 500 error rather than a label batch editor
5. Apply patch, restart_all
6. Repeat step 3, but this time you'll get a label batch editor with your
   item in it.

Signed-off-by: Eric Garcia <cubingguy714@gmail.com>
Comment 3 Marcel de Rooy 2024-06-28 08:05:33 UTC
Security always has a price ;)
I understand that it is easier to do it like this than adding much more code to achieve the same.
But before we open the doors again for exceptions here and there, and compromise our security somewhere along the way, the release team should discuss and have concensus about that.

Moving to ID
Copying Martin as new QAM and Jonathan as CSRF expert.
Comment 4 Owen Leonard 2024-06-28 11:41:28 UTC
(In reply to Phil Ringnalda from comment #0)
> we don't believe that removing an item from an existing
> batch is worthy of CSRF protection

I believe so, and filed Bug 37206.
Comment 5 Phil Ringnalda 2024-06-28 18:28:35 UTC
Okay, I think I've got it, but unassigning because I don't have enough time to do it right before I go on vacation. I should be old enough by now to know not to ever touch printing, because in every project it's canonically a tar-baby that's going to stick other bugs to you.

What this Print label feature should be doing is not "add", because cud-add is actually two ops with one name: cud_add with a non-zero batch_id is add, cud_add with batch_id=0 or batch_id omitted is create-and-add, because new is very much not cud_new, it is show-blank-form.

What this feature should be doing is calling new, with added behavior for new to stick a passed &item_id=n into the add textarea, where the user can then click Add to create-and-add, an awkward but necessary step.

And ugh, someone really needs to fix bug 37223 because that's horrid behavior.
Comment 6 Phil Ringnalda 2024-07-06 18:15:24 UTC
Meh, that's wrong.

The user doesn't need to sit considering the repercussions of cud-add writing a line to creator_batches, they just need to show their csrf_token visa to be allowed in, and the link should just cud-add with form-submit.js. We just need bug 37267 to let the link keep opening a new window.
Comment 7 Phil Ringnalda 2024-07-10 19:30:49 UTC
*** Bug 37297 has been marked as a duplicate of this bug. ***
Comment 8 Lucas Gass (lukeg) 2024-07-11 16:14:43 UTC
Created attachment 168839 [details] [review]
Bug 37192: Add ability to pass data-new_tab to form-submit.js and open in new tab
Comment 9 Lucas Gass (lukeg) 2024-07-11 16:14:45 UTC
Created attachment 168840 [details] [review]
Bug 37192: Adjust template for additem.tt

Test plan:
1. Search the catalog for something that will return results, like Perl
2. On any bib that has at least one item, click Edit items
3. In the Actions menu to the left of any item, choose Print label
4. That opened a popup with a 500 error rather than a label batch editor
5. Apply patch, restart_all
6. Repeat step 3, but this time you'll get a label batch editor with your
   item in it.
Comment 10 ByWater Sandboxes 2024-07-11 17:01:42 UTC
Created attachment 168851 [details] [review]
Bug 37192: Add ability to pass data-new_tab to form-submit.js and open in new tab

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Comment 11 ByWater Sandboxes 2024-07-11 17:01:44 UTC
Created attachment 168852 [details] [review]
Bug 37192: Adjust template for additem.tt

Test plan:
1. Search the catalog for something that will return results, like Perl
2. On any bib that has at least one item, click Edit items
3. In the Actions menu to the left of any item, choose Print label
4. That opened a popup with a 500 error rather than a label batch editor
5. Apply patch, restart_all
6. Repeat step 3, but this time you'll get a label batch editor with your
   item in it.

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Comment 12 Phil Ringnalda 2024-07-11 18:00:12 UTC
*** Bug 37267 has been marked as a duplicate of this bug. ***
Comment 13 Martin Renvoize (ashimema) 2024-07-17 15:17:26 UTC
Created attachment 169089 [details] [review]
Bug 37192: Add ability to pass data-new_tab to form-submit.js and open in new tab

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize (ashimema) 2024-07-17 15:17:29 UTC
Created attachment 169090 [details] [review]
Bug 37192: Adjust template for additem.tt

Test plan:
1. Search the catalog for something that will return results, like Perl
2. On any bib that has at least one item, click Edit items
3. In the Actions menu to the left of any item, choose Print label
4. That opened a popup with a 500 error rather than a label batch editor
5. Apply patch, restart_all
6. Repeat step 3, but this time you'll get a label batch editor with your
   item in it.

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize (ashimema) 2024-07-17 15:17:53 UTC
Pragmatic solution without re-writing the whole of Koha.

Passing QA
Comment 16 Lucas Gass (lukeg) 2024-07-17 15:18:47 UTC
Maybe this has already been done somewhere else but if not, we should document how to use form-submit.js somewhere.
Comment 17 Katrin Fischer 2024-07-18 15:57:10 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 18 CJ Lynce 2024-07-19 19:26:58 UTC
Could this be backported to 24.05?
Comment 19 Lucas Gass (lukeg) 2024-09-06 16:27:57 UTC
Backported to 24.05.x for upcoming 24.05.04
Comment 20 Fridolin Somers 2024-09-17 13:53:39 UTC
Not for 23.11.x
Comment 21 Lucas Gass (lukeg) 2024-10-07 19:18:10 UTC
fixes a bug, nothing to document