From cc2a62bdee88908edb429155ae0d274ee9a6dfdb Mon Sep 17 00:00:00 2001
From: Galen Charlton <gmc@esilibrary.com>
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 <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>
---
 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