Bug 37728 - More "op" are missing in POSTed forms
Summary: More "op" are missing in POSTed forms
Status: NEW
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords: RM_priority
Depends on: 36192 37765 37766 37767 37768 37769 37779 37783 37785 37786 37794 37797 37823 37827 37853 37887 37913 37914
Blocks:
  Show dependency treegraph
 
Reported: 2024-08-26 09:02 UTC by Jonathan Druart
Modified: 2024-09-13 07:07 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 37728: Adjust test to catch more missing 'op' in POSTed forms (1.55 KB, patch)
2024-08-26 09:05 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2024-08-26 09:02:06 UTC
xt/find-missing-op-in-forms.t is missing some occurrences.
Comment 1 Jonathan Druart 2024-08-26 09:05:56 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.
Comment 2 Jonathan Druart 2024-08-26 09:06:55 UTC
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';
Comment 3 Katrin Fischer 2024-08-26 09:47:27 UTC
Not good, but good that we caught them now. 
Who can work on resolving these?
Comment 4 Jonathan Druart 2024-08-28 13:30:28 UTC
I can help, but not let alone.
Comment 5 Phil Ringnalda 2024-08-29 02:56:08 UTC
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.
Comment 6 Phil Ringnalda 2024-08-29 04:03:35 UTC
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.
Comment 7 David Cook 2024-08-29 04:20:29 UTC
Thanks, Phil. Much appreciated! You've been killing it lately :D.
Comment 8 David Cook 2024-08-29 04:21:43 UTC
(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.
Comment 9 Phil Ringnalda 2024-08-29 17:32:17 UTC
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?
Comment 10 Phil Ringnalda 2024-08-29 23:35:43 UTC
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.
Comment 11 Phil Ringnalda 2024-08-30 01:25:53 UTC
Asked that question in the form of a Needs Signoff in bug 37783.
Comment 12 Phil Ringnalda 2024-08-30 17:14:29 UTC
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.
Comment 13 Katrin Fischer 2024-08-30 17:21:07 UTC
Thanks Phil, awesome job!
Comment 14 Emily Lamancusa 2024-08-30 20:26:01 UTC
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!
Comment 15 Phil Ringnalda 2024-09-06 23:06:30 UTC
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.
Comment 16 Katrin Fischer 2024-09-09 14:30:41 UTC
Should this be NSO?
Comment 17 Phil Ringnalda 2024-09-09 14:58:50 UTC
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';
Comment 18 Phil Ringnalda 2024-09-12 23:51:13 UTC
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.
Comment 19 Katrin Fischer 2024-09-13 07:07:13 UTC
Thanks a lot for your work, Phil!