Modularize the Differential "status" transaction and move away from ArcanistDifferentialRevisionStatus

Summary:
Ref T2543. Converts the TYPE_STATUS transaction (used to render "This revision now requires changes to proceed.", "This revision is accepted and ready to land.", etc) to ModularTransactions.

Also, continue consolidating all the status-related information (here, more colors and icons) into a single place. By the end of this, we may learn that NEEDS_REVIEW uses //every// color.

Test Plan:
Reviewed old status transactions (unchanged) and created new ones (looked the same as the old ones).

(I plan to migrate all of these a few diffs from now, around when I change the storage format.)

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T2543

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

src/__phutil_library_map__.php

@@ -575,6 +575,7 @@ phutil_register_library_map(array(
     'DifferentialRevisionStatus' => 'applications/differential/constants/DifferentialRevisionStatus.php',
     'DifferentialRevisionStatusDatasource' => 'applications/differential/typeahead/DifferentialRevisionStatusDatasource.php',
     'DifferentialRevisionStatusFunctionDatasource' => 'applications/differential/typeahead/DifferentialRevisionStatusFunctionDatasource.php',
+    'DifferentialRevisionStatusTransaction' => 'applications/differential/xaction/DifferentialRevisionStatusTransaction.php',
     'DifferentialRevisionSummaryHeraldField' => 'applications/differential/herald/DifferentialRevisionSummaryHeraldField.php',
     'DifferentialRevisionSummaryTransaction' => 'applications/differential/xaction/DifferentialRevisionSummaryTransaction.php',
     'DifferentialRevisionTestPlanTransaction' => 'applications/differential/xaction/DifferentialRevisionTestPlanTransaction.php',
@@ -5571,6 +5572,7 @@ phutil_register_library_map(array(
     'DifferentialRevisionStatus' => 'Phobject',
     'DifferentialRevisionStatusDatasource' => 'PhabricatorTypeaheadDatasource',
     'DifferentialRevisionStatusFunctionDatasource' => 'PhabricatorTypeaheadCompositeDatasource',
+    'DifferentialRevisionStatusTransaction' => 'DifferentialRevisionTransactionType',
     'DifferentialRevisionSummaryHeraldField' => 'DifferentialRevisionHeraldField',
     'DifferentialRevisionSummaryTransaction' => 'DifferentialRevisionTransactionType',
     'DifferentialRevisionTestPlanTransaction' => 'DifferentialRevisionTransactionType',

src/applications/differential/constants/DifferentialRevisionStatus.php

@@ -32,6 +32,14 @@ final class DifferentialRevisionStatus extends Phobject {
     return idx($this->spec, 'color.tag', 'bluegrey');
   }
 
+  public function getTimelineIcon() {
+    return idx($this->spec, 'icon.timeline');
+  }
+
+  public function getTimelineColor() {
+    return idx($this->spec, 'color.timeline');
+  }
+
   public function getANSIColor() {
     return idx($this->spec, 'color.ansi');
   }
@@ -56,6 +64,10 @@ final class DifferentialRevisionStatus extends Phobject {
     return ($this->key === self::NEEDS_REVIEW);
   }
 
+  public function isNeedsRevision() {
+    return ($this->key === self::NEEDS_REVISION);
+  }
+
   public function isPublished() {
     return ($this->key === self::PUBLISHED);
   }
@@ -116,19 +128,23 @@ final class DifferentialRevisionStatus extends Phobject {
         'name' => pht('Needs Review'),
         'legacy' => 0,
         'icon' => 'fa-code',
+        'icon.timeline' => 'fa-undo',
         'closed' => false,
         'color.icon' => 'grey',
         'color.tag' => 'bluegrey',
         'color.ansi' => 'magenta',
+        'color.timeline' => 'orange',
       ),
       self::NEEDS_REVISION => array(
         'name' => pht('Needs Revision'),
         'legacy' => 1,
         'icon' => 'fa-refresh',
+        'icon.timeline' => 'fa-times',
         'closed' => false,
         'color.icon' => 'red',
         'color.tag' => 'red',
         'color.ansi' => 'red',
+        'color.timeline' => 'red',
       ),
       self::CHANGES_PLANNED => array(
         'name' => pht('Changes Planned'),
@@ -143,10 +159,12 @@ final class DifferentialRevisionStatus extends Phobject {
         'name' => pht('Accepted'),
         'legacy' => 2,
         'icon' => 'fa-check',
+        'icon.timeline' => 'fa-check',
         'closed' => $close_on_accept,
         'color.icon' => 'green',
         'color.tag' => 'green',
         'color.ansi' => 'green',
+        'color.timeline' => 'green',
       ),
       self::PUBLISHED => array(
         'name' => pht('Closed'),

src/applications/differential/editor/DifferentialTransactionEditor.php

@@ -71,7 +71,6 @@ final class DifferentialTransactionEditor
 
     $types[] = DifferentialTransaction::TYPE_ACTION;
     $types[] = DifferentialTransaction::TYPE_INLINE;
-    $types[] = DifferentialTransaction::TYPE_STATUS;
     $types[] = DifferentialTransaction::TYPE_UPDATE;
 
     return $types;
@@ -606,7 +605,8 @@ final class DifferentialTransactionEditor
 
         if ($new_status !== null && ($new_status != $old_status)) {
           $xaction = id(new DifferentialTransaction())
-            ->setTransactionType(DifferentialTransaction::TYPE_STATUS)
+            ->setTransactionType(
+                DifferentialRevisionStatusTransaction::TRANSACTIONTYPE)
             ->setOldValue($old_status)
             ->setNewValue($new_status);
 

src/applications/differential/storage/DifferentialTransaction.php

@@ -8,7 +8,6 @@ final class DifferentialTransaction
   const TYPE_INLINE  = 'differential:inline';
   const TYPE_UPDATE  = 'differential:update';
   const TYPE_ACTION  = 'differential:action';
-  const TYPE_STATUS  = 'differential:status';
 
   const MAILTAG_REVIEWERS      = 'differential-reviewers';
   const MAILTAG_CLOSED         = 'differential-committed';
@@ -42,6 +41,10 @@ final class DifferentialTransaction
       }
     }
 
+    if ($xaction_type == 'differential:status') {
+      return new DifferentialRevisionStatusTransaction();
+    }
+
     return parent::newFallbackModularTransactionType();
   }
 
@@ -305,15 +308,6 @@ final class DifferentialTransaction
             return DifferentialAction::getBasicStoryText($new, $author_handle);
         }
         break;
-      case self::TYPE_STATUS:
-        switch ($this->getNewValue()) {
-          case ArcanistDifferentialRevisionStatus::ACCEPTED:
-            return pht('This revision is now accepted and ready to land.');
-          case ArcanistDifferentialRevisionStatus::NEEDS_REVISION:
-            return pht('This revision now requires changes to proceed.');
-          case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW:
-            return pht('This revision now requires review to proceed.');
-        }
      }
 
     return parent::getTitle();
@@ -457,21 +451,6 @@ final class DifferentialTransaction
               $object_link);
         }
         break;
-      case self::TYPE_STATUS:
-        switch ($this->getNewValue()) {
-          case ArcanistDifferentialRevisionStatus::ACCEPTED:
-            return pht(
-              '%s is now accepted and ready to land.',
-              $object_link);
-          case ArcanistDifferentialRevisionStatus::NEEDS_REVISION:
-            return pht(
-              '%s now requires changes to proceed.',
-              $object_link);
-          case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW:
-            return pht(
-              '%s now requires review to proceed.',
-              $object_link);
-        }
      }
 
     return parent::getTitleForFeed();
@@ -483,16 +462,6 @@ final class DifferentialTransaction
         return 'fa-comment';
       case self::TYPE_UPDATE:
         return 'fa-refresh';
-      case self::TYPE_STATUS:
-        switch ($this->getNewValue()) {
-          case ArcanistDifferentialRevisionStatus::ACCEPTED:
-            return 'fa-check';
-          case ArcanistDifferentialRevisionStatus::NEEDS_REVISION:
-            return 'fa-times';
-          case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW:
-            return 'fa-undo';
-        }
-        break;
       case self::TYPE_ACTION:
         switch ($this->getNewValue()) {
           case DifferentialAction::ACTION_CLOSE:
@@ -530,14 +499,12 @@ final class DifferentialTransaction
     // Never group status changes with other types of actions, they're indirect
     // and don't make sense when combined with direct actions.
 
-    $type_status = self::TYPE_STATUS;
-
-    if ($this->getTransactionType() == $type_status) {
+    if ($this->isStatusTransaction($this)) {
       return false;
     }
 
     foreach ($group as $xaction) {
-      if ($xaction->getTransactionType() == $type_status) {
+      if ($this->isStatusTransaction($xaction)) {
         return false;
       }
     }
@@ -545,21 +512,25 @@ final class DifferentialTransaction
     return parent::shouldDisplayGroupWith($group);
   }
 
+  private function isStatusTransaction($xaction) {
+    $old_status = 'differential:status';
+    if ($xaction->getTransactionType() == $old_status) {
+      return true;
+    }
+
+    $new_status = DifferentialRevisionStatusTransaction::TRANSACTIONTYPE;
+    if ($xaction->getTransactionType() == $new_status) {
+      return true;
+    }
+
+    return false;
+  }
+
 
   public function getColor() {
     switch ($this->getTransactionType()) {
       case self::TYPE_UPDATE:
         return PhabricatorTransactions::COLOR_SKY;
-      case self::TYPE_STATUS:
-        switch ($this->getNewValue()) {
-          case ArcanistDifferentialRevisionStatus::ACCEPTED:
-            return PhabricatorTransactions::COLOR_GREEN;
-          case ArcanistDifferentialRevisionStatus::NEEDS_REVISION:
-            return PhabricatorTransactions::COLOR_RED;
-          case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW:
-            return PhabricatorTransactions::COLOR_ORANGE;
-        }
-        break;
       case self::TYPE_ACTION:
         switch ($this->getNewValue()) {
           case DifferentialAction::ACTION_CLOSE:

src/applications/differential/xaction/DifferentialRevisionStatusTransaction.php

@@ -0,0 +1,74 @@
+<?php
+
+final class DifferentialRevisionStatusTransaction
+  extends DifferentialRevisionTransactionType {
+
+  const TRANSACTIONTYPE = 'differential.revision.status';
+
+  public function generateOldValue($object) {
+    return $object->getStatus();
+  }
+
+  public function applyInternalEffects($object, $value) {
+    $object->setStatus($value);
+  }
+
+  public function getTitle() {
+    $new = $this->getNewValue();
+    $status = DifferentialRevisionStatus::newForLegacyStatus($new);
+
+    if ($status->isAccepted()) {
+      return pht('This revision is now accepted and ready to land.');
+    }
+
+    if ($status->isNeedsRevision()) {
+      return pht('This revision now requires changes to proceed.');
+    }
+
+    if ($status->isNeedsReview()) {
+      return pht('This revision now requires review to proceed.');
+    }
+
+    return null;
+  }
+
+  public function getTitleForFeed() {
+    $status = $this->newStatusObject();
+
+    if ($status->isAccepted()) {
+      return pht(
+        '%s is now accepted and ready to land.',
+        $this->renderObject());
+    }
+
+    if ($status->isNeedsRevision()) {
+      return pht(
+        '%s now requires changes to proceed.',
+        $this->renderObject());
+    }
+
+    if ($status->isNeedsReview()) {
+      return pht(
+        '%s now requires review to proceed.',
+        $this->renderObject());
+    }
+
+    return null;
+  }
+
+  public function getIcon() {
+    $status = $this->newStatusObject();
+    return $status->getTimelineIcon();
+  }
+
+  public function getColor() {
+    $status = $this->newStatusObject();
+    return $status->getTimelineColor();
+  }
+
+  private function newStatusObject() {
+    $new = $this->getNewValue();
+    return DifferentialRevisionStatus::newForLegacyStatus($new);
+  }
+
+}