Bug 20123

Summary: Allow multiple instances of Koha to have different timezones on the same server
Product: Koha Reporter: Kyle M Hall <kyle>
Component: Architecture, internals, and plumbingAssignee: Kyle M Hall <kyle>
Status: CLOSED FIXED QA Contact: Tomás Cohen Arazi <tomascohen>
Severity: new feature    
Priority: P5 - low CC: bob, dilan, jonathan.druart, katrin.fischer, mtj, mtompset, nick, satish, tomascohen
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Koha now has the ability to set times zones in Koha on a per-instance basis. That means that a single Koha server can support instances in several time zones simultaneously! Each instance's timezone can be set in its' koha-conf.xml See https://wiki.koha-community.org/wiki/Time_Zone_Configuration for more details.
Version(s) released in:
Bug Depends on:    
Bug Blocks: 18674, 20525    
Attachments: Bug 20123 - Allow multiple instances of Koha to have different timezones on the same server
Bug 20123: Allow multiple instances of Koha to have different timezones on the same server
Bug 20123: Allow multiple instances of Koha to have different timezones on the same server
Bug 20123: Allow multiple instances of Koha to have different timezones on the same server
Bug 20123: Allow multiple instances of Koha to have different timezones on the same server
Bug 20123: Allow multiple instances of Koha to have different timezones on the same server
Bug 20123 [QA Followup]: If tz is 'local', no need to tell database to set the timezone
Bug 20123: Add commented out example to koha-conf.xml
Bug 20123: Add commented out example to koha-conf.xml
Bug 20123: Allow multiple instances of Koha to have different timezones on the same server
Bug 20123: (QA follow-up) If tz is 'local', no need to tell database to set the timezone
Bug 20123: Add commented out example to koha-conf.xml
Bug 20123: Make the timezone entry empty and uncommented by default
Bug 20123: (follow-up) Mock timezone in t/Auth_with_shibboleth.t

Description Kyle M Hall 2018-02-01 18:21:34 UTC
Most of Koha depends on the local timezone of the server, except for Koha::Database which support an ENV override with the key TZ.

We should take this a step further. We should not only accept the TZ environment variable for all of Koha, we should really be able to set the timezone in the koha conf file as well so we don't have to pass that environment variable to things like cronjobs and one-off scripts.
Comment 1 Kyle M Hall 2018-02-01 18:43:46 UTC
Created attachment 71125 [details] [review]
Bug 20123 - Allow multiple instances of Koha to have different timezones on the same server

Most of Koha depends on the local timezone of the server, except for Koha::Database which support an ENV override with the key TZ.

We should take this a step further. We should not only accept the TZ environment variable for all of Koha, we should really be able to set the timezone in the koha conf file as well so we don't have to pass that environment variable to things like cronjobs and one-off scripts.

Test Plan:
1) Apply this patch
2) Set a timzone in your koha_conf file, that is *not* your local time zone
   Available timzones are listed here: https://en.wikipedia.org/wiki/List_of_tz_database_time_zones
3) Restart apache/plack
4) Perform some actions, check the timestamps in the database and in the
   html output, note they are for the set timezone and not the local
   timezone.
Comment 2 Mark Tompsett 2018-02-14 05:22:42 UTC
This adds a new function to C4::Context with no test coverage. Our current RM will Fail QA for this. Write some tests please.
Comment 3 Kyle M Hall 2018-02-15 13:52:39 UTC
(In reply to M. Tompsett from comment #2)
> This adds a new function to C4::Context with no test coverage. Our current
> RM will Fail QA for this. Write some tests please.

Good point, I'll get those written!
Comment 4 Kyle M Hall 2018-02-16 17:55:38 UTC
Created attachment 71839 [details] [review]
Bug 20123: Allow multiple instances of Koha to have different timezones on the same server

Most of Koha depends on the local timezone of the server, except for Koha::Database which support an ENV override with the key TZ.

We should take this a step further. We should not only accept the TZ environment variable for all of Koha, we should really be able to set the timezone in the koha conf file as well so we don't have to pass that environment variable to things like cronjobs and one-off scripts.

Test Plan:
1) Apply this patch
2) Set a timzone in your koha_conf file, that is *not* your local time zone
   Available timzones are listed here: https://en.wikipedia.org/wiki/List_of_tz_database_time_zones
3) Restart apache/plack
4) Perform some actions, check the timestamps in the database and in the
   html output, note they are for the set timezone and not the local
   timezone.
5) prove t/timezones.t
Comment 5 Kyle M Hall 2018-02-16 18:01:53 UTC
Created attachment 71840 [details] [review]
Bug 20123: Allow multiple instances of Koha to have different timezones on the same server

Most of Koha depends on the local timezone of the server, except for Koha::Database which support an ENV override with the key TZ.

We should take this a step further. We should not only accept the TZ environment variable for all of Koha, we should really be able to set the timezone in the koha conf file as well so we don't have to pass that environment variable to things like cronjobs and one-off scripts.

Test Plan:
1) Apply this patch
2) Set a timzone in your koha_conf file, that is *not* your local time zone
   Available timzones are listed here: https://en.wikipedia.org/wiki/List_of_tz_database_time_zones
3) Restart apache/plack
4) Perform some actions, check the timestamps in the database and in the
   html output, note they are for the set timezone and not the local
   timezone.
5) prove t/timezones.t
Comment 6 Dilan Johnpullé 2018-02-22 03:09:13 UTC
I'm having some trouble applying this patch in my devbox. My terminal output is as follows:

vagrant@kohadevbox:kohaclone((673a40d...))$ git bz apply -s 20123
Bug 20123 - Allow multiple instances of Koha to have different timezones on the same server

71840 - Bug 20123: Allow multiple instances of Koha to have different timezones on the same server

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 20123: Allow multiple instances of Koha to have different timezones on the same server
Using index info to reconstruct a base tree...
M	Koha/Database.pm
Falling back to patching base and 3-way merge...
Auto-merging Koha/Database.pm
CONFLICT (content): Merge conflict in Koha/Database.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 20123: Allow multiple instances of Koha to have different timezones on the same server
The copy of the patch that failed is found in:
   /home/vagrant/kohaclone/.git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-20123-Allow-multiple-instances-of-Koha-to-have-x5v_At.patch
Comment 7 Kyle M Hall 2018-02-22 11:10:38 UTC
Created attachment 72080 [details] [review]
Bug 20123: Allow multiple instances of Koha to have different timezones on the same server

Most of Koha depends on the local timezone of the server, except for Koha::Database which support an ENV override with the key TZ.

We should take this a step further. We should not only accept the TZ environment variable for all of Koha, we should really be able to set the timezone in the koha conf file as well so we don't have to pass that environment variable to things like cronjobs and one-off scripts.

Test Plan:
1) Apply this patch
2) Set a timzone in your koha_conf file, that is *not* your local time zone
   Available timzones are listed here: https://en.wikipedia.org/wiki/List_of_tz_database_time_zones
3) Restart apache/plack
4) Perform some actions, check the timestamps in the database and in the
   html output, note they are for the set timezone and not the local
   timezone.
Comment 8 Kyle M Hall 2018-02-22 11:11:18 UTC
(In reply to Dilan Johnpullé from comment #6)
> I'm having some trouble applying this patch in my devbox. My terminal output
> is as follows:

Please give it a try now! The patch just needed to be rebased!
Comment 9 Kyle M Hall 2018-02-22 11:12:51 UTC
Created attachment 72081 [details] [review]
Bug 20123: Allow multiple instances of Koha to have different timezones on the same server

Most of Koha depends on the local timezone of the server, except for Koha::Database which support an ENV override with the key TZ.

We should take this a step further. We should not only accept the TZ environment variable for all of Koha, we should really be able to set the timezone in the koha conf file as well so we don't have to pass that environment variable to things like cronjobs and one-off scripts.

Test Plan:
1) Apply this patch
2) Set a timzone in your koha_conf file, that is *not* your local time zone
   Available timzones are listed here: https://en.wikipedia.org/wiki/List_of_tz_database_time_zones
3) Restart apache/plack
4) Perform some actions, check the timestamps in the database and in the
   html output, note they are for the set timezone and not the local
   timezone.
Comment 10 Dilan Johnpullé 2018-02-26 00:06:10 UTC
Hi Kyle I'm still running into errors when applying the new patch. Looks like whitespace. Terminal output as follows:

vagrant@kohadevbox:kohaclone((673a40d...))$ git bz apply -s 20123
Bug 20123 - Allow multiple instances of Koha to have different timezones on the same server

72081 - Bug 20123: Allow multiple instances of Koha to have different timezones on the same server

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 20123: Allow multiple instances of Koha to have different timezones on the same server
Using index info to reconstruct a base tree...
<stdin>:88: new blank line at EOF.
+
warning: 1 line adds whitespace errors.
Falling back to patching base and 3-way merge...
error: The following untracked working tree files would be overwritten by merge:
	t/timezones.t
Please move or remove them before you can merge.
Aborting
Failed to merge in the changes.
Patch failed at 0001 Bug 20123: Allow multiple instances of Koha to have different timezones on the same server
The copy of the patch that failed is found in:
   /home/vagrant/kohaclone/.git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-20123-Allow-multiple-instances-of-Koha-to-have-AyegrT.patch
Comment 11 Mark Tompsett 2018-02-26 11:50:33 UTC
Created attachment 72202 [details] [review]
Bug 20123: Allow multiple instances of Koha to have different timezones on the same server

Most of Koha depends on the local timezone of the server, except for Koha::Database which support an ENV override with the key TZ.

We should take this a step further. We should not only accept the TZ environment variable for all of Koha, we should really be able to set the timezone in the koha conf file as well so we don't have to pass that environment variable to things like cronjobs and one-off scripts.

Test Plan:
1) Apply this patch
2) Set a timzone in your koha_conf file, that is *not* your local time zone
   Available timzones are listed here: https://en.wikipedia.org/wiki/List_of_tz_database_time_zones
3) Restart apache/plack
4) Perform some actions, check the timestamps in the database and in the
   html output, note they are for the set timezone and not the local
   timezone.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 12 Mark Tompsett 2018-02-26 14:07:25 UTC
Just ran a web installer upgrade... :(

[Mon Feb 26 13:53:31 2018] updatedatabase.pl: DBD::mysql::db do failed: Unknown or incorrect time zone: 'local' [for Statement "SET time_zone = "local""] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1438.

I think MySQL = 'System', and PostGreSql might be 'Default', but I'm not sure.
Comment 13 Jonathan Druart 2018-02-26 14:25:27 UTC
And what about other timezone problems? Like running cronjobs?
Comment 14 Mark Tompsett 2018-02-26 18:47:47 UTC
(In reply to Jonathan Druart from comment #13)
> And what about other timezone problems? Like running cronjobs?

By setting the timezone in the config file, since each instance gets a config file, the timezone setting would be based on the koha-conf.xml file where you've added <timezone>foo/bar</timezone> in. This is the change necessary for the cronjobs, because environment variables aren't generally set, except for the web interface with the apache SetEnv. And plack is a pain, as you have manually start it with the TZ= setting. This is why config overrides env overrides default local time. At least in my understanding. The Koha/Database tweak uses the C4/Context tweak which reads the config file. So, I don't think cronjobs will be an issue. The issue, for which I leave it Failed QA, is 'local' is not valid for MySQL.
Comment 15 Jonathan Druart 2018-02-26 19:41:48 UTC
Imagine you host 2 libraries on the same server, UTC and UTC+12
You want to send emails at 8am. It will be 8am in the given timezone, so 2 cronjobs.
We need our cronjobs to take a repeatable --library option to make it works in the different timezone correctly.

overdue_notices.pl already handles it, not process_message_queue.pl.

We will need to provide a documentation page on the wiki to raise the different problems and explain how we could configure such installation.
Comment 16 Katrin Fischer 2018-02-26 23:27:26 UTC
(In reply to Jonathan Druart from comment #15)
> Imagine you host 2 libraries on the same server, UTC and UTC+12
> You want to send emails at 8am. It will be 8am in the given timezone, so 2
> cronjobs.
> We need our cronjobs to take a repeatable --library option to make it works
> in the different timezone correctly.
> 
> overdue_notices.pl already handles it, not process_message_queue.pl.
> 
> We will need to provide a documentation page on the wiki to raise the
> different problems and explain how we could configure such installation.

I am not sure if it would be necessary to change the cronjobs (might still be nice for other reasons). I think instead of using koha-foreach, you could schedule the cronjobs individually per instance.
Comment 17 Mark Tompsett 2018-02-27 00:23:56 UTC
Ah... you desire to run cronjobs at the appointed times but in the correct time zone for each instance. I assumed it didn't matter what time things were run, as long as order was maintained.

That becomes more interesting, as koha-common in daily.d is run around 7:35am local time, and triggering everything with an offset, particularly if it is earlier, is going to be interesting.
Comment 18 Mark Tompsett 2018-02-27 00:27:09 UTC
(In reply to Katrin Fischer from comment #16)
> I am not sure if it would be necessary to change the cronjobs (might still
> be nice for other reasons). I think instead of using koha-foreach, you could
> schedule the cronjobs individually per instance.

It may be necessary, but that wouldn't be because it prevents multiple instances from having different timezones. It would be because the expected timing of things would be different. As such, perhaps that is beyond the scope of this patch which adds the capability, but not necessarily corrects all the cronjobs to account for it. It's not a fatal error, and perhaps it could be corrected in another bug report?
Comment 19 Tomás Cohen Arazi 2018-02-27 00:49:41 UTC
Having a new multi-timezone-aware cron tool might be out of the scope of this bug, LOL.

It's an interesting idea, tough. I've been looking at ways of using Minion for the tasks BTW.
Comment 20 Katrin Fischer 2018-02-27 06:40:11 UTC
Am I missing something? Why not just run fines for one instance at another time than for another? (scheduling 2 jobs that is running at a suitable calculated time for each). And yep, fines for example you don't want to run in the middle of the day :)
Comment 21 Kyle M Hall 2018-02-27 12:12:00 UTC
Created attachment 72255 [details] [review]
Bug 20123 [QA Followup]: If tz is 'local', no need to tell database to set the timezone
Comment 22 Mark Tompsett 2018-02-27 19:27:30 UTC
Please add something, even commented into koha-conf and koha-conf.in template files for the <timezone> tag.

"America/Toronto" generated noise on an upgrade. Perhaps more clarity on what needs to go in there?
Comment 23 Mark Tompsett 2018-02-27 19:28:24 UTC
(In reply to Tomás Cohen Arazi from comment #19)
> Having a new multi-timezone-aware cron tool might be out of the scope of
> this bug, LOL.

I agree. That is out of scope. +1 for the idea, but let's try to keep scopes small and easy to measure and test.


> It's an interesting idea, tough. I've been looking at ways of using Minion
> for the tasks BTW.

Links?
Comment 24 Jonathan Druart 2018-02-27 19:49:23 UTC
I was asking questions and clarifications about how it could be setup on a server (I suggested a wiki page). I think it is good to ask questions before things get integrated. Maybe the authors of the patches did not think about everything.

I did not ask anything else.
Comment 25 Katrin Fischer 2018-02-27 22:32:04 UTC
+1 for asking questions and documentation - we have too many things hidden away already.
Comment 26 Kyle M Hall 2018-03-09 14:26:29 UTC
Created attachment 72603 [details] [review]
Bug 20123: Add commented out example to koha-conf.xml
Comment 27 Kyle M Hall 2018-03-09 14:29:21 UTC
Created attachment 72605 [details] [review]
Bug 20123: Add commented out example to koha-conf.xml
Comment 28 Kyle M Hall 2018-03-09 14:30:04 UTC
Added followup to document the feature in the koha conf. Is there anything else still needed for QA?
Comment 29 Katrin Fischer 2018-04-01 11:34:04 UTC
I like the idea of a wiki page with information on how this will work and where the limits might be. Do you need to change MySQL configuration as well or is the koha-conf variable enough?
Comment 30 Tomás Cohen Arazi 2018-04-01 13:48:38 UTC
(In reply to Katrin Fischer from comment #29)
> I like the idea of a wiki page with information on how this will work and
> where the limits might be. Do you need to change MySQL configuration as well
> or is the koha-conf variable enough?

Only koha-conf.xml! Isn't that beautiful?
Comment 31 Kyle M Hall 2018-04-03 12:53:24 UTC
(In reply to Katrin Fischer from comment #29)
> I like the idea of a wiki page with information on how this will work and
> where the limits might be. Do you need to change MySQL configuration as well
> or is the koha-conf variable enough?

As Tomas wrote, the koha conf is the only place the timezone will need set. Any entry in the Time Zone Database can be used.

It would be a pretty short wiki page, but I can put it up if you want it!
Comment 32 Katrin Fischer 2018-04-03 12:57:07 UTC
Maybe a note that cronjobs might need to be set up to run at different times for each instance?
Comment 33 Kyle M Hall 2018-04-03 13:04:50 UTC
(In reply to Katrin Fischer from comment #32)
> Maybe a note that cronjobs might need to be set up to run at different times
> for each instance?

Where would you like that note to be?
Comment 34 Katrin Fischer 2018-04-03 13:06:08 UTC
I was thinking on the wiki page :)
Comment 35 Kyle M Hall 2018-04-04 11:32:10 UTC
(In reply to Katrin Fischer from comment #34)
> I was thinking on the wiki page :)

Sounds good! Here it is: https://wiki.koha-community.org/wiki/Time_Zone_Configuration
Comment 36 Katrin Fischer 2018-04-05 05:56:20 UTC
Thx Kyle! Do we still need QA review here? Tomas, maybe you could do it?
Comment 37 Tomás Cohen Arazi 2018-04-05 13:32:36 UTC
Created attachment 73687 [details] [review]
Bug 20123: Allow multiple instances of Koha to have different timezones on the same server

Most of Koha depends on the local timezone of the server, except for Koha::Database which support an ENV override with the key TZ.

We should take this a step further. We should not only accept the TZ environment variable for all of Koha, we should really be able to set the timezone in the koha conf file as well so we don't have to pass that environment variable to things like cronjobs and one-off scripts.

Test Plan:
1) Apply this patch
2) Set a timzone in your koha_conf file, that is *not* your local time zone
   Available timzones are listed here: https://en.wikipedia.org/wiki/List_of_tz_database_time_zones
3) Restart apache/plack
4) Perform some actions, check the timestamps in the database and in the
   html output, note they are for the set timezone and not the local
   timezone.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 38 Tomás Cohen Arazi 2018-04-05 13:32:48 UTC
Created attachment 73688 [details] [review]
Bug 20123: (QA follow-up) If tz is 'local', no need to tell database to set the timezone

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 39 Tomás Cohen Arazi 2018-04-05 13:32:56 UTC
Created attachment 73689 [details] [review]
Bug 20123: Add commented out example to koha-conf.xml

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 40 Tomás Cohen Arazi 2018-04-05 13:33:38 UTC
Created attachment 73690 [details] [review]
Bug 20123: Make the timezone entry empty and uncommented by default

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 41 Jonathan Druart 2018-04-06 16:24:51 UTC
Me again, I would add to the wiki page the consequences on log timestamps and also that koha-foreach should no longer be used (i.e. lot of cronjobs will have to be adjusted).
Comment 42 Tomás Cohen Arazi 2018-04-06 18:16:23 UTC
(In reply to Jonathan Druart from comment #41)
> Me again, I would add to the wiki page the consequences on log timestamps
> and also that koha-foreach should no longer be used (i.e. lot of cronjobs
> will have to be adjusted).

I don't think log timestamps will be affected, because the daemonized starman process doesn't know about this change.

I think we should add (yet) another option switch to koha-foreach:

--timezone

:-D
Comment 43 Kyle M Hall 2018-04-09 11:36:24 UTC
(In reply to Tomás Cohen Arazi from comment #42)
> (In reply to Jonathan Druart from comment #41)
> > Me again, I would add to the wiki page the consequences on log timestamps
> > and also that koha-foreach should no longer be used (i.e. lot of cronjobs
> > will have to be adjusted).
> 
> I don't think log timestamps will be affected, because the daemonized
> starman process doesn't know about this change.
> 
> I think we should add (yet) another option switch to koha-foreach:
> 
> --timezone
> 
> :-D

I think log timestamps having the local time works just fine, we can always add a 'converted' time option later.

As for koha-foreach, I like your idea!
Comment 44 Jonathan Druart 2018-04-11 19:54:37 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 45 Jonathan Druart 2018-04-11 20:34:26 UTC
Koha is awesome, you broke shibboleth tests, congratulations:

t/Auth_with_shibboleth.t .. 1/11 
    #   Failed test 'new user added with no debug'
    #   at t/Auth_with_shibboleth.t line 204.
    # found warning: Invalid timezone in koha-conf.xml (HASH(0x7ec4188)) at /home/vagrant/kohaclone/C4/Context.pm line 986.
    # didn't expect to find a warning
    # Looks like you failed 1 test of 18.

#   Failed test 'checkpw_shib tests'
#   at t/Auth_with_shibboleth.t line 248.
Comment 46 Tomás Cohen Arazi 2018-04-11 21:09:25 UTC
Created attachment 74043 [details] [review]
Bug 20123: (follow-up) Mock timezone in t/Auth_with_shibboleth.t

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 47 Jonathan Druart 2018-04-11 21:23:05 UTC
follow-up pushed to master, thanks Tomas!
Comment 48 Nick Clemens 2018-05-14 19:26:36 UTC
New feature, not backported for 17.11.x