|
Description
Jonathan Druart
2024-08-26 09:02:06 UTC
Created attachment 170701 [details] [review] Bug 37728: Adjust test to catch more missing 'op' in POSTed forms The test is not catching missing op in forms that are not the first POST form of the template. Found when fixing the test for bug 37309. It was not catching the missing op in the second form. So this is certainly hiding... more than 20 bugs. not ok 1 - The <form> in the following files are missing it's corresponding op parameter, or op does not start with 'cud-' (see bug 34478) # 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 43. # got: '20' # expected: '0' # $VAR1 = 'koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-illrequests.tt:260,293'; # $VAR2 = 'koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-passwd.tt:110'; # $VAR3 = 'koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-password-recovery.tt:152'; # $VAR4 = 'koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-shelves.tt:808'; # $VAR5 = 'koha-tmpl/intranet-tmpl/prog/en/modules/admin/aqbudgetperiods.tt:327'; # $VAR6 = 'koha-tmpl/intranet-tmpl/prog/en/modules/admin/aqcontract.tt:150,182'; # $VAR7 = 'koha-tmpl/intranet-tmpl/prog/en/modules/admin/aqplan.tt:209,270'; # $VAR8 = 'koha-tmpl/intranet-tmpl/prog/en/modules/admin/auth_tag_structure.tt:206,236'; # $VAR9 = 'koha-tmpl/intranet-tmpl/prog/en/modules/admin/currency.tt:183,205'; # $VAR10 = 'koha-tmpl/intranet-tmpl/prog/en/modules/admin/itemtypes.tt:382'; # $VAR11 = 'koha-tmpl/intranet-tmpl/prog/en/modules/admin/marc_subfields_structure.tt:333'; # $VAR12 = 'koha-tmpl/intranet-tmpl/prog/en/modules/admin/marctagstructure.tt:217'; # $VAR13 = 'koha-tmpl/intranet-tmpl/prog/en/modules/admin/systempreferences.tt:292,302'; # $VAR14 = 'koha-tmpl/intranet-tmpl/prog/en/modules/circ/pendingreserves.tt:290'; # $VAR15 = 'koha-tmpl/intranet-tmpl/prog/en/modules/installer/step3.tt:294'; # $VAR16 = 'koha-tmpl/intranet-tmpl/prog/en/modules/members/boraccount.tt:191'; # $VAR17 = 'koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt:1443'; # $VAR18 = 'koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-collection.tt:152'; # $VAR19 = 'koha-tmpl/intranet-tmpl/prog/en/modules/tags/review.tt:248,274'; # $VAR20 = 'koha-tmpl/intranet-tmpl/prog/en/modules/tools/letter.tt:547,589'; Not good, but good that we caught them now. Who can work on resolving these? I can help, but not let alone. I can't do things like opac-illrequests.tt which would require that I spend several hours of my employer's money learning how to write a test plan for something we don't use, but I can do some of them. Wish I would have realized how many of these were "a cancel deletion button which should be a GET, and an OK button in a deletion confirmed page which currently doesn't even show but should be a GET" before I filed them as separate bugs. Thanks, Phil. Much appreciated! You've been killing it lately :D. (In reply to Katrin Fischer from comment #3) > Who can work on resolving these? I'll help out as well. I'm a bit worried about the stability of 24.05 and the upcoming 24.11, so I the present is a great time to get as much done as we can. Not quite sure what to do with koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt:1443 - JS inserts the op in the form in line 1859, and the form won't work without JS since it's also inserting the reserve_id, and without a reserve_id you aren't going to cancel. Do we just stick the op in normally instead of inserting it, to make the test happy? Or is there a reason I'm not seeing that it needs to be inserted by JS? And although I was thinking of sticking it in normally right below the CSRF token, it would actually be much funnier to stick it in the empty <div id="inputs"> in the template, which would make the test happy, and then the very first thing the JS does is to empty() that div, so it can stay happy about putting in the op itself. Asked that question in the form of a Needs Signoff in bug 37783. Current status after applying the patch and following dependencies (minus the one that's filed but not patched): # $VAR1 = 'koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-illrequests.tt:260,293'; # $VAR2 = 'koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-passwd.tt:110'; # $VAR3 = 'koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-password-recovery.tt:152'; # $VAR4 = 'koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-shelves.tt:808'; # $VAR5 = 'koha-tmpl/intranet-tmpl/prog/en/modules/admin/aqbudgetperiods.tt:327'; # $VAR6 = 'koha-tmpl/intranet-tmpl/prog/en/modules/admin/aqcontract.tt:150,182'; # $VAR7 = 'koha-tmpl/intranet-tmpl/prog/en/modules/admin/aqplan.tt:209,270'; # $VAR8 = 'koha-tmpl/intranet-tmpl/prog/en/modules/installer/step3.tt:294'; # $VAR10 = 'koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-collection.tt:152'; I don't have any current plans to touch those, since they are in areas where I don't know either the code or the UI, so they are up for grabs. Thanks Phil, awesome job! I can grab the acquisitions ones. I'll see if I have time to SO/QA some of yours after (the day got away from me... D:) Thanks for jumping on so many, Phil! Shame I can't write a test plan for the opac-illrequests.tt one, because it looks like the patch would be an amusing [%# This will get its name="op" value="cud-" from the loop over whole.keys below %] to fake out the test. Should this be NSO? I could trivially sign off that it catches more missing 'op' in POSTed forms, but I can't sign off that it passes when there aren't any, since I haven't seen that. # $VAR1 = 'koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-illrequests.tt:260,293'; # $VAR2 = 'koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-password-recovery.tt:152'; # $VAR3 = 'koha-tmpl/intranet-tmpl/prog/en/modules/admin/aqcontract.tt:167'; # $VAR4 = 'koha-tmpl/intranet-tmpl/prog/en/modules/admin/aqplan.tt:209,270'; # $VAR5 = 'koha-tmpl/intranet-tmpl/prog/en/modules/installer/step3.tt:294'; # $VAR6 = 'koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-collection.tt:158'; Seriously, I'm done. Somebody take these. # $VAR1 = 'koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-illrequests.tt:260,293'; Requires having an ILL backend that does availability and knowing how to get to typedisclaimer and how to write a test plan so someone else can. # $VAR2 = 'koha-tmpl/intranet-tmpl/prog/en/modules/installer/step3.tt:294'; Requires knowing how to write an installer test plan, and knowing what on earth it's using to keep track of substeps. # $VAR3 = 'koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-collection.tt:158'; Should be easy for someone who uses serials-collection, and thus knows whether it needs to keep POSTing because sometimes you do stuff to so many things at once that you exceed Apache's URL maxlength if it's a GET, or not. Thanks a lot for your work, Phil! (In reply to Phil Ringnalda from comment #18) > Seriously, I'm done. Somebody take these. > > # $VAR1 = > 'koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-illrequests.tt:260,293'; > > Requires having an ILL backend that does availability and knowing how to get > to typedisclaimer and how to write a test plan so someone else can. Not familiar with this, Pedro maybe? > # $VAR2 = 'koha-tmpl/intranet-tmpl/prog/en/modules/installer/step3.tt:294'; > > Requires knowing how to write an installer test plan, and knowing what on > earth it's using to keep track of substeps. Done on bug 37981. > # $VAR3 = > 'koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-collection.tt:158'; > > Should be easy for someone who uses serials-collection, and thus knows > whether it needs to keep POSTing because sometimes you do stuff to so many > things at once that you exceed Apache's URL maxlength if it's a GET, or not. Done on bug 37982. (In reply to Jonathan Druart from comment #20) > (In reply to Phil Ringnalda from comment #18) > > Seriously, I'm done. Somebody take these. > > > > # $VAR1 = > > 'koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-illrequests.tt:260,293'; > > > > Requires having an ILL backend that does availability and knowing how to get > > to typedisclaimer and how to write a test plan so someone else can. > > Not familiar with this, Pedro maybe? op is added dynamically in the following code: [% FOREACH key IN whole.keys %] [% value = whole.$key %] [% IF key != 'custom_key' && key != 'custom_value' && key != 'csrf_token' %] <input type="hidden" name="[% key | html %]" value="[% value | html %]"> [% END %] [% END %] op is part of the 'whole' sent by availability_template_params in Koha::ILL::Request::Workflow::Availability These hidden form inputs are added dynamically because we want to move all the params from the previous form to the next, the same logic applies for the type disclaimer. Created attachment 171912 [details] [review] Bug 37728: (follow-up) Tell the test that opac-illrequests.tt has an op xt/find-missing-op-in-forms.t wants to see name="op" value="cud-..." in any form with method="post", but opac-illrequests.tt inserts its op input by passing around whole and unpack and repacking whole.keys which includes the op. We just need to tell the test that it really exists. At first, I thought of this approach as a joke, faking out the test, but when I tried to be more responsible and put a whole <input type="hidden" etc. in the comment, I realized that would be more likely to confuse someone who thought that was uncommented and that they were making real changes than it would be to future-proof against changes in the test's approach. Created attachment 171913 [details] [review] Bug 37728: Adjust test to catch more missing 'op' in POSTed forms The test is not catching missing op in forms that are not the first POST form of the template. Found when fixing the test for bug 37309. It was not catching the missing op in the second form. Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org> (In reply to Phil Ringnalda from comment #22) > Created attachment 171912 [details] [review] [review] > Bug 37728: (follow-up) Tell the test that opac-illrequests.tt has an op > > xt/find-missing-op-in-forms.t wants to see name="op" value="cud-..." in any > form with method="post", but opac-illrequests.tt inserts its op input by > passing around whole and unpack and repacking whole.keys which includes > the op. We just need to tell the test that it really exists. > > At first, I thought of this approach as a joke, faking out the test, but > when I tried to be more responsible and put a whole <input type="hidden" etc. > in the comment, I realized that would be more likely to confuse someone > who thought that was uncommented and that they were making real changes than > it would be to future-proof against changes in the test's approach. lol, well, why not! The test will need to stay as silly as it is now :) Created attachment 171916 [details] [review] Bug 37728: (follow-up) Tell the test that opac-illrequests.tt has an op xt/find-missing-op-in-forms.t wants to see name="op" value="cud-..." in any form with method="post", but opac-illrequests.tt inserts its op input by passing around whole and unpack and repacking whole.keys which includes the op. We just need to tell the test that it really exists. At first, I thought of this approach as a joke, faking out the test, but when I tried to be more responsible and put a whole <input type="hidden" etc. in the comment, I realized that would be more likely to confuse someone who thought that was uncommented and that they were making real changes than it would be to future-proof against changes in the test's approach. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 171917 [details] [review] Bug 37728: Adjust test to catch more missing 'op' in POSTed forms The test is not catching missing op in forms that are not the first POST form of the template. Found when fixing the test for bug 37309. It was not catching the missing op in the second form. Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org> I think we can consider this signed off now. Just 3 more QAs, 2 pushes, and 1 security release, and we can land this two-line test change :D Created attachment 171922 [details] [review] Bug 37728: (follow-up) Tell the test that opac-illrequests.tt has an op xt/find-missing-op-in-forms.t wants to see name="op" value="cud-..." in any form with method="post", but opac-illrequests.tt inserts its op input by passing around whole and unpack and repacking whole.keys which includes the op. We just need to tell the test that it really exists. At first, I thought of this approach as a joke, faking out the test, but when I tried to be more responsible and put a whole <input type="hidden" etc. in the comment, I realized that would be more likely to confuse someone who thought that was uncommented and that they were making real changes than it would be to future-proof against changes in the test's approach. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Created attachment 171923 [details] [review] Bug 37728: Adjust test to catch more missing 'op' in POSTed forms The test is not catching missing op in forms that are not the first POST form of the template. Found when fixing the test for bug 37309. It was not catching the missing op in the second form. Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Pushed for 24.11! Well done everyone, thank you! Are we going to try to get this and its dependencies into 24.05, or just call 24.11 enough protection? Looks like that ship has sailed. |