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

RESOLVED FIXED in Bugzilla 4.2

Status

()

Bugzilla
Email Notifications
P2
enhancement
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: BenB, Assigned: glob)

Tracking

(Blocks: 1 bug, {access})

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

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

8 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

8 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

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, HTML+text, XML, daily/weekly digest)

Comment 1

8 years ago
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

8 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

8 years ago
Priority: P2 → P4

Updated

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

Comment 4

7 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

7 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

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

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

Comment 7

7 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

7 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 8

7 years ago
Created attachment 540481 [details] [diff] [review]
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'.
(Assignee)

Comment 9

7 years ago
Created attachment 540556 [details] [diff] [review]
patvh v2

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

7 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)

Comment 10

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

7 years ago
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

7 years ago
Created attachment 543879 [details] [diff] [review]
patch v3

> 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 13

7 years ago
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

7 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

7 years ago
Created attachment 547017 [details] [diff] [review]
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.
Attachment #543879 - Attachment is obsolete: true
Attachment #547017 - Flags: review?(wicked)
(Assignee)

Comment 16

7 years ago
Created attachment 547018 [details] [diff] [review]
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 :)
Attachment #547018 - Flags: review?(wicked)

Comment 17

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

7 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) :)

Comment 19

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

7 years ago
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

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

Comment 21

7 years ago
Created attachment 547629 [details] [diff] [review]
patch v6

let $email->parts_set() deal with single-part emails.
Attachment #547018 - Attachment is obsolete: true
Attachment #547018 - Flags: review?(wicked)
(Assignee)

Updated

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

Comment 22

7 years ago
Created attachment 547630 [details] [diff] [review]
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

7 years ago
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

7 years ago
Flags: approval+
(Assignee)

Comment 24

7 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: 7 years ago
Resolution: --- → FIXED

Updated

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