Bug 1907593 - Consider changing default setting when a reviewer "requests changes" so revisions don't disappear from other reviewers' queues

Author: dklawren

moz-extensions/src/__phutil_library_map__.php

@@ -22,6 +22,7 @@
     'DifferentialUpliftRequestCustomField' => 'differential/customfield/DifferentialUpliftRequestCustomField.php',
     'DifferentialUpliftRequestCustomFieldTestCase' => 'differential/customfield/__tests__/DifferentialUpliftRequestCustomFieldTestCase.php',
     'PhabricatorUpdateUpliftCommentAction' => 'applications/transactions/commentaction/PhabricatorUpdateUpliftCommentAction.php',
+    'DifferentialRevisionRequiredActionWithNeedsChangesResultBucket' => 'differential/query/DifferentialRevisionRequiredActionWithNeedsChangesResultBucket.php',
     'DifferentialRevisionWarning' => 'differential/view/DifferentialRevisionWarning.php',
     'EmailAPIAuthorization' => 'email/EmailAPIAuthorization.php',
     'EmailAffectedFile' => 'email/model/EmailAffectedFile.php',
@@ -137,6 +138,7 @@
     'DifferentialBugzillaBugIDValidator' => 'Phobject',
     'DifferentialUpliftRequestCustomField' => 'DifferentialStoredCustomField',
     'PhabricatorUpdateUpliftCommentAction' => 'PhabricatorEditEngineCommentAction',
+    'DifferentialRevisionRequiredActionWithNeedsChangesResultBucket' => 'DifferentialRevisionRequiredActionResultBucket',
     'DifferentialRevisionWarning' => 'Phobject',
     'EmailRevisionAbandoned' => 'PublicEmailBody',
     'EmailRevisionAccepted' => 'PublicEmailBody',

moz-extensions/src/differential/query/DifferentialRevisionRequiredActionWithNeedsChangesResultBucket.php

@@ -0,0 +1,68 @@
+<?php
+
+final class DifferentialRevisionRequiredActionWithNeedsChangesResultBucket
+  extends DifferentialRevisionRequiredActionResultBucket {
+
+  const BUCKETKEY = 'action-with-needs-changes';
+
+  public function getResultBucketName() {
+    return pht('Bucket by Required Action (Include Needs Changes)');
+  }
+
+  protected function filterMustReview(array $phids) {
+    $needs_review_statuses = array_fuse(array(
+      DifferentialReviewerStatus::STATUS_BLOCKING,
+      DifferentialReviewerStatus::STATUS_REJECTED,
+      DifferentialReviewerStatus::STATUS_REJECTED_OLDER,
+    ));
+
+    return $this->filterWithNeedsRevision($phids, $needs_review_statuses);
+  }
+
+  protected function filterShouldReview(array $phids) {
+    $needs_review_statuses = array_fuse(array(
+      DifferentialReviewerStatus::STATUS_ADDED,
+      DifferentialReviewerStatus::STATUS_COMMENTED,
+      DifferentialReviewerStatus::STATUS_ACCEPTED,
+    ));
+
+    return $this->filterWithNeedsRevision($phids, $needs_review_statuses);
+  }
+
+  private function filterWithNeedsRevision(array $phids, array $needs_review_statuses) {
+    $inactive = array_fuse(array(
+      DifferentialReviewerStatus::STATUS_ADDED,
+      DifferentialReviewerStatus::STATUS_COMMENTED,
+    ));
+
+    // Standard NEEDS_REVIEW revisions (same as parent).
+    $objects = $this->getRevisionsUnderReview($this->objects, $phids);
+
+    // Also pick up NEEDS_REVISION revisions where the viewer hasn't acted yet.
+    foreach ($this->getRevisionsNotAuthored($this->objects, $phids) as $key => $object) {
+      if ($object->isNeedsRevision()) {
+        $objects[$key] = $object;
+      }
+    }
+
+    $results = array();
+    foreach ($objects as $key => $object) {
+      if ($object->isNeedsReview()) {
+        // Same as parent — check voided accepts too.
+        if (!$this->hasReviewersWithStatus($object, $phids, $needs_review_statuses, true)) {
+          continue;
+        }
+      } else {
+        // NEEDS_REVISION — only show if this reviewer hasn't acted yet.
+        if (!$this->hasReviewersWithStatus($object, $phids, $inactive)) {
+          continue;
+        }
+      }
+
+      $results[$key] = $object;
+      unset($this->objects[$key]);
+    }
+
+    return $results;
+  }
+}

src/applications/differential/query/DifferentialRevisionRequiredActionResultBucket.php

@@ -1,14 +1,14 @@
 <?php
 
-final class DifferentialRevisionRequiredActionResultBucket
+class DifferentialRevisionRequiredActionResultBucket
   extends DifferentialRevisionResultBucket {
 
   const BUCKETKEY = 'action';
 
   const KEY_MUSTREVIEW = 'must-review';
   const KEY_SHOULDREVIEW = 'should-review';
 
-  private $objects;
+  protected $objects;
 
   public function getResultBucketName() {
     return pht('Bucket by Required Action');
@@ -96,7 +96,7 @@ protected function buildResultGroups(
     return $groups;
   }
 
-  private function filterMustReview(array $phids) {
+  protected function filterMustReview(array $phids) {
     $blocking = array(
       DifferentialReviewerStatus::STATUS_BLOCKING,
       DifferentialReviewerStatus::STATUS_REJECTED,
@@ -119,7 +119,7 @@ private function filterMustReview(array $phids) {
     return $results;
   }
 
-  private function filterShouldReview(array $phids) {
+  protected function filterShouldReview(array $phids) {
     $reviewing = array(
       DifferentialReviewerStatus::STATUS_ADDED,
       DifferentialReviewerStatus::STATUS_COMMENTED,

src/applications/differential/query/DifferentialRevisionSearchEngine.php

@@ -140,6 +140,7 @@ protected function getBuiltinQueryNames() {
 
     if ($this->requireViewer()->isLoggedIn()) {
       $names['active'] = pht('Active Revisions');
+      $names['action-with-needs-changes'] = pht('Active With Needs Changes');
       $names['authored'] = pht('Authored');
     }
 
@@ -158,6 +159,13 @@ public function buildSavedQueryFromBuiltin($query_key) {
       case 'active':
         $bucket_key = DifferentialRevisionRequiredActionResultBucket::BUCKETKEY;
 
+        return $query
+          ->setParameter('responsiblePHIDs', array($viewer->getPHID()))
+          ->setParameter('statuses', array('open()'))
+          ->setParameter('bucket', $bucket_key);
+      case 'action-with-needs-changes':
+        $bucket_key = DifferentialRevisionRequiredActionWithNeedsChangesResultBucket::BUCKETKEY;
+
         return $query
           ->setParameter('responsiblePHIDs', array($viewer->getPHID()))
           ->setParameter('statuses', array('open()'))