Page MenuHomePhabricator

Revision with an older "accept" showed up in "waiting on other reviewers" after "request review"
Closed, ResolvedPublic

Description

See D17585, might just be a transient state bug related to downgrading / voiding stuff. This is mostly so I don't forget about it.

See also PHI190.

Event Timeline

This is a legitimate bug but I think it's very long-standing and fixing it requires doing a lot of extra data loading to get the current diff PHID. Until someone else notices, I'd rather wait for diff PHIDs to be denormalized onto revisions.

Fix is something like this, but needs a needActiveDiffs(...):

diff --git a/src/applications/differential/query/DifferentialRevisionRequiredActionResultBucket.php b/src/applications/differential/query/DifferentialRevisionRequiredActionResultBucket.php
index 6d83d97a47..c70377874e 100644
--- a/src/applications/differential/query/DifferentialRevisionRequiredActionResultBucket.php
+++ b/src/applications/differential/query/DifferentialRevisionRequiredActionResultBucket.php
@@ -115,6 +115,11 @@ final class DifferentialRevisionRequiredActionResultBucket
     $reviewing = array(
       DifferentialReviewerStatus::STATUS_ADDED,
       DifferentialReviewerStatus::STATUS_COMMENTED,
+
+      // If an author has used "Request Review" to put an accepted revision
+      // back into the "needs review" state, include "Accepted" reviewers
+      // whose reviews have been voided.
+      DifferentialReviewerStatus::STATUS_ACCEPTED,
     );
     $reviewing = array_fuse($reviewing);
 
@@ -122,7 +127,7 @@ final class DifferentialRevisionRequiredActionResultBucket
 
     $results = array();
     foreach ($objects as $key => $object) {
-      if (!$this->hasReviewersWithStatus($object, $phids, $reviewing)) {
+      if (!$this->hasReviewersWithStatus($object, $phids, $reviewing, false)) {
         continue;
       }
 
diff --git a/src/applications/differential/query/DifferentialRevisionResultBucket.php b/src/applications/differential/query/DifferentialRevisionResultBucket.php
index 762e2d97f4..34a17aaaf6 100644
--- a/src/applications/differential/query/DifferentialRevisionResultBucket.php
+++ b/src/applications/differential/query/DifferentialRevisionResultBucket.php
@@ -54,7 +54,8 @@ abstract class DifferentialRevisionResultBucket
   protected function hasReviewersWithStatus(
     DifferentialRevision $revision,
     array $phids,
-    array $statuses) {
+    array $statuses,
+    $current = null) {
 
     foreach ($revision->getReviewers() as $reviewer) {
       $reviewer_phid = $reviewer->getReviewerPHID();
@@ -67,6 +68,17 @@ abstract class DifferentialRevisionResultBucket
         continue;
       }
 
+      if ($current !== null) {
+        if ($status == DifferentialReviewerStatus::STATUS_ACCEPTED) {
+          $diff_phid = $revision->getActiveDiff()->getPHID();
+          $is_current = $reviewer->isAccepted($diff_phid);
+        }
+
+        if ($is_current !== $current) {
+          continue;
+        }
+      }
+
       return true;
     }