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.
Created attachment 174076 [details] [review] Bug 38378: Can't delete frequencies due to stray cud- in del op
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
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.
+1 to what Phil said.
Going to change the title of this one
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.
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.
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
Somehow the older patches did not get obsoleted by git bz and I can't figure out why. Sorry for that.
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
It looks like "Manage subscription fields" was just fixed by bug 38309. You can do it!
(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.
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?'
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.
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.
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.
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
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>
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>
Pushed for 24.11! Well done everyone, thank you!