Coming patch just adds a test which check userenv is not null before using it.
Created attachment 3829 [details] [review] Patch
<<<<<<< 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
Created attachment 4132 [details] [review] Signed off patch
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 ?
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.
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?
Created attachment 7354 [details] [review] Unit tests for _GetCircControlBranch These unit tests call _GetCircControlBranch for each value of syspref CircControl, with and without a userenv. Not sure about what should be returned when userenv is not set, undef or the item branch (or maybe the patron branch)? Without the first patch it returns undef, and the item branch with the patch applied. Tests expect that it must returns the item branch when userenv is not set.
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,
Created attachment 7443 [details] [review] Unit tests for _GetCircControlBranch prove failed because "plan tests => 15" was executed after the first test. Now it's "use Test::More tests => 15", and prove returns success.
Created attachment 7617 [details] [review] Bug 6137: unit tests for C4::Circulation::_GetCircControlBranch Signed-off-by: Paul Poulain <paul.poulain@biblibre.com>
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 ?
Yep, looks like it should be to me. (Note Julian wrote the unit test not me ;))
QA comment, OK, passed QA, will move the test to db_dependant directory when pushing
patch is in master and 3.6.x branches, marking as resolved