Page MenuHomePhabricator

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

Authored by epriestley on Oct 5 2018, 6:43 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Mar 27, 12:44 AM
Unknown Object (File)
Wed, Mar 27, 12:44 AM
Unknown Object (File)
Sun, Mar 24, 12:36 AM
Unknown Object (File)
Wed, Mar 20, 10:29 PM
Unknown Object (File)
Wed, Mar 20, 10:29 PM
Unknown Object (File)
Mon, Mar 11, 11:19 AM
Unknown Object (File)
Feb 24 2024, 4:57 PM
Unknown Object (File)
Feb 9 2024, 11:55 AM
Subscribers
None

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

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

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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