Make Pholio mail render without a ton of over-escaped HTML
ClosedPublic
Actions

Authored by epriestley on Oct 5 2018, 6:43 PM.

Details

Reviewers

amckinley

Maniphest Tasks

T12814: pholio transaction emails have too much html
T13202: Plans: 2018 Week 38-40 Bonus Content

Commits

rP99034efa8b8d: Make Pholio mail render without a ton of over-escaped HTML

Summary

Ref T13202. See PHI900. Fixes T12814. Pholio currently builds HTML comments in an older way that can dump a bunch of over-escaped HTML into mail bodies.

Update the logic to be more similar to the Differential rendering logic and stop over-escaping things.

The result isn't perfect, but is dramatically less broken.

Test Plan

Screen Shot 2018-10-05 at 11.40.39 AM.png (881×1 px, 96 KB)

Diff Detail

Repository

rP Phabricator

Branch

mock1

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 20990
Build 28514: Run Core Tests
Build 28513: arc lint + arc unit

Event Timeline

epriestley created this revision.Oct 5 2018, 6:43 PM

Harbormaster completed remote builds in B20990: Diff 47150.Oct 5 2018, 6:44 PM

epriestley requested review of this revision.Oct 5 2018, 6:44 PM

amckinley accepted this revision.Oct 5 2018, 8:08 PM

amckinley added inline comments.

src/applications/pholio/editor/PholioMockEditor.php
149	Why not call the new `isInlineCommentTransaction` method?

This revision is now accepted and ready to land.Oct 5 2018, 8:08 PM

epriestley marked an inline comment as done.Oct 5 2018, 8:17 PM

epriestley added inline comments.

src/applications/pholio/editor/PholioMockEditor.php
149	`isInlineCommentTransaction()` is really an existing method called by higher-level logic which currently means `shouldSuppressCommentFromOverallListOfCommentsAtTheTopOfEmail()`. Without it, `addHeadersAndCommentsToMailBody()` treats the inline comments as normal comments, and we get a big mess at the top of the mail like: alice did some stuff. alice added a comment. actual comment alice added a comment. inline comment alice added a comment. inline comment The `isInlineCommentTransaction()` method suppresses this, but other future or extension comments might want to suppress this behavior but also might not be renderable using the same "inline comment" logic. I think `isInlineCommentTransaction()` should probably be nuked and replaced with something more clear (and something that supports modular transactions) but didn't want to scope creep this change too much.