Page MenuHomePhabricator

Process Remarkup in text and HTML email bodies appropriately
ClosedPublic

Authored by lpriestley on Nov 15 2014, 7:49 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Feb 18, 7:52 PM
Unknown Object (File)
Sun, Feb 18, 3:37 PM
Unknown Object (File)
Sat, Feb 10, 8:42 PM
Unknown Object (File)
Sat, Feb 10, 6:30 PM
Unknown Object (File)
Sat, Feb 10, 11:03 AM
Unknown Object (File)
Sat, Feb 10, 6:18 AM
Unknown Object (File)
Mon, Feb 5, 11:36 PM
Unknown Object (File)
Sat, Feb 3, 7:54 PM
Tokens
"Yellow Medal" token, awarded by chad.

Details

Summary

Ref T6343, adding HTMLMailMode to remarkup, and most objects should now be processed and appear pretty in emails.

Test Plan

Add a comment to a Maniphest task containing a mention of an object like '{T1}' or 'T1'. Emails should show a styled version of the object similar to how the object looks in the context of the Maniphest task in the UI.

Diff Detail

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

Event Timeline

lpriestley retitled this revision from to Process Remarkup in text and HTML email bodies appropriately.
lpriestley updated this object.
lpriestley edited the test plan for this revision. (Show Details)
lpriestley added a reviewer: epriestley.
epriestley edited edge metadata.

We should fix the $viewer thing, see inline. Looks great otherwise.

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

We should use the actual viewer for these (the user who is currently logged in).

To make it available:

  • Add a $viewer property to this class with a getter + setter.
  • Call setViewer($this->requireActor()) when the MailBody object is created inside the PhabricatorApplicationTransactionEditor.
  • Then use $this->getViewer() to pass the correct viewer.
44

Same as above.

src/applications/paste/mail/PasteCreateMailReceiver.php
75

This isn't technically remarkup, it's always just the string "You successfully created a paste."

It doesn't really matter if we add this as a raw or remarkup section, but sticking with raw is maybe cleaner.

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
2161–2162

This is where you can setViewer($this->requireActor()) to make the appropriate user account available inside the mail body.

src/infrastructure/markup/rule/PhabricatorObjectRemarkupRule.php
66–68

Consider passing $status or $is_closed into renderObjectTagForMail() to clean this up a little bit more.

This revision now requires changes to proceed.Nov 15 2014, 8:01 PM
src/infrastructure/markup/rule/PhabricatorObjectRemarkupRule.php
66–68

I was debating this. My thought was that the standalone acetic html tag can be re-used for things besides emails, and perhaps closed might not be a property in that case. If you don't anticipate such a case, your way makes more sense to me.

Yeah I don't know if the objects like T2000 need to look the same in email. I'd try just plain old anchor for now and revisit if needed.

epriestley edited edge metadata.

Okay, here are the issues I caught with rules that are actually broken; we should fix these:

  • The icon rule ({icon paw} produces ) throws an exception. (This one is probably really hard to make work in HTML email.) I think it would be OK to just render {icon paw} or similar for now.
  • The macro rule produces image tags with relative URIs (starting with / instead of http://...).
  • The diviner rule (@{article:example}) produces links with relative URIs.

Here are the minor issues I caught with rules that are iffy, but not obvious to the user and don't need to be fixed for now:

  • The nav sequence rule ({nav a > b > c}) works OK but produces markup with class attributes.
  • The username mention rule (@username) produces markup with class attributes.
  • The interpreter block rules like cowsay produces markup with class attributes.

See inline for the try/catch thing.

This revision now requires changes to proceed.Nov 17 2014, 7:38 PM

durr

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

In case we missed any exceptions, let's do this:

  • Put try/catch around this.
  • If we catch any Exception, call:
} catch (Exception $ex) {
  phlog($ex);
  $this->htmlSections[] = phutil_escape_html_newlines(phutil_tag('div', array(), $text));
}

That will use the old mode (just dump the remarup into the mail as text) for anything we break on, and log the exception. This should prevent this from causing any major issues, and we can clean up the minor stuff ("this mail could look nicer") over time / as issues arise.

lpriestley edited edge metadata.

Adding try/catch for html mail styling. Fixing a few rules to leave out class attributes when composing email body.

lpriestley edited edge metadata.

removing .gitignore

epriestley edited edge metadata.

Cool, looks good to me. Thanks!

This revision is now accepted and ready to land.Nov 18 2014, 2:25 AM
This revision was automatically updated to reflect the committed changes.