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

RESOLVED FIXED in Bugzilla 4.2

Status

()

enhancement
P2
normal
RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: BenB, Assigned: glob)

Tracking

(Blocks 1 bug, {access})

Dependency tree / graph
Bug Flags:
approval +
blocking4.2 +

Details

Attachments

(2 attachments, 5 obsolete attachments)

Reporter

Description

9 years ago
+++ 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.

Updated

9 years ago
Summary: Allow different styles of bugmail by user preference (HTML, XML, etc.) → Add a preference that specifies which type of bugmail users get (text only, HTML, HTML+text)
Reporter

Updated

9 years ago
Summary: Add a preference that specifies which type of bugmail users get (text only, HTML, HTML+text) → Add a preference that specifies which type of bugmail users get (text only, HTML, HTML+text, XML, daily/weekly digest)
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.
Summary: Add a preference that specifies which type of bugmail users get (text only, HTML, HTML+text, XML, daily/weekly digest) → Add a preference that specifies which type of bugmail users get (text only, HTML, HTML+text)

Comment 2

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

Updated

9 years ago
Priority: P2 → P4

Updated

8 years ago
Flags: blocking4.2+
Target Milestone: --- → Bugzilla 4.2
Version: unspecified → 4.1
Assignee

Comment 4

8 years ago
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).
Assignee: email-notifications → glob
Priority: P4 → P2
Assignee

Comment 5

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

Comment 6

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

With 23000 bugmail messages in my folder, size matters.
Assignee

Comment 7

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

Updated

8 years ago
Status: NEW → ASSIGNED
Assignee

Comment 8

8 years ago
Posted patch wip patch (obsolete) — Splinter Review
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'.
Assignee

Comment 9

8 years ago
Posted patch patvh v2 (obsolete) — Splinter Review
this patch adds a 'tab' column to the settings table, and extends most userpref tabs to display relevant settings.
Attachment #540481 - Attachment is obsolete: true
Attachment #540556 - Flags: review?(wicked)
Assignee

Updated

8 years ago
Summary: Add a preference that specifies which type of bugmail users get (text only, HTML, HTML+text) → Add a preference that specifies which type of bugmail users get (text only, HTML+text)
(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 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.
Attachment #540556 - Flags: review?(wicked) → review-
Assignee

Comment 12

8 years ago
Posted patch patch v3 (obsolete) — Splinter Review
> I honestly don't want all this complexity for this user pref.

no worries; here's the minimal patch.
Attachment #540556 - Attachment is obsolete: true
Attachment #543879 - Flags: review?(wicked)
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 dkl@mozilla.com -- Bug 580490
>      quicksearch_fulltext => { options => ['on', 'off'], default => 'on' },
> +    # 2011-06-21 glob@mozilla.com -- Bug 589128
> +    email_format       => { options => [ 'HTML', 'Text Only'],

I would prefer these to be all lowercase with underscores, and then translated in settings-descs.
Attachment #543879 - Flags: review?(wicked) → review-
Assignee

Comment 14

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

Comment 15

8 years ago
Posted patch patch v4 (obsolete) — Splinter Review
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.
Attachment #543879 - Attachment is obsolete: true
Attachment #547017 - Flags: review?(wicked)
Assignee

Comment 16

8 years ago
Posted patch patch v5 (obsolete) — Splinter Review
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 :)
Attachment #547018 - Flags: review?(wicked)
(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.
Assignee

Comment 18

8 years ago
(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) :)
(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).
It would probably be nice (for review purposes) to split out the changes between v4 and v5 into a separate patch, though, at least.
Assignee

Updated

8 years ago
Attachment #547017 - Attachment is obsolete: true
Attachment #547017 - Flags: review?(wicked)
Assignee

Comment 21

8 years ago
Posted patch patch v6Splinter Review
let $email->parts_set() deal with single-part emails.
Attachment #547018 - Attachment is obsolete: true
Attachment #547018 - Flags: review?(wicked)
Assignee

Updated

8 years ago
Attachment #547629 - Flags: review?(wicked)
Assignee

Comment 22

8 years ago
same a attachment 547629 [details] [diff] [review] but without all the changes to use the new $user->setting() method.  for review purposes only.
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.
Attachment #547629 - Flags: review?(wicked) → review+

Updated

8 years ago
Flags: approval+
Assignee

Comment 24

8 years ago
Committing to: bzr+ssh://bjones%40mozilla.com@bzr.mozilla.org/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.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Updated

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