663747 - Add an option to make email subjects for new bugs compatible with gmail's threading

Status: RESOLVED FIXED

(Bugzilla :: Email Notifications, enhancement)

Description

[:glob ✱]

Spun off bug 650575; add a user setting to strip the "New: " prefix from subjects of new mail, in order to make gmail's threading work (gmail honours the subject only when it comes to threading, it ignores In-Reply-To).

Comment 1

[:glob ✱]

mkanat said "It would be even cooler if it defaulted to "on" for people whose account was @gmail.com or @googlemail.com".

Comment 2

[Frédéric Buclin]

(In reply to comment #1)
> mkanat said "It would be even cooler if it defaulted to "on" for people
> whose account was @gmail.com or @googlemail.com".

I don't think we should hardcode such tricks. Personally, I use Thunderbird to read emails, and the world is not limited to Gmail either, so no reason to treat it differently, IMO. We should always start with default off.

Comment 3

[David Lawrence [:dkl]]

(In reply to comment #2)
> (In reply to comment #1)
> > mkanat said "It would be even cooler if it defaulted to "on" for people
> > whose account was @gmail.com or @googlemail.com".
> 
> I don't think we should hardcode such tricks. Personally, I use Thunderbird
> to read emails, and the world is not limited to Gmail either, so no reason
> to treat it differently, IMO. We should always start with default off.

I agree with LpSolit that we just keep this simple as possible. The user will know whether they need this functionality or not and just make it either on or off. Not on but only for certain addresses.

dkl

Comment 4

[Max Kanat-Alexander]

Yeah, I agree with LpSolit, let's not default it to on for anybody.

Comment 5

[Max Kanat-Alexander]

glob, I assume this should be assigned to you?

Comment 6

[:glob ✱]

(Quoting bug 650575  comment #34)
> glob: Particularly if this goes upstream, and even if not, can we remove the
> Gmail-specific naming in this feature? From the comments, it looks like
> several people have this problem, and I bet it's not only gmail which has a
> bad threading algorithm. A better name for the preference would be:
> 
> Add "New:" to subject line of email sent when a new bug is filed
> and default it to "on".

Comment 7

[:glob ✱]

Attachment: patch v1

Comment 8

[Max Kanat-Alexander]

Comment on attachment 539786 [details] [diff] [review]
patch v1

Review of attachment 539786 [details] [diff] [review]:
-----------------------------------------------------------------

::: Bugzilla/Install.pm
@@ +90,5 @@
>      timezone           => { subclass => 'Timezone', default => 'local' },
>      # 2011-02-07 [e-mail] -- Bug 580490
>      quicksearch_fulltext => { options => ['on', 'off'], default => 'on' },
> +    # 2011-06-16 [e-mail] -- Bug 663747
> +    new_subject_prefix => { options => ['on', 'off'], default => 'on' },

Let's call it bugmail_new_prefix.

::: template/en/default/email/bugmail-header.txt.tmpl
@@ +22,4 @@
>    
>  [% PROCESS "global/field-descs.none.tmpl" %]
>  [% PROCESS "global/reason-descs.none.tmpl" %]
> +[% show_new = (bug.lastdiffed ? 0 : 1)

You probably don't need the ?: here anymore.

@@ +22,5 @@
>    
>  [% PROCESS "global/field-descs.none.tmpl" %]
>  [% PROCESS "global/reason-descs.none.tmpl" %]
> +[% show_new = (bug.lastdiffed ? 0 : 1)
> +              && (to_user.settings.new_subject_prefix.value == 'on') %]

Is this the only thing that accesses to_user.settings in bugmail, right now? We should make sure that this doesn't significantly degrade email-sending performance.

::: template/en/default/global/setting-descs.none.tmpl
@@ +49,4 @@
>     "timezone"                         => "Timezone used to display dates and times",
>     "local"                            => "Same as the server",
>     "quicksearch_fulltext"             => "Include comments when performing quick searches (slower)",
> +   "new_subject_prefix"               => "Add 'New:' to subject line of email sent when a new $terms.bug is filed",

This is very long, does it wrap when shown in the UI? (I don't want to mess up the General Preferences UI.)

Comment 9

[Marc Schumann [:Wurblzap]]

I don't think we should address this in such a user-visible way. We don't care much about broken browsers, so why should we care much about broken e-mail clients? Let's care a little, all right, if we find a way. But a user pref is too much imho, and I'd say if we don't find a way to care silently, we shouldn't care at all.

Comment 10

[Max Kanat-Alexander]

Hey Marc. Normally I'd agree with you, but this affects such a large portion of our userbase (I hear about this problem *all the time*) and it's basically impossible to automatically know how people want it to behave (since many people use IMAP clients even with gmail) that I think in this case, it's worth a specific user preference.

Comment 11

[Marc Schumann [:Wurblzap]]

Hmmmmm... Thinking about this I'd rather see the 'New' marker removed than a pref added...

Comment 12

[:glob ✱]

(In reply to comment #8)

> >  [% PROCESS "global/field-descs.none.tmpl" %]
> >  [% PROCESS "global/reason-descs.none.tmpl" %]
> > +[% show_new = (bug.lastdiffed ? 0 : 1)
> > +              && (to_user.settings.new_subject_prefix.value == 'on') %]
> 
> Is this the only thing that accesses to_user.settings in bugmail, right now?

yes.

> We should make sure that this doesn't significantly degrade email-sending
> performance.

on my system the query executed to read the settings takes 0.3 seconds cold (it being the first query run on the database after mysql was started), and 0.000 secs warm.  and user settings are cached, so we should be ok here :)

> This is very long, does it wrap when shown in the UI? (I don't want to mess
> up the General Preferences UI.)

it's only a few pixels longer than "Quote the associated comment when you click on its reply link".
when resizing the window, the header wraps before this text does.

Comment 13

[:glob ✱]

Attachment: patch v2

Comment 14

[:glob ✱]

Comment on attachment 540009 [details] [diff] [review]
patch v2

found a small bug

Comment 15

[:glob ✱]

Attachment: patch v3

'isnew' is needed for the X-Bugzilla-Type header, so it needs to be retained.

Comment 16

[Max Kanat-Alexander]

(In reply to comment #12)
> on my system the query executed to read the settings takes 0.3 seconds cold
> (it being the first query run on the database after mysql was started), and
> 0.000 secs warm.  and user settings are cached, so we should be ok here :)

  No, we won't be okay. Those are new User objects for every single recipient on every single bug that's being changed. There isn't any caching that will assist us there.

  Could you test how it affects performance on a bug with a few thousand users on the CC list and a profile_setting table approximately the size of bmo's? We should assume the cold time, since most people don't have MySQL set up with enough cache space to keep all this settings info in RAM. (Make sure you're testing with the O_DIRECT access method in my.cnf, too, so that the disk's cache isn't affecting things either.)

  I'm sorry to make you go through a hassle like this, but when we've made even small performance changes in email, before, it has significantly impacted the performance of updating bugs, and I'd like to avoid regressing that if at all possible.

> it's only a few pixels longer than "Quote the associated comment when you
> click on its reply link".
> when resizing the window, the header wraps before this text does.

  Okay.

Comment 17

[Max Kanat-Alexander]

(In reply to comment #11)
> Hmmmmm... Thinking about this I'd rather see the 'New' marker removed than a
> pref added...

  That's a possibility. I think we should do the preference as a transitional thing, to see how people use it a bit, before we make the change everywhere. I do sort of like the "New" markers, myself.

Comment 18

[:glob ✱]

(In reply to comment #16)
>   Could you test how it affects performance on a bug with a few thousand
> users on the CC list and a profile_setting table approximately the size of
> bmo's?

these tests were done on a sanitised bmo dump, using the most cc'd bug on bmo, with 327 cc's and 324 votes, resulting in 450 email notifications for each change.  mysqld was restarted between each test run.

all times are hires wallclock seconds, as measured by Benchmark:

without patch:
  8.49903, 8.02419, 8.0225

with patch:
  8.00234, 7.78278, 7.54392, 7.74295, 7.78025, 7.78716

after getting quicker responses with the patch, i backed out the template change and retested:
  7.4803, 8.84966, 8.60096

Comment 19

[Max Kanat-Alexander]

Comment on attachment 540392 [details] [diff] [review]
patch v3

Okay, sounds great! Thanks for doing that testing, I'm happy to know in advance that we're probably not regressing performance here. :-)

Comment 20

[Frédéric Buclin]

Comment on attachment 540392 [details] [diff] [review]
patch v3

>=== modified file 'Bugzilla/Install.pm'

>     # 2011-02-07 [e-mail] -- Bug 580490
>     quicksearch_fulltext => { options => ['on', 'off'], default => 'on' },
>+    # 2011-06-16 [e-mail] -- Bug 663747
>+    bugmail_new_prefix => { options => ['on', 'off'], default => 'on' },

Bitrot due to bug 589128. Holding approval till a new patch is attached.

Comment 21

[Matt Selsky [:selsky]]

Attachment: patch v4

Update for bitrot.

Comment 22

[Gervase Markham [:gerv]]

Comment on attachment 619495 [details] [diff] [review]
patch v4

r=gerv.

Gerv

Comment 23

[:glob ✱]

thanks for updating the patch selsky!

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla/Install.pm
modified template/en/default/email/bugmail-header.txt.tmpl
modified template/en/default/global/setting-descs.none.tmpl
Committed revision 8222.

Comment 24

[Frédéric Buclin]

Added to relnotes for 4.4.