Bug 18351 - No warning when deleting budgets that have funds attached
Summary: No warning when deleting budgets that have funds attached
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Aleisha Amohia
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-03-30 03:08 UTC by Aleisha Amohia
Modified: 2017-11-23 23:11 UTC (History)
11 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
Bug 18351: [FOLLOW-UP] Prevent deletion if funds are added after clicking 'Delete' and before confirming delete (2.51 KB, patch)
2017-08-18 02:00 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 18351: [FOLLOW-UP] Prevent deletion if funds are added after clicking 'Delete' and before confirming delete (2.46 KB, patch)
2017-08-22 04:57 UTC, Dilan Johnpullé
Details | Diff | Splinter Review
Bug 18351: [FOLLOW-UP] Removing swp file (11.46 KB, patch)
2017-09-03 23:30 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 18351: Able to delete budget with funds (9.73 KB, patch)
2017-09-08 07:40 UTC, Marcel de Rooy
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.
Comment 20 Aleisha Amohia 2017-08-18 02:00:35 UTC
Created attachment 66160 [details] [review]
Bug 18351: [FOLLOW-UP] Prevent deletion if funds are added after clicking 'Delete' and before confirming delete

To test:
1) Create a budget with no fund
2) Click Delete but do not confirm
3) In another tab, add a fund to this budget
4) Go back to original tab and click 'Yes, delete'
5) Confirm budget is not deleted and an appropriate error message shows
Comment 21 Dilan Johnpullé 2017-08-22 04:57:15 UTC
Created attachment 66304 [details] [review]
Bug 18351: [FOLLOW-UP] Prevent deletion if funds are added after clicking 'Delete' and before confirming delete

Followed test plan and patch works as described.

Signed-off-by: Dilan Johnpullé <dilan@calyx.net.au>
Comment 22 Marcel de Rooy 2017-08-25 08:17:25 UTC
+    my @funds = Koha::Database->new()->schema->resultset('Aqbudget')->search({ budget_period_id => $budgetperiod->{budget_period_id} });

Please introduce a Koha Object for that one.
Copy and paste operation. Should not be hard ;)
Comment 23 Aleisha Amohia 2017-08-27 21:56:57 UTC
(In reply to Marcel de Rooy from comment #22)
> +    my @funds =
> Koha::Database->new()->schema->resultset('Aqbudget')->search({
> budget_period_id => $budgetperiod->{budget_period_id} });
> 
> Please introduce a Koha Object for that one.
> Copy and paste operation. Should not be hard ;)

Hi Marcel,

Where is this line? It may have been fixed in a follow-up patch as I can't find it in the most recent version.

Aleisha
Comment 24 Marcel de Rooy 2017-08-28 07:03:31 UTC
(In reply to Aleisha Amohia from comment #23)
> (In reply to Marcel de Rooy from comment #22)
> > +    my @funds =
> > Koha::Database->new()->schema->resultset('Aqbudget')->search({
> > budget_period_id => $budgetperiod->{budget_period_id} });
> > 
> > Please introduce a Koha Object for that one.
> > Copy and paste operation. Should not be hard ;)
> 
> Hi Marcel,
> 
> Where is this line? It may have been fixed in a follow-up patch as I can't
> find it in the most recent version.
> 
> Aleisha

OK I did not check that.
Will come back here later.
Comment 25 Marcel de Rooy 2017-09-01 05:45:20 UTC
QA: Looking here now
Comment 26 Marcel de Rooy 2017-09-01 05:48:44 UTC
 admin/aqbudgetperiods.pl                                |   4 ++--
 .../prog/en/modules/admin/.aqbudgetperiods.tt.swp       | Bin 0 -> 40960 bytes
 2 files changed, 2 insertions(+), 2 deletions(-)
 create mode 100644 koha-tmpl/intranet-tmpl/prog/en/modules/admin/.aqbudgetperiods.tt.swp

Please remove swap files.
Comment 27 Aleisha Amohia 2017-09-03 23:30:47 UTC
Created attachment 66791 [details] [review]
Bug 18351: [FOLLOW-UP] Removing swp file
Comment 28 Marcel de Rooy 2017-09-08 07:00:46 UTC
(In reply to Aleisha Amohia from comment #27)
> Created attachment 66791 [details] [review] [review]
> Bug 18351: [FOLLOW-UP] Removing swp file

Thx for responding. I meant: get rid of the swp file in the original patch. No need to have it in git. I will fix it now.

QA: Looking here now.
Comment 29 Marcel de Rooy 2017-09-08 07:40:28 UTC
Created attachment 66974 [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>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

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>

Bug 18351: [FOLLOW-UP] Code fix

See comment 10.
Ready for testing.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

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>

Bug 18351: [FOLLOW-UP] Prevent deletion if funds are added after clicking 'Delete' and before confirming delete

Followed test plan and patch works as described.

Signed-off-by: Dilan Johnpullé <dilan@calyx.net.au>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 30 Marcel de Rooy 2017-09-08 07:41:21 UTC
Looks good to me. Minor comment (no blocker):

The script contains a $total variable that seems to be unused (not passed to template).
The template now contains:
        [% IF ( total || funds_exist ) %]
            <span>Cannot delete budget '[% budget_period_description %]'</span>
The total check appears to be unneeded.
Similary:
[% IF ( delete_confirm ) %]
    [% IF ( total ) %]
        <div class="dialog message">
        <h3>Cannot delete budget</h3>
        <p><strong>This record is used [% total %] times</strong>
        . Deletion is not possible.</p>
    [% ELSIF ( funds_exist ) %]
        <div class="dialog message">
        <h3>Cannot delete budget</h3>
        <p>This budget has funds attached. You must delete all attached funds before you can delete this budget.</p>
The total branch here probably was meant for existing funds, but seems unused.

Passed QA
Comment 31 Jonathan Druart 2017-09-12 15:11:50 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 32 Fridolin SOMERS 2017-10-10 10:30:30 UTC
Pushed to 17.05.x, will be in 17.05.05.
Comment 33 Katrin Fischer 2017-10-14 12:38:34 UTC
This patch has been pushed to 16.11.x and will be in 16.11.13.
Comment 34 Mason James 2017-11-23 23:11:41 UTC
Pushed to 16.05.x, for 16.05.18 release