[aur-dev] [PATCH v6 1/3] Add user set timezones

[Lukas Fleischer]

On Thu, 19 Jan 2017 at 04:23:37, 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.
> 
> Signed-off-by: Mark Weiman <mark.weiman at markzz.com>
> ---
> [...]

Thanks, we're almost there. There are several minor issues left, though.
See below.

> diff --git a/web/lib/aur.inc.php b/web/lib/aur.inc.php
> index 9015ae8..8a0ed2b 100644
> --- a/web/lib/aur.inc.php
> +++ b/web/lib/aur.inc.php
> [...]
> @@ -356,7 +359,7 @@ function uid_from_sid($sid="") {
>  function html_header($title="", $details=array()) {
>         global $LANG;
>         global $SUPPORTED_LANGS;
> -
> +       

Unrelated whitespace change?

>         include('header.php');
>         return;
>  }
> [...]
> diff --git a/web/lib/timezone.inc.php b/web/lib/timezone.inc.php
> new file mode 100644
> index 0000000..f3b4c71
> --- /dev/null
> +++ b/web/lib/timezone.inc.php
> @@ -0,0 +1,65 @@
> [...]
> +/**
> + * Set the $TZ global variable.

The comment needs to be updated to reflect the changes in this reroll of
the patch series.

> + *
> + * @return null
> + */
> +function set_tz() {
> +       $timezones = generate_timezone_list();
> +       $update_cookie = false;
> +
> +       if (isset($_COOKIE["AURTZ"])) {
> +               $TZ = $_COOKIE["AURTZ"];

It is quite confusing to use an upper case variable name here enough
though this no longer is a global variable. Something like $timezone
might be more appropriate.

> +       } elseif (isset($_COOKIE["AURSID"])) {
> +               $dbh = DB::connect();
> +               $q = "SELECT Timezone FROM Users, Sessions ";
> +               $q .= "WHERE Users.ID = Sessions.UsersID ";
> +               $q .= "AND Sessions.SessionID = ";
> +               $q .= $dbh->quote($_COOKIE["AURSID"]);
> +               $result = $dbh->query($q);
> +
> +               if ($result) {
> +                       $row = $result->fetchAll();
> +                       $TZ = $row[0][0];

As I mentioned in an earlier review, this can be simplified as

    $timezone = $result->fetchColumn(0);

already assuming that $TZ becomes $timezone.

> +               } else {
> +                       $TZ = config_get("options", "default_timezone");

This else-branch is not needed. The default case is already handled
below.

> +               }
> +
> +               $update_cookie = true;
> +       }
> +
> +       if (isset($TZ) && array_key_exists($TZ, $timezones)) {
> +               date_default_timezone_set($TZ);
> +       } else {
> +               $TZ = config_get("options", "default_timezone");
> +               date_default_timezone_set($TZ);
> +       }

As I also mentioned in an earlier review, this can be simplified as
follows (also assuming that $TZ is replaced by $timezone):

    if (!isset($timezone) || !array_key_exists($timezone, $timezones)) {
        $timezone = config_get("options", "default_timezone");
    }
    date_default_timezone_set($timezone);

> +
> +       if ($update_cookie) {
> +               $timeout = intval(config_get("options", "persistent_cookie_timeout"));
> +               $cookie_time = time() + $timeout;
> +               setcookie("AURTZ", $TZ, $cookie_time, "/");
> +       }
> +}
> diff --git a/web/template/account_edit_form.php b/web/template/account_edit_form.php
> index 19821a0..8b207e7 100644
> --- a/web/template/account_edit_form.php
> +++ b/web/template/account_edit_form.php
> @@ -129,6 +129,24 @@
>  ?>
>                         </select>
>                 </p>
> +               <p>
> +                       <label for="id_timezone"><?= __("Timezone") ?></label>
> +                       <select name="TZ" id="id_timezone">
> +<?php
> +       $timezones = generate_timezone_list();
> +       while (list($key, $val) = each($timezones)) {
> +           if ($TZ == "") {
> +               $TZ = config_get("options", "default_timezone");
> +        }

Broken indentation. I also think this if-block belongs to
display_account_form() and should be executed before
account_edit_form.php is included.

> +               if ($TZ == $key) {
> +                       print "<option value=\"".$key."\" selected=\"selected\"> ".$val."</option>\n";
> +               } else {
> +                       print "<option value=\"".$key."\"> ".$val."</option>\n";
> +               }
> +       }
> +?>
> +                       </select>
> +               </p>
>         </fieldset>
>  
>         <fieldset>
> [...]

Looks good otherwise!