Bug 11732 - Eliminate warning on undefined branchcode
Summary: Eliminate warning on undefined branchcode
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: Main
Hardware: All All
: P5 - low trivial (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-02-10 12:28 UTC by Marcel de Rooy
Modified: 2014-12-07 20:05 UTC (History)
3 users (show)

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


Attachments
Bug 11732: Eliminate warning on undefined branchcode (1.08 KB, patch)
2014-02-10 12:29 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 11732: Eliminate warning on undefined branchcode (1.22 KB, patch)
2014-02-12 14:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[SIGNED OFF] Bug 11732: Eliminate warning on undefined branchcode (1.56 KB, patch)
2014-02-13 18:52 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 11732: Eliminate warning on undefined branchcode (1.61 KB, patch)
2014-02-14 15:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11732: Eliminate warning on undefined branchcode (1.70 KB, patch)
2014-02-14 15:28 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 2014-02-10 12:28:11 UTC
When you run the Reserves test, you have the warnings:
Use of uninitialized value $branchcode in hash element at /usr/share/koha/testclone/C4/Letters.pm line 138.
Use of uninitialized value $branchcode in hash element at /usr/share/koha/testclone/C4/Letters.pm line 148.

This patch removes that warning.
Comment 1 Marcel de Rooy 2014-02-10 12:29:55 UTC Comment hidden (obsolete)
Comment 2 Mark Tompsett 2014-02-10 15:54:14 UTC
I tried to test this, but test 7 fails when I run:
prove -v t/db_dependent/Reserves.t
And an error message is not triggered even in master. I suspect I don't have things set to pretend to be logged in.
This eyeballs just fine, but I was wondering...
why not move line 129 to after the if?
And why not // instead of ||?
Sorry, I couldn't trigger this.
Comment 3 Marcel de Rooy 2014-02-12 14:38:56 UTC Comment hidden (obsolete)
Comment 4 Marcel de Rooy 2014-02-12 14:42:15 UTC
(In reply to M. Tompsett from comment #2)
> I tried to test this, but test 7 fails when I run:
> prove -v t/db_dependent/Reserves.t
> And an error message is not triggered even in master. I suspect I don't have
> things set to pretend to be logged in.
The test creates branches and categories, even sets a pref, so this apparently requires more digging into other circulation settings.. It is interesting to know why it fails (with the AddReturn call, I understand), but this report just fixes a trivial uninitialized value.

> This eyeballs just fine, but I was wondering...
> why not move line 129 to after the if?
> And why not // instead of ||?
Fine with me. New patch submitted.
Comment 5 Mark Tompsett 2014-02-12 18:29:58 UTC
I still can't trigger it with t/db_dependent/Reserves.t
However, I did trigger your patch with:
    perl -e "use C4::Letters; my \$blah=C4::Letters::getletter('circulation','DUE');"

This got me thinking. Shouldn't I be able to trigger this with:
    prove -v t/Letters.t
OR
    prove -v t/db_dependent/Letters.t
? At which point I discovered some rather ugly failings.

Could you be explicit to the point of annoyance to confirm that I am trying to test it the same way, and have the same settings, and have similar data which triggers the error in master?
Comment 6 Marcel de Rooy 2014-02-13 08:03:21 UTC
(In reply to M. Tompsett from comment #5)
> Could you be explicit to the point of annoyance to confirm that I am trying
> to test it the same way, and have the same settings, and have similar data
> which triggers the error in master?

No annoyance as long as you keep testing :)
t/db_dependent/Letters.t does indeed produce some ugly errors with me too. Does not call getletter.
t/Letters.t does not call getletter; so will not trigger the patch too.
The call ..$var=getletter(..).. will imo not trigger the patch; userenv will return false. 
Note that Reserves.t also overrides userenv on line 23. We could do the same: 
perl -e "use C4::Letters; *C4::Context::userenv= sub { return {} }; my $blah=C4::Letters::getletter('circulation','DUE', 'BRA');"
Without the patch, you have a warning. With the patch, you will have not.
Comment 7 Mark Tompsett 2014-02-13 18:52:48 UTC Comment hidden (obsolete)
Comment 8 Kyle M Hall 2014-02-14 15:12:28 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2014-02-14 15:28:06 UTC
Created attachment 25275 [details] [review]
Bug 11732: Eliminate warning on undefined branchcode

When you run the Reserves test, you have the warnings:
Use of uninitialized value $branchcode in hash element at /usr/share/koha/testclone/C4/Letters.pm line 138.
Use of uninitialized value $branchcode in hash element at /usr/share/koha/testclone/C4/Letters.pm line 148.
This patch removes that warning.

Test plan:
Run the Reserves.t again.

Revised Test Plan
-----------------
Run the following on the command line prompt before and after
applying the patch:
    perl -e "use C4::Letters; *C4::Context::userenv= sub { return {} }; my \$blah=C4::Letters::getletter('circulation','DUE', 'BRA');"
Before the patch there will be errors (as above), after there will not.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
IndependentBranches must be on.
Comment 10 Galen Charlton 2014-02-19 21:21:32 UTC
Pushed to master.  Thanks, Marcel!
Comment 11 Fridolin Somers 2014-05-19 15:30:41 UTC
Pushed to 3.14.x, will be in 3.14.07