From 8e45b466da625cfaac58168336b130b04f4b448d Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 10 Sep 2013 15:22:24 -0700 Subject: [PATCH] Improve voicing in text published to JIRA issues Summary: Ref T3687. JIRA is able to piggyback on a fair amount of Asana infrastructure, but the voicing we use on Asana tasks (which are always about one object) isn't very good for JIRA issues (which may have many linked objects). Specifically, we publish stories like this to Asana: alincoln accepted this revision. This is meaningless in JIRA since you have no idea what it's talking about. Instead, publish like this: alincoln accepted D999: Put a bird on it Additionally, supplement it with a URI, so the total story text we publish is: alincoln accepted D999: Put a bird on it https://phabricator.whitehouse.gov/D999 Signifcantly less useless! Test Plan: {F57523} {F57524} Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T3687 Differential Revision: https://secure.phabricator.com/D6907 --- ...alDoorkeeperRevisionFeedStoryPublisher.php | 4 +- ...sionDoorkeeperCommitFeedStoryPublisher.php | 4 +- .../engine/DoorkeeperFeedStoryPublisher.php | 46 +++++++++++++++++++ .../worker/DoorkeeperFeedWorkerAsana.php | 4 +- .../worker/DoorkeeperFeedWorkerJIRA.php | 12 ++++- .../feed/story/PhabricatorFeedStoryAudit.php | 12 ++++- .../PhabricatorFeedStoryDifferential.php | 12 ++++- 7 files changed, 86 insertions(+), 8 deletions(-) diff --git a/src/applications/differential/doorkeeper/DifferentialDoorkeeperRevisionFeedStoryPublisher.php b/src/applications/differential/doorkeeper/DifferentialDoorkeeperRevisionFeedStoryPublisher.php index d24b997b1f..74f9117ced 100644 --- a/src/applications/differential/doorkeeper/DifferentialDoorkeeperRevisionFeedStoryPublisher.php +++ b/src/applications/differential/doorkeeper/DifferentialDoorkeeperRevisionFeedStoryPublisher.php @@ -93,9 +93,11 @@ final class DifferentialDoorkeeperRevisionFeedStoryPublisher } public function getStoryText($object) { + $implied_context = $this->getRenderWithImpliedContext(); + $story = $this->getFeedStory(); if ($story instanceof PhabricatorFeedStoryDifferential) { - $text = $story->renderForAsanaBridge(); + $text = $story->renderForAsanaBridge($implied_context); } else { $text = $story->renderText(); } diff --git a/src/applications/diffusion/doorkeeper/DiffusionDoorkeeperCommitFeedStoryPublisher.php b/src/applications/diffusion/doorkeeper/DiffusionDoorkeeperCommitFeedStoryPublisher.php index feae1628ad..0a5b6b2a21 100644 --- a/src/applications/diffusion/doorkeeper/DiffusionDoorkeeperCommitFeedStoryPublisher.php +++ b/src/applications/diffusion/doorkeeper/DiffusionDoorkeeperCommitFeedStoryPublisher.php @@ -176,9 +176,11 @@ final class DiffusionDoorkeeperCommitFeedStoryPublisher } public function getStoryText($object) { + $implied_context = $this->getRenderWithImpliedContext(); + $story = $this->getFeedStory(); if ($story instanceof PhabricatorFeedStoryAudit) { - $text = $story->renderForAsanaBridge(); + $text = $story->renderForAsanaBridge($implied_context); } else { $text = $story->renderText(); } diff --git a/src/applications/doorkeeper/engine/DoorkeeperFeedStoryPublisher.php b/src/applications/doorkeeper/engine/DoorkeeperFeedStoryPublisher.php index bd6809d87f..13a00a6b44 100644 --- a/src/applications/doorkeeper/engine/DoorkeeperFeedStoryPublisher.php +++ b/src/applications/doorkeeper/engine/DoorkeeperFeedStoryPublisher.php @@ -1,9 +1,55 @@ renderWithImpliedContext = $render_with_implied_context; + return $this; + } + + /** + * Determine if rendering should assume object context. For discussion, see + * @{method:setRenderWithImpliedContext}. + * + * @return bool True if rendering should assume object context is implied. + * @task config + */ + public function getRenderWithImpliedContext() { + return $this->renderWithImpliedContext; + } public function setFeedStory(PhabricatorFeedStory $feed_story) { $this->feedStory = $feed_story; diff --git a/src/applications/doorkeeper/worker/DoorkeeperFeedWorkerAsana.php b/src/applications/doorkeeper/worker/DoorkeeperFeedWorkerAsana.php index a389df3bd7..80df37905a 100644 --- a/src/applications/doorkeeper/worker/DoorkeeperFeedWorkerAsana.php +++ b/src/applications/doorkeeper/worker/DoorkeeperFeedWorkerAsana.php @@ -414,7 +414,9 @@ final class DoorkeeperFeedWorkerAsana extends DoorkeeperFeedWorker { // because everything else is idempotent, so this is the only effect we // can't safely run more than once. - $text = $publisher->getStoryText($object); + $text = $publisher + ->setRenderWithImpliedContext(true) + ->getStoryText($object); $this->makeAsanaAPICall( $oauth_token, diff --git a/src/applications/doorkeeper/worker/DoorkeeperFeedWorkerJIRA.php b/src/applications/doorkeeper/worker/DoorkeeperFeedWorkerJIRA.php index e9e6a3fae9..d6686b7de7 100644 --- a/src/applications/doorkeeper/worker/DoorkeeperFeedWorkerJIRA.php +++ b/src/applications/doorkeeper/worker/DoorkeeperFeedWorkerJIRA.php @@ -54,7 +54,7 @@ final class DoorkeeperFeedWorkerJIRA extends DoorkeeperFeedWorker { return; } - $story_text = $publisher->getStoryText($object); + $story_text = $this->renderStoryText(); $xobjs = mgroup($xobjs, 'getApplicationDomain'); foreach ($xobjs as $domain => $xobj_list) { @@ -156,4 +156,14 @@ final class DoorkeeperFeedWorkerJIRA extends DoorkeeperFeedWorker { return $try_users; } + private function renderStoryText() { + $object = $this->getStoryObject(); + $publisher = $this->getPublisher(); + + $text = $publisher->getStoryText($object); + $uri = $publisher->getObjectURI($object); + + return $text."\n\n".$uri; + } + } diff --git a/src/applications/feed/story/PhabricatorFeedStoryAudit.php b/src/applications/feed/story/PhabricatorFeedStoryAudit.php index e9f7f855a6..bade63e766 100644 --- a/src/applications/feed/story/PhabricatorFeedStoryAudit.php +++ b/src/applications/feed/story/PhabricatorFeedStoryAudit.php @@ -50,7 +50,7 @@ final class PhabricatorFeedStoryAudit extends PhabricatorFeedStory { // TODO: At some point, make feed rendering not terrible and remove this // hacky mess. - public function renderForAsanaBridge() { + public function renderForAsanaBridge($implied_context = false) { $data = $this->getStoryData(); $comment = $data->getValue('content'); @@ -58,7 +58,15 @@ final class PhabricatorFeedStoryAudit extends PhabricatorFeedStory { $action = $this->getValue('action'); $verb = PhabricatorAuditActionConstants::getActionPastTenseVerb($action); - $title = "{$author_name} {$verb} this commit."; + $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()) diff --git a/src/applications/feed/story/PhabricatorFeedStoryDifferential.php b/src/applications/feed/story/PhabricatorFeedStoryDifferential.php index 7cfcbe2e97..b144076f93 100644 --- a/src/applications/feed/story/PhabricatorFeedStoryDifferential.php +++ b/src/applications/feed/story/PhabricatorFeedStoryDifferential.php @@ -90,7 +90,7 @@ final class PhabricatorFeedStoryDifferential extends PhabricatorFeedStory { // TODO: At some point, make feed rendering not terrible and remove this // hacky mess. - public function renderForAsanaBridge() { + public function renderForAsanaBridge($implied_context = false) { $data = $this->getStoryData(); $comment = $data->getValue('feedback_content'); @@ -102,7 +102,15 @@ final class PhabricatorFeedStoryDifferential extends PhabricatorFeedStory { ->setConfig('viewer', new PhabricatorUser()) ->setMode(PhutilRemarkupEngine::MODE_TEXT); - $title = "{$author_name} {$verb} this revision."; + $revision_phid = $this->getPrimaryObjectPHID(); + $revision_name = $this->getHandle($revision_phid)->getFullName(); + + if ($implied_context) { + $title = "{$author_name} {$verb} this revision."; + } else { + $title = "{$author_name} {$verb} revision {$revision_name}."; + } + if (strlen($comment)) { $comment = $engine->markupText($comment);