From 7ae33d14ec7b682fc664e397fcd011992211ce86 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 19 May 2016 13:28:21 -0700 Subject: [PATCH] Use new Differential bucketing logic on default (non-dashboard) homepage Summary: Ref T10939. If you haven't installed a dashboard, we show an "Active Revisions" panel on the homepage by default. I waited a bit to update this, but the new buckets don't seem to have caused any major problems so far. Update this to use the new logic. I'm just showing "must review" + "should review", which is similar to the old beahvior. Also replace the notification count with this same number. This is a little different from the old behavior, but simpler, and I think we should probably move toward getting rid of these counts completely. Test Plan: - Viewed homepage as logged-in user, saw my revisions (including revisions I have authority over only because of project membership). - Saw consistent notification count. - Grepped for removed method. Reviewers: chad Reviewed By: chad Maniphest Tasks: T10939 Differential Revision: https://secure.phabricator.com/D15950 --- .../PhabricatorDifferentialApplication.php | 122 ++++++++++-------- .../query/DifferentialRevisionQuery.php | 44 ------- ...tialRevisionRequiredActionResultBucket.php | 5 + .../PhabricatorHomeMainController.php | 29 ++--- .../PhabricatorSearchResultBucketGroup.php | 10 ++ 5 files changed, 90 insertions(+), 120 deletions(-) diff --git a/src/applications/differential/application/PhabricatorDifferentialApplication.php b/src/applications/differential/application/PhabricatorDifferentialApplication.php index 787bf52788..ed0053061b 100644 --- a/src/applications/differential/application/PhabricatorDifferentialApplication.php +++ b/src/applications/differential/application/PhabricatorDifferentialApplication.php @@ -102,68 +102,78 @@ final class PhabricatorDifferentialApplication extends PhabricatorApplication { ); } + public static function loadNeedAttentionRevisions(PhabricatorUser $viewer) { + if (!$viewer->isLoggedIn()) { + return array(); + } + + $viewer_phid = $viewer->getPHID(); + + $responsible_phids = id(new DifferentialResponsibleDatasource()) + ->setViewer($viewer) + ->evaluateTokens(array($viewer_phid)); + + $revision_query = id(new DifferentialRevisionQuery()) + ->setViewer($viewer) + ->withStatus(DifferentialRevisionQuery::STATUS_OPEN) + ->withResponsibleUsers($responsible_phids) + ->needReviewerStatus(true) + ->needRelationships(true) + ->needFlags(true) + ->needDrafts(true) + ->setLimit(self::MAX_STATUS_ITEMS); + + $revisions = $revision_query->execute(); + + $query = id(new PhabricatorSavedQuery()) + ->attachParameterMap( + array( + 'responsiblePHIDs' => $responsible_phids, + )); + + $groups = id(new DifferentialRevisionRequiredActionResultBucket()) + ->setViewer($viewer) + ->newResultGroups($query, $revisions); + + $include = array(); + foreach ($groups as $group) { + switch ($group->getKey()) { + case DifferentialRevisionRequiredActionResultBucket::KEY_MUSTREVIEW: + case DifferentialRevisionRequiredActionResultBucket::KEY_SHOULDREVIEW: + foreach ($group->getObjects() as $object) { + $include[] = $object; + } + break; + default: + break; + } + } + + return $include; + } + public function loadStatus(PhabricatorUser $user) { + $revisions = self::loadNeedAttentionRevisions($user); $limit = self::MAX_STATUS_ITEMS; - $revisions = id(new DifferentialRevisionQuery()) - ->setViewer($user) - ->withResponsibleUsers(array($user->getPHID())) - ->withStatus(DifferentialRevisionQuery::STATUS_OPEN) - ->needRelationships(true) - ->setLimit($limit) - ->execute(); + if (count($revisions) >= $limit) { + $display_count = ($limit - 1); + $display_label = pht( + '%s+ Active Review(s)', + new PhutilNumber($display_count)); + } else { + $display_count = count($revisions); + $display_label = pht( + '%s Review(s) Need Attention', + new PhutilNumber($display_count)); + } $status = array(); - if (count($revisions) >= $limit) { - $all_count = count($revisions); - $all_count_str = pht( - '%s+ Active Review(s)', - new PhutilNumber($limit - 1)); - $type = PhabricatorApplicationStatusView::TYPE_WARNING; - $status[] = id(new PhabricatorApplicationStatusView()) - ->setType($type) - ->setText($all_count_str) - ->setCount($all_count); - } else { - list($blocking, $active, $waiting) = - DifferentialRevisionQuery::splitResponsible( - $revisions, - array($user->getPHID())); - - $blocking = count($blocking); - $blocking_str = pht( - '%s Review(s) Blocking Others', - new PhutilNumber($blocking)); - - $type = PhabricatorApplicationStatusView::TYPE_NEEDS_ATTENTION; - $status[] = id(new PhabricatorApplicationStatusView()) - ->setType($type) - ->setText($blocking_str) - ->setCount($blocking); - - $active = count($active); - $active_str = pht( - '%s Review(s) Need Attention', - new PhutilNumber($active)); - - $type = PhabricatorApplicationStatusView::TYPE_WARNING; - $status[] = id(new PhabricatorApplicationStatusView()) - ->setType($type) - ->setText($active_str) - ->setCount($active); - - $waiting = count($waiting); - $waiting_str = pht( - '%s Review(s) Waiting on Others', - new PhutilNumber($waiting)); - - $type = PhabricatorApplicationStatusView::TYPE_INFO; - $status[] = id(new PhabricatorApplicationStatusView()) - ->setType($type) - ->setText($waiting_str) - ->setCount($waiting); - } + $status[] = id(new PhabricatorApplicationStatusView()) + ->setType(PhabricatorApplicationStatusView::TYPE_WARNING) + ->setText($display_label) + ->setCount($display_count); return $status; } diff --git a/src/applications/differential/query/DifferentialRevisionQuery.php b/src/applications/differential/query/DifferentialRevisionQuery.php index c27581edb8..f36a686eb5 100644 --- a/src/applications/differential/query/DifferentialRevisionQuery.php +++ b/src/applications/differential/query/DifferentialRevisionQuery.php @@ -1033,50 +1033,6 @@ final class DifferentialRevisionQuery } } - - public static function splitResponsible(array $revisions, array $user_phids) { - $blocking = array(); - $active = array(); - $waiting = array(); - $status_review = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW; - - // Bucket revisions into $blocking (revisions where you are blocking - // others), $active (revisions you need to do something about) and $waiting - // (revisions you're waiting on someone else to do something about). - foreach ($revisions as $revision) { - $needs_review = ($revision->getStatus() == $status_review); - $filter_is_author = in_array($revision->getAuthorPHID(), $user_phids); - if (!$revision->getReviewers()) { - $needs_review = false; - $author_is_reviewer = false; - } else { - $author_is_reviewer = in_array( - $revision->getAuthorPHID(), - $revision->getReviewers()); - } - - // If exactly one of "needs review" and "the user is the author" is - // true, the user needs to act on it. Otherwise, they're waiting on - // it. - if ($needs_review ^ $filter_is_author) { - if ($needs_review) { - array_unshift($blocking, $revision); - } else { - $active[] = $revision; - } - // User is author **and** reviewer. An exotic but configurable workflow. - // User needs to act on it double. - } else if ($needs_review && $author_is_reviewer) { - array_unshift($blocking, $revision); - $active[] = $revision; - } else { - $waiting[] = $revision; - } - } - - return array($blocking, $active, $waiting); - } - private function loadReviewerAuthority( array $revisions, array $edges, diff --git a/src/applications/differential/query/DifferentialRevisionRequiredActionResultBucket.php b/src/applications/differential/query/DifferentialRevisionRequiredActionResultBucket.php index 28e6e6c132..b9fec508c5 100644 --- a/src/applications/differential/query/DifferentialRevisionRequiredActionResultBucket.php +++ b/src/applications/differential/query/DifferentialRevisionRequiredActionResultBucket.php @@ -5,6 +5,9 @@ final class DifferentialRevisionRequiredActionResultBucket const BUCKETKEY = 'action'; + const KEY_MUSTREVIEW = 'must-review'; + const KEY_SHOULDREVIEW = 'should-review'; + private $objects; public function getResultBucketName() { @@ -30,11 +33,13 @@ final class DifferentialRevisionRequiredActionResultBucket $groups[] = $this->newGroup() ->setName(pht('Must Review')) + ->setKey(self::KEY_MUSTREVIEW) ->setNoDataString(pht('No revisions are blocked on your review.')) ->setObjects($this->filterMustReview($phids)); $groups[] = $this->newGroup() ->setName(pht('Ready to Review')) + ->setKey(self::KEY_SHOULDREVIEW) ->setNoDataString(pht('No revisions are waiting on you to review them.')) ->setObjects($this->filterShouldReview($phids)); diff --git a/src/applications/home/controller/PhabricatorHomeMainController.php b/src/applications/home/controller/PhabricatorHomeMainController.php index 950944188b..c5387a5fb7 100644 --- a/src/applications/home/controller/PhabricatorHomeMainController.php +++ b/src/applications/home/controller/PhabricatorHomeMainController.php @@ -205,40 +205,29 @@ final class PhabricatorHomeMainController extends PhabricatorHomeController { } private function buildRevisionPanel() { - $user = $this->getRequest()->getUser(); - $user_phid = $user->getPHID(); + $viewer = $this->getViewer(); - $revision_query = id(new DifferentialRevisionQuery()) - ->setViewer($user) - ->withStatus(DifferentialRevisionQuery::STATUS_OPEN) - ->withResponsibleUsers(array($user_phid)) - ->needRelationships(true) - ->needFlags(true) - ->needDrafts(true); + $revisions = PhabricatorDifferentialApplication::loadNeedAttentionRevisions( + $viewer); - $revisions = $revision_query->execute(); - - list($blocking, $active) = DifferentialRevisionQuery::splitResponsible( - $revisions, - array($user_phid)); - - if (!$blocking && !$active) { + if (!$revisions) { return $this->renderMiniPanel( pht('No Waiting Revisions'), pht('No revisions are waiting on you.')); } $title = pht('Revisions Waiting on You'); - $href = '/differential'; + $href = '/differential/'; $panel = new PHUIObjectBoxView(); $panel->setHeader($this->renderSectionHeader($title, $href)); $revision_view = id(new DifferentialRevisionListView()) ->setHighlightAge(true) - ->setRevisions(array_merge($blocking, $active)) - ->setUser($user); + ->setRevisions($revisions) + ->setUser($viewer); + $phids = array_merge( - array($user_phid), + array($viewer->getPHID()), $revision_view->getRequiredHandlePHIDs()); $handles = $this->loadViewerHandles($phids); diff --git a/src/applications/search/buckets/PhabricatorSearchResultBucketGroup.php b/src/applications/search/buckets/PhabricatorSearchResultBucketGroup.php index 2c685d8c98..2fcdad13e4 100644 --- a/src/applications/search/buckets/PhabricatorSearchResultBucketGroup.php +++ b/src/applications/search/buckets/PhabricatorSearchResultBucketGroup.php @@ -4,6 +4,7 @@ final class PhabricatorSearchResultBucketGroup extends Phobject { private $name; + private $key; private $noDataString; private $objects; @@ -25,6 +26,15 @@ final class PhabricatorSearchResultBucketGroup return $this->name; } + public function setKey($key) { + $this->key = $key; + return $this; + } + + public function getKey() { + return $this->key; + } + public function setObjects(array $objects) { $this->objects = $objects; return $this;