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 <gmc@esilibrary.com>
Rescued-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

diff --git a/C4/Budgets.pm b/C4/Budgets.pm
index 9c53236..493e2b7 100644 (file)
--- 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 db9787f..339d5bd 100755 (executable)
--- 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  :
 #