diff --git a/src/applications/differential/herald/DifferentialReviewersAddBlockingReviewersHeraldAction.php b/src/applications/differential/herald/DifferentialReviewersAddBlockingReviewersHeraldAction.php index 0ad16ea83b..79a00bc940 100644 --- a/src/applications/differential/herald/DifferentialReviewersAddBlockingReviewersHeraldAction.php +++ b/src/applications/differential/herald/DifferentialReviewersAddBlockingReviewersHeraldAction.php @@ -30,4 +30,3 @@ final class DifferentialReviewersAddBlockingReviewersHeraldAction } } - diff --git a/src/applications/differential/herald/DifferentialReviewersAddBlockingSelfHeraldAction.php b/src/applications/differential/herald/DifferentialReviewersAddBlockingSelfHeraldAction.php index d173e4b814..938f298955 100644 --- a/src/applications/differential/herald/DifferentialReviewersAddBlockingSelfHeraldAction.php +++ b/src/applications/differential/herald/DifferentialReviewersAddBlockingSelfHeraldAction.php @@ -27,4 +27,3 @@ final class DifferentialReviewersAddBlockingSelfHeraldAction } } - diff --git a/src/applications/differential/herald/DifferentialReviewersAddReviewersHeraldAction.php b/src/applications/differential/herald/DifferentialReviewersAddReviewersHeraldAction.php index 9530f903bf..45f209385e 100644 --- a/src/applications/differential/herald/DifferentialReviewersAddReviewersHeraldAction.php +++ b/src/applications/differential/herald/DifferentialReviewersAddReviewersHeraldAction.php @@ -30,4 +30,3 @@ final class DifferentialReviewersAddReviewersHeraldAction } } - diff --git a/src/applications/differential/herald/DifferentialReviewersAddSelfHeraldAction.php b/src/applications/differential/herald/DifferentialReviewersAddSelfHeraldAction.php index de22adbd27..a7e9991901 100644 --- a/src/applications/differential/herald/DifferentialReviewersAddSelfHeraldAction.php +++ b/src/applications/differential/herald/DifferentialReviewersAddSelfHeraldAction.php @@ -27,4 +27,3 @@ final class DifferentialReviewersAddSelfHeraldAction } } - diff --git a/src/applications/differential/herald/DifferentialReviewersHeraldAction.php b/src/applications/differential/herald/DifferentialReviewersHeraldAction.php index 848dfac9ff..c568537f7c 100644 --- a/src/applications/differential/herald/DifferentialReviewersHeraldAction.php +++ b/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,10 +32,9 @@ abstract class DifferentialReviewersHeraldAction if ($authors) { $this->logEffect(self::DO_AUTHORS, $authors); - } - - if (!$phids) { - return; + if (!$phids) { + return; + } } $reviewers = $object->getReviewerStatus(); @@ -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; - unset($phids[$phid]); + $current[] = $phid; } - if ($already) { - $this->logEffect(self::DO_ALREADY_REVIEWERS, $already); - } + $allowed_types = array( + PhabricatorPeopleUserPHIDType::TYPECONST, + PhabricatorProjectProjectPHIDType::TYPECONST, + ); - if (!$phids) { + $targets = $this->loadStandardTargets($phids, $allowed_types, $current); + if (!$targets) { return; } - $targets = id(new PhabricatorObjectQuery()) - ->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; - } + $phids = array_fuse(array_keys($targets)); $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.', diff --git a/src/applications/harbormaster/herald/HarbormasterRunBuildPlansHeraldAction.php b/src/applications/harbormaster/herald/HarbormasterRunBuildPlansHeraldAction.php index 82b9c09316..830598e812 100644 --- a/src/applications/harbormaster/herald/HarbormasterRunBuildPlansHeraldAction.php +++ b/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); - if (!$phids) { - $this->logEffect(self::DO_NO_TARGETS); + $allowed_types = array( + HarbormasterBuildPlanPHIDType::TYPECONST, + ); + + $targets = $this->loadStandardTargets($phids, $allowed_types, array()); + if (!$targets) { return; } - $plans = id(new HarbormasterBuildPlanQuery()) - ->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; - } + $phids = array_fuse(array_keys($targets)); 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.', diff --git a/src/applications/legalpad/herald/LegalpadRequireSignatureHeraldAction.php b/src/applications/legalpad/herald/LegalpadRequireSignatureHeraldAction.php index 9ff4c08b13..22d41449ad 100644 --- a/src/applications/legalpad/herald/LegalpadRequireSignatureHeraldAction.php +++ b/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(); - foreach ($phids as $phid) { - if (isset($current[$phid])) { - $already[] = $phid; - unset($phids[$phid]); - } - } + $allowed_types = array( + PhabricatorLegalpadDocumentPHIDType::TYPECONST, + ); - if ($already) { - $this->logEffect(self::DO_ALREADY_REQUIRED, $phids); - } - - if (!$phids) { + $targets = $this->loadStandardTargets($phids, $allowed_types, $current); + if (!$targets) { return; } - $documents = id(new LegalpadDocumentQuery()) - ->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; - } + $phids = array_fuse(array_keys($targets)); $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.', diff --git a/src/applications/maniphest/herald/ManiphestTaskAssignHeraldAction.php b/src/applications/maniphest/herald/ManiphestTaskAssignHeraldAction.php index 169446f7cf..79e74e7466 100644 --- a/src/applications/maniphest/herald/ManiphestTaskAssignHeraldAction.php +++ b/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) { - $this->logEffect(self::DO_ALREADY, array($phid)); + $current = array($object->getOwnerPHID()); + + $allowed_types = array( + PhabricatorPeopleUserPHIDType::TYPECONST, + ); + + $targets = $this->loadStandardTargets($phids, $allowed_types, $current); + if (!$targets) { return; } - $user = id(new PhabricatorPeopleQuery()) - ->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; - } + $phid = head_key($targets); $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.', diff --git a/src/applications/metamta/herald/PhabricatorMetaMTAEmailHeraldAction.php b/src/applications/metamta/herald/PhabricatorMetaMTAEmailHeraldAction.php index 60e72ce26a..835bde8d4d 100644 --- a/src/applications/metamta/herald/PhabricatorMetaMTAEmailHeraldAction.php +++ b/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); } diff --git a/src/applications/project/herald/PhabricatorProjectHeraldAction.php b/src/applications/project/herald/PhabricatorProjectHeraldAction.php index 2d8c2c9b48..a720ebe5b0 100644 --- a/src/applications/project/herald/PhabricatorProjectHeraldAction.php +++ b/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) { - $this->logEffect(self::DO_NO_TARGETS); + $allowed_types = array( + 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()) - ->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; - } + $phids = array_fuse(array_keys($targets)); $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.', diff --git a/src/applications/subscriptions/herald/PhabricatorSubscriptionsHeraldAction.php b/src/applications/subscriptions/herald/PhabricatorSubscriptionsHeraldAction.php index 5ac859a527..6ec7b6f776 100644 --- a/src/applications/subscriptions/herald/PhabricatorSubscriptionsHeraldAction.php +++ b/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) { - $kind = '+'; - } else { - $kind = '-'; - } + $allowed_types = array( + PhabricatorPeopleUserPHIDType::TYPECONST, + PhabricatorProjectProjectPHIDType::TYPECONST, + ); - $subscriber_phids = array_fuse($phids); - if (!$subscriber_phids) { - $this->logEffect(self::DO_NO_TARGETS); + // Evaluating "No Effect" is a bit tricky for this rule type, so just + // do it manually below. + $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) { - 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) { + if (!$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.', diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index 033fedf805..45782758f8 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/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 ". - "lists (found key '%s').", - $key)); + 'Edge transactions must have destination PHIDs as in edge '. + 'lists (found key "%s" on transaction of type "%s").', + $key, + $edge_type)); } if (!is_array($item) && $item !== $key) { throw new Exception( pht( - "Edge transactions must have PHIDs or edge specs as values ". - "(found value '%s').", - $item)); + 'Edge transactions must have PHIDs or edge specs as values '. + '(found value "%s" on transaction of type "%s").', + $item, + $edge_type)); } } } diff --git a/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php b/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php index 108319139d..8566710cda 100644 --- a/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php +++ b/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.', + ), + ); }