Closed
Bug 652165
Opened 12 years ago
Closed 12 years ago
Flagmails have a wrong Date: value
Categories
(Bugzilla :: Email Notifications, defect)
Tracking
()
RESOLVED
FIXED
Bugzilla 4.0
People
(Reporter: LpSolit, Assigned: LpSolit)
References
Details
(Keywords: regression)
Attachments
(2 files)
6.54 KB,
patch
|
wicked
:
review+
|
Details | Diff | Splinter Review |
5.41 KB,
patch
|
wicked
:
review+
|
Details | Diff | Splinter Review |
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).
Flags: blocking4.0.1+
![]() |
Assignee | |
Comment 1•12 years ago
|
||
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.
Attachment #527884 -
Flags: review?(wicked)
Comment 2•12 years ago
|
||
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...
![]() |
Assignee | |
Comment 3•12 years ago
|
||
(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.
![]() |
Assignee | |
Comment 4•12 years ago
|
||
Attachment #527894 -
Flags: review?(wicked)
![]() |
Assignee | |
Comment 5•12 years ago
|
||
(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•12 years ago
|
||
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.
Attachment #527884 -
Flags: review?(wicked) → review+
Comment 7•12 years ago
|
||
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.
Attachment #527894 -
Flags: review?(wicked) → review+
Updated•12 years ago
|
Flags: approval?
Flags: approval4.0?
![]() |
Assignee | |
Comment 8•12 years ago
|
||
Thanks for the review, wicked. :) POD was indeed already out-of-date, which is why I didn't bother fixing it. :)
Flags: approval?
Flags: approval4.0?
Flags: approval4.0+
Flags: approval+
![]() |
Assignee | |
Comment 9•12 years ago
|
||
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/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://lpsolit%40gmail.com@bzr.mozilla.org/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.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•