Remove HeraldActionConfig, HeraldFieldConfig

Summary:
Ref T2769. Move all of this stuff into Adapters and get rid of the hard-coded classes.

I cheated in two places.

Test Plan: Edited and activated Herald rules.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, chad

Maniphest Tasks: T2769

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

src/__phutil_library_map__.php

@@ -595,7 +595,6 @@ phutil_register_library_map(array(
     'HarbormasterRunnerWorker' => 'applications/harbormaster/worker/HarbormasterRunnerWorker.php',
     'HarbormasterScratchTable' => 'applications/harbormaster/storage/HarbormasterScratchTable.php',
     'HeraldAction' => 'applications/herald/storage/HeraldAction.php',
-    'HeraldActionConfig' => 'applications/herald/config/HeraldActionConfig.php',
     'HeraldAdapter' => 'applications/herald/adapter/HeraldAdapter.php',
     'HeraldApplyTranscript' => 'applications/herald/storage/transcript/HeraldApplyTranscript.php',
     'HeraldCommitAdapter' => 'applications/herald/adapter/HeraldCommitAdapter.php',
@@ -609,13 +608,13 @@ phutil_register_library_map(array(
     'HeraldEditLogQuery' => 'applications/herald/query/HeraldEditLogQuery.php',
     'HeraldEffect' => 'applications/herald/engine/HeraldEffect.php',
     'HeraldEngine' => 'applications/herald/engine/HeraldEngine.php',
-    'HeraldFieldConfig' => 'applications/herald/config/HeraldFieldConfig.php',
+    'HeraldInvalidActionException' => 'applications/herald/engine/exception/HeraldInvalidActionException.php',
-    'HeraldInvalidConditionException' => 'applications/herald/engine/engine/HeraldInvalidConditionException.php',
+    'HeraldInvalidConditionException' => 'applications/herald/engine/exception/HeraldInvalidConditionException.php',
-    'HeraldInvalidFieldException' => 'applications/herald/engine/engine/HeraldInvalidFieldException.php',
+    'HeraldInvalidFieldException' => 'applications/herald/engine/exception/HeraldInvalidFieldException.php',
     'HeraldNewController' => 'applications/herald/controller/HeraldNewController.php',
     'HeraldObjectTranscript' => 'applications/herald/storage/transcript/HeraldObjectTranscript.php',
     'HeraldPHIDTypeRule' => 'applications/herald/phid/HeraldPHIDTypeRule.php',
-    'HeraldRecursiveConditionsException' => 'applications/herald/engine/engine/HeraldRecursiveConditionsException.php',
+    'HeraldRecursiveConditionsException' => 'applications/herald/engine/exception/HeraldRecursiveConditionsException.php',
     'HeraldRepetitionPolicyConfig' => 'applications/herald/config/HeraldRepetitionPolicyConfig.php',
     'HeraldRule' => 'applications/herald/storage/HeraldRule.php',
     'HeraldRuleController' => 'applications/herald/controller/HeraldRuleController.php',
@@ -2617,6 +2616,7 @@ phutil_register_library_map(array(
     'HeraldDifferentialRevisionAdapter' => 'HeraldAdapter',
     'HeraldDryRunAdapter' => 'HeraldAdapter',
     'HeraldEditLogQuery' => 'PhabricatorOffsetPagedQuery',
+    'HeraldInvalidActionException' => 'Exception',
     'HeraldInvalidConditionException' => 'Exception',
     'HeraldInvalidFieldException' => 'Exception',
     'HeraldNewController' => 'HeraldController',

src/applications/herald/adapter/HeraldAdapter.php

@@ -435,6 +435,48 @@ abstract class HeraldAdapter {
     }
   }
 
+  public function willSaveAction(
+    HeraldRule $rule,
+    HeraldAction $action) {
+
+    $target = $action->getTarget();
+    if (is_array($target)) {
+      $target = array_keys($target);
+    }
+
+    $author_phid = $rule->getAuthorPHID();
+
+    $rule_type = $rule->getRuleType();
+    if ($rule_type == HeraldRuleTypeConfig::RULE_TYPE_PERSONAL) {
+      switch ($action->getAction()) {
+        case self::ACTION_EMAIL:
+        case self::ACTION_ADD_CC:
+        case self::ACTION_REMOVE_CC:
+        case self::ACTION_AUDIT:
+          // For personal rules, force these actions to target the rule owner.
+          $target = array($author_phid);
+          break;
+        case self::ACTION_FLAG:
+          // Make sure flag color is valid; set to blue if not.
+          $color_map = PhabricatorFlagColor::getColorNameMap();
+          if (empty($color_map[$target])) {
+            $target = PhabricatorFlagColor::COLOR_BLUE;
+          }
+          break;
+        case self::ACTION_NOTHING:
+          break;
+        default:
+          throw new HeraldInvalidActionException(
+            pht(
+              'Unrecognized action type "%s"!',
+              $action->getAction()));
+      }
+    }
+
+    $action->setTarget($target);
+  }
+
+
 
 /* -(  Values  )------------------------------------------------------------- */
 

src/applications/herald/adapter/HeraldCommitAdapter.php

@@ -299,13 +299,13 @@ final class HeraldCommitAdapter extends HeraldAdapter {
     foreach ($effects as $effect) {
       $action = $effect->getAction();
       switch ($action) {
-        case HeraldActionConfig::ACTION_NOTHING:
+        case self::ACTION_NOTHING:
           $result[] = new HeraldApplyTranscript(
             $effect,
             true,
             pht('Great success at doing nothing.'));
           break;
-        case HeraldActionConfig::ACTION_EMAIL:
+        case self::ACTION_EMAIL:
           foreach ($effect->getTarget() as $phid) {
             $this->emailPHIDs[$phid] = true;
           }
@@ -314,7 +314,7 @@ final class HeraldCommitAdapter extends HeraldAdapter {
             true,
             pht('Added address to email targets.'));
           break;
-        case HeraldActionConfig::ACTION_AUDIT:
+        case self::ACTION_AUDIT:
           foreach ($effect->getTarget() as $phid) {
             if (empty($this->auditMap[$phid])) {
               $this->auditMap[$phid] = array();
@@ -326,7 +326,7 @@ final class HeraldCommitAdapter extends HeraldAdapter {
             true,
             pht('Triggered an audit.'));
           break;
-        case HeraldActionConfig::ACTION_FLAG:
+        case self::ACTION_FLAG:
           $result[] = parent::applyFlagEffect(
             $effect,
             $this->commit->getPHID());

src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php

@@ -269,7 +269,7 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter {
     $result = array();
     if ($this->explicitCCs) {
       $effect = new HeraldEffect();
-      $effect->setAction(HeraldActionConfig::ACTION_ADD_CC);
+      $effect->setAction(self::ACTION_ADD_CC);
       $effect->setTarget(array_keys($this->explicitCCs));
       $effect->setReason(
         pht('CCs provided explicitly by revision author or carried over '.
@@ -287,20 +287,20 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter {
     foreach ($effects as $effect) {
       $action = $effect->getAction();
       switch ($action) {
-        case HeraldActionConfig::ACTION_NOTHING:
+        case self::ACTION_NOTHING:
           $result[] = new HeraldApplyTranscript(
             $effect,
             true,
             pht('OK, did nothing.'));
           break;
-        case HeraldActionConfig::ACTION_FLAG:
+        case self::ACTION_FLAG:
           $result[] = parent::applyFlagEffect(
             $effect,
             $this->revision->getPHID());
           break;
-        case HeraldActionConfig::ACTION_EMAIL:
+        case self::ACTION_EMAIL:
-        case HeraldActionConfig::ACTION_ADD_CC:
+        case self::ACTION_ADD_CC:
-          $op = ($action == HeraldActionConfig::ACTION_EMAIL) ? 'email' : 'CC';
+          $op = ($action == self::ACTION_EMAIL) ? 'email' : 'CC';
           $base_target = $effect->getTarget();
           $forbidden = array();
           foreach ($base_target as $key => $fbid) {
@@ -308,7 +308,7 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter {
               $forbidden[] = $fbid;
               unset($base_target[$key]);
             } else {
-              if ($action == HeraldActionConfig::ACTION_EMAIL) {
+              if ($action == self::ACTION_EMAIL) {
                 $this->emailPHIDs[$fbid] = true;
               } else {
                 $this->newCCs[$fbid] = true;
@@ -338,7 +338,7 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter {
               pht('Added addresses to %s list.', $op));
           }
           break;
-        case HeraldActionConfig::ACTION_REMOVE_CC:
+        case self::ACTION_REMOVE_CC:
           foreach ($effect->getTarget() as $fbid) {
             $this->remCCs[$fbid] = true;
           }

src/applications/herald/config/HeraldActionConfig.php

@@ -1,60 +0,0 @@
-<?php
-
-final class HeraldActionConfig {
-
-  const ACTION_ADD_CC       = 'addcc';
-  const ACTION_REMOVE_CC    = 'remcc';
-  const ACTION_EMAIL        = 'email';
-  const ACTION_NOTHING      = 'nothing';
-  const ACTION_AUDIT        = 'audit';
-  const ACTION_FLAG         = 'flag';
-
-  /**
-   * Create a HeraldAction to save from data.
-   *
-   * $data is of the form:
-   *   array(
-   *     0 => <action type>
-   *     1 => array(<targets>)
-   *   )
-   */
-  public static function willSaveAction($rule_type,
-                                        $author_phid,
-                                        $data) {
-    $obj = new HeraldAction();
-    $obj->setAction($data[0]);
-
-    // for personal rule types, set the target to be the owner of the rule
-    if ($rule_type == HeraldRuleTypeConfig::RULE_TYPE_PERSONAL) {
-      switch ($obj->getAction()) {
-        case HeraldActionConfig::ACTION_EMAIL:
-        case HeraldActionConfig::ACTION_ADD_CC:
-        case HeraldActionConfig::ACTION_REMOVE_CC:
-        case HeraldActionConfig::ACTION_AUDIT:
-          $data[1] = array($author_phid => $author_phid);
-          break;
-        case HeraldActionConfig::ACTION_FLAG:
-          // Make sure flag color is valid; set to blue if not.
-          $color_map = PhabricatorFlagColor::getColorNameMap();
-          if (empty($color_map[$data[1]])) {
-            $data[1] = PhabricatorFlagColor::COLOR_BLUE;
-          }
-          break;
-        case HeraldActionConfig::ACTION_NOTHING:
-          break;
-        default:
-          throw new Exception('Unrecognized action type: ' .
-                              $obj->getAction());
-      }
-    }
-
-    if (is_array($data[1])) {
-      $obj->setTarget(array_keys($data[1]));
-    } else {
-      $obj->setTarget($data[1]);
-    }
-
-    return $obj;
-  }
-
-}

src/applications/herald/config/HeraldFieldConfig.php

@@ -1,24 +0,0 @@
-<?php
-
-final class HeraldFieldConfig {
-
-  // TODO: Remove; still required by conditions, etc.
-  const FIELD_TITLE                  = 'title';
-  const FIELD_BODY                   = 'body';
-  const FIELD_AUTHOR                 = 'author';
-  const FIELD_REVIEWER               = 'reviewer';
-  const FIELD_REVIEWERS              = 'reviewers';
-  const FIELD_CC                     = 'cc';
-  const FIELD_TAGS                   = 'tags';
-  const FIELD_DIFF_FILE              = 'diff-file';
-  const FIELD_DIFF_CONTENT           = 'diff-content';
-  const FIELD_REPOSITORY             = 'repository';
-  const FIELD_RULE                   = 'rule';
-  const FIELD_AFFECTED_PACKAGE       = 'affected-package';
-  const FIELD_AFFECTED_PACKAGE_OWNER = 'affected-package-owner';
-  const FIELD_NEED_AUDIT_FOR_PACKAGE  = 'need-audit-for-package';
-  const FIELD_DIFFERENTIAL_REVISION  = 'differential-revision';
-  const FIELD_DIFFERENTIAL_REVIEWERS = 'differential-reviewers';
-  const FIELD_DIFFERENTIAL_CCS       = 'differential-ccs';
-
-}

src/applications/herald/controller/HeraldRuleController.php

@@ -261,9 +261,17 @@ final class HeraldRuleController extends HeraldController {
         $action[1] = null;
       }
 
-      $actions[] = HeraldActionConfig::willSaveAction($rule->getRuleType(),
+      $obj = new HeraldAction();
-                                                      $rule->getAuthorPHID(),
+      $obj->setAction($action[0]);
-                                                      $action);
+      $obj->setTarget($action[1]);
+
+      try {
+        $adapter->willSaveAction($rule, $obj);
+      } catch (HeraldInvalidActionException $ex) {
+        $errors[] = $ex;
+      }
+
+      $actions[] = $obj;
     }
 
     $rule->attachConditions($conditions);
@@ -328,7 +336,7 @@ final class HeraldRuleController extends HeraldController {
       foreach ($rule->getActions() as $action) {
 
         switch ($action->getAction()) {
-          case HeraldActionConfig::ACTION_FLAG:
+          case HeraldAdapter::ACTION_FLAG:
             $current_value = $action->getTarget();
             break;
           default:

src/applications/herald/controller/HeraldTranscriptController.php

@@ -273,10 +273,10 @@ final class HeraldTranscriptController extends HeraldController {
 
       $target = $apply_xscript->getTarget();
       switch ($apply_xscript->getAction()) {
-        case HeraldActionConfig::ACTION_NOTHING:
+        case HeraldAdapter::ACTION_NOTHING:
           $target = '';
           break;
-        case HeraldActionConfig::ACTION_FLAG:
+        case HeraldAdapter::ACTION_FLAG:
           $target = PhabricatorFlagColor::getColorName($target);
           break;
         default:

src/applications/herald/engine/exception/HeraldInvalidActionException.php

@@ -0,0 +1,3 @@
+<?php
+
+final class HeraldInvalidActionException extends Exception {}