Bug 37766 - Fix forms that POST without an op in MARC bibliographic frameworks
Summary: Fix forms that POST without an op in MARC bibliographic frameworks
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: Main
Hardware: All All
: P3 normal
Assignee: Phil Ringnalda
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on: 36192
Blocks: 37728
  Show dependency treegraph
 
Reported: 2024-08-29 03:37 UTC by Phil Ringnalda
Modified: 2024-09-13 10:07 UTC (History)
2 users (show)

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


Attachments
Bug 37766: Fix forms that POST without an op in MARC bibliographic frameworks (4.11 KB, patch)
2024-08-30 00:06 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 37766: Fix forms that POST without an op in MARC bibliographic frameworks (4.16 KB, patch)
2024-08-30 15:02 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 37766: Fix forms that POST without an op in MARC bibliographic frameworks (4.24 KB, patch)
2024-09-02 13:33 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 Phil Ringnalda 2024-08-29 03:37:51 UTC
We intend not to have forms with method="post" without an op variable (so we
can check that the op starts with "cud-" as part of the CSRF protection), but
because of bug 37728 some were missed.

In MARC bibliographic frameworks, that's the tag search form, which is very much not something that should or needs to POST, and doing so prevents bookmarking or linking to a search for a particular tag, and the cancel "No, do not delete" button in the confirmation page while deleting a subfield, which doesn't need to POST because it doesn't need to send anything other than what's needed to go back to where you were.
Comment 1 Phil Ringnalda 2024-08-30 00:06:16 UTC
Created attachment 170893 [details] [review]
Bug 37766: Fix forms that POST without an op in MARC bibliographic frameworks

We intend not to have forms with method="post" without an op variable (so we
can check that the op starts with "cud-" as part of the CSRF protection), but
because of bug 37728 some were missed.

In MARC bibliographic frameworks, that's the tag search form, which should be
a GET so the URL includes what you searched for and you can bookmark it or
link to the search, and the cancel "No, do not delete" button in the page
to confirm deleting a subfield, which should also be a GET to take you back
to the page where you were, which was ?tagfield=903&frameworkcode=VR when
you clicked Delete.

Test plan:
 1. No visible change in behavior (only the URL), so start with the patch
    applied
 2. Administration - MARC bibliographic framework - choose one other than
    Default, since the "&framework=" of Default could be confused with a
    failure to get the code in there - Actions - MARC structure
 3. Type any three digit number higher than 009 (you want something with
    subfields) in the Search for tag input and hit Enter
 4. Verify that your URL has the searchfield and frameworkcode correct
    and that number or next highest number tag is displayed first
 5. Change the In framework select menu to another non-Default framework
    and click search, and verify that the URL change to that frameworkcode,
    and that framework is displayed
 6. Toggle the Display only used tags/subfields checkbox, search for a
    different tag, and verify that the state of the checkbox persists as
    you do more searches
 7. On any other listed tag - Actions - View subfields
 8. For any displayed subfield click Delete
 9. In the confirmation page click No, do not delete
10. Verify that the page you return to has the correct tagfield and
    frameworkcode for the tag you chose

Sponsored-by: Chetco Community Public Library
Comment 2 Owen Leonard 2024-08-30 15:02:34 UTC
Created attachment 170914 [details] [review]
Bug 37766: Fix forms that POST without an op in MARC bibliographic frameworks

We intend not to have forms with method="post" without an op variable
(so we can check that the op starts with "cud-" as part of the CSRF
protection), but because of bug 37728 some were missed.

In MARC bibliographic frameworks, that's the tag search form, which
should be a GET so the URL includes what you searched for and you can
bookmark it or link to the search, and the cancel "No, do not delete"
button in the page to confirm deleting a subfield, which should also be
a GET to take you back to the page where you were, which was
?tagfield=903&frameworkcode=VR when you clicked Delete.

Test plan:
 1. No visible change in behavior (only the URL), so start with the
    patch applied
 2. Administration - MARC bibliographic framework - choose one other
    than Default, since the "&framework=" of Default could be confused
    with a failure to get the code in there - Actions - MARC structure
 3. Type any three digit number higher than 009 (you want something with
    subfields) in the Search for tag input and hit Enter
 4. Verify that your URL has the searchfield and frameworkcode correct
    and that number or next highest number tag is displayed first
 5. Change the In framework select menu to another non-Default framework
    and click search, and verify that the URL change to that
    frameworkcode, and that framework is displayed
 6. Toggle the Display only used tags/subfields checkbox, search for a
    different tag, and verify that the state of the checkbox persists as
    you do more searches
 7. On any other listed tag - Actions - View subfields
 8. For any displayed subfield click Delete
 9. In the confirmation page click No, do not delete
10. Verify that the page you return to has the correct tagfield and
    frameworkcode for the tag you chose

Sponsored-by: Chetco Community Public Library
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 3 Jonathan Druart 2024-09-02 13:33:53 UTC
Created attachment 170942 [details] [review]
Bug 37766: Fix forms that POST without an op in MARC bibliographic frameworks

We intend not to have forms with method="post" without an op variable
(so we can check that the op starts with "cud-" as part of the CSRF
protection), but because of bug 37728 some were missed.

In MARC bibliographic frameworks, that's the tag search form, which
should be a GET so the URL includes what you searched for and you can
bookmark it or link to the search, and the cancel "No, do not delete"
button in the page to confirm deleting a subfield, which should also be
a GET to take you back to the page where you were, which was
?tagfield=903&frameworkcode=VR when you clicked Delete.

Test plan:
 1. No visible change in behavior (only the URL), so start with the
    patch applied
 2. Administration - MARC bibliographic framework - choose one other
    than Default, since the "&framework=" of Default could be confused
    with a failure to get the code in there - Actions - MARC structure
 3. Type any three digit number higher than 009 (you want something with
    subfields) in the Search for tag input and hit Enter
 4. Verify that your URL has the searchfield and frameworkcode correct
    and that number or next highest number tag is displayed first
 5. Change the In framework select menu to another non-Default framework
    and click search, and verify that the URL change to that
    frameworkcode, and that framework is displayed
 6. Toggle the Display only used tags/subfields checkbox, search for a
    different tag, and verify that the state of the checkbox persists as
    you do more searches
 7. On any other listed tag - Actions - View subfields
 8. For any displayed subfield click Delete
 9. In the confirmation page click No, do not delete
10. Verify that the page you return to has the correct tagfield and
    frameworkcode for the tag you chose

Sponsored-by: Chetco Community Public Library
Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 4 Katrin Fischer 2024-09-02 14:28:59 UTC
-                                <input type="hidden" name="tagsubfield" value="[% mss.tagsubfield | html %]" />

Is this removal intended here?
Comment 5 Phil Ringnalda 2024-09-03 00:40:51 UTC
Very much intentional: the form above, that does the "Yes, delete" action, needs to tell the script the tag, the subfield, and the framework(In reply to Katrin Fischer from comment #4)
> -                                <input type="hidden" name="tagsubfield"
> value="[% mss.tagsubfield | html %]" />
> 
> Is this removal intended here?

Very much intentional: the form above, that does the "Yes, delete" action, needs to tell the script the tag, the subfield, and the framework, but the cancel one only needs to tell it the tag and framework to go back to showing the subfields for that tag.
Comment 6 Katrin Fischer 2024-09-03 07:14:19 UTC
If we are deleting subfields, could this be a remant with the idea to lead you back to the correct open tab? Tabs have been reworked a couple times, so not sure if that would still work. Will have another look here later, fighting with Jenkins a bit.
Comment 7 Phil Ringnalda 2024-09-03 14:19:47 UTC
Only if at some point we either used tabs is View subfields, or had UI to delete subfields in Edit subfields. Currently, tabs are Edit-only, and Delete is View-only.
Comment 8 Jonathan Druart 2024-09-03 14:44:06 UTC
I had a special look at this removal when QAing and it is safe.
Comment 9 Katrin Fischer 2024-09-13 09:25:13 UTC
(In reply to Jonathan Druart from comment #8)
> I had a special look at this removal when QAing and it is safe.

Thanks all!
Comment 10 Katrin Fischer 2024-09-13 10:07:45 UTC
Pushed for 24.11!

Well done everyone, thank you!