399070 - Remove the 'timezone' parameter

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Frédéric Buclin]

Per our discussions at our last 2 Bugzilla meetings, this param should go away. Bugzilla should use the server timezone.

Comment 1

[Dave Miller [:justdave]]

Or better yet, the end-user's timezone.  But that's another bug. (bug 182238)

We should probably use UTC in the database (that's also another bug - bug 176979).

Comment 2

[Bruce Wilson]

I would suggest that a configurable timezone is important when the server is operated in a different timezone than the primary users.  E.G. if my server is hosted in California, my team in Chicago would rather not have to convert all times by two hours.

End-user's timezone would fix this.

Comment 3

[Paul]

For me, I don't have control over the system time because bugzilla runs on a shared server.  Having control over the bugzilla time is nice.  And it's not so nice because I prefer GMT because bugzilla gets used globally, but the server time is set to EST.

In the case of removal how would bug 182238 allow users to choose their timezone ?  Relative to EST ?  I'd prefer relative to GMT but a conversion from server time to bugzilla time would be necessary for me.

Comment 4

[miketosh]

It would make sense to use the server's timezone on the server side, and allow the users to have their own user-timezone as a preference, with the server's timezone as the default site default.  UTC would be great for me, but it would make more sense to make it per-user.

Comment 5

[Max Kanat-Alexander]

  Yes, we would love to have the time zone be configurable. That's bug 182238.

  Perhaps what some people here are missing is that the current timezone parameter doesn't *do* anything. It just puts some letters and numbers next to every time displayed. It doesn't actually *adjust* the times. You're not losing any functionality by having this parameter go away.

Comment 6

[Teemu Mannermaa (:wicked)]

(In reply to comment #5)
>   Perhaps what some people here are missing is that the current timezone
> parameter doesn't *do* anything. It just puts some letters and numbers next to

Actually, recently I found out that it's not exactly like that. It seems buglist.cgi *does* adjust one of it's timestamp according to specified timezone. Possibly this was the timestamp on the top of the buglist, not sure about changed/opened timestamps.

Comment 7

[Max Kanat-Alexander]

(In reply to comment #6)
> Actually, recently I found out that it's not exactly like that. It seems
> buglist.cgi *does* adjust one of it's timestamp according to specified
> timezone. Possibly this was the timestamp on the top of the buglist, not sure
> about changed/opened timestamps.

  Yeah, I think that's an accident.

Comment 8

[miketosh]

(In reply to comment #5)
>   Yes, we would love to have the time zone be configurable. That's bug 182238.
> 
Sorry, that is what I meant.  Get rid of this param, and have someone work on bug 182238.

Comment 9

[Jeremy Baron]

(In reply to comment #3)
fwiw, you could probably set TZ=GMT in bugzilla's environment and it would "Just Work".

Comment 10

[Frédéric Buclin]

(In reply to comment #4)
> It would make sense to use the server's timezone on the server side, and allow
> the users to have their own user-timezone as a preference, with the server's
> timezone as the default site default.

Yes, that's how I implemented bug 182238. As soon as bug 182238 is fixed, this one should be trivial to fix as the 'timezone' Parameter would no longer be in use.

Comment 11

[Frédéric Buclin]

Bug 182238 is fixed now. Time to kill the 'timezone' parameter.

Comment 12

[Frédéric Buclin]

Attachment: patch, v0.9

The timezone parameter is dead, and all code related to it has been fixed thanks to the great new format_time() function. I also fixed code which was generating dates and times either in the local timezone of the server or incorrect one (the current code for iCalendar and Atom feeds was either broken or displayed dates in the future due to DST). I tested this patch both locally and on landfill.

I also removed explicit calls to Time::Zone and templates no longer rely on [% USE date %] to format dates/times. This way, all dates and times are displayed consistently.

There is one thing left to fix: dates/times displayed in emails related to user account administration (lost password, request new account, change login name) still uses time2str(), ignoring user's timezone. I will probably fix that in a separate bug to keep this patch small.

Comment 13

[Max Kanat-Alexander]

Comment on attachment 335767 [details] [diff] [review]
patch, v0.9

>Index: buglist.cgi
> sub DiffDate {

  At some point we should move this into the template or into a filter.

>     if( $age < 18*60*60 ) {
>-        $date = sprintf "%02d:%02d:%02d", $h,$m,$s;
>+        $date = format_time($datestr, '%H:%M:%S');

  Since format_time does the timezone transitions, are you sure that we won't double-do a timezone transition here?

>Index: Bugzilla/Util.pm
>+        if ($timezone) {
>+            $cache->{'timezones'}->{$timezone} ||= DateTime::TimeZone->new(name => $timezone);

  Are you sure we actually need to cache those? Why not just make the caller send us a TimeZone object?

  In any case, the name should be util_timezones--if we adjust the cache outside of Bugzilla.pm, we always prefix the module name to the item, so that we don't ever get name conflicts.

  Also, where is $cache->{'timezones'} created? I don't see an ||= {} anywhere.

>+"FILTER time" in Templates.pm.

  That should be L<Bugzilla::Template>.

>Index: Bugzilla/WebService/Bugzilla.pm
> sub timezone {

  As a note to myself--we need to add other return values to this, such as tz_name for the full timezone name.

>Index: template/en/default/list/list.atom.tmpl
>+  <updated>[% bugs.nsort('changedtime').last.changedtime FILTER time("%Y-%m-%dT%H:%M:%SZ", "UTC") FILTER xml %]</updated>

  Nit: You could split that onto multiple lines so that it wasn't longer than 80 characters.

  Overall this looks good, though. :-)

Comment 14

[Frédéric Buclin]

(In reply to comment #13)
>   At some point we should move this into the template or into a filter.

It's only used in buglist.cgi. If I find it useful elsewhere, I will move it into Template.pm. But in another bug. :-D


>   Since format_time does the timezone transitions, are you sure that we won't
> double-do a timezone transition here?

Yes, because I pass $datestr, not $date. And $datestr comes directly from the DB, which stores date/time using the local timezone, which is exactly what format_time expects. So this is fine.


>   Are you sure we actually need to cache those? Why not just make the caller
> send us a TimeZone object?

Yes, we really need to cache them. For instance, Atom feeds and iCalendar buglists require times in UTC, and call format_time() 3 times per bug. And I don't see how we could let these templates create a DateTime::TimeZone object for us. The reason I implemented this cache is to let us call format_time() several times from templates without any perf issue (thanks to FILTER time).


>   In any case, the name should be util_timezones--if we adjust the cache
> outside of Bugzilla.pm, we always prefix the module name to the item, so that
> we don't ever get name conflicts.

Ah ok. Will do.


>   Also, where is $cache->{'timezones'} created? I don't see an ||= {} anywhere.

No need. You can create $foo->{bar}->{baz}->{bat} in a row, without all these intermediate ||={} declarations.

Comment 15

[Max Kanat-Alexander]

By the way, I talked to the DateTime author, and he says that creation of timezone objects is not expensive--only the creation of the local timezone object is expensive (because it has to do some IO to figure it out).

Comment 16

[Frédéric Buclin]

Attachment: patch, v0.9.3

Fix items which I didn't reply to.

Comment 17

[Max Kanat-Alexander]

Comment on attachment 335785 [details] [diff] [review]
patch, v0.9.3

I'm pretty sure you don't need to cache the non-local timezones--see my previous comment.

Comment 18

[Frédéric Buclin]

Attachment: patch, v1

Remove the cache.

Comment 19

[Max Kanat-Alexander]

Comment on attachment 335796 [details] [diff] [review]
patch, v1

Looks great!

Comment 20

[Frédéric Buclin]

Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.383; previous revision: 1.382
done
Checking in editwhines.cgi;
/cvsroot/mozilla/webtools/bugzilla/editwhines.cgi,v  <--  editwhines.cgi
new revision: 1.23; previous revision: 1.22
done
Checking in importxml.pl;
/cvsroot/mozilla/webtools/bugzilla/importxml.pl,v  <--  importxml.pl
new revision: 1.86; previous revision: 1.85
done
Checking in Bugzilla/Template.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v  <--  Template.pm
new revision: 1.91; previous revision: 1.90
done
Checking in Bugzilla/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v  <--  Util.pm
new revision: 1.74; previous revision: 1.73
done
Checking in Bugzilla/Config/Common.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/Common.pm,v  <--  Common.pm
new revision: 1.23; previous revision: 1.22
done
Checking in Bugzilla/Config/Core.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/Core.pm,v  <--  Core.pm
new revision: 1.10; previous revision: 1.9
done
Checking in Bugzilla/WebService/Bugzilla.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Bugzilla.pm,v  <--  Bugzilla.pm
new revision: 1.9; previous revision: 1.8
done
Checking in docs/en/xml/administration.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/en/xml/administration.xml,v  <--  administration.xml
new revision: 1.92; previous revision: 1.91
done
Checking in template/en/default/admin/params/core.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/params/core.html.tmpl,v  <--  core.html.tmpl
new revision: 1.12; previous revision: 1.11
done
Checking in template/en/default/bug/summarize-time.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/summarize-time.html.tmpl,v  <--  summarize-time.html.tmpl
new revision: 1.13; previous revision: 1.12
done
Checking in template/en/default/list/list.atom.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/list/list.atom.tmpl,v  <--  list.atom.tmpl
new revision: 1.6; previous revision: 1.5
done
Checking in template/en/default/list/list.csv.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/list/list.csv.tmpl,v  <--  list.csv.tmpl
new revision: 1.8; previous revision: 1.7
done
Checking in template/en/default/list/list.ics.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/list/list.ics.tmpl,v  <--  list.ics.tmpl
new revision: 1.10; previous revision: 1.9
done
Checking in template/en/default/whine/schedule.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/whine/schedule.html.tmpl,v  <--  schedule.html.tmpl
new revision: 1.12; previous revision: 1.11
done

Comment 21

[Max Kanat-Alexander]

Added to the release notes for Bugzilla 3.4 in bug 494037.