mirror of
https://we.phorge.it/source/phorge.git
synced 2024-11-19 05:12:41 +01:00
Add "responsible users" and "subscribers" to DifferentialRevisionQuery, plus a
couple bug fixes Summary: - Add the ability to query for "responsible users" (author or reviewer). - Add the ability to query for "subscribers" (reviewer or CC). - Fix an issue where CC and Reviewer used the same join table alias and were incompatible. - Remove support for 'paths' for the moment, since each path needs a repository ID. (There are no clients for this.) - Remove single withX() methods that have no callsites -- withPath() is singular because it accepts two arguments and I didn't want to have an ad-hoc type format, but I think we can get away without these for other conditions. - Include GROUP BY in more cases where may need it. This doesn't actually change program behavior since we uniquify in loadFromArray(), it just means less data over the wire. These new query classes are to support rewriting the Differential list view on top of DifferentialRevisionQuery. Test Plan: - Issued queries via conduit for "responsible users". - Issued queries via conduit for "subscribers". - Issued queries via conduit for "cc" with "reviewer" at the same time. - Issued queries via conduit for "cc", "reviewer", "responsible users" and "subscribers" at the same time. - Issued a "subscribers" and "reviewers" query which returned duplicates; verified GROUP BY took effect. Reviewers: nh, btrahan, jungejason Reviewed By: nh CC: aran, nh Differential Revision: 1182
This commit is contained in:
epriestley
parent
bd12a2b839
commit
682e0aa468
2 changed files with 121 additions and 66 deletions
|
|
@ -39,16 +39,20 @@ class ConduitAPI_differential_query_Method extends ConduitAPIMethod {
|
|||
$order_types = implode(', ', $order_types);
|
||||
|
||||
return array(
|
||||
'authors' => 'optional list<phid>',
|
||||
'ccs' => 'optional list<phid>',
|
||||
'reviewers' => 'optional list<phid>',
|
||||
'paths' => 'optional list<string>',
|
||||
'status' => 'optional enum<'.$status_types.'>',
|
||||
'order' => 'optional enum<'.$order_types.'>',
|
||||
'limit' => 'optional uint',
|
||||
'offset' => 'optional uint',
|
||||
'ids' => 'optional list<uint>',
|
||||
'phids' => 'optional list<phid>',
|
||||
'authors' => 'optional list<phid>',
|
||||
'ccs' => 'optional list<phid>',
|
||||
'reviewers' => 'optional list<phid>',
|
||||
// TODO: Implement this, it needs to accept a repository ID in addition
|
||||
// to a path so the signature needs to be a little more complicated.
|
||||
// 'paths' => 'optional list<pair<...>>',
|
||||
'status' => 'optional enum<'.$status_types.'>',
|
||||
'order' => 'optional enum<'.$order_types.'>',
|
||||
'limit' => 'optional uint',
|
||||
'offset' => 'optional uint',
|
||||
'ids' => 'optional list<uint>',
|
||||
'phids' => 'optional list<phid>',
|
||||
'subscribers' => 'optional list<phid>',
|
||||
'responsibleUsers' => 'optional list<phid>',
|
||||
);
|
||||
}
|
||||
|
||||
|
|
@ -62,30 +66,39 @@ class ConduitAPI_differential_query_Method extends ConduitAPIMethod {
|
|||
}
|
||||
|
||||
protected function execute(ConduitAPIRequest $request) {
|
||||
$authors = $request->getValue('authors');
|
||||
$ccs = $request->getValue('ccs');
|
||||
$reviewers = $request->getValue('reviewers');
|
||||
$paths = $request->getValue('paths');
|
||||
$status = $request->getValue('status');
|
||||
$order = $request->getValue('order');
|
||||
$limit = $request->getValue('limit');
|
||||
$offset = $request->getValue('offset');
|
||||
$ids = $request->getValue('ids');
|
||||
$phids = $request->getValue('phids');
|
||||
$authors = $request->getValue('authors');
|
||||
$ccs = $request->getValue('ccs');
|
||||
$reviewers = $request->getValue('reviewers');
|
||||
$status = $request->getValue('status');
|
||||
$order = $request->getValue('order');
|
||||
$limit = $request->getValue('limit');
|
||||
$offset = $request->getValue('offset');
|
||||
$ids = $request->getValue('ids');
|
||||
$phids = $request->getValue('phids');
|
||||
$subscribers = $request->getValue('subscribers');
|
||||
$responsible_users = $request->getValue('responsibleUsers');
|
||||
|
||||
$query = new DifferentialRevisionQuery();
|
||||
$query->withAuthors($authors);
|
||||
if ($authors) {
|
||||
$query->withAuthors($authors);
|
||||
}
|
||||
if ($ccs) {
|
||||
$query->withCCs($ccs);
|
||||
}
|
||||
if ($reviewers) {
|
||||
$query->withReviewers($reviewers);
|
||||
}
|
||||
/* TODO: Implement.
|
||||
$paths = $request->getValue('paths');
|
||||
if ($paths) {
|
||||
foreach ($paths as $path) {
|
||||
$query->withPath($path);
|
||||
|
||||
// (Lookup the repository IDs.)
|
||||
|
||||
$query->withPath($repository_id, $path);
|
||||
}
|
||||
}
|
||||
*/
|
||||
if ($status) {
|
||||
$query->withStatus($status);
|
||||
}
|
||||
|
|
@ -104,6 +117,12 @@ class ConduitAPI_differential_query_Method extends ConduitAPIMethod {
|
|||
if ($phids) {
|
||||
$query->withPHIDs($phids);
|
||||
}
|
||||
if ($responsible_users) {
|
||||
$query->withResponsibleUsers($responsible_users);
|
||||
}
|
||||
if ($subscribers) {
|
||||
$query->withSubscribers($subscribers);
|
||||
}
|
||||
|
||||
$revisions = $query->execute();
|
||||
|
||||
|
|
|
|||
|
|
@ -43,6 +43,8 @@ final class DifferentialRevisionQuery {
|
|||
private $reviewers = array();
|
||||
private $revIDs = array();
|
||||
private $phids = array();
|
||||
private $subscribers = array();
|
||||
private $responsibles = array();
|
||||
|
||||
private $order = 'order-modified';
|
||||
const ORDER_MODIFIED = 'order-modified';
|
||||
|
|
@ -83,18 +85,6 @@ final class DifferentialRevisionQuery {
|
|||
return $this;
|
||||
}
|
||||
|
||||
/**
|
||||
* Filter results to revisions authored by a given PHID.
|
||||
*
|
||||
* @param phid Author PHID
|
||||
* @return this
|
||||
* @task config
|
||||
*/
|
||||
public function withAuthor($author_phid) {
|
||||
$this->authors[] = $author_phid;
|
||||
return $this;
|
||||
}
|
||||
|
||||
/**
|
||||
* Filter results to revisions authored by one of the given PHIDs.
|
||||
*
|
||||
|
|
@ -102,14 +92,14 @@ final class DifferentialRevisionQuery {
|
|||
* @return this
|
||||
* @task config
|
||||
*/
|
||||
public function withAuthors(array $author_list) {
|
||||
$this->authors = $author_list;
|
||||
public function withAuthors(array $author_phids) {
|
||||
$this->authors = $author_phids;
|
||||
return $this;
|
||||
}
|
||||
|
||||
/**
|
||||
* Filter results to revisions which CC one of the listed people. Calling this
|
||||
* function will clear anything set by previous calls to withCCs or withCC.
|
||||
* function will clear anything set by previous calls to @{method:withCCs}.
|
||||
*
|
||||
* @param array List of PHIDs of subscribers
|
||||
* @return this
|
||||
|
|
@ -120,22 +110,11 @@ final class DifferentialRevisionQuery {
|
|||
return $this;
|
||||
}
|
||||
|
||||
/**
|
||||
* Filter results to include revisions which CC the given PHID.
|
||||
*
|
||||
* @param phid CC PHID
|
||||
* @return this
|
||||
* @task config
|
||||
*/
|
||||
public function withCC($cc) {
|
||||
$this->ccs[] = $cc;
|
||||
return $this;
|
||||
}
|
||||
|
||||
/**
|
||||
* Filter results to revisions that have one of the provided PHIDs as
|
||||
* reviewers. Calling this function will clear anything set by previous calls
|
||||
* to withReviewers or withReviewer.
|
||||
* to @{method:withReviewers}.
|
||||
*
|
||||
* @param array List of PHIDs of reviewers
|
||||
* @return this
|
||||
|
|
@ -146,18 +125,6 @@ final class DifferentialRevisionQuery {
|
|||
return $this;
|
||||
}
|
||||
|
||||
/**
|
||||
* Filter results to include revisions which have the given PHID as a
|
||||
* reviewer.
|
||||
*
|
||||
* @param phid reviewer PHID
|
||||
* @return this
|
||||
* @task config
|
||||
*/
|
||||
public function withReviewer($reviewer) {
|
||||
$this->reviewers[] = $reviewer;
|
||||
return $this;
|
||||
}
|
||||
|
||||
/**
|
||||
* Filter results to revisions with a given status. Provide a class constant,
|
||||
|
|
@ -199,6 +166,34 @@ final class DifferentialRevisionQuery {
|
|||
}
|
||||
|
||||
|
||||
/**
|
||||
* Given a set of users, filter results to return only revisions they are
|
||||
* responsible for (i.e., they are either authors or reviewers).
|
||||
*
|
||||
* @param array List of user PHIDs.
|
||||
* @return this
|
||||
* @task config
|
||||
*/
|
||||
public function withResponsibleUsers(array $responsible_phids) {
|
||||
$this->responsibles = $responsible_phids;
|
||||
return $this;
|
||||
}
|
||||
|
||||
|
||||
/**
|
||||
* Filter results to only return revisions with a given set of subscribers
|
||||
* (i.e., they are authors, reviewers or CC'd).
|
||||
*
|
||||
* @param array List of user PHIDs.
|
||||
* @return this
|
||||
* @task config
|
||||
*/
|
||||
public function withSubscribers(array $subscriber_phids) {
|
||||
$this->subscribers = $subscriber_phids;
|
||||
return $this;
|
||||
}
|
||||
|
||||
|
||||
/**
|
||||
* Set result ordering. Provide a class constant, such as
|
||||
* ##DifferentialRevisionQuery::ORDER_CREATED##.
|
||||
|
|
@ -331,8 +326,9 @@ final class DifferentialRevisionQuery {
|
|||
if ($this->ccs) {
|
||||
$joins[] = qsprintf(
|
||||
$conn_r,
|
||||
'JOIN %T rel ON rel.revisionID = r.id '.
|
||||
'AND rel.relation = %s AND rel.objectPHID in (%Ls)',
|
||||
'JOIN %T cc_rel ON cc_rel.revisionID = r.id '.
|
||||
'AND cc_rel.relation = %s '.
|
||||
'AND cc_rel.objectPHID in (%Ls)',
|
||||
DifferentialRevision::RELATIONSHIP_TABLE,
|
||||
DifferentialRevision::RELATION_SUBSCRIBED,
|
||||
$this->ccs);
|
||||
|
|
@ -341,13 +337,39 @@ final class DifferentialRevisionQuery {
|
|||
if ($this->reviewers) {
|
||||
$joins[] = qsprintf(
|
||||
$conn_r,
|
||||
'JOIN %T rel ON rel.revisionID = r.id '.
|
||||
'AND rel.relation = %s AND rel.objectPHID in (%Ls)',
|
||||
'JOIN %T reviewer_rel ON reviewer_rel.revisionID = r.id '.
|
||||
'AND reviewer_rel.relation = %s '.
|
||||
'AND reviewer_rel.objectPHID in (%Ls)',
|
||||
DifferentialRevision::RELATIONSHIP_TABLE,
|
||||
DifferentialRevision::RELATION_REVIEWER,
|
||||
$this->reviewers);
|
||||
}
|
||||
|
||||
if ($this->subscribers) {
|
||||
$joins[] = qsprintf(
|
||||
$conn_r,
|
||||
'JOIN %T sub_rel ON sub_rel.revisionID = r.id '.
|
||||
'AND sub_rel.relation IN (%Ls) '.
|
||||
'AND sub_rel.objectPHID in (%Ls)',
|
||||
DifferentialRevision::RELATIONSHIP_TABLE,
|
||||
array(
|
||||
DifferentialRevision::RELATION_SUBSCRIBED,
|
||||
DifferentialRevision::RELATION_REVIEWER,
|
||||
),
|
||||
$this->subscribers);
|
||||
}
|
||||
|
||||
if ($this->responsibles) {
|
||||
$joins[] = qsprintf(
|
||||
$conn_r,
|
||||
'LEFT JOIN %T responsibles_rel ON responsibles_rel.revisionID = r.id '.
|
||||
'AND responsibles_rel.relation = %s '.
|
||||
'AND responsibles_rel.objectPHID in (%Ls)',
|
||||
DifferentialRevision::RELATIONSHIP_TABLE,
|
||||
DifferentialRevision::RELATION_REVIEWER,
|
||||
$this->responsibles);
|
||||
}
|
||||
|
||||
$joins = implode(' ', $joins);
|
||||
|
||||
return $joins;
|
||||
|
|
@ -395,6 +417,13 @@ final class DifferentialRevisionQuery {
|
|||
$this->phids);
|
||||
}
|
||||
|
||||
if ($this->responsibles) {
|
||||
$where[] = qsprintf(
|
||||
$conn_r,
|
||||
'(responsibles_rel.objectPHID IS NOT NULL OR r.authorPHID IN (%Ls))',
|
||||
$this->responsibles);
|
||||
}
|
||||
|
||||
switch ($this->status) {
|
||||
case self::STATUS_ANY:
|
||||
break;
|
||||
|
|
@ -427,7 +456,14 @@ final class DifferentialRevisionQuery {
|
|||
* @task internal
|
||||
*/
|
||||
private function buildGroupByClause($conn_r) {
|
||||
$needs_distinct = (count($this->pathIDs) > 1);
|
||||
$join_triggers = array_merge(
|
||||
$this->pathIDs,
|
||||
$this->ccs,
|
||||
$this->reviewers,
|
||||
$this->subscribers,
|
||||
$this->responsibles);
|
||||
|
||||
$needs_distinct = (count($join_triggers) > 1);
|
||||
|
||||
if ($needs_distinct) {
|
||||
return 'GROUP BY r.id';
|
||||
|
|
|
|||
Loading…
Reference in a new issue