Remove the 'timezone' parameter

RESOLVED FIXED in Bugzilla 3.4

Status

()

--
enhancement
RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: LpSolit, Assigned: LpSolit)

Tracking

3.1.2
Bugzilla 3.4
Dependency tree / graph
Bug Flags:
approval +

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

11 years ago
Per our discussions at our last 2 Bugzilla meetings, this param should go away. Bugzilla should use the server timezone.
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

11 years ago
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

11 years ago
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

11 years ago
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

11 years ago
  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.
(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

11 years ago
(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

11 years ago
(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

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

Comment 10

10 years ago
(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.
Depends on: 182238

Updated

10 years ago
Target Milestone: --- → Bugzilla 3.4
(Assignee)

Comment 11

10 years ago
Bug 182238 is fixed now. Time to kill the 'timezone' parameter.
Assignee: administration → LpSolit
(Assignee)

Comment 12

10 years ago
Created attachment 335767 [details] [diff] [review]
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.
Attachment #335767 - Flags: review?(mkanat)

Comment 13

10 years ago
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. :-)
Attachment #335767 - Flags: review?(mkanat) → review-
(Assignee)

Comment 14

10 years ago
(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.
Status: NEW → ASSIGNED

Comment 15

10 years ago
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).
(Assignee)

Comment 16

10 years ago
Created attachment 335785 [details] [diff] [review]
patch, v0.9.3

Fix items which I didn't reply to.
Attachment #335767 - Attachment is obsolete: true
Attachment #335785 - Flags: review?(mkanat)

Comment 17

10 years ago
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.
Attachment #335785 - Flags: review?(mkanat) → review-
(Assignee)

Comment 18

10 years ago
Created attachment 335796 [details] [diff] [review]
patch, v1

Remove the cache.
Attachment #335785 - Attachment is obsolete: true
Attachment #335796 - Flags: review?(mkanat)

Comment 19

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

Looks great!
Attachment #335796 - Flags: review?(mkanat) → review+

Updated

10 years ago
Flags: approval+
(Assignee)

Comment 20

10 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: relnote
Resolution: --- → FIXED
(Assignee)

Updated

10 years ago
Blocks: 452519

Comment 21

9 years ago
Added to the release notes for Bugzilla 3.4 in bug 494037.
Keywords: relnote
(Assignee)

Updated

9 years ago
Blocks: 552349
You need to log in before you can comment on or make changes to this bug.