From cc2a62bdee88908edb429155ae0d274ee9a6dfdb 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 Rescued-by: Martin Renvoize Signed-off-by: Martin Renvoize Signed-off-by: Josef Moravec Signed-off-by: Nick Clemens --- C4/Budgets.pm | 28 +++++++--------------------- t/db_dependent/Budgets.t | 8 +------- 2 files changed, 8 insertions(+), 28 deletions(-) diff --git a/C4/Budgets.pm b/C4/Budgets.pm index 9c53236f7d..493e2b7b1e 100644 --- a/C4/Budgets.pm +++ b/C4/Budgets.pm @@ -442,27 +442,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; } sub DelBudgetPeriod{ diff --git a/t/db_dependent/Budgets.t b/t/db_dependent/Budgets.t index db9787fb4e..339d5bd6e4 100755 --- a/t/db_dependent/Budgets.t +++ b/t/db_dependent/Budgets.t @@ -1,6 +1,6 @@ #!/usr/bin/perl use Modern::Perl; -use Test::More tests => 147; +use Test::More tests => 144; BEGIN { use_ok('C4::Budgets') @@ -58,7 +58,6 @@ $bpid = AddBudgetPeriod({ budget_period_enddate => '2008-12-31', }); is( $bpid, undef, 'AddBugetPeriod without start date returns undef' ); -is( GetBudgetPeriod(0), undef ,'GetBudgetPeriod(0) returned undef : noactive BudgetPeriod' ); my $budgetperiods = GetBudgetPeriods(); is( @$budgetperiods, 0, 'GetBudgetPeriods returns the correct number of budget periods' ); @@ -76,8 +75,6 @@ is( $budgetperiod->{budget_period_startdate}, $my_budgetperiod->{budget_period_s is( $budgetperiod->{budget_period_enddate}, $my_budgetperiod->{budget_period_enddate}, 'AddBudgetPeriod stores the end date correctly' ); is( $budgetperiod->{budget_period_description}, $my_budgetperiod->{budget_period_description}, 'AddBudgetPeriod stores the description correctly' ); is( $budgetperiod->{budget_period_active}, $my_budgetperiod->{budget_period_active}, 'AddBudgetPeriod stores active correctly' ); -is( GetBudgetPeriod(0), undef ,'GetBudgetPeriod(0) returned undef : noactive BudgetPeriod' ); - $my_budgetperiod = { budget_period_startdate => '2009-01-01', @@ -96,8 +93,6 @@ is( $budgetperiod->{budget_period_startdate}, $my_budgetperiod->{budget_period_s is( $budgetperiod->{budget_period_enddate}, $my_budgetperiod->{budget_period_enddate}, 'ModBudgetPeriod updates the end date correctly' ); is( $budgetperiod->{budget_period_description}, $my_budgetperiod->{budget_period_description}, 'ModBudgetPeriod updates the description correctly' ); is( $budgetperiod->{budget_period_active}, $my_budgetperiod->{budget_period_active}, 'ModBudgetPeriod upates active correctly' ); -isnt( GetBudgetPeriod(0), undef, 'GetBugetPeriods functions correctly' ); - $budgetperiods = GetBudgetPeriods(); is( @$budgetperiods, 1, 'GetBudgetPeriods returns the correct number of budget periods' ); @@ -111,7 +106,6 @@ is( DelBudgetPeriod($bpid), 1, 'DelBudgetPeriod returns true' ); $budgetperiods = GetBudgetPeriods(); is( @$budgetperiods, 0, 'GetBudgetPeriods returns the correct number of budget periods' ); - # # Budget : # -- 2.39.5