775275 - Port HTML email support from Bugzilla 4.2 to BMO 4.0

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: General, defect)

Description

[David Lawrence [:dkl]]

In case we have to postpone the roll out of 4.2 much later than hoped, we will see what work would be involved backporting the HTML support from 4.2 to 4.0. Once work has been done and test, we can roll it out as an incremental change during one of our weekly updates.

Comment 1

[Reed Loden [:reed]]

Is this really that big of a priority/goal? I can name a bunch of other stuff that would be far more useful to folks *right now* than having HTML bugmail.

Comment 2

[:glob ✱]

(In reply to Reed Loden [:reed] from comment #1)
> Is this really that big of a priority/goal? I can name a bunch of other
> stuff that would be far more useful to folks *right now* than having HTML
> bugmail.

yes, it's by far the thing people are most excited about in 4.2, and i'm not expecting it to be a large amount of work.

Comment 3

[David Lawrence [:dkl]]

Attachment: Patch to port HTML email to bmo/4.0 (v1)

Not a small patch unfortunately :( but wasn't too difficult. Mostly it is pulling over the related templates from 4.2 and making BugMail.pm work. I also pulled in the SecureMail changes as well for multipart email support. So you probably don't need to spend too much time on that part. I have tested what I know to test but please try it yourself.

dkl

Comment 4

[:glob ✱]

Comment on attachment 644493 [details] [diff] [review]
Patch to port HTML email to bmo/4.0 (v1)

Review of attachment 644493 [details] [diff] [review]:
-----------------------------------------------------------------

changes to extensions/ComponentWatching/Extension.pm and extensions/LimitedEmail/Config.pm appear to be accidentally included.

the file template/en/default/email/bugmail-common.txt.html can be removed from the patch.

i get a warning when sending email (non-encrypted):

Parsing of undecoded UTF-8 will give garbage when decoding entities at /usr/local/share/perl5/HTML/TreeBuilder.pm line 121.
  at /usr/local/share/perl5/HTML/TreeBuilder.pm line 121
  HTML::TreeBuilder::parse_content(...) called at ./extensions/SecureMail/Extension.pm line 533
  Bugzilla::Extension::SecureMail::__ANON__(...) called at /usr/local/share/perl5/Email/MIME.pm line 718
  Email::MIME::__ANON__(...) called at /usr/local/share/perl5/Email/MIME.pm line 721
  Email::MIME::__ANON__(...) called at /usr/local/share/perl5/Email/MIME.pm line 728
  Email::MIME::walk_parts(...) called at ./extensions/SecureMail/Extension.pm line 545
  Bugzilla::Extension::SecureMail::_filter_bug_links(...) called at ./extensions/SecureMail/Extension.pm line 303 
  Bugzilla::Extension::SecureMail::mailer_before_send(...) called at Bugzilla/Hook.pm line 33
  Bugzilla::Hook::process(...) called at Bugzilla/Mailer.pm line 174 \tBugzilla::Mailer::MessageToMTA(...) called at Bugzilla/BugMail.pm line 433 
  Bugzilla::BugMail::sendMail(...) called at Bugzilla/BugMail.pm line 323 
  Bugzilla::BugMail::Send(...) called at Bugzilla/Bug.pm line 1201 
  Bugzilla::Bug::_send_bugmail(...) called at Bugzilla/Bug.pm line 1150 
  Bugzilla::Bug::send_changes(...) called at /opt/bugzilla/htdocs/775275/process_bug.cgi line 393

i don't know why i didn't see it when those changes were committed to securemail (probably didn't look), but we'll need to fix them before we put this on bmo.  i suspect using body_str/body_str_set should do it.

i realised while testing this the ComponentWatcher extension will need updating due to the changes to $diffs passed to bugmail_recipients -- may as well update that here too :)

::: extensions/SecureMail/Config.pm
@@ +43,5 @@
> +    {
> +         package => 'HTML-Tree', 
> +         module  => 'HTML::Tree', 
> +         version => 0, 
> +    }

please file a bug which blocks this to get this module installed onto the production webheads.

this patch cannot be committed until that module has been installed.

::: extensions/SecureMail/Extension.pm
@@ +528,5 @@
> +    my ($email) = @_;
> +    $email->walk_parts(sub {
> +        my $part = shift;
> +        return if $part->content_type !~ /text\/html/;
> +        my$body = $part->body;

missing space after my, and that should probably be $part->body_str

@@ +530,5 @@
> +        my $part = shift;
> +        return if $part->content_type !~ /text\/html/;
> +        my$body = $part->body;
> +        my $tree = HTML::Tree->new->parse_content($body);
> +        my @links = $tree->look_down( _tag  => q{a}, class => qr/bz_bug_link/ );

you need to update Template::get_bug_link to include the bz_bug_link class so securemail can find them.

@@ +540,5 @@
> +                $link->attr('title', '(secure bug)');
> +                $link->attr('class', 'bz_bug_link');
> +            }
> +        }
> +        $part->body_set($tree->as_HTML);

and body_str_set here.

Comment 5

[David Lawrence [:dkl]]

(In reply to Byron Jones ‹:glob› from comment #4)

> changes to extensions/ComponentWatching/Extension.pm and
> extensions/LimitedEmail/Config.pm appear to be accidentally included.

LimitedEmail was a mistake in the patch that I changed when testing the email with my client.
ComponentWatching needs to be there as the format of the diffs passed in has changed and ComponentWatching uses that.

dkl

Comment 6

[David Lawrence [:dkl]]

Attachment: Patch to port HTML email to bmo/4.0 (v2)

New patch with issues addressed.

dkl

Comment 7

[:glob ✱]

Comment on attachment 648375 [details] [diff] [review]
Patch to port HTML email to bmo/4.0 (v2)

> # Creates a link to an attachment, including its title.
> sub get_attachment_link {
>-    my ($attachid, $link_text) = @_;
>+    my ($attachid, $link_text, $user) = @_;
>     my $dbh = Bugzilla->dbh;
>-    my $user = Bugzilla->user;
>+    $user ||= Bugzilla->user;

you missed the other change to get_attachment_link to use the $user object instead of Bugzilla->user


i still see "Parsing of undecoded UTF-8 will give garbage when decoding entities" warnings, and now unicode is garbled:

> from Byron Jones ‹:glob› at

Comment 8

[David Lawrence [:dkl]]

(In reply to Byron Jones ‹:glob› from comment #7)
> you missed the other change to get_attachment_link to use the $user object
> instead of Bugzilla->user

The rest of the changes from the security advisory patch we already have in bmo/4.0. We just were using the current Bugzilla->user so that part is in the patch.

dkl

Comment 9

[:glob ✱]

(In reply to David Lawrence [:dkl] from comment #8)
> (In reply to Byron Jones ‹:glob› from comment #7)
> > you missed the other change to get_attachment_link to use the $user object
> > instead of Bugzilla->user
> 
> The rest of the changes from the security advisory patch we already have in
> bmo/4.0. We just were using the current Bugzilla->user so that part is in
> the patch.

ah, sorry, i forgot to |bzr up| before applying your updated patch.

Comment 10

[David Lawrence [:dkl]]

Attachment: Patch to port HTML email to bmo/4.0 (v3)

Ok this one should work better.

Comment 11

[:glob ✱]

Comment on attachment 650166 [details] [diff] [review]
Patch to port HTML email to bmo/4.0 (v3)

this patch is missing all the bugmail template files, so i can't give this an r+ as there may be other changes in those files :(  i'll test with the bugmail templates from v2.

note - as per our meeting, need to change the default value for the setting from html to text when creating the setting.

hrm.. something is adding an ! at character 909 of the html.
... title="(!
...v></body>!

(investigating)

after the html part is updated with _filter_bug_links, we end up with extremely long lines, and the part isn't marked as binary, so it's triggering this problem.

changing:
> $part->body_set($tree->as_HTML)
to:
> $body = $tree->as_HTML;
> $part->body_set($body);
> $part->charset_set('UTF-8') if Bugzilla->params->{'utf8'};
> $part->encoding_set('quoted-printable');
fixes this as using quoted-printable forces the html to be wrapped correctly.

otherwise i think we're good to go here.

Comment 12

[David Lawrence [:dkl]]

Attachment: Patch to port HTML email to bmo/4.0 (v4)

Proper patch this time. Also applied change to _filter_bug_links. Performing testing now.

dkl

Comment 13

[David Lawrence [:dkl]]

Attachment: Patch to port HTML email to bmo/4.0 (v5)

Removed LimitedEmail extension changes from last patch.

dkl

Comment 14

[:glob ✱]

Comment on attachment 654243 [details] [diff] [review]
Patch to port HTML email to bmo/4.0 (v5)

r=glob

Comment 15

[David Lawrence [:dkl]]

Committing to: bzr+ssh://[e-mail]/bmo/4.0
modified .htaccess
modified Bugzilla.pm
modified Bugzilla/BugMail.pm
modified Bugzilla/Hook.pm
modified Bugzilla/Install.pm
modified Bugzilla/Mailer.pm
modified Bugzilla/Template.pm
modified Bugzilla/User.pm
modified extensions/BMO/Extension.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 extensions/ComponentWatching/Extension.pm
modified extensions/SecureMail/Config.pm
modified extensions/SecureMail/Extension.pm
modified skins/standard/global.css
added template/en/default/email/bugmail-common.txt.tmpl
added template/en/default/email/bugmail-header.txt.tmpl
added template/en/default/email/bugmail.html.tmpl
modified template/en/default/email/newchangedmail.txt.tmpl
modified template/en/default/global/setting-descs.none.tmpl
Committed revision 8280.

Comment 16

[:glob ✱]

don't forget to commit the fixes to the extensions to the bmo/4.2 branch :)

Comment 17

[:glob ✱]

this caused an internal server error when creating bugs:

Can't locate object method "fields" via package "Bugzilla" at Bugzilla/BugMail.pm line 527

4.2 replaced get_fields() with fields().  to get the fix in quickly, i ported 4.2's fields() directly across and made no other changes.

Committing to: bzr+ssh://[e-mail]/bmo/4.0-dev/
modified skins/contrib/Mozilla/global.css
Committed revision 7964.

Comment 18

[David Lawrence [:dkl]]

(In reply to Byron Jones ‹:glob› from comment #16)
> don't forget to commit the fixes to the extensions to the bmo/4.2 branch :)

Done.

Committing to: bzr+ssh://[e-mail]/bmo/4.2
modified extensions/SecureMail/Config.pm
modified extensions/SecureMail/Extension.pm
Committed revision 8313.

Comment 19

[Daniel C]

In Gmail, the text for comment headers, and flag / field changes seems OK, but the text for the actual comments is a little on the small side I would say...

Using the Inspector to look at computed styles I can see:

Plain text e-mails / current non-comment text: font-size: 12.8px;

Comment text: font-size: 9.6px;

I think Gmail is styling the <pre> tag to have a small font-size.

Comment 20

[:glob ✱]

(In reply to Daniel Cater from comment #19)
> In Gmail, the text for comment headers, and flag / field changes seems OK,
> but the text for the actual comments is a little on the small side I would
> say...

we aren't setting the font size of comments, we leave it up to the mail client to use its default.  it looks fine in most other clients.

we are unable to fix gmail's display, as it ignores styles.

Comment 21

[Daniel C]

(In reply to Byron Jones ‹:glob› from comment #20)
> (In reply to Daniel Cater from comment #19)
> > In Gmail, the text for comment headers, and flag / field changes seems OK,
> > but the text for the actual comments is a little on the small side I would
> > say...
> 
> we aren't setting the font size of comments, we leave it up to the mail
> client to use its default.  it looks fine in most other clients.
> 
> we are unable to fix gmail's display, as it ignores styles.

Gmail supposedly suports inline CSS for font-size: http://www.campaignmonitor.com/css/

Given how many Gmail users there are using Bugzilla I think it makes sense to fix this problem.

Comment 22

[:glob ✱]

(In reply to Daniel Cater from comment #21)
> Given how many Gmail users there are using Bugzilla I think it makes sense
> to fix this problem.

please file a new bug for this, so we can investigate if it's possible to change this without impacting non-gmail users (where the font size looks right currently).