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.
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.
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>
Seems like we are missing something as: <input type="hidden" name="op" value="cud-show" />
Created attachment 185970 [details] [review] Bug 38967: Add hidden op param to POST form
(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.
(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.
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>
Nice work everyone! Pushed to main for 25.11
Great news! Any chance for a backport for this bug fix?
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. ```
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
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.
I have to admit I don't remember. I think it's good to not enforce POST for this export.
(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?
(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).
(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.
(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
(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.
Created attachment 186688 [details] [review] Bug 38967: Add an exception for itemsearch.tt Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment on attachment 186688 [details] [review] Bug 38967: Add an exception for itemsearch.tt You can have more than on form in a file!
(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.
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" ) {
(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.
follow-up pushed to main
Nice work everyone! Pushed to 25.05.x
Pushed to 24.11.x for 24.11.10