65477 - Send HTML bugmail

Status: RESOLVED FIXED

(Bugzilla :: Email Notifications, enhancement, P2)

Description

[Dave Miller [:justdave]]

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)

Comment 1

[Jesse Ruderman]

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

Comment 2

[Ben Bucksch (:BenB)]

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

Comment 3

[Tara Hernandez]

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

Comment 4

[Tara Hernandez]

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

Comment 5

[Matthew Tuck [:CodeMachine]]

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.

Comment 6

[Dave Miller [:justdave]]

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

Comment 7

[Dave Miller [:justdave]]

forgot to reassign...

Comment 8

[Dave Miller [:justdave]]

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

Comment 9

[now [e-mail]]

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

Comment 10

[Christopher Aillon (sabbatical, not receiving bugmail)]

-> New product Bugzilla

Comment 11

[Dave Miller [:justdave]]

enhancement, no patch -> 2.18

Comment 12

[Christopher Aillon (sabbatical, not receiving bugmail)]

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

Comment 13

[Malcolm Scott]

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

Comment 14

[Dave Miller [:justdave]]

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)

Comment 15

[Dave Miller [:justdave]]

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.

Comment 16

[Olav Vitters]

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

Comment 17

[Aaron Leventhal]

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.

Comment 18

[Frédéric Buclin]

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".

Comment 19

[Max Kanat-Alexander]

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.

Comment 20

[timeless]

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.

Comment 21

[Max Kanat-Alexander]

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.

Comment 22

[Max Kanat-Alexander]

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

Comment 23

[Max Kanat-Alexander]

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.

Comment 24

[Christian Höltje]

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!

Comment 25

[Christian Höltje]

(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.

Comment 26

[Vitaliy Filippov]

Attachment: Bugzilla HTML bugmail implementation part 1 (patch)

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.

Comment 27

[Vitaliy Filippov]

Attachment: Bugzilla HTML bugmail implementation part 2 (newchangedemail.txt.tmpl)

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

Comment 28

[Vitaliy Filippov]

Attachment: Bugzilla HTML bugmail implementation part 3 (request/email.txt.tmpl)

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

Comment 30

[Guy Pyrzak]

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?

Comment 31

[Guy Pyrzak]

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?

Comment 32

[Max Kanat-Alexander]

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.

Comment 33

[Max Kanat-Alexander]

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.

Comment 34

[Guy Pyrzak]

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

Comment 35

[Max Kanat-Alexander]

(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".

Comment 36

[Justin Lebar (not reading 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.

Comment 37

[Max Kanat-Alexander]

(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.

Comment 38

[Justin Lebar (not reading bugmail)]

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

Comment 39

[Max Kanat-Alexander]

(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.

Comment 40

[Ben Bucksch (:BenB)]

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.

Comment 41

[Max Kanat-Alexander]

(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.

Comment 42

[Ben Bucksch (:BenB)]

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.

Comment 43

[Max Kanat-Alexander]

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/

Comment 44

[Frédéric Buclin]

(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.

Comment 45

[Max Kanat-Alexander]

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/.

Comment 46

[Ben Bucksch (:BenB)]

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

Comment 47

[Frédéric Buclin]

(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.

Comment 48

[Ben Bucksch (:BenB)]

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

Comment 49

[Max Kanat-Alexander]

(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.

Comment 50

[Max Kanat-Alexander]

(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.

Comment 51

[Frédéric Buclin]

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

Comment 52

[Guy Pyrzak]

(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 53

[Guy Pyrzak]

Attachment: v1

Comment 54

[Frédéric Buclin]

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 <[e-mail]>
>+  #                 Frédéric Buclin <[e-mail]>

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 -

Comment 55

[Guy Pyrzak]

Attachment: v2

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

Comment 56

[Max Kanat-Alexander]

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 57

[Guy Pyrzak]

Attachment: V3

Comment 58

[Max Kanat-Alexander]

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

Comment 59

[Guy Pyrzak]

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

Comment 60

[Guy Pyrzak]

Attachment: v4

all changes applied and tested with unicode characters

Comment 61

[Guy Pyrzak]

Attachment: v4.1

passing in the bug object instead of just the id

Comment 62

[Guy Pyrzak]

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.

Comment 63

[Guy Pyrzak]

Attachment: v4.2

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

Comment 64

[Frédéric Buclin]

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 <[e-mail]>
>+  #%]
>+[% 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 <[e-mail]>
>+  #%]
>+[% 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 65

[Max Kanat-Alexander]

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

Comment 66

[Max Kanat-Alexander]

Attachment: Thunderbird Screenshot

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.

Comment 67

[Ben Bucksch (:BenB)]

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.

Comment 68

[Ben Bucksch (:BenB)]

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.

Comment 69

[Justin Lebar (not reading bugmail)]

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

Comment 70

[Max Kanat-Alexander]

  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.

Comment 71

[Ben Bucksch (:BenB)]

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

Comment 72

[Max Kanat-Alexander]

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.

Comment 73

[Ben Bucksch (:BenB)]

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

Comment 74

[Ben Bucksch (:BenB)]

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

Comment 75

[Max Kanat-Alexander]

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

Comment 76

[Max Kanat-Alexander]

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.

Comment 77

[Max Kanat-Alexander]

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/

Comment 78

[Ben Bucksch (:BenB)]

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.

Comment 79

[Max Kanat-Alexander]

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

Comment 80

[Guy Pyrzak]

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 81

[Guy Pyrzak]

Attachment: V5

Comment 82

[Frédéric Buclin]

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.

Comment 83

[Frédéric Buclin]

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 84

[Frédéric Buclin]

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 <[e-mail]>
>+  #%]
>+  [% 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 %]

Comment 85

[Max Kanat-Alexander]

(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.

Comment 86

[Ben Bucksch (:BenB)]

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.

Comment 87

[Marc Schumann [:Wurblzap]]

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

Comment 88

[Frédéric Buclin]

(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 89

[Guy Pyrzak]

Attachment: V6

Comment 90

[Frédéric Buclin]

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 91

[Guy Pyrzak]

Attachment: v7

Comment 92

[Max Kanat-Alexander]

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 93

[Guy Pyrzak]

Attachment: V8

Comment 94

[Guy Pyrzak]

Comment on attachment 480345 [details] [diff] [review]
V8

ignore this one, bundle got messed up

Comment 95

[Guy Pyrzak]

Attachment: v9

Comment 96

[Guy Pyrzak]

Comment on attachment 480349 [details] [diff] [review]
v9

gah, I'm sorry guys, I keep forgetting stuff in the bundle

Comment 97

[Guy Pyrzak]

Attachment: v10

Comment 98

[Max Kanat-Alexander]

Comment on attachment 480352 [details] [diff] [review]
v10

Can't call method "id" on an undefined value at Bugzilla/BugMail.pm line 108.

Comment 99

[Max Kanat-Alexander]

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

Comment 100

[Guy Pyrzak]

Attachment: v11

Comment 101

[Max Kanat-Alexander]

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

Comment 102

[Guy Pyrzak]

Committing to: bzr+ssh://[e-mail]/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!

Comment 104

[ehaier]

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