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)

See also bug 64494, "New emailtech causes unreadable mails with variable font width". This bug sounds like it's the right solution.

*** Bug 64494 has been marked as a duplicate of this bug. ***

*** Bug 51946 has been marked as a duplicate of this bug. ***

*** Bug 51946 has been marked as a duplicate of this bug. ***

I'll move this to BZ3 at the moment. My understanding is a user will be able to change this, but I guess we're unlikely to have more than one type until after 3.0. I'm not sure what this has to do with bug #17464 though, because I can't so any circumstance where a user would want to have a different type of notification based on change type.

it has to do with bug 17464 not because of the change type, but because of the preference system it's using. Makes it easy in the database to add a flag for the type of mail you want. I think this is doable in 2.16 (probably more likely that some of the other things that are flagged for 2.16 right now).

forgot to reassign...

*** Bug 87565 has been marked as a duplicate of this bug. ***

I don't usually like HTML mail, but presenting the changes in an HTML table sounds like a great idea.

-> New product Bugzilla

enhancement, no patch -> 2.18

*** Bug 141690 has been marked as a duplicate of this bug. ***

As an alternative to the user choosing between HTML and Plain Text bugmail, they could be sent out together in a multi-part MIME message. Just a thought...

Enhancements which don't currently have patches on them which are targetted at 2.18 are being retargetted to 2.20 because we're about to freeze for 2.18. Consideration will be taken for moving items back to 2.18 on a case-by-case basis (but is unlikely for enhancements)

Bugzilla 2.20 feature set is now frozen as of 15 Sept 2004. Anything flagged enhancement that hasn't already landed is being pushed out. If this bug is otherwise ready to land, we'll handle it on a case-by-case basis, please set the blocking2.20 flag to '?' if you think it qualifies.

*** Bug 342033 has been marked as a duplicate of this bug. ***

It is not an enhancement to make bugmail accessible. Without this the summary of changes is not usable for FOSS community members who use screen readers.

This bug is retargetted to Bugzilla 3.2 for one of the following reasons: - it has no assignee (except the default one) - we don't expect someone to fix it in the next two weeks (i.e. before we freeze the trunk to prepare Bugzilla 3.0 RC1) - it's not a blocker If you are working on this bug and you think you will be able to submit a patch in the next two weeks, retarget this bug to 3.0. If this bug is something you would like to see implemented in 3.0 but you are not a developer or you don't think you will be able to fix this bug yourself in the next two weeks, please *do not* retarget this bug. If you think this bug should absolutely be fixed before we release 3.0, either ask on IRC or use the "blocking3.0 flag".

Now that we use the Email:: modules for sending mail, it should be trivial to implement a multipart/alternative email that contains both HTML and txt. We should have an actual separate template for the text part, the HTML part, and the headers.

personally I'm relatively opposed to this "ideal" option. for the simple reason that I use a quota-d corporate mailbox and it turns out that the biggest waster of my mailbox's quota is actually html, and guess where it comes from? oddly enough, it comes from Bugzilla's html reports -- which are basically huge tables with lots of links to queries. people have a tendency to send these html pages around not caring about the consequences of sending them. I'm sure that people would similarly forward the html bugmails. if you think i shouldn't have to worry about mail quotas, well.. yeah.. um... what can i say? i do have to worry. An alternate diff format where we used two-three lines instead of 3 columns to make a diff would be more to my liking for certain things. Hopefully such a diff wouldn't have the same alignment problems and might make some people happier. > Status|ASSIGNED |RESOLVED > Resolution| |WORKSFORME ____Status< ASSIGNED ____Status> RESOLVED _Resolution> WORKSFORME note that ">" and "<" take the same amount of horizontal space in most fonts so you're unlikely to have alignment problems there.

We don't want to definitely put this on the roadmap for 3.2 because it might require more re-arch than we have time to do.

This should become our meta-bug for tracking the different styles of bug notifications that people want.

Bugzilla 3.2 is now frozen. Only enhancements blocking 3.2 or specifically approved for 3.2 may be checked in to the 3.2 branch. If you would like to nominate your enhancement for Bugzilla 3.2, set the "blocking3.2" flag to "?", and either the target milestone will be changed back, or the blocking3.2 flag will be granted, if we will accept this enhancement for Bugzilla 3.2.

Wouldn't it be a better idea to be allowed to use Template for the mail? This would let people who only want text/plain to use that template, or html, or whatever? Ciao!

(In reply to comment #24) > Wouldn't it be a better idea to be allowed to use Template for the mail? This > would let people who only want text/plain to use that template, or html, or > whatever? I see that 3 does it that way already, though being able to have multiple templates (txt, html, both) would be nice.

Hi! In our company, we implemented HTML+plaintext multipart/alternative Bugmail. The implementation consists of 2 parts: small patch and templates email/newchangedmail.txt.tmpl and request/email.txt.tmpl which were modified so crucially that any patch, I think, would only make the review more complicated.

template/en/default/email/newchangedemail.txt.tmpl

template/en/default/request/email.txt.tmpl

since there hasn't been any progress on this bug for about a year I'm wondering if we should try to redo this? It says it is blocking on another bug, is that still true? Basically... PING?

So looking through the code as it exists the following are places where email is called: ./Bugzilla/Auth.pm: $template->process('email/lockout.txt.tmpl', $vars, \$message) ./Bugzilla/BugMail.pm: $template->process("email/newchangedmail.txt.tmpl", $vars, \$msg) ./Bugzilla/Token.pm: $template->process('account/email/request-new.txt.tmpl', $vars, \$message) ./Bugzilla/Token.pm: $template->process("account/email/change-old.txt.tmpl", $vars, \$message) ./Bugzilla/Token.pm: $template->process("account/email/change-new.txt.tmpl", $vars, \$message) ./relogin.cgi: $mail_template->process('email/sudo.txt.tmpl', { reason => $reason }, \$message); ./sanitycheck.pl: $template->process('email/sanitycheck.txt.tmpl', $vars, \$message) ./token.cgi: $template->process("account/email/confirm.html.tmpl", $vars) ./token.cgi: $template->process('account/email/confirm-new.html.tmpl', $vars) ./whineatnews.pl: $template->process("email/whine.txt.tmpl", $vars, \$msg) I think the steps that we need to do are basically replace the "txt" with the pref from the user "html", "txt" and then make sure a template exists for each of those. Does that sound right?

LpSolit is working toward this. For HTML mail, we should not have a preference--we should just send multipart/alternative emails. Then people can specify their preferences in their email client. I'm going to change the focus of this bug to be on what it's really been about for quite some time now--sending HTML bugmail. pyrzak: Note that this bug is just about bugmail, not about any other type of email that Bugzilla sends.

I notice that one of the earlier patches also addresses requestmail. I think that's something that we should save for a separate bug. As part of this, I think we should probably create a template/en/default/bugmail/ and have various separate templates there, unless the implementor, when implementing this, doesn't think it's necessary.

do you think people know about this capability in their email clients? I would prefer if this bug were more generalized. I spoke with LpSolit about this. I am happy to let him work on this but since it is 95% template I am happy to work on it. Lpsolit let me know what you prefer

(In reply to comment #34) > do you think people know about this capability in their email clients? People who care about receiving text email do know about it, yes. > I would prefer if this bug were more generalized. Why? The feature that people actually want is "HTML bugmail".

> Why? The feature that people actually want is "HTML bugmail". Maybe I'm an exception, but the feature I want is plain text bugmail that doesn't look horrible in a variable-width font.

(In reply to comment #36) > Maybe I'm an exception, but the feature I want is plain text bugmail that > doesn't look horrible in a variable-width font. Well, that would be accomplished by changing the format of the existing text bugmails, which would be a separate bug. However, HTML bugmail would probably also solve your problem, since the diffs would be a real <table> instead of a funky text table.

(In reply to comment #37) > Well, that would be accomplished by changing the format of the existing text > bugmails, which would be a separate bug. I filed bug 579218, but it was duped here. :)

(In reply to comment #38) > I filed bug 579218, but it was duped here. :) Ah, yeah. However, since HTML bugmail will solve the problem, we probably would only implement HTML bugmail and not modify the format of the text emails.

mkanat, some people really hate HTML, they don't want to even receive it. There's a large percentage of them among programmers (in general, not necessarily at mozilla.org). It should be a pref: text, HTML, text+HTML or XML.

(In reply to comment #40) > mkanat, some people really hate HTML, they don't want to even receive it. > There's a large percentage of them among programmers (in general, not > necessarily at mozilla.org). It should be a pref: text, HTML, text+HTML or XML. I agree that there are a lot of people who don't want to receive HTML, but the standard solution for that provided by the email systems of the world and supported by every mail client I know is multipart/alternative. Every mail client I've ever used has a "prefer text version" preference that you can set to view the text version of every email you receive.

multipart/alternative is intended for cases where you don't know the preference of the recipient, or there are many recipients of the same mail. Neither is true here: You can make a pref, and you generate one mail per user anyway. It does make a difference. For storage (almost 20000 bugmails in my folder, and that's not including the older ones), and for "message source": for example, I use proportional fonts for plaintext, because I find that important for readability. But every now and then, somebody does a table or ASCII-art. Then I go to Message Source to see it in original form with fixed width fonts. There are tons of other cases where people look at the message source directly and not in an email client, it does make a difference. Keep in mind that the user audience here are programmers. They care about such thing, very much so. I've heard many users speak in the worst terms about HTML mails. Forcing people to receive HTML mail (even if it's just multipart/alternative), is like painting a target mark on your software and actively asking to be ridiculed and offended, it is likely to significantly reduce the reputation of the whole bugzilla project. For the record, I am not one of them, I would like HTML bugmail, but many of my (non-mozilla) programmer friends feel like this.

Ben: I understand what you're saying. I will say that viewing the message source is probably not a common use case. What I think we should do is first implement multipart/alternative, and then file a separate bug to add a preference. If that bug gets enough votes or general comments of support from people with actual evidence that the preference is necessary, then we can implement it. I'm trying to put a very high bar here on adding new preferences. (See http://ometer.com/free-software-ui.html for a good explanation of why.) Additionally, I'd like to see reasonable evidence that a feature is required before implementing it. For a bit more of my thoughts on that, see: http://www.codesimplicity.com/post/if-it-aint-broken/

(In reply to comment #33) > As part of this, I think we should probably create a > template/en/default/bugmail/ and have various separate templates there, unless > the implementor, when implementing this, doesn't think it's necessary. It's not necessary. The current directory is template/en/default/email/, which is fine. But maybe now would be a good time to rename newchangedmail.txt.tmpl to bugmail.txt.tmpl, for clarity.

LpSolit: Well, as part of this, it will split into three separate templates--header, text, and html. That was why I was thinking a directory might be a good idea. Either /bug/email/ or /email/bug/ or /bugmail/.

> I'd like to see reasonable evidence that a feature is required > before implementing it. What sort of evidence do you expect? About 30% of the programmers hate HTML email with a passion. The more experienced the programmer, the higher the percentage. That number is of course just a guess, from talking to friends and colleges, but the sentiments are not. Do you need a survey to prove it? I think *that* would be setting the "bar too high", to use your words.

(In reply to comment #45) > LpSolit: Well, as part of this, it will split into three separate > templates--header, text, and html. That was why I was thinking a directory > might be a good idea. Either /bug/email/ or /email/bug/ or /bugmail/. Well, ideally, I would like to have all email-related templates in a single directory (we don't have so many templates, even when implementing multipart for all of them). So in the worst case, I much prefer email/bug/ than anything else.

Per mkanat comment 43, I filed bug 589128 about a preference and moved the following dependencies there: bug 72132 - Sending Bugzilla mails in MIME-digest format (per-day or per-week) bug 235720 - XML bugmail

(In reply to comment #46) > What sort of evidence do you expect? Just some reasonable number of actual individuals who personally want Bugzilla to have such a preference, with explanations from those individuals that seem like valid justifications for the preference.

(In reply to comment #47) > Well, ideally, I would like to have all email-related templates in a single > directory (we don't have so many templates, even when implementing multipart > for all of them). So in the worst case, I much prefer email/bug/ than anything > else. Okay, that sounds reasonable. I agree that we should keep all the emails in one place. /email/ is where I always expect to find them, and when they're not there, I often become confused as to where they are. I'll leave it up to the implementor if they want to create a separate subdirectory.

(In reply to comment #30) > It says it is blocking on another bug, is that still true? It was blocked by bug 466968, which is now fixed in Bugzilla 4.2. So we are good to go with this one. It should now be trivial to implement. Feel free to attach a patch. :)

(In reply to comment #35) > (In reply to comment #34) > Why? The feature that people actually want is "HTML bugmail". Well the reason is we've already got HTML whines, and it was implemented very nicely and generalizably. If you look at all the email templates they all start the same exact way. I just figured it would be better to do this once in a centralized way then doing it again for create mails etc. I understood bugmail as a short way of saying "bugzilla mail" but if bugmail is actually email about bugs. I'm happy to just do bugmail and generalize the bugmail headers and leave the other email headers as separate templates. I also feel like in terms of look and feel it will be clugy to have the most common mail we send out be html and all the other email be plain text. I guess I was advocating for consistency in email especially since it looks like giving that consistency would reduce the repeated lines of code. It would also allow people who wanted to brand the HTML emails do it in a single place instead of in multiple places (if we had a header). But again maybe that's a wrong direction. I just know based on 2 customers who have asked about this capability, that they'd like it to have something at the top that makes it clear other than the "from" about what type of mail this is.

Comment on attachment 467952 [details] [diff] [review] v1 >=== modified file 'Bugzilla/BugMail.pm' You should add your code to a private subroutine, e.g. _generate_bugmail(), to keep the code in sendMail() easy to read. Also, why using base64 as encoding? Why not using quoted-printable as we already do? >=== added directory 'template/en/default/email/bugmail' >=== added file 'template/en/default/email/bugmail/newchangedmail.html.tmpl' IMO, we don't need a new subdirectory. newchangedmail.html.tmpl should be renamed to bugmail.html.tmpl, for clarity. Same for the other ones. >+ # Contributor(s): André Batosti <batosti@async.com.br> >+ # Frédéric Buclin <LpSolit@gmail.com> Is that really UTF8? I guess you could replace both of us by your name. >+ [%+ PROCESS generate_diffs -%] Unless I'm wrong (I didn't test your patch), you don't need most of all these [%+ -%] and [%~ %] I put everywhere in the .txt template. This is HTML; it doesn't care about extra newlines or whitespaces. >+ <b>Comment #[% comment.count FILTER html %] from >+ [% comment.author.identity FILTER html%]</b> Missing [%+ here, as it's at the beginning of a line. Either that, or move "from" on the next line. >+ <pre>[%+ comment.body_full({ is_bugmail => 1, wrap => 1 }) FILTER html %]</pre> No need for +, there is a <pre> in front of it. >+ </p> >+ <h5>You are receiving this mail because:</h5> Maybe add a newline before <h5>, for readability. >+ [% terms.Bug FILTER html %]&nbsp;[% bug.id FILTER html %] No need to filter stuff beginning by terms.* >+ [%+ last_changer = "" %] Useless + >+ [% terms.Bug FILTER html %] [%+ bug.id FILTER html %] depends on [% terms.bug FILTER html %] [%+ change.blocker.id FILTER html %], which changed state. >+ [%+ terms.Bug FILTER html %] [%+ change.blocker.id FILTER html %] Summary: [% change.blocker.short_desc FILTER html %] >+ [%+ urlbase FILTER html %]show_bug.cgi?id=[% change.blocker.id FILTER html %] Too long lines. >+ [% "${change.realname} < ${change.login_name}>" FILTER html %] Extra whitespace after < >+ [% END %] changed: "changed:" should be on its own line. I had to put it here in the .txt template to avoid indentation problems. >+ <th> >+ [% field_label FILTER html %] >+ </th> Nit: I know it's just me, but I prefer a slighly more compact form: <th>[% foo %]</th>. >+ <td> >+ [% field_label FILTER html %] >+ </td> Why <td> here and <th> above? >+ [% END -%] Useless -

updated with LpSolit's suggestions. However, with some testing i get a very strange error message "Wide character in subroutine entry"

Comment on attachment 467987 [details] [diff] [review] v2 >=== modified file 'Bugzilla/BugMail.pm' >>+ $msg_header =~ s/(?:\015+)?\012/\015\012/msg; >+ $msg_text =~ s/(?:\015+)?\012/\015\012/msg; >+ $msg_html =~ s/(?:\015+)?\012/\015\012/msg; You should let MessageToMTA do that. >+ my @parts = ( >+ Email::MIME->create( >+ attributes => { >+ content_type => "text/plain", >+ charset => "utf-8", >+ }, You should let MessageToMTA set that. Also, it's not even true if the utf8 parameter isn't on. >+ Email::MIME->create( >+ attributes => { >+ content_type => "text/html", >+ charset => "utf-8", >+ }, Same there. >+ ); >+ if( !is_7bit_clean($msg_text) ){ >+ $parts[0]->encoding_set('quoted-printable') ; >+ } else { >+ $parts[0]->encoding_set('base64') ; >+ } >+ if( !is_7bit_clean($msg_html) ){ >+ $parts[1]->encoding_set('quoted-printable') ; >+ } else { >+ $parts[1]->encoding_set('base64') ; >+ } You should let MessageToMTA deal with that. >+ # appending 1 blank lines because the header parser doesn't read in the >+ # last line if it doesn't end with a new line and template strips it out >+ $email->header_obj_set( Email::Simple::Header->new( $msg_header . $email->crlf) ); Does that clear out any existing boundary or required headers that Email::MIME might already have set? >+ $email->content_type_set('multipart/alternative'); And does that? Your patch appears to be missing two templates. Also, remember to post a bundle when your patch contains renames.

Comment on attachment 468368 [details] [diff] [review] V3 >=== modified file 'Bugzilla/BugMail.pm' >+sub _generate_bugmail { >+ my ( $user, $vars ) = @_; >+ my $msg_text; >+ my $msg_html; >+ my $msg_header; This isn't C, so you don't need to declare variables at the top of a function. Just declare them in the line where they're used. >+ $template->process("email/bugmail_multipart.txt.tmpl", $vars, \$msg_header) >+ || ThrowTemplateError($template->error()); That tempate should instead be called bugmail-header. >+ $msg_header =~ s/(?:\015+)?\012/\015\012/msg; >+ $msg_text =~ s/(?:\015+)?\012/\015\012/msg; >+ $msg_html =~ s/(?:\015+)?\012/\015\012/msg; Why are you doing that (and why are you doing it here)? >+ Email::MIME->create( >+ attributes => { >+ content_type => "text/plain", >+ }, >+ body => $msg_text, >+ ), >+ Email::MIME->create( >+ attributes => { >+ content_type => "text/html", >+ }, >+ body => $msg_html, Have you tried this with Unicode to make sure that it works OK? You might need to use body_str or something like that, I'm not sure. >+ my $email = Email::MIME->create( >+ parts => [ @parts ], Just do: parts => \@parts There's no need to copy @parts. >+ # appending 1 blank lines because the header parser doesn't read in the >+ # last line if it doesn't end with a new line and template strips it out That sentence is a little unclear. Perhaps more punctuation? >+ my $email_header = Email::Simple::Header->new( $msg_header . $email->crlf); >+ # now apply the old headers to the new header object; >+ my @default_headers = $email->header_pairs; >+ for ( my $i = 0; $i < @default_headers; $i += 2) { >+ if ( ! $email_header->header( "$default_headers[$i]") ) { >+ $email_header->header_set( "$default_headers[$i]" => " $default_headers[$i + 1]"); >+ } >+ } Instead of this, make the headers into an Email::MIME object all by themselves. Then add the other parts to the existing Email::MIME object. >+ # need this because all other attempts to set the header get changed down stream by something Hmm, okay. What something? >=== added file 'template/en/default/email/bugmail.html.tmpl' >+ # The Initial Developer of the Original Code is Netscape Communications >+ # Corporation. Portions created by Netscape are >+ # Copyright (C) 1998 Netscape Communications Corporation. All >+ # Rights Reserved. Actually, it's you. :-) >+[% PROCESS "global/field-descs.none.tmpl" %] What do you need that for? (field_descs can be accessed without loading that template) >+<html> Does having a DOCTYPE help, in any email client? >+ <b>Comment #[% comment.count FILTER html %] >+ from [%+ comment.author.identity FILTER html%]</b> Nit: Space before % on the end, there. >+ <pre>[% comment.body_full({ is_bugmail => 1, wrap => 1 }) FILTER html %]</pre> Instead of using is_bugmail, just filter this through quoteUrls: comment.body_full({ wrap => 1 }) FILTER quoteUrls(bug, comment) >+[% BLOCK generate_diffs %] >+ <a href="[% urlbase FILTER html %]show_bug.cgi?id=[% bug.id FILTER html %]"> >+ [% terms.Bug %]&nbsp;[% bug.id %] >+ </a> You should't need &nbsp; there, no? Just make bug.id [%+ bug.id %] (add the +). Also, it should use FILTER bug_link instead of a manual <a>, I think, no? Perhaps there should be some switch to bug_link to put in the full urlbase. >+ <tr> >+ <td colspan="3"> >+ [% terms.Bug %] [%+ bug.id FILTER html %] depends You should use bug_link there. >+ on [% terms.bug %] [%+ change.blocker.id FILTER html %], And there, too. >+ <div> >+ <a href="[% urlbase FILTER html %]show_bug.cgi?id=[% change.blocker.id FILTER html %]"> >+ [% terms.Bug %] [%+ change.blocker.id %]: >+ [%+ change.blocker.short_desc FILTER html %] >+ </a> Use bug_link instead of the manual <a>. >+ [% IF isnew %] >+ <tr> >+ <th>[% field_label FILTER html %]</th> >+ <td>[% new_value FILTER html %]</td> >+ </tr> You've been doing colspan="3" above, so you'll need an empty <td> there at the end, or a colspan="2" on the second td. >=== added file 'template/en/default/email/bugmail_multipart.txt.tmpl' >+[% PROCESS "global/field-descs.none.tmpl" %] >+[% PROCESS "global/reason-descs.none.tmpl" %] >+[% isnew = bug.lastdiffed ? 0 : 1 %] You can put some newlines after those. TT will trim it out. In general, this is a really cool patch! :-) I'm looking forward to it being done. :-)

this blog post http://mikekleiman.blogspot.com/2007/12/using-doctype-in-email.html indicates doctype won't change anything in most email clients.

all changes applied and tested with unicode characters

passing in the bug object instead of just the id

there is a bug in the code that causes depends on and blocks to no longer appear properly. I need to investigate this before submitting having this reviewed, just noticed it was something i caused and not part of the previous patch.

turns out you need [% PROCESS "global/field-descs.none.tmpl" %] for the blocks/depends stuff to appear properly.

Comment on attachment 468583 [details] [diff] [review] v4.2 >=== modified file 'Bugzilla/BugMail.pm' >+use Data::Dumper; >+ warn Dumper($vars); Please remove debug code. >=== added file 'template/en/default/email/bugmail-header.txt.tmpl' >+ # Guy Pyrzak <guy.pyrzak@gmail.com> >+ #%] >+[% PROCESS "global/field-descs.none.tmpl" %] Nit: add one newline before [% PROCESS ... %] >=== added file 'template/en/default/email/bugmail.html.tmpl' >+ # Contributor(s): Guy Pyrzak <guy.pyrzak@gmail.com> >+ #%] >+[% PROCESS "global/field-descs.none.tmpl" %] Nit: here too >+ [% field_label = field_descs.${change.field_name} %] >+ [% old_value = display_value(change.field_name, change.old) %] >+ [% new_value = display_value(change.field_name, change.new) %] >+ >+ [% IF change.field_name == "estimated_time" || change.field_name == "remaining_time" %] >+ [% old_value = old_value FILTER format('%.2f') %] >+ [% new_value = new_value FILTER format('%.2f') %] >+ [% END %] >+ >+ [% IF change.attach_id %] >+ [% field_label = field_label.replace('^(Attachment )?', "Attachment #${change.attach_id} ") %] >+ [% END %] >+ >+ [% IF change.field_name == 'longdescs.isprivate' %] >+ [% field_label = field_label.replace('^(Comment )?', "Comment #${change.num} ") %] >+ [% END %] This is exactly the same code as for the .txt template. It would make sense to share this code between both templates (easier to maintain).

Comment on attachment 468583 [details] [diff] [review] v4.2 Okay, in addition to LpSolit's comments: >=== modified file 'Bugzilla/BugMail.pm' >+sub _generate_bugmail { >+ my ( $user, $vars ) = @_; Nit: We don't usually put an extra space inside those parens. >+ my $template = Bugzilla->template_inner($user->settings->{'lang'}->{'value'}); >+ my ($msg_text, $msg_html,$msg_header); Nit: space after second comma. >=== modified file 'Bugzilla/Template.pm' >+ if( $options->{full_url} ) { Nit: Space after "if", no space necessary inside the parens. >=== added file 'template/en/default/email/bugmail-header.txt.tmpl' >+[% PROCESS "global/field-descs.none.tmpl" %] You might be able to get away with just global/variables.none.tmpl. >+From: [% Param('mailfrom') %] Also, newline between the isnew line and the From line. >+<html> >+ <pre>[% comment.body_full({ wrap => 1 }) FILTER quoteUrls(bug, comment) %]</pre> Hmm, is quoteUrls also going to need a full_url argument? >+[% BLOCK generate_diffs %] >+ >+ [% "${terms.Bug} ${bug.id}" FILTER bug_link(bug, full_url => 1) FILTER none %] >+ </a> I don't think you need the </a> there. >+ [% IF change.realname %] >+ [% "${change.realname} <${change.login_name}>" FILTER html %] >+ [% ELSE %] >+ [% change.login_name FILTER html %] Hmm. How about we just use user.html.tmpl there? This is going to hugely improve the usability of bugmail. I'm pretty excited about it. :-)

Okay, here's what it looks like in Thunderbird, right now. Here's some thoughts: We should probably say "Max K-A changed Bug 2323:" or something like that, no? Instead of having "Bug 2323" and then "Max K-A changed:" on separate lines. Perhaps even "Max K-A changed %product% bug 2323:"? The contents of the What column should be right-aligned. user.html.tmpl should also be used in the "Comment #10 From" header for the user that the comment is from. Perhaps there should be a <hr> above "Configure bugmail", to make it clear that it's not part of the comment. Also, perhaps "Configure bugmail" should be below the "you are receiving this mail because" line. (In which case the <hr> would go after the last comment.) I think perhaps the changes table will need a border=1, or perhaps just some cellspacing, to separate the removed/added fields more clearly.

Please make the comment field in proportional font. I know it breaks ascii-art and stuff, but that's rare even in bugzilla. Proportional fonts are *much* more readable. I have all my plaintext mail in proportional fonts. With this change, I would be forced to see fixed-width fonts, unless I hack userContent.css or similar. In Thunderbird and most other email clients, you can change the HTML vs. plaintext pref only for all mail, not just for bugzilla mail, and that's not an option, so the plaintext part in multipart/alternative doesn't help at all. I think this change should block the commit of this feature, because I'd consider that a very noticable regression.

Also, please make sure that "configure bugmail" and "you are receiving this because" litter at the end is marked in grey, as are signatures in plaintext mail. Further tweaks (like marking the HTML signature in markup, titles for dependency changes etc.) can be in a followup bugs, but these two are very obvious.

I think the comment should look like it does in the Bugzilla web interface. If it's monospaced in the web interface, it should be monospaced in the e-mail. We could re-open bug 579218 for people who want to read e-mails with a proportional font...

For now, I agree with jlebar that comments should look in email exactly like they look in Bugzilla itself. When Bugzilla itself has proportional-font comments, then bugmail will get them too. Styling the "signature" stuff differently is a reasonable idea, but I'll leave the implementation of that up to pyrzak. My idea was the <hr>, but other ideas are possible.

> I agree with jlebar that comments should look in email exactly like > they look in Bugzilla itself. Reasonable, but a regression from my POV. > "signature" ... implementation .bugzilla-footer { color: grey } <div class="bugzilla-footer mail-signature"> Configure... </div>

Well, we can't actually use CSS to any real degree--Outlook uses the MS Word HTML renderer. But we can accomplish the same with old HTML4-style attributes.

it can't even use something as simple as that? I think even Outlook itself sends CSS like that out. Having the <div class="bugzilla-footer mail-signature"> is important for user agents like Thunderbird. You can then use userContent.css to style (if you're so inclined to hack custom CSS).

I checked (searched on web). Outlook (including 2007) does support the above CSS.

BenB: It's Outlook 2010 that matters: http://fixoutlook.org/

Although you may be right that simple coloring CSS will help and work OK even in Outlook 2010. That's also something that we could explore in a follow-up bug. Adding CSS will require a fair bit of testing in every popular mail client, and slow down development on this, so I just think it's something we should do afterward.

Oh, Outlook 2007 has the same rendering engine, so that's good that at least we can probably use color CSS. Here's a good chart, by the way, to help understand which email clients we need to test and support: http://www.campaignmonitor.com/stats/email-clients/

I agree fine-tuning (beyond status quo of plaintext) should be in other bugs. Just a response to this: > which email clients we need to test and support: > http://www.campaignmonitor.com/stats/email-clients/ Please don't use that, it's completely screwed: "email client usage is determined when an image is opened" That's why Thunderbird doesn't appear, neither many other clients. Even those client that appear in the chart are off with the numbers. Completely wrong statistic.

(In reply to comment #78) > Please don't use that, it's completely screwed: > "email client usage is determined when an image is opened" Yeah, it is pretty screwed, I agree. But it at least gives us an idea that we should probably test Outlook, Hotmail, Yahoo Mail, Gmail, an iPhone, and Apple Mail, in addition to Thunderbird. (I'm really not worried about Thunderbird's HTML/CSS support though. :-) )

So my objective for this bug was simply to get the HTML part in there and then do LOTS of styling in an immediately closely following up bug(s), where we could debate styles and proportional font, how outlook is broken etc. I also wanted to add hook as well. But my hope for this bug was just to get the perl parts in there and something that is at the same level as the text email. I think we're close if not there now and have a good starting off point, but my concern is we'll start fixing other bugs in this one. I'll implement the changes and post the patch shortly.

Comment on attachment 470204 [details] [diff] [review] V5 I cannot test your patch at all. I get: Can't locate object method "create" via package "Email::MIME" at Bugzilla/BugMail.pm line 374. Adding |use Email::MIME| at the top of BugMail.pm doesn't help.

You need at least Email::MIME 1.901 to use create(). It's not defined for older versions, which is why it fails for me (I have version 1.863, so you must either bump the minimum version required, or find something which works with older versions of Email::MIME. It's probably better to just bump the min version). Note that Email::MIME 1.901 already requires Email::MIME::Encodings 1.313, so we can probably drop this check from Bugzilla/Install/Requirement.pm.

Comment on attachment 470204 [details] [diff] [review] V5 OK, I installed Email::MIME 1.903, and I now have some more comments. >=== modified file 'Bugzilla/BugMail.pm' >+use Email::Simple::Header; Why use'ing Email::Simple::Header? You explicity use Email::MIME, not Email::Simple::Header, in BugMail.pm. >+sub _generate_bugmail { >+ my ($user, $vars) = @_; >+ my $template = Bugzilla->template_inner($user->settings->{'lang'}->{'value'}); >+ my ($msg_text, $msg_html, $msg_header); The indentation is wrong. Must be 4 whitespaces, not 2. >+ my $email = new Email::MIME( $msg_header ); Shouldn't you use Email::MIME->create() here, and pass @parts directly to it? >+ $email->content_type_set( 'multipart/alternative' ); If I read the POD correctly, the content type should automatically be set to multipart/mixed, already. Is this really needed? > sub _get_diffs { > foreach my $diff (@$diffs) { >+ $diff->{who} = new Bugzilla::User($diff->{who}); You should cache the user object. It's usually a pretty limited set of users who edit a given bug. IMO, this is going to generate a lot of extra calls to the DB. >=== added file 'template/en/default/email/bugmail-common.txt.html' >+ # Contributor(s): Guy Pyrzak <guy.pyrzak@gmail.com> >+ #%] >+ [% PROCESS "global/field-descs.none.tmpl" %] Add a newline before [% PROCESS ...%]. >=== added file 'template/en/default/email/bugmail.html.tmpl' >+<html> >+ <head> >+ </head> Shouldn't we specify a DOCTYPE? I think using HTML 4.01 Strict for bugmails would be a good idea. >+ <b>[% "Comment # ${comment.count}" FILTER bug_link( bug, {comment_num => comment.count, full_url => 1}) FILTER none %] >+ from [% INCLUDE global/user.html.tmpl who = comment.author %]</b> In emails, I like to quickly know who did changes (especially with all these users whose nick is "bugzilla"), and so I like to see the full email address of the changer. The .txt bugmail still has it, but this information is lost in the .html bugmail without hovering the link. >+ <a href="[% urlbase FILTER html %]userprefs.cgi?tab=email"> >+ Configure [% terms.bug %]mail >+ </a> This should really be in the signature. This is not part of the changes, and that's something which shouldn't take our attention on it. >+ <h5>You are receiving this mail because:</h5> Same here about <h5>. It's bold when it's really something which you should barely notice. >+[% BLOCK generate_diffs %] >+ >+ [% "${terms.Bug} ${bug.id}" FILTER bug_link(bug, full_url => 1) FILTER none %] Wrong indentation (2 in templates, not 4). Also, having "bug NNN" alone looks weird to me. Shouldn't we say "Visit bug NNN" or "Go to bug NNN" instead? Also, this line is too close to the table. You should add a newline, IMO. >+ <table border="1" cellspacing="0" cellpadding="10"> Especially for new bugs, the generated table has too much padding. It's now mostly impossible to read all the information without scrolling. >+ [% PROCESS "email/bugmail-common.txt.html"%] A few things: - Missing whitespace before %] - I think URLs should be linkified. - Also, for some reasons, I cannot click "attachment NNN". The text is blue, but it's not a link. Maybe <span> conflicts with <a></a>? >=== renamed file 'template/en/default/email/newchangedmail.txt.tmpl' => 'template/en/default/email/bugmail.txt.tmpl' >+ [% PROCESS "email/bugmail-common.txt.html"%] Missing whitespace before %]

(In reply to comment #84) > >+ my $email = new Email::MIME( $msg_header ); > > Shouldn't you use Email::MIME->create() here, and pass @parts directly to it? No, this is actually an evolution over the last few patches, for what works best. This turns out to be the best way to make things work right without a lot of strange hacks. > If I read the POD correctly, the content type should automatically be set to > multipart/mixed, already. Is this really needed? Yes, you don't want multipart/mixed. > Shouldn't we specify a DOCTYPE? I think using HTML 4.01 Strict for bugmails > would be a good idea. No, it doesn't help for email, and actually may make things render differently in different email clients. (pyrzak and I discussed this on IRC already) > In emails, I like to quickly know who did changes (especially with all these > users whose nick is "bugzilla"), and so I like to see the full email address of > the changer. The .txt bugmail still has it, but this information is lost in the > .html bugmail without hovering the link. I'm okay with that, personally. This should display the full name of the user as a link to their email address. It should not be displaying simply the first half of their email address--that would be wrong. > This should really be in the signature. This is not part of the changes, and > that's something which shouldn't take our attention on it. Define "signature" in a multipart/alternative HTML email. > Same here about <h5>. It's bold when it's really something which you should > barely notice. That's true. > - Also, for some reasons, I cannot click "attachment NNN". The text is blue, > but it's not a link. Maybe <span> conflicts with <a></a>? It's because the links are relative links instead of full links.

A few comments: - multipart/mixed is entirely different from multipart/alternative. While the syntax is the same, the former means essentially an attachment, while the latter means "show either (but not both), they're essentially the same" - there is no such thing as "signature" in HTML mail, there's no standard for it yet, unfortunately. Thus my comment 71. - Yes, please recognize and link URLs in the HTML part (as you do in the web interface), but not in the plaintext part. When Thunderbird gets HTML, it assumes that URLs are already linked and doesn't run the URL recognizer. It does run the URL recognizer on plaintext, so no need there (apart from "bug 123" in plaintext, but that's for another bug). - Yes, Full name, with email address as link target, is good. Never the first half of the email address.

(In reply to comment #86) Mod +1 to-be-heeded

(In reply to comment #83) > You need at least Email::MIME 1.901 to use create(). It's not defined for older > versions This will be fixed by bug 516457.

Comment on attachment 480273 [details] [diff] [review] V6 >=== modified file 'Bugzilla/BugMail.pm' >+ if( ! $user_cache{ $user_id} ){ >+ $user_cache{ $user_id} = new Bugzilla::User($user_id); >+ } Cleaner is to write: $user_cache{ $user_id} ||= new Bugzilla::User($user_id); No need for the if () check. >+ if( ! $user_cache->{ $diff->{who} } ){ >+ $user_cache->{ $diff->{who} } = new Bugzilla::User($diff->{who}); >+ } Same here. >=== removed file 'lib/README' >--- lib/README 2007-10-19 11:46:10 +0000 >+++ lib/README 1970-01-01 00:00:00 +0000 >@@ -1,4 +0,0 @@ >-This directory contains the Perl modules that Bugzilla requires to run. >- >-If you would rather have Bugzilla use the Perl modules installed on your >-system, you can delete everything in this directory. > Why is this file removed? I doubt that's intentional. >=== modified file 'skins/standard/global.css' >+<<<<<<< TREE >+ vertical-align:middle !important;} >+======= > vertical-align:middle !important; } >+>>>>>>> MERGE-SOURCE I guess I won't be able to apply your patch correctly with such <<<<<<<<<<< TREE / >>>>>>>>>>>> MERGE-SOURCE lines in it.

Comment on attachment 480311 [details] [diff] [review] v7 This should probably be a bundle, not a patch, no? >=== modified file 'Bugzilla/BugMail.pm' >+ my %user_cache = ( ); You don't need "= ( )" there. >+ # now we have to add people to the hash >+ foreach my $user ( @assignees, @qa_contacts, @ccs ){ >+ if( $user ){ >+ $user_cache{$user->id} = $user; >+ } >+ } This entire loop can be replaced by a single "map" statement. Also, there are a lot of spacing issues in this patch, like how there's an extra space inside of the parens and no space after them, twice in this loop. However, all those spacing issues *could* be fixed on checkin. >+ MessageToMTA( _generate_bugmail($user, $vars) ); Nit: That could be two lines. >=== modified file 'skins/standard/global.css' >--- skins/standard/global.css 2010-09-28 03:29:59 +0000 >+++ skins/standard/global.css 2010-10-01 23:57:12 +0000 >@@ -536,7 +536,6 @@ > } > > #keyword_container { >- padding-bottom: 2em; This is an unrelated change. >=== added file 'template/en/default/email/bugmail-header.txt.tmpl' >+[% PROCESS "global/field-descs.none.tmpl" %] Why are you processing field-descs? I don't see anything in this template that would require it. >=== added file 'template/en/default/email/bugmail.html.tmpl' >+[% PROCESS "global/field-descs.none.tmpl" %] Same comment here about field-descs. The "field_descs" variable itself is now a global variable, you don't have to process this template to get it.

Comment on attachment 480345 [details] [diff] [review] V8 ignore this one, bundle got messed up

Comment on attachment 480349 [details] [diff] [review] v9 gah, I'm sorry guys, I keep forgetting stuff in the bundle

Comment on attachment 480352 [details] [diff] [review] v10 Can't call method "id" on an undefined value at Bugzilla/BugMail.pm line 108.

Also, I think we need an &nbsp; for empty added/removed columns--otherwise the formatting looks odd.

Comment on attachment 480457 [details] [diff] [review] v11 >+ my @qa_contacts = ($bug->qa_contact) || (); Hmm. I think you mean my @qa_contacts = $bug->qa_contact || (); "($bug->qa_contact) || ()" looks like "if this list is empty, instead use the empty list". >+ my @users_list=(@ccs, @qa_contacts, @assignees); >+ my %user_cache = map { $_->id => $_ } (@assignees, @qa_contacts, @ccs); Looks like that @users_list line needs to go away. However, all that can be fixed on checkin. Looks beautiful and works great. :-) Congratulations! :-)

Committing to: bzr+ssh://guy.pyrzak%40gmail.com@bzr.mozilla.org/bugzilla/trunk/ modified Bugzilla/BugMail.pm modified Bugzilla/Template.pm added template/en/default/email/bugmail-common.txt.html added template/en/default/email/bugmail-header.txt.tmpl added template/en/default/email/bugmail.html.tmpl renamed template/en/default/email/newchangedmail.txt.tmpl => template/en/default/email/bugmail.txt.tmpl Committed revision 7513. B00M!

file error - parse error - email/bugmail.txt.tmpl line 74: unexpected end of input