Summary: | Fix form that looks like it would POST without an op in reserve/request.tt | ||
---|---|---|---|
Product: | Koha | Reporter: | Phil Ringnalda <phil> |
Component: | Circulation | Assignee: | Phil Ringnalda <phil> |
Status: | RESOLVED FIXED | QA Contact: | Jonathan Druart <jonathan.druart> |
Severity: | normal | ||
Priority: | P3 | CC: | dcook, gmcharlt, jonathan.druart, kyle.m.hall, lucas, martin.renvoize, oleonard, pedro.amorim |
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: | 37817, 37728 | ||
Attachments: |
Bug 37783: Fix form that looks like it would POST without an op in reserve/request.tt
Bug 37783: Fix form that looks like it would POST without an op in reserve/request.tt |
Description
Phil Ringnalda
2024-08-30 01:12:14 UTC
Created attachment 170895 [details] [review] Bug 37783: Fix form that looks like it would POST without an op in reserve/request.tt 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 reserve/request.tt the modal for cancelling a hold looks like it is a form that will do a POST without an op input, but in fact it requires JavaScript to work at all, and with JavaScript it clears out the div where it stashes inputs and then inserts one with the op cud-cancel. To persuade the test at xt/find-missing-op-in-forms.t that there is an op, and to let a casual skimmer of the code see what that op will be, without actually changing the behavior in any way, we can just stick the op in the div which the JS will .empty() out before sticking the same thing back in. Test plan: 1. Search for any record with an item, click Place hold, place two holds 2. In the row for the second hold, click the trash can icon to delete 3. Nothing changed from normallly cancelling a hold, did it? It shouldn't have. Sponsored-by: Chetco Community Public Library This is not great and I was going to sign it off as it could be a quick solution for now. However it's buggy (and is ofc before your patch). If you select 2 holds then click on the cancel link (for the single line then), the modal is going to delete the 2 holds ("This action will cancel 2 hold(s)"). I let you decide if you want to fix this along with this patch. IMO if we fix the bug we will certainly Outch, actually it displays "2 holds will be cancelled", but only one is cancelled. Also if you click "select all", then uncheck and click the cancel (single), the modal shows "This action will cancel 0 hold(s)". The way we deal with this message is totally broken. Created attachment 170961 [details] [review] Bug 37783: Fix form that looks like it would POST without an op in reserve/request.tt 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 reserve/request.tt the modal for cancelling a hold looks like it is a form that will do a POST without an op input, but in fact it requires JavaScript to work at all, and with JavaScript it clears out the div where it stashes inputs and then inserts one with the op cud-cancel. To persuade the test at xt/find-missing-op-in-forms.t that there is an op, and to let a casual skimmer of the code see what that op will be, without actually changing the behavior in any way, we can just stick the op in the div which the JS will .empty() out before sticking the same thing back in. Test plan: 1. Search for any record with an item, click Place hold, place two holds 2. In the row for the second hold, click the trash can icon to delete 3. Nothing changed from normallly cancelling a hold, did it? It shouldn't have. Sponsored-by: Chetco Community Public Library Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Trivial, skipping QA. Pushed for 24.11! Well done everyone, thank you! Merge conflicts in 24.05.x, please rebase if needed. This causes the "Cancel selected" button to not work. To reproduce: 1) Place a hold on a record 2) On the "existing holds" table, check a hold. 3) Click 'Cancel selected". Nothing happens. Ever. I believe this is because the payload now contains: ... op: cud-cancel biblionumber: 76 op: cud-cancel_bulk ... Revert this patch. Repeat the test plan. The selected hold is canceled as expected. |