From 9a27518493c99061d6f8f4119faa445b5ec51e64 Mon Sep 17 00:00:00 2001 From: Galen Charlton Date: Thu, 11 Jul 2013 16:41:26 +0000 Subject: [PATCH] bug 10577: improve semantics of GetBudgetPeriod() Remove the option to pass zero to this function in order to get "the" active budget. This was a problem in three ways: - Koha doesn't require that there be only one active budget at a time, so the concept of "the" active budget doesn't make sense. - Having the single parameter be either an ID or a flag based on its value is poor function design. - No callers of GetBudgetPeriod() were actually using this modality. This patch also improves the DB-dependent tests for budgets by - wrapping the test in a transaction - counting budgets correctly To test: [1] Apply the patch. [2] Verify that prove -v t/db_dependent/Budgets.t passes [3] Verify in the staff interface that: - the budget hierarchy displays correctly - you can add and modify a budget Signed-off-by: Galen Charlton --- C4/Budgets.pm | 28 +++++++--------------------- t/db_dependent/Budgets.t | 23 +++++++++++++++++++---- 2 files changed, 26 insertions(+), 25 deletions(-) diff --git a/C4/Budgets.pm b/C4/Budgets.pm index 2c78284..24c4207 100644 --- a/C4/Budgets.pm +++ b/C4/Budgets.pm @@ -446,27 +446,13 @@ sub GetBudgetPeriods { sub GetBudgetPeriod { my ($budget_period_id) = @_; my $dbh = C4::Context->dbh; - ## $total = number of records linked to the record that must be deleted - my $total = 0; - ## get information about the record that will be deleted - my $sth; - if ($budget_period_id) { - $sth = $dbh->prepare( qq| - SELECT * - FROM aqbudgetperiods - WHERE budget_period_id=? | - ); - $sth->execute($budget_period_id); - } else { # ACTIVE BUDGET - $sth = $dbh->prepare(qq| - SELECT * - FROM aqbudgetperiods - WHERE budget_period_active=1 | - ); - $sth->execute(); - } - my $data = $sth->fetchrow_hashref; - return $data; + my $sth = $dbh->prepare( qq| + SELECT * + FROM aqbudgetperiods + WHERE budget_period_id=? | + ); + $sth->execute($budget_period_id); + return $sth->fetchrow_hashref; } # ------------------------------------------------------------------- diff --git a/t/db_dependent/Budgets.t b/t/db_dependent/Budgets.t index 717f8fe..2a2439d 100755 --- a/t/db_dependent/Budgets.t +++ b/t/db_dependent/Budgets.t @@ -7,6 +7,11 @@ use C4::Dates; use YAML; +# Start transaction +my $dbh = C4::Context->dbh; +$dbh->{AutoCommit} = 0; +$dbh->{RaiseError} = 1; + # # Budget Periods : # @@ -15,6 +20,9 @@ my $budgetperiod; my $active_period; my $mod_status; my $del_status; + + +my $orig_num_active = count_active_budget_periods(); ok($bpid=AddBudgetPeriod( { budget_period_startdate => '2008-01-01' , budget_period_enddate => '2008-12-31' @@ -23,11 +31,13 @@ ok($bpid=AddBudgetPeriod( ok($budgetperiod=GetBudgetPeriod($bpid), "GetBudgetPeriod($bpid) returned ".Dump($budgetperiod)); -ok(!GetBudgetPeriod(0) ,"GetBudgetPeriod(0) returned undef : noactive BudgetPeriod"); -$$budgetperiod{budget_period_active}=1; +my $num_active = count_active_budget_periods(); +ok($num_active == $orig_num_active, 'new budget is in inactive by default'); +$budgetperiod->{budget_period_active}=1; ok($mod_status=ModBudgetPeriod($budgetperiod),"ModBudgetPeriod OK"); -ok($active_period=GetBudgetPeriod(0),"GetBudgetPeriod(0) returned".Dump($active_period)); -ok(scalar(GetBudgetPeriods())>0,"GetBudgetPeriods OK");#Should at least return the Budget inserted +$num_active = count_active_budget_periods(); +ok($num_active == ($orig_num_active + 1), 'making new budget active increased the number of active budgets by 1'); +ok(scalar(@{ GetBudgetPeriods() }) > 0,"GetBudgetPeriods OK");#Should at least return the Budget inserted ok($del_status=DelBudgetPeriod($bpid),"DelBudgetPeriod returned $del_status"); # @@ -104,3 +114,8 @@ is($budget_name, $budget->{budget_name}, "Test the GetBudgetName routine"); ok($del_status=DelBudget($budget_id), "DelBudget returned $del_status"); + +sub count_active_budget_periods { + my $budget_periods = GetBudgetPeriods({budget_period_active => 1}); + return scalar(@$budget_periods); +} -- 1.7.10.4