Port HTML email support from Bugzilla 4.2 to BMO 4.0

RESOLVED FIXED

Status

()

RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: dkl, Assigned: dkl)

Tracking

(Blocks: 1 bug)

Production
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

7 years ago
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.
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.
(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.
(Assignee)

Comment 3

7 years ago
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
Attachment #644493 - Flags: review?(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.
Attachment #644493 - Flags: review?(glob) → review-
(Assignee)

Comment 5

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

Updated

7 years ago
Depends on: 779903
(Assignee)

Comment 6

7 years ago
New patch with issues addressed.

dkl
Attachment #644493 - Attachment is obsolete: true
Attachment #648375 - Flags: review?(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
Attachment #648375 - Flags: review?(glob) → review-
(Assignee)

Comment 8

7 years ago
(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
(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.
(Assignee)

Comment 10

7 years ago
Ok this one should work better.
Attachment #648375 - Attachment is obsolete: true
Attachment #650166 - Flags: review?(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.
Attachment #650166 - Flags: review?(glob) → review-
(Assignee)

Comment 12

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

dkl
Attachment #650166 - Attachment is obsolete: true
Attachment #654238 - Flags: review?(glob)
(Assignee)

Comment 13

7 years ago
Removed LimitedEmail extension changes from last patch.

dkl
Attachment #654238 - Attachment is obsolete: true
Attachment #654238 - Flags: review?(glob)
Attachment #654243 - Flags: review?(glob)
Comment on attachment 654243 [details] [diff] [review]
Patch to port HTML email to bmo/4.0 (v5)

r=glob
Attachment #654243 - Flags: review?(glob) → review+
(Assignee)

Comment 15

7 years ago
Committing to: bzr+ssh://dlawrence%40mozilla.com@bzr.mozilla.org/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.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
OS: Linux → All
Hardware: x86_64 → All
Resolution: --- → FIXED
don't forget to commit the fixes to the extensions to the bmo/4.2 branch :)
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://bjones%40mozilla.com@bzr.mozilla.org/bmo/4.0-dev/
modified skins/contrib/Mozilla/global.css
Committed revision 7964.
(Assignee)

Comment 18

7 years ago
(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://dlawrence%40mozilla.com@bzr.mozilla.org/bmo/4.2
modified extensions/SecureMail/Config.pm
modified extensions/SecureMail/Extension.pm
Committed revision 8313.
Blocks: 785308
No longer depends on: 785308

Comment 19

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

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

Updated

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