| Summary: | Fix form that POSTs without an op in itemtype administration | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Phil Ringnalda <phil> |
| Component: | System Administration | Assignee: | Phil Ringnalda <phil> |
| Status: | RESOLVED FIXED | QA Contact: | Jonathan Druart <jonathan.druart> |
| Severity: | normal | ||
| Priority: | P3 | CC: | gmcharlt, jonathan.druart, lucas, martin.renvoize, oleonard |
| Version: | Main | ||
| Hardware: | All | ||
| OS: | All | ||
| Change sponsored?: | --- | Patch complexity: | --- |
| Documentation contact: | Documentation submission: | ||
| Text to go in the release notes: | Version(s) released in: |
24.11.00
|
|
| Circulation function: | |||
| Bug Depends on: | 36192 | ||
| Bug Blocks: | 37728 | ||
| Attachments: |
Bug 37768: Fix form that POSTs without an op in itemtype administration
Bug 37768: Fix form that POSTs without an op in itemtype administration |
||
|
Description
Phil Ringnalda
2024-08-29 03:54:26 UTC
Created attachment 170892 [details] [review] Bug 37768: Fix form that POSTs without an op in itemtype administration 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. For itemtype administration, that's the "No, do not delete" cancel button when you decide not to delete an itemtype, which doesn't need to POST anything since it's just taking you back to the list of itemtypes. The only visible change from switching to a GET is that the URL ends with a "?" from a GET with no params, but someone can fix that by choosing one of our various link-as-a-cancel-button styles and switching it to a link in a bug that doesn't block an RM_priority bug. Test plan: 1. You aren't going to see a visible difference, so start with the patch applied 2. Administration - Item types 3. You need an itemtype that isn't in use to be able to delete it - ktd provides you with an unused Computer Files type, so click the Delete button for that row 4. In the "Are you sure..." page, click No, do not delete 5. Verify that you are back at the list of itemtypes, with only the "?" at the end of the URL to tell you that you did a GET rather than a POST Sponsored-by: Chetco Community Public Library Owen is this the expected UI? Don't we want to display a link instead? Do we want to display a link, like we sometimes do, or a link pretending to be a button, like we sometime do, or a button being a button, like we sometimes do? Most of these bugs blocking bug 37728 have at least one form that doesn't need to be a form, if you want to delay landing that long enough to change them. Oh, and most of them are loading a new page to ask for confirmation even when they don't need to look up whether they are going to disallow deleting something because it's in use, a thing which we generally ask with just a confirm popup now, do we want to switch to doing that instead? And even for things that we won't let you delete because they are in use, we generally don't load a new page to look that up, we just don't let you delete them, like authority terms linked to bib records where there just isn't a delete button if you can't delete, should we switch that as well? Created attachment 170958 [details] [review] Bug 37768: Fix form that POSTs without an op in itemtype administration 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. For itemtype administration, that's the "No, do not delete" cancel button when you decide not to delete an itemtype, which doesn't need to POST anything since it's just taking you back to the list of itemtypes. The only visible change from switching to a GET is that the URL ends with a "?" from a GET with no params, but someone can fix that by choosing one of our various link-as-a-cancel-button styles and switching it to a link in a bug that doesn't block an RM_priority bug. Test plan: 1. You aren't going to see a visible difference, so start with the patch applied 2. Administration - Item types 3. You need an itemtype that isn't in use to be able to delete it - ktd provides you with an unused Computer Files type, so click the Delete button for that row 4. In the "Are you sure..." page, click No, do not delete 5. Verify that you are back at the list of itemtypes, with only the "?" at the end of the URL to tell you that you did a GET rather than a POST Sponsored-by: Chetco Community Public Library Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Definitely not the place to fix the UI discrepancies, but would be nice to see more consistencies. I have no idea what we want however :) Trivial, skipping QA. I haven't come up with a way to find all instances of this pattern, but I have 32 tabs open to the ones that were easy to find, if anyone wants to choose our One True Way to do a cancel buttonlink. (In reply to Phil Ringnalda from comment #8) > I haven't come up with a way to find all instances of this pattern, but I > have 32 tabs open to the ones that were easy to find, if anyone wants to > choose our One True Way to do a cancel buttonlink. I discussed this with Owen (pm) and the conclusion is that "No, do not delete" is in an alert box and the use of a button is the current pattern in this case. It seems that we are consistent in Koha (always using a "yes" button and a "no" button). Then... to go further, it's a rabbit hole. You can start there: https://ux.stackexchange.com/questions/82492/yes-no-button-placement-on-confirmation-dialog-boxes We could discuss it more on a separate bug report, but we will need time and energy to "fix" all of the occurrences. I think working towards more consistency is a good goal. Could we get a bug report about the findings here so that we don't lose sight of this? Pushed for 24.11! Well done everyone, thank you! Merge conflicts in 24.05.x, no backport. |