Bug 38378 - Serial frequency deletion is broken
Summary: Serial frequency deletion is broken
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Serials (show other bugs)
Version: Main
Hardware: All All
: P3 major
Assignee: Phil Ringnalda
QA Contact: David Cook
URL:
Keywords:
Depends on: 36192
Blocks:
  Show dependency treegraph
 
Reported: 2024-11-06 13:23 UTC by Lari Strand
Modified: 2024-11-18 16:08 UTC (History)
2 users (show)

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


Attachments
Bug 38378: Can't delete frequencies due to stray cud- in del op (844 bytes, patch)
2024-11-06 13:32 UTC, Lari Strand
Details | Diff | Splinter Review
Bug 38378: Can't delete frequencies due to stray cud- in del op (844 bytes, patch)
2024-11-06 13:39 UTC, Lari Strand
Details | Diff | Splinter Review
Bug 38378: Serial frequency deletion needs to be converted to POST from GET (2.00 KB, patch)
2024-11-07 13:32 UTC, Lari Strand
Details | Diff | Splinter Review
Bug 38378: Serial frequency deletion needs to be converted to POST from GET (2.00 KB, patch)
2024-11-07 13:49 UTC, Lari Strand
Details | Diff | Splinter Review
Bug 38378: Serial frequency deletion needs to be converted to POST from GET (2.93 KB, patch)
2024-11-08 22:16 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 38378: Serial frequency deletion needs to be converted to POST from GET (2.99 KB, patch)
2024-11-12 17:52 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 38378: Serial frequency deletion needs to be converted to POST from GET (3.04 KB, patch)
2024-11-13 03:33 UTC, David Cook
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lari Strand 2024-11-06 13:23:06 UTC
serials/subscription-frequencies.pl has a delete op that was changed to cud-del when it shouldn't have been. This breaks the delete button's function. Existing subscription frequencies cannot be deleted and the button does nothing.
Comment 1 Lari Strand 2024-11-06 13:32:07 UTC
Created attachment 174076 [details] [review]
Bug 38378: Can't delete frequencies due to stray cud- in del op
Comment 2 Lari Strand 2024-11-06 13:39:02 UTC
Created attachment 174077 [details] [review]
Bug 38378: Can't delete frequencies due to stray cud- in del op

serials/subscription-frequencies.pl has a delete op that was changed to cud-del when it shouldn't have been. This breaks the delete button's function. Existing subscription frequencies cannot be deleted and the button does nothing. This patch fixess the issue.

Test plan:

1. Serials - Manage frequencies - for any frequency click Delete, create one if needed
2. Note that the frequency can't be deleted
3. Apply patch, restart_all
4. Repeat step 1, note that now the deletion works
Comment 3 Phil Ringnalda 2024-11-06 16:06:36 UTC
That would then break deleting a frequency that is in use, which returns a page saying it is in use and asks for a confirmation that you want to delete it, and if you do, POSTs with op=cud-del.

It is a cud- op, the d is for delete, and it should require a POST.

The problem is that the link for the first attempt at deleting is doing a GET with just op=del. Two possible solutions for that: either the link needs to use submit-form-link to POST (probably with a data-confirmation-msg, it's a bit sketchy that you can delete an unused frequency with no confirmation at all), or the ops need to be reworked into a GET of delete-confirm which returns a page that says whether or not the frequency is in use and asks for confirmation either way and POSTS to cud-delete.
Comment 4 David Cook 2024-11-06 22:55:25 UTC
+1 to what Phil said.
Comment 5 David Cook 2024-11-06 23:05:59 UTC
Going to change the title of this one
Comment 6 Lari Strand 2024-11-07 07:18:13 UTC
Ok I get it. I don't think I have what it takes to fix this one as Phil suggested :D. Please someone take over.
Comment 7 Lari Strand 2024-11-07 13:32:17 UTC
Created attachment 174195 [details] [review]
Bug 38378: Serial frequency deletion needs to be converted to POST from GET

Deleting frequencies and checking if they are used by a subscription is broken. This patch fixes the issue.
Comment 8 Lari Strand 2024-11-07 13:49:46 UTC
Created attachment 174231 [details] [review]
Bug 38378: Serial frequency deletion needs to be converted to POST from GET

Deleting frequencies and checking if they are used by a subscription is broken. This patch fixes the issue.

Test plan:

1. Serials - Manage frequencies - for any frequency click Delete, create one if needed
2. Note that the frequency can't be deleted
3. Apply patch, restart_all
4. Repeat step 1, note that now the deletion works

5. Serials - Manage frequencies - create one
6. Create a subscription in serials that uses the created frequency
7. Try to delete the created frequency
8. Note that nothing happens
9. Apply patch, restart all 
10. Repeat step 7, note that now Koha tells you if the frequency is in use by a subscription/subscriptions and when you confirm deletion the frequency gets removed
Comment 9 Lari Strand 2024-11-07 13:54:58 UTC
Somehow the older patches did not get obsoleted by git bz and I can't figure out why. Sorry for that.
Comment 10 David Cook 2024-11-07 22:32:57 UTC
Take a look at some other examples and you'll get the idea.

Here's some I did:
commit 5d3b7cf08949d5776b732f24cefcbe377260c281
commit 47d6725a722728c5d0ae38953a0d8c74d99d3d9e

However, Owen does it much better on the "Manage numbering patterns" page:
commit 7dc1b9e95436452d2008bce17aa88df6496064a6
Comment 11 David Cook 2024-11-07 22:34:51 UTC
It looks like "Manage subscription fields" was just fixed by bug 38309.

You can do it!
Comment 12 David Cook 2024-11-07 22:35:22 UTC
(In reply to David Cook from comment #11)
> You can do it!

That's me trying to sound encouraging and not demanding ;). You've got this.
Comment 13 Phil Ringnalda 2024-11-07 22:55:17 UTC
Yeah, avoid those more complicated early-on examples of jamming a form into the space where a link was, submit-form-link is The Way :)

Once you have one to copy-paste from, it really doesn't take much:

- If the template doesn't already have one, add [% USE Asset %] up top
- If the template doesn't already have one, down in the [% MACRO jsinclude BLOCK %] at the bottom add [% Asset.js("js/form-submit.js") | $raw %] which is what does all the hard work for you
- Add the class submit-form-link to the link
- Replace the href attribute with "#"
- Put the path and filename from the href in a data-action attribute
- Add a data-method="post" attribute
- For all the paramters after the ? in the original href, add an attribute where the name is 'data-' plus the key, and the value is the value, so href="?goats=fun" becomes data-goats="fun". And that's where you need to change the ?op=del to data-op=cud-del, because there shouldn't be an op named del which does deletion
- If you need a confirmation alert (and you do, for delete), add data-confirmation-msg="[% t('Really?!?') | html %] only with better text than Really?!?, typically 'Are you sure you want to delete this frequency?'
Comment 14 Phil Ringnalda 2024-11-07 23:09:15 UTC
The always-unobvious steps to obsolete a patch manually in Bugzilla are to click the Details link to the right of the patch, then at the end of the patch summary line where it's easy to miss is a (edit details) link that gives you a checkbox to obsolete.
Comment 15 David Cook 2024-11-08 00:08:15 UTC
Yes! Thank you, Phil! Legend! 

I was searching for bug 36246 (using keywords) and I just couldn't find it. 

I think I like Owen's from commit 7dc1b9e95436452d2008bce17aa88df6496064a6 best but bug 36246 is what I was trying to find as an example of what folk have been trying to do.
Comment 16 Phil Ringnalda 2024-11-08 21:51:19 UTC
I don't think we were sufficiently persuasive about how easy it would be, taking.

Now that I actually looked at the page, where these are already links pretending to be buttons, just wrap it in a form and turn it into a real button is the right solution here.
Comment 17 Phil Ringnalda 2024-11-08 22:16:37 UTC
Created attachment 174309 [details] [review]
Bug 38378: Serial frequency deletion needs to be converted to POST from GET

Deleting a serial frequency is (or can be) a two step process. If you are
deleting a frequency which is in use, you get back a page warning you that it
is in use, which the CSRF changes converted to a form POSTing with a CSRF
token and the new op cud-del. However, to get there you have to go through
the step that's the only step if the frequency isn't in use, clicking a link
that still thinks the op is named del rather than cud-del. That link needs to
instead be a form with a CSRF token and a POST of cud-del.

Test plan:
 1. Without the patch, Serials - Manage frequencies
 2. For any frequency, click Delete, click OK in the confirmation popup
 3. Nothing happened except your URL changing, the frequency is still there
 4. Apply patch, reload Manage frequencies
 5. For any frequency, click Delete, click OK in the confirmation popup
 6. This time, your frequency was deleted

Sponsored-by: Chetco Community Public Library
Comment 18 Brendan Lawlor 2024-11-12 17:52:57 UTC
Created attachment 174434 [details] [review]
Bug 38378: Serial frequency deletion needs to be converted to POST from GET

Deleting a serial frequency is (or can be) a two step process. If you are
deleting a frequency which is in use, you get back a page warning you that it
is in use, which the CSRF changes converted to a form POSTing with a CSRF
token and the new op cud-del. However, to get there you have to go through
the step that's the only step if the frequency isn't in use, clicking a link
that still thinks the op is named del rather than cud-del. That link needs to
instead be a form with a CSRF token and a POST of cud-del.

Test plan:
 1. Without the patch, Serials - Manage frequencies
 2. For any frequency, click Delete, click OK in the confirmation popup
 3. Nothing happened except your URL changing, the frequency is still there
 4. Apply patch, reload Manage frequencies
 5. For any frequency, click Delete, click OK in the confirmation popup
 6. This time, your frequency was deleted

Sponsored-by: Chetco Community Public Library
Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Comment 19 David Cook 2024-11-13 03:33:55 UTC
Created attachment 174444 [details] [review]
Bug 38378: Serial frequency deletion needs to be converted to POST from GET

Deleting a serial frequency is (or can be) a two step process. If you are
deleting a frequency which is in use, you get back a page warning you that it
is in use, which the CSRF changes converted to a form POSTing with a CSRF
token and the new op cud-del. However, to get there you have to go through
the step that's the only step if the frequency isn't in use, clicking a link
that still thinks the op is named del rather than cud-del. That link needs to
instead be a form with a CSRF token and a POST of cud-del.

Test plan:
 1. Without the patch, Serials - Manage frequencies
 2. For any frequency, click Delete, click OK in the confirmation popup
 3. Nothing happened except your URL changing, the frequency is still there
 4. Apply patch, reload Manage frequencies
 5. For any frequency, click Delete, click OK in the confirmation popup
 6. This time, your frequency was deleted

Sponsored-by: Chetco Community Public Library
Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 20 Katrin Fischer 2024-11-18 16:08:41 UTC
Pushed for 24.11!

Well done everyone, thank you!