When you SELECT cardnumber FROM borrowers you get a Batch operations menuitem to send the results to Patron batch modification, but because of what looks like it might be an accidental change in https://git.koha-community.org/Koha-community/Koha/commit/8a20e0cb6a0efd6b993e361d439dbb4486ba15d1 they are sent in a GET with the op show (which no longer exists) rather than a POST with cud-show.
Created attachment 168187 [details] [review] Bug 37197: Reports option to send to batch patron modification needs to use POST When you have an SQL report that selects cardnumber from borrowers, you get a menuitem to send the results to Batch patron modification. Currently that fails with a message about no card numbers or borrowernumbers given, because it tries to do a GET with the op show, when modborrowers.pl is expecting a POST with the op cud-show. Test plan: 1. Without the patch, Reports - Create from SQL - name it and paste select cardnumber from borrowers in the SQL textarea and save, then Run report. 2. Above the results is a "Batch operations with 20 visible records" menu, choose Batch patron modification 3. Note that instead of modifying the records, you're stuck with a message saying "No patron card numbers or borrowernumbers given." even though there's a whole string of them in the URL. 4. Apply patch, refresh the page with the results (oddly, no apparent need to even restart_all), and choose the menuitem again, but this time with a successful result.
Created attachment 168287 [details] [review] Bug 37197: Reports option to send to batch patron modification needs to use POST When you have an SQL report that selects cardnumber from borrowers, you get a menuitem to send the results to Batch patron modification. Currently that fails with a message about no card numbers or borrowernumbers given, because it tries to do a GET with the op show, when modborrowers.pl is expecting a POST with the op cud-show. Test plan: 1. Without the patch, Reports - Create from SQL - name it and paste select cardnumber from borrowers in the SQL textarea and save, then Run report. 2. Above the results is a "Batch operations with 20 visible records" menu, choose Batch patron modification 3. Note that instead of modifying the records, you're stuck with a message saying "No patron card numbers or borrowernumbers given." even though there's a whole string of them in the URL. 4. Apply patch, refresh the page with the results (oddly, no apparent need to even restart_all), and choose the menuitem again, but this time with a successful result. Signed-off-by: David Nind <david@davidnind.com>
Created attachment 168322 [details] [review] Bug 37197: Reports option to send to batch patron modification needs to use POST When you have an SQL report that selects cardnumber from borrowers, you get a menuitem to send the results to Batch patron modification. Currently that fails with a message about no card numbers or borrowernumbers given, because it tries to do a GET with the op show, when modborrowers.pl is expecting a POST with the op cud-show. Test plan: 1. Without the patch, Reports - Create from SQL - name it and paste select cardnumber from borrowers in the SQL textarea and save, then Run report. 2. Above the results is a "Batch operations with 20 visible records" menu, choose Batch patron modification 3. Note that instead of modifying the records, you're stuck with a message saying "No patron card numbers or borrowernumbers given." even though there's a whole string of them in the URL. 4. Apply patch, refresh the page with the results (oddly, no apparent need to even restart_all), and choose the menuitem again, but this time with a successful result. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
I am wondering if POST is the correct one here as we are not changing data but sending them to display in another tool. Why not GET and op=show?
My bib numbers are seven digits, so &b=nnnnnnn is ten digits, and a report showing 1000 rows per page would be a query string of 10,000 characters. Well, 9,999 since the first one wouldn't have an ampersand. I don't have any real feeling about what a true current answer to "what is the safe maximum size of a URL" would actually be, but there certainly are a lot of answers a lot smaller than that.
A bit of research suggests that Apache has a limit of 8177 characters (I'm assuming that Apache would be the limitation rather than the browser although this might be browser dependent).
(In reply to Phil Ringnalda from comment #0) > what looks like it might be an accidental change in > https://git.koha-community.org/Koha-community/Koha/commit/ > 8a20e0cb6a0efd6b993e361d439dbb4486ba15d1 That one probably was an accidental change. The clearly intentional one was https://git.koha-community.org/Koha-community/Koha/commit/21fa3f64c31dad23e50e633a6f51f98c8f7ade12 which did not work because it left the wrong op in the GET form in reports, and would have needed to put the multi_param('cardnumber') into a new op with a name that could take a GET.
But if something is wrong with the semantics of a hidden textarea in a URL that ends in guided_reports.pl POSTing to modborrowers.pl, the exact same thing is wrong with an unhidden textarea in a URL that ends in modborrowers.pl POSTing to modborrowers.pl, and the same thing is wrong with batch item and record modification, and batch item and record deletion, and they should all be changed to do a GET of op=show, and they should all break when Apache returns Request URI too long. And there's no semantic difference between uploading a textarea of cardnumbers and uploading a text file of cardnumbers, so while we're writing a little JS to convert the textarea content to a query string, we should also convert the file selected for upload to a query string too. Note that my patch *was* wrong, because I didn't see that I should have also undone the change from a hidden textarea to a bunch of hidden inputs.
Oh, hey, we do have an example of desperately trying to stay under the Apache URL length limit, Add to list concatenates biblionumbers separated by '/' and crams them into a single query param. That can save up to 1999 characters by replacing &b= with /. If someone fixed bug 37188 by making batch patron modification only an option when borrowernumber is selected (probably rarely more than 7 digits) instead of cardnumber (14 digits in my world), then Apache would only break it for large sites, and they really ought to have someone who knows how to edit Apache's conf files.
It should be cud-show and POST for this exact limitation problem.
koha-tmpl/intranet-tmpl/prog/en/modules/patron_lists/lists.tt 95 <li> 96 <a href="/cgi-bin/koha/tools/modborrowers.pl?patron_list_id=[% l.patron_list_id | uri %]&op=show"> 97 <i class="fa-solid fa-pencil" aria-hidden="true"></i> Batch edit patrons 98 </a> 99 </li> This one seems to be another problematic one.
And koha-tmpl/intranet-tmpl/prog/en/modules/patron_lists/patron-lists-tab.tt: <a href="/cgi-bin/koha/tools/modborrowers.pl?patron_list_id=[% l.patron_list_id | uri %]&op=show">
We have UI to batch modify patrons in a patron list that includes the patron you are checking items out to? Thanks, I hate it! Every action you can take with a patron list, batch modify, batch delete, and print cards, is broken for the same reason: they want to GET ?list_id=1 and the op takes a cud- POST. Do we need a new pattern for all of these, having two ops like get_a_little and cud-post_a_lot which each just get their input and call sub do_the_work? Or do we want to say that if anyone has so much data they have to POST it, then everybody has to POST even if it's just one digit they need to send?
(In reply to Phil Ringnalda from comment #13) > We have UI to batch modify patrons in a patron list that includes the patron > you are checking items out to? Thanks, I hate it! > > Every action you can take with a patron list, batch modify, batch delete, > and print cards, is broken for the same reason: they want to GET ?list_id=1 > and the op takes a cud- POST. > > Do we need a new pattern for all of these, having two ops like get_a_little > and cud-post_a_lot which each just get their input and call sub do_the_work? > Or do we want to say that if anyone has so much data they have to POST it, > then everybody has to POST even if it's just one digit they need to send? Good question. I have never considered we could support both, that's an interesting idea. We could indeed have "show" allowed for patron_list_id if it makes things here easier to fix.
We are doing that already for items actually tools/batchMod.pl 209 if ($op eq "cud-show" || $op eq "show"){
Bug 36326 comment 21 makes it hard to claim that as a precedent, though.
*** Bug 37479 has been marked as a duplicate of this bug. ***
I suggest we push forward this patch that fixes a bug - and move our discussion of how to make this consistent on to another bug
Thanks for all the hard work! Pushed to main for the next 24.11.00 release as RM Assistant
(In reply to Phil Ringnalda from comment #13) > Every action you can take with a patron list, batch modify, batch delete, > and print cards, is broken for the same reason: they want to GET ?list_id=1 > and the op takes a cud- POST. Actually, batch delete isn't broken, I just automatically use Staff patrons whenever I need a patron and batch patron deletion won't delete Staff. Filed bug 37612 for batch modify and bug 37614 for print cards.
(In reply to Phil Ringnalda from comment #8) > Note that my patch *was* wrong, because I didn't see that I should have also > undone the change from a hidden textarea to a bunch of hidden inputs. Filed bug 37615 for that.