Modularize Herald task status/priority fields

Summary: Ref T8726. These are a bit involved because they have custom rendering and editor values.

Test Plan: Created new rule using these fields, edited tasks to trigger them, viewed transcripts.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: eadler, joshuaspence, epriestley

Maniphest Tasks: T8726

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

src/__phutil_library_map__.php

@@ -950,7 +950,7 @@ phutil_register_library_map(array(
     'HeraldInvalidActionException' => 'applications/herald/engine/exception/HeraldInvalidActionException.php',
     'HeraldInvalidConditionException' => 'applications/herald/engine/exception/HeraldInvalidConditionException.php',
     'HeraldManageGlobalRulesCapability' => 'applications/herald/capability/HeraldManageGlobalRulesCapability.php',
-    'HeraldManiphestTaskAdapter' => 'applications/herald/adapter/HeraldManiphestTaskAdapter.php',
+    'HeraldManiphestTaskAdapter' => 'applications/maniphest/herald/HeraldManiphestTaskAdapter.php',
     'HeraldNewController' => 'applications/herald/controller/HeraldNewController.php',
     'HeraldNewObjectField' => 'applications/herald/field/HeraldNewObjectField.php',
     'HeraldObjectTranscript' => 'applications/herald/storage/transcript/HeraldObjectTranscript.php',
@@ -1072,6 +1072,7 @@ phutil_register_library_map(array(
     'ManiphestExcelFormatTestCase' => 'applications/maniphest/export/__tests__/ManiphestExcelFormatTestCase.php',
     'ManiphestExportController' => 'applications/maniphest/controller/ManiphestExportController.php',
     'ManiphestGetTaskTransactionsConduitAPIMethod' => 'applications/maniphest/conduit/ManiphestGetTaskTransactionsConduitAPIMethod.php',
+    'ManiphestHeraldField' => 'applications/maniphest/herald/ManiphestHeraldField.php',
     'ManiphestHovercardEventListener' => 'applications/maniphest/event/ManiphestHovercardEventListener.php',
     'ManiphestInfoConduitAPIMethod' => 'applications/maniphest/conduit/ManiphestInfoConduitAPIMethod.php',
     'ManiphestNameIndex' => 'applications/maniphest/storage/ManiphestNameIndex.php',
@@ -1105,12 +1106,14 @@ phutil_register_library_map(array(
     'ManiphestTaskPHIDType' => 'applications/maniphest/phid/ManiphestTaskPHIDType.php',
     'ManiphestTaskPriority' => 'applications/maniphest/constants/ManiphestTaskPriority.php',
     'ManiphestTaskPriorityDatasource' => 'applications/maniphest/typeahead/ManiphestTaskPriorityDatasource.php',
+    'ManiphestTaskPriorityHeraldField' => 'applications/maniphest/herald/ManiphestTaskPriorityHeraldField.php',
     'ManiphestTaskQuery' => 'applications/maniphest/query/ManiphestTaskQuery.php',
     'ManiphestTaskResultListView' => 'applications/maniphest/view/ManiphestTaskResultListView.php',
     'ManiphestTaskSearchEngine' => 'applications/maniphest/query/ManiphestTaskSearchEngine.php',
     'ManiphestTaskStatus' => 'applications/maniphest/constants/ManiphestTaskStatus.php',
     'ManiphestTaskStatusDatasource' => 'applications/maniphest/typeahead/ManiphestTaskStatusDatasource.php',
     'ManiphestTaskStatusFunctionDatasource' => 'applications/maniphest/typeahead/ManiphestTaskStatusFunctionDatasource.php',
+    'ManiphestTaskStatusHeraldField' => 'applications/maniphest/herald/ManiphestTaskStatusHeraldField.php',
     'ManiphestTaskStatusTestCase' => 'applications/maniphest/constants/__tests__/ManiphestTaskStatusTestCase.php',
     'ManiphestTaskTestCase' => 'applications/maniphest/__tests__/ManiphestTaskTestCase.php',
     'ManiphestTransaction' => 'applications/maniphest/storage/ManiphestTransaction.php',
@@ -4602,6 +4605,7 @@ phutil_register_library_map(array(
     'ManiphestExcelFormatTestCase' => 'PhabricatorTestCase',
     'ManiphestExportController' => 'ManiphestController',
     'ManiphestGetTaskTransactionsConduitAPIMethod' => 'ManiphestConduitAPIMethod',
+    'ManiphestHeraldField' => 'HeraldField',
     'ManiphestHovercardEventListener' => 'PhabricatorEventListener',
     'ManiphestInfoConduitAPIMethod' => 'ManiphestConduitAPIMethod',
     'ManiphestNameIndex' => 'ManiphestDAO',
@@ -4649,12 +4653,14 @@ phutil_register_library_map(array(
     'ManiphestTaskPHIDType' => 'PhabricatorPHIDType',
     'ManiphestTaskPriority' => 'ManiphestConstants',
     'ManiphestTaskPriorityDatasource' => 'PhabricatorTypeaheadDatasource',
+    'ManiphestTaskPriorityHeraldField' => 'ManiphestHeraldField',
     'ManiphestTaskQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'ManiphestTaskResultListView' => 'ManiphestView',
     'ManiphestTaskSearchEngine' => 'PhabricatorApplicationSearchEngine',
     'ManiphestTaskStatus' => 'ManiphestConstants',
     'ManiphestTaskStatusDatasource' => 'PhabricatorTypeaheadDatasource',
     'ManiphestTaskStatusFunctionDatasource' => 'PhabricatorTypeaheadCompositeDatasource',
+    'ManiphestTaskStatusHeraldField' => 'ManiphestHeraldField',
     'ManiphestTaskStatusTestCase' => 'PhabricatorTestCase',
     'ManiphestTaskTestCase' => 'PhabricatorTestCase',
     'ManiphestTransaction' => 'PhabricatorApplicationTransaction',

src/applications/herald/adapter/HeraldAdapter.php

@@ -29,8 +29,6 @@ abstract class HeraldAdapter extends Phobject {
   const FIELD_BRANCHES               = 'branches';
   const FIELD_AUTHOR_RAW             = 'author-raw';
   const FIELD_COMMITTER_RAW          = 'committer-raw';
-  const FIELD_TASK_PRIORITY          = 'taskpriority';
-  const FIELD_TASK_STATUS            = 'taskstatus';
   const FIELD_PUSHER_IS_COMMITTER    = 'pusher-is-committer';
   const FIELD_PATH                   = 'path';
 
@@ -383,8 +381,6 @@ abstract class HeraldAdapter extends Phobject {
       self::FIELD_BRANCHES => pht('Commit\'s branches'),
       self::FIELD_AUTHOR_RAW => pht('Raw author name'),
       self::FIELD_COMMITTER_RAW => pht('Raw committer name'),
-      self::FIELD_TASK_PRIORITY => pht('Task priority'),
-      self::FIELD_TASK_STATUS => pht('Task status'),
       self::FIELD_PUSHER_IS_COMMITTER => pht('Pusher same as committer'),
       self::FIELD_PATH => pht('Path'),
     );
@@ -443,8 +439,6 @@ abstract class HeraldAdapter extends Phobject {
         );
       case self::FIELD_REVIEWER:
       case self::FIELD_PUSHER:
-      case self::FIELD_TASK_PRIORITY:
-      case self::FIELD_TASK_STATUS:
         return array(
           self::CONDITION_IS_ANY,
           self::CONDITION_IS_NOT_ANY,
@@ -906,10 +900,6 @@ abstract class HeraldAdapter extends Phobject {
         switch ($field) {
           case self::FIELD_REPOSITORY:
             return self::VALUE_REPOSITORY;
-          case self::FIELD_TASK_PRIORITY:
-            return self::VALUE_TASK_PRIORITY;
-          case self::FIELD_TASK_STATUS:
-            return self::VALUE_TASK_STATUS;
           default:
             return self::VALUE_USER;
         }
@@ -1070,9 +1060,34 @@ abstract class HeraldAdapter extends Phobject {
     return $map;
   }
 
+  public function getEditorValueForCondition(
+    PhabricatorUser $viewer,
+    HeraldCondition $condition,
+    array $handles) {
+
+    $impl = $this->getFieldImplementation($condition->getFieldName());
+    if ($impl) {
+      return $impl->getEditorValue(
+        $viewer,
+        $condition->getValue());
+    }
+
+    $value = $condition->getValue();
+    if (is_array($value)) {
+      $value_map = array();
+      foreach ($value as $k => $phid) {
+        $value_map[$phid] = $handles[$phid]->getName();
+      }
+      $value = $value_map;
+    }
+
+    return $value;
+  }
+
   public function renderRuleAsText(
     HeraldRule $rule,
-    PhabricatorHandleList $handles) {
+    PhabricatorHandleList $handles,
+    PhabricatorUser $viewer) {
 
     require_celerity_resource('herald-css');
 
@@ -1102,7 +1117,7 @@ abstract class HeraldAdapter extends Phobject {
         ),
         array(
           $icon,
-          $this->renderConditionAsText($condition, $handles),
+          $this->renderConditionAsText($condition, $handles, $viewer),
         ));
     }
 
@@ -1147,7 +1162,8 @@ abstract class HeraldAdapter extends Phobject {
 
   private function renderConditionAsText(
     HeraldCondition $condition,
-    PhabricatorHandleList $handles) {
+    PhabricatorHandleList $handles,
+    PhabricatorUser $viewer) {
 
     $field_type = $condition->getFieldName();
 
@@ -1158,7 +1174,7 @@ abstract class HeraldAdapter extends Phobject {
     $condition_type = $condition->getFieldCondition();
     $condition_name = idx($this->getConditionNameMap(), $condition_type);
 
-    $value = $this->renderConditionValueAsText($condition, $handles);
+    $value = $this->renderConditionValueAsText($condition, $handles, $viewer);
 
     return hsprintf('    %s %s %s', $field_name, $condition_name, $value);
   }
@@ -1184,31 +1200,22 @@ abstract class HeraldAdapter extends Phobject {
 
   private function renderConditionValueAsText(
     HeraldCondition $condition,
-    PhabricatorHandleList $handles) {
+    PhabricatorHandleList $handles,
+    PhabricatorUser $viewer) {
+
+    $impl = $this->getFieldImplementation($condition->getFieldName());
+    if ($impl) {
+      return $impl->renderConditionValue(
+        $viewer,
+        $condition->getValue());
+    }
 
     $value = $condition->getValue();
     if (!is_array($value)) {
       $value = array($value);
     }
+
     switch ($condition->getFieldName()) {
-      case self::FIELD_TASK_PRIORITY:
-        $priority_map = ManiphestTaskPriority::getTaskPriorityMap();
-        foreach ($value as $index => $val) {
-          $name = idx($priority_map, $val);
-          if ($name) {
-            $value[$index] = $name;
-          }
-        }
-        break;
-      case self::FIELD_TASK_STATUS:
-        $status_map = ManiphestTaskStatus::getTaskStatusMap();
-        foreach ($value as $index => $val) {
-          $name = idx($status_map, $val);
-          if ($name) {
-            $value[$index] = $name;
-          }
-        }
-        break;
       case HeraldPreCommitRefAdapter::FIELD_REF_CHANGE:
         $change_map =
           PhabricatorRepositoryPushLog::getHeraldChangeFlagConditionOptions();

src/applications/herald/controller/HeraldRuleController.php

@@ -354,34 +354,11 @@ final class HeraldRuleController extends HeraldController {
     if ($rule->getConditions()) {
       $serial_conditions = array();
       foreach ($rule->getConditions() as $condition) {
-        $value = $condition->getValue();
-        switch ($condition->getFieldName()) {
-          case HeraldAdapter::FIELD_TASK_PRIORITY:
-            $value_map = array();
-            $priority_map = ManiphestTaskPriority::getTaskPriorityMap();
-            foreach ($value as $priority) {
-              $value_map[$priority] = idx($priority_map, $priority);
-            }
-            $value = $value_map;
-            break;
-          case HeraldAdapter::FIELD_TASK_STATUS:
-            $value_map = array();
-            $status_map = ManiphestTaskStatus::getTaskStatusMap();
-            foreach ($value as $status) {
-              $value_map[$status] = idx($status_map, $status);
-            }
-            $value = $value_map;
-            break;
-          default:
-            if (is_array($value)) {
-              $value_map = array();
-              foreach ($value as $k => $fbid) {
-                $value_map[$fbid] = $handles[$fbid]->getName();
-              }
-              $value = $value_map;
-            }
-            break;
-        }
+        $value = $adapter->getEditorValueForCondition(
+          $this->getViewer(),
+          $condition,
+          $handles);
+
         $serial_conditions[] = array(
           $condition->getFieldName(),
           $condition->getFieldCondition(),

src/applications/herald/controller/HeraldRuleViewController.php

@@ -152,7 +152,8 @@ final class HeraldRuleViewController extends HeraldController {
         PHUIPropertyListView::ICON_SUMMARY);
 
       $handles = $viewer->loadHandles(HeraldAdapter::getHandlePHIDs($rule));
-      $view->addTextContent($adapter->renderRuleAsText($rule, $handles));
+      $rule_text = $adapter->renderRuleAsText($rule, $handles, $viewer);
+      $view->addTextContent($rule_text);
     }
 
     return $view;

src/applications/herald/field/HeraldField.php

@@ -49,6 +49,51 @@ abstract class HeraldField extends Phobject {
     return array($this->getFieldConstant() => $this);
   }
 
+  public function renderConditionValue(
+    PhabricatorUser $viewer,
+    $value) {
+
+    // TODO: While this is less of a mess than it used to be, it would still
+    // be nice to push this down into individual fields better eventually and
+    // stop guessing which values are PHIDs and which aren't.
+
+    if (!is_array($value)) {
+      return $value;
+    }
+
+    $type_unknown = PhabricatorPHIDConstants::PHID_TYPE_UNKNOWN;
+
+    foreach ($value as $key => $val) {
+      if (is_string($val)) {
+        if (phid_get_type($val) !== $type_unknown) {
+          $value[$key] = $viewer->renderHandle($val);
+        }
+      }
+    }
+
+    return phutil_implode_html(', ', $value);
+  }
+
+  public function getEditorValue(
+    PhabricatorUser $viewer,
+    $value) {
+
+    // TODO: This should be better structured and pushed down into individual
+    // fields. As it is used to manually build tokenizer tokens, it can
+    // probably be removed entirely.
+
+    if (is_array($value)) {
+      $handles = $viewer->loadHandles($value);
+      $value_map = array();
+      foreach ($value as $k => $phid) {
+        $value_map[$phid] = $handles[$phid]->getName();
+      }
+      $value = $value_map;
+    }
+
+    return $value;
+  }
+
   final public function setAdapter(HeraldAdapter $adapter) {
     $this->adapter = $adapter;
     return $this;

src/applications/maniphest/herald/HeraldManiphestTaskAdapter.php

@@ -74,8 +74,6 @@ final class HeraldManiphestTaskAdapter extends HeraldAdapter {
         self::FIELD_BODY,
         self::FIELD_AUTHOR,
         self::FIELD_ASSIGNEE,
-        self::FIELD_TASK_PRIORITY,
-        self::FIELD_TASK_STATUS,
       ),
       parent::getFields());
   }
@@ -124,10 +122,6 @@ final class HeraldManiphestTaskAdapter extends HeraldAdapter {
         return $this->getTask()->getAuthorPHID();
       case self::FIELD_ASSIGNEE:
         return $this->getTask()->getOwnerPHID();
-      case self::FIELD_TASK_PRIORITY:
-        return $this->getTask()->getPriority();
-      case self::FIELD_TASK_STATUS:
-        return $this->getTask()->getStatus();
     }
 
     return parent::getHeraldField($field);

src/applications/maniphest/herald/ManiphestHeraldField.php

@@ -0,0 +1,9 @@
+<?php
+
+abstract class ManiphestHeraldField extends HeraldField {
+
+  public function supportsObject($object) {
+    return ($object instanceof ManiphestTask);
+  }
+
+}

src/applications/maniphest/herald/ManiphestTaskPriorityHeraldField.php

@@ -0,0 +1,55 @@
+<?php
+
+final class ManiphestTaskPriorityHeraldField
+  extends ManiphestHeraldField {
+
+  const FIELDCONST = 'taskpriority';
+
+  public function getHeraldFieldName() {
+    return pht('Task priority');
+  }
+
+  public function getHeraldFieldValue($object) {
+    return $object->getPriority();
+  }
+
+  protected function getHeraldFieldStandardConditions() {
+    return self::STANDARD_PHID;
+  }
+
+  public function getHeraldFieldValueType($condition) {
+    return HeraldAdapter::VALUE_TASK_PRIORITY;
+  }
+
+  public function renderConditionValue(
+    PhabricatorUser $viewer,
+    $value) {
+
+    $priority_map = ManiphestTaskPriority::getTaskPriorityMap();
+
+    $value = (array)$value;
+    foreach ($value as $index => $val) {
+      $name = idx($priority_map, $val);
+      if ($name !== null) {
+        $value[$index] = $name;
+      }
+    }
+
+    return implode(', ', $value);
+  }
+
+  public function getEditorValue(
+    PhabricatorUser $viewer,
+    $value) {
+
+    $priority_map = ManiphestTaskPriority::getTaskPriorityMap();
+
+    $value_map = array();
+    foreach ($value as $priority) {
+      $value_map[$priority] = idx($priority_map, $priority, $priority);
+    }
+
+    return $value_map;
+  }
+
+}

src/applications/maniphest/herald/ManiphestTaskStatusHeraldField.php

@@ -0,0 +1,55 @@
+<?php
+
+final class ManiphestTaskStatusHeraldField
+  extends ManiphestHeraldField {
+
+  const FIELDCONST = 'taskstatus';
+
+  public function getHeraldFieldName() {
+    return pht('Task status');
+  }
+
+  public function getHeraldFieldValue($object) {
+    return $object->getStatus();
+  }
+
+  protected function getHeraldFieldStandardConditions() {
+    return self::STANDARD_PHID;
+  }
+
+  public function getHeraldFieldValueType($condition) {
+    return HeraldAdapter::VALUE_TASK_STATUS;
+  }
+
+  public function renderConditionValue(
+    PhabricatorUser $viewer,
+    $value) {
+
+    $status_map = ManiphestTaskStatus::getTaskStatusMap();
+
+    $value = (array)$value;
+    foreach ($value as $index => $val) {
+      $name = idx($status_map, $val);
+      if ($name !== null) {
+        $value[$index] = $name;
+      }
+    }
+
+    return implode(', ', $value);
+  }
+
+  public function getEditorValue(
+    PhabricatorUser $viewer,
+    $value) {
+
+    $status_map = ManiphestTaskStatus::getTaskStatusMap();
+
+    $value_map = array();
+    foreach ($value as $status) {
+      $value_map[$status] = idx($status_map, $status, $status);
+    }
+
+    return $value_map;
+  }
+
+}