On Tue, 15 Nov 2016 at 06:19:26, Mark Weiman wrote:
Currently, aurweb displays all dates and times in UTC time. This patch adds a capability for each logged in user to set their preferred timezone.
Implements: FS#48729
Signed-off-by: Mark Weiman <mark.weiman@markzz.com> --- conf/config.proto | 1 + schema/aur-schema.sql | 1 + upgrading/4.5.0.txt | 5 +++ web/html/account.php | 3 ++ web/html/register.php | 4 ++- web/html/voters.php | 2 +- web/lib/acctfuncs.inc.php | 16 ++++++--- web/lib/aur.inc.php | 4 +++ web/lib/pkgreqfuncs.inc.php | 2 +- web/lib/timezone.inc.php | 69 ++++++++++++++++++++++++++++++++++++ web/template/account_edit_form.php | 15 ++++++++ web/template/flag_comment.php | 2 +- web/template/pkg_comments.php | 6 ++-- web/template/pkg_details.php | 6 ++-- web/template/pkgbase_details.php | 6 ++-- web/template/pkgreq_results.php | 2 +- web/template/stats/updates_table.php | 2 +- web/template/tu_details.php | 4 +-- web/template/tu_list.php | 4 +-- 19 files changed, 130 insertions(+), 24 deletions(-) create mode 100644 upgrading/4.5.0.txt create mode 100644 web/lib/timezone.inc.php
First of all, thanks for taking the time to implement all this! I added some inline comments below.
diff --git a/conf/config.proto b/conf/config.proto index 96fad80..b6a414d 100644 --- a/conf/config.proto +++ b/conf/config.proto @@ -11,6 +11,7 @@ username_min_len = 3 username_max_len = 16 passwd_min_len = 4 default_lang = en +deafult_timezone = UTC
Typo?
sql_debug = 0 max_sessions_per_user = 8 login_timeout = 7200 diff --git a/schema/aur-schema.sql b/schema/aur-schema.sql index 30209bd..95a4373 100644 --- a/schema/aur-schema.sql +++ b/schema/aur-schema.sql @@ -32,6 +32,7 @@ CREATE TABLE Users ( ResetKey CHAR(32) NOT NULL DEFAULT '', RealName VARCHAR(64) NOT NULL DEFAULT '', LangPreference VARCHAR(6) NOT NULL DEFAULT 'en', + Timezone VARCHAR(50) NOT NULL DEFAULT 'UTC',
Out of curiosity: How did you choose the length of 50 characters? Is it large enough according to some standard?
Homepage TEXT NULL DEFAULT NULL, IRCNick VARCHAR(32) NOT NULL DEFAULT '', PGPKey VARCHAR(40) NULL DEFAULT NULL, diff --git a/upgrading/4.5.0.txt b/upgrading/4.5.0.txt new file mode 100644 index 0000000..a9c0220 --- /dev/null +++ b/upgrading/4.5.0.txt @@ -0,0 +1,5 @@ +1. Add Timezone column to Users
This is really nitpicky but we usually add colons after these lines.
+ +--- +ALTER TABLE Users ADD COLUMN Timezone VARCHAR(50) NOT NULL DEFAULT 'UTC'; +--- \ No newline at end of file diff --git a/web/html/account.php b/web/html/account.php index 2892f04..91e5703 100644 --- a/web/html/account.php +++ b/web/html/account.php @@ -34,6 +34,7 @@ if ($action == "UpdateAccount") { in_request("U"), in_request("T"), in_request("S"), in_request("E"), in_request("H"), in_request("P"), in_request("C"), in_request("R"), in_request("L"), + in_request("TZ"),
Okay. Looks a bit weird but I agree that doing this is better than reformatting the whole list in every patch that adds or removes a parameter. Maybe we should change the formatting to "one parameter per line" here. Not in the scope of this patch, though.
[...] @@ -64,7 +66,7 @@ if (in_request("Action") == "NewAccount") { } } else { print '<p>' . __("Use this form to create an account.") . '</p>'; - display_account_form("NewAccount", "", "", "", "", "", "", "", "", $LANG); + display_account_form("NewAccount", "", "", "", "", "", "", "", "", $LANG, "UTC");
Should this refer to the default timezone from the configuration file instead?
[...] @@ -359,6 +362,9 @@ function process_account_form($TYPE,$A,$U="",$T="",$S="",$E="",$H="",$P="",$C=""
$ssh_key_result = account_set_ssh_keys($UID, $ssh_keys, $ssh_fingerprints);
+ error_log($result ? "t" : "f"); + error_log($ssh_key_result ? "t" : "f"); + error_log($q);
Huh? Leftover debug statements? I think these should go...
[...] +function set_tz() { [...] + if (!array_key_exists($TZ, $timezones)) { + error_log("Am I Here?");
And another one...
[...] + * Add the selected timezone's offset to a timestamp. + * + * @param $timestamp int Timestamp to be manipulated + * + * @return int Manipulated timestamp + */ +function tz_timestamp($timestamp) { + global $TZ; + $offset = isset($TZ) ? timezone_offset_get(new DateTimeZone($TZ), new DateTime()) : 0; + return $timestamp + $offset; +} [...]
I do not know about timezone handling in PHP, so this question might be naive: Is there any reason you chose this implementation instead of calling something like date_default_timezone_set() in set_tz() and using the default date/time functions without any wrappers? Regards, Lukas P.S.: Not a big issue but if you resubmit a patch several times, it is usually a good idea to add a comment on what changed between the "---" marker and the diff stat. Just to make it easier for reviewers to pick a version to review and to know about previously addressed stuff.