Page MenuHomePhabricator

HTML emails
ClosedPublic

Authored by talshiri on Jun 4 2014, 8:35 PM.
Tags
None
Referenced Files
F12815844: D9375.id24708.diff
Thu, Mar 28, 3:14 AM
Unknown Object (File)
Thu, Mar 21, 9:14 AM
Unknown Object (File)
Wed, Mar 20, 5:25 PM
Unknown Object (File)
Sun, Mar 17, 4:42 PM
Unknown Object (File)
Fri, Mar 15, 7:03 PM
Unknown Object (File)
Fri, Mar 15, 5:19 PM
Unknown Object (File)
Fri, Mar 15, 3:42 AM
Unknown Object (File)
Fri, Mar 15, 3:42 AM
Tokens
"Evil Spooky Haunted Tree" token, awarded by chad.

Details

Summary

Added support for side-by-side HTML and plaintext email building.

We can control if the HTML stuff is sent by by a new config, metamta.html-emails

Test Plan

Been running this in our deployment for a few months now.

Well behaved clients
  • Gmail
  • Mail.app
Bad clients
  • Airmail. They confuse Gmail too, though.
Need testing
  • Outlook (Windows + Mac)

Diff Detail

Repository
rP Phabricator
Branch
html_emails
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 1472
Build 1472: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Is there any interest in this patch?
We've been dogfooding it for a while now, and it works pretty well (we also reply from emails a lot).

We're likely 'sorta' interested in it, but not likely to find time to review/support it for a while. Our main concern will likely be support costs to the upstream.

@chad: yeah, that's fair. It also needs testing across more email clients to verify that it doesn't break replies and threading.

added support for colorized diffs for inline comments
added PhabricatorMetaMTAMailSection helper for building email sections

talshiri edited the test plan for this revision. (Show Details)
talshiri edited the test plan for this revision. (Show Details)
talshiri edited the test plan for this revision. (Show Details)

few fixes + rebase

I can test Outlook (Windows) rendering

Awesome! Can you arc patch D9375 locally and see if:

  1. rendering looks reasonable
  2. reply-by-email (especially to Differential which has additional html) doesn't introduce the quoted text problem seen on T185
In D9375#45, @talshiri wrote:

Awesome! Can you arc patch D9375 locally and see if:

  1. rendering looks reasonable

Will be back at a windows machine tomorrow, and definitely test this part

  1. reply-by-email (especially to Differential which has additional html) doesn't introduce the quoted text problem seen on T185

I don't currently have an inbound email setup configured, but I'll see what I can do here

Actually I think I can test the rendering with something like litmus, so it should be pretty easy to verify this one.

The main thing that needs testing is the reply by mail. Do you happen to have a Mailgun/AWS/Sendgrid account?

  • added per-user config and removed diff colorization (will come back again in a separate patch)
talshiri retitled this revision from HTML emails + basic DIFF colorizing to HTML emails.Aug 14 2014, 4:52 AM
talshiri updated this object.
talshiri removed a reviewer: chad.

Some really nitpicky inlines.

Some of the handling of HTML looks like it should be broken to me. In particular, I would expect treating phutil_tag()s like strings to produce too much escaping.

I think the HTML APIs should use the HTML rendering code (i.e., accept phutil_tag(), not plain strings), and this mostly does that correctly, but I would expect stuff like this:

$body->addRawHTMLSection(implode('<br>', $html_headers));

...to fail (applying too much escaping). Let me poke at this locally and see if I'm crazy...

src/applications/config/option/PhabricatorMetaMTAConfigOptions.php
335

I think we should avoid introducing this option for now so we can default it to true when we do introduce it. Changing defaults later is a little sketchier than just adding a new option to control a new behavior.

343–344

Missing and/or extra words, "HTML is enabled reply addresses"

src/applications/differential/editor/DifferentialTransactionEditor.php
1167–1168

Prefer $patch_section.

src/applications/metamta/adapter/PhabricatorMailImplementationPHPMailerLiteAdapter.php
78

Prefer Javadoc comments, with /** on its own line.

/**
 * blah blah
 */
src/applications/metamta/management/PhabricatorMailManagementSendTestWorkflow.php
123

Maybe something like:

pht('(This is a placeholder plaintext email body for a test message sent with --html.)');
src/applications/metamta/storage/PhabricatorMetaMTAMail.php
542

Oh -- although it's outside of the scope of this diff, this isn't quite correct. I can send you a followup to fix it.

epriestley edited edge metadata.

It looks like this narrowly escapes actually breaking anything, but the handling isn't quite correct, and will break if we make minor changes in the future (like wrapping the entire mail body in a <div> to apply styling in renderHTML(). See inlines to clean this up.

src/applications/metamta/view/PhabricatorMetaMTAMailBody.php
46–48

strlen() on a phutil_tag() isn't meaningful -- just add this section unconditionally? These strlen() checks probably aren't really necessary anyway.

84–88

Use an array, not string concatenation:

$this->htmlSections[] = array(
  phutil_tag(...),
  $html_fragment,
);
161

Use:

$br = phutil_tag('br');
$body = phutil_implode_html(array($br, $br), $this->htmlSections);
return array($body, $br);
src/applications/metamta/view/PhabricatorMetaMTAMailSection.php
16

Just return $this->htmlFragments.

20

A \n inside single quotes is not a newline in PHP, it's a literal backslash + n.

src/applications/settings/panel/PhabricatorSettingsPanelEmailFormat.php
162 ↗(On Diff #24710)

Label this control and add explanatory text.

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
2015

Not all transactions have colors, this sometimes produces style="color:".

The transaction colors are not all HTML colors.

This also seems super annoying, design-wise. Let's just drop it and we can find some more tasteful approach later (like a colored bullet)?

2027

Use phutil_implode_html().

This revision now requires changes to proceed.Aug 14 2014, 2:41 PM

Also, let's put an explicit caveat on the option in the settings panel about it being beta/not-expected-to-work-perfectly.

src/applications/config/option/PhabricatorMetaMTAConfigOptions.php
335

So should I remove the checks? I've just defaulted it to true for now

343–344

I have no idea what I was thinking

src/applications/metamta/view/PhabricatorMetaMTAMailBody.php
161

clients expect renderHTML() to a return a string, no an array

src/applications/metamta/view/PhabricatorMetaMTAMailSection.php
20

I totally should've written test cases for this class.

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
2015

Yeah, it's pretty crude but was necessary for us due to the long list of actions Phabricator sends ("X added a reviewer: X", "X added a subscriber: X", "X accepted your review").
This can probably be refined and addressed in other ways though. Nuking it.

talshiri edited edge metadata.
  • cosmetics
talshiri edited edge metadata.

my arcanist seems to be messed up?

Yeah, just remove the check against the config option for now. Once this is stable we can add the option and default it to true (unless we discover that, say, several common clients have major problems we aren't able to fix, in which case we might default it to false).

src/applications/metamta/view/PhabricatorMetaMTAMailBody.php
161

Oh, sorry -- do this at the end:

hsprintf('%s', array($body, $br))
  • cleaned out escaping and removed colorful headers
  • removed server config option for html email
src/applications/metamta/view/PhabricatorMetaMTAMailBody.php
163

This is a weird hack to force it to emit a string. a PhutilSafeHTML doesn't seem to get serialized?

epriestley edited edge metadata.

One inline. I'll tweak that in the pull if it seems to wokr.

src/applications/metamta/view/PhabricatorMetaMTAMailBody.php
163

This will probably work and is a little more conventional:

return (string)hsprintf('%s', array($body, $br));
This revision is now accepted and ready to land.Aug 15 2014, 3:03 PM
epriestley updated this revision to Diff 24738.

Closed by commit rP4c57e6d34d7a (authored by @talshiri, committed by @epriestley).

src/applications/metamta/view/PhabricatorMetaMTAMailBody.php
163

Oooh you can cast things!