| Summary: | Fix a little bug when userenv is null | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Julian Maurice <julian.maurice> |
| Component: | Circulation | Assignee: | Julian Maurice <julian.maurice> |
| Status: | CLOSED FIXED | QA Contact: | Bugs List <koha-bugs> |
| Severity: | trivial | ||
| Priority: | P3 | CC: | chris, gmcharlt, koha.sekjal, m.de.rooy, paul.poulain |
| Version: | 3.6 | ||
| Hardware: | All | ||
| OS: | All | ||
| Change sponsored?: | --- | Patch complexity: | --- |
| Documentation contact: | Documentation submission: | ||
| Text to go in the release notes: | Version(s) released in: | ||
| Circulation function: | |||
| Attachments: |
Patch
Signed off patch Unit tests for _GetCircControlBranch Unit tests for _GetCircControlBranch Bug 6137: unit tests for C4::Circulation::_GetCircControlBranch |
||
|
Description
Julian Maurice
2011-04-08 09:19:42 UTC
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 |