Feed - fix some whacky "text mode" rendering code

Summary: ...add a "renderingTarget" to FeedStory and use it as appropos. Overall, not a ton of changes was necessary to make this work. I think this could be made to be even cleaner by going through each and every feed story and re-implementing as necessary with the full toolset available. But this is good enough for now I think, and just something to keep cleaning up when we're in here. Fixes T4630.

Test Plan: made a task. gave it a token. viewed my feed - saw stories. used conduit.feed.query with mode == 'text' and got good readable results.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: spicyj, epriestley, Korvin

Maniphest Tasks: T4630

Differential Revision: https://secure.phabricator.com/D8750

src/applications/feed/story/PhabricatorFeedStory.php

@@ -14,6 +14,7 @@ abstract class PhabricatorFeedStory implements PhabricatorPolicyInterface {
   private $hasViewed;
   private $framed;
   private $hovercard = false;
+  private $renderingTarget = PhabricatorApplicationTransaction::TARGET_HTML;
 
   private $handles  = array();
   private $objects  = array();
@@ -124,6 +125,27 @@ abstract class PhabricatorFeedStory implements PhabricatorPolicyInterface {
     return $this;
   }
 
+  public function setRenderingTarget($target) {
+    $this->validateRenderingTarget($target);
+    $this->renderingTarget = $target;
+    return $this;
+  }
+
+  public function getRenderingTarget() {
+    return $this->renderingTarget;
+  }
+
+  private function validateRenderingTarget($target) {
+    switch ($target) {
+      case PhabricatorApplicationTransaction::TARGET_HTML:
+      case PhabricatorApplicationTransaction::TARGET_TEXT:
+        break;
+      default:
+        throw new Exception('Unknown rendering target: '.$target);
+        break;
+    }
+  }
+
   public function setObjects(array $objects) {
     $this->objects = $objects;
     return $this;
@@ -227,16 +249,30 @@ abstract class PhabricatorFeedStory implements PhabricatorPolicyInterface {
   }
 
   final protected function renderHandleList(array $phids) {
-    $list = array();
+    $items = array();
     foreach ($phids as $phid) {
-      $list[] = $this->linkTo($phid);
+      $items[] = $this->linkTo($phid);
     }
-    return phutil_implode_html(', ', $list);
+    $list = null;
+    switch ($this->getRenderingTarget()) {
+      case PhabricatorApplicationTransaction::TARGET_TEXT:
+        $list = implode(', ', $items);
+        break;
+      case PhabricatorApplicationTransaction::TARGET_HTML:
+        $list = phutil_implode_html(', ', $items);
+        break;
+    }
+    return $list;
   }
 
   final protected function linkTo($phid) {
     $handle = $this->getHandle($phid);
 
+    switch ($this->getRenderingTarget()) {
+      case PhabricatorApplicationTransaction::TARGET_TEXT:
+        return $handle->getLinkName();
+    }
+
     // NOTE: We render our own link here to customize the styling and add
     // the '_top' target for framed feeds.
 
@@ -258,14 +294,23 @@ abstract class PhabricatorFeedStory implements PhabricatorPolicyInterface {
   }
 
   final protected function renderString($str) {
+    switch ($this->getRenderingTarget()) {
+      case PhabricatorApplicationTransaction::TARGET_TEXT:
+        return $str;
+      case PhabricatorApplicationTransaction::TARGET_HTML:
         return phutil_tag('strong', array(), $str);
     }
+  }
 
   final protected function renderSummary($text, $len = 128) {
     if ($len) {
       $text = phutil_utf8_shorten($text, $len);
     }
+    switch ($this->getRenderingTarget()) {
+      case PhabricatorApplicationTransaction::TARGET_HTML:
         $text = phutil_escape_html_newlines($text);
+        break;
+    }
     return $text;
   }
 

src/applications/maniphest/controller/ManiphestTransactionSaveController.php

@@ -152,11 +152,11 @@ final class ManiphestTransactionSaveController extends ManiphestController {
     if ($action == ManiphestTransaction::TYPE_OWNER) {
       if ($task->getOwnerPHID() == $transaction->getNewValue()) {
         // If this is actually no-op, don't generate the side effect.
-        break;
-      }
+      } else {
         // Otherwise, when a task is reassigned, move the previous owner to CC.
         $added_ccs[] = $task->getOwnerPHID();
       }
+    }
 
     if ($did_scuttle || ($action == ManiphestTransaction::TYPE_STATUS)) {
       if (!$task->getOwnerPHID() &&

src/applications/tokens/feed/PhabricatorTokenGivenFeedStory.php

@@ -28,27 +28,29 @@ final class PhabricatorTokenGivenFeedStory
     $href = $this->getHandle($this->getPrimaryObjectPHID())->getURI();
     $view->setHref($href);
 
-    $token = $this->getObject($this->getValue('tokenPHID'));
+    $view->setTitle($this->renderTitle());
+    $view->setImage($this->getHandle($author_phid)->getImageURI());
 
+    return $view;
+  }
+
+  private function renderTitle() {
+    $token = $this->getObject($this->getValue('tokenPHID'));
     $title = pht(
       '%s awarded %s a %s token.',
       $this->linkTo($this->getValue('authorPHID')),
       $this->linkTo($this->getValue('objectPHID')),
       $token->getName());
 
-    $view->setTitle($title);
-    $view->setImage($this->getHandle($author_phid)->getImageURI());
-
-    return $view;
+    return $title;
   }
 
   public function renderText() {
-    // TODO: This is grotesque; the feed notification handler relies on it.
-    return htmlspecialchars_decode(
-      strip_tags(
-        hsprintf(
-          '%s',
-          $this->renderView()->getTitle())));
+    $old_target = $this->getRenderingTarget();
+    $this->setRenderingTarget(PhabricatorApplicationTransaction::TARGET_TEXT);
+    $title = $this->renderTitle();
+    $this->setRenderingTarget($old_target);
+    return $title;
   }
 
 }

src/applications/transactions/feed/PhabricatorApplicationTransactionFeedStory.php

@@ -65,12 +65,13 @@ class PhabricatorApplicationTransactionFeedStory
   }
 
   public function renderText() {
-    // TODO: This is grotesque; the feed notification handler relies on it.
-    return htmlspecialchars_decode(
-      strip_tags(
-        hsprintf(
-          '%s',
-          $this->renderView()->getTitle())));
+    $xaction = $this->getPrimaryTransaction();
+    $old_target = $xaction->getRenderingTarget();
+    $new_target = PhabricatorApplicationTransaction::TARGET_TEXT;
+    $xaction->setRenderingTarget($new_target);
+    $text = $xaction->getTitleForFeed($this);
+    $xaction->setRenderingTarget($old_target);
+    return $text;
   }
 
 }