Closed
Bug 775275
Opened 12 years ago
Closed 12 years ago
Port HTML email support from Bugzilla 4.2 to BMO 4.0
Categories
(bugzilla.mozilla.org :: General, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: dkl, Assigned: dkl)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 4 obsolete files)
82.68 KB,
patch
|
glob
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
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•12 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•12 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 | ||
Comment 6•12 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•12 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•12 years ago
|
||
Ok this one should work better.
Attachment #648375 -
Attachment is obsolete: true
Attachment #650166 -
Flags: review?(glob)
Comment 11•12 years ago
|
||
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•12 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•12 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 14•12 years ago
|
||
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•12 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
Closed: 12 years ago
OS: Linux → All
Hardware: x86_64 → All
Resolution: --- → FIXED
Comment 16•12 years ago
|
||
don't forget to commit the fixes to the extensions to the bmo/4.2 branch :)
Comment 17•12 years ago
|
||
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•12 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.
Comment 19•12 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.
Comment 20•12 years ago
|
||
(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•12 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.
Comment 22•12 years ago
|
||
(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).
You need to log in
before you can comment on or make changes to this bug.
Description
•