Page MenuHomePhabricator

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

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

Details

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

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Oct 5 2018, 6:43 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.

This revision was automatically updated to reflect the committed changes.