Modularize the Diffusion/Differential "Block" Herald actions

Also removes HeraldCustomAction. This completes action modularization.

Ref T8726.

resources/sql/autopatches/20150803.herald.2.sql

@@ -0,0 +1,13 @@
+UPDATE {$NAMESPACE}_herald.herald_action a
+  JOIN {$NAMESPACE}_herald.herald_rule r
+  ON a.ruleID = r.id
+  SET a.action = 'diffusion.block'
+  WHERE r.contentType != 'differential.diff'
+  AND a.action = 'block';
+
+UPDATE {$NAMESPACE}_herald.herald_action a
+  JOIN {$NAMESPACE}_herald.herald_rule r
+  ON a.ruleID = r.id
+  SET a.action = 'differential.block'
+  WHERE r.contentType = 'differential.diff'
+  AND a.action = 'block';

src/__phutil_library_map__.php

@@ -303,6 +303,7 @@ phutil_register_library_map(array(
     'DifferentialAuditorsField' => 'applications/differential/customfield/DifferentialAuditorsField.php',
     'DifferentialAuthorField' => 'applications/differential/customfield/DifferentialAuthorField.php',
     'DifferentialBlameRevisionField' => 'applications/differential/customfield/DifferentialBlameRevisionField.php',
+    'DifferentialBlockHeraldAction' => 'applications/differential/herald/DifferentialBlockHeraldAction.php',
     'DifferentialBranchField' => 'applications/differential/customfield/DifferentialBranchField.php',
     'DifferentialChangeHeraldFieldGroup' => 'applications/differential/herald/DifferentialChangeHeraldFieldGroup.php',
     'DifferentialChangeType' => 'applications/differential/constants/DifferentialChangeType.php',
@@ -501,6 +502,7 @@ phutil_register_library_map(array(
     'DiffusionAuditorsAddAuditorsHeraldAction' => 'applications/diffusion/herald/DiffusionAuditorsAddAuditorsHeraldAction.php',
     'DiffusionAuditorsAddSelfHeraldAction' => 'applications/diffusion/herald/DiffusionAuditorsAddSelfHeraldAction.php',
     'DiffusionAuditorsHeraldAction' => 'applications/diffusion/herald/DiffusionAuditorsHeraldAction.php',
+    'DiffusionBlockHeraldAction' => 'applications/diffusion/herald/DiffusionBlockHeraldAction.php',
     'DiffusionBranchQueryConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionBranchQueryConduitAPIMethod.php',
     'DiffusionBranchTableController' => 'applications/diffusion/controller/DiffusionBranchTableController.php',
     'DiffusionBranchTableView' => 'applications/diffusion/view/DiffusionBranchTableView.php',
@@ -1031,7 +1033,6 @@ phutil_register_library_map(array(
     'HeraldConditionTranscript' => 'applications/herald/storage/transcript/HeraldConditionTranscript.php',
     'HeraldContentSourceField' => 'applications/herald/field/HeraldContentSourceField.php',
     'HeraldController' => 'applications/herald/controller/HeraldController.php',
-    'HeraldCustomAction' => 'applications/herald/extension/HeraldCustomAction.php',
     'HeraldDAO' => 'applications/herald/storage/HeraldDAO.php',
     'HeraldDifferentialAdapter' => 'applications/differential/herald/HeraldDifferentialAdapter.php',
     'HeraldDifferentialDiffAdapter' => 'applications/differential/herald/HeraldDifferentialDiffAdapter.php',
@@ -3908,6 +3909,7 @@ phutil_register_library_map(array(
     'DifferentialAuditorsField' => 'DifferentialStoredCustomField',
     'DifferentialAuthorField' => 'DifferentialCustomField',
     'DifferentialBlameRevisionField' => 'DifferentialStoredCustomField',
+    'DifferentialBlockHeraldAction' => 'HeraldAction',
     'DifferentialBranchField' => 'DifferentialCustomField',
     'DifferentialChangeHeraldFieldGroup' => 'HeraldFieldGroup',
     'DifferentialChangeType' => 'Phobject',
@@ -4135,6 +4137,7 @@ phutil_register_library_map(array(
     'DiffusionAuditorsAddAuditorsHeraldAction' => 'DiffusionAuditorsHeraldAction',
     'DiffusionAuditorsAddSelfHeraldAction' => 'DiffusionAuditorsHeraldAction',
     'DiffusionAuditorsHeraldAction' => 'HeraldAction',
+    'DiffusionBlockHeraldAction' => 'HeraldAction',
     'DiffusionBranchQueryConduitAPIMethod' => 'DiffusionQueryConduitAPIMethod',
     'DiffusionBranchTableController' => 'DiffusionController',
     'DiffusionBranchTableView' => 'DiffusionView',
@@ -4745,7 +4748,6 @@ phutil_register_library_map(array(
     'HeraldConditionTranscript' => 'Phobject',
     'HeraldContentSourceField' => 'HeraldField',
     'HeraldController' => 'PhabricatorController',
-    'HeraldCustomAction' => 'Phobject',
     'HeraldDAO' => 'PhabricatorLiskDAO',
     'HeraldDifferentialAdapter' => 'HeraldAdapter',
     'HeraldDifferentialDiffAdapter' => 'HeraldDifferentialAdapter',

src/applications/differential/editor/DifferentialDiffEditor.php

@@ -131,10 +131,11 @@ final class DifferentialDiffEditor
           $rules = mpull($rules, null, 'getID');
 
           $effects = $engine->applyRules($rules, $adapter);
+          $action_block = DifferentialBlockHeraldAction::ACTIONCONST;
 
           $blocking_effect = null;
           foreach ($effects as $effect) {
-            if ($effect->getAction() == HeraldAdapter::ACTION_BLOCK) {
+            if ($effect->getAction() == $action_block) {
               $blocking_effect = $effect;
               break;
             }

src/applications/differential/herald/DifferentialBlockHeraldAction.php

@@ -0,0 +1,56 @@
+<?php
+
+final class DifferentialBlockHeraldAction
+  extends HeraldAction {
+
+  const ACTIONCONST = 'differential.block';
+
+  const DO_BLOCK = 'do.block';
+
+  public function getHeraldActionName() {
+    return pht('Block diff with message');
+  }
+
+  public function getActionGroupKey() {
+    return HeraldApplicationActionGroup::ACTIONGROUPKEY;
+  }
+
+  public function supportsObject($object) {
+    return ($object instanceof DifferentialDiff);
+  }
+
+  public function supportsRuleType($rule_type) {
+    return ($rule_type != HeraldRuleTypeConfig::RULE_TYPE_PERSONAL);
+  }
+
+  public function applyEffect($object, HeraldEffect $effect) {
+    // This rule intentionally has no direct effect: the caller handles it
+    // after executing Herald.
+    $this->logEffect(self::DO_BLOCK);
+  }
+
+  public function getHeraldActionStandardType() {
+    return self::STANDARD_TEXT;
+  }
+
+  public function renderActionDescription($value) {
+    return pht('Block diff with message: %s', $value);
+  }
+
+  protected function getActionEffectMap() {
+    return array(
+      self::DO_BLOCK => array(
+        'icon' => 'fa-stop',
+        'color' => 'red',
+        'name' => pht('Blocked Diff'),
+      ),
+    );
+  }
+
+  protected function renderActionEffectDescription($type, $data) {
+    switch ($type) {
+      case self::DO_ADD_AUDITORS:
+        return pht('Blocked diff.');
+    }
+  }
+}

src/applications/differential/herald/HeraldDifferentialDiffAdapter.php

@@ -63,43 +63,4 @@ final class HeraldDifferentialDiffAdapter extends HeraldDifferentialAdapter {
     return pht('New Diff');
   }
 
-  public function getActionNameMap($rule_type) {
-    return array(
-      self::ACTION_BLOCK => pht('Block diff with message'),
-    ) + parent::getActionNameMap($rule_type);
-  }
-
-  public function getActions($rule_type) {
-    switch ($rule_type) {
-      case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL:
-        return array_merge(
-          array(
-            self::ACTION_BLOCK,
-          ),
-          parent::getActions($rule_type));
-    }
-  }
-
-  public function applyHeraldEffects(array $effects) {
-    assert_instances_of($effects, 'HeraldEffect');
-
-    $result = array();
-    foreach ($effects as $effect) {
-      $action = $effect->getAction();
-      switch ($action) {
-        case self::ACTION_BLOCK:
-          $result[] = new HeraldApplyTranscript(
-            $effect,
-            true,
-            pht('Blocked diff.'));
-          break;
-        default:
-          $result[] = $this->applyStandardEffect($effect);
-          break;
-      }
-    }
-
-    return $result;
-  }
-
 }

src/applications/diffusion/engine/DiffusionCommitHookEngine.php

@@ -325,9 +325,11 @@ final class DiffusionCommitHookEngine extends Phobject {
         $this->emailPHIDs[$email_phid] = $email_phid;
       }
 
+      $block_action = DiffusionBlockHeraldAction::ACTIONCONST;
+
       if ($blocking_effect === null) {
         foreach ($effects as $effect) {
-          if ($effect->getAction() == HeraldAdapter::ACTION_BLOCK) {
+          if ($effect->getAction() == $block_action) {
             $blocking_effect = $effect;
             $blocked_update = $update;
             break;

src/applications/diffusion/herald/DiffusionBlockHeraldAction.php

@@ -0,0 +1,56 @@
+<?php
+
+final class DiffusionBlockHeraldAction
+  extends HeraldAction {
+
+  const ACTIONCONST = 'diffusion.block';
+
+  const DO_BLOCK = 'do.block';
+
+  public function getHeraldActionName() {
+    return pht('Block push with message');
+  }
+
+  public function getActionGroupKey() {
+    return HeraldApplicationActionGroup::ACTIONGROUPKEY;
+  }
+
+  public function supportsObject($object) {
+    return ($object instanceof PhabricatorRepositoryPushLog);
+  }
+
+  public function supportsRuleType($rule_type) {
+    return ($rule_type != HeraldRuleTypeConfig::RULE_TYPE_PERSONAL);
+  }
+
+  public function applyEffect($object, HeraldEffect $effect) {
+    // This rule intentionally has no direct effect: the caller handles it
+    // after executing Herald.
+    $this->logEffect(self::DO_BLOCK);
+  }
+
+  public function getHeraldActionStandardType() {
+    return self::STANDARD_TEXT;
+  }
+
+  public function renderActionDescription($value) {
+    return pht('Block push with message: %s', $value);
+  }
+
+  protected function getActionEffectMap() {
+    return array(
+      self::DO_BLOCK => array(
+        'icon' => 'fa-stop',
+        'color' => 'red',
+        'name' => pht('Blocked Push'),
+      ),
+    );
+  }
+
+  protected function renderActionEffectDescription($type, $data) {
+    switch ($type) {
+      case self::DO_ADD_AUDITORS:
+        return pht('Blocked push.');
+    }
+  }
+}

src/applications/diffusion/herald/HeraldPreCommitAdapter.php

@@ -73,43 +73,4 @@ abstract class HeraldPreCommitAdapter extends HeraldAdapter {
       $this->hookEngine->getRepository()->getProjectPHIDs());
   }
 
-  public function getActions($rule_type) {
-    switch ($rule_type) {
-      case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL:
-      case HeraldRuleTypeConfig::RULE_TYPE_OBJECT:
-        return array_merge(
-          array(
-            self::ACTION_BLOCK,
-          ),
-          parent::getActions($rule_type));
-      case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL:
-        return array_merge(
-          array(
-          ),
-          parent::getActions($rule_type));
-    }
-  }
-
-  public function applyHeraldEffects(array $effects) {
-    assert_instances_of($effects, 'HeraldEffect');
-
-    $result = array();
-    foreach ($effects as $effect) {
-      $action = $effect->getAction();
-      switch ($action) {
-        case self::ACTION_BLOCK:
-          $result[] = new HeraldApplyTranscript(
-            $effect,
-            true,
-            pht('Blocked push.'));
-          break;
-        default:
-          $result[] = $this->applyStandardEffect($effect);
-          break;
-      }
-    }
-
-    return $result;
-  }
-
 }

src/applications/herald/action/HeraldAction.php

@@ -8,12 +8,15 @@ abstract class HeraldAction extends Phobject {
 
   const STANDARD_NONE = 'standard.none';
   const STANDARD_PHID_LIST = 'standard.phid.list';
+  const STANDARD_TEXT = 'standard.text';
 
   const DO_STANDARD_EMPTY = 'do.standard.empty';
   const DO_STANDARD_NO_EFFECT = 'do.standard.no-effect';
   const DO_STANDARD_INVALID = 'do.standard.invalid';
   const DO_STANDARD_UNLOADABLE = 'do.standard.unloadable';
   const DO_STANDARD_PERMISSION = 'do.standard.permission';
+  const DO_STANDARD_INVALID_ACTION = 'do.standard.invalid-action';
+  const DO_STANDARD_WRONG_RULE_TYPE = 'do.standard.wrong-rule-type';
 
   abstract public function getHeraldActionName();
   abstract public function supportsObject($object);
@@ -48,6 +51,8 @@ abstract class HeraldAction extends Phobject {
     switch ($this->getHeraldActionStandardType()) {
       case self::STANDARD_NONE:
         return new HeraldEmptyFieldValue();
+      case self::STANDARD_TEXT:
+        return new HeraldTextFieldValue();
       case self::STANDARD_PHID_LIST:
         $tokenizer = id(new HeraldTokenizerFieldValue())
           ->setKey($this->getHeraldActionName())
@@ -324,6 +329,16 @@ abstract class HeraldAction extends Phobject {
         'color' => 'red',
         'name' => pht('No Permission'),
       ),
+      self::DO_STANDARD_INVALID_ACTION => array(
+        'icon' => 'fa-ban',
+        'color' => 'red',
+        'name' => pht('Invalid Action'),
+      ),
+      self::DO_STANDARD_WRONG_RULE_TYPE => array(
+        'icon' => 'fa-ban',
+        'color' => 'red',
+        'name' => pht('Wrong Rule Type'),
+      ),
     );
   }
 
@@ -357,6 +372,14 @@ abstract class HeraldAction extends Phobject {
           '%s target(s) do not have permission to see this object: %s.',
           new PhutilNumber(count($data)),
           $this->renderHandleList($data));
+      case self::DO_STANDARD_INVALID_ACTION:
+        return pht(
+          'No implementation is available for rule "%s".',
+          $data);
+      case self::DO_STANDARD_WRONG_RULE_TYPE:
+        return pht(
+          'This action does not support rules of type "%s".',
+          $data);
     }
 
     return null;

src/applications/herald/adapter/HeraldAdapter.php

@@ -26,12 +26,9 @@ abstract class HeraldAdapter extends Phobject {
   const CONDITION_IS_TRUE         = 'true';
   const CONDITION_IS_FALSE        = 'false';
 
-  const ACTION_BLOCK = 'block';
-
   private $contentSource;
   private $isNewObject;
   private $applicationEmail;
-  private $customActions = null;
   private $queuedTransactions = array();
   private $emailPHIDs = array();
   private $forcedEmailPHIDs = array();
@@ -55,37 +52,6 @@ abstract class HeraldAdapter extends Phobject {
     return $this;
   }
 
-  public function getCustomActions() {
-    if ($this->customActions === null) {
-      $custom_actions = id(new PhutilSymbolLoader())
-        ->setAncestorClass('HeraldCustomAction')
-        ->loadObjects();
-
-      foreach ($custom_actions as $key => $object) {
-        if (!$object->appliesToAdapter($this)) {
-          unset($custom_actions[$key]);
-        }
-      }
-
-      $this->customActions = array();
-      foreach ($custom_actions as $action) {
-        $key = $action->getActionKey();
-
-        if (array_key_exists($key, $this->customActions)) {
-          throw new Exception(
-            pht(
-              "More than one Herald custom action implementation ".
-              "handles the action key: '%s'.",
-              $key));
-        }
-
-        $this->customActions[$key] = $action;
-      }
-    }
-
-    return $this->customActions;
-  }
-
   public function setContentSource(PhabricatorContentSource $content_source) {
     $this->contentSource = $content_source;
     return $this;
@@ -145,19 +111,6 @@ abstract class HeraldAdapter extends Phobject {
     return $result;
   }
 
-  protected function handleCustomHeraldEffect(HeraldEffect $effect) {
-    $custom_action = idx($this->getCustomActions(), $effect->getAction());
-
-    if ($custom_action !== null) {
-      return $custom_action->applyEffect(
-        $this,
-        $this->getObject(),
-        $effect);
-    }
-
-    return null;
-  }
-
   public function isAvailableToUser(PhabricatorUser $viewer) {
     $applications = id(new PhabricatorApplicationQuery())
       ->setViewer($viewer)
@@ -654,6 +607,20 @@ abstract class HeraldAdapter extends Phobject {
     return $this->actionMap;
   }
 
+  private function requireActionImplementation($action_key) {
+    $action = $this->getActionImplementation($action_key);
+
+    if (!$action) {
+      throw new Exception(
+        pht(
+          'No action with key "%s" is available to Herald adapter "%s".',
+          $action_key,
+          get_class($this)));
+    }
+
+    return $action;
+  }
+
   private function getActionsForRuleType($rule_type) {
     $actions = $this->getActionImplementationMap();
 
@@ -683,22 +650,8 @@ abstract class HeraldAdapter extends Phobject {
     return $action->getActionGroupKey();
   }
 
-  public function getCustomActionsForRuleType($rule_type) {
-    $results = array();
-    foreach ($this->getCustomActions() as $custom_action) {
-      if ($custom_action->appliesToRuleType($rule_type)) {
-        $results[] = $custom_action;
-      }
-    }
-    return $results;
-  }
-
   public function getActions($rule_type) {
-    $custom_actions = $this->getCustomActionsForRuleType($rule_type);
-    $custom_actions = mpull($custom_actions, 'getActionKey');
-
-    $actions = $custom_actions;
-
+    $actions = array();
     foreach ($this->getActionsForRuleType($rule_type) as $key => $action) {
       $actions[] = $key;
     }
@@ -707,62 +660,21 @@ abstract class HeraldAdapter extends Phobject {
   }
 
   public function getActionNameMap($rule_type) {
-    switch ($rule_type) {
-      case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL:
-      case HeraldRuleTypeConfig::RULE_TYPE_OBJECT:
-        $standard = array(
-          self::ACTION_BLOCK => pht('Block change with message'),
-        );
-        break;
-      case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL:
-        $standard = array(
-        );
-        break;
-      default:
-        throw new Exception(pht("Unknown rule type '%s'!", $rule_type));
-    }
-
-    $custom_actions = $this->getCustomActionsForRuleType($rule_type);
-    $standard += mpull($custom_actions, 'getActionName', 'getActionKey');
-
+    $map = array();
     foreach ($this->getActionsForRuleType($rule_type) as $key => $action) {
-      $standard[$key] = $action->getHeraldActionName();
+      $map[$key] = $action->getHeraldActionName();
     }
 
-    return $standard;
+    return $map;
   }
 
   public function willSaveAction(
     HeraldRule $rule,
     HeraldActionRecord $action) {
 
-    $impl = $this->getActionImplementation($action->getAction());
-    if ($impl) {
-      $target = $action->getTarget();
-      $target = $impl->willSaveActionValue($target);
-      $action->setTarget($target);
-      return;
-    }
-
+    $impl = $this->requireActionImplementation($action->getAction());
     $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_BLOCK:
-          break;
-        default:
-          throw new HeraldInvalidActionException(
-            pht(
-              'Unrecognized action type "%s"!',
-              $action->getAction()));
-      }
-    }
+    $target = $impl->willSaveActionValue($target);
 
     $action->setTarget($target);
   }
@@ -778,26 +690,8 @@ abstract class HeraldAdapter extends Phobject {
   }
 
   public function getValueTypeForAction($action, $rule_type) {
-    $impl = $this->getActionImplementation($action);
-    if ($impl) {
-      return $impl->getHeraldActionValueType();
-    }
-
-    $is_personal = ($rule_type == HeraldRuleTypeConfig::RULE_TYPE_PERSONAL);
-
-    if (!$is_personal) {
-      switch ($action) {
-        case self::ACTION_BLOCK:
-          return new HeraldTextFieldValue();
-      }
-    }
-
-    $custom_action = idx($this->getCustomActions(), $action);
-    if ($custom_action !== null) {
-      return $custom_action->getActionType();
-    }
-
-    throw new Exception(pht("Unknown or invalid action '%s'.", $action));
+    $impl = $this->requireActionImplementation($action);
+    return $impl->getHeraldActionValueType();
   }
 
   private function buildTokenizerFieldValue(
@@ -1113,38 +1007,32 @@ abstract class HeraldAdapter extends Phobject {
     $rule_type = $effect->getRule()->getRuleType();
 
     $impl = $this->getActionImplementation($action);
-    if ($impl) {
-      if ($impl->supportsRuleType($rule_type)) {
-        $impl->applyEffect($this->getObject(), $effect);
-        return $impl->getApplyTranscript($effect);
-      }
-    }
-
-    $supported = $this->getActions($rule_type);
-    $supported = array_fuse($supported);
-    if (empty($supported[$action])) {
+    if (!$impl) {
       return new HeraldApplyTranscript(
         $effect,
         false,
-        pht(
-          'Adapter "%s" does not support action "%s" for rule type "%s".',
-          get_class($this),
-          $action,
-          $rule_type));
+        array(
+          array(
+            HeraldAction::DO_STANDARD_INVALID_ACTION,
+            $action,
+          ),
+        ));
     }
 
-    $result = $this->handleCustomHeraldEffect($effect);
-
-    if (!$result) {
+    if (!$impl->supportsRuleType($rule_type)) {
       return new HeraldApplyTranscript(
         $effect,
         false,
-        pht(
-          'No custom action exists to handle rule action "%s".',
-          $action));
+        array(
+          array(
+            HeraldAction::DO_STANDARD_WRONG_RULE_TYPE,
+            $rule_type,
+          ),
+        ));
     }
 
-    return $result;
+    $impl->applyEffect($this->getObject(), $effect);
+    return $impl->getApplyTranscript($effect);
   }
 
   public function loadEdgePHIDs($type) {

src/applications/herald/extension/HeraldCustomAction.php

@@ -1,20 +0,0 @@
-<?php
-
-abstract class HeraldCustomAction extends Phobject {
-
-  abstract public function appliesToAdapter(HeraldAdapter $adapter);
-
-  abstract public function appliesToRuleType($rule_type);
-
-  abstract public function getActionKey();
-
-  abstract public function getActionName();
-
-  abstract public function getActionType();
-
-  abstract public function applyEffect(
-    HeraldAdapter $adapter,
-    $object,
-    HeraldEffect $effect);
-
-}