[aur-dev] [PATCH v2] Add user set timezones

[Lukas Fleischer]

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 at 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.