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

@@ -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',

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;
+  }
+
 }

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;
+  }
+
+
 }

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(
+      $bucket->setViewer($viewer);
-          $revisions,
-          $query->getParameter('responsiblePHIDs'));
-        list($blocking, $active, $waiting) = $split;
 
-      $views[] = id(clone $template)
+      try {
-        ->setHeader(pht('Blocking Others'))
+        $groups = $bucket->newResultGroups($query, $revisions);
-        ->setNoDataString(
-          pht('No revisions are blocked on your action.'))
-        ->setHighlightAge(true)
-        ->setRevisions($blocking)
-        ->setHandles(array());
 
-      $views[] = id(clone $template)
+        foreach ($groups as $group) {
-        ->setHeader(pht('Action Required'))
+          $views[] = id(clone $template)
-        ->setNoDataString(
+            ->setHeader($group->getName())
-          pht('No revisions require your action.'))
+            ->setNoDataString($group->getNoDataString())
-        ->setHighlightAge(true)
+            ->setRevisions($group->getObjects());
-        ->setRevisions($active)
+        }
-        ->setHandles(array());
+      } catch (Exception $ex) {
-
+        $this->addError($ex->getMessage());
-      $views[] = id(clone $template)
+      }
-        ->setHeader(pht('Waiting on Others'))
-        ->setNoDataString(
-          pht('You have no revisions waiting on others.'))
-        ->setRevisions($waiting)
-        ->setHandles(array());
     } else {
       $views[] = id(clone $template)
         ->setRevisions($revisions)

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();
+  }
+
 }

src/applications/search/buckets/PhabricatorSearchResultBucketGroup.php

@@ -0,0 +1,37 @@
+<?php
+
+final class PhabricatorSearchResultBucketGroup
+  extends Phobject {
+
+  private $name;
+  private $noDataString;
+  private $objects;
+
+  public function setNoDataString($no_data_string) {
+    $this->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;
+  }
+
+}

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) {