Move most Herald field configuration into dynamic Adapters

Summary: Ref T2769. Herald has a giant hard-coded list of fields. Primarily make these dynamic and adapter-based.

Test Plan: Viewed and edited Herald rules.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2769

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

src/applications/herald/adapter/HeraldAdapter.php

@@ -2,6 +2,20 @@
 
 abstract class HeraldAdapter {
 
+  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';
+
   abstract public function getPHID();
   abstract public function getHeraldName();
   abstract public function getHeraldTypeName();
@@ -21,6 +35,26 @@ abstract class HeraldAdapter {
   }
 
   abstract public function getAdapterContentName();
+  abstract public function getFields();
+
+  public function getFieldNameMap() {
+    return array(
+      self::FIELD_TITLE => pht('Title'),
+      self::FIELD_BODY => pht('Body'),
+      self::FIELD_AUTHOR => pht('Author'),
+      self::FIELD_REVIEWER => pht('Reviewer'),
+      self::FIELD_REVIEWERS => pht('Reviewers'),
+      self::FIELD_CC => pht('CCs'),
+      self::FIELD_TAGS => pht('Tags'),
+      self::FIELD_DIFF_FILE => pht('Any changed filename'),
+      self::FIELD_DIFF_CONTENT => pht('Any changed file content'),
+      self::FIELD_REPOSITORY => pht('Repository'),
+      self::FIELD_RULE => pht('Another Herald rule'),
+      self::FIELD_AFFECTED_PACKAGE => pht('Any affected package'),
+      self::FIELD_AFFECTED_PACKAGE_OWNER =>
+        pht("Any affected package's owner"),
+    );
+  }
 
   public static function applyFlagEffect(HeraldEffect $effect, $phid) {
     $color = $effect->getTarget();
@@ -82,6 +116,22 @@ abstract class HeraldAdapter {
     return $adapters;
   }
 
+  public static function getAdapterForContentType($content_type) {
+    $adapters = self::getAllAdapters();
+
+    foreach ($adapters as $adapter) {
+      if ($adapter->getAdapterContentType() == $content_type) {
+        return $adapter;
+      }
+    }
+
+    throw new Exception(
+      pht(
+        'No adapter exists for Herald content type "%s".',
+        $content_type));
+  }
+
+
 
 }
 

src/applications/herald/adapter/HeraldCommitAdapter.php

@@ -2,6 +2,11 @@
 
 final class HeraldCommitAdapter extends HeraldAdapter {
 
+  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';
+
   protected $diff;
   protected $revision;
 
@@ -30,6 +35,34 @@ final class HeraldCommitAdapter extends HeraldAdapter {
     return pht('Commits');
   }
 
+  public function getFieldNameMap() {
+    return array(
+      self::FIELD_NEED_AUDIT_FOR_PACKAGE =>
+        pht('Affected packages that need audit'),
+      self::FIELD_DIFFERENTIAL_REVISION => pht('Differential revision'),
+      self::FIELD_DIFFERENTIAL_REVIEWERS => pht('Differential reviewers'),
+      self::FIELD_DIFFERENTIAL_CCS => pht('Differential CCs'),
+    ) + parent::getFieldNameMap();
+  }
+
+  public function getFields() {
+    return array(
+      self::FIELD_BODY,
+      self::FIELD_AUTHOR,
+      self::FIELD_REVIEWER,
+      self::FIELD_REPOSITORY,
+      self::FIELD_DIFF_FILE,
+      self::FIELD_DIFF_CONTENT,
+      self::FIELD_RULE,
+      self::FIELD_AFFECTED_PACKAGE,
+      self::FIELD_AFFECTED_PACKAGE_OWNER,
+      self::FIELD_NEED_AUDIT_FOR_PACKAGE,
+      self::FIELD_DIFFERENTIAL_REVISION,
+      self::FIELD_DIFFERENTIAL_REVIEWERS,
+      self::FIELD_DIFFERENTIAL_CCS,
+    );
+  }
+
   public static function newLegacyAdapter(
     PhabricatorRepository $repository,
     PhabricatorRepositoryCommit $commit,

src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php

@@ -30,6 +30,23 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter {
     return pht('Differential Revisions');
   }
 
+  public function getFields() {
+    return array(
+      self::FIELD_TITLE,
+      self::FIELD_BODY,
+      self::FIELD_AUTHOR,
+      self::FIELD_REVIEWERS,
+      self::FIELD_CC,
+      self::FIELD_REPOSITORY,
+      self::FIELD_DIFF_FILE,
+      self::FIELD_DIFF_CONTENT,
+      self::FIELD_RULE,
+      self::FIELD_AFFECTED_PACKAGE,
+      self::FIELD_AFFECTED_PACKAGE_OWNER,
+    );
+  }
+
+
   public static function newLegacyAdapter(
     DifferentialRevision $revision,
     DifferentialDiff $diff) {

src/applications/herald/adapter/HeraldDryRunAdapter.php

@@ -26,6 +26,10 @@ final class HeraldDryRunAdapter extends HeraldAdapter {
     return null;
   }
 
+  public function getFields() {
+    return array();
+  }
+
   public function applyHeraldEffects(array $effects) {
     assert_instances_of($effects, 'HeraldEffect');
     $results = array();

src/applications/herald/config/HeraldConditionConfig.php

@@ -61,7 +61,6 @@ final class HeraldConditionConfig {
       case HeraldFieldConfig::FIELD_AUTHOR:
       case HeraldFieldConfig::FIELD_REPOSITORY:
       case HeraldFieldConfig::FIELD_REVIEWER:
-      case HeraldFieldConfig::FIELD_MERGE_REQUESTER:
         return array_select_keys(
           $map,
           array(

src/applications/herald/config/HeraldFieldConfig.php

@@ -2,6 +2,7 @@
 
 final class HeraldFieldConfig {
 
+  // TODO: Remove; still required by conditions, etc.
   const FIELD_TITLE                  = 'title';
   const FIELD_BODY                   = 'body';
   const FIELD_AUTHOR                 = 'author';
@@ -19,8 +20,8 @@ final class HeraldFieldConfig {
   const FIELD_DIFFERENTIAL_REVISION  = 'differential-revision';
   const FIELD_DIFFERENTIAL_REVIEWERS = 'differential-reviewers';
   const FIELD_DIFFERENTIAL_CCS       = 'differential-ccs';
-  const FIELD_MERGE_REQUESTER        = 'merge-requester';
 
+  // TODO: Remove; still required by transcripts.
   public static function getFieldMap() {
     $map = array(
       self::FIELD_TITLE                  => pht('Title'),
@@ -42,53 +43,9 @@ final class HeraldFieldConfig {
       self::FIELD_DIFFERENTIAL_REVISION  => pht('Differential revision'),
       self::FIELD_DIFFERENTIAL_REVIEWERS => pht('Differential reviewers'),
       self::FIELD_DIFFERENTIAL_CCS       => pht('Differential CCs'),
-      self::FIELD_MERGE_REQUESTER        => pht('Merge requester')
     );
 
     return $map;
   }
 
-  public static function getFieldMapForContentType($type) {
-    $map = self::getFieldMap();
-
-    switch ($type) {
-      case HeraldContentTypeConfig::CONTENT_TYPE_DIFFERENTIAL:
-        return array_select_keys(
-          $map,
-          array(
-            self::FIELD_TITLE,
-            self::FIELD_BODY,
-            self::FIELD_AUTHOR,
-            self::FIELD_REVIEWERS,
-            self::FIELD_CC,
-            self::FIELD_REPOSITORY,
-            self::FIELD_DIFF_FILE,
-            self::FIELD_DIFF_CONTENT,
-            self::FIELD_RULE,
-            self::FIELD_AFFECTED_PACKAGE,
-            self::FIELD_AFFECTED_PACKAGE_OWNER,
-          ));
-      case HeraldContentTypeConfig::CONTENT_TYPE_COMMIT:
-        return array_select_keys(
-          $map,
-          array(
-            self::FIELD_BODY,
-            self::FIELD_AUTHOR,
-            self::FIELD_REVIEWER,
-            self::FIELD_REPOSITORY,
-            self::FIELD_DIFF_FILE,
-            self::FIELD_DIFF_CONTENT,
-            self::FIELD_RULE,
-            self::FIELD_AFFECTED_PACKAGE,
-            self::FIELD_AFFECTED_PACKAGE_OWNER,
-            self::FIELD_NEED_AUDIT_FOR_PACKAGE,
-            self::FIELD_DIFFERENTIAL_REVISION,
-            self::FIELD_DIFFERENTIAL_REVIEWERS,
-            self::FIELD_DIFFERENTIAL_CCS,
-          ));
-      default:
-        throw new Exception("Unknown content type.");
-    }
-  }
-
 }

src/applications/herald/controller/HeraldRuleController.php

@@ -18,22 +18,24 @@ final class HeraldRuleController extends HeraldController {
     $rule_type_map = HeraldRuleTypeConfig::getRuleTypeMap();
 
     if ($this->id) {
-      $rule = id(new HeraldRule())->load($this->id);
+      $rule = id(new HeraldRuleQuery())
+        ->setViewer($user)
+        ->withIDs(array($this->id))
+        ->requireCapabilities(
+          array(
+            PhabricatorPolicyCapability::CAN_VIEW,
+            PhabricatorPolicyCapability::CAN_EDIT,
+          ))
+        ->executeOne();
       if (!$rule) {
         return new Aphront404Response();
       }
-      if (!$this->canEditRule($rule, $user)) {
-        throw new Exception("You don't own this rule and can't edit it.");
-      }
     } else {
       $rule = new HeraldRule();
       $rule->setAuthorPHID($user->getPHID());
       $rule->setMustMatchAll(true);
 
       $content_type = $request->getStr('content_type');
-      if (!isset($content_type_map[$content_type])) {
-        $content_type = HeraldContentTypeConfig::CONTENT_TYPE_DIFFERENTIAL;
-      }
       $rule->setContentType($content_type);
 
       $rule_type = $request->getStr('rule_type');
@@ -43,6 +45,8 @@ final class HeraldRuleController extends HeraldController {
       $rule->setRuleType($rule_type);
     }
 
+    $adapter = HeraldAdapter::getAdapterForContentType($rule->getContentType());
+
     $local_version = id(new HeraldRule())->getConfigVersion();
     if ($rule->getConfigVersion() > $local_version) {
       throw new Exception(
@@ -169,7 +173,7 @@ final class HeraldRuleController extends HeraldController {
           ->setValue(pht('Save Rule'))
           ->addCancelButton('/herald/view/'.$rule->getContentType().'/'));
 
-    $this->setupEditorBehavior($rule, $handles);
+    $this->setupEditorBehavior($rule, $handles, $adapter);
 
     $title = $rule->getID()
         ? pht('Edit Herald Rule')
@@ -194,13 +198,6 @@ final class HeraldRuleController extends HeraldController {
       ));
   }
 
-  private function canEditRule($rule, $user) {
-    return
-      ($user->getIsAdmin()) ||
-      ($rule->getRuleType() == HeraldRuleTypeConfig::RULE_TYPE_GLOBAL) ||
-      ($rule->getAuthorPHID() == $user->getPHID());
-  }
-
   private function saveRule($rule, $request) {
     $rule->setName($request->getStr('name'));
     $rule->setMustMatchAll(($request->getStr('must_match') == 'all'));
@@ -329,7 +326,11 @@ final class HeraldRuleController extends HeraldController {
     return array($e_name, $errors);
   }
 
-  private function setupEditorBehavior($rule, $handles) {
+  private function setupEditorBehavior(
+    HeraldRule $rule,
+    array $handles,
+    HeraldAdapter $adapter) {
+
     $serial_conditions = array(
       array('default', 'default', ''),
     );
@@ -386,9 +387,11 @@ final class HeraldRuleController extends HeraldController {
     $all_rules = mpull($all_rules, 'getName', 'getID');
     asort($all_rules);
 
+    $fields = $adapter->getFields();
+    $field_map = array_select_keys($adapter->getFieldNameMap(), $fields);
+
     $config_info = array();
-    $config_info['fields']
+    $config_info['fields'] = $field_map;
-      = HeraldFieldConfig::getFieldMapForContentType($rule->getContentType());
     $config_info['conditions'] = HeraldConditionConfig::getConditionMap();
     foreach ($config_info['fields'] as $field => $name) {
       $config_info['conditionMap'][$field] = array_keys(

src/applications/herald/engine/HeraldEngine.php

@@ -446,7 +446,6 @@ final class HeraldEngine {
         break;
       case HeraldFieldConfig::FIELD_AUTHOR:
       case HeraldFieldConfig::FIELD_REPOSITORY:
-      case HeraldFieldConfig::FIELD_MERGE_REQUESTER:
         // TODO: Type should be PHID.
         $result = $this->object->getHeraldField($field);
         break;