Add an explcit "Changes Planned" state for Differential

Summary: Ref T2222. Ref T4481. This fixes the issue where "Plan Changes" could immediately trigger a state change (e.g., back to accepted) because of state-based transitions out of the NEEDS_REVISION state.

Test Plan: Planned changes an "accepted" revision, it didn't immediately return to being accepted.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222, T4481

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

src/applications/differential/constants/DifferentialRevisionStatus.php

@@ -21,12 +21,16 @@ final class DifferentialRevisionStatus {
         self::COLOR_STATUS_DEFAULT,
       ArcanistDifferentialRevisionStatus::NEEDS_REVISION =>
         self::COLOR_STATUS_RED,
+      ArcanistDifferentialRevisionStatus::CHANGES_PLANNED =>
+        self::COLOR_STATUS_RED,
       ArcanistDifferentialRevisionStatus::ACCEPTED       =>
         self::COLOR_STATUS_GREEN,
       ArcanistDifferentialRevisionStatus::CLOSED         =>
         self::COLOR_STATUS_DARK,
       ArcanistDifferentialRevisionStatus::ABANDONED      =>
         self::COLOR_STATUS_DARK,
+      ArcanistDifferentialRevisionStatus::IN_PREPARATION =>
+        self::COLOR_STATUS_DARK,
     );
     return idx($map, $status, $default);
   }
@@ -39,12 +43,16 @@ final class DifferentialRevisionStatus {
         'oh-open',
       ArcanistDifferentialRevisionStatus::NEEDS_REVISION =>
         'oh-open-red',
+      ArcanistDifferentialRevisionStatus::CHANGES_PLANNED =>
+        'oh-open-red',
       ArcanistDifferentialRevisionStatus::ACCEPTED       =>
         'oh-open-green',
       ArcanistDifferentialRevisionStatus::CLOSED         =>
         'oh-closed-dark',
       ArcanistDifferentialRevisionStatus::ABANDONED      =>
         'oh-closed-dark',
+      ArcanistDifferentialRevisionStatus::IN_PREPARATION =>
+        'question-blue',
     );
     return idx($map, $status, $default);
   }
@@ -92,9 +100,11 @@ final class DifferentialRevisionStatus {
     return array(
       ArcanistDifferentialRevisionStatus::NEEDS_REVIEW,
       ArcanistDifferentialRevisionStatus::NEEDS_REVISION,
+      ArcanistDifferentialRevisionStatus::CHANGES_PLANNED,
       ArcanistDifferentialRevisionStatus::ACCEPTED,
       ArcanistDifferentialRevisionStatus::CLOSED,
       ArcanistDifferentialRevisionStatus::ABANDONED,
+      ArcanistDifferentialRevisionStatus::IN_PREPARATION,
     );
   }
 

src/applications/differential/controller/DifferentialRevisionViewController.php

@@ -523,6 +523,7 @@ final class DifferentialRevisionViewController extends DifferentialController {
           $actions[DifferentialAction::ACTION_RETHINK] = true;
           break;
         case ArcanistDifferentialRevisionStatus::NEEDS_REVISION:
+        case ArcanistDifferentialRevisionStatus::CHANGES_PLANNED:
           $actions[DifferentialAction::ACTION_ACCEPT] = $allow_self_accept;
           $actions[DifferentialAction::ACTION_ABANDON] = true;
           $actions[DifferentialAction::ACTION_REQUEST] = true;
@@ -547,6 +548,7 @@ final class DifferentialRevisionViewController extends DifferentialController {
           $actions[DifferentialAction::ACTION_RESIGN] = $viewer_is_reviewer;
           break;
         case ArcanistDifferentialRevisionStatus::NEEDS_REVISION:
+        case ArcanistDifferentialRevisionStatus::CHANGES_PLANNED:
           $actions[DifferentialAction::ACTION_ACCEPT] = true;
           $actions[DifferentialAction::ACTION_REJECT] = !$viewer_has_rejected;
           $actions[DifferentialAction::ACTION_RESIGN] = $viewer_is_reviewer;

src/applications/differential/editor/DifferentialRevisionEditor.php

@@ -437,6 +437,7 @@ final class DifferentialRevisionEditor extends PhabricatorEditor {
     // be able to put the revision into "accepted".
     switch ($revision->getStatus()) {
       case ArcanistDifferentialRevisionStatus::NEEDS_REVISION:
+      case ArcanistDifferentialRevisionStatus::CHANGES_PLANNED:
       case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW:
         $revision = self::updateAcceptedStatus(
           $this->getActor(),

src/applications/differential/editor/DifferentialTransactionEditor.php

@@ -72,6 +72,7 @@ final class DifferentialTransactionEditor
         $status_abandoned = ArcanistDifferentialRevisionStatus::ABANDONED;
         $status_review = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW;
         $status_revision = ArcanistDifferentialRevisionStatus::NEEDS_REVISION;
+        $status_plan = ArcanistDifferentialRevisionStatus::CHANGES_PLANNED;
 
         $action_type = $xaction->getNewValue();
         switch ($action_type) {
@@ -86,7 +87,7 @@ final class DifferentialTransactionEditor
             $actor = $this->getActor();
             $actor_phid = $actor->getPHID();
 
-            // These transactions can cause effects in to ways: by altering the
+            // These transactions can cause effects in two ways: by altering the
             // status of an existing reviewer; or by adding the actor as a new
             // reviewer.
 
@@ -112,7 +113,7 @@ final class DifferentialTransactionEditor
           case DifferentialAction::ACTION_REOPEN:
             return ($object->getStatus() == $status_closed);
           case DifferentialAction::ACTION_RETHINK:
-            return ($object->getStatus() != $status_revision);
+            return ($object->getStatus() != $status_plan);
           case DifferentialAction::ACTION_REQUEST:
             return ($object->getStatus() != $status_review);
           case DifferentialAction::ACTION_RESIGN:
@@ -139,6 +140,7 @@ final class DifferentialTransactionEditor
 
     $status_review = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW;
     $status_revision = ArcanistDifferentialRevisionStatus::NEEDS_REVISION;
+    $status_plan = ArcanistDifferentialRevisionStatus::CHANGES_PLANNED;
 
     switch ($xaction->getTransactionType()) {
       case PhabricatorTransactions::TYPE_VIEW_POLICY:
@@ -169,7 +171,7 @@ final class DifferentialTransactionEditor
             $object->setStatus(ArcanistDifferentialRevisionStatus::ABANDONED);
             return;
           case DifferentialAction::ACTION_RETHINK:
-            $object->setStatus($status_revision);
+            $object->setStatus($status_plan);
             return;
           case DifferentialAction::ACTION_RECLAIM:
             $object->setStatus($status_review);
@@ -714,7 +716,7 @@ final class DifferentialTransactionEditor
         if (!$actor_is_author) {
           return pht(
             "You can not plan changes to this revision because you do not ".
-            "own it. To plan chagnes to a revision, you must be its owner.");
+            "own it. To plan changes to a revision, you must be its owner.");
         }
 
         switch ($revision_status) {
@@ -723,6 +725,9 @@ final class DifferentialTransactionEditor
           case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW:
             // These are OK.
             break;
+          case ArcanistDifferentialRevisionStatus::CHANGES_PLANNED:
+            // Let this through, it's a no-op.
+            break;
           case ArcanistDifferentialRevisionStatus::ABANDONED:
             return pht(
               "You can not plan changes to this revision because it has ".
@@ -752,6 +757,7 @@ final class DifferentialTransactionEditor
         switch ($revision_status) {
           case ArcanistDifferentialRevisionStatus::ACCEPTED:
           case ArcanistDifferentialRevisionStatus::NEEDS_REVISION:
+          case ArcanistDifferentialRevisionStatus::CHANGES_PLANNED:
             // These are OK.
             break;
           case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW:

src/applications/differential/view/DifferentialRevisionListView.php

@@ -182,6 +182,7 @@ final class DifferentialRevisionListView extends AphrontView {
         case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW:
           break;
         case ArcanistDifferentialRevisionStatus::NEEDS_REVISION:
+        case ArcanistDifferentialRevisionStatus::CHANGES_PLANNED:
           $item->setBarColor('red');
           break;
         case ArcanistDifferentialRevisionStatus::ACCEPTED: