Page MenuHomePhabricator

HTML emails
ClosedPublic

Authored by talshiri on Jun 4 2014, 8:35 PM.

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
Lint
Lint Skipped
Unit
Unit Tests Skipped

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).

Looks kinda like this

chad added a subscriber: chad.Jun 19 2014, 6:21 PM

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.

talshiri updated this revision to Diff 23493.Jun 30 2014, 9:48 PM

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)Jul 9 2014, 9:15 PM
talshiri edited the test plan for this revision. (Show Details)
talshiri edited the test plan for this revision. (Show Details)
talshiri updated this revision to Diff 24409.Aug 5 2014, 3:01 AM
talshiri edited the test plan for this revision. (Show Details)

few fixes + rebase

talshiri edited the test plan for this revision. (Show Details)Aug 12 2014, 1:56 AM

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?

talshiri updated this revision to Diff 24708.Aug 14 2014, 4:52 AM
  • 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.
talshiri updated this revision to Diff 24710.Aug 14 2014, 4:54 AM

really fix it

Harbormaster completed remote builds in B2222: Diff 24710.
epriestley edited edge metadata.Aug 14 2014, 2:28 PM

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
1182–1183

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
562

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 requested changes to this revision.Aug 14 2014, 2:41 PM
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
45–47

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

81–85

Use an array, not string concatenation:

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

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
160

Label this control and add explanatory text.

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

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)?

2120

Use phutil_implode_html().

This revision now requires changes to proceed.Aug 14 2014, 2:41 PM
chad awarded a token.Aug 14 2014, 2:43 PM

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

talshiri added inline comments.Aug 14 2014, 8:23 PM
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
156

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
2111

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 updated this revision to Diff 24727.Aug 14 2014, 8:24 PM
talshiri edited edge metadata.
  • cosmetics
talshiri updated this revision to Diff 24728.Aug 14 2014, 8:25 PM
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
156

Oh, sorry -- do this at the end:

hsprintf('%s', array($body, $br))
talshiri updated this revision to Diff 24733.Aug 14 2014, 11:31 PM
  • cleaned out escaping and removed colorful headers
talshiri updated this revision to Diff 24734.Aug 14 2014, 11:42 PM
  • removed server config option for html email
talshiri added inline comments.Aug 15 2014, 12:45 AM
src/applications/metamta/view/PhabricatorMetaMTAMailBody.php
158

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

epriestley accepted this revision.Aug 15 2014, 3:03 PM
epriestley edited edge metadata.

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

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

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 closed this revision.Aug 15 2014, 3:12 PM
epriestley updated this revision to Diff 24738.

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

talshiri added inline comments.Aug 15 2014, 6:55 PM
src/applications/metamta/view/PhabricatorMetaMTAMailBody.php
158

Oooh you can cast things!