Page MenuHomePhabricator

HTML emails for Calendar event description changes should respect remarkup rules
ClosedPublic

Authored by lpriestley on Jul 5 2015, 5:26 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Dec 17, 5:54 AM
Unknown Object (File)
Thu, Dec 12, 7:38 PM
Unknown Object (File)
Thu, Dec 12, 3:08 AM
Unknown Object (File)
Tue, Dec 10, 6:39 PM
Unknown Object (File)
Mon, Dec 2, 9:38 AM
Unknown Object (File)
Mon, Dec 2, 9:38 AM
Unknown Object (File)
Mon, Dec 2, 9:38 AM
Unknown Object (File)
Mon, Dec 2, 9:38 AM
Subscribers

Details

Summary

Ref T7964, HTML emails for Calendar event description changes should respect remarkup rules

Test Plan

Create event and edit description, check that email has a correctly formatted remarkup description section.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

lpriestley retitled this revision from to HTML emails for Calendar event description changes should respect remarkup rules.
lpriestley updated this object.
lpriestley edited the test plan for this revision. (Show Details)
lpriestley added a reviewer: epriestley.
epriestley edited edge metadata.

Sorry, I think I led you astray a bit here.

I think the call to addRemarkupSection() in PhabricatorApplicationTransactionEditor also needs to be converted.

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

I think we can actually toss this..

52–53

...and do addPlaintextSection() here. As written, this isn't correct -- it adds HTML to the "plaintext" sections.

66

..and addHTMLSection(...) here.

68–69

...and addHTMLSection() here too.

120

Then this can be left as-is..

218

And this can be removed.

This revision now requires changes to proceed.Jul 6 2015, 6:08 PM
src/applications/metamta/view/PhabricatorMetaMTAMailBody.php
52–53

But it previously added "styled_text"? Seems counterintuitive to add plaintext by default in a remarkup section?

lpriestley edited edge metadata.

Updating PhabricatorApplicationTransactionEditor as well

epriestley edited edge metadata.

This looks correct to me, except that I think the bold for headers got stripped out and never gets added now?

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

It looks like we lost the bold here?

This revision now requires changes to proceed.Nov 4 2015, 6:42 PM
src/applications/metamta/view/PhabricatorMetaMTAMailBody.php
129

Trying to recall why I removed the phutil_tag. I think this was intentional. What's the right way to make this bold without a phutil_tag?

There's no way to make it bold without a phutil_tag().

My guess is that you might have removed it because the header is sometimes null, so it would have rendered an empty tag. Maybe something like this?

if ($header !== null) {
  $header = phutil_tag(...);
}

...but I'm not sure.

lpriestley edited edge metadata.

Adding back bold headers to mail

epriestley edited edge metadata.
This revision is now accepted and ready to land.Nov 7 2015, 3:23 PM
This revision was automatically updated to reflect the committed changes.