Partially modernize Doorkeeper/Asana bridge

Summary: Fixes T6201. This stuff didn't fully get updated for ApplicationTransactions. Get it working again (notably, make inline comment text publish) and clean it up a little bit. Test Plan: - Published a Differential feed story into Asana with comment text. - Pulbished a Diffusion feed story into Asana with comment text. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T6201 Differential Revision: https://secure.phabricator.com/D10584
2025-01-31 00:48:21 +01:00 · 2014-10-01 07:09:34 -07:00 · 2014-10-01 07:09:34 -07:00 · 3fe226f9f0
commit 3fe226f9f0
parent 97dd3dcb37
10 changed files with 121 additions and 215 deletions
--- a/src/applications/differential/doorkeeper/DifferentialDoorkeeperRevisionFeedStoryPublisher.php
+++ b/src/applications/differential/doorkeeper/DifferentialDoorkeeperRevisionFeedStoryPublisher.php
@ -86,18 +86,6 @@ final class DifferentialDoorkeeperRevisionFeedStoryPublisher
    return pht('%s Review Request', $prefix);
  }

-  public function getStoryText($object) {
-    $implied_context = $this->getRenderWithImpliedContext();
-
-    $story = $this->getFeedStory();
-    if ($story instanceof PhabricatorFeedStoryDifferential) {
-      $text = $story->renderForAsanaBridge($implied_context);
-    } else {
-      $text = $story->renderText();
-    }
-    return $text;
-  }
-
  private function getTitlePrefix(DifferentialRevision $revision) {
    $prefix_key = 'metamta.differential.subject-prefix';
    return PhabricatorEnv::getEnvConfig($prefix_key);
--- a/src/applications/differential/storage/DifferentialTransaction.php
+++ b/src/applications/differential/storage/DifferentialTransaction.php
@ -596,5 +596,59 @@ final class DifferentialTransaction extends PhabricatorApplicationTransaction {
    return parent::getNoEffectDescription();
  }

+  public function renderAsTextForDoorkeeper(
+    DoorkeeperFeedStoryPublisher $publisher,
+    PhabricatorFeedStory $story,
+    array $xactions) {
+
+    $body = parent::renderAsTextForDoorkeeper($publisher, $story, $xactions);
+
+    $inlines = array();
+    foreach ($xactions as $xaction) {
+      if ($xaction->getTransactionType() == self::TYPE_INLINE) {
+        $inlines[] = $xaction;
+      }
+    }
+
+    // TODO: This is a bit gross, but far less bad than it used to be. It
+    // could be further cleaned up at some point.
+
+    if ($inlines) {
+      $engine = PhabricatorMarkupEngine::newMarkupEngine(array())
+        ->setConfig('viewer', new PhabricatorUser())
+        ->setMode(PhutilRemarkupEngine::MODE_TEXT);
+
+      $body .= "\n\n";
+      $body .= pht('Inline Comments');
+      $body .= "\n";
+
+      $changeset_ids = array();
+      foreach ($inlines as $inline) {
+        $changeset_ids[] = $inline->getComment()->getChangesetID();
+      }
+
+      $changesets = id(new DifferentialChangeset())->loadAllWhere(
+        'id IN (%Ld)',
+        $changeset_ids);
+
+      foreach ($inlines as $inline) {
+        $comment = $inline->getComment();
+        $changeset = idx($changesets, $comment->getChangesetID());
+        if (!$changeset) {
+          continue;
+        }
+
+        $filename = $changeset->getDisplayFilename();
+        $linenumber = $comment->getLineNumber();
+        $inline_text = $engine->markupText($comment->getContent());
+        $inline_text = rtrim($inline_text);
+
+        $body .= "{$filename}:{$linenumber} {$inline_text}\n";
+      }
+    }
+
+    return $body;
+  }
+

 }
--- a/src/applications/diffusion/doorkeeper/DiffusionDoorkeeperCommitFeedStoryPublisher.php
+++ b/src/applications/diffusion/doorkeeper/DiffusionDoorkeeperCommitFeedStoryPublisher.php
@ -180,18 +180,6 @@ final class DiffusionDoorkeeperCommitFeedStoryPublisher
    return pht('%s Audit', $prefix);
  }

-  public function getStoryText($object) {
-    $implied_context = $this->getRenderWithImpliedContext();
-
-    $story = $this->getFeedStory();
-    if ($story instanceof PhabricatorFeedStoryAudit) {
-      $text = $story->renderForAsanaBridge($implied_context);
-    } else {
-      $text = $story->renderText();
-    }
-    return $text;
-  }
-
  private function getTitlePrefix(PhabricatorRepositoryCommit $commit) {
    $prefix_key = 'metamta.diffusion.subject-prefix';
    return PhabricatorEnv::getEnvConfig($prefix_key);
--- a/src/applications/doorkeeper/engine/DoorkeeperFeedStoryPublisher.php
+++ b/src/applications/doorkeeper/engine/DoorkeeperFeedStoryPublisher.php
@ -81,6 +81,11 @@ abstract class DoorkeeperFeedStoryPublisher {
    return $object;
  }

+
+  public function getStoryText($object) {
+    return $this->getFeedStory()->renderAsTextForDoorkeeper($this);
+  }
+
  abstract public function isStoryAboutObjectCreation($object);
  abstract public function isStoryAboutObjectClosure($object);
  abstract public function getOwnerPHID($object);
@ -92,6 +97,5 @@ abstract class DoorkeeperFeedStoryPublisher {
  abstract public function getObjectDescription($object);
  abstract public function isObjectClosed($object);
  abstract public function getResponsibilityTitle($object);
-  abstract public function getStoryText($object);

 }
--- a/src/applications/feed/constants/PhabricatorFeedStoryTypeConstants.php
+++ b/src/applications/feed/constants/PhabricatorFeedStoryTypeConstants.php
@ -4,7 +4,6 @@ final class PhabricatorFeedStoryTypeConstants
  extends PhabricatorFeedConstants {

  const STORY_PHRICTION     = 'PhabricatorFeedStoryPhriction';
-  const STORY_AUDIT         = 'PhabricatorFeedStoryAudit';
  const STORY_COMMIT        = 'PhabricatorFeedStoryCommit';

 }
--- a/src/applications/feed/story/PhabricatorFeedStory.php
+++ b/src/applications/feed/story/PhabricatorFeedStory.php
@ -251,6 +251,17 @@ abstract class PhabricatorFeedStory
  }

  abstract public function renderView();
+  public function renderAsTextForDoorkeeper(
+    DoorkeeperFeedStoryPublisher $publisher) {
+
+    // TODO: This (and text rendering) should be properly abstract and
+    // universal. However, this is far less bad than it used to be, and we
+    // need to clean up more old feed code to really make this reasonable.
+
+    return pht(
+      '(Unable to render story of class %s for Doorkeeper.)',
+      get_class($this));
+  }

  public function getRequiredHandlePHIDs() {
    return array();
--- a/src/applications/feed/story/PhabricatorFeedStoryAudit.php
+++ b/src/applications/feed/story/PhabricatorFeedStoryAudit.php
@ -47,38 +47,4 @@ final class PhabricatorFeedStoryAudit extends PhabricatorFeedStory {
    return $text;
  }

-
-  // TODO: At some point, make feed rendering not terrible and remove this
-  // hacky mess.
-  public function renderForAsanaBridge($implied_context = false) {
-    $data = $this->getStoryData();
-    $comment = $data->getValue('content');
-
-    $author_name = $this->getHandle($this->getAuthorPHID())->getName();
-    $action = $this->getValue('action');
-    $verb = PhabricatorAuditActionConstants::getActionPastTenseVerb($action);
-
-    $commit_phid = $this->getPrimaryObjectPHID();
-    $commit_name = $this->getHandle($commit_phid)->getFullName();
-
-    if ($implied_context) {
-      $title = "{$author_name} {$verb} this commit.";
-    } else {
-      $title = "{$author_name} {$verb} commit {$commit_name}.";
-    }
-
-    if (strlen($comment)) {
-      $engine = PhabricatorMarkupEngine::newMarkupEngine(array())
-        ->setConfig('viewer', new PhabricatorUser())
-        ->setMode(PhutilRemarkupEngine::MODE_TEXT);
-
-      $comment = $engine->markupText($comment);
-
-      $title .= "\n\n";
-      $title .= $comment;
-    }
-
-    return $title;
-  }
-
 }
--- a/src/applications/feed/story/PhabricatorFeedStoryDifferential.php
+++ b/src/applications/feed/story/PhabricatorFeedStoryDifferential.php
@ -235,159 +235,4 @@ final class PhabricatorFeedStoryDifferential extends PhabricatorFeedStory {
    );
  }

-  // TODO: At some point, make feed rendering not terrible and remove this
-  // hacky mess.
-  public function renderForAsanaBridge($implied_context = false) {
-    $data = $this->getStoryData();
-    $comment = $data->getValue('feedback_content');
-
-    $author_name = $this->getHandle($this->getAuthorPHID())->getName();
-    $action = $this->getValue('action');
-
-    $engine = PhabricatorMarkupEngine::newMarkupEngine(array())
-      ->setConfig('viewer', new PhabricatorUser())
-      ->setMode(PhutilRemarkupEngine::MODE_TEXT);
-
-    $revision_phid = $this->getPrimaryObjectPHID();
-    $revision_name = $this->getHandle($revision_phid)->getFullName();
-
-    if ($implied_context) {
-      $title = DifferentialAction::getBasicStoryText(
-            $action, $author_name);
-    } else {
-      switch ($action) {
-        case DifferentialAction::ACTION_COMMENT:
-          $title = pht('%s commented on revision %s',
-            $author_name, $revision_name);
-        break;
-        case DifferentialAction::ACTION_ACCEPT:
-          $title = pht('%s accepted revision %s',
-            $author_name, $revision_name);
-        break;
-        case DifferentialAction::ACTION_REJECT:
-          $title = pht('%s requested changes to revision %s',
-            $author_name, $revision_name);
-        break;
-        case DifferentialAction::ACTION_RETHINK:
-          $title = pht('%s planned changes to revision %s',
-            $author_name, $revision_name);
-        break;
-        case DifferentialAction::ACTION_ABANDON:
-          $title = pht('%s abandoned revision %s',
-            $author_name, $revision_name);
-        break;
-        case DifferentialAction::ACTION_CLOSE:
-          $title = pht('%s closed revision %s',
-            $author_name, $revision_name);
-        break;
-        case DifferentialAction::ACTION_REQUEST:
-          $title = pht('%s requested a review of revision %s',
-            $author_name, $revision_name);
-        break;
-        case DifferentialAction::ACTION_RECLAIM:
-          $title = pht('%s reclaimed revision %s',
-            $author_name, $revision_name);
-        break;
-        case DifferentialAction::ACTION_UPDATE:
-          $title = pht('%s updated revision %s',
-            $author_name, $revision_name);
-        break;
-        case DifferentialAction::ACTION_RESIGN:
-          $title = pht('%s resigned from revision %s',
-            $author_name, $revision_name);
-        break;
-        case DifferentialAction::ACTION_SUMMARIZE:
-          $title = pht('%s summarized revision %s',
-            $author_name, $revision_name);
-        break;
-        case DifferentialAction::ACTION_TESTPLAN:
-          $title = pht('%s explained the test plan for revision %s',
-            $author_name, $revision_name);
-        break;
-        case DifferentialAction::ACTION_CREATE:
-          $title = pht('%s created revision %s',
-            $author_name, $revision_name);
-        break;
-        case DifferentialAction::ACTION_ADDREVIEWERS:
-          $title = pht('%s added reviewers to revision %s',
-            $author_name, $revision_name);
-        break;
-        case DifferentialAction::ACTION_ADDCCS:
-          $title = pht('%s added CCs to revision %s',
-            $author_name, $revision_name);
-        break;
-        case DifferentialAction::ACTION_CLAIM:
-          $title = pht('%s commandeered revision %s',
-            $author_name, $revision_name);
-        break;
-        case DifferentialAction::ACTION_REOPEN:
-          $title = pht('%s reopened revision %s',
-          $author_name, $revision_name);
-        break;
-        case DifferentialTransaction::TYPE_INLINE:
-          $title = pht('%s added inline comments to %s',
-          $author_name, $revision_name);
-        break;
-        default:
-          $title = pht('%s edited revision %s',
-          $author_name, $revision_name);
-        break;
-      }
-    }
-
-    if (strlen($comment)) {
-      $comment = $engine->markupText($comment);
-
-      $title .= "\n\n";
-      $title .= $comment;
-    }
-
-    // Roughly render inlines into the comment.
-    $xaction_phids = $data->getValue('temporaryTransactionPHIDs');
-    if ($xaction_phids) {
-      $inlines = id(new DifferentialTransactionQuery())
-        ->setViewer(PhabricatorUser::getOmnipotentUser())
-        ->withPHIDs($xaction_phids)
-        ->needComments(true)
-        ->withTransactionTypes(
-          array(
-            DifferentialTransaction::TYPE_INLINE,
-          ))
-        ->execute();
-      if ($inlines) {
-        $title .= "\n\n";
-        $title .= pht('Inline Comments');
-        $title .= "\n";
-
-        $changeset_ids = array();
-        foreach ($inlines as $inline) {
-          $changeset_ids[] = $inline->getComment()->getChangesetID();
-        }
-
-        $changesets = id(new DifferentialChangeset())->loadAllWhere(
-          'id IN (%Ld)',
-          $changeset_ids);
-
-        foreach ($inlines as $inline) {
-          $comment = $inline->getComment();
-          $changeset = idx($changesets, $comment->getChangesetID());
-          if (!$changeset) {
-            continue;
-          }
-
-          $filename = $changeset->getDisplayFilename();
-          $linenumber = $comment->getLineNumber();
-          $inline_text = $engine->markupText($comment->getContent());
-          $inline_text = rtrim($inline_text);
-
-          $title .= "{$filename}:{$linenumber} {$inline_text}\n";
-        }
-      }
-    }
-
-
-    return $title;
-  }
-
-
 }
--- a/src/applications/transactions/feed/PhabricatorApplicationTransactionFeedStory.php
+++ b/src/applications/transactions/feed/PhabricatorApplicationTransactionFeedStory.php
@ -104,4 +104,17 @@ class PhabricatorApplicationTransactionFeedStory
    return $text;
  }

+  public function renderAsTextForDoorkeeper(
+    DoorkeeperFeedStoryPublisher $publisher) {
+
+    $xactions = array();
+    $xaction_phids = $this->getValue('transactionPHIDs');
+    foreach ($xaction_phids as $xaction_phid) {
+      $xactions[] = $this->getObject($xaction_phid);
+    }
+
+    $primary = $this->getPrimaryTransaction();
+    return $primary->renderAsTextForDoorkeeper($publisher, $this, $xactions);
+  }
+
 }
--- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php
+++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php
@ -1078,6 +1078,44 @@ abstract class PhabricatorApplicationTransaction
    return null;
  }

+  public function renderAsTextForDoorkeeper(
+    DoorkeeperFeedStoryPublisher $publisher,
+    PhabricatorFeedStory $story,
+    array $xactions) {
+
+    $text = array();
+    $body = array();
+
+    foreach ($xactions as $xaction) {
+      $xaction_body = $xaction->getBodyForMail();
+      if ($xaction_body !== null) {
+        $body[] = $xaction_body;
+      }
+
+      if ($xaction->shouldHideForMail($xactions)) {
+        continue;
+      }
+
+      $old_target = $xaction->getRenderingTarget();
+      $new_target = PhabricatorApplicationTransaction::TARGET_TEXT;
+      $xaction->setRenderingTarget($new_target);
+
+      if ($publisher->getRenderWithImpliedContext()) {
+        $text[] = $xaction->getTitle();
+      } else {
+        $text[] = $xaction->getTitleForFeed($story);
+      }
+
+      $xaction->setRenderingTarget($old_target);
+    }
+
+    $text = implode("\n", $text);
+    $body = implode("\n\n", $body);
+
+    return rtrim($text."\n\n".$body);
+  }
+
+

 /* -(  PhabricatorPolicyInterface Implementation  )-------------------------- */