From 12e6106a5907c5160dfca8b997e7b6796d707260 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 6 Nov 2017 11:51:59 -0800
Subject: [PATCH] Refine bucketing and display rules for voided "Accepts" in
 Differential

Summary:
See PHI190. This clarifies the ruleset a bit:

  - If you accepted, then the author used "Request Review" explicitly, we now show "Accepted Earlier" instead of "Accepted" in the "Reviewers" list on the main revision page. This makes it sligthly more clear why the revision is back in your review queue without picking through the transaction log.
  - Instead of moving all non-current accepts into "Ready to Review", move only voided accepts into "Ready to Review". This stops us from pulling older accepts which haven't been voided (which could have been incorrectly pulled) and correctly pulls older, voided accepts from before an update (for example: accept, then request review, then update) and generally aligns better with intent/expectation.

Test Plan:
  - Accepted, requested review.
  - Saw reviewer as "Accepted Earlier".
  - Saw review in "Ready to Review" bucket.
  - Accepted, updated (with sticky accept).
  - Saw reviewer as "Accepted Prior Diff".
  - Saw review as "Waiting on Authors".

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18764
---
 .../DifferentialRevisionRequiredActionResultBucket.php |  2 +-
 .../query/DifferentialRevisionResultBucket.php         |  9 ++++-----
 .../differential/view/DifferentialReviewersView.php    | 10 ++++++++--
 3 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/src/applications/differential/query/DifferentialRevisionRequiredActionResultBucket.php b/src/applications/differential/query/DifferentialRevisionRequiredActionResultBucket.php
index 5f5f4008db..711f70afb3 100644
--- a/src/applications/differential/query/DifferentialRevisionRequiredActionResultBucket.php
+++ b/src/applications/differential/query/DifferentialRevisionRequiredActionResultBucket.php
@@ -138,7 +138,7 @@ final class DifferentialRevisionRequiredActionResultBucket
 
     $results = array();
     foreach ($objects as $key => $object) {
-      if (!$this->hasReviewersWithStatus($object, $phids, $reviewing, false)) {
+      if (!$this->hasReviewersWithStatus($object, $phids, $reviewing, true)) {
         continue;
       }
 
diff --git a/src/applications/differential/query/DifferentialRevisionResultBucket.php b/src/applications/differential/query/DifferentialRevisionResultBucket.php
index a0769ac349..c5cc5c0e6c 100644
--- a/src/applications/differential/query/DifferentialRevisionResultBucket.php
+++ b/src/applications/differential/query/DifferentialRevisionResultBucket.php
@@ -54,7 +54,7 @@ abstract class DifferentialRevisionResultBucket
     DifferentialRevision $revision,
     array $phids,
     array $statuses,
-    $current = null) {
+    $include_voided = null) {
 
     foreach ($revision->getReviewers() as $reviewer) {
       $reviewer_phid = $reviewer->getReviewerPHID();
@@ -67,11 +67,10 @@ abstract class DifferentialRevisionResultBucket
         continue;
       }
 
-      if ($current !== null) {
+      if ($include_voided !== null) {
         if ($status == DifferentialReviewerStatus::STATUS_ACCEPTED) {
-          $diff_phid = $revision->getActiveDiffPHID();
-          $is_current = $reviewer->isAccepted($diff_phid);
-          if ($is_current !== $current) {
+          $is_voided = (bool)$reviewer->getVoidedPHID();
+          if ($is_voided !== $include_voided) {
             continue;
           }
         }
diff --git a/src/applications/differential/view/DifferentialReviewersView.php b/src/applications/differential/view/DifferentialReviewersView.php
index 33aad25289..f88669e539 100644
--- a/src/applications/differential/view/DifferentialReviewersView.php
+++ b/src/applications/differential/view/DifferentialReviewersView.php
@@ -47,6 +47,7 @@ final class DifferentialReviewersView extends AphrontView {
 
       $action_phid = $reviewer->getLastActionDiffPHID();
       $is_current_action = $this->isCurrent($action_phid);
+      $is_voided = (bool)$reviewer->getVoidedPHID();
 
       $comment_phid = $reviewer->getLastCommentDiffPHID();
       $is_current_comment = $this->isCurrent($comment_phid);
@@ -86,7 +87,7 @@ final class DifferentialReviewersView extends AphrontView {
           break;
 
         case DifferentialReviewerStatus::STATUS_ACCEPTED:
-          if ($is_current_action) {
+          if ($is_current_action && !$is_voided) {
             $icon = PHUIStatusItemView::ICON_ACCEPT;
             $color = 'green';
             if ($authority_name !== null) {
@@ -97,7 +98,12 @@ final class DifferentialReviewersView extends AphrontView {
           } else {
             $icon = 'fa-check-circle-o';
             $color = 'bluegrey';
-            if ($authority_name !== null) {
+
+            if (!$is_current_action && $is_voided) {
+              // The reviewer accepted the revision, but later the author
+              // used "Request Review" to request an updated review.
+              $label = pht('Accepted Earlier');
+            } else if ($authority_name !== null) {
               $label = pht('Accepted Prior Diff (by %s)', $authority_name);
             } else {
               $label = pht('Accepted Prior Diff');