Bugzilla – Attachment 19578 Details for
Bug 10577
C4::Budgets::GetBudgetPeriod has inappropriate overloading of its behavior
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
bug 10577: improve semantics of GetBudgetPeriod()
bug-10577-improve-semantics-of-GetBudgetPeriod.patch (text/plain), 4.26 KB, created by
Galen Charlton
on 2013-07-11 16:47:12 UTC
(
hide
)
Description:
bug 10577: improve semantics of GetBudgetPeriod()
Filename:
MIME Type:
Creator:
Galen Charlton
Created:
2013-07-11 16:47:12 UTC
Size:
4.26 KB
patch
obsolete
>From 9a27518493c99061d6f8f4119faa445b5ec51e64 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> >--- > 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
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 10577
:
19578
|
86563
|
86564
|
86565
|
86566
|
87009
|
87010
|
87080
|
87081
|
87082