589128 - Add a preference that specifies which type of bugmail users get (text only, HTML+text)

Status: RESOLVED FIXED

(Bugzilla :: Email Notifications, enhancement, P2)

Description

[Ben Bucksch (:BenB)]

+++ This bug was initially created as a clone of Bug #65477 +++

Summary: Allow different styles of bugmail by user preference (HTML, XML, etc.)

From discussion in bug 51946, we suddenly realized that the new email preference 
flags made available by bug 17464 would make it possible for there to be more 
than one type of bugmail notification, and for the user to choose which type they 
wanted.

The three types I can think of right off the bat are:
1) text/plain optimized for a monospace font (should be the default)
2) text/plain optimized for the proportional font
3) text/html with the changes in a table (like the Bug Activity link online)

++++++++++++++++

Bug 65477 was about this, but bug 65477 comment 32 changed it to "Send HTML bugmail" via multipart/alternative, everybody would get both HTML and plaintext in the same mail, and there would be no preference.

The following comments there argue that some people (esp. some programmers) do not want HTML mail (in fact hate it with a passion), so there should be a preference. mkanat said we should file a new bug about this, so here it is.

The preference would also allow XML bugmail (for automatic processing, push-style) or other funny uses.

Comment 1

[Max Kanat-Alexander]

At first, the only available types of bugmail will be HTML, HTML+text, and text-only, so changing the summary to reflect that.

Implementing the other types of bugmail would happen after this preference was implemented.

Comment 2

[Guy Pyrzak]

by the way the ability to do a digest (either daily or weekly) is totally independent of the format (html, txt, xml) and could be developed independently, and maybe should.

I might want digested html, or digested txt, or digested xml, one pref is for the frequency and the other is for the format.

Comment 3

[Justin Lebar (not reading bugmail)]

(In reply to comment #1)
> At first, the only available types of bugmail will be HTML, HTML+text, and
> text-only, so changing the summary to reflect that.

Would this be monospace-optimized text-only or proportional-optimized text-only?  If it's the latter, perhaps we should re-open bug 579218 for those people under the enlightened dictatorship of Google Mail who want to receive plain-text bugmail.

Comment 4

[:glob ✱]

i can take this.

i don't see the point of offering 'html only'.  i propose two settings:  text (plain-text only), and html (html & plain-text).

Comment 5

[:glob ✱]

(In reply to comment #3)
> Would this be monospace-optimized text-only or proportional-optimized
> text-only?  If it's the latter, perhaps we should re-open bug 579218

we can't change the formatting of the plain-text email as this would break a lot of applications which parse bugmail.

Comment 6

[Ben Bucksch (:BenB)]

> i don't see the point of offering 'html only'.

With 23000 bugmail messages in my folder, size matters.

Comment 7

[:glob ✱]

(In reply to comment #6)
> > i don't see the point of offering 'html only'.
> With 23000 bugmail messages in my folder, size matters.

i'd say that having that much bugmail isn't the norm, and we probably shouldn't be further complicating the preferences ux to cater for edge cases.

my concern is most bugzilla users won't understand the difference between html and html+text, will opt for html, and then will later encounter an issue with unreadable email.

for power users that want to retain the html part only, a simple procmail filter would work wonders.

Comment 8

[:glob ✱]

Attachment: wip patch

before i go too far into the rabbit hole, here's the core of the patch.

then next thing i plan on doing is extending the setting table to add a 'tab' column, as the setting should appear on the 'email' preferences, not under 'general'.

Comment 9

[:glob ✱]

Attachment: patvh v2

this patch adds a 'tab' column to the settings table, and extends most userpref tabs to display relevant settings.

Comment 10

[Max Kanat-Alexander]

(In reply to comment #5)
> we can't change the formatting of the plain-text email as this would break a
> lot of applications which parse bugmail.

  FWIW, I just want to clear this up: bugmail is not a stable API. It can break its API and has done so in the past. If there was extreme value to end users that required breaking its format, we would do so.

(In reply to comment #7)
> i'd say that having that much bugmail isn't the norm, and we probably
> shouldn't be further complicating the preferences ux to cater for edge cases.

  Agreed. The entire corpus of bugmail that I have received since 2002 is only about 500MB, if it were twice that size I would (a) be pretty much okay with that (b) delete some of it.

> my concern is most bugzilla users won't understand the difference between
> html and html+text, will opt for html, and then will later encounter an
> issue with unreadable email.

  That seems reasonable. Perhaps the option should just be a boolean about sending email in plain-text only. (That might make it more difficult to find on the page, though.)

Comment 11

[Frédéric Buclin]

Comment on attachment 540556 [details] [diff] [review]
patvh v2

I honestly don't want all this complexity for this user pref. We already have a user pref about emails named "Language used in email" in the User Preferences tab. There is no reason to put this one in the Email Preferences tab. Much easier will be to add a link from the Email Preferences tab pointing to email-related user prefs once bug 589138 is implemented. For now, just implement this user pref the same way other user prefs are.

Comment 12

[:glob ✱]

Attachment: patch v3

> I honestly don't want all this complexity for this user pref.

no worries; here's the minimal patch.

Comment 13

[Max Kanat-Alexander]

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

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

::: Bugzilla/BugMail.pm
@@ +389,5 @@
>              },
>              body => $msg_text,
> +        )
> +    );
> +    if ($user->settings->{'email_format'}->{'value'} eq 'HTML') {

We really need $user->setting('email_format') that just returns the value. Would you be so kind as to do that?

@@ +403,4 @@
>  
>      # TT trims the trailing newline, and threadingmarker may be ignored.
>      my $email = new Email::MIME("$msg_header\n");
> +    if (scalar @parts == 1) {

Do scalar(@parts), otherwise precedence is unclear.

@@ +409,5 @@
> +        $email->content_type_set($part->content_type);
> +    } else {
> +        $email->parts_set(\@parts);
> +        $email->content_type_set('multipart/alternative');
> +    }

Why can't you always use parts_set for both?

::: 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-21 [e-mail] -- Bug 589128
> +    email_format       => { options => [ 'HTML', 'Text Only'],

I would prefer these to be all lowercase with underscores, and then translated in settings-descs.

Comment 14

[:glob ✱]

(In reply to comment #13)

> Why can't you always use parts_set for both?

it doesn't make sense to send a multi-part message when there's only one part.

Comment 15

[:glob ✱]

Attachment: patch v4

fixes max's review comments.

i've added $user->setting() but only uses it in one place; i'll attach another patch which changes all calls which read a setting's value to use this new method.

Comment 16

[:glob ✱]

Attachment: patch v5

fixes max's review comments; adds $user->setting() and updates all places which get a setting's value to use the new method.

i don't know which patch is preferred, so you get both :)

Comment 17

[Frédéric Buclin]

(In reply to comment #14)
> it doesn't make sense to send a multi-part message when there's only one
> part.

Doesn't $email->parts_set() make this check for us? Otherwise your patch looks good.

Comment 18

[:glob ✱]

(In reply to comment #17)
> (In reply to comment #14)
> > it doesn't make sense to send a multi-part message when there's only one
> > part.
> 
> Doesn't $email->parts_set() make this check for us? Otherwise your patch
> looks good.

ah, it does do that.  i can do an updated patch once i know which one is preferred (v4 or v5) :)

Comment 19

[Frédéric Buclin]

(In reply to comment #18)
> ah, it does do that.  i can do an updated patch once i know which one is
> preferred (v4 or v5) :)

v5. Your patch will land on trunk only, so this cleanup is fine (and safe).

Comment 20

[Max Kanat-Alexander]

It would probably be nice (for review purposes) to split out the changes between v4 and v5 into a separate patch, though, at least.

Comment 21

[:glob ✱]

Attachment: patch v6

let $email->parts_set() deal with single-part emails.

Comment 22

[:glob ✱]

Attachment: patch v6 (for review purposes only)

same a attachment 547629 [details] [diff] [review] but without all the changes to use the new $user->setting() method.  for review purposes only.

Comment 23

[Frédéric Buclin]

Comment on attachment 547629 [details] [diff] [review]
patch v6

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

>+    email_format       => { options => [ 'html', 'text_only'],
>+                                       default => 'html' },

Please fix the indentation as follow, including the removal of the extra whitespace before 'html':

    email_format       => { options => ['html', 'text_only'],
                            default => 'html' },


Otherwise looks good and works fine. r=LpSolit with the fix on checkin above.

Comment 24

[:glob ✱]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified process_bug.cgi
modified relogin.cgi
modified sanitycheck.cgi
modified whine.pl
modified whineatnews.pl
modified Bugzilla/Bug.pm
modified Bugzilla/BugMail.pm
modified Bugzilla/Flag.pm
modified Bugzilla/Install.pm
modified Bugzilla/Token.pm
modified Bugzilla/User.pm
modified Bugzilla/Search/Quicksearch.pm
modified extensions/Voting/Extension.pm
modified template/en/default/global/setting-descs.none.tmpl
Committed revision 7865.