Bug 17872 - Fix small error in GetBudgetHierarchy and one of its calls
Summary: Fix small error in GetBudgetHierarchy and one of its calls
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords: Academy
Depends on:
Blocks:
 
Reported: 2017-01-10 09:03 UTC by Marcel de Rooy
Modified: 2017-12-07 22:20 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 17872: Fix small error in GetBudgetHierarchy and one of its calls (2.42 KB, patch)
2017-01-10 09:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17872: Fix small error in GetBudgetHierarchy and one of its calls (2.57 KB, patch)
2017-04-11 20:05 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 17872: Fix small error in GetBudgetHierarchy and one of its calls (2.65 KB, patch)
2017-04-11 21:39 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2017-01-10 09:03:23 UTC

    
Comment 1 Marcel de Rooy 2017-01-10 09:10:01 UTC
Created attachment 58723 [details] [review]
Bug 17872: Fix small error in GetBudgetHierarchy and one of its calls

In aqbudgetperiods.pl a commented line is removed that contains a
wrong userenv hash key. Should be branch instead of branchcode.

In aqbudgets GetBudgetHierarchy is called with the same wrong userenv
hash key. Should be userenv->{branch}.
This made another bug visible: if you call GetBudgetHierarchy with a
branch and without owner, the where clause should take into account
that the branchcode can be empty (not null).

Test plan:
[1] Run Budgets.t
[2] Run aqbudgets.pl from Administration
[3] Add the show_mine=1 parameter in the URL
[4] Change owner of one of the funds and repeat step 2 and 3.
Comment 2 Mark Tompsett 2017-01-20 20:03:07 UTC
Comment on attachment 58723 [details] [review]
Bug 17872: Fix small error in GetBudgetHierarchy and one of its calls

Review of attachment 58723 [details] [review]:
-----------------------------------------------------------------

::: admin/aqbudgets.pl
@@ +237,4 @@
>      );
>  
>      my @budgets = @{
> +        GetBudgetHierarchy( $$period{budget_period_id}, C4::Context->userenv->{branch}, ( $show_mine ? $borrower_id : 0 ))

Really don't like C4::Context->userenv->{branch}!
Can this code be triggered by a test which isn't "logged in"?
Comment 3 Marcel de Rooy 2017-01-23 07:53:59 UTC
(In reply to M. Tompsett from comment #2)
> Comment on attachment 58723 [details] [review] [review]
> Bug 17872: Fix small error in GetBudgetHierarchy and one of its calls
> 
> Review of attachment 58723 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: admin/aqbudgets.pl
> @@ +237,4 @@
> >      );
> >  
> >      my @budgets = @{
> > +        GetBudgetHierarchy( $$period{budget_period_id}, C4::Context->userenv->{branch}, ( $show_mine ? $borrower_id : 0 ))
> 
> Really don't like C4::Context->userenv->{branch}!
> Can this code be triggered by a test which isn't "logged in"?

Corrects the even worse userenv->{branchcode}.
This code cannot be reached when you are not logged in.

Would you feel better with:
my $userenv = C4::Context->userenv;
my $branch = $userenv ? $userenv->{branch} : undef;
Comment 4 Jonathan Druart 2017-01-23 08:18:13 UTC
(In reply to M. Tompsett from comment #2)
> Really don't like C4::Context->userenv->{branch}!
> Can this code be triggered by a test which isn't "logged in"?

You have to be logged in to see this view.
That would be a completely unnecessary test.
Comment 5 Marc Véron 2017-04-11 20:05:33 UTC
Created attachment 62064 [details] [review]
Bug 17872: Fix small error in GetBudgetHierarchy and one of its calls

In aqbudgetperiods.pl a commented line is removed that contains a
wrong userenv hash key. Should be branch instead of branchcode.

In aqbudgets GetBudgetHierarchy is called with the same wrong userenv
hash key. Should be userenv->{branch}.
This made another bug visible: if you call GetBudgetHierarchy with a
branch and without owner, the where clause should take into account
that the branchcode can be empty (not null).

Test plan:
[1] Run Budgets.t
[2] Run aqbudgets.pl from Administration
[3] Add the show_mine=1 parameter in the URL
[4] Change owner of one of the funds and repeat step 2 and 3.

t/Budgets.t returns green. Followed test plan, works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 6 Jonathan Druart 2017-04-11 21:39:09 UTC
Created attachment 62066 [details] [review]
Bug 17872: Fix small error in GetBudgetHierarchy and one of its calls

In aqbudgetperiods.pl a commented line is removed that contains a
wrong userenv hash key. Should be branch instead of branchcode.

In aqbudgets GetBudgetHierarchy is called with the same wrong userenv
hash key. Should be userenv->{branch}.
This made another bug visible: if you call GetBudgetHierarchy with a
branch and without owner, the where clause should take into account
that the branchcode can be empty (not null).

Test plan:
[1] Run Budgets.t
[2] Run aqbudgets.pl from Administration
[3] Add the show_mine=1 parameter in the URL
[4] Change owner of one of the funds and repeat step 2 and 3.

t/Budgets.t returns green. Followed test plan, works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 7 Marcel de Rooy 2017-04-12 11:17:57 UTC
(In reply to Jonathan Druart from comment #6)

> Signed-off-by: Marc Véron <veron@veron.ch>
> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Thanks, Marc and Jonathan.
Comment 8 Kyle M Hall 2017-04-13 12:34:31 UTC
Pushed to master for 17.05, thanks Marcel!
Comment 9 Katrin Fischer 2017-04-16 12:15:07 UTC
This patch has been pushed to 16.11.x and will be in 16.11.07.
Comment 10 Julian Maurice 2017-04-21 11:10:53 UTC
Pushed to 3.22.x for 3.22.20
Comment 11 Mason James 2017-05-03 04:26:50 UTC
Pushed to 16.05.x, for 16.05.12 release