Created attachment 24243 [details] [review] proposed patch
Created attachment 24244 [details] [review] bug 11537: Increasing test coverage for C4::Log To test: 1. run prove t/Log.t Notice 1 test is run 2. apply patch 3. run prove t/Log.t Notice 3 tests are run Signed-off-by: Liz Rea <liz@catalyst.net.nz> 3 tests are in fact run. Thanks for the patch!
Comment on attachment 24244 [details] [review] bug 11537: Increasing test coverage for C4::Log Removing, as signoff has to be outside catalyst.
Created attachment 24245 [details] [review] bug 11537: Increasing test coverage for C4::Log To test: 1. run prove t/Log.t Notice 1 test is run 2. apply patch 3. run prove t/Log.t Notice 3 tests are run Signed-off-by: David Cook <dcook@prosentient.com.au> Works as described. I think there might be a typo in "Testing git log status". Should this read "Testing get log status"? (Noticed while looking at the code/running perl t/Log.t.) In any case, thanks for the patch. Looks good!
Created attachment 24246 [details] [review] bug 11537: Increasing test coverage for C4::Log To test: 1. run prove t/Log.t Notice 1 test is run 2. apply patch 3. run prove t/Log.t Notice 3 tests are run Signed-off-by: David Cook <dcook@prosentient.com.au> Works as described. I think there might be a typo in "Testing git log status". Should this read "Testing get log status"? (Noticed while looking at the code/running perl t/Log.t.) In any case, thanks for the patch. Looks good! Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Fixed typo, thanks David
Created attachment 24268 [details] [review] [PASSED QA] Bug 11537: Increasing test coverage for C4::Log To test: 1. run prove t/Log.t Notice 1 test is run 2. apply patch 3. run prove t/Log.t Notice 3 tests are run Signed-off-by: David Cook <dcook@prosentient.com.au> Works as described. I think there might be a typo in "Testing git log status". Should this read "Testing get log status"? (Noticed while looking at the code/running perl t/Log.t.) In any case, thanks for the patch. Looks good! Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Fixed typo, thanks David Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Works as described: 1..3 ok 1 - use C4::Log; ok 2 - Testing GetLogStatus ok 3 - Testing hash is non empty
Welcome Alex!
Pushed to master. Congratulations on having your first patch accepted into Koha, Alex!
Upon Jenkins (the continuous integration service) complaining, I've also pushed a follow-up that mocks the calls to C4::Context::preference() that C4::Log::GetLogStatus() makes, which will allow the test to pass without requiring that a Koha database is present and pointed to by a valid Koha configuration.