Bug 6137 - Fix a little bug when userenv is null
Summary: Fix a little bug when userenv is null
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: 3.6
Hardware: All All
: P3 trivial (vote)
Assignee: Julian Maurice
QA Contact: Bugs List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-08 09:19 UTC by Julian Maurice
Modified: 2012-10-25 23:10 UTC (History)
5 users (show)

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


Attachments
Patch (868 bytes, patch)
2011-04-08 09:26 UTC, Julian Maurice
Details | Diff | Splinter Review
Signed off patch (1.00 KB, patch)
2011-05-16 22:24 UTC, Chris Cormack
Details | Diff | Splinter Review
Unit tests for _GetCircControlBranch (3.59 KB, patch)
2012-01-27 15:09 UTC, Julian Maurice
Details | Diff | Splinter Review
Unit tests for _GetCircControlBranch (3.58 KB, patch)
2012-02-03 12:58 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 6137: unit tests for C4::Circulation::_GetCircControlBranch (3.63 KB, patch)
2012-02-13 12:41 UTC, Paul Poulain
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Maurice 2011-04-08 09:19:42 UTC
Coming patch just adds a test which check userenv is not null before using it.
Comment 1 Julian Maurice 2011-04-08 09:26:31 UTC Comment hidden (obsolete)
Comment 2 Chris Cormack 2011-05-16 22:23:30 UTC
<<<<<<< HEAD
    if ($circcontrol eq 'PickupLibrary') {
        $branch= C4::Context->userenv->{'branch'} if C4::Context->userenv;
=======
    if ($circcontrol eq 'PickupLibrary' and (C4::Context->userenv and C4::Context->userenv->{'branch'}) ){
        $branch= C4::Context->userenv->{'branch'};
>>>>>>> Bug 6137: fix a Circulation bug. userenv null

Patch conflicted, I chose the change in this patch to resolve the conflict as it is a more complete fix
Comment 3 Chris Cormack 2011-05-16 22:24:59 UTC
Created attachment 4132 [details] [review]
Signed off patch
Comment 4 Paul Poulain 2011-08-10 11:53:19 UTC
QA comment

I think this bugfix is OK, but a use case is needed to earn passed QA status: how can I reproduce the problem ?
Comment 5 Ian Walls 2011-09-22 12:14:56 UTC
Yes, going to need a test case so we can see what this bug looks like in action, and thus prove that the fix here does indeed fix the bad behaviour.
Comment 6 Marcel de Rooy 2011-11-07 14:42:15 UTC
As mentioned above, test plan is wanted.
Just looking at the code, the patch could change the flow through code. It seems that the check on existence should be handled within the if block, and provide the alternative in case it fails. Do I overlook something?
Comment 7 Julian Maurice 2012-01-27 15:09:47 UTC Comment hidden (obsolete)
Comment 8 Paul Poulain 2012-02-01 11:13:12 UTC
testing the unit tests, it seems something is wrong:

12:10 ~/koha.dev/koha-community (new/bug_6137 $%)$ prove t/Circulation.t t/Circulation.t .. 2/? # Now, set a userenv
t/Circulation.t .. All 15 subtests passed 

Test Summary Report
-------------------
t/Circulation.t (Wstat: 0 Tests: 15 Failed: 0)
  Parse errors: Plan (1..15) must be at the beginning or end of the TAP output
Files=1, Tests=15,  2 wallclock secs ( 0.04 usr  0.01 sys +  0.92 cusr  0.06 csys =  1.03 CPU)
Result: FAIL


marking failed QA for now,
Comment 9 Julian Maurice 2012-02-03 12:58:16 UTC Comment hidden (obsolete)
Comment 10 Paul Poulain 2012-02-13 12:41:11 UTC
Created attachment 7617 [details] [review]
Bug 6137: unit tests for C4::Circulation::_GetCircControlBranch

Signed-off-by: Paul Poulain <paul.poulain@biblibre.com>
Comment 11 Paul Poulain 2012-02-13 12:44:22 UTC
OK, unit test signed-off, but I've one concern: chris, as this patch check for some syspref, it should be in t/db_dependant, isn't it ?
Comment 12 Chris Cormack 2012-02-14 00:52:54 UTC
Yep, looks like it should be to me. (Note Julian wrote the unit test not me ;))
Comment 13 Paul Poulain 2012-02-17 17:41:30 UTC
QA comment, OK, passed QA, will move the test to db_dependant directory when pushing
Comment 14 Julian Maurice 2012-03-08 08:22:41 UTC
patch is in master and 3.6.x branches, marking as resolved