diff --git a/src/applications/differential/controller/DifferentialChangesetViewController.php b/src/applications/differential/controller/DifferentialChangesetViewController.php --- a/src/applications/differential/controller/DifferentialChangesetViewController.php +++ b/src/applications/differential/controller/DifferentialChangesetViewController.php @@ -287,7 +287,8 @@ } 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 --- a/src/applications/differential/controller/DifferentialInlineCommentEditController.php +++ b/src/applications/differential/controller/DifferentialInlineCommentEditController.php @@ -39,13 +39,17 @@ 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 --- a/src/applications/differential/controller/DifferentialInlineCommentPreviewController.php +++ b/src/applications/differential/controller/DifferentialInlineCommentPreviewController.php @@ -6,8 +6,19 @@ 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 --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/DifferentialRevisionViewController.php @@ -96,7 +96,7 @@ $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 @@ $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 --- 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 $viewerAndChangesetIDs; - private $draftComments; - private $draftsByAuthors; - - public function withRevisionIDs(array $ids) { - $this->revisionIDs = $ids; + private $drafts; + private $authorPHIDs; + private $changesetIDs; + private $revisionPHIDs; + private $deletedDrafts; + + 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 @@ 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 @@ } public function executeOne() { + // TODO: Remove when this query moves to PolicyAware. return head($this->execute()); } @@ -84,33 +95,7 @@ $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 @@ $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 --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/src/applications/differential/storage/DifferentialRevision.php @@ -281,14 +281,16 @@ } 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 @@ ->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 @@ 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();