Bug 18299 - Removal of admin/env_tz_test.pl script
Summary: Removal of admin/env_tz_test.pl script
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Alex Buckley
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-03-18 01:31 UTC by Alex Buckley
Modified: 2018-06-04 20:10 UTC (History)
7 users (show)

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


Attachments
Bug 18299 - Moved the SQL queries from admin/env_tz_test.pl into the C4/Context.pm in two new subroutines. (3.21 KB, patch)
2017-03-18 21:11 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18299 - Implemented new locatime function to compare against the value of the output of the get_current_time() function (1.08 KB, patch)
2017-04-21 00:58 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18299 - Moved the SQL queries from admin/env_tz_test.pl into the C4/Context.pm in two new subroutines. (3.34 KB, patch)
2017-04-21 04:57 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18299 - Implemented new locatime function to compare against the value of the output of the get_current_time() function (1.21 KB, patch)
2017-04-21 04:58 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18299 - Removed unused env_tz_test.pl based on user testing feedback (4.98 KB, patch)
2017-05-02 22:06 UTC, Alex Buckley
Details | Diff | Splinter Review
[SIGNED OFF] Bug 18299 - Removed unused env_tz_test.pl based on user testing feedback (5.05 KB, patch)
2017-05-02 22:47 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 18299 - Removed unused env_tz_test.pl based on user testing feedback (5.11 KB, patch)
2017-05-03 18:40 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Buckley 2017-03-18 01:31:17 UTC
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).
Comment 1 Alex Buckley 2017-03-18 21:11:06 UTC
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.
Comment 2 Alex Buckley 2017-03-18 21:33:01 UTC
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
Comment 3 Marc Véron 2017-04-20 09:40:08 UTC
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
Comment 4 Alex Buckley 2017-04-21 00:58:42 UTC
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
Comment 5 Alex Buckley 2017-04-21 01:04:20 UTC
(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
Comment 6 Marc Véron 2017-04-21 04:57:44 UTC
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>
Comment 7 Marc Véron 2017-04-21 04:58:37 UTC
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>
Comment 8 Jonathan Druart 2017-04-24 15:41:07 UTC
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?
Comment 9 Marcel de Rooy 2017-05-01 11:17:26 UTC
(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.
Comment 10 Alex Buckley 2017-05-02 22:06:39 UTC
Created attachment 62996 [details] [review]
Bug 18299 - Removed unused env_tz_test.pl based on user testing feedback
Comment 11 Katrin Fischer 2017-05-02 22:47:06 UTC
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>
Comment 12 Jonathan Druart 2017-05-03 18:40:01 UTC
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>
Comment 13 David Cook 2017-05-04 00:05:38 UTC
(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?
Comment 14 Kyle M Hall 2017-05-08 13:25:10 UTC
Pushed to master for 17.05, thanks Alex!
Comment 15 Kyle M Hall 2017-05-08 13:25:48 UTC
(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.
Comment 16 Katrin Fischer 2017-05-14 09:24:50 UTC
This won't get ported back to 16.11.x as it is an enhancement.
Comment 17 David Cook 2017-05-26 03:55:07 UTC
(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.
Comment 18 David Cook 2017-06-13 01:49:21 UTC
(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