the default bugzilla html bugmail is a bit harsh on the eyes, and is in need of some adjustment.

i played around with getting the short_desc into the bugmail too, but i wasn't able to come up with a layout that i liked.

Comment on attachment 634488 [details] [diff] [review] patch v1 Review of attachment 634488 [details] [diff] [review]: ----------------------------------------------------------------- Do we really need a custom copy of bugmail.txt.tmpl for just a license change? Overall I like the design as a good start. Shame the border lines do not display in Gmail but it still looks very good anyway. dkl ::: extensions/BMO/template/en/default/email/bugmail.html.tmpl @@ +51,5 @@ > + [% IF comment.count %] > + <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 %] Not an issue really, but user.html.tmpl adds the show_admin_menu() attribute to each user name element which means nothing to the email client. Probably safe to leave in. @@ +100,5 @@ > + > +[% BLOCK generate_new %] > + <div class="new"> > + <table border="0" cellspacing="0" cellpadding="5"> > + [% FOREACH change = diffs %] The field names are no longer in the same order for new bug mail as 4.0 due to the way the fields are retrieved from the database in Bugzilla->fields. 4.0 user Bugzilla->get_fields which used Bugzilla::Field->match. Bugzilla->fields in 4.2 uses Bugzilla::Field->get_all and then caches the data in hashes which do not retain any sort of order. It doesn't look right to have the bug id near the bottom and keywords at the top. It is in random order where in 4.0 it was the sort order from the fielddefs table. This is an upstream issue and not a stopper for this patch but just wanted to make note of it. @@ +103,5 @@ > + <table border="0" cellspacing="0" cellpadding="5"> > + [% FOREACH change = diffs %] > + [% PROCESS "email/bugmail-common.txt.tmpl" %] > + <tr> > + <td class="c1" nowrap><b>[% field_label FILTER html %]</b></th> Use CSS white-space: nowrap instead here.

(In reply to David Lawrence [:dkl] from comment #2) > Do we really need a custom copy of bugmail.txt.tmpl for just a license change? ah, it was initially included because of the referenced bugs stuff. that's no longer needed. > Not an issue really, but user.html.tmpl adds the show_admin_menu() attribute > to each user name element which means nothing to the email client. Probably > safe to leave in. i did notice that, but it was reasonably painful to remove, and as you pointed out email clients ignore it. > It doesn't look right to have the bug id near the bottom and keywords at the > top. It is in random order where in 4.0 it was the sort order from the > fielddefs table. This is an upstream issue and not a stopper for this patch > but just wanted to make note of it. agreed, it looks really bad :) i'll fix that. i'm thinking it should always be ID, then summary, then all fields sorted alphabetically. > @@ +103,5 @@ > > + <table border="0" cellspacing="0" cellpadding="5"> > > + [% FOREACH change = diffs %] > > + [% PROCESS "email/bugmail-common.txt.tmpl" %] > > + <tr> > > + <td class="c1" nowrap><b>[% field_label FILTER html %]</b></th> > > Use CSS white-space: nowrap instead here. css doesn't work in all email clients, such as gmail, so switching to css here wouldn't be a good idea :(

(In reply to Byron Jones ‹:glob› from comment #3) > > Use CSS white-space: nowrap instead here. > > css doesn't work in all email clients, such as gmail, so switching to css > here wouldn't be a good idea :( Ah right. Still learning the nuances of each email client so will keep that in mind better for future reviews. dkl

Comment on attachment 636699 [details] [diff] [review] patch v2 Review of attachment 636699 [details] [diff] [review]: ----------------------------------------------------------------- Looks great. All changes can be done on commit. r=dkl ::: Bugzilla/BugMail.pm @@ +490,4 @@ > ON fielddefs.id = bugs_activity.fieldid > WHERE bugs_activity.bug_id = ? > $when_restriction > + ORDER BY bugs_activity.bug_when, fielddefs.name", {Slice=>{}}, @args); Sort by fielddefs.description @@ +522,5 @@ > > + # always show the Bug ID field first, then Summary, > + # then remaining fields sorted by name. > + my @prepend; > + foreach my $name (qw(bug_id short_desc)) { Per IRC: foreach my $name (qw(bug_id short_desc classification product component)) { @@ +527,5 @@ > + my $idx = firstidx { $_->name eq $name } @fields; > + push(@prepend, $fields[$idx]); > + splice(@fields, $idx, 1); > + } > + @fields = sort { $a->name cmp $b->name } @fields; Sorty by $a->description cmp $b->description ::: extensions/BMO/template/en/default/email/bugmail.html.tmpl @@ +104,5 @@ > + <table border="0" cellspacing="0" cellpadding="3"> > + [% FOREACH change = diffs %] > + [% PROCESS "email/bugmail-common.txt.tmpl" %] > + <tr> > + <td class="c1" nowrap><b>[% field_label FILTER html %]</b></th> s/<\/th>/<\/td>/

thanks dkl. the version i've committed has a few minor differences from the one you reviewed: - moved the "do not reply" customisations from the default bugzilla template to bmo's, and added it to the html part - reuse the same order of fields as 4.0 on the new bug email - added x-bugzilla-id to the headers Committing to: bzr+ssh://bjones%40mozilla.com@bzr.mozilla.org/bmo/4.2/ modified Bugzilla/BugMail.pm added extensions/BMO/template/en/default/email added extensions/BMO/template/en/default/email/bugmail-header.txt.tmpl added extensions/BMO/template/en/default/email/bugmail.html.tmpl added extensions/BMO/template/en/default/email/bugmail.txt.tmpl modified template/en/default/email/bugmail.txt.tmpl Committed revision 8217.