652165 - Flagmails have a wrong Date: value

Status: RESOLVED FIXED

(Bugzilla :: Email Notifications, defect)

Description

[Frédéric Buclin]

Due to bug 452761, all flagmails have the wrong Date: in them since Bugzilla 4.0. We have:

Date: [% bug.delta_ts ... %]

but we send flagmails before updating the bug timestamp, and so Date: is set to the last time the bug was edited, which could be months or years ago. This explains why I missed tens of unread flagmails (I sort my emails by date!).

I'm marking the bug as blocking 4.0.1 as it really means lost of productivity (I never saw these requests till recently).

Comment 1

[Frédéric Buclin]

Attachment: patch for 4.2, v1

I did some refactoring to 1) fix the problem described in this bug, and 2) stop calling format_time() and ->timezone for each user getting an email, which are both a waste of ressources.

Comment 2

[Reed Loden [:reed]]

Comment on attachment 527884 [details] [diff] [review]
patch for 4.2, v1

Seems like you'd want to use the server's TZ rather than UTC...

Comment 3

[Frédéric Buclin]

(In reply to comment #2)
> Seems like you'd want to use the server's TZ rather than UTC...

No need, this is a waste of ressource.

Comment 4

[Frédéric Buclin]

Attachment: patch for 4.0, v1

Comment 5

[Frédéric Buclin]

(In reply to comment #1)
> I did some refactoring to 1) fix the problem described in this bug, and 2) stop
> calling format_time() and ->timezone for each user getting an email, which are
> both a waste of ressources.

About the waste of ressource, here are some data with only 3 users in the CC list and no watcher:

old: # spent 57.4ms (580µs+56.8) within Bugzilla::Util::format_time which was called 28 times, avg 2.05ms/call
new: # spent 24.2ms (514µs+23.6) within Bugzilla::Util::format_time which was called 23 times, avg 1.05ms/call

old: # spent  49.4ms making 28 calls to Bugzilla::Util::datetime_from, avg 1.76ms/call
new: # spent  17.9ms making 23 calls to Bugzilla::Util::datetime_from, avg 776µs/call

Hardcoding the timezone is really what makes a big difference. And formatting the date only once for all users also helps a lot. With more users watching the bug, the benefit would be even higher.

Comment 6

[Teemu Mannermaa (:wicked)]

Comment on attachment 527884 [details] [diff] [review]
patch for 4.2, v1

>Index: Bugzilla/BugMail.pm
>===================================================================
>+    # The Date: header doesn't depend on the user getting the email.

Eh, what? :) This should be reworded, maybe something like s/depend on/have to be in the timezone of/?

Nit: And maybe other sentence to say that the MUA usually converts to the local timezone of the user for email display?
This way there's no confusion on why we do things this way.. Even though there might be some complaints from users of broken MUAs that don't do this. (No, I don't know any.)

>Index: Bugzilla/Flag.pm
>===================================================================
>@@ -905,7 +906,7 @@ or deleted.
>-    my ($class, $flag, $old_flag, $obj) = @_;
>+    my ($class, $flag, $old_flag, $obj, $timestamp) = @_;

Nit: Please, update the pod for this change too. (It seems it's already out-of-date from other changes.) Bonus points for adding short descriptions of the parameters too. :)

>@@ -941,6 +942,10 @@ sub notify {
>+    # The Date: header doesn't depend on the user getting the email.

Same rewording here, please.

>+    $timestamp ||= Bugzilla->dbh->selectrow_array('SELECT LOCALTIMESTAMP(0)');

It's a bit concerning to apparently see both NOW() and LOCALTIMESTAMP() mixed in for generating timestamps for some attachment fields. Especially since they seem to behave differently on Pg and perhaps on Oracle (sounds like they are same on MySQL). What might happen is that we get a timestamp from DB in localtime/UTC and assume it's in UTC/localtime because we don't know what DB returned (it doesn't have timezone info) and then format it incorrectly to UTC.

However, since previously the code works (or doesn't work) then this change (which only moves things around) shouldn't make things any worse.. I also tested this on both MySQL and Pg and it indeed seems to work.

Comment 7

[Teemu Mannermaa (:wicked)]

Comment on attachment 527894 [details] [diff] [review]
patch for 4.0, v1

>=== modified file 'Bugzilla/BugMail.pm'
>+    # The Date: header doesn't depend on the user getting the email.

Same comment change here as in trunk version.

>=== modified file 'Bugzilla/Flag.pm'
>@@ -903,7 +903,7 @@
>-    my ($class, $flag, $old_flag, $obj) = @_;
>+    my ($class, $flag, $old_flag, $obj, $timestamp) = @_;

Nit: Please, update pod here too.

>@@ -939,6 +939,10 @@
>+    # The Date: header doesn't depend on the user getting the email.

And change this comment too.

Otherwise looks and passes my brief tests as good as trunk version.

Comment 8

[Frédéric Buclin]

Thanks for the review, wicked. :) POD was indeed already out-of-date, which is why I didn't bother fixing it. :)

Comment 9

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla/BugMail.pm
modified Bugzilla/Flag.pm
modified template/en/default/email/bugmail-header.txt.tmpl
modified template/en/default/request/email.txt.tmpl
Committed revision 7782.

Committing to: bzr+ssh://[e-mail]/bugzilla/4.0/
modified Bugzilla/BugMail.pm
modified Bugzilla/Flag.pm
modified template/en/default/email/newchangedmail.txt.tmpl
modified template/en/default/request/email.txt.tmpl
Committed revision 7573.