Skip to content
This repository was archived by the owner on Apr 26, 2024. It is now read-only.

Commit d0593a5

Browse files
author
epriestley
committed
Replace "loadDraftAndPublishedComments()" with a Query
Summary: Ref T13513. Continue marching toward coherent query pathways for all access to inline comments. Test Plan: - Viewed a commit and a path within that commit, as a user with unpublished inlines and a different user. - Saw appropriate inlines in all cases (published inlines, plus undeleted unpublished inlines authored by the current viewer). - Grepped for "loadDraftAndPublishedComments()". Maniphest Tasks: T13513 Differential Revision: https://secure.phabricator.com/D21228
1 parent c1f1345 commit d0593a5

File tree

4 files changed

+93
-30
lines changed

4 files changed

+93
-30
lines changed

src/applications/audit/storage/PhabricatorAuditInlineComment.php

Lines changed: 0 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -89,29 +89,6 @@ public static function loadPublishedComments(
8989
return self::buildProxies($inlines);
9090
}
9191

92-
public static function loadDraftAndPublishedComments(
93-
PhabricatorUser $viewer,
94-
$commit_phid,
95-
$path_id = null) {
96-
97-
if ($path_id === null) {
98-
$inlines = id(new PhabricatorAuditTransactionComment())->loadAllWhere(
99-
'commitPHID = %s AND (transactionPHID IS NOT NULL OR authorPHID = %s)
100-
AND pathID IS NOT NULL',
101-
$commit_phid,
102-
$viewer->getPHID());
103-
} else {
104-
$inlines = id(new PhabricatorAuditTransactionComment())->loadAllWhere(
105-
'commitPHID = %s AND pathID = %d AND
106-
((authorPHID = %s AND isDeleted = 0) OR transactionPHID IS NOT NULL)',
107-
$commit_phid,
108-
$path_id,
109-
$viewer->getPHID());
110-
}
111-
112-
return self::buildProxies($inlines);
113-
}
114-
11592
private static function buildProxies(array $inlines) {
11693
$results = array();
11794
foreach ($inlines as $key => $inline) {

src/applications/diffusion/controller/DiffusionCommitController.php

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -414,9 +414,14 @@ public function handleRequest(AphrontRequest $request) {
414414
$visible_changesets = $changesets;
415415
} else {
416416
$visible_changesets = array();
417-
$inlines = PhabricatorAuditInlineComment::loadDraftAndPublishedComments(
418-
$viewer,
419-
$commit->getPHID());
417+
418+
$inlines = id(new DiffusionDiffInlineCommentQuery())
419+
->setViewer($viewer)
420+
->withCommitPHIDs(array($commit->getPHID()))
421+
->withVisibleComments(true)
422+
->execute();
423+
$inlines = mpull($inlines, 'newInlineCommentObject');
424+
420425
$path_ids = mpull($inlines, null, 'getPathID');
421426
foreach ($changesets as $key => $changeset) {
422427
if (array_key_exists($changeset->getID(), $path_ids)) {

src/applications/diffusion/controller/DiffusionDiffController.php

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -99,10 +99,13 @@ public function handleRequest(AphrontRequest $request) {
9999
($viewer->getPHID() == $commit->getAuthorPHID()));
100100
$parser->setObjectOwnerPHID($commit->getAuthorPHID());
101101

102-
$inlines = PhabricatorAuditInlineComment::loadDraftAndPublishedComments(
103-
$viewer,
104-
$commit->getPHID(),
105-
$path_id);
102+
$inlines = id(new DiffusionDiffInlineCommentQuery())
103+
->setViewer($viewer)
104+
->withCommitPHIDs(array($commit->getPHID()))
105+
->withPathIDs(array($path_id))
106+
->withVisibleComments(true)
107+
->execute();
108+
$inlines = mpull($inlines, 'newInlineCommentObject');
106109

107110
if ($inlines) {
108111
foreach ($inlines as $inline) {

src/infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@ abstract class PhabricatorDiffInlineCommentQuery
55

66
private $fixedStates;
77
private $needReplyToComments;
8+
private $visibleComments;
9+
private $publishableComments;
810

911
abstract protected function buildInlineCommentWhereClauseParts(
1012
AphrontDatabaseConnection $conn);
@@ -20,6 +22,16 @@ public function needReplyToComments($need_reply_to) {
2022
return $this;
2123
}
2224

25+
public function withVisibleComments($with_visible) {
26+
$this->visibleComments = $with_visible;
27+
return $this;
28+
}
29+
30+
public function withPublishableComments($with_publishable) {
31+
$this->publishableComments = $with_publishable;
32+
return $this;
33+
}
34+
2335
protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) {
2436
$where = parent::buildWhereClauseParts($conn);
2537
$alias = $this->getPrimaryTableAlias();
@@ -36,6 +48,72 @@ protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) {
3648
$this->fixedStates);
3749
}
3850

51+
$show_published = false;
52+
$show_publishable = false;
53+
54+
if ($this->visibleComments !== null) {
55+
if (!$this->visibleComments) {
56+
throw new Exception(
57+
pht(
58+
'Querying for comments that are not visible is '.
59+
'not supported.'));
60+
}
61+
$show_published = true;
62+
$show_publishable = true;
63+
}
64+
65+
if ($this->publishableComments !== null) {
66+
if (!$this->publishableComments) {
67+
throw new Exception(
68+
pht(
69+
'Querying for comments that are not publishable is '.
70+
'not supported.'));
71+
}
72+
$show_publishable = true;
73+
}
74+
75+
if ($show_publishable || $show_published) {
76+
$clauses = array();
77+
78+
if ($show_published) {
79+
// Published comments are always visible.
80+
$clauses[] = qsprintf(
81+
$conn,
82+
'%T.transactionPHID IS NOT NULL',
83+
$alias);
84+
}
85+
86+
if ($show_publishable) {
87+
$viewer = $this->getViewer();
88+
$viewer_phid = $viewer->getPHID();
89+
90+
// If the viewer has a PHID, unpublished comments they authored and
91+
// have not deleted are visible.
92+
if ($viewer_phid) {
93+
$clauses[] = qsprintf(
94+
$conn,
95+
'%T.authorPHID = %s
96+
AND %T.isDeleted = 0
97+
AND %T.transactionPHID IS NULL ',
98+
$alias,
99+
$viewer_phid,
100+
$alias,
101+
$alias);
102+
}
103+
}
104+
105+
// We can end up with a known-empty query if we (for example) query for
106+
// publishable comments and the viewer is logged-out.
107+
if (!$clauses) {
108+
throw new PhabricatorEmptyQueryException();
109+
}
110+
111+
$where[] = qsprintf(
112+
$conn,
113+
'%LO',
114+
$clauses);
115+
}
116+
39117
return $where;
40118
}
41119

0 commit comments

Comments
 (0)