Share target filtering code in HeraldAction

Ref T8726. This shares some target filtering code with the base class.

src/applications/differential/herald/DifferentialReviewersAddBlockingReviewersHeraldAction.php

@@ -30,4 +30,3 @@ final class DifferentialReviewersAddBlockingReviewersHeraldAction
   }
 
 }
-

src/applications/differential/herald/DifferentialReviewersAddBlockingSelfHeraldAction.php

@@ -27,4 +27,3 @@ final class DifferentialReviewersAddBlockingSelfHeraldAction
   }
 
 }
-

src/applications/differential/herald/DifferentialReviewersAddReviewersHeraldAction.php

@@ -30,4 +30,3 @@ final class DifferentialReviewersAddReviewersHeraldAction
   }
 
 }
-

src/applications/differential/herald/DifferentialReviewersAddSelfHeraldAction.php

@@ -27,4 +27,3 @@ final class DifferentialReviewersAddSelfHeraldAction
   }
 
 }
-

src/applications/differential/herald/DifferentialReviewersHeraldAction.php

@@ -3,11 +3,7 @@
 abstract class DifferentialReviewersHeraldAction
   extends HeraldAction {
 
-  const DO_NO_TARGETS = 'do.no-targets';
   const DO_AUTHORS = 'do.authors';
-  const DO_INVALID = 'do.invalid';
-  const DO_ALREADY_REVIEWERS = 'do.already-reviewers';
-  const DO_PERMISSION = 'do.permission';
   const DO_ADD_REVIEWERS = 'do.add-reviewers';
   const DO_ADD_BLOCKING_REVIEWERS = 'do.add-blocking-reviewers';
 
@@ -24,10 +20,6 @@ abstract class DifferentialReviewersHeraldAction
     $object = $adapter->getObject();
 
     $phids = array_fuse($phids);
-    if (!$phids) {
-      $this->logEffect(self::DO_NO_TARGETS);
-      return;
-    }
 
     // Don't try to add revision authors as reviewers.
     $authors = array();
@@ -40,11 +32,10 @@ abstract class DifferentialReviewersHeraldAction
 
     if ($authors) {
       $this->logEffect(self::DO_AUTHORS, $authors);
-    }
-
       if (!$phids) {
         return;
       }
+    }
 
     $reviewers = $object->getReviewerStatus();
     $reviewers = mpull($reviewers, null, 'getReviewerPHID');
@@ -58,7 +49,7 @@ abstract class DifferentialReviewersHeraldAction
     $new_strength = DifferentialReviewerStatus::getStatusStrength(
       $new_status);
 
-    $already = array();
+    $current = array();
     foreach ($phids as $phid) {
       if (!isset($reviewers[$phid])) {
         continue;
@@ -72,65 +63,20 @@ abstract class DifferentialReviewersHeraldAction
         continue;
       }
 
-      $already[] = $phid;
+      $current[] = $phid;
-      unset($phids[$phid]);
     }
 
-    if ($already) {
+    $allowed_types = array(
-      $this->logEffect(self::DO_ALREADY_REVIEWERS, $already);
+      PhabricatorPeopleUserPHIDType::TYPECONST,
-    }
+      PhabricatorProjectProjectPHIDType::TYPECONST,
+    );
 
-    if (!$phids) {
+    $targets = $this->loadStandardTargets($phids, $allowed_types, $current);
+    if (!$targets) {
       return;
     }
 
-    $targets = id(new PhabricatorObjectQuery())
+    $phids = array_fuse(array_keys($targets));
-      ->setViewer(PhabricatorUser::getOmnipotentUser())
-      ->withPHIDs($phids)
-      ->execute();
-    $targets = mpull($targets, null, 'getPHID');
-
-    $invalid = array();
-    foreach ($phids as $phid) {
-      if (empty($targets[$phid])) {
-        $invalid[] = $phid;
-        unset($phids[$phid]);
-      }
-    }
-
-    if ($invalid) {
-      $this->logEffect(self::DO_INVALID, $invalid);
-    }
-
-    if (!$phids) {
-      return;
-    }
-
-    $no_access = array();
-    foreach ($targets as $phid => $target) {
-      if (!($target instanceof PhabricatorUser)) {
-        continue;
-      }
-
-      $can_view = PhabricatorPolicyFilter::hasCapability(
-        $target,
-        $object,
-        PhabricatorPolicyCapability::CAN_VIEW);
-      if ($can_view) {
-        continue;
-      }
-
-      $no_access[] = $phid;
-      unset($phids[$phid]);
-    }
-
-    if ($no_access) {
-      $this->logEffect(self::DO_PERMISSION, $no_access);
-    }
-
-    if (!$phids) {
-      return;
-    }
 
     $value = array();
     foreach ($phids as $phid) {
@@ -162,31 +108,11 @@ abstract class DifferentialReviewersHeraldAction
 
   protected function getActionEffectMap() {
     return array(
-      self::DO_NO_TARGETS => array(
-        'icon' => 'fa-ban',
-        'color' => 'grey',
-        'name' => pht('No Targets'),
-      ),
       self::DO_AUTHORS => array(
         'icon' => 'fa-user',
         'color' => 'grey',
         'name' => pht('Revision Author'),
       ),
-      self::DO_INVALID => array(
-        'icon' => 'fa-ban',
-        'color' => 'red',
-        'name' => pht('Invalid Targets'),
-      ),
-      self::DO_ALREADY_REVIEWERS => array(
-        'icon' => 'fa-user',
-        'color' => 'grey',
-        'name' => pht('Already Reviewers'),
-      ),
-      self::DO_PERMISSION => array(
-        'icon' => 'fa-ban',
-        'color' => 'red',
-        'name' => pht('No Permission'),
-      ),
       self::DO_ADD_REVIEWERS => array(
         'icon' => 'fa-user',
         'color' => 'green',
@@ -202,27 +128,10 @@ abstract class DifferentialReviewersHeraldAction
 
   protected function renderActionEffectDescription($type, $data) {
     switch ($type) {
-      case self::DO_NO_TARGETS:
-        return pht('Rule lists no targets.');
       case self::DO_AUTHORS:
         return pht(
           'Declined to add revision author as reviewer: %s.',
           $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_ALREADY_REVIEWERS:
-        return pht(
-          '%s target(s) were already reviewers: %s.',
-          new PhutilNumber(count($data)),
-          $this->renderHandleList($data));
-      case self::DO_PERMISSION:
-        return pht(
-          '%s target(s) do not have permission to see the revision: %s.',
-          new PhutilNumber(count($data)),
-          $this->renderHandleList($data));
       case self::DO_ADD_REVIEWERS:
         return pht(
           'Added %s reviewer(s): %s.',

src/applications/harbormaster/herald/HarbormasterRunBuildPlansHeraldAction.php

@@ -3,8 +3,6 @@
 final class HarbormasterRunBuildPlansHeraldAction
   extends HeraldAction {
 
-  const DO_NO_TARGETS = 'do.no-targets';
-  const DO_INVALID = 'do.invalid';
   const DO_BUILD = 'do.build';
 
   const ACTIONCONST = 'harbormaster.build';
@@ -21,33 +19,16 @@ final class HarbormasterRunBuildPlansHeraldAction
   protected function applyBuilds(array $phids) {
     $adapter = $this->getAdapter();
 
-    $phids = array_fuse($phids);
+    $allowed_types = array(
-    if (!$phids) {
+      HarbormasterBuildPlanPHIDType::TYPECONST,
-      $this->logEffect(self::DO_NO_TARGETS);
+    );
+
+    $targets = $this->loadStandardTargets($phids, $allowed_types, array());
+    if (!$targets) {
       return;
     }
 
-    $plans = id(new HarbormasterBuildPlanQuery())
+    $phids = array_fuse(array_keys($targets));
-      ->setViewer(PhabricatorUser::getOmnipotentUser())
-      ->withPHIDs($phids)
-      ->execute();
-    $plans = mpull($plans, null, 'getPHID');
-
-    $invalid = array();
-    foreach ($phids as $phid) {
-      if (empty($plans[$phid])) {
-        $invalid[] = $phid;
-        unset($plans[$phid]);
-      }
-    }
-
-    if ($invalid) {
-      $this->logEffect(self::DO_INVALID, $phids);
-    }
-
-    if (!$phids) {
-      return;
-    }
 
     foreach ($phids as $phid) {
       $adapter->queueHarbormasterBuildPlanPHID($phid);
@@ -58,16 +39,6 @@ final class HarbormasterRunBuildPlansHeraldAction
 
   protected function getActionEffectMap() {
     return array(
-      self::DO_NO_TARGETS => array(
-        'icon' => 'fa-ban',
-        'color' => 'grey',
-        'name' => pht('No Targets'),
-      ),
-      self::DO_INVALID => array(
-        'icon' => 'fa-ban',
-        'color' => 'red',
-        'name' => pht('Invalid Targets'),
-      ),
       self::DO_BUILD => array(
         'icon' => 'fa-play',
         'color' => 'green',
@@ -78,13 +49,6 @@ final class HarbormasterRunBuildPlansHeraldAction
 
   protected function renderActionEffectDescription($type, $data) {
     switch ($type) {
-      case self::DO_NO_TARGETS:
-        return pht('Rule lists no targets.');
-      case self::DO_INVALID:
-        return pht(
-          '%s build plan(s) are not valid: %s.',
-          new PhutilNumber(count($data)),
-          $this->renderHandleList($data));
       case self::DO_BUILD:
         return pht(
           'Started %s build(s): %s.',

src/applications/legalpad/herald/LegalpadRequireSignatureHeraldAction.php

@@ -3,9 +3,6 @@
 final class LegalpadRequireSignatureHeraldAction
   extends HeraldAction {
 
-  const DO_NO_TARGETS = 'do.no-targets';
-  const DO_ALREADY_REQUIRED = 'do.already-required';
-  const DO_INVALID = 'do.invalid';
   const DO_SIGNED = 'do.signed';
   const DO_REQUIRED = 'do.required';
 
@@ -24,54 +21,20 @@ final class LegalpadRequireSignatureHeraldAction
 
   protected function applyRequire(array $phids) {
     $adapter = $this->getAdapter();
+
     $edgetype_legal = LegalpadObjectNeedsSignatureEdgeType::EDGECONST;
-
-    $phids = array_fuse($phids);
-
-    if (!$phids) {
-      $this->logEffect(self::DO_NO_TARGETS);
-      return;
-    }
-
     $current = $adapter->loadEdgePHIDs($edgetype_legal);
 
-    $already = array();
+    $allowed_types = array(
-    foreach ($phids as $phid) {
+      PhabricatorLegalpadDocumentPHIDType::TYPECONST,
-      if (isset($current[$phid])) {
+    );
-        $already[] = $phid;
-        unset($phids[$phid]);
-      }
-    }
 
-    if ($already) {
+    $targets = $this->loadStandardTargets($phids, $allowed_types, $current);
-      $this->logEffect(self::DO_ALREADY_REQUIRED, $phids);
+    if (!$targets) {
-    }
-
-    if (!$phids) {
       return;
     }
 
-    $documents = id(new LegalpadDocumentQuery())
+    $phids = array_fuse(array_keys($targets));
-      ->setViewer(PhabricatorUser::getOmnipotentUser())
-      ->withPHIDs($phids)
-      ->execute();
-    $documents = mpull($documents, null, 'getPHID');
-
-    $invalid = array();
-    foreach ($phids as $phid) {
-      if (empty($documents[$phid])) {
-        $invalid[] = $phid;
-        unset($documents[$phid]);
-      }
-    }
-
-    if ($invalid) {
-      $this->logEffect(self::DO_INVALID, $phids);
-    }
-
-    if (!$phids) {
-      return;
-    }
 
     $object = $adapter->getObject();
     $author_phid = $object->getAuthorPHID();
@@ -114,21 +77,6 @@ final class LegalpadRequireSignatureHeraldAction
 
   protected function getActionEffectMap() {
     return array(
-      self::DO_NO_TARGETS => array(
-        'icon' => 'fa-ban',
-        'color' => 'grey',
-        'name' => pht('No Targets'),
-      ),
-      self::DO_INVALID => array(
-        'icon' => 'fa-ban',
-        'color' => 'red',
-        'name' => pht('Invalid Targets'),
-      ),
-      self::DO_ALREADY_REQUIRED => array(
-        'icon' => 'fa-terminal',
-        'color' => 'grey',
-        'name' => pht('Already Required'),
-      ),
       self::DO_SIGNED => array(
         'icon' => 'fa-terminal',
         'color' => 'green',
@@ -144,18 +92,6 @@ final class LegalpadRequireSignatureHeraldAction
 
   protected function renderActionEffectDescription($type, $data) {
     switch ($type) {
-      case self::DO_NO_TARGETS:
-        return pht('Rule lists no targets.');
-      case self::DO_INVALID:
-        return pht(
-          '%s document(s) are not valid: %s.',
-          new PhutilNumber(count($data)),
-          $this->renderHandleList($data));
-      case self::DO_ALREADY_REQUIRED:
-        return pht(
-          '%s document signature(s) are already required: %s.',
-          new PhutilNumber(count($data)),
-          $this->renderHandleList($data));
       case self::DO_SIGNED:
         return pht(
           '%s document(s) are already signed: %s.',

src/applications/maniphest/herald/ManiphestTaskAssignHeraldAction.php

@@ -3,10 +3,6 @@
 abstract class ManiphestTaskAssignHeraldAction
   extends HeraldAction {
 
-  const DO_EMPTY = 'do.send';
-  const DO_ALREADY = 'do.already';
-  const DO_INVALID = 'do.invalid';
-  const DO_PERMISSION = 'do.permission';
   const DO_ASSIGN = 'do.assign';
 
   public function supportsObject($object) {
@@ -18,38 +14,21 @@ abstract class ManiphestTaskAssignHeraldAction
   }
 
   protected function applyAssign(array $phids) {
-    $phid = head($phids);
-
-    if (!$phid) {
-      $this->logEffect(self::DO_EMPTY);
-      return;
-    }
-
     $adapter = $this->getAdapter();
     $object = $adapter->getObject();
 
-    if ($object->getOwnerPHID() == $phid) {
+    $current = array($object->getOwnerPHID());
-      $this->logEffect(self::DO_ALREADY, array($phid));
+
+    $allowed_types = array(
+      PhabricatorPeopleUserPHIDType::TYPECONST,
+    );
+
+    $targets = $this->loadStandardTargets($phids, $allowed_types, $current);
+    if (!$targets) {
       return;
     }
 
-    $user = id(new PhabricatorPeopleQuery())
+    $phid = head_key($targets);
-      ->setViewer(PhabricatorUser::getOmnipotentUser())
-      ->withPHIDs(array($phid))
-      ->executeOne();
-    if (!$user) {
-      $this->logEffect(self::DO_INVALID, array($phid));
-      return;
-    }
-
-    $can_view = PhabricatorPolicyFilter::hasCapability(
-      $user,
-      $object,
-      PhabricatorPolicyCapability::CAN_VIEW);
-    if (!$can_view) {
-      $this->logEffect(self::DO_PERMISSION, array($phid));
-      return;
-    }
 
     $xaction = $adapter->newTransaction()
       ->setTransactionType(ManiphestTransaction::TYPE_OWNER)
@@ -62,26 +41,6 @@ abstract class ManiphestTaskAssignHeraldAction
 
   protected function getActionEffectMap() {
     return array(
-      self::DO_EMPTY => array(
-        'icon' => 'fa-ban',
-        'color' => 'grey',
-        'name' => pht('Empty Action'),
-      ),
-      self::DO_ALREADY => array(
-        'icon' => 'fa-user',
-        'color' => 'grey',
-        'name' => pht('Already Assigned'),
-      ),
-      self::DO_INVALID => array(
-        'icon' => 'fa-ban',
-        'color' => 'red',
-        'name' => pht('Invalid Owner'),
-      ),
-      self::DO_PERMISSION => array(
-        'icon' => 'fa-ban',
-        'color' => 'red',
-        'name' => pht('No Permission'),
-      ),
       self::DO_ASSIGN => array(
         'icon' => 'fa-user',
         'color' => 'green',
@@ -92,20 +51,6 @@ abstract class ManiphestTaskAssignHeraldAction
 
   protected function renderActionEffectDescription($type, $data) {
     switch ($type) {
-      case self::DO_EMPTY:
-        return pht('Action lists no user to assign.');
-      case self::DO_ALREADY:
-        return pht(
-          'User is already task owner: %s.',
-          $this->renderHandleList($data));
-      case self::DO_INVALID:
-        return pht(
-          'User is invalid: %s.',
-          $this->renderHandleList($data));
-      case self::DO_PERMISSION:
-        return pht(
-          'User does not have permission to see task: %s.',
-          $this->renderHandleList($data));
       case self::DO_ASSIGN:
         return pht(
           'Assigned task to: %s.',

src/applications/metamta/herald/PhabricatorMetaMTAEmailHeraldAction.php

@@ -24,6 +24,22 @@ abstract class PhabricatorMetaMTAEmailHeraldAction
   protected function applyEmail(array $phids, $force) {
     $adapter = $this->getAdapter();
 
+    $allowed_types = array(
+      PhabricatorPeopleUserPHIDType::TYPECONST,
+      PhabricatorProjectProjectPHIDType::TYPECONST,
+    );
+
+    // There's no stateful behavior for this action: we always just send an
+    // email.
+    $current = array();
+
+    $targets = $this->loadStandardTargets($phids, $allowed_types, $current);
+    if (!$targets) {
+      return;
+    }
+
+    $phids = array_fuse(array_keys($targets));
+
     foreach ($phids as $phid) {
       $adapter->addEmailPHID($phid, $force);
     }

src/applications/project/herald/PhabricatorProjectHeraldAction.php

@@ -3,10 +3,6 @@
 abstract class PhabricatorProjectHeraldAction
   extends HeraldAction {
 
-  const DO_NO_TARGETS = 'do.no-targets';
-  const DO_INVALID = 'do.invalid';
-  const DO_ALREADY_ASSOCIATED = 'do.already-associated';
-  const DO_ALREADY_UNASSOCIATED = 'do.already-unassociated';
   const DO_ADD_PROJECTS = 'do.add-projects';
   const DO_REMOVE_PROJECTS = 'do.remove-projects';
 
@@ -23,38 +19,24 @@ abstract class PhabricatorProjectHeraldAction
   }
 
   protected function applyProjects(array $phids, $is_add) {
-    $phids = array_fuse($phids);
     $adapter = $this->getAdapter();
 
-    if (!$phids) {
+    $allowed_types = array(
-      $this->logEffect(self::DO_NO_TARGETS);
+      PhabricatorProjectProjectPHIDType::TYPECONST,
+    );
+
+    // Detection of "No Effect" is a bit tricky for this action, so just do it
+    // manually a little later on.
+    $current = array();
+
+    $targets = $this->loadStandardTargets($phids, $allowed_types, $current);
+    if (!$targets) {
       return;
     }
 
-    $projects = id(new PhabricatorProjectQuery())
+    $phids = array_fuse(array_keys($targets));
-      ->setViewer(PhabricatorUser::getOmnipotentUser())
-      ->withPHIDs($phids)
-      ->execute();
-    $projects = mpull($projects, null, 'getPHID');
-
-    $invalid = array();
-    foreach ($phids as $phid) {
-      if (empty($projects[$phid])) {
-        $invalid[] = $phid;
-        unset($phids[$phid]);
-      }
-    }
-
-    if ($invalid) {
-      $this->logEffect(self::DO_INVALID, $invalid);
-    }
-
-    if (!$phids) {
-      return;
-    }
 
     $project_type = PhabricatorProjectObjectHasProjectEdgeType::EDGECONST;
-
     $current = $adapter->loadEdgePHIDs($project_type);
 
     if ($is_add) {
@@ -67,7 +49,7 @@ abstract class PhabricatorProjectHeraldAction
       }
 
       if ($already) {
-        $this->logEffect(self::DO_ALREADY_ASSOCIATED, $already);
+        $this->logEffect(self::DO_STANDARD_NO_EFFECT, $already);
       }
     } else {
       $already = array();
@@ -79,7 +61,7 @@ abstract class PhabricatorProjectHeraldAction
       }
 
       if ($already) {
-        $this->logEffect(self::DO_ALREADY_UNASSOCIATED, $already);
+        $this->logEffect(self::DO_STANDARD_NO_EFFECT, $already);
       }
     }
 
@@ -112,26 +94,6 @@ abstract class PhabricatorProjectHeraldAction
 
   protected function getActionEffectMap() {
     return array(
-      self::DO_NO_TARGETS => array(
-        'icon' => 'fa-ban',
-        'color' => 'grey',
-        'name' => pht('No Targets'),
-      ),
-      self::DO_INVALID => array(
-        'icon' => 'fa-ban',
-        'color' => 'red',
-        'name' => pht('Invalid Targets'),
-      ),
-      self::DO_ALREADY_ASSOCIATED => array(
-        'icon' => 'fa-chevron-right',
-        'color' => 'grey',
-        'name' => pht('Already Associated'),
-      ),
-      self::DO_ALREADY_UNASSOCIATED => array(
-        'icon' => 'fa-chevron-right',
-        'color' => 'grey',
-        'name' => pht('Already Unassociated'),
-      ),
       self::DO_ADD_PROJECTS => array(
         'icon' => 'fa-briefcase',
         'color' => 'green',
@@ -147,23 +109,6 @@ abstract class PhabricatorProjectHeraldAction
 
   protected function renderActionEffectDescription($type, $data) {
     switch ($type) {
-      case self::DO_NO_TARGETS:
-        return pht('Rule lists no projects.');
-      case self::DO_INVALID:
-        return pht(
-          'Declined to act on %s invalid project(s): %s.',
-          new PhutilNumber(count($data)),
-          $this->renderHandleList($data));
-      case self::DO_ALREADY_ASSOCIATED:
-        return pht(
-          '%s project(s) are already associated: %s.',
-          new PhutilNumber(count($data)),
-          $this->renderHandleList($data));
-      case self::DO_ALREADY_UNASSOCIATED:
-        return pht(
-          '%s project(s) are not associated: %s.',
-          new PhutilNumber(count($data)),
-          $this->renderHandleList($data));
       case self::DO_ADD_PROJECTS:
         return pht(
           'Added %s project(s): %s.',

src/applications/subscriptions/herald/PhabricatorSubscriptionsHeraldAction.php

@@ -3,12 +3,8 @@
 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';
 
@@ -23,18 +19,22 @@ abstract class PhabricatorSubscriptionsHeraldAction
   protected function applySubscribe(array $phids, $is_add) {
     $adapter = $this->getAdapter();
 
-    if ($is_add) {
+    $allowed_types = array(
-      $kind = '+';
+      PhabricatorPeopleUserPHIDType::TYPECONST,
-    } else {
+      PhabricatorProjectProjectPHIDType::TYPECONST,
-      $kind = '-';
+    );
-    }
 
-    $subscriber_phids = array_fuse($phids);
+    // Evaluating "No Effect" is a bit tricky for this rule type, so just
-    if (!$subscriber_phids) {
+    // do it manually below.
-      $this->logEffect(self::DO_NO_TARGETS);
+    $current = array();
+
+    $targets = $this->loadStandardTargets($phids, $allowed_types, $current);
+    if (!$targets) {
       return;
     }
 
+    $phids = array_fuse(array_keys($targets));
+
     // The "Add Subscribers" rule only adds subscribers who haven't previously
     // unsubscribed from the object explicitly. Filter these subscribers out
     // before continuing.
@@ -43,9 +43,9 @@ abstract class PhabricatorSubscriptionsHeraldAction
         PhabricatorObjectHasUnsubscriberEdgeType::EDGECONST);
 
       foreach ($unsubscribed as $phid) {
-        if (isset($subscriber_phids[$phid])) {
+        if (isset($phids[$phid])) {
           $unsubscribed[$phid] = $phid;
-          unset($subscriber_phids[$phid]);
+          unset($phids[$phid]);
         }
       }
 
@@ -56,41 +56,16 @@ abstract class PhabricatorSubscriptionsHeraldAction
       }
     }
 
-    if (!$subscriber_phids) {
+    if (!$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) {
+    foreach ($phids as $phid) {
       if ($object->isAutomaticallySubscribed($phid)) {
         $auto[$phid] = $phid;
-        unset($subscriber_phids[$phid]);
+        unset($phids[$phid]);
       }
     }
 
@@ -98,7 +73,7 @@ abstract class PhabricatorSubscriptionsHeraldAction
       $this->logEffect(self::DO_AUTOSUBSCRIBED, array_values($auto));
     }
 
-    if (!$subscriber_phids) {
+    if (!$phids) {
       return;
     }
 
@@ -107,57 +82,58 @@ abstract class PhabricatorSubscriptionsHeraldAction
 
     if ($is_add) {
       $already = array();
-      foreach ($subscriber_phids as $phid) {
+      foreach ($phids as $phid) {
         if (isset($current[$phid])) {
           $already[$phid] = $phid;
-          unset($subscriber_phids[$phid]);
+          unset($phids[$phid]);
         }
       }
 
       if ($already) {
-        $this->logEffect(self::DO_ALREADY_SUBSCRIBED, $already);
+        $this->logEffect(self::DO_STANDARD_NO_EFFECT, $already);
       }
     } else {
       $already = array();
-      foreach ($subscriber_phids as $phid) {
+      foreach ($phids as $phid) {
         if (empty($current[$phid])) {
           $already[$phid] = $phid;
-          unset($subscriber_phids[$phid]);
+          unset($phids[$phid]);
         }
       }
 
       if ($already) {
-        $this->logEffect(self::DO_ALREADY_UNSUBSCRIBED, $already);
+        $this->logEffect(self::DO_STANDARD_NO_EFFECT, $already);
       }
     }
 
-    if (!$subscriber_phids) {
+    if (!$phids) {
       return;
     }
 
+    if ($is_add) {
+      $kind = '+';
+    } else {
+      $kind = '-';
+    }
+
     $xaction = $adapter->newTransaction()
       ->setTransactionType(PhabricatorTransactions::TYPE_SUBSCRIBERS)
       ->setNewValue(
         array(
-          $kind => $subscriber_phids,
+          $kind => $phids,
         ));
 
     $adapter->queueTransaction($xaction);
 
     if ($is_add) {
-      $this->logEffect(self::DO_SUBSCRIBED, $subscriber_phids);
+      $this->logEffect(self::DO_SUBSCRIBED, $phids);
     } else {
-      $this->logEffect(self::DO_UNSUBSCRIBED, $subscriber_phids);
+      $this->logEffect(self::DO_UNSUBSCRIBED, $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',
@@ -168,21 +144,6 @@ abstract class PhabricatorSubscriptionsHeraldAction
         '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',
@@ -198,34 +159,17 @@ abstract class PhabricatorSubscriptionsHeraldAction
 
   protected 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.',

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php

@@ -1706,7 +1706,7 @@ abstract class PhabricatorApplicationTransactionEditor
 
     $lists = array($new_set, $new_add, $new_rem);
     foreach ($lists as $list) {
-      $this->checkEdgeList($list);
+      $this->checkEdgeList($list, $xaction->getMetadataValue('edge:type'));
     }
 
     $result = array();
@@ -1743,7 +1743,7 @@ abstract class PhabricatorApplicationTransactionEditor
     return $result;
   }
 
-  private function checkEdgeList($list) {
+  private function checkEdgeList($list, $edge_type) {
     if (!$list) {
       return;
     }
@@ -1751,16 +1751,18 @@ abstract class PhabricatorApplicationTransactionEditor
       if (phid_get_type($key) === PhabricatorPHIDConstants::PHID_TYPE_UNKNOWN) {
         throw new Exception(
           pht(
-            "Edge transactions must have destination PHIDs as in edge ".
+            'Edge transactions must have destination PHIDs as in edge '.
-            "lists (found key '%s').",
+            'lists (found key "%s" on transaction of type "%s").',
-            $key));
+            $key,
+            $edge_type));
       }
       if (!is_array($item) && $item !== $key) {
         throw new Exception(
           pht(
-            "Edge transactions must have PHIDs or edge specs as values ".
+            'Edge transactions must have PHIDs or edge specs as values '.
-            "(found value '%s').",
+            '(found value "%s" on transaction of type "%s").',
-            $item));
+            $item,
+            $edge_type));
       }
     }
   }

src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php

@@ -1348,6 +1348,16 @@ final class PhabricatorUSEnglishTranslation
         'Added auditors: %2$s.',
       ),
 
+      '%s target(s) do not have permission to see this object: %s.' => array(
+        'A target does not have permission to see this object: %2$s.',
+        'Targets do not have permission to see this object: %2$s.',
+      ),
+
+      'This action has no effect on %s target(s): %s.' => array(
+        'This action has no effect on a target: %2$s.',
+        'This action has no effect on targets: %2$s.',
+      ),
+
     );
   }