Bug 18351 - No warning when deleting budgets that have funds attached
Summary: No warning when deleting budgets that have funds attached
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Assignee: Aleisha Amohia
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-03-30 03:08 UTC by Aleisha Amohia
Modified: 2017-08-11 08:56 UTC (History)
7 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 18351: Able to delete budget with funds (5.93 KB, patch)
2017-04-04 20:46 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 18351: Able to delete budget with funds (5.99 KB, patch)
2017-04-05 09:59 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 18351: [FOLLOW-UP] Some code fixes (12.65 KB, patch)
2017-06-13 05:08 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 18351: [FOLLOW-UP] Some code fixes (12.70 KB, patch)
2017-06-16 17:02 UTC, Lee Jamison
Details | Diff | Splinter Review
Bug 18351: [FOLLOW-UP] Code fix (975 bytes, patch)
2017-06-27 00:12 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 18351: [FOLLOW-UP] Prevent deletion from forcing URL (3.40 KB, patch)
2017-08-03 02:06 UTC, Aleisha Amohia
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 18351: Able to delete budget with funds (6.05 KB, patch)
2017-08-07 15:18 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 18351: [FOLLOW-UP] Some code fixes (12.76 KB, patch)
2017-08-07 15:18 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 18351: [FOLLOW-UP] Code fix (1.01 KB, patch)
2017-08-07 15:18 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 18351: [FOLLOW-UP] Prevent deletion from forcing URL (3.46 KB, patch)
2017-08-07 15:18 UTC, Owen Leonard
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Aleisha Amohia 2017-03-30 03:08:39 UTC
At the moment, you can delete any budget, regardless of whether there are funds attached. This is bad because it means the funds still exist but the user can't see them and the user can't create a new budget with the same budget ID
Comment 1 Aleisha Amohia 2017-04-04 20:46:50 UTC
Created attachment 61846 [details] [review]
Bug 18351: Able to delete budget with funds

To test:
1) Create a budget, add a fund
2) Delete budget. Notice this is successful and triggers no warning
message etc.
3) Go to Funds. Notice the funds appear as if they are not there
4) Go into mysql and view the aqbudgetperiods table - notice the funds
are still there and are now inaccessible.
5) Apply patch
6) Create a budget, add a fund
7) Attempt to delete budget. Notice you can't click Delete button.
Confirm number of funds in hover message is correct.
8) Delete fund
9) Confirm you can now delete budget.

Sponsored-by: Catalyst IT
Comment 2 sandboxes@biblibre.com 2017-04-05 09:59:11 UTC
Patch tested with a sandbox, by Felix Hemme <felix.hemme@thulb.uni-jena.de>
Comment 3 sandboxes@biblibre.com 2017-04-05 09:59:32 UTC
Created attachment 61871 [details] [review]
Bug 18351: Able to delete budget with funds

To test:
1) Create a budget, add a fund
2) Delete budget. Notice this is successful and triggers no warning
message etc.
3) Go to Funds. Notice the funds appear as if they are not there
4) Go into mysql and view the aqbudgetperiods table - notice the funds
are still there and are now inaccessible.
5) Apply patch
6) Create a budget, add a fund
7) Attempt to delete budget. Notice you can't click Delete button.
Confirm number of funds in hover message is correct.
8) Delete fund
9) Confirm you can now delete budget.

Sponsored-by: Catalyst IT

Signed-off-by: Felix Hemme <felix.hemme@thulb.uni-jena.de>
Comment 4 Felix Hemme 2017-04-05 10:02:35 UTC
Comment on step 4: funds are stored in table aqbudgets
Comment 5 Jonathan Druart 2017-04-12 16:34:58 UTC
Aleisha,
I'd suggest to use C4::Budgets::GetBudgets with ({ budget_period_id => $budget_period_id }) instead of calling
  Koha::Database->new()->schema->resultset('Aqbudget')->search

It will make things easier to rewrite later. Indeed we should use a Koha::Acquisition::Funds->search here, but does not exist yet.
Comment 6 Aleisha Amohia 2017-04-18 01:36:15 UTC
I was about to fix the comment above but I realised that this seems to have been fixed in the latest version of Koha - can someone please check and try to reproduce this bug?
Comment 7 Josef Moravec 2017-05-10 08:05:31 UTC
(In reply to Aleisha Amohia from comment #6)
> I was about to fix the comment above but I realised that this seems to have
> been fixed in the latest version of Koha - can someone please check and try
> to reproduce this bug?

I could reproduce the bug on current master.
Comment 8 Aleisha Amohia 2017-06-13 05:08:00 UTC
Created attachment 64251 [details] [review]
Bug 18351: [FOLLOW-UP] Some code fixes

See Comment 5. Ready to test.
Comment 9 Lee Jamison 2017-06-16 17:02:21 UTC
Created attachment 64405 [details] [review]
Bug 18351: [FOLLOW-UP] Some code fixes

See Comment 5. Ready to test.

Signed-off-by: Lee Jamison <ldjamison@marywood.edu>
Comment 10 Nick Clemens 2017-06-23 10:33:01 UTC
GetBudgets returns a reference, not an array, try:
my $funds = GetBudgets({ budget_period_id => $budgetperiod->{budget_period_id} }); 
$budgetperiod->{count} = scalar @$funds;
Comment 11 Aleisha Amohia 2017-06-27 00:12:40 UTC
Created attachment 64664 [details] [review]
Bug 18351: [FOLLOW-UP] Code fix

See comment 10.
Ready for testing.
Comment 12 Hugo Agud 2017-07-25 07:45:43 UTC
I have been able to reproduce the test plan with exit in a kohadevbox
Comment 13 Nick Clemens 2017-07-31 12:42:25 UTC
Sorry Aleisha, should have caught this last pass.

The patch does prevent the button from cancelling the deletion, however, deletion is still allowed when forcing the URL:
/cgi-bin/koha/admin/aqbudgetperiods.pl?op=delete_confirm&
budget_period_id=##

We should either add the check in the script as well, or offer the option to delete the funds too.
Comment 14 Aleisha Amohia 2017-08-03 02:06:49 UTC
Created attachment 65440 [details] [review]
Bug 18351: [FOLLOW-UP] Prevent deletion from forcing URL

This patch adds a check in the script for existing funds so that the
budget cannot be deleted when forcing the URL and has other small fixes.
Comment 15 Owen Leonard 2017-08-07 15:18:02 UTC
Created attachment 65569 [details] [review]
[SIGNED-OFF] Bug 18351: Able to delete budget with funds

To test:
1) Create a budget, add a fund
2) Delete budget. Notice this is successful and triggers no warning
message etc.
3) Go to Funds. Notice the funds appear as if they are not there
4) Go into mysql and view the aqbudgetperiods table - notice the funds
are still there and are now inaccessible.
5) Apply patch
6) Create a budget, add a fund
7) Attempt to delete budget. Notice you can't click Delete button.
Confirm number of funds in hover message is correct.
8) Delete fund
9) Confirm you can now delete budget.

Sponsored-by: Catalyst IT

Signed-off-by: Felix Hemme <felix.hemme@thulb.uni-jena.de>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 16 Owen Leonard 2017-08-07 15:18:05 UTC
Created attachment 65570 [details] [review]
[SIGNED-OFF] Bug 18351: [FOLLOW-UP] Some code fixes

See Comment 5. Ready to test.

Signed-off-by: Lee Jamison <ldjamison@marywood.edu>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 17 Owen Leonard 2017-08-07 15:18:08 UTC
Created attachment 65571 [details] [review]
[SIGNED-OFF] Bug 18351: [FOLLOW-UP] Code fix

See comment 10.
Ready for testing.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 18 Owen Leonard 2017-08-07 15:18:12 UTC
Created attachment 65572 [details] [review]
[SIGNED-OFF] Bug 18351: [FOLLOW-UP] Prevent deletion from forcing URL

This patch adds a check in the script for existing funds so that the
budget cannot be deleted when forcing the URL and has other small fixes.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 19 Julian Maurice 2017-08-11 08:56:16 UTC
Aleisha, there's still one check missing, after the user clicks on 'Yes, delete'.
To reproduce:

1. Create a budget with no fund
2. Try to delete the budget but do not click on 'Yes, delete'
3. In another tab, add a fund to this budget.
4. Go back to the first tab and click on 'Yes, delete'.
5. The budget is deleted.