Bug 37728 - More "op" are missing in POSTed forms
Summary: More "op" are missing in POSTed forms
Status: Passed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement
Assignee: Jonathan Druart
QA Contact: Pedro Amorim
URL:
Keywords: RM_priority
Depends on: 36192 37765 37766 37767 37768 37769 37779 37783 37785 37786 37794 37797 37823 37827 37853 37887 37913 37914 37981 37982
Blocks:
  Show dependency treegraph
 
Reported: 2024-08-26 09:02 UTC by Jonathan Druart
Modified: 2024-09-30 14:39 UTC (History)
9 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
Bug 37728: (follow-up) Tell the test that opac-illrequests.tt has an op (2.40 KB, patch)
2024-09-24 02:47 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 37728: Adjust test to catch more missing 'op' in POSTed forms (1.61 KB, patch)
2024-09-24 02:49 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 37728: (follow-up) Tell the test that opac-illrequests.tt has an op (2.47 KB, patch)
2024-09-24 04:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 37728: Adjust test to catch more missing 'op' in POSTed forms (1.61 KB, patch)
2024-09-24 04:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 37728: (follow-up) Tell the test that opac-illrequests.tt has an op (2.53 KB, patch)
2024-09-24 09:52 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37728: Adjust test to catch more missing 'op' in POSTed forms (1.67 KB, patch)
2024-09-24 09:52 UTC, Pedro Amorim
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 (emlam) 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!
Comment 20 Jonathan Druart 2024-09-23 09:22:12 UTC
(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.
Comment 21 Pedro Amorim 2024-09-23 15:04:22 UTC
(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.
Comment 22 Phil Ringnalda 2024-09-24 02:47:07 UTC
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.
Comment 23 Phil Ringnalda 2024-09-24 02:49:29 UTC
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>
Comment 24 Jonathan Druart 2024-09-24 04:04:22 UTC
(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 :)
Comment 25 Jonathan Druart 2024-09-24 04:07:13 UTC
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>
Comment 26 Jonathan Druart 2024-09-24 04:07:16 UTC
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>
Comment 27 Jonathan Druart 2024-09-24 04:07:34 UTC
I think we can consider this signed off now.
Comment 28 Phil Ringnalda 2024-09-24 04:14:16 UTC
Just 3 more QAs, 2 pushes, and 1 security release, and we can land this two-line test change :D
Comment 29 Pedro Amorim 2024-09-24 09:52:33 UTC
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>
Comment 30 Pedro Amorim 2024-09-24 09:52:38 UTC
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>