Bug 17872

Summary: Fix small error in GetBudgetHierarchy and one of its calls
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: AcquisitionsAssignee: Marcel de Rooy <m.de.rooy>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: minor    
Priority: P5 - low CC: jonathan.druart, julian.maurice, katrin.fischer, kyle, mtj, veron
Version: MainKeywords: Academy
Hardware: All   
OS: All   
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
Bug 17872: Fix small error in GetBudgetHierarchy and one of its calls
Bug 17872: Fix small error in GetBudgetHierarchy and one of its calls

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