From 49bd5721c5a7a5b8abd8571af9f10dcc8c7286b9 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 2 Aug 2014 00:06:56 -0700 Subject: [PATCH] Use standard infrastructure for Feed in Audit Summary: Ref T4896. Instead of using custom stuff, use standard stuff. Test Plan: Viewed a bunch of feed stories and published some over the Asana bridge. Reviewers: joshuaspence, btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T4896 Differential Revision: https://secure.phabricator.com/D10114 --- .../editor/PhabricatorAuditCommentEditor.php | 49 ----------- .../audit/editor/PhabricatorAuditEditor.php | 6 ++ .../storage/PhabricatorAuditTransaction.php | 87 +++++++++++++++++++ .../PhabricatorAuthLoginController.php | 2 +- ...sionDoorkeeperCommitFeedStoryPublisher.php | 28 +++--- ...ricatorApplicationTransactionFeedStory.php | 2 +- 6 files changed, 112 insertions(+), 62 deletions(-) diff --git a/src/applications/audit/editor/PhabricatorAuditCommentEditor.php b/src/applications/audit/editor/PhabricatorAuditCommentEditor.php index 54f005bbe7..d658a5a6df 100644 --- a/src/applications/audit/editor/PhabricatorAuditCommentEditor.php +++ b/src/applications/audit/editor/PhabricatorAuditCommentEditor.php @@ -245,27 +245,6 @@ final class PhabricatorAuditCommentEditor extends PhabricatorEditor { ->setExcludeMailRecipientPHIDs($this->getExcludeMailRecipientPHIDs()) ->setDisableEmail($this->noEmail) ->applyTransactions($commit, $xactions); - - $feed_dont_publish_phids = array(); - foreach ($requests as $request) { - $status = $request->getAuditStatus(); - switch ($status) { - case PhabricatorAuditStatusConstants::RESIGNED: - case PhabricatorAuditStatusConstants::NONE: - case PhabricatorAuditStatusConstants::AUDIT_NOT_REQUIRED: - $feed_dont_publish_phids[$request->getAuditorPHID()] = 1; - break; - default: - unset($feed_dont_publish_phids[$request->getAuditorPHID()]); - break; - } - } - $feed_dont_publish_phids = array_keys($feed_dont_publish_phids); - - $feed_phids = array_diff($requests_phids, $feed_dont_publish_phids); - foreach ($comments as $comment) { - $this->publishFeedStory($comment, $feed_phids); - } } @@ -305,32 +284,4 @@ final class PhabricatorAuditCommentEditor extends PhabricatorEditor { return array_keys($phids); } - private function publishFeedStory( - PhabricatorAuditComment $comment, - array $more_phids) { - - $commit = $this->commit; - $actor = $this->getActor(); - - $related_phids = array_merge( - array( - $actor->getPHID(), - $commit->getPHID(), - ), - $more_phids); - - id(new PhabricatorFeedStoryPublisher()) - ->setRelatedPHIDs($related_phids) - ->setStoryAuthorPHID($actor->getPHID()) - ->setStoryTime(time()) - ->setStoryType(PhabricatorFeedStoryTypeConstants::STORY_AUDIT) - ->setStoryData( - array( - 'commitPHID' => $commit->getPHID(), - 'action' => $comment->getAction(), - 'content' => $comment->getContent(), - )) - ->publish(); - } - } diff --git a/src/applications/audit/editor/PhabricatorAuditEditor.php b/src/applications/audit/editor/PhabricatorAuditEditor.php index 5aef278637..d016799988 100644 --- a/src/applications/audit/editor/PhabricatorAuditEditor.php +++ b/src/applications/audit/editor/PhabricatorAuditEditor.php @@ -234,4 +234,10 @@ final class PhabricatorAuditEditor return implode("\n", $block); } + protected function shouldPublishFeedStory( + PhabricatorLiskDAO $object, + array $xactions) { + return true; + } + } diff --git a/src/applications/audit/storage/PhabricatorAuditTransaction.php b/src/applications/audit/storage/PhabricatorAuditTransaction.php index 80b1ef6433..94f5884533 100644 --- a/src/applications/audit/storage/PhabricatorAuditTransaction.php +++ b/src/applications/audit/storage/PhabricatorAuditTransaction.php @@ -179,6 +179,93 @@ final class PhabricatorAuditTransaction return parent::getTitle(); } + public function getTitleForFeed(PhabricatorFeedStory $story) { + $old = $this->getOldValue(); + $new = $this->getNewValue(); + + $author_handle = $this->renderHandleLink($this->getAuthorPHID()); + $object_handle = $this->renderHandleLink($this->getObjectPHID()); + + $type = $this->getTransactionType(); + + switch ($type) { + case PhabricatorAuditActionConstants::ADD_CCS: + case PhabricatorAuditActionConstants::ADD_AUDITORS: + if (!is_array($old)) { + $old = array(); + } + if (!is_array($new)) { + $new = array(); + } + $add = array_keys(array_diff_key($new, $old)); + $rem = array_keys(array_diff_key($old, $new)); + break; + } + + switch ($type) { + case PhabricatorAuditActionConstants::INLINE: + return pht( + '%s added inline comments to %s.', + $author_handle, + $object_handle); + + case PhabricatorAuditActionConstants::ADD_AUDITORS: + if ($add && $rem) { + return pht( + '%s edited auditors for %s; added: %s, removed: %s.', + $author_handle, + $object_handle, + $this->renderHandleList($add), + $this->renderHandleList($rem)); + } else if ($add) { + return pht( + '%s added auditors to %s: %s.', + $author_handle, + $object_handle, + $this->renderHandleList($add)); + } else if ($rem) { + return pht( + '%s removed auditors from %s: %s.', + $author_handle, + $object_handle, + $this->renderHandleList($rem)); + } else { + return pht( + '%s added auditors to %s...', + $author_handle, + $object_handle); + } + + case PhabricatorAuditActionConstants::ACTION: + switch ($new) { + case PhabricatorAuditActionConstants::ACCEPT: + return pht( + '%s accepted %s.', + $author_handle, + $object_handle); + case PhabricatorAuditActionConstants::CONCERN: + return pht( + '%s raised a concern with %s.', + $author_handle, + $object_handle); + case PhabricatorAuditActionConstants::RESIGN: + return pht( + '%s resigned from auditing %s.', + $author_handle, + $object_handle); + case PhabricatorAuditActionConstants::CLOSE: + return pht( + '%s closed the audit of %s.', + $author_handle, + $object_handle); + } + + } + + return parent::getTitleForFeed($story); + } + + // TODO: These two mail methods can likely be abstracted by introducing a // formal concept of "inline comment" transactions. diff --git a/src/applications/auth/controller/PhabricatorAuthLoginController.php b/src/applications/auth/controller/PhabricatorAuthLoginController.php index 068be3126c..5658a44c7c 100644 --- a/src/applications/auth/controller/PhabricatorAuthLoginController.php +++ b/src/applications/auth/controller/PhabricatorAuthLoginController.php @@ -94,7 +94,7 @@ final class PhabricatorAuthLoginController } else { return $this->renderError( pht( - 'The external account ("%s") you just used to login is alerady '. + 'The external account ("%s") you just used to login is already '. 'associated with another Phabricator user account. Login to the '. 'other Phabricator account and unlink the external account before '. 'linking it to a new Phabricator account.', diff --git a/src/applications/diffusion/doorkeeper/DiffusionDoorkeeperCommitFeedStoryPublisher.php b/src/applications/diffusion/doorkeeper/DiffusionDoorkeeperCommitFeedStoryPublisher.php index 3d3f747cd7..a8bab3a3f1 100644 --- a/src/applications/diffusion/doorkeeper/DiffusionDoorkeeperCommitFeedStoryPublisher.php +++ b/src/applications/diffusion/doorkeeper/DiffusionDoorkeeperCommitFeedStoryPublisher.php @@ -12,7 +12,9 @@ final class DiffusionDoorkeeperCommitFeedStoryPublisher } public function canPublishStory(PhabricatorFeedStory $story, $object) { - return ($object instanceof PhabricatorRepositoryCommit); + return + ($story instanceof PhabricatorApplicationTransactionFeedStory) && + ($object instanceof PhabricatorRepositoryCommit); } public function isStoryAboutObjectCreation($object) { @@ -29,17 +31,21 @@ final class DiffusionDoorkeeperCommitFeedStoryPublisher // After ApplicationTransactions, we could annotate feed stories more // explicitly. - $story = $this->getFeedStory(); - $action = $story->getStoryData()->getValue('action'); - - if ($action == PhabricatorAuditActionConstants::CLOSE) { - return true; - } - $fully_audited = PhabricatorAuditCommitStatusConstants::FULLY_AUDITED; - if (($action == PhabricatorAuditActionConstants::ACCEPT) && - $object->getAuditStatus() == $fully_audited) { - return true; + + $story = $this->getFeedStory(); + $xaction = $story->getPrimaryTransaction(); + switch ($xaction->getTransactionType()) { + case PhabricatorAuditActionConstants::ACTION: + switch ($xaction->getNewValue()) { + case PhabricatorAuditActionConstants::CLOSE: + return true; + case PhabricatorAuditActionConstants::ACCEPT: + if ($object->getAuditStatus() == $fully_audited) { + return true; + } + break; + } } return false; diff --git a/src/applications/transactions/feed/PhabricatorApplicationTransactionFeedStory.php b/src/applications/transactions/feed/PhabricatorApplicationTransactionFeedStory.php index 4ccc265450..dc820fb6db 100644 --- a/src/applications/transactions/feed/PhabricatorApplicationTransactionFeedStory.php +++ b/src/applications/transactions/feed/PhabricatorApplicationTransactionFeedStory.php @@ -30,7 +30,7 @@ class PhabricatorApplicationTransactionFeedStory return head($this->getValue('transactionPHIDs')); } - protected function getPrimaryTransaction() { + public function getPrimaryTransaction() { return $this->getObject($this->getPrimaryTransactionPHID()); }