Bug 38967 - Export to CSV or Barcode file from item search results fail when "select visible rows" and many items are selected
Summary: Export to CSV or Barcode file from item search results fail when "select visi...
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Lucas Gass (lukeg)
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2025-01-24 20:15 UTC by Kristi Krueger
Modified: 2025-10-16 10:03 UTC (History)
6 users (show)

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


Attachments
Bug 38967: Make exportItems use POST (2.42 KB, patch)
2025-08-22 19:51 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 38967: Make exportItems use POST (2.47 KB, patch)
2025-08-25 14:30 UTC, Eric Garcia
Details | Diff | Splinter Review
Bug 38967: Add hidden op param to POST form (1.05 KB, patch)
2025-08-29 20:48 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 38967: Make exportItems use POST (2.57 KB, patch)
2025-09-19 08:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 38967: (follow-up) Force 'op' parameter (1.26 KB, patch)
2025-09-19 21:21 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 38967: Add an exception for itemsearch.tt (1.08 KB, patch)
2025-09-22 12:52 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kristi Krueger 2025-01-24 20:15:03 UTC
To replicate: 
1. Go to item search. 
2. Enter parameters that retrieve more than 500 items.
3. From the item search results page, show All entries, then "Select visible rows."
4. Select either Export selected results to CSV or Barcode file.
5. A resulting error message of either "This site can't be reached. The connection was reset" or a 400 Bad request page with a URL with all the item numbers inserted appears.
Comment 1 Lucas Gass (lukeg) 2025-08-22 19:51:05 UTC
Created attachment 185724 [details] [review]
Bug 38967: Make exportItems use POST

To test:
1. Go to item search.
2. Enter parameters that retrieve more than 500 items.
3. From the item search results page, show All entries, then "Select visible rows."
4. Select either Export selected results to CSV or Barcode file.
5. A resulting error message of either "This site can't be reached. The connection was reset" or a 400 Bad request page with a URL with all the item numbers inserted appears.
6. APPLY PATCH
Try steps 1 - 4 again, this time it should export.
Comment 2 Eric Garcia 2025-08-25 14:30:05 UTC
Created attachment 185743 [details] [review]
Bug 38967: Make exportItems use POST

To test:
1. Go to item search.
2. Enter parameters that retrieve more than 500 items.
3. From the item search results page, show All entries, then "Select visible rows."
4. Select either Export selected results to CSV or Barcode file.
5. A resulting error message of either "This site can't be reached. The connection was reset" or a 400 Bad request page with a URL with all the item numbers inserted appears.
6. APPLY PATCH
Try steps 1 - 4 again, this time it should export.

Signed-off-by: Eric Garcia <cubingguy714@gmail.com>
Comment 3 Marcel de Rooy 2025-08-29 07:02:29 UTC
Seems like we are missing something as:
        <input type="hidden" name="op" value="cud-show" />
Comment 4 Lucas Gass (lukeg) 2025-08-29 20:48:15 UTC
Created attachment 185970 [details] [review]
Bug 38967: Add hidden op param to POST form
Comment 5 Jonathan Druart 2025-09-16 08:51:27 UTC
(In reply to Marcel de Rooy from comment #3)
> Seems like we are missing something as:
>         <input type="hidden" name="op" value="cud-show" />

Do we really need it? If we want it, we should add a check in the controller otherwise it's useless.
Comment 6 Marcel de Rooy 2025-09-19 08:42:50 UTC
(In reply to Jonathan Druart from comment #5)
> (In reply to Marcel de Rooy from comment #3)
> > Seems like we are missing something as:
> >         <input type="hidden" name="op" value="cud-show" />
> 
> Do we really need it? If we want it, we should add a check in the controller
> otherwise it's useless.

Ah yes. If there is no op, we pass the Middleware.
Comment 7 Marcel de Rooy 2025-09-19 08:43:44 UTC
Created attachment 186611 [details] [review]
Bug 38967: Make exportItems use POST

To test:
1. Go to item search.
2. Enter parameters that retrieve more than 500 items.
3. From the item search results page, show All entries, then "Select visible rows."
4. Select either Export selected results to CSV or Barcode file.
5. A resulting error message of either "This site can't be reached. The connection was reset" or a 400 Bad request page with a URL with all the item numbers inserted appears.
6. APPLY PATCH
Try steps 1 - 4 again, this time it should export.

Signed-off-by: Eric Garcia <cubingguy714@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Lucas Gass (lukeg) 2025-09-19 14:44:49 UTC
Nice work everyone!

Pushed to main for 25.11
Comment 9 Kristi Krueger 2025-09-19 15:36:12 UTC
Great news! Any chance for  a backport for this bug fix?
Comment 10 Tomás Cohen Arazi (tcohen) 2025-09-19 21:05:07 UTC
This broke some tests.

```
$ prove xt/find-missing-op-in-forms.t
xt/find-missing-op-in-forms.t .. 2/3
#   Failed test 'The <form> in the following files are missing it's corresponding op parameter, or op does not start with 'cud-' (see bug 34478)'
#   at xt/find-missing-op-in-forms.t line 39.
#          got: '1'
#     expected: '0'
# $VAR1 = 'koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/itemsearch.tt:447';
# Looks like you failed 1 test of 3.
```
Comment 11 Tomás Cohen Arazi (tcohen) 2025-09-19 21:21:07 UTC
Created attachment 186629 [details] [review]
Bug 38967: (follow-up) Force 'op' parameter

This fixes a failing test:

To test:
1. Run:
   $ ktd --shell
  k$ prove xt/find-missing-op-in-forms.t
xt/find-missing-op-in-forms.t .. 2/3
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests pass!
4. Sign off :-D
Comment 12 Tomás Cohen Arazi (tcohen) 2025-09-19 21:22:22 UTC
I'm not sure if the proposed solution is correct or if we prefer to add an exception in the tests.

Adding Jonathan in case there's a strong opinion.
Comment 13 Jonathan Druart 2025-09-22 09:54:00 UTC
I have to admit I don't remember.

I think it's good to not enforce POST for this export.
Comment 14 Marcel de Rooy 2025-09-22 10:38:38 UTC
(In reply to Jonathan Druart from comment #13)
> I have to admit I don't remember.
> 
> I think it's good to not enforce POST for this export.

Wasnt this about a too long URL?
Shouldnt we just add an exception to the failing unit test here?
Comment 15 Jonathan Druart 2025-09-22 10:58:46 UTC
(In reply to Marcel de Rooy from comment #14)
> (In reply to Jonathan Druart from comment #13)
> > I have to admit I don't remember.
> > 
> > I think it's good to not enforce POST for this export.
> 
> Wasnt this about a too long URL?

Yes.

> Shouldnt we just add an exception to the failing unit test here?

No, we have other place where we have "dump cud-" for this same reason.

I meant that we can keep the op=cud- in the template without enforcing it in the controller (ie. allow the export for GET requests as well).
Comment 16 Tomás Cohen Arazi (tcohen) 2025-09-22 12:34:36 UTC
(In reply to Jonathan Druart from comment #15)
> > Shouldnt we just add an exception to the failing unit test here?
> 
> No, we have other place where we have "dump cud-" for this same reason.
> 
> I meant that we can keep the op=cud- in the template without enforcing it in
> the controller (ie. allow the export for GET requests as well).

That's what my follow-up does. But shouldn't it be better to explicitly add an exception in the tests? That's why I didn't push the follow-up on behalf of Lucas.
Comment 17 Marcel de Rooy 2025-09-22 12:37:35 UTC
(In reply to Tomás Cohen Arazi (tcohen) from comment #16)
> (In reply to Jonathan Druart from comment #15)
> > > Shouldnt we just add an exception to the failing unit test here?
> > 
> > No, we have other place where we have "dump cud-" for this same reason.
> > 
> > I meant that we can keep the op=cud- in the template without enforcing it in
> > the controller (ie. allow the export for GET requests as well).
> 
> That's what my follow-up does. But shouldn't it be better to explicitly add
> an exception in the tests? That's why I didn't push the follow-up on behalf
> of Lucas.

And it is does the same as the follow-up of Lukas which we obsoleted..
If we add a test exception, we can remove the op parameter which is not checked in the controller.
But left or right does not matter much here imo
Comment 18 Tomás Cohen Arazi (tcohen) 2025-09-22 12:47:10 UTC
(In reply to Marcel de Rooy from comment #17)
> (In reply to Tomás Cohen Arazi (tcohen) from comment #16)
> > (In reply to Jonathan Druart from comment #15)
> > > > Shouldnt we just add an exception to the failing unit test here?
> > > 
> > > No, we have other place where we have "dump cud-" for this same reason.
> > > 
> > > I meant that we can keep the op=cud- in the template without enforcing it in
> > > the controller (ie. allow the export for GET requests as well).
> > 
> > That's what my follow-up does. But shouldn't it be better to explicitly add
> > an exception in the tests? That's why I didn't push the follow-up on behalf
> > of Lucas.
> 
> And it is does the same as the follow-up of Lukas which we obsoleted..
> If we add a test exception, we can remove the op parameter which is not
> checked in the controller.
> But left or right does not matter much here imo

I didn't notice the obsoleted patch. It makes sense.


+1 for adding an exception. it is clearer -for the time being- than a weird "op" param to workaround our own checks.
Comment 19 Tomás Cohen Arazi (tcohen) 2025-09-22 12:52:07 UTC
Created attachment 186688 [details] [review]
Bug 38967: Add an exception for itemsearch.tt

Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 20 Jonathan Druart 2025-09-22 13:24:53 UTC
Comment on attachment 186688 [details] [review]
Bug 38967: Add an exception for itemsearch.tt

You can have more than on form in a file!
Comment 21 Jonathan Druart 2025-09-22 13:25:50 UTC
(In reply to Jonathan Druart from comment #20)
> Comment on attachment 186688 [details] [review] [review]
> Bug 38967: Add an exception for itemsearch.tt
> 
> You can have more than on form in a file!

You can have more than one form in a template file, I really don't think this is the solution.

We already have several occurrences of dumb "cud-", I think we should continue to stick to this pattern.
Comment 22 Jonathan Druart 2025-09-22 13:27:25 UTC
eg.

koha-tmpl/intranet-tmpl/prog/en/includes/cat-toolbar.inc
 86                             <form action="/cgi-bin/koha/tools/batchMod.pl" method="post">
 87                                 [% INCLUDE 'csrf-token.inc' %]
 88                                 <input type="hidden" name="op" value="cud-show" />
 89                                 <input type="hidden" name="del" value="1" />
 90                                 <input type="hidden" name="biblionumber" value="[% biblionumber | html %]" />
 91                                 <input type="hidden" name="src" value="CATALOGUING" />
 92                                 <button id="batchdelete" type="submit" class="dropdown-item">Delete items in a batch</button>
 93                             </form>


tools/batchMod.pl
223 if ( $op eq "cud-show" || $op eq "show" ) {
Comment 23 Tomás Cohen Arazi (tcohen) 2025-09-22 13:29:57 UTC
(In reply to Jonathan Druart from comment #21)
> (In reply to Jonathan Druart from comment #20)
> > Comment on attachment 186688 [details] [review] [review] [review]
> > Bug 38967: Add an exception for itemsearch.tt
> > 
> > You can have more than on form in a file!
> 
> You can have more than one form in a template file, I really don't think
> this is the solution.
> 
> We already have several occurrences of dumb "cud-", I think we should
> continue to stick to this pattern.

Yes, I kept thinking about it. In this particular case it is even worse as the form is actually generated through JS.
Comment 24 Lucas Gass (lukeg) 2025-09-22 22:27:08 UTC
follow-up pushed to main
Comment 25 Paul Derscheid 2025-09-25 16:30:00 UTC
Nice work everyone!

Pushed to 25.05.x
Comment 26 Fridolin Somers 2025-10-16 10:03:36 UTC
Pushed to 24.11.x for 24.11.10