From 7c1f6519e0b2cfde0d7a5d938ead8db29534d34f Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 21 May 2019 07:22:38 -0700 Subject: [PATCH] Support "none()" in Differential to find revisions with no (un-resigned) reviewers Summary: Ref T13289. In Maniphest, you can currently search for "Owner: none()" to find tasks with no owner, but there's no way to search for "Reviewers: none()" in Differential right now. Add support for this, since it's consistent and reasonable and doesn't seem too weird or niche. Test Plan: Searched for "Reviewers: none()", found revisions with no reviewers. Searched for "Reviewers: alice, none()", "Reviewers: alice", and "Reviewers: " and got sensible results. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13289 Differential Revision: https://secure.phabricator.com/D20537 --- src/__phutil_library_map__.php | 4 + .../query/DifferentialRevisionQuery.php | 60 +++++++++++++- .../DifferentialRevisionSearchEngine.php | 2 +- .../DifferentialNoReviewersDatasource.php | 78 +++++++++++++++++++ ...DifferentialReviewerFunctionDatasource.php | 26 +++++++ .../view/DifferentialRevisionListView.php | 10 +++ 6 files changed, 177 insertions(+), 3 deletions(-) create mode 100644 src/applications/differential/typeahead/DifferentialNoReviewersDatasource.php create mode 100644 src/applications/differential/typeahead/DifferentialReviewerFunctionDatasource.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 20de984e5a..5d4d64641b 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -538,6 +538,7 @@ phutil_register_library_map(array( 'DifferentialMailEngineExtension' => 'applications/differential/engineextension/DifferentialMailEngineExtension.php', 'DifferentialMailView' => 'applications/differential/mail/DifferentialMailView.php', 'DifferentialManiphestTasksField' => 'applications/differential/customfield/DifferentialManiphestTasksField.php', + 'DifferentialNoReviewersDatasource' => 'applications/differential/typeahead/DifferentialNoReviewersDatasource.php', 'DifferentialParseCacheGarbageCollector' => 'applications/differential/garbagecollector/DifferentialParseCacheGarbageCollector.php', 'DifferentialParseCommitMessageConduitAPIMethod' => 'applications/differential/conduit/DifferentialParseCommitMessageConduitAPIMethod.php', 'DifferentialParseRenderTestCase' => 'applications/differential/__tests__/DifferentialParseRenderTestCase.php', @@ -561,6 +562,7 @@ phutil_register_library_map(array( 'DifferentialReviewer' => 'applications/differential/storage/DifferentialReviewer.php', 'DifferentialReviewerDatasource' => 'applications/differential/typeahead/DifferentialReviewerDatasource.php', 'DifferentialReviewerForRevisionEdgeType' => 'applications/differential/edge/DifferentialReviewerForRevisionEdgeType.php', + 'DifferentialReviewerFunctionDatasource' => 'applications/differential/typeahead/DifferentialReviewerFunctionDatasource.php', 'DifferentialReviewerStatus' => 'applications/differential/constants/DifferentialReviewerStatus.php', 'DifferentialReviewersAddBlockingReviewersHeraldAction' => 'applications/differential/herald/DifferentialReviewersAddBlockingReviewersHeraldAction.php', 'DifferentialReviewersAddBlockingSelfHeraldAction' => 'applications/differential/herald/DifferentialReviewersAddBlockingSelfHeraldAction.php', @@ -6197,6 +6199,7 @@ phutil_register_library_map(array( 'DifferentialMailEngineExtension' => 'PhabricatorMailEngineExtension', 'DifferentialMailView' => 'Phobject', 'DifferentialManiphestTasksField' => 'DifferentialCoreCustomField', + 'DifferentialNoReviewersDatasource' => 'PhabricatorTypeaheadDatasource', 'DifferentialParseCacheGarbageCollector' => 'PhabricatorGarbageCollector', 'DifferentialParseCommitMessageConduitAPIMethod' => 'DifferentialConduitAPIMethod', 'DifferentialParseRenderTestCase' => 'PhabricatorTestCase', @@ -6220,6 +6223,7 @@ phutil_register_library_map(array( 'DifferentialReviewer' => 'DifferentialDAO', 'DifferentialReviewerDatasource' => 'PhabricatorTypeaheadCompositeDatasource', 'DifferentialReviewerForRevisionEdgeType' => 'PhabricatorEdgeType', + 'DifferentialReviewerFunctionDatasource' => 'PhabricatorTypeaheadCompositeDatasource', 'DifferentialReviewerStatus' => 'Phobject', 'DifferentialReviewersAddBlockingReviewersHeraldAction' => 'DifferentialReviewersHeraldAction', 'DifferentialReviewersAddBlockingSelfHeraldAction' => 'DifferentialReviewersHeraldAction', diff --git a/src/applications/differential/query/DifferentialRevisionQuery.php b/src/applications/differential/query/DifferentialRevisionQuery.php index 738df3156a..84f9f07f61 100644 --- a/src/applications/differential/query/DifferentialRevisionQuery.php +++ b/src/applications/differential/query/DifferentialRevisionQuery.php @@ -26,6 +26,7 @@ final class DifferentialRevisionQuery private $isOpen; private $createdEpochMin; private $createdEpochMax; + private $noReviewers; const ORDER_MODIFIED = 'order-modified'; const ORDER_CREATED = 'order-created'; @@ -98,7 +99,31 @@ final class DifferentialRevisionQuery * @task config */ public function withReviewers(array $reviewer_phids) { - $this->reviewers = $reviewer_phids; + if ($reviewer_phids === array()) { + throw new Exception( + pht( + 'Empty "withReviewers()" constraint is invalid. Provide one or '. + 'more values, or remove the constraint.')); + } + + $with_none = false; + + foreach ($reviewer_phids as $key => $phid) { + switch ($phid) { + case DifferentialNoReviewersDatasource::FUNCTION_TOKEN: + $with_none = true; + unset($reviewer_phids[$key]); + break; + default: + break; + } + } + + $this->noReviewers = $with_none; + if ($reviewer_phids) { + $this->reviewers = array_values($reviewer_phids); + } + return $this; } @@ -572,7 +597,7 @@ final class DifferentialRevisionQuery if ($this->reviewers) { $joins[] = qsprintf( $conn, - 'JOIN %T reviewer ON reviewer.revisionPHID = r.phid + 'LEFT JOIN %T reviewer ON reviewer.revisionPHID = r.phid AND reviewer.reviewerStatus != %s AND reviewer.reviewerPHID in (%Ls)', id(new DifferentialReviewer())->getTableName(), @@ -580,6 +605,15 @@ final class DifferentialRevisionQuery $this->reviewers); } + if ($this->noReviewers) { + $joins[] = qsprintf( + $conn, + 'LEFT JOIN %T no_reviewer ON no_reviewer.revisionPHID = r.phid + AND no_reviewer.reviewerStatus != %s', + id(new DifferentialReviewer())->getTableName(), + DifferentialReviewerStatus::STATUS_RESIGNED); + } + if ($this->draftAuthors) { $joins[] = qsprintf( $conn, @@ -715,6 +749,24 @@ final class DifferentialRevisionQuery $statuses); } + $reviewer_subclauses = array(); + + if ($this->noReviewers) { + $reviewer_subclauses[] = qsprintf( + $conn, + 'no_reviewer.reviewerPHID IS NULL'); + } + + if ($this->reviewers) { + $reviewer_subclauses[] = qsprintf( + $conn, + 'reviewer.reviewerPHID IS NOT NULL'); + } + + if ($reviewer_subclauses) { + $where[] = qsprintf($conn, '%LO', $reviewer_subclauses); + } + $where[] = $this->buildWhereClauseParts($conn); return $this->formatWhereClause($conn, $where); @@ -735,6 +787,10 @@ final class DifferentialRevisionQuery return true; } + if ($this->noReviewers) { + return true; + } + return parent::shouldGroupQueryResultRows(); } diff --git a/src/applications/differential/query/DifferentialRevisionSearchEngine.php b/src/applications/differential/query/DifferentialRevisionSearchEngine.php index be753c5e17..14c9dd0301 100644 --- a/src/applications/differential/query/DifferentialRevisionSearchEngine.php +++ b/src/applications/differential/query/DifferentialRevisionSearchEngine.php @@ -73,7 +73,7 @@ final class DifferentialRevisionSearchEngine ->setLabel(pht('Reviewers')) ->setKey('reviewerPHIDs') ->setAliases(array('reviewer', 'reviewers', 'reviewerPHID')) - ->setDatasource(new DiffusionAuditorFunctionDatasource()) + ->setDatasource(new DifferentialReviewerFunctionDatasource()) ->setDescription( pht('Find revisions with specific reviewers.')), id(new PhabricatorSearchDatasourceField()) diff --git a/src/applications/differential/typeahead/DifferentialNoReviewersDatasource.php b/src/applications/differential/typeahead/DifferentialNoReviewersDatasource.php new file mode 100644 index 0000000000..083b6c7cb5 --- /dev/null +++ b/src/applications/differential/typeahead/DifferentialNoReviewersDatasource.php @@ -0,0 +1,78 @@ + array( + 'name' => pht('No Reviewers'), + 'summary' => pht('Find results which have no reviewers.'), + 'description' => pht( + "This function includes results which have no reviewers. Use a ". + "query like this to find results with no reviewers:\n\n%s\n\n". + "If you combine this function with other functions, the query will ". + "return results which match the other selectors //or// have no ". + "reviewers. For example, this query will find results which have ". + "`alincoln` as a reviewer, and will also find results which have ". + "no reviewers:". + "\n\n%s", + '> none()', + '> alincoln, none()'), + ), + ); + } + + public function loadResults() { + $results = array( + $this->buildNoReviewersResult(), + ); + return $this->filterResultsAgainstTokens($results); + } + + protected function evaluateFunction($function, array $argv_list) { + $results = array(); + + foreach ($argv_list as $argv) { + $results[] = self::FUNCTION_TOKEN; + } + + return $results; + } + + public function renderFunctionTokens($function, array $argv_list) { + $results = array(); + foreach ($argv_list as $argv) { + $results[] = PhabricatorTypeaheadTokenView::newFromTypeaheadResult( + $this->buildNoReviewersResult()); + } + return $results; + } + + private function buildNoReviewersResult() { + $name = pht('No Reviewers'); + + return $this->newFunctionResult() + ->setName($name.' none') + ->setDisplayName($name) + ->setIcon('fa-ban') + ->setPHID('none()') + ->setUnique(true) + ->addAttribute(pht('Select results with no reviewers.')); + } + +} diff --git a/src/applications/differential/typeahead/DifferentialReviewerFunctionDatasource.php b/src/applications/differential/typeahead/DifferentialReviewerFunctionDatasource.php new file mode 100644 index 0000000000..3b79016055 --- /dev/null +++ b/src/applications/differential/typeahead/DifferentialReviewerFunctionDatasource.php @@ -0,0 +1,26 @@ +revisions as $key => $revision) { $reviewers = $revision->getReviewers(); + + // Don't show reviewers who have resigned. The "Reviewers" constraint + // does not respect these reviewers and they largely don't count as + // reviewers. + foreach ($reviewers as $reviewer_key => $reviewer) { + if ($reviewer->isResigned()) { + unset($reviewers[$reviewer_key]); + } + } + if (count($reviewers) > $reviewer_limit) { $reviewers = array_slice($reviewers, 0, $reviewer_limit); $reviewer_more[$key] = true;