From 42d49be47bfcbad59ce0634f06a99ba43a40d389 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 16 May 2016 08:09:11 -0700 Subject: [PATCH] Change Differential revision buckets to focus on "next required action" Summary: Ref T10939. Ref T4144. This splits the existing buckets ("Blocking Others", "Action Required", "Waiting on Others") into 6-7 buckets with a stronger focus on what the next action you need to take is. See T10939#175423 for some discussion. Overall, I think some of the root problems here are caused by reviewer laziness and shotgun review workflows (where a ton of people get automatically added to everything, probably unnecessarily), but these buckets haven't been updated since the introduction of blocking reviewers or project/package reviewers and I think splitting the 3 buckets into 6 buckets isn't unreasonable, even though it's kind of a lot of buckets and the root problem here is approximately "I want to ignore a bunch of stuff on my dashboard". I didn't remove the old bucketing code yet since it's still in use on the default homepage. This also isn't quite right until I fix the tokenizer to work properly, since it won't bucket project/package reviewers accurately. Test Plan: {F1395972} Reviewers: chad Reviewed By: chad Maniphest Tasks: T4144, T10939 Differential Revision: https://secure.phabricator.com/D15924 --- src/__phutil_library_map__.php | 2 + ...tialRevisionRequiredActionResultBucket.php | 188 ++++++++++++++++++ .../DifferentialRevisionResultBucket.php | 64 ++++++ .../DifferentialRevisionSearchEngine.php | 40 ++-- .../buckets/PhabricatorSearchResultBucket.php | 23 +++ .../PhabricatorSearchResultBucketGroup.php | 37 ++++ ...PhabricatorApplicationSearchController.php | 12 +- 7 files changed, 339 insertions(+), 27 deletions(-) create mode 100644 src/applications/search/buckets/PhabricatorSearchResultBucketGroup.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index f34305d91f..59362a1e8f 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3320,6 +3320,7 @@ phutil_register_library_map(array( 'PhabricatorSearchPreferencesSettingsPanel' => 'applications/settings/panel/PhabricatorSearchPreferencesSettingsPanel.php', 'PhabricatorSearchRelationship' => 'applications/search/constants/PhabricatorSearchRelationship.php', 'PhabricatorSearchResultBucket' => 'applications/search/buckets/PhabricatorSearchResultBucket.php', + 'PhabricatorSearchResultBucketGroup' => 'applications/search/buckets/PhabricatorSearchResultBucketGroup.php', 'PhabricatorSearchResultView' => 'applications/search/view/PhabricatorSearchResultView.php', 'PhabricatorSearchSchemaSpec' => 'applications/search/storage/PhabricatorSearchSchemaSpec.php', 'PhabricatorSearchSelectController' => 'applications/search/controller/PhabricatorSearchSelectController.php', @@ -8015,6 +8016,7 @@ phutil_register_library_map(array( 'PhabricatorSearchPreferencesSettingsPanel' => 'PhabricatorSettingsPanel', 'PhabricatorSearchRelationship' => 'Phobject', 'PhabricatorSearchResultBucket' => 'Phobject', + 'PhabricatorSearchResultBucketGroup' => 'Phobject', 'PhabricatorSearchResultView' => 'AphrontView', 'PhabricatorSearchSchemaSpec' => 'PhabricatorConfigSchemaSpec', 'PhabricatorSearchSelectController' => 'PhabricatorSearchBaseController', diff --git a/src/applications/differential/query/DifferentialRevisionRequiredActionResultBucket.php b/src/applications/differential/query/DifferentialRevisionRequiredActionResultBucket.php index 14e113cdca..269a8a2cd7 100644 --- a/src/applications/differential/query/DifferentialRevisionRequiredActionResultBucket.php +++ b/src/applications/differential/query/DifferentialRevisionRequiredActionResultBucket.php @@ -5,8 +5,196 @@ final class DifferentialRevisionRequiredActionResultBucket const BUCKETKEY = 'action'; + private $objects; + public function getResultBucketName() { return pht('Bucket by Required Action'); } + protected function buildResultGroups( + PhabricatorSavedQuery $query, + array $objects) { + + $this->objects = $objects; + + $phids = $query->getParameter('responsiblePHIDs', array()); + if (!$phids) { + throw new Exception( + pht( + 'You can not bucket results by required action without '. + 'specifying "Responsible Users".')); + } + $phids = array_fuse($phids); + + $groups = array(); + + $groups[] = $this->newGroup() + ->setName(pht('Must Review')) + ->setNoDataString(pht('No revisions are blocked on your review.')) + ->setObjects($this->filterMustReview($phids)); + + $groups[] = $this->newGroup() + ->setName(pht('Ready to Review')) + ->setNoDataString(pht('No revisions are waiting on you to review them.')) + ->setObjects($this->filterShouldReview($phids)); + + $groups[] = $this->newGroup() + ->setName(pht('Ready to Land')) + ->setNoDataString(pht('No revisions are ready to land.')) + ->setObjects($this->filterShouldLand($phids)); + + $groups[] = $this->newGroup() + ->setName(pht('Ready to Update')) + ->setNoDataString(pht('No revisions are waiting for updates.')) + ->setObjects($this->filterShouldUpdate($phids)); + + $groups[] = $this->newGroup() + ->setName(pht('Waiting on Review')) + ->setNoDataString(pht('None of your revisions are waiting on review.')) + ->setObjects($this->filterWaitingForReview($phids)); + + $groups[] = $this->newGroup() + ->setName(pht('Waiting on Authors')) + ->setNoDataString(pht('No revisions are waiting on author action.')) + ->setObjects($this->filterWaitingOnAuthors($phids)); + + // Because you can apply these buckets to queries which include revisions + // that have been closed, add an "Other" bucket if we still have stuff + // that didn't get filtered into any of the previous buckets. + if ($this->objects) { + $groups[] = $this->newGroup() + ->setName(pht('Other Revisions')) + ->setObjects($this->objects); + } + + return $groups; + } + + private function filterMustReview(array $phids) { + $blocking = array( + DifferentialReviewerStatus::STATUS_BLOCKING, + DifferentialReviewerStatus::STATUS_REJECTED, + DifferentialReviewerStatus::STATUS_REJECTED_OLDER, + ); + $blocking = array_fuse($blocking); + + $objects = $this->getRevisionsUnderReview($this->objects, $phids); + + $results = array(); + foreach ($objects as $key => $object) { + if (!$this->hasReviewersWithStatus($object, $phids, $blocking)) { + continue; + } + + $results[$key] = $object; + unset($this->objects[$key]); + } + + return $results; + } + + private function filterShouldReview(array $phids) { + $reviewing = array( + DifferentialReviewerStatus::STATUS_ADDED, + ); + $reviewing = array_fuse($reviewing); + + $objects = $this->getRevisionsUnderReview($this->objects, $phids); + + $results = array(); + foreach ($objects as $key => $object) { + if (!$this->hasReviewersWithStatus($object, $phids, $reviewing)) { + continue; + } + + $results[$key] = $object; + unset($this->objects[$key]); + } + + return $results; + } + + private function filterShouldLand(array $phids) { + $status_accepted = ArcanistDifferentialRevisionStatus::ACCEPTED; + + $objects = $this->getRevisionsAuthored($this->objects, $phids); + + $results = array(); + foreach ($objects as $key => $object) { + if ($object->getStatus() != $status_accepted) { + continue; + } + + $results[$key] = $object; + unset($this->objects[$key]); + } + + return $results; + } + + private function filterShouldUpdate(array $phids) { + $statuses = array( + ArcanistDifferentialRevisionStatus::NEEDS_REVISION, + ArcanistDifferentialRevisionStatus::CHANGES_PLANNED, + ArcanistDifferentialRevisionStatus::IN_PREPARATION, + ); + $statuses = array_fuse($statuses); + + $objects = $this->getRevisionsAuthored($this->objects, $phids); + + $results = array(); + foreach ($objects as $key => $object) { + if (empty($statuses[$object->getStatus()])) { + continue; + } + + $results[$key] = $object; + unset($this->objects[$key]); + } + + return $results; + } + + private function filterWaitingForReview(array $phids) { + $status_review = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW; + + $objects = $this->getRevisionsAuthored($this->objects, $phids); + + $results = array(); + foreach ($objects as $key => $object) { + if ($object->getStatus() != $status_review) { + continue; + } + + $results[$key] = $object; + unset($this->objects[$key]); + } + + return $results; + } + + private function filterWaitingOnAuthors(array $phids) { + $statuses = array( + ArcanistDifferentialRevisionStatus::ACCEPTED, + ArcanistDifferentialRevisionStatus::NEEDS_REVISION, + ArcanistDifferentialRevisionStatus::CHANGES_PLANNED, + ArcanistDifferentialRevisionStatus::IN_PREPARATION, + ); + $statuses = array_fuse($statuses); + + $objects = $this->getRevisionsNotAuthored($this->objects, $phids); + + $results = array(); + foreach ($objects as $key => $object) { + if (empty($statuses[$object->getStatus()])) { + continue; + } + + $results[$key] = $object; + unset($this->objects[$key]); + } + + return $results; + } + } diff --git a/src/applications/differential/query/DifferentialRevisionResultBucket.php b/src/applications/differential/query/DifferentialRevisionResultBucket.php index 2d455895b2..75ce5ea853 100644 --- a/src/applications/differential/query/DifferentialRevisionResultBucket.php +++ b/src/applications/differential/query/DifferentialRevisionResultBucket.php @@ -10,4 +10,68 @@ abstract class DifferentialRevisionResultBucket ->execute(); } + protected function getRevisionsUnderReview(array $objects, array $phids) { + $results = array(); + + $objects = $this->getRevisionsNotAuthored($objects, $phids); + + $status_review = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW; + foreach ($objects as $key => $object) { + if ($object->getStatus() !== $status_review) { + continue; + } + + $results[$key] = $object; + } + + return $results; + } + + protected function getRevisionsAuthored(array $objects, array $phids) { + $results = array(); + + foreach ($objects as $key => $object) { + if (isset($phids[$object->getAuthorPHID()])) { + $results[$key] = $object; + } + } + + return $results; + } + + protected function getRevisionsNotAuthored(array $objects, array $phids) { + $results = array(); + + foreach ($objects as $key => $object) { + if (empty($phids[$object->getAuthorPHID()])) { + $results[$key] = $object; + } + } + + return $results; + } + + protected function hasReviewersWithStatus( + DifferentialRevision $revision, + array $phids, + array $statuses) { + + foreach ($revision->getReviewerStatus() as $reviewer) { + $reviewer_phid = $reviewer->getReviewerPHID(); + if (empty($phids[$reviewer_phid])) { + continue; + } + + $status = $reviewer->getStatus(); + if (empty($statuses[$status])) { + continue; + } + + return true; + } + + return false; + } + + } diff --git a/src/applications/differential/query/DifferentialRevisionSearchEngine.php b/src/applications/differential/query/DifferentialRevisionSearchEngine.php index 6a0ca74b46..814c9c997d 100644 --- a/src/applications/differential/query/DifferentialRevisionSearchEngine.php +++ b/src/applications/differential/query/DifferentialRevisionSearchEngine.php @@ -19,7 +19,8 @@ final class DifferentialRevisionSearchEngine return id(new DifferentialRevisionQuery()) ->needFlags(true) ->needDrafts(true) - ->needRelationships(true); + ->needRelationships(true) + ->needReviewerStatus(true); } protected function buildQueryFromParameters(array $map) { @@ -153,33 +154,20 @@ final class DifferentialRevisionSearchEngine $views = array(); if ($bucket) { - $split = DifferentialRevisionQuery::splitResponsible( - $revisions, - $query->getParameter('responsiblePHIDs')); - list($blocking, $active, $waiting) = $split; + $bucket->setViewer($viewer); - $views[] = id(clone $template) - ->setHeader(pht('Blocking Others')) - ->setNoDataString( - pht('No revisions are blocked on your action.')) - ->setHighlightAge(true) - ->setRevisions($blocking) - ->setHandles(array()); + try { + $groups = $bucket->newResultGroups($query, $revisions); - $views[] = id(clone $template) - ->setHeader(pht('Action Required')) - ->setNoDataString( - pht('No revisions require your action.')) - ->setHighlightAge(true) - ->setRevisions($active) - ->setHandles(array()); - - $views[] = id(clone $template) - ->setHeader(pht('Waiting on Others')) - ->setNoDataString( - pht('You have no revisions waiting on others.')) - ->setRevisions($waiting) - ->setHandles(array()); + foreach ($groups as $group) { + $views[] = id(clone $template) + ->setHeader($group->getName()) + ->setNoDataString($group->getNoDataString()) + ->setRevisions($group->getObjects()); + } + } catch (Exception $ex) { + $this->addError($ex->getMessage()); + } } else { $views[] = id(clone $template) ->setRevisions($revisions) diff --git a/src/applications/search/buckets/PhabricatorSearchResultBucket.php b/src/applications/search/buckets/PhabricatorSearchResultBucket.php index a614e65d10..16e5a6150f 100644 --- a/src/applications/search/buckets/PhabricatorSearchResultBucket.php +++ b/src/applications/search/buckets/PhabricatorSearchResultBucket.php @@ -3,6 +3,7 @@ abstract class PhabricatorSearchResultBucket extends Phobject { + private $viewer; private $pageSize; final public function setPageSize($page_size) { @@ -18,14 +19,36 @@ abstract class PhabricatorSearchResultBucket return $this->pageSize; } + public function setViewer(PhabricatorUser $viewer) { + $this->viewer = $viewer; + return $this; + } + + public function getViewer() { + return $this->viewer; + } + protected function getDefaultPageSize() { return 1000; } abstract public function getResultBucketName(); + abstract protected function buildResultGroups( + PhabricatorSavedQuery $query, + array $objects); + + final public function newResultGroups( + PhabricatorSavedQuery $query, + array $objects) { + return $this->buildResultGroups($query, $objects); + } final public function getResultBucketKey() { return $this->getPhobjectClassConstant('BUCKETKEY'); } + final protected function newGroup() { + return new PhabricatorSearchResultBucketGroup(); + } + } diff --git a/src/applications/search/buckets/PhabricatorSearchResultBucketGroup.php b/src/applications/search/buckets/PhabricatorSearchResultBucketGroup.php new file mode 100644 index 0000000000..2c685d8c98 --- /dev/null +++ b/src/applications/search/buckets/PhabricatorSearchResultBucketGroup.php @@ -0,0 +1,37 @@ +noDataString = $no_data_string; + return $this; + } + + public function getNoDataString() { + return $this->noDataString; + } + + public function setName($name) { + $this->name = $name; + return $this; + } + + public function getName() { + return $this->name; + } + + public function setObjects(array $objects) { + $this->objects = $objects; + return $this; + } + + public function getObjects() { + return $this->objects; + } + +} diff --git a/src/applications/search/controller/PhabricatorApplicationSearchController.php b/src/applications/search/controller/PhabricatorApplicationSearchController.php index 3b30d5d30c..688a7db527 100644 --- a/src/applications/search/controller/PhabricatorApplicationSearchController.php +++ b/src/applications/search/controller/PhabricatorApplicationSearchController.php @@ -213,6 +213,8 @@ final class PhabricatorApplicationSearchController if ($run_query) { + $exec_errors = array(); + $box->setAnchor( id(new PhabricatorAnchorView()) ->setAnchorName('R')); @@ -280,10 +282,18 @@ final class PhabricatorApplicationSearchController } } } catch (PhabricatorTypeaheadInvalidTokenException $ex) { - $errors[] = pht( + $exec_errors[] = pht( 'This query specifies an invalid parameter. Review the '. 'query parameters and correct errors.'); } + + // The engine may have encountered additional errors during rendering; + // merge them in and show everything. + foreach ($engine->getErrors() as $error) { + $exec_errors[] = $error; + } + + $errors = $exec_errors; } if ($errors) {