Treat "request review" more like an update

Summary:
After "reject; plan changes; request review", revisions go back to "needs revision". Instead, they should remain in "needs review" (the reviewers need to review comments on the "request review", in the normal case). Generally, "request reivew" should act a lot like "update", just not actually change the diff.

To accomplish this, downgrade reviewers on "request review" to "rejected older", just like we would on an update.

Test Plan: Did "reject; plan; request", revision ended in "needs review". Rejected it into "needs revision"; updated it into "needs review".

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: dctrwatson, epriestley

Differential Revision: https://secure.phabricator.com/D8558

src/applications/differential/editor/DifferentialTransactionEditor.php

@@ -225,6 +225,8 @@ final class DifferentialTransactionEditor
     PhabricatorLiskDAO $object,
     PhabricatorApplicationTransaction $xaction) {
 
+    $results = parent::expandTransaction($object, $xaction);
+
     $actor = $this->getActor();
     $actor_phid = $actor->getPHID();
     $type_edge = PhabricatorTransactions::TYPE_EDGE;
@@ -232,13 +234,23 @@ final class DifferentialTransactionEditor
     $edge_reviewer = PhabricatorEdgeConfig::TYPE_DREV_HAS_REVIEWER;
     $edge_ref_task = PhabricatorEdgeConfig::TYPE_DREV_HAS_RELATED_TASK;
 
-    $results = parent::expandTransaction($object, $xaction);
+    $downgrade_rejects = false;
+    if ($this->getIsCloseByCommit()) {
+      // Never downgrade reviewers when we're closing a revision after a
+      // commit.
+    } else {
       switch ($xaction->getTransactionType()) {
         case DifferentialTransaction::TYPE_UPDATE:
-        if ($this->getIsCloseByCommit()) {
+          $downgrade_rejects = true;
-          // Don't bother with any of this if this update is a side effect of
-          // commit detection.
           break;
+        case DifferentialTransaction::TYPE_ACTION:
+          switch ($xaction->getNewValue()) {
+            case DifferentialAction::ACTION_REQUEST:
+              $downgrade_rejects = true;
+              break;
+          }
+          break;
+      }
     }
 
     $new_accept = DifferentialReviewerStatus::STATUS_ACCEPTED;
@@ -246,10 +258,16 @@ final class DifferentialTransactionEditor
     $old_accept = DifferentialReviewerStatus::STATUS_ACCEPTED_OLDER;
     $old_reject = DifferentialReviewerStatus::STATUS_REJECTED_OLDER;
 
+    if ($downgrade_rejects) {
       // When a revision is updated, change all "reject" to "rejected older
       // revision". This means we won't immediately push the update back into
       // "needs review", but outstanding rejects will still block it from
       // moving to "accepted".
+
+      // We also do this for "Request Review", even though the diff is not
+      // updated directly. Essentially, this acts like an update which doesn't
+      // actually change the diff text.
+
       $edits = array();
       foreach ($object->getReviewerStatus() as $reviewer) {
         if ($reviewer->getStatus() == $new_reject) {
@@ -270,6 +288,15 @@ final class DifferentialTransactionEditor
           ->setIgnoreOnNoEffect(true)
           ->setNewValue(array('+' => $edits));
       }
+    }
+
+    switch ($xaction->getTransactionType()) {
+      case DifferentialTransaction::TYPE_UPDATE:
+        if ($this->getIsCloseByCommit()) {
+          // Don't bother with any of this if this update is a side effect of
+          // commit detection.
+          break;
+        }
 
         // When a revision is updated and the diff comes from a branch named
         // "T123" or similar, automatically associate the commit with the
@@ -529,7 +556,6 @@ final class DifferentialTransactionEditor
 
     $object->attachReviewerStatus($new_revision->getReviewerStatus());
 
-
     foreach ($xactions as $xaction) {
       switch ($xaction->getTransactionType()) {
         case DifferentialTransaction::TYPE_UPDATE: