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
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
Patch tested with a sandbox, by Felix Hemme <felix.hemme@thulb.uni-jena.de>
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 on step 4: funds are stored in table aqbudgets
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.
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?
(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.
Created attachment 64251 [details] [review] Bug 18351: [FOLLOW-UP] Some code fixes See Comment 5. Ready to test.
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>
GetBudgets returns a reference, not an array, try: my $funds = GetBudgets({ budget_period_id => $budgetperiod->{budget_period_id} }); $budgetperiod->{count} = scalar @$funds;
Created attachment 64664 [details] [review] Bug 18351: [FOLLOW-UP] Code fix See comment 10. Ready for testing.
I have been able to reproduce the test plan with exit in a kohadevbox
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.
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.
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>
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>
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>
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>
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.
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
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>
+ 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 ;)
(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
(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.
QA: Looking here now
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.
Created attachment 66791 [details] [review] Bug 18351: [FOLLOW-UP] Removing swp file
(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.
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>
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
Pushed to master for 17.11, thanks to everybody involved!
Pushed to 17.05.x, will be in 17.05.05.
This patch has been pushed to 16.11.x and will be in 16.11.13.
Pushed to 16.05.x, for 16.05.18 release