From 5645a07d998455c8edf744f5a78c1a4d1d3f3ad8 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 20 Apr 2015 14:33:58 -0700 Subject: [PATCH] Modernize DifferentialInlineCommentQuery Summary: Ref T7447. This class is currently a big mess with a lot of `withWeirdSpecialThingUsedInOnePlace()` type qualifiers. Try to generalize/normalize it a bit. Test Plan: - Viewed inline comments. - Created a new inline comment. - Edited an inline comment. - Marked an inline comment complete. - Deleted, then undeleted an inline comment. - Previewed inline comments. - Viewed drafts as another user, verified they don't show up. Reviewers: btrahan Reviewed By: btrahan Subscribers: yelirekim, epriestley Maniphest Tasks: T7447 Differential Revision: https://secure.phabricator.com/D12483 --- .../DifferentialChangesetViewController.php | 3 +- ...ifferentialInlineCommentEditController.php | 4 + ...erentialInlineCommentPreviewController.php | 13 +- .../DifferentialRevisionViewController.php | 7 +- .../query/DifferentialInlineCommentQuery.php | 127 ++++++++---------- .../storage/DifferentialRevision.php | 22 +-- 6 files changed, 84 insertions(+), 92 deletions(-) diff --git a/src/applications/differential/controller/DifferentialChangesetViewController.php b/src/applications/differential/controller/DifferentialChangesetViewController.php index f617c52f2b..b701bb05b1 100644 --- a/src/applications/differential/controller/DifferentialChangesetViewController.php +++ b/src/applications/differential/controller/DifferentialChangesetViewController.php @@ -287,7 +287,8 @@ final class DifferentialChangesetViewController extends DifferentialController { } return id(new DifferentialInlineCommentQuery()) - ->withViewerAndChangesetIDs($author_phid, $changeset_ids) + ->setViewer($this->getViewer()) + ->withChangesetIDs($changeset_ids) ->execute(); } diff --git a/src/applications/differential/controller/DifferentialInlineCommentEditController.php b/src/applications/differential/controller/DifferentialInlineCommentEditController.php index a247d9f500..dcb48361dc 100644 --- a/src/applications/differential/controller/DifferentialInlineCommentEditController.php +++ b/src/applications/differential/controller/DifferentialInlineCommentEditController.php @@ -39,13 +39,17 @@ final class DifferentialInlineCommentEditController protected function loadComment($id) { return id(new DifferentialInlineCommentQuery()) + ->setViewer($this->getViewer()) ->withIDs(array($id)) + ->withDeletedDrafts(true) ->executeOne(); } protected function loadCommentByPHID($phid) { return id(new DifferentialInlineCommentQuery()) + ->setViewer($this->getViewer()) ->withPHIDs(array($phid)) + ->withDeletedDrafts(true) ->executeOne(); } diff --git a/src/applications/differential/controller/DifferentialInlineCommentPreviewController.php b/src/applications/differential/controller/DifferentialInlineCommentPreviewController.php index 1d555e6589..846cab5439 100644 --- a/src/applications/differential/controller/DifferentialInlineCommentPreviewController.php +++ b/src/applications/differential/controller/DifferentialInlineCommentPreviewController.php @@ -6,8 +6,19 @@ extends PhabricatorInlineCommentPreviewController { protected function loadInlineComments() { $viewer = $this->getViewer(); + $revision = id(new DifferentialRevisionQuery()) + ->setViewer($viewer) + ->withIDs(array($this->getRevisionID())) + ->executeOne(); + if (!$revision) { + return array(); + } + return id(new DifferentialInlineCommentQuery()) - ->withDraftComments($viewer->getPHID(), $this->getRevisionID()) + ->setViewer($this->getViewer()) + ->withDrafts(true) + ->withAuthorPHIDs(array($viewer->getPHID())) + ->withRevisionPHIDs(array($revision->getPHID())) ->execute(); } diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php index eadc085ce7..6905d03cea 100644 --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/DifferentialRevisionViewController.php @@ -96,7 +96,7 @@ final class DifferentialRevisionViewController extends DifferentialController { $props = mpull($props, 'getData', 'getName'); $all_changesets = $changesets; - $inlines = $revision->loadInlineComments($all_changesets); + $inlines = $revision->loadInlineComments($all_changesets, $user); $object_phids = array_merge( $revision->getReviewers(), @@ -158,7 +158,10 @@ final class DifferentialRevisionViewController extends DifferentialController { $warning = $warning->render(); $my_inlines = id(new DifferentialInlineCommentQuery()) - ->withDraftComments($user->getPHID(), $this->revisionID) + ->setViewer($user) + ->withDrafts(true) + ->withAuthorPHIDs(array($user->getPHID())) + ->withRevisionPHIDs(array($revision->getPHID())) ->execute(); $visible_changesets = array(); diff --git a/src/applications/differential/query/DifferentialInlineCommentQuery.php b/src/applications/differential/query/DifferentialInlineCommentQuery.php index 127dcb7c7d..dcc85b1e92 100644 --- a/src/applications/differential/query/DifferentialInlineCommentQuery.php +++ b/src/applications/differential/query/DifferentialInlineCommentQuery.php @@ -6,24 +6,24 @@ final class DifferentialInlineCommentQuery extends PhabricatorOffsetPagedQuery { - private $revisionIDs; - private $notDraft; + // TODO: Remove this when this query eventually moves to PolicyAware. + private $viewer; + private $ids; private $phids; - private $commentIDs; + private $drafts; + private $authorPHIDs; + private $changesetIDs; + private $revisionPHIDs; + private $deletedDrafts; - private $viewerAndChangesetIDs; - private $draftComments; - private $draftsByAuthors; - - public function withRevisionIDs(array $ids) { - $this->revisionIDs = $ids; + public function setViewer(PhabricatorUser $viewer) { + $this->viewer = $viewer; return $this; } - public function withNotDraft($not_draft) { - $this->notDraft = $not_draft; - return $this; + public function getViewer() { + return $this->viewer; } public function withIDs(array $ids) { @@ -36,18 +36,28 @@ final class DifferentialInlineCommentQuery return $this; } - public function withViewerAndChangesetIDs($author_phid, array $ids) { - $this->viewerAndChangesetIDs = array($author_phid, $ids); + public function withDrafts($drafts) { + $this->drafts = $drafts; return $this; } - public function withDraftComments($author_phid, $revision_id) { - $this->draftComments = array($author_phid, $revision_id); + public function withAuthorPHIDs(array $author_phids) { + $this->authorPHIDs = $author_phids; return $this; } - public function withDraftsByAuthors(array $author_phids) { - $this->draftsByAuthors = $author_phids; + public function withChangesetIDs(array $ids) { + $this->changesetIDs = $ids; + return $this; + } + + public function withRevisionPHIDs(array $revision_phids) { + $this->revisionPHIDs = $revision_phids; + return $this; + } + + public function withDeletedDrafts($deleted_drafts) { + $this->deletedDrafts = $deleted_drafts; return $this; } @@ -73,6 +83,7 @@ final class DifferentialInlineCommentQuery } public function executeOne() { + // TODO: Remove when this query moves to PolicyAware. return head($this->execute()); } @@ -84,33 +95,7 @@ final class DifferentialInlineCommentQuery $conn_r, 'changesetID IS NOT NULL'); - if ($this->revisionIDs) { - - // Look up revision PHIDs. - $revision_phids = queryfx_all( - $conn_r, - 'SELECT phid FROM %T WHERE id IN (%Ld)', - id(new DifferentialRevision())->getTableName(), - $this->revisionIDs); - - if (!$revision_phids) { - throw new PhabricatorEmptyQueryException(); - } - $revision_phids = ipull($revision_phids, 'phid'); - - $where[] = qsprintf( - $conn_r, - 'revisionPHID IN (%Ls)', - $revision_phids); - } - - if ($this->notDraft) { - $where[] = qsprintf( - $conn_r, - 'transactionPHID IS NOT NULL'); - } - - if ($this->ids) { + if ($this->ids !== null) { $where[] = qsprintf( $conn_r, 'id IN (%Ld)', @@ -124,44 +109,42 @@ final class DifferentialInlineCommentQuery $this->phids); } - if ($this->viewerAndChangesetIDs) { - list($phid, $ids) = $this->viewerAndChangesetIDs; + if ($this->revisionPHIDs !== null) { $where[] = qsprintf( $conn_r, - 'changesetID IN (%Ld) AND - ((authorPHID = %s AND isDeleted = 0) OR transactionPHID IS NOT NULL)', - $ids, - $phid); + 'revisionPHID IN (%Ls)', + $this->revisionPHIDs); } - if ($this->draftComments) { - list($phid, $rev_id) = $this->draftComments; - - $rev_phid = queryfx_one( + if ($this->changesetIDs !== null) { + $where[] = qsprintf( $conn_r, - 'SELECT phid FROM %T WHERE id = %d', - id(new DifferentialRevision())->getTableName(), - $rev_id); + 'changesetID IN (%Ld)', + $this->changesetIDs); + } - if (!$rev_phid) { - throw new PhabricatorEmptyQueryException(); + if ($this->drafts === null) { + if ($this->deletedDrafts) { + $where[] = qsprintf( + $conn_r, + '(authorPHID = %s) OR (transactionPHID IS NOT NULL)', + $this->getViewer()->getPHID()); + } else { + $where[] = qsprintf( + $conn_r, + '(authorPHID = %s AND isDeleted = 0) + OR (transactionPHID IS NOT NULL)', + $this->getViewer()->getPHID()); } - - $rev_phid = $rev_phid['phid']; - + } else if ($this->drafts) { $where[] = qsprintf( $conn_r, - 'authorPHID = %s AND revisionPHID = %s AND transactionPHID IS NULL - AND isDeleted = 0', - $phid, - $rev_phid); - } - - if ($this->draftsByAuthors) { + '(authorPHID = %s AND isDeleted = 0) AND (transactionPHID IS NULL)', + $this->getViewer()->getPHID()); + } else { $where[] = qsprintf( $conn_r, - 'authorPHID IN (%Ls) AND isDeleted = 0 AND transactionPHID IS NULL', - $this->draftsByAuthors); + 'transactionPHID IS NOT NULL'); } return $this->formatWhereClause($where); diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php index f74139f67a..c1db12e5b3 100644 --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/src/applications/differential/storage/DifferentialRevision.php @@ -281,14 +281,16 @@ final class DifferentialRevision extends DifferentialDAO } public function loadInlineComments( - array &$changesets) { + array &$changesets, + PhabricatorUser $viewer) { assert_instances_of($changesets, 'DifferentialChangeset'); $inline_comments = array(); $inline_comments = id(new DifferentialInlineCommentQuery()) - ->withRevisionIDs(array($this->getID())) - ->withNotDraft(true) + ->setViewer($viewer) + ->withDrafts(false) + ->withRevisionPHIDs(array($this->getPHID())) ->execute(); $load_changesets = array(); @@ -535,7 +537,7 @@ final class DifferentialRevision extends DifferentialDAO ->execute(); // NOTE: this mutates $changesets to include changesets for all inline // comments...! - $inlines = $this->loadInlineComments($changesets); + $inlines = $this->loadInlineComments($changesets, $request->getViewer()); $changesets = mpull($changesets, null, 'getID'); return $timeline @@ -569,18 +571,6 @@ final class DifferentialRevision extends DifferentialDAO self::TABLE_COMMIT, $this->getID()); - try { - $inlines = id(new DifferentialInlineCommentQuery()) - ->withRevisionIDs(array($this->getID())) - ->execute(); - foreach ($inlines as $inline) { - $inline->delete(); - } - } catch (PhabricatorEmptyQueryException $ex) { - // TODO: There's still some funky legacy wrapping going on here, and - // we might catch a raw query exception. - } - // we have to do paths a little differentally as they do not have // an id or phid column for delete() to act on $dummy_path = new DifferentialAffectedPath();