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

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

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;

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)

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();