From ee4c7268fe2328a593226b4d32ee9ee94a6c382e Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 8 Jun 2015 10:30:22 -0700 Subject: [PATCH] Make "ADD_CC" and "REMOVE_CC" available as "standard" Herald effects Summary: Ref T8455. Begins consolidating the code for applying these effects: - Makes Add/Remove subscribers a standard effect, and uses it in Pholio. - This includes the "don't re-subscribe users who have explicitly unsubscribed" logic from Differential in the standard effect. I think this rule is always desirable. - This adds new filtering of invalid PHID types to resolve the `arc diff` issue in T8455 once Differential uses this standard effect. - Added "Remove Subscribers" to MockAdapter in order to test that it works. - Relabeled "CC" in Pholio to "Subscribers" for consistency. Test Plan: - Created several rules which add subscribers to (and remove subscribers from) mocks. - Updated mocks, changing properties and adding and removing subscribers. - Observed transactions applying and aggregating properly. - Observed add/remove rules each working correctly. - Observed the "don't re-add unsubscribed users" condition acting on subscribers who had previously been added but explicitly removed/unsubscribed. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T8455 Differential Revision: https://secure.phabricator.com/D13179 --- .../herald/adapter/HeraldAdapter.php | 95 +++++++++++++++++++ .../adapter/HeraldPholioMockAdapter.php | 20 +--- .../controller/PholioMockEditController.php | 2 +- .../pholio/editor/PholioMockEditor.php | 18 ---- 4 files changed, 98 insertions(+), 37 deletions(-) diff --git a/src/applications/herald/adapter/HeraldAdapter.php b/src/applications/herald/adapter/HeraldAdapter.php index 00b357ff07..f50bdf4203 100644 --- a/src/applications/herald/adapter/HeraldAdapter.php +++ b/src/applications/herald/adapter/HeraldAdapter.php @@ -110,6 +110,7 @@ abstract class HeraldAdapter { private $queuedTransactions = array(); private $emailPHIDs = array(); private $forcedEmailPHIDs = array(); + private $unsubscribedPHIDs; public function getEmailPHIDs() { return array_values($this->emailPHIDs); @@ -1555,6 +1556,9 @@ abstract class HeraldAdapter { 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_FLAG: return $this->applyFlagEffect($effect); case self::ACTION_EMAIL: @@ -1615,6 +1619,97 @@ abstract class HeraldAdapter { 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 diff --git a/src/applications/herald/adapter/HeraldPholioMockAdapter.php b/src/applications/herald/adapter/HeraldPholioMockAdapter.php index a96deb8f5b..e4aab69e95 100644 --- a/src/applications/herald/adapter/HeraldPholioMockAdapter.php +++ b/src/applications/herald/adapter/HeraldPholioMockAdapter.php @@ -3,7 +3,6 @@ final class HeraldPholioMockAdapter extends HeraldAdapter { private $mock; - private $ccPHIDs = array(); public function getAdapterApplicationClass() { return 'PhabricatorPholioApplication'; @@ -29,14 +28,6 @@ final class HeraldPholioMockAdapter extends HeraldAdapter { return $this->mock; } - private function setCcPHIDs(array $cc_phids) { - $this->ccPHIDs = $cc_phids; - return $this; - } - public function getCcPHIDs() { - return $this->ccPHIDs; - } - public function getAdapterContentName() { return pht('Pholio Mocks'); } @@ -71,6 +62,7 @@ final class HeraldPholioMockAdapter extends HeraldAdapter { return array_merge( array( self::ACTION_ADD_CC, + self::ACTION_REMOVE_CC, self::ACTION_NOTHING, ), parent::getActions($rule_type)); @@ -78,6 +70,7 @@ final class HeraldPholioMockAdapter extends HeraldAdapter { return array_merge( array( self::ACTION_ADD_CC, + self::ACTION_REMOVE_CC, self::ACTION_FLAG, self::ACTION_NOTHING, ), @@ -117,15 +110,6 @@ final class HeraldPholioMockAdapter extends HeraldAdapter { foreach ($effects as $effect) { $action = $effect->getAction(); switch ($action) { - case self::ACTION_ADD_CC: - foreach ($effect->getTarget() as $phid) { - $this->ccPHIDs[] = $phid; - } - $result[] = new HeraldApplyTranscript( - $effect, - true, - pht('Added address to cc list.')); - break; default: $result[] = $this->applyStandardEffect($effect); break; diff --git a/src/applications/pholio/controller/PholioMockEditController.php b/src/applications/pholio/controller/PholioMockEditController.php index e092a500be..9d18fa32f1 100644 --- a/src/applications/pholio/controller/PholioMockEditController.php +++ b/src/applications/pholio/controller/PholioMockEditController.php @@ -339,7 +339,7 @@ final class PholioMockEditController extends PholioController { ->setDatasource(new PhabricatorProjectDatasource())) ->appendControl( id(new AphrontFormTokenizerControl()) - ->setLabel(pht('CC')) + ->setLabel(pht('Subscribers')) ->setName('cc') ->setValue($v_cc) ->setUser($user) diff --git a/src/applications/pholio/editor/PholioMockEditor.php b/src/applications/pholio/editor/PholioMockEditor.php index 7a70ac5b99..14d3a0af16 100644 --- a/src/applications/pholio/editor/PholioMockEditor.php +++ b/src/applications/pholio/editor/PholioMockEditor.php @@ -453,24 +453,6 @@ final class PholioMockEditor extends PhabricatorApplicationTransactionEditor { ->setMock($object); } - protected function didApplyHeraldRules( - PhabricatorLiskDAO $object, - HeraldAdapter $adapter, - HeraldTranscript $transcript) { - - $xactions = array(); - - $cc_phids = $adapter->getCcPHIDs(); - if ($cc_phids) { - $value = array_fuse($cc_phids); - $xactions[] = id(new PholioTransaction()) - ->setTransactionType(PhabricatorTransactions::TYPE_SUBSCRIBERS) - ->setNewValue(array('+' => $value)); - } - - return $xactions; - } - protected function sortTransactions(array $xactions) { $head = array(); $tail = array();