Bug 36190 - op param for stateful requests must start with 'cud-'
Summary: op param for stateful requests must start with 'cud-'
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Assignee: Nick Clemens (kidclamp)
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 34478
Blocks: 36192
  Show dependency treegraph
 
Reported: 2024-02-28 13:05 UTC by Jonathan Druart
Modified: 2024-03-18 09:06 UTC (History)
5 users (show)

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


Attachments
Bug 36190: Adjust the test (1.88 KB, patch)
2024-02-28 13:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36190: To not assume op is invalid if it is a TT variable (904 bytes, patch)
2024-02-28 13:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36190: To not assume op is invalid if it is a TT variable (960 bytes, patch)
2024-03-13 12:40 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36190: (QA follow-up) Tidy (1.30 KB, patch)
2024-03-13 12:40 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36190: Fix problems found by test (19.35 KB, patch)
2024-03-13 12:40 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36190: Adjust the test (1.92 KB, patch)
2024-03-14 23:26 UTC, David Nind
Details | Diff | Splinter Review
Bug 36190: To not assume op is invalid if it is a TT variable (1006 bytes, patch)
2024-03-14 23:26 UTC, David Nind
Details | Diff | Splinter Review
Bug 36190: (QA follow-up) Tidy (1.35 KB, patch)
2024-03-14 23:26 UTC, David Nind
Details | Diff | Splinter Review
Bug 36190: Fix problems found by test (19.37 KB, patch)
2024-03-14 23:27 UTC, David Nind
Details | Diff | Splinter Review
Bug 36190: Adjust the test (2.02 KB, patch)
2024-03-15 07:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36190: To not assume op is invalid if it is a TT variable (1.08 KB, patch)
2024-03-15 07:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36190: (QA follow-up) Tidy (1.44 KB, patch)
2024-03-15 07:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36190: Fix problems found by test (19.46 KB, patch)
2024-03-15 07:55 UTC, Marcel de Rooy
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-02-28 13:05:53 UTC
This is enforced in Koha::Middleware::CSRF but the tests is not catching incorrect occurrences.
Comment 1 Jonathan Druart 2024-02-28 13:06:50 UTC
Created attachment 162530 [details] [review]
Bug 36190: Adjust the test
Comment 2 Jonathan Druart 2024-02-28 13:11:16 UTC
Created attachment 162531 [details] [review]
Bug 36190: To not assume op is invalid if it is a TT variable
Comment 3 Nick Clemens (kidclamp) 2024-03-13 12:40:17 UTC
Created attachment 163110 [details] [review]
Bug 36190: To not assume op is invalid if it is a TT variable

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 4 Nick Clemens (kidclamp) 2024-03-13 12:40:19 UTC
Created attachment 163111 [details] [review]
Bug 36190: (QA follow-up) Tidy
Comment 5 Nick Clemens (kidclamp) 2024-03-13 12:40:21 UTC
Created attachment 163112 [details] [review]
Bug 36190: Fix problems found by test

opac-shelves           - forms were stateless - changed to GET
sco-main               - forms stateless - changed to GET
                         ** untested because sco + auth is broken
smart-rules.tt         - JS form template - added placeholder 'cud-' op
ill-requests           - added cud- tp ops
                         ** tested comments, but not checkout, simple changes
boraccount             - removed repeated op - updated script
patroncards/edit-batch - add placeholder 'cud-' op
patroncards/manage     - add placheholder 'cud-' op
elasticsearch/mappings - separate forms - this could use a style follow-up, but makes more sense separate I think
reports/dictionary      - stateless - changed to GET
guided_reports_start    - stateless - changed to GET
suggestion/suggestion   - add placeholder 'cud-' op
inventory               - filed bug 36305, needs more handling
Comment 6 David Nind 2024-03-13 22:26:25 UTC
I'm getting a sha1/fake ancestor error when trying to apply the patch:

Applying: Bug 36190: To not assume op is invalid if it is a TT variable
error: sha1 information is lacking or useless (xt/find-missing-op-in-forms.t).
error: could not build fake ancestor
Patch failed at 0001 Bug 36190: To not assume op is invalid if it is a TT variable
...

Testing notes - how I planned to test:
1. Run the tests before the patch is applied: prove xt/find-missing-op-in-forms.t
2. Apply the first patch and run the tests: tests should fail
3. Apply the rest of the patches and run the tests: tests should pass!
Comment 7 Nick Clemens (kidclamp) 2024-03-14 10:52:47 UTC
(In reply to David Nind from comment #6)
> I'm getting a sha1/fake ancestor error when trying to apply the patch:

Fixed, I obsoleted a patch without reattaching
> 
> Testing notes - how I planned to test:
> 1. Run the tests before the patch is applied: prove
> xt/find-missing-op-in-forms.t
> 2. Apply the first patch and run the tests: tests should fail
> 3. Apply the rest of the patches and run the tests: tests should pass!

Correct, except the test will have 1 remaining failure, should be handled on bug 36305
Comment 8 David Nind 2024-03-14 23:26:54 UTC
Created attachment 163149 [details] [review]
Bug 36190: Adjust the test

Signed-off-by: David Nind <david@davidnind.com>
Comment 9 David Nind 2024-03-14 23:26:56 UTC
Created attachment 163150 [details] [review]
Bug 36190: To not assume op is invalid if it is a TT variable

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: David Nind <david@davidnind.com>
Comment 10 David Nind 2024-03-14 23:26:59 UTC
Created attachment 163151 [details] [review]
Bug 36190: (QA follow-up) Tidy

Signed-off-by: David Nind <david@davidnind.com>
Comment 11 David Nind 2024-03-14 23:27:02 UTC
Created attachment 163152 [details] [review]
Bug 36190: Fix problems found by test

opac-shelves           - forms were stateless - changed to GET
sco-main               - forms stateless - changed to GET
                         ** untested because sco + auth is broken
smart-rules.tt         - JS form template - added placeholder 'cud-' op
ill-requests           - added cud- tp ops
                         ** tested comments, but not checkout, simple changes
boraccount             - removed repeated op - updated script
patroncards/edit-batch - add placeholder 'cud-' op
patroncards/manage     - add placheholder 'cud-' op
elasticsearch/mappings - separate forms - this could use a style follow-up, but makes more sense separate I think
reports/dictionary      - stateless - changed to GET
guided_reports_start    - stateless - changed to GET
suggestion/suggestion   - add placeholder 'cud-' op
inventory               - filed bug 36305, needs more handling

Signed-off-by: David Nind <david@davidnind.com>
Comment 12 David Nind 2024-03-14 23:35:13 UTC
Thanks Nick!

Testing notes (using KTD):

1. Run the tests before the patch is applied - tests should pass: prove -v xt/find-missing-op-in-forms.t
2. Apply the first three patches (using interactive mode) and run the tests: tests should fail with 12 forms missing the required information
3. Apply the remaining patch and run the tests again: tests still fail,[1] but this time there is only 1 form missing the required information (addressed in bug 36305)

[1] After all patches are applied, only 1 form is missing the required information

prove -v xt/find-missing-op-in-forms.t
xt/find-missing-op-in-forms.t .. 
1..1
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: '1'
#     expected: '0'
# $VAR1 = 'koha-tmpl/intranet-tmpl/prog/en/modules/tools/inventory.tt:71,248';
# Looks like you failed 1 test of 1.
Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/1 subtests 

Test Summary Report
-------------------
xt/find-missing-op-in-forms.t (Wstat: 256 Tests: 1 Failed: 1)
  Failed test:  1
  Non-zero exit status: 1
Files=1, Tests=1,  0 wallclock secs ( 0.01 usr  0.00 sys +  0.15 cusr  0.01 csys =  0.17 CPU)
Result: FAIL
Comment 13 Marcel de Rooy 2024-03-15 07:55:31 UTC
Created attachment 163162 [details] [review]
Bug 36190: Adjust the test

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Marcel de Rooy 2024-03-15 07:55:33 UTC
Created attachment 163163 [details] [review]
Bug 36190: To not assume op is invalid if it is a TT variable

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Marcel de Rooy 2024-03-15 07:55:36 UTC
Created attachment 163164 [details] [review]
Bug 36190: (QA follow-up) Tidy

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 16 Marcel de Rooy 2024-03-15 07:55:38 UTC
Created attachment 163165 [details] [review]
Bug 36190: Fix problems found by test

opac-shelves           - forms were stateless - changed to GET
sco-main               - forms stateless - changed to GET
                         ** untested because sco + auth is broken
smart-rules.tt         - JS form template - added placeholder 'cud-' op
ill-requests           - added cud- tp ops
                         ** tested comments, but not checkout, simple changes
boraccount             - removed repeated op - updated script
patroncards/edit-batch - add placeholder 'cud-' op
patroncards/manage     - add placheholder 'cud-' op
elasticsearch/mappings - separate forms - this could use a style follow-up, but makes more sense separate I think
reports/dictionary      - stateless - changed to GET
guided_reports_start    - stateless - changed to GET
suggestion/suggestion   - add placeholder 'cud-' op
inventory               - filed bug 36305, needs more handling

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 17 Katrin Fischer 2024-03-15 08:40:13 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 18 Fridolin Somers 2024-03-18 09:06:20 UTC
Depends on Bug 34478 not in 23.11.x