Bug 37089 - Cannot delete a fund or a currency
Summary: Cannot delete a fund or a currency
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: Main
Hardware: All All
: P5 - low major
Assignee: Emily Lamancusa (emlam)
QA Contact: Lucas Gass (lukeg)
URL:
Keywords:
Depends on:
Blocks: 36192
  Show dependency tree / graph
 
Reported: 2024-06-14 14:02 UTC by Emily Lamancusa (emlam)
Modified: 2024-07-26 20:42 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This fixes the acquisitions funds and currencies pages so that they can be deleted. After the CSRF changes in 24.05 to improve form security, the 'Delete' actions no longer worked. The deletion confirmation message was displayed with '' for the name and no values for the fund or currency, then didn't delete them.
Version(s) released in:
24.11.00,24.05.02
Circulation function:

Attachments
Bug 37089: cud-delete_confirm should not be cud (1.95 KB, patch)
2024-06-14 14:08 UTC, Emily Lamancusa (emlam) 		Details | Diff | Splinter Review
Bug 37089: cud-delete_confirm should not be cud (2.02 KB, patch)
2024-06-14 14:27 UTC, Owen Leonard 		Details | Diff | Splinter Review
Bug 37089: cud-delete_confirm should not be cud (2.07 KB, patch)
2024-06-14 17:07 UTC, Lucas Gass (lukeg) 		Details | Diff | Splinter Review
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Emily Lamancusa (emlam) 2024-06-14 14:02:01 UTC 
aqbudgets.pl and currency.pl are looking for op=cud-delete_confirm, but delete_confirm isn't CUD and the page is just a GET displaying the information.

To reproduce:
Part 1 - Delete a fund
1. Go to Acquisitions > Funds
2. Try to delete a fund
--> Confirmation page shows fund name as '' and fund amount as 0.00
3. Click "Yes, delete this fund"
--> The fund is still there

Part 2 - Delete a currency
1. Go to Acquisitions > Currencies
2. Try to delete a currency
--> Confirmation shows empty values for currency and rate
3. Click, "Yes, delete this currency"
--> An error message appears and the currency is not deleted
Comment 1 Emily Lamancusa (emlam) 2024-06-14 14:08:13 UTC 
Created attachment 167715 [details] [review]
Bug 37089: cud-delete_confirm should not be cud

Adjust aqbudgets.pl and currency.pl to check for the correct op value

To test:
Part 1 - Delete a fund
1. Go to Acquisitions > Funds
2. Try to delete a fund
--> Confirmation page shows fund name as '' and fund amount as 0.00
3. Click "Yes, delete this fund"
--> The fund is still there

Part 2 - Delete a currency
3. Go to Acquisitions > Currencies
4. Try to delete a currency
--> Confirmation shows empty values for currency and rate
5. Click, "Yes, delete this currency"
--> An error message appears and the currency is not deleted

6. Apply patch and restart_all
7. Repeat steps 1-5
--> Confirmation pages display correct information, and deletions
    occur successfully
Comment 2 Owen Leonard 2024-06-14 14:27:50 UTC 
Created attachment 167717 [details] [review]
Bug 37089: cud-delete_confirm should not be cud

Adjust aqbudgets.pl and currency.pl to check for the correct op value

To test:
Part 1 - Delete a fund
1. Go to Acquisitions > Funds
2. Try to delete a fund
   --> Confirmation page shows fund name as '' and fund amount as 0.00
3. Click "Yes, delete this fund"
   --> The fund is still there

Part 2 - Delete a currency
3. Go to Acquisitions > Currencies
4. Try to delete a currency
   --> Confirmation shows empty values for currency and rate
5. Click, "Yes, delete this currency"
   --> An error message appears and the currency is not deleted

6. Apply patch and restart_all
7. Repeat steps 1-5
   --> Confirmation pages display correct information, and deletions
       occur successfully

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 3 Lucas Gass (lukeg) 2024-06-14 17:07:53 UTC 
Created attachment 167751 [details] [review]
Bug 37089: cud-delete_confirm should not be cud

Adjust aqbudgets.pl and currency.pl to check for the correct op value

To test:
Part 1 - Delete a fund
1. Go to Acquisitions > Funds
2. Try to delete a fund
   --> Confirmation page shows fund name as '' and fund amount as 0.00
3. Click "Yes, delete this fund"
   --> The fund is still there

Part 2 - Delete a currency
3. Go to Acquisitions > Currencies
4. Try to delete a currency
   --> Confirmation shows empty values for currency and rate
5. Click, "Yes, delete this currency"
   --> An error message appears and the currency is not deleted

6. Apply patch and restart_all
7. Repeat steps 1-5
   --> Confirmation pages display correct information, and deletions
       occur successfully

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 4 Katrin Fischer 2024-06-21 12:04:16 UTC 
It looks like we still got quite a few of "cud-delete-confirm" - should we check those systematically?
Comment 5 Katrin Fischer 2024-06-21 13:04:41 UTC 
Pushed for 24.11!

Well done everyone, thank you!
Comment 6 Emily Lamancusa (emlam) 2024-06-21 13:57:26 UTC 
(In reply to Katrin Fischer from comment #4)
> It looks like we still got quite a few of "cud-delete-confirm" - should we
> check those systematically?

I agree! That was my goal here as well. :) I actually do have a list that I've been working through (pulled by git grep). The others will take a bit more attention because they *don't* seem to break functionality. I just filed bug 37152 with the list and what I've tested so far.
Comment 7 Katrin Fischer 2024-06-21 14:31:07 UTC 
(In reply to Emily Lamancusa from comment #6)
> (In reply to Katrin Fischer from comment #4)
> > It looks like we still got quite a few of "cud-delete-confirm" - should we
> > check those systematically?
> 
> I agree! That was my goal here as well. :) I actually do have a list that
> I've been working through (pulled by git grep). The others will take a bit
> more attention because they *don't* seem to break functionality. I just
> filed bug 37152 with the list and what I've tested so far.

Awesome, thanks Emily!
Comment 8 Lucas Gass (lukeg) 2024-07-18 19:23:29 UTC 
Backported to 24.05.x for upcoming 24.05.02
Comment 9 Fridolin Somers 2024-07-19 08:34:52 UTC 
Full CSRF not in 23.11.x
Comment 10 David Nind 2024-07-26 20:42:50 UTC 
No changes to the manual required - this fixes something that was not working.