Make "Add Subscribers" and "Remove Subscribers" Herald actions modular

Summary: Ref T8726. Converts these actions to be modular. No real surprises in this change.

Test Plan:
{F658709}

  - Wrote some rules.
  - Migrated them forward.
  - Used a bunch of these rules.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: joshuaspence, epriestley

Maniphest Tasks: T8726

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

resources/sql/autopatches/20150724.herald.1.sql

@@ -0,0 +1,27 @@
+UPDATE {$NAMESPACE}_herald.herald_actionrecord a
+  JOIN {$NAMESPACE}_herald.herald_rule r
+  ON a.ruleID = r.id
+  SET a.action = 'subscribers.add'
+  WHERE r.ruleType != 'personal'
+  AND a.action = 'addcc';
+
+UPDATE {$NAMESPACE}_herald.herald_actionrecord a
+  JOIN {$NAMESPACE}_herald.herald_rule r
+  ON a.ruleID = r.id
+  SET a.action = 'subscribers.self.add'
+  WHERE r.ruleType = 'personal'
+  AND a.action = 'addcc';
+
+UPDATE {$NAMESPACE}_herald.herald_actionrecord a
+  JOIN {$NAMESPACE}_herald.herald_rule r
+  ON a.ruleID = r.id
+  SET a.action = 'subscribers.remove'
+  WHERE r.ruleType != 'personal'
+  AND a.action = 'remcc';
+
+UPDATE {$NAMESPACE}_herald.herald_actionrecord a
+  JOIN {$NAMESPACE}_herald.herald_rule r
+  ON a.ruleID = r.id
+  SET a.action = 'subscribers.self.remove'
+  WHERE r.ruleType = 'personal'
+  AND a.action = 'remcc';

src/__phutil_library_map__.php

@@ -2880,10 +2880,15 @@ phutil_register_library_map(array(
     'PhabricatorSubscribedToObjectEdgeType' => 'applications/transactions/edges/PhabricatorSubscribedToObjectEdgeType.php',
     'PhabricatorSubscribersQuery' => 'applications/subscriptions/query/PhabricatorSubscribersQuery.php',
     'PhabricatorSubscriptionTriggerClock' => 'infrastructure/daemon/workers/clock/PhabricatorSubscriptionTriggerClock.php',
+    'PhabricatorSubscriptionsAddSelfHeraldAction' => 'applications/subscriptions/herald/PhabricatorSubscriptionsAddSelfHeraldAction.php',
+    'PhabricatorSubscriptionsAddSubscribersHeraldAction' => 'applications/subscriptions/herald/PhabricatorSubscriptionsAddSubscribersHeraldAction.php',
     'PhabricatorSubscriptionsApplication' => 'applications/subscriptions/application/PhabricatorSubscriptionsApplication.php',
     'PhabricatorSubscriptionsEditController' => 'applications/subscriptions/controller/PhabricatorSubscriptionsEditController.php',
     'PhabricatorSubscriptionsEditor' => 'applications/subscriptions/editor/PhabricatorSubscriptionsEditor.php',
+    'PhabricatorSubscriptionsHeraldAction' => 'applications/subscriptions/herald/PhabricatorSubscriptionsHeraldAction.php',
     'PhabricatorSubscriptionsListController' => 'applications/subscriptions/controller/PhabricatorSubscriptionsListController.php',
+    'PhabricatorSubscriptionsRemoveSelfHeraldAction' => 'applications/subscriptions/herald/PhabricatorSubscriptionsRemoveSelfHeraldAction.php',
+    'PhabricatorSubscriptionsRemoveSubscribersHeraldAction' => 'applications/subscriptions/herald/PhabricatorSubscriptionsRemoveSubscribersHeraldAction.php',
     'PhabricatorSubscriptionsSubscribeEmailCommand' => 'applications/subscriptions/command/PhabricatorSubscriptionsSubscribeEmailCommand.php',
     'PhabricatorSubscriptionsSubscribersPolicyRule' => 'applications/subscriptions/policyrule/PhabricatorSubscriptionsSubscribersPolicyRule.php',
     'PhabricatorSubscriptionsTransactionController' => 'applications/subscriptions/controller/PhabricatorSubscriptionsTransactionController.php',
@@ -6894,10 +6899,15 @@ phutil_register_library_map(array(
     'PhabricatorSubscribedToObjectEdgeType' => 'PhabricatorEdgeType',
     'PhabricatorSubscribersQuery' => 'PhabricatorQuery',
     'PhabricatorSubscriptionTriggerClock' => 'PhabricatorTriggerClock',
+    'PhabricatorSubscriptionsAddSelfHeraldAction' => 'PhabricatorSubscriptionsHeraldAction',
+    'PhabricatorSubscriptionsAddSubscribersHeraldAction' => 'PhabricatorSubscriptionsHeraldAction',
     'PhabricatorSubscriptionsApplication' => 'PhabricatorApplication',
     'PhabricatorSubscriptionsEditController' => 'PhabricatorController',
     'PhabricatorSubscriptionsEditor' => 'PhabricatorEditor',
+    'PhabricatorSubscriptionsHeraldAction' => 'HeraldAction',
     'PhabricatorSubscriptionsListController' => 'PhabricatorController',
+    'PhabricatorSubscriptionsRemoveSelfHeraldAction' => 'PhabricatorSubscriptionsHeraldAction',
+    'PhabricatorSubscriptionsRemoveSubscribersHeraldAction' => 'PhabricatorSubscriptionsHeraldAction',
     'PhabricatorSubscriptionsSubscribeEmailCommand' => 'MetaMTAEmailTransactionCommand',
     'PhabricatorSubscriptionsSubscribersPolicyRule' => 'PhabricatorPolicyRule',
     'PhabricatorSubscriptionsTransactionController' => 'PhabricatorController',

src/applications/differential/herald/HeraldDifferentialRevisionAdapter.php

@@ -162,8 +162,6 @@ final class HeraldDifferentialRevisionAdapter
       case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL:
         return array_merge(
           array(
-            self::ACTION_ADD_CC,
-            self::ACTION_REMOVE_CC,
             self::ACTION_EMAIL,
             self::ACTION_ADD_REVIEWERS,
             self::ACTION_ADD_BLOCKING_REVIEWERS,
@@ -174,8 +172,6 @@ final class HeraldDifferentialRevisionAdapter
       case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL:
         return array_merge(
           array(
-            self::ACTION_ADD_CC,
-            self::ACTION_REMOVE_CC,
             self::ACTION_EMAIL,
             self::ACTION_ADD_REVIEWERS,
             self::ACTION_ADD_BLOCKING_REVIEWERS,

src/applications/diffusion/herald/HeraldCommitAdapter.php

@@ -89,8 +89,6 @@ final class HeraldCommitAdapter extends HeraldAdapter {
       case HeraldRuleTypeConfig::RULE_TYPE_OBJECT:
         return array_merge(
           array(
-            self::ACTION_ADD_CC,
-            self::ACTION_REMOVE_CC,
             self::ACTION_EMAIL,
             self::ACTION_AUDIT,
             self::ACTION_APPLY_BUILD_PLANS,
@@ -99,8 +97,6 @@ final class HeraldCommitAdapter extends HeraldAdapter {
       case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL:
         return array_merge(
           array(
-            self::ACTION_ADD_CC,
-            self::ACTION_REMOVE_CC,
             self::ACTION_EMAIL,
             self::ACTION_AUDIT,
           ),

src/applications/herald/action/HeraldAction.php

@@ -41,7 +41,7 @@ abstract class HeraldAction extends Phobject {
         return new HeraldEmptyFieldValue();
       case self::STANDARD_PHID_LIST:
         $tokenizer = id(new HeraldTokenizerFieldValue())
-          ->setKey($this->getHeraldFieldName())
+          ->setKey($this->getHeraldActionName())
           ->setDatasource($this->getDatasource());
 
         $value_map = $this->getDatasourceValueMap();
@@ -56,6 +56,17 @@ abstract class HeraldAction extends Phobject {
   }
 
   public function willSaveActionValue($value) {
+    try {
+      $type = $this->getHeraldActionStandardType();
+    } catch (PhutilMethodNotImplementedException $ex) {
+      return $value;
+    }
+
+    switch ($type) {
+      case self::STANDARD_PHID_LIST:
+        return array_keys($value);
+    }
+
     return $value;
   }
 
@@ -162,4 +173,15 @@ abstract class HeraldAction extends Phobject {
     return idx($map, 'name');
   }
 
+  protected function renderHandleList($phids) {
+    if (!is_array($phids)) {
+      return pht('(Invalid List)');
+    }
+
+    return $this->getViewer()
+      ->renderHandleList($phids)
+      ->setAsInline(true)
+      ->render();
+  }
+
 }

src/applications/herald/adapter/HeraldAdapter.php

@@ -26,8 +26,6 @@ abstract class HeraldAdapter extends Phobject {
   const CONDITION_IS_TRUE         = 'true';
   const CONDITION_IS_FALSE        = 'false';
 
-  const ACTION_ADD_CC       = 'addcc';
-  const ACTION_REMOVE_CC    = 'remcc';
   const ACTION_EMAIL        = 'email';
   const ACTION_AUDIT        = 'audit';
   const ACTION_ASSIGN_TASK  = 'assigntask';
@@ -46,9 +44,9 @@ abstract class HeraldAdapter extends Phobject {
   private $queuedTransactions = array();
   private $emailPHIDs = array();
   private $forcedEmailPHIDs = array();
-  private $unsubscribedPHIDs;
   private $fieldMap;
   private $actionMap;
+  private $edgeCache = array();
 
   public function getEmailPHIDs() {
     return array_values($this->emailPHIDs);
@@ -179,7 +177,7 @@ abstract class HeraldAdapter extends Phobject {
     return $this->queuedTransactions;
   }
 
-  protected function newTransaction() {
+  public function newTransaction() {
     $object = $this->newObject();
 
     if (!($object instanceof PhabricatorApplicationTransactionInterface)) {
@@ -723,8 +721,6 @@ abstract class HeraldAdapter extends Phobject {
       case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL:
       case HeraldRuleTypeConfig::RULE_TYPE_OBJECT:
         $standard = array(
-          self::ACTION_ADD_CC       => pht('Add Subscribers'),
-          self::ACTION_REMOVE_CC    => pht('Remove Subscribers'),
           self::ACTION_EMAIL        => pht('Send an email to'),
           self::ACTION_AUDIT        => pht('Trigger an Audit by'),
           self::ACTION_ASSIGN_TASK  => pht('Assign task to'),
@@ -739,8 +735,6 @@ abstract class HeraldAdapter extends Phobject {
         break;
       case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL:
         $standard = array(
-          self::ACTION_ADD_CC       => pht('Add me as a subscriber'),
-          self::ACTION_REMOVE_CC    => pht('Remove me as a subscriber'),
           self::ACTION_EMAIL        => pht('Send me an email'),
           self::ACTION_AUDIT        => pht('Trigger an Audit by me'),
           self::ACTION_ASSIGN_TASK  => pht('Assign task to me'),
@@ -786,8 +780,6 @@ abstract class HeraldAdapter extends Phobject {
     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:
         case self::ACTION_ASSIGN_TASK:
         case self::ACTION_ADD_REVIEWERS:
@@ -828,8 +820,6 @@ abstract class HeraldAdapter extends Phobject {
 
     if ($is_personal) {
       switch ($action) {
-        case self::ACTION_ADD_CC:
-        case self::ACTION_REMOVE_CC:
         case self::ACTION_EMAIL:
         case self::ACTION_AUDIT:
         case self::ACTION_ASSIGN_TASK:
@@ -843,8 +833,6 @@ abstract class HeraldAdapter extends Phobject {
       }
     } else {
       switch ($action) {
-        case self::ACTION_ADD_CC:
-        case self::ACTION_REMOVE_CC:
         case self::ACTION_EMAIL:
           return $this->buildTokenizerFieldValue(
             new PhabricatorMetaMTAMailableDatasource());
@@ -1048,17 +1036,24 @@ abstract class HeraldAdapter extends Phobject {
     PhabricatorUser $viewer) {
 
     $field_type = $condition->getFieldName();
+    $field = $this->getFieldImplementation($field_type);
 
-    $default = pht('(Unknown Field "%s")', $field_type);
+    if (!$field) {
+      return pht('Unknown Field: "%s"', $field_type);
+    }
 
-    $field_name = idx($this->getFieldNameMap(), $field_type, $default);
+    $field_name = $field->getHeraldFieldName();
 
     $condition_type = $condition->getFieldCondition();
     $condition_name = idx($this->getConditionNameMap(), $condition_type);
 
     $value = $this->renderConditionValueAsText($condition, $handles, $viewer);
 
-    return hsprintf('    %s %s %s', $field_name, $condition_name, $value);
+    return array(
+      $field_name,
+      $condition_name,
+      $value,
+    );
   }
 
   private function renderActionAsText(
@@ -1068,8 +1063,10 @@ abstract class HeraldAdapter extends Phobject {
 
     $impl = $this->getActionImplementation($action->getAction());
     if ($impl) {
+      $impl->setViewer($viewer);
+
       $value = $action->getTarget();
-      return $impl->renderActionDescription($viewer, $value);
+      return $impl->renderActionDescription($value);
     }
 
     $rule_global = HeraldRuleTypeConfig::RULE_TYPE_GLOBAL;
@@ -1180,14 +1177,16 @@ abstract class HeraldAdapter extends Phobject {
    */
   protected function applyStandardEffect(HeraldEffect $effect) {
     $action = $effect->getAction();
+    $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);
       }
+    }
 
-    $rule_type = $effect->getRule()->getRuleType();
     $supported = $this->getActions($rule_type);
     $supported = array_fuse($supported);
     if (empty($supported[$action])) {
@@ -1205,9 +1204,6 @@ abstract class HeraldAdapter extends Phobject {
       case self::ACTION_ADD_PROJECTS:
       case self::ACTION_REMOVE_PROJECTS:
         return $this->applyProjectsEffect($effect);
-      case self::ACTION_ADD_CC:
-      case self::ACTION_REMOVE_CC:
-        return $this->applySubscribersEffect($effect);
       case self::ACTION_EMAIL:
         return $this->applyEmailEffect($effect);
       default:
@@ -1257,96 +1253,6 @@ abstract class HeraldAdapter extends Phobject {
       pht('Added projects.'));
   }
 
-  /**
-   * @task apply
-   */
-  private function applySubscribersEffect(HeraldEffect $effect) {
-    if ($effect->getAction() == self::ACTION_ADD_CC) {
-      $kind = '+';
-      $is_add = true;
-    } else {
-      $kind = '-';
-      $is_add = false;
-    }
-
-    $subscriber_phids = array_fuse($effect->getTarget());
-    if (!$subscriber_phids) {
-      return new HeraldApplyTranscript(
-        $effect,
-        false,
-        pht('This action lists no users or objects to affect.'));
-    }
-
-    // The "Add Subscribers" rule only adds subscribers who haven't previously
-    // unsubscribed from the object explicitly. Filter these subscribers out
-    // before continuing.
-    $unsubscribed = array();
-    if ($is_add) {
-      if ($this->unsubscribedPHIDs === null) {
-        $this->unsubscribedPHIDs = PhabricatorEdgeQuery::loadDestinationPHIDs(
-          $this->getObject()->getPHID(),
-          PhabricatorObjectHasUnsubscriberEdgeType::EDGECONST);
-      }
-
-      foreach ($this->unsubscribedPHIDs as $phid) {
-        if (isset($subscriber_phids[$phid])) {
-          $unsubscribed[$phid] = $phid;
-          unset($subscriber_phids[$phid]);
-        }
-      }
-    }
-
-    if (!$subscriber_phids) {
-      return new HeraldApplyTranscript(
-        $effect,
-        false,
-        pht('All targets have previously unsubscribed explicitly.'));
-    }
-
-    // Filter out PHIDs which aren't valid subscribers. Lower levels of the
-    // stack will fail loudly if we try to add subscribers with invalid PHIDs
-    // or unknown PHID types, so drop them here.
-    $invalid = array();
-    foreach ($subscriber_phids as $phid) {
-      $type = phid_get_type($phid);
-      switch ($type) {
-        case PhabricatorPeopleUserPHIDType::TYPECONST:
-        case PhabricatorProjectProjectPHIDType::TYPECONST:
-          break;
-        default:
-          $invalid[$phid] = $phid;
-          unset($subscriber_phids[$phid]);
-          break;
-      }
-    }
-
-    if (!$subscriber_phids) {
-      return new HeraldApplyTranscript(
-        $effect,
-        false,
-        pht('All targets are invalid as subscribers.'));
-    }
-
-    $xaction = $this->newTransaction()
-      ->setTransactionType(PhabricatorTransactions::TYPE_SUBSCRIBERS)
-      ->setNewValue(
-        array(
-          $kind => $subscriber_phids,
-        ));
-
-    $this->queueTransaction($xaction);
-
-    // TODO: We could be more detailed about this, but doing it meaningfully
-    // probably requires substantial changes to how transactions are rendered
-    // first.
-    if ($is_add) {
-      $message = pht('Subscribed targets.');
-    } else {
-      $message = pht('Unsubscribed targets.');
-    }
-
-    return new HeraldApplyTranscript($effect, true, $message);
-  }
 
   /**
    * @task apply
@@ -1370,5 +1276,15 @@ abstract class HeraldAdapter extends Phobject {
       pht('Added mailable to mail targets.'));
   }
 
+  public function loadEdgePHIDs($type) {
+    if (!isset($this->edgeCache[$type])) {
+      $phids = PhabricatorEdgeQuery::loadDestinationPHIDs(
+        $this->getObject()->getPHID(),
+        $type);
+
+      $this->edgeCache[$type] = array_fuse($phids);
+    }
+    return $this->edgeCache[$type];
+  }
 
 }

src/applications/maniphest/herald/HeraldManiphestTaskAdapter.php

@@ -72,8 +72,6 @@ final class HeraldManiphestTaskAdapter extends HeraldAdapter {
       case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL:
         return array_merge(
           array(
-            self::ACTION_ADD_CC,
-            self::ACTION_REMOVE_CC,
             self::ACTION_EMAIL,
             self::ACTION_ASSIGN_TASK,
           ),
@@ -81,8 +79,6 @@ final class HeraldManiphestTaskAdapter extends HeraldAdapter {
       case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL:
         return array_merge(
           array(
-            self::ACTION_ADD_CC,
-            self::ACTION_REMOVE_CC,
             self::ACTION_EMAIL,
             self::ACTION_ASSIGN_TASK,
           ),

src/applications/pholio/herald/HeraldPholioMockAdapter.php

@@ -47,25 +47,6 @@ final class HeraldPholioMockAdapter extends HeraldAdapter {
     }
   }
 
-  public function getActions($rule_type) {
-    switch ($rule_type) {
-      case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL:
-        return array_merge(
-          array(
-            self::ACTION_ADD_CC,
-            self::ACTION_REMOVE_CC,
-          ),
-          parent::getActions($rule_type));
-      case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL:
-        return array_merge(
-          array(
-            self::ACTION_ADD_CC,
-            self::ACTION_REMOVE_CC,
-          ),
-          parent::getActions($rule_type));
-    }
-  }
-
   public function getHeraldName() {
     return 'M'.$this->getMock()->getID();
   }

src/applications/phriction/herald/PhrictionDocumentHeraldAdapter.php

@@ -53,16 +53,12 @@ final class PhrictionDocumentHeraldAdapter extends HeraldAdapter {
       case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL:
         return array_merge(
           array(
-            self::ACTION_ADD_CC,
-            self::ACTION_REMOVE_CC,
             self::ACTION_EMAIL,
           ),
           parent::getActions($rule_type));
       case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL:
         return array_merge(
           array(
-            self::ACTION_ADD_CC,
-            self::ACTION_REMOVE_CC,
             self::ACTION_EMAIL,
           ),
           parent::getActions($rule_type));

src/applications/subscriptions/herald/PhabricatorSubscriptionsAddSelfHeraldAction.php

@@ -0,0 +1,37 @@
+<?php
+
+final class PhabricatorSubscriptionsAddSelfHeraldAction
+  extends PhabricatorSubscriptionsHeraldAction {
+
+  const ACTIONCONST = 'subscribers.self.add';
+
+  public function getHeraldActionName() {
+    return pht('Add me as a subscriber');
+  }
+
+  public function getActionGroupKey() {
+    return HeraldSupportActionGroup::ACTIONGROUPKEY;
+  }
+
+  public function supportsObject($object) {
+    return ($object instanceof PhabricatorSubscribableInterface);
+  }
+
+  public function supportsRuleType($rule_type) {
+    return ($rule_type == HeraldRuleTypeConfig::RULE_TYPE_PERSONAL);
+  }
+
+  public function applyEffect($object, HeraldEffect $effect) {
+    $phid = $effect->getRule()->getAuthorPHID();
+    return $this->applySubscribe(array($phid), $is_add = true);
+  }
+
+  public function getHeraldActionStandardType() {
+    return self::STANDARD_NONE;
+  }
+
+  public function renderActionDescription($value) {
+    return pht('Add rule author as subscriber.');
+  }
+
+}

src/applications/subscriptions/herald/PhabricatorSubscriptionsAddSubscribersHeraldAction.php

@@ -0,0 +1,40 @@
+<?php
+
+final class PhabricatorSubscriptionsAddSubscribersHeraldAction
+  extends PhabricatorSubscriptionsHeraldAction {
+
+  const ACTIONCONST = 'subscribers.add';
+
+  public function getHeraldActionName() {
+    return pht('Add subscribers');
+  }
+
+  public function getActionGroupKey() {
+    return HeraldSupportActionGroup::ACTIONGROUPKEY;
+  }
+
+  public function supportsObject($object) {
+    return ($object instanceof PhabricatorSubscribableInterface);
+  }
+
+  public function supportsRuleType($rule_type) {
+    return ($rule_type != HeraldRuleTypeConfig::RULE_TYPE_PERSONAL);
+  }
+
+  public function applyEffect($object, HeraldEffect $effect) {
+    return $this->applySubscribe($effect->getTarget(), $is_add = true);
+  }
+
+  public function getHeraldActionStandardType() {
+    return self::STANDARD_PHID_LIST;
+  }
+
+  protected function getDatasource() {
+    return new PhabricatorMetaMTAMailableDatasource();
+  }
+
+  public function renderActionDescription($value) {
+    return pht('Add subscribers: %s.', $this->renderHandleList($value));
+  }
+
+}

src/applications/subscriptions/herald/PhabricatorSubscriptionsHeraldAction.php

@@ -0,0 +1,235 @@
+<?php
+
+abstract class PhabricatorSubscriptionsHeraldAction
+  extends HeraldAction {
+
+  const DO_NO_TARGETS = 'do.no-targets';
+  const DO_PREVIOUSLY_UNSUBSCRIBED = 'do.previously-unsubscribed';
+  const DO_INVALID = 'do.invalid';
+  const DO_AUTOSUBSCRIBED = 'do.autosubscribed';
+  const DO_ALREADY_SUBSCRIBED = 'do.already-subscribed';
+  const DO_ALREADY_UNSUBSCRIBED = 'do.already-unsubscribed';
+  const DO_SUBSCRIBED = 'do.subscribed';
+  const DO_UNSUBSCRIBED = 'do.unsubscribed';
+
+  protected function applySubscribe(array $phids, $is_add) {
+    $adapter = $this->getAdapter();
+
+    if ($is_add) {
+      $kind = '+';
+    } else {
+      $kind = '-';
+    }
+
+    $subscriber_phids = array_fuse($phids);
+    if (!$subscriber_phids) {
+      $this->logEffect(self::DO_NO_TARGETS);
+      return;
+    }
+
+    // The "Add Subscribers" rule only adds subscribers who haven't previously
+    // unsubscribed from the object explicitly. Filter these subscribers out
+    // before continuing.
+    if ($is_add) {
+      $unsubscribed = $adapter->loadEdgePHIDs(
+        PhabricatorObjectHasUnsubscriberEdgeType::EDGECONST);
+
+      foreach ($unsubscribed as $phid) {
+        if (isset($subscriber_phids[$phid])) {
+          $unsubscribed[$phid] = $phid;
+          unset($subscriber_phids[$phid]);
+        }
+      }
+
+      if ($unsubscribed) {
+        $this->logEffect(
+          self::DO_PREVIOUSLY_UNSUBSCRIBED,
+          array_values($unsubscribed));
+      }
+    }
+
+    if (!$subscriber_phids) {
+      return;
+    }
+
+    // Filter out PHIDs which aren't valid subscribers. Lower levels of the
+    // stack will fail loudly if we try to add subscribers with invalid PHIDs
+    // or unknown PHID types, so drop them here.
+    $invalid = array();
+    foreach ($subscriber_phids as $phid) {
+      $type = phid_get_type($phid);
+      switch ($type) {
+        case PhabricatorPeopleUserPHIDType::TYPECONST:
+        case PhabricatorProjectProjectPHIDType::TYPECONST:
+          break;
+        default:
+          $invalid[$phid] = $phid;
+          unset($subscriber_phids[$phid]);
+          break;
+      }
+    }
+
+    if ($invalid) {
+      $this->logEffect(self::DO_INVALID, array_values($invalid));
+    }
+
+    if (!$subscriber_phids) {
+      return;
+    }
+
+    $auto = array();
+    $object = $adapter->getObject();
+    foreach ($subscriber_phids as $phid) {
+      if ($object->isAutomaticallySubscribed($phid)) {
+        $auto[$phid] = $phid;
+        unset($subscriber_phids[$phid]);
+      }
+    }
+
+    if ($auto) {
+      $this->logEffect(self::DO_AUTOSUBSCRIBED, array_values($auto));
+    }
+
+    if (!$subscriber_phids) {
+      return;
+    }
+
+    $current = $adapter->loadEdgePHIDs(
+      PhabricatorObjectHasSubscriberEdgeType::EDGECONST);
+
+    if ($is_add) {
+      $already = array();
+      foreach ($subscriber_phids as $phid) {
+        if (isset($current[$phid])) {
+          $already[$phid] = $phid;
+          unset($subscriber_phids[$phid]);
+        }
+      }
+
+      if ($already) {
+        $this->logEffect(self::DO_ALREADY_SUBSCRIBED, $already);
+      }
+    } else {
+      $already = array();
+      foreach ($subscriber_phids as $phid) {
+        if (empty($current[$phid])) {
+          $already[$phid] = $phid;
+          unset($subscriber_phids[$phid]);
+        }
+      }
+
+      if ($already) {
+        $this->logEffect(self::DO_ALREADY_UNSUBSCRIBED, $already);
+      }
+    }
+
+    if (!$subscriber_phids) {
+      return;
+    }
+
+    $xaction = $adapter->newTransaction()
+      ->setTransactionType(PhabricatorTransactions::TYPE_SUBSCRIBERS)
+      ->setNewValue(
+        array(
+          $kind => $subscriber_phids,
+        ));
+
+    $adapter->queueTransaction($xaction);
+
+    if ($is_add) {
+      $this->logEffect(self::DO_SUBSCRIBED, $subscriber_phids);
+    } else {
+      $this->logEffect(self::DO_UNSUBSCRIBED, $subscriber_phids);
+    }
+  }
+
+  protected function getActionEffectMap() {
+    return array(
+      self::DO_NO_TARGETS => array(
+        'icon' => 'fa-ban',
+        'color' => 'grey',
+        'name' => pht('No Targets'),
+      ),
+      self::DO_PREVIOUSLY_UNSUBSCRIBED => array(
+        'icon' => 'fa-minus-circle',
+        'color' => 'grey',
+        'name' => pht('Previously Unsubscribed'),
+      ),
+      self::DO_AUTOSUBSCRIBED => array(
+        'icon' => 'fa-envelope',
+        'color' => 'grey',
+        'name' => pht('Automatically Subscribed'),
+      ),
+      self::DO_INVALID => array(
+        'icon' => 'fa-ban',
+        'color' => 'red',
+        'name' => pht('Invalid Targets'),
+      ),
+      self::DO_ALREADY_SUBSCRIBED => array(
+        'icon' => 'fa-chevron-right',
+        'color' => 'grey',
+        'name' => pht('Already Subscribed'),
+      ),
+      self::DO_ALREADY_UNSUBSCRIBED => array(
+        'icon' => 'fa-chevron-right',
+        'color' => 'grey',
+        'name' => pht('Already Unsubscribed'),
+      ),
+      self::DO_SUBSCRIBED => array(
+        'icon' => 'fa-envelope',
+        'color' => 'green',
+        'name' => pht('Added Subscribers'),
+      ),
+      self::DO_UNSUBSCRIBED => array(
+        'icon' => 'fa-minus-circle',
+        'color' => 'green',
+        'name' => pht('Removed Subscribers'),
+      ),
+    );
+  }
+
+  public function renderActionEffectDescription($type, $data) {
+    switch ($type) {
+      case self::DO_NO_TARGETS:
+        return pht('Rule lists no targets.');
+      case self::DO_PREVIOUSLY_UNSUBSCRIBED:
+        return pht(
+          'Declined to resubscribe %s target(s) because they previously '.
+          'unsubscribed: %s.',
+          new PhutilNumber(count($data)),
+          $this->renderHandleList($data));
+      case self::DO_INVALID:
+        return pht(
+          'Declined to act on %s invalid target(s): %s.',
+          new PhutilNumber(count($data)),
+          $this->renderHandleList($data));
+      case self::DO_AUTOSUBSCRIBED:
+        return pht(
+          '%s automatically subscribed target(s) were not affected: %s.',
+          new PhutilNumber(count($data)),
+          $this->renderHandleList($data));
+      case self::DO_ALREADY_SUBSCRIBED:
+        return pht(
+          '%s target(s) are already subscribed: %s.',
+          new PhutilNumber(count($data)),
+          $this->renderHandleList($data));
+      case self::DO_ALREADY_UNSUBSCRIBED:
+        return pht(
+          '%s target(s) are not subscribed: %s.',
+          new PhutilNumber(count($data)),
+          $this->renderHandleList($data));
+      case self::DO_SUBSCRIBED:
+        return pht(
+          'Added %s subscriber(s): %s.',
+          new PhutilNumber(count($data)),
+          $this->renderHandleList($data));
+      case self::DO_UNSUBSCRIBED:
+        return pht(
+          'Removed %s subscriber(s): %s.',
+          new PhutilNumber(count($data)),
+          $this->renderHandleList($data));
+    }
+  }
+
+
+}

src/applications/subscriptions/herald/PhabricatorSubscriptionsRemoveSelfHeraldAction.php

@@ -0,0 +1,37 @@
+<?php
+
+final class PhabricatorSubscriptionsRemoveSelfHeraldAction
+  extends PhabricatorSubscriptionsHeraldAction {
+
+  const ACTIONCONST = 'subscribers.self.remove';
+
+  public function getHeraldActionName() {
+    return pht('Remove me as a subscriber');
+  }
+
+  public function getActionGroupKey() {
+    return HeraldSupportActionGroup::ACTIONGROUPKEY;
+  }
+
+  public function supportsObject($object) {
+    return ($object instanceof PhabricatorSubscribableInterface);
+  }
+
+  public function supportsRuleType($rule_type) {
+    return ($rule_type == HeraldRuleTypeConfig::RULE_TYPE_PERSONAL);
+  }
+
+  public function applyEffect($object, HeraldEffect $effect) {
+    $phid = $effect->getRule()->getAuthorPHID();
+    return $this->applySubscribe(array($phid), $is_add = false);
+  }
+
+  public function getHeraldActionStandardType() {
+    return self::STANDARD_NONE;
+  }
+
+  public function renderActionDescription($value) {
+    return pht('Remove rule author as subscriber.');
+  }
+
+}

src/applications/subscriptions/herald/PhabricatorSubscriptionsRemoveSubscribersHeraldAction.php

@@ -0,0 +1,40 @@
+<?php
+
+final class PhabricatorSubscriptionsRemoveSubscribersHeraldAction
+  extends PhabricatorSubscriptionsHeraldAction {
+
+  const ACTIONCONST = 'subscribers.remove';
+
+  public function getHeraldActionName() {
+    return pht('Remove subscribers');
+  }
+
+  public function getActionGroupKey() {
+    return HeraldSupportActionGroup::ACTIONGROUPKEY;
+  }
+
+  public function supportsObject($object) {
+    return ($object instanceof PhabricatorSubscribableInterface);
+  }
+
+  public function supportsRuleType($rule_type) {
+    return ($rule_type != HeraldRuleTypeConfig::RULE_TYPE_PERSONAL);
+  }
+
+  public function applyEffect($object, HeraldEffect $effect) {
+    return $this->applySubscribe($effect->getTarget(), $is_add = false);
+  }
+
+  public function getHeraldActionStandardType() {
+    return self::STANDARD_PHID_LIST;
+  }
+
+  protected function getDatasource() {
+    return new PhabricatorMetaMTAMailableDatasource();
+  }
+
+  public function renderActionDescription($value) {
+    return pht('Remove subscribers: %s.', $this->renderHandleList($value));
+  }
+
+}

src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php

@@ -1182,6 +1182,7 @@ final class PhabricatorUSEnglishTranslation
         '%s Task',
         '%s Tasks',
       ),
+
       '%s added %s badge(s) for %s: %s.' => array(
         array(
           '%s added a badge for %s: %3$s.',
@@ -1256,6 +1257,39 @@ final class PhabricatorUSEnglishTranslation
         ),
       ),
 
+      '%s automatically subscribed target(s) were not affected: %s.' => array(
+        'An automatically subscribed target was not affected: %2$s.',
+        'Automatically subscribed targets were not affected: %2$s.',
+      ),
+
+      'Declined to resubscribe %s target(s) because they previously '.
+      'unsubscribed: %s.' => array(
+        'Delined to resubscribe a target because they previously '.
+        'unsubscribed: %2$s.',
+        'Declined to resubscribe targets because they previously '.
+        'unsubscribed: %2$s.',
+      ),
+
+      '%s target(s) are not subscribed: %s.' => array(
+        'A target is not subscribed: %2$s.',
+        'Targets are not subscribed: %2$s.',
+      ),
+
+      '%s target(s) are already subscribed: %s.' => array(
+        'A target is already subscribed: %2$s.',
+        'Targets are already subscribed: %2$s.',
+      ),
+
+      'Added %s subscriber(s): %s.' => array(
+        'Added a subscriber: %2$s.',
+        'Added subscribers: %2$s.',
+      ),
+
+      'Removed %s subscriber(s): %s.' => array(
+        'Removed a subscriber: %2$s.',
+        'Removed subscribers: %2$s.',
+      ),
+
     );
   }