This bug report requires the removal of SQL queries out of the admin/env_tz_test.pl script into perl module(s) in the Koha or C4 directory. It also requires POD and unit tests for the new subroutines in the perl module(s).
Created attachment 61238 [details] [review] Bug 18299 - Moved the SQL queries from admin/env_tz_test.pl into the C4/Context.pm in two new subroutines. Both new subroutines have a successful subtest in the t/db_dependent/Context.t unit test in addition to POD.
Test Plan: 1. Restart memcached: sudo service memcached restart 2. Drop and recreate the Koha instance database 3. Go through the web installer, selecting all data to be installed in step 3 4. After the web installer is finished log in with the Koha database administrator credentials 5. Create yourself a patron account 6. Set the patron account to have superlibrarian privileges 7. Log out and back in as your newly created patron 8. Navigate to the Administration area 9. Write in the URL: cgi-bin/koha/admin/env_tz_test.pl 10. Notice a page titled with "Dumping ENV". Scroll down and observe the current time printed below the "Checking different TIME elements in the system" title 11. Apply all patches associated with this bug report 12. Repeat steps 1,2,3,4,5,6,7,8,9 13. Again notice the current time is displayed below the "Checking different TIME elements in the system" title 14. In your terminal navigate from your Koha instance root directory to t/db_dependent 15. Enter koha shell: sudo koha-shell <instancename> 16. Run Context.t unit test: prove -v Context.t 17. The tests should run successful 18. Exit the koha shell 19. Navigate back to your Koha instance root directory 20. View admin/env_tz_test.pl script and observe it contains no SQL queries 21. Navigate back to your Koha instance root directory 22. View C4/Context.pm and notice that the get_env_variables and get_current_time subroutines (the subroutines called by env_tz_test.pl to run the SQL queries) contain POD
Problem with test plan step 16. Run Context.t unit test: prove -v Context.t I get: not ok 41 - Correct time # Failed test 'Correct time' # at t/db_dependent/Context.t line 159. # got: '2017-04-20 11:32:32' # expected: '2017-04-20 09:32:32' # Looks like you failed 1 test of 41. (got: is local time here) Time shown by env_tz_test.pl is local time: Checking different TIME elements in the system: perl localime: Thu Apr 20 11:30:31 2017 system(date): Don Apr 20 11:30:31 CEST 2017 mysql dbh (Context) time_zone : SYSTEM mysql dbh (Context) now() : 2017-04-20 11:30:31
Created attachment 62510 [details] [review] Bug 18299 - Implemented new locatime function to compare against the value of the output of the get_current_time() function
(In response to Marc Veron in comment 3) Hi Marc I have changed the function I use to get the localtime value to compare against the output of the get_current_time in the Context.t test in the patch I just applied. I have run the test and it nows says all tests are successful, so if you test again it should work successfully. Cheers Alex
Created attachment 62515 [details] [review] Bug 18299 - Moved the SQL queries from admin/env_tz_test.pl into the C4/Context.pm in two new subroutines. Both new subroutines have a successful subtest in the t/db_dependent/Context.t unit test in addition to POD. Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 62516 [details] [review] Bug 18299 - Implemented new locatime function to compare against the value of the output of the get_current_time() function Signed-off-by: Marc Véron <veron@veron.ch>
I was not aware of this script and I think it does not make sense to have it. I'd prefer to remove it than adding more code to the codebase to maintain it. Opinions?
(In reply to Jonathan Druart from comment #8) > I was not aware of this script and I think it does not make sense to have it. > I'd prefer to remove it than adding more code to the codebase to maintain it. > > Opinions? No strong opinion. But yes, we could do without this script.
Created attachment 62996 [details] [review] Bug 18299 - Removed unused env_tz_test.pl based on user testing feedback
Created attachment 63001 [details] [review] [SIGNED OFF] Bug 18299 - Removed unused env_tz_test.pl based on user testing feedback Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 63042 [details] [review] Bug 18299 - Removed unused env_tz_test.pl based on user testing feedback Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
(In reply to Jonathan Druart from comment #8) > I was not aware of this script and I think it does not make sense to have it. > I'd prefer to remove it than adding more code to the codebase to maintain it. > > Opinions? I was not aware of this script until recently, but I have been using it to double-check that I've set up the timezone for Koha correctly in Apache. I'm OK with removing the script overall, but could we add timezone information to the About page in the "Server information" tab?
Pushed to master for 17.05, thanks Alex!
(In reply to David Cook from comment #13) > (In reply to Jonathan Druart from comment #8) > > I was not aware of this script and I think it does not make sense to have it. > > I'd prefer to remove it than adding more code to the codebase to maintain it. > > > > Opinions? > > I was not aware of this script until recently, but I have been using it to > double-check that I've set up the timezone for Koha correctly in Apache. > > I'm OK with removing the script overall, but could we add timezone > information to the About page in the "Server information" tab? Can you file an enhancement bug for that? I imagine we should be able to make that happen pretty easily.
This won't get ported back to 16.11.x as it is an enhancement.
(In reply to Kyle M Hall from comment #15) > (In reply to David Cook from comment #13) > > (In reply to Jonathan Druart from comment #8) > > > I was not aware of this script and I think it does not make sense to have it. > > > I'd prefer to remove it than adding more code to the codebase to maintain it. > > > > > > Opinions? > > > > I was not aware of this script until recently, but I have been using it to > > double-check that I've set up the timezone for Koha correctly in Apache. > > > > I'm OK with removing the script overall, but could we add timezone > > information to the About page in the "Server information" tab? > > Can you file an enhancement bug for that? I imagine we should be able to > make that happen pretty easily. Sounds good.
(In reply to Kyle M Hall from comment #15) > (In reply to David Cook from comment #13) > > (In reply to Jonathan Druart from comment #8) > > > I was not aware of this script and I think it does not make sense to have it. > > > I'd prefer to remove it than adding more code to the codebase to maintain it. > > > > > > Opinions? > > > > I was not aware of this script until recently, but I have been using it to > > double-check that I've set up the timezone for Koha correctly in Apache. > > > > I'm OK with removing the script overall, but could we add timezone > > information to the About page in the "Server information" tab? > > Can you file an enhancement bug for that? I imagine we should be able to > make that happen pretty easily. I've filed it here: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18674 Although I haven't had time to add a patch myself