From 32dd8af9e5d55d4024c7ba2abfc0a0705a32e261 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 21 Oct 2013 17:01:27 -0700 Subject: [PATCH] Reduce surface area of DifferentialComment API Summary: Ref T2222. Shrink the API to make it easier to move this object's storage to ApplicationTransactions. Fixes T3415. This moves the "Summary" and "Test Plan" into the property list, and thereby fixes all the attribution problems associated with commandeering, creating a revision from another user's diff, etc. Test Plan: Browsed several revisions. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T3415, T2222 Differential Revision: https://secure.phabricator.com/D7375 --- .../DifferentialRevisionViewController.php | 53 +------------------ .../editor/DifferentialCommentEditor.php | 2 +- .../editor/DifferentialRevisionEditor.php | 3 +- .../storage/DifferentialComment.php | 51 ++++++++++-------- .../view/DifferentialRevisionDetailView.php | 22 ++++++++ 5 files changed, 56 insertions(+), 75 deletions(-) diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php index bc0ccaeedb..ee0e13f6eb 100644 --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/DifferentialRevisionViewController.php @@ -91,9 +91,6 @@ final class DifferentialRevisionViewController extends DifferentialController { $aux_fields = $this->loadAuxiliaryFields($revision); $comments = $revision->loadComments(); - $comments = array_merge( - $this->getImplicitComments($revision, reset($diffs)), - $comments); $all_changesets = $changesets; $inlines = $this->loadInlineComments( @@ -111,22 +108,8 @@ final class DifferentialRevisionViewController extends DifferentialController { mpull($comments, 'getAuthorPHID')); foreach ($comments as $comment) { - $metadata = $comment->getMetadata(); - $added_reviewers = idx( - $metadata, - DifferentialComment::METADATA_ADDED_REVIEWERS); - if ($added_reviewers) { - foreach ($added_reviewers as $phid) { - $object_phids[] = $phid; - } - } - $added_ccs = idx( - $metadata, - DifferentialComment::METADATA_ADDED_CCS); - if ($added_ccs) { - foreach ($added_ccs as $phid) { - $object_phids[] = $phid; - } + foreach ($comment->getRequiredHandlePHIDs() as $phid) { + $object_phids[] = $phid; } } @@ -479,38 +462,6 @@ final class DifferentialRevisionViewController extends DifferentialController { )); } - private function getImplicitComments( - DifferentialRevision $revision, - DifferentialDiff $diff) { - - $author_phid = nonempty( - $diff->getAuthorPHID(), - $revision->getAuthorPHID()); - - $template = new DifferentialComment(); - $template->setAuthorPHID($author_phid); - $template->setRevisionID($revision->getID()); - $template->setDateCreated($revision->getDateCreated()); - - $comments = array(); - - if (strlen($revision->getSummary())) { - $summary_comment = clone $template; - $summary_comment->setContent($revision->getSummary()); - $summary_comment->setAction(DifferentialAction::ACTION_SUMMARIZE); - $comments[] = $summary_comment; - } - - if (strlen($revision->getTestPlan())) { - $testplan_comment = clone $template; - $testplan_comment->setContent($revision->getTestPlan()); - $testplan_comment->setAction(DifferentialAction::ACTION_TESTPLAN); - $comments[] = $testplan_comment; - } - - return $comments; - } - private function getRevisionActions(DifferentialRevision $revision) { $user = $this->getRequest()->getUser(); $viewer_phid = $user->getPHID(); diff --git a/src/applications/differential/editor/DifferentialCommentEditor.php b/src/applications/differential/editor/DifferentialCommentEditor.php index 36d38bc58b..0fef83eb2d 100644 --- a/src/applications/differential/editor/DifferentialCommentEditor.php +++ b/src/applications/differential/editor/DifferentialCommentEditor.php @@ -566,7 +566,7 @@ final class DifferentialCommentEditor extends PhabricatorEditor { $comment = id(new DifferentialComment()) ->setAuthorPHID($actor_phid) - ->setRevisionID($revision->getID()) + ->setRevision($revision) ->setAction($action) ->setContent((string)$this->message) ->setMetadata($metadata); diff --git a/src/applications/differential/editor/DifferentialRevisionEditor.php b/src/applications/differential/editor/DifferentialRevisionEditor.php index e5418ecab2..028948d32b 100644 --- a/src/applications/differential/editor/DifferentialRevisionEditor.php +++ b/src/applications/differential/editor/DifferentialRevisionEditor.php @@ -783,10 +783,9 @@ final class DifferentialRevisionEditor extends PhabricatorEditor { private function createComment() { - $revision_id = $this->revision->getID(); $comment = id(new DifferentialComment()) ->setAuthorPHID($this->getActorPHID()) - ->setRevisionID($revision_id) + ->setRevision($this->revision) ->setContent($this->getComments()) ->setAction(DifferentialAction::ACTION_UPDATE) ->setMetadata( diff --git a/src/applications/differential/storage/DifferentialComment.php b/src/applications/differential/storage/DifferentialComment.php index 401b291b51..8d56afdf01 100644 --- a/src/applications/differential/storage/DifferentialComment.php +++ b/src/applications/differential/storage/DifferentialComment.php @@ -20,11 +20,39 @@ final class DifferentialComment extends DifferentialDAO private $arbitraryDiffForFacebook; + public function setRevision(DifferentialRevision $revision) { + return $this->setRevisionID($revision->getID()); + } + public function giveFacebookSomeArbitraryDiff(DifferentialDiff $diff) { $this->arbitraryDiffForFacebook = $diff; return $this; } + public function getRequiredHandlePHIDs() { + $phids = array(); + + $metadata = $this->getMetadata(); + $added_reviewers = idx( + $metadata, + self::METADATA_ADDED_REVIEWERS); + if ($added_reviewers) { + foreach ($added_reviewers as $phid) { + $phids[] = $phid; + } + } + $added_ccs = idx( + $metadata, + self::METADATA_ADDED_CCS); + if ($added_ccs) { + foreach ($added_ccs as $phid) { + $phids[] = $phid; + } + } + + return $phids; + } + public function getConfiguration() { return array( self::CONFIG_SERIALIZATION => array( @@ -42,16 +70,8 @@ final class DifferentialComment extends DifferentialDAO return PhabricatorContentSource::newFromSerialized($this->contentSource); } - public function getMarkupFieldKey($field) { - if ($this->getID()) { - return 'DC:'.$this->getID(); - } - - // The summary and test plan render as comments, but do not have IDs. - // They are also mutable. Build keys using content hashes. - $hash = PhabricatorHash::digest($this->getMarkupText($field)); - return 'DC:'.$hash; + return 'DC:'.$this->getID(); } public function newMarkupEngine($field) { @@ -70,18 +90,7 @@ final class DifferentialComment extends DifferentialDAO } public function shouldUseMarkupCache($field) { - if ($this->getID()) { - return true; - } - - $action = $this->getAction(); - switch ($action) { - case DifferentialAction::ACTION_SUMMARIZE: - case DifferentialAction::ACTION_TESTPLAN: - return true; - } - - return false; + return (bool)$this->getID(); } } diff --git a/src/applications/differential/view/DifferentialRevisionDetailView.php b/src/applications/differential/view/DifferentialRevisionDetailView.php index f965acaf92..47d021a033 100644 --- a/src/applications/differential/view/DifferentialRevisionDetailView.php +++ b/src/applications/differential/view/DifferentialRevisionDetailView.php @@ -112,6 +112,28 @@ final class DifferentialRevisionDetailView extends AphrontView { $properties->setHasKeyboardShortcuts(true); $properties->setActionList($actions); + $properties->invokeWillRenderEvent(); + + if (strlen($revision->getSummary())) { + $properties->addSectionHeader(pht('Summary')); + $properties->addTextContent( + PhabricatorMarkupEngine::renderOneObject( + id(new PhabricatorMarkupOneOff())->setContent( + $revision->getSummary()), + 'default', + $user)); + } + + if (strlen($revision->getTestPlan())) { + $properties->addSectionHeader(pht('Test Plan')); + $properties->addTextContent( + PhabricatorMarkupEngine::renderOneObject( + id(new PhabricatorMarkupOneOff())->setContent( + $revision->getTestPlan()), + 'default', + $user)); + } + $object_box = id(new PHUIObjectBoxView()) ->setHeader($header) ->addPropertyList($properties); -- 2.51.2