|
Description
Kyle M Hall (khall)
2018-02-01 18:21:34 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. This adds a new function to C4::Context with no test coverage. Our current RM will Fail QA for this. Write some tests please. (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! 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 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 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 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. (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! 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. 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 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> 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. And what about other timezone problems? Like running cronjobs? (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. 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. (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. 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. (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? 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. 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 :) Created attachment 72255 [details] [review] Bug 20123 [QA Followup]: If tz is 'local', no need to tell database to set the timezone 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? (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? 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. +1 for asking questions and documentation - we have too many things hidden away already. Created attachment 72603 [details] [review] Bug 20123: Add commented out example to koha-conf.xml Created attachment 72605 [details] [review] Bug 20123: Add commented out example to koha-conf.xml Added followup to document the feature in the koha conf. Is there anything else still needed for QA? 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? (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? (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! Maybe a note that cronjobs might need to be set up to run at different times for each instance? (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? I was thinking on the wiki page :) (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 Thx Kyle! Do we still need QA review here? Tomas, maybe you could do it? 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> 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> Created attachment 73689 [details] [review] Bug 20123: Add commented out example to koha-conf.xml Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> 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> 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). (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 (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! Pushed to master for 18.05, thanks to everybody involved! 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.
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> follow-up pushed to master, thanks Tomas! New feature, not backported for 17.11.x |