[Bug 37197] New: Batch patron modification from reports fails by using GET instead of POST
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37197 Bug ID: 37197 Summary: Batch patron modification from reports fails by using GET instead of POST Change sponsored?: --- Product: Koha Version: Main Hardware: All OS: All Status: NEW Keywords: regression Severity: major Priority: P5 - low Component: Reports Assignee: koha-bugs@lists.koha-community.org Reporter: phil@chetcolibrary.org QA Contact: testopia@bugs.koha-community.org Depends on: 36192 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/8a20e0cb6a0efd6b99... they are sent in a GET with the op show (which no longer exists) rather than a POST with cud-show. Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36192 [Bug 36192] [OMNIBUS] CSRF Protection for Koha -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37197 Phil Ringnalda <phil@chetcolibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@gmail.com Assignee|koha-bugs@lists.koha-commun |phil@chetcolibrary.org |ity.org | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37197 Phil Ringnalda <phil@chetcolibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37197 --- Comment #1 from Phil Ringnalda <phil@chetcolibrary.org> --- Created attachment 168187 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=168187&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37197 Phil Ringnalda <phil@chetcolibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |37188 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37188 [Bug 37188] Batch patron modification from report results broken in 24.05 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37197 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37197 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #168187|0 |1 is obsolete| | --- Comment #2 from David Nind <david@davidnind.com> --- Created attachment 168287 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=168287&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37197 Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37197 Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #168287|0 |1 is obsolete| | --- Comment #3 from Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> --- Created attachment 168322 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=168322&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37197 Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |matt.blenkinsop@ptfs-europe | |.com QA Contact|testopia@bugs.koha-communit |matt.blenkinsop@ptfs-europe |y.org |.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37197 --- Comment #4 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37197 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Failed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37197 --- Comment #5 from Phil Ringnalda <phil@chetcolibrary.org> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37197 --- Comment #6 from Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> --- 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). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37197 Phil Ringnalda <phil@chetcolibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de --- Comment #7 from Phil Ringnalda <phil@chetcolibrary.org> --- (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/21fa3f64c31dad23e5... 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37197 Phil Ringnalda <phil@chetcolibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |In Discussion Assignee|phil@chetcolibrary.org |koha-bugs@lists.koha-commun | |ity.org --- Comment #8 from Phil Ringnalda <phil@chetcolibrary.org> --- 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37197 --- Comment #9 from Phil Ringnalda <phil@chetcolibrary.org> --- 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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37197 --- Comment #10 from Jonathan Druart <jonathan.druart@gmail.com> --- It should be cud-show and POST for this exact limitation problem. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37197 --- Comment #11 from Jonathan Druart <jonathan.druart@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37197 --- Comment #12 from Jonathan Druart <jonathan.druart@gmail.com> --- 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"> -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37197 --- Comment #13 from Phil Ringnalda <phil@chetcolibrary.org> --- 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? -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37197 --- Comment #14 from Jonathan Druart <jonathan.druart@gmail.com> --- (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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37197 --- Comment #15 from Jonathan Druart <jonathan.druart@gmail.com> --- We are doing that already for items actually tools/batchMod.pl 209 if ($op eq "cud-show" || $op eq "show"){ -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37197 --- Comment #16 from Phil Ringnalda <phil@chetcolibrary.org> --- Bug 36326 comment 21 makes it hard to claim that as a precedent, though. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37197 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com --- Comment #17 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- *** Bug 37479 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37197 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Passed QA --- Comment #18 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- 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 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37197 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |37481 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37481 [Bug 37481] Some operations that do not affect data should be submitted as POST requests -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37197 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |phil@chetcolibrary.org |ity.org | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37197 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |24.11.00 released in| | Status|Passed QA |Pushed to main -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37197 --- Comment #19 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Thanks for all the hard work! Pushed to main for the next 24.11.00 release as RM Assistant -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37197 Phil Ringnalda <phil@chetcolibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|24.11.00 | released in| | --- Comment #20 from Phil Ringnalda <phil@chetcolibrary.org> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37197 Phil Ringnalda <phil@chetcolibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |37615 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37615 [Bug 37615] Clean up the form for sending cardnumbers from a report to batch patron modification -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37197 --- Comment #21 from Phil Ringnalda <phil@chetcolibrary.org> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37197 Phil Ringnalda <phil@chetcolibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |rel_24_05_candidate -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37197 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to main |Pushed to stable Version(s)| |24.11.00,24.05.05 released in| | CC| |lucas@bywatersolutions.com --- Comment #22 from Lucas Gass <lucas@bywatersolutions.com> --- Backported to 24.05.x for upcoming 24.05.05 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37197 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|rel_24_05_candidate | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37197 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to stable |Needs documenting CC| |fridolin.somers@biblibre.co | |m --- Comment #23 from Fridolin Somers <fridolin.somers@biblibre.com> --- Not for 23.11.x -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37197 Phil Ringnalda <phil@chetcolibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs documenting |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37197 Bug 37197 depends on bug 36192, which changed state. Bug 36192 Summary: [OMNIBUS] CSRF Protection for Koha https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36192 What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org