Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set Calendar time zone #1179

Merged
merged 7 commits into from Jun 22, 2018
Merged

Conversation

apooravc
Copy link
Contributor

@apooravc apooravc commented Jun 14, 2018

This PR adds a global calendar_timezone which the user could use to select his/her time zone for Calendar date/time functions. date_default_timezone_get (used as default time zone for Calendar) gets the value of date.timezone in php.ini file if it is valid/supported (valid time zones given here) otherwise defaults to UTC. The user selected time zone from Main globals or User specific globals (given more preference when different values) is set as date.timezone configuration option value using ini_set.

date function is used to fill entries in date & start_datetime columns in patient_tracker and patient_tracker_element tables respectively. @teryhill Yet to add other time zones (list here). Should I add all of which are given there? Or go for a particular set covering major regions in time zones?

@teryhill
Copy link
Contributor

look at the demo files at full calendar. get-timezones.php, get-events.php and utils.php.

array(
date_default_timezone_get() => xl('Default - php.ini value'),
'Asia/Dubai' => xl('Dubai'),
'Asia/Kolkata' => xl('Kolkata'),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xlt table entry here is "Calcutta" I suppose, but I guess you would not need that if you live in the time zone! Were these values added for a particular reason? Is there a time-zone gui widget you can grab somewhere?

Copy link
Contributor Author

@apooravc apooravc Jun 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These were sample values added just for testing code initially. Added all valid time zones in second commit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I knew that! :)

@apooravc
Copy link
Contributor Author

apooravc commented Jun 16, 2018

Used iteration through timezone_identifiers_list to create an option list of all valid time zones in the second commit. Also when time zone in php.ini is same as one of the time zones in option list, the Default option disappears, so used ! as an identifier for Default option in such case. Eg: UTC as php.ini value and UTC is also in timezone_identifiers_list; in this case, Default option won't be there in option list if ! is not concatenated also. @teryhill @aethelwulffe Please have a look.

@apooravc
Copy link
Contributor Author

apooravc commented Jun 16, 2018

In the third commit:

  1. Moved global check code from library/patient_tracker.inc.php to interface/globals.php to cover (set) time zone all over EHR and not just Calendar.

  2. Added reference to globals.php from patient_tracker.inc.php file.

Note about ini_set function: "Sets the value of the given configuration option. The configuration option will keep this new value during the script's execution, and will be restored at the script's ending." Referring here to those scripts which will use date functions which depend on time zones. Needs to be tested wherever time is involved. @teryhill Please have a look.

@aethelwulffe
Copy link
Contributor

Good idea. This stuff has been on todo lists for a while. One place for time stuff, and only one place.

@teryhill
Copy link
Contributor

Testing it with the developer mode in the updater. works great.

@apooravc
Copy link
Contributor Author

@teryhill Also, this PR was first intended to be used to set time zone for Calendar only but now it can be applied to whole of EHR. So, should I let global option for setting the time zone remain under Calendar sub-section or should I shift it to somewhere else (and if shifting, then where)?

@teryhill
Copy link
Contributor

Move it to under the Systems tab

@teryhill
Copy link
Contributor

Also look at moving your function from globals.inc.php to edit_globals.php.

@apooravc
Copy link
Contributor Author

apooravc commented Jun 18, 2018

@teryhill The code creating $zone_opt_arr (line86-106) right? Is it causing errors?

@teryhill
Copy link
Contributor

@apooravc it is not causing errors. It is just "Cleaner" to have it outside of globals.

@@ -19,6 +19,7 @@
*
*/
require_once(dirname(__FILE__) . '/appointments.inc.php');
require_once("../interface/globals.php");

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this needed?

Copy link
Contributor Author

@apooravc apooravc Jun 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the code in globals.php uses ini_set to set the user selected time zone in Globals as time zone for the script in which globals.php is included. So, wherever date/time functions, this should be included in script containing such functions.

@teryhill
Copy link
Contributor

It is called in a program that includes this one.

@apooravc
Copy link
Contributor Author

apooravc commented Jun 19, 2018

@teryhill You mean that globals.php is called in edit_globals? Yes, but edit_globals only lets a user save his/her global option which could be then accessed by $GLOBALS['calendar_timezone']. Suppose we're saving an appointment, then patient_tracker.inc.php is executed only and without including globals.php here, the time zone for date function inside manage_tracker_status would not be saved according to user saved option.

@teryhill
Copy link
Contributor

The time zone information is in globals.php, correct. Globals php is a main program that is loaded with the main code . Look at main.php, it is loaded there, look at patient_tracker.php , it is loaded there. patient_tracker.inc is used by patient tracker. Globals is called by the "Top" programs.

@teryhill teryhill mentioned this pull request Jun 19, 2018
@apooravc
Copy link
Contributor Author

@teryhill I looked up main.php and also checked again by making appt. without including globals.php, works correctly.

@apooravc
Copy link
Contributor Author

apooravc commented Jun 20, 2018

Changes done in last two commits:

  1. Removed include globals.php from patient_tracker.inc.php as it is included in main.php itself.
  2. Changed global name from calendar_timezone to ehr_timezone as it now covers time zone all over EHR.
  3. Moved time zone option list from Calendar to System tab in Admin > Globals and also moved code from globals.inc.php to edit_globals.php.

@teryhill Please have a look.

Copy link
Contributor

@aethelwulffe aethelwulffe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am thinking that this will work for whatever conditions are required. It currently works. The idea to default to php.ini settings would be informational, but very often may be incorrect for the user. The real issue is cross-timezone organizations.

@teryhill
Copy link
Contributor

This option is a user specific item also @aethelwulffe so if the user is in another time zone they can set there system for the different zone. Needs to have the / put back in so the locations are easier to determine.

@apooravc
Copy link
Contributor Author

apooravc commented Jun 21, 2018

@teryhill Added back / to options so that user may find it easier to navigate to a time zone.

One more thing I noticed is that after moving global from Calendar to System tab, the time zone option list now only appears in Admin > Globals (Edit Global Settings) > System and not also in User Preferences (Edit User Settings). Back then it used to do in User Preferences (Edit User Settings) > Calendar.

@teryhill
Copy link
Contributor

You need to add the "Systems" tag to the users.

image

@apooravc
Copy link
Contributor Author

@teryhill Thought so, on it.

@apooravc
Copy link
Contributor Author

@teryhill Added System tab to Edit User Settings. Please have a look.

@aethelwulffe
Copy link
Contributor

OK, please tell me that these changes are final, and we can merge this. :)

@apooravc
Copy link
Contributor Author

@aethelwulffe These changes are final.

@aethelwulffe aethelwulffe merged commit d2f4266 into LibreHealthIO:master Jun 22, 2018
@apooravc apooravc deleted the set-timezone branch July 13, 2018 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants