Modularize "Send an Email" Herald actions

Summary: Ref T8726. No surprises.

Test Plan:
Created rules using both action variants, applied upgrade, saw rules still work correctly.

{F658842}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: joshuaspence, epriestley

Maniphest Tasks: T8726

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

resources/sql/autopatches/20150724.herald.2.sql

@@ -0,0 +1,13 @@
+UPDATE {$NAMESPACE}_herald.herald_actionrecord a
+  JOIN {$NAMESPACE}_herald.herald_rule r
+  ON a.ruleID = r.id
+  SET a.action = 'email.other'
+  WHERE r.ruleType != 'personal'
+  AND a.action = 'email';
+
+UPDATE {$NAMESPACE}_herald.herald_actionrecord a
+  JOIN {$NAMESPACE}_herald.herald_rule r
+  ON a.ruleID = r.id
+  SET a.action = 'email.self'
+  WHERE r.ruleType = 'personal'
+  AND a.action = 'email';

src/__phutil_library_map__.php

@@ -2274,6 +2274,9 @@ phutil_register_library_map(array(
     'PhabricatorMetaMTADAO' => 'applications/metamta/storage/PhabricatorMetaMTADAO.php',
     'PhabricatorMetaMTAEmailBodyParser' => 'applications/metamta/parser/PhabricatorMetaMTAEmailBodyParser.php',
     'PhabricatorMetaMTAEmailBodyParserTestCase' => 'applications/metamta/parser/__tests__/PhabricatorMetaMTAEmailBodyParserTestCase.php',
+    'PhabricatorMetaMTAEmailHeraldAction' => 'applications/metamta/herald/PhabricatorMetaMTAEmailHeraldAction.php',
+    'PhabricatorMetaMTAEmailOthersHeraldAction' => 'applications/metamta/herald/PhabricatorMetaMTAEmailOthersHeraldAction.php',
+    'PhabricatorMetaMTAEmailSelfHeraldAction' => 'applications/metamta/herald/PhabricatorMetaMTAEmailSelfHeraldAction.php',
     'PhabricatorMetaMTAErrorMailAction' => 'applications/metamta/action/PhabricatorMetaMTAErrorMailAction.php',
     'PhabricatorMetaMTAMail' => 'applications/metamta/storage/PhabricatorMetaMTAMail.php',
     'PhabricatorMetaMTAMailBody' => 'applications/metamta/view/PhabricatorMetaMTAMailBody.php',
@@ -6170,6 +6173,9 @@ phutil_register_library_map(array(
     'PhabricatorMetaMTADAO' => 'PhabricatorLiskDAO',
     'PhabricatorMetaMTAEmailBodyParser' => 'Phobject',
     'PhabricatorMetaMTAEmailBodyParserTestCase' => 'PhabricatorTestCase',
+    'PhabricatorMetaMTAEmailHeraldAction' => 'HeraldAction',
+    'PhabricatorMetaMTAEmailOthersHeraldAction' => 'PhabricatorMetaMTAEmailHeraldAction',
+    'PhabricatorMetaMTAEmailSelfHeraldAction' => 'PhabricatorMetaMTAEmailHeraldAction',
     'PhabricatorMetaMTAErrorMailAction' => 'PhabricatorSystemAction',
     'PhabricatorMetaMTAMail' => array(
       'PhabricatorMetaMTADAO',

src/applications/differential/herald/HeraldDifferentialRevisionAdapter.php

@@ -162,7 +162,6 @@ final class HeraldDifferentialRevisionAdapter
       case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL:
         return array_merge(
           array(
-            self::ACTION_EMAIL,
             self::ACTION_ADD_REVIEWERS,
             self::ACTION_ADD_BLOCKING_REVIEWERS,
             self::ACTION_APPLY_BUILD_PLANS,
@@ -172,7 +171,6 @@ final class HeraldDifferentialRevisionAdapter
       case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL:
         return array_merge(
           array(
-            self::ACTION_EMAIL,
             self::ACTION_ADD_REVIEWERS,
             self::ACTION_ADD_BLOCKING_REVIEWERS,
           ),

src/applications/diffusion/herald/HeraldCommitAdapter.php

@@ -89,7 +89,6 @@ final class HeraldCommitAdapter extends HeraldAdapter {
       case HeraldRuleTypeConfig::RULE_TYPE_OBJECT:
         return array_merge(
           array(
-            self::ACTION_EMAIL,
             self::ACTION_AUDIT,
             self::ACTION_APPLY_BUILD_PLANS,
           ),
@@ -97,7 +96,6 @@ final class HeraldCommitAdapter extends HeraldAdapter {
       case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL:
         return array_merge(
           array(
-            self::ACTION_EMAIL,
             self::ACTION_AUDIT,
           ),
           parent::getActions($rule_type));

src/applications/diffusion/herald/HeraldPreCommitAdapter.php

@@ -80,13 +80,11 @@ abstract class HeraldPreCommitAdapter extends HeraldAdapter {
         return array_merge(
           array(
             self::ACTION_BLOCK,
-            self::ACTION_EMAIL,
           ),
           parent::getActions($rule_type));
       case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL:
         return array_merge(
           array(
-            self::ACTION_EMAIL,
           ),
           parent::getActions($rule_type));
     }

src/applications/herald/adapter/HeraldAdapter.php

@@ -26,7 +26,6 @@ abstract class HeraldAdapter extends Phobject {
   const CONDITION_IS_TRUE         = 'true';
   const CONDITION_IS_FALSE        = 'false';
 
-  const ACTION_EMAIL        = 'email';
   const ACTION_AUDIT        = 'audit';
   const ACTION_ASSIGN_TASK  = 'assigntask';
   const ACTION_ADD_PROJECTS = 'addprojects';
@@ -56,6 +55,14 @@ abstract class HeraldAdapter extends Phobject {
     return array_values($this->forcedEmailPHIDs);
   }
 
+  public function addEmailPHID($phid, $force) {
+    $this->emailPHIDs[$phid] = $phid;
+    if ($force) {
+      $this->forcedEmailPHIDs[$phid] = $phid;
+    }
+    return $this;
+  }
+
   public function getCustomActions() {
     if ($this->customActions === null) {
       $custom_actions = id(new PhutilSymbolLoader())
@@ -721,7 +728,6 @@ abstract class HeraldAdapter extends Phobject {
       case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL:
       case HeraldRuleTypeConfig::RULE_TYPE_OBJECT:
         $standard = array(
-          self::ACTION_EMAIL        => pht('Send an email to'),
           self::ACTION_AUDIT        => pht('Trigger an Audit by'),
           self::ACTION_ASSIGN_TASK  => pht('Assign task to'),
           self::ACTION_ADD_PROJECTS => pht('Add projects'),
@@ -735,7 +741,6 @@ abstract class HeraldAdapter extends Phobject {
         break;
       case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL:
         $standard = array(
-          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'),
           self::ACTION_ADD_REVIEWERS => pht('Add me as a reviewer'),
@@ -779,7 +784,6 @@ abstract class HeraldAdapter extends Phobject {
     $rule_type = $rule->getRuleType();
     if ($rule_type == HeraldRuleTypeConfig::RULE_TYPE_PERSONAL) {
       switch ($action->getAction()) {
-        case self::ACTION_EMAIL:
         case self::ACTION_AUDIT:
         case self::ACTION_ASSIGN_TASK:
         case self::ACTION_ADD_REVIEWERS:
@@ -820,7 +824,6 @@ abstract class HeraldAdapter extends Phobject {
 
     if ($is_personal) {
       switch ($action) {
-        case self::ACTION_EMAIL:
         case self::ACTION_AUDIT:
         case self::ACTION_ASSIGN_TASK:
         case self::ACTION_ADD_REVIEWERS:
@@ -833,9 +836,6 @@ abstract class HeraldAdapter extends Phobject {
       }
     } else {
       switch ($action) {
-        case self::ACTION_EMAIL:
-          return $this->buildTokenizerFieldValue(
-            new PhabricatorMetaMTAMailableDatasource());
         case self::ACTION_ADD_PROJECTS:
         case self::ACTION_REMOVE_PROJECTS:
           return $this->buildTokenizerFieldValue(
@@ -1204,8 +1204,6 @@ abstract class HeraldAdapter extends Phobject {
       case self::ACTION_ADD_PROJECTS:
       case self::ACTION_REMOVE_PROJECTS:
         return $this->applyProjectsEffect($effect);
-      case self::ACTION_EMAIL:
-        return $this->applyEmailEffect($effect);
       default:
         break;
     }
@@ -1253,29 +1251,6 @@ abstract class HeraldAdapter extends Phobject {
       pht('Added projects.'));
   }
 
-
-  /**
-   * @task apply
-   */
-  private function applyEmailEffect(HeraldEffect $effect) {
-    foreach ($effect->getTarget() as $phid) {
-      $this->emailPHIDs[$phid] = $phid;
-
-      // If this is a personal rule, we'll force delivery of a real email. This
-      // effect is stronger than notification preferences, so you get an actual
-      // email even if your preferences are set to "Notify" or "Ignore".
-      $rule = $effect->getRule();
-      if ($rule->isPersonalRule()) {
-        $this->forcedEmailPHIDs[$phid] = $phid;
-      }
-    }
-
-    return new HeraldApplyTranscript(
-      $effect,
-      true,
-      pht('Added mailable to mail targets.'));
-  }
-
   public function loadEdgePHIDs($type) {
     if (!isset($this->edgeCache[$type])) {
       $phids = PhabricatorEdgeQuery::loadDestinationPHIDs(

src/applications/maniphest/herald/HeraldManiphestTaskAdapter.php

@@ -72,14 +72,12 @@ final class HeraldManiphestTaskAdapter extends HeraldAdapter {
       case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL:
         return array_merge(
           array(
-            self::ACTION_EMAIL,
             self::ACTION_ASSIGN_TASK,
           ),
           parent::getActions($rule_type));
       case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL:
         return array_merge(
           array(
-            self::ACTION_EMAIL,
             self::ACTION_ASSIGN_TASK,
           ),
           parent::getActions($rule_type));

src/applications/metamta/herald/PhabricatorMetaMTAEmailHeraldAction.php

@@ -0,0 +1,69 @@
+<?php
+
+abstract class PhabricatorMetaMTAEmailHeraldAction
+  extends HeraldAction {
+
+  const DO_SEND = 'do.send';
+  const DO_FORCE = 'do.force';
+
+  public function supportsObject($object) {
+    // NOTE: This implementation lacks generality, but there's no great way to
+    // figure out if something generates email right now.
+
+    if ($object instanceof DifferentialDiff) {
+      return false;
+    }
+
+    return true;
+  }
+
+  public function getActionGroupKey() {
+    return HeraldNotifyActionGroup::ACTIONGROUPKEY;
+  }
+
+  protected function applyEmail(array $phids, $force) {
+    $adapter = $this->getAdapter();
+
+    foreach ($phids as $phid) {
+      $adapter->addEmailPHID($phid, $force);
+    }
+
+    if ($force) {
+      $this->logEffect(self::DO_FORCE, $phids);
+    } else {
+      $this->logEffect(self::DO_SEND, $phids);
+    }
+  }
+
+  protected function getActionEffectMap() {
+    return array(
+      self::DO_SEND => array(
+        'icon' => 'fa-envelope',
+        'color' => 'green',
+        'name' => pht('Sent Mail'),
+      ),
+      self::DO_FORCE => array(
+        'icon' => 'fa-envelope',
+        'color' => 'blue',
+        'name' => pht('Forced Mail'),
+      ),
+    );
+  }
+
+  public function renderActionEffectDescription($type, $data) {
+    switch ($type) {
+      case self::DO_SEND:
+        return pht(
+          'Queued email to be delivered to %s target(s): %s.',
+          new PhutilNumber(count($data)),
+          $this->renderHandleList($data));
+      case self::DO_FORCE:
+        return pht(
+          'Queued email to be delivered to %s target(s), ignoring their '.
+          'notification preferences: %s.',
+          new PhutilNumber(count($data)),
+          $this->renderHandleList($data));
+    }
+  }
+
+}

src/applications/metamta/herald/PhabricatorMetaMTAEmailOthersHeraldAction.php

@@ -0,0 +1,32 @@
+<?php
+
+final class PhabricatorMetaMTAEmailOthersHeraldAction
+  extends PhabricatorMetaMTAEmailHeraldAction {
+
+  const ACTIONCONST = 'email.other';
+
+  public function getHeraldActionName() {
+    return pht('Send an email to');
+  }
+
+  public function supportsRuleType($rule_type) {
+    return ($rule_type != HeraldRuleTypeConfig::RULE_TYPE_PERSONAL);
+  }
+
+  public function applyEffect($object, HeraldEffect $effect) {
+    return $this->applyEmail($effect->getTarget(), $force = false);
+  }
+
+  public function getHeraldActionStandardType() {
+    return self::STANDARD_PHID_LIST;
+  }
+
+  protected function getDatasource() {
+    return new PhabricatorMetaMTAMailableDatasource();
+  }
+
+  public function renderActionDescription($value) {
+    return pht('Send an email to: %s.', $this->renderHandleList($value));
+  }
+
+}

src/applications/metamta/herald/PhabricatorMetaMTAEmailSelfHeraldAction.php

@@ -0,0 +1,34 @@
+<?php
+
+final class PhabricatorMetaMTAEmailSelfHeraldAction
+  extends PhabricatorMetaMTAEmailHeraldAction {
+
+  const ACTIONCONST = 'email.self';
+
+  public function getHeraldActionName() {
+    return pht('Send me an email');
+  }
+
+  public function supportsRuleType($rule_type) {
+    return ($rule_type == HeraldRuleTypeConfig::RULE_TYPE_PERSONAL);
+  }
+
+  public function applyEffect($object, HeraldEffect $effect) {
+    $phid = $effect->getRule()->getAuthorPHID();
+
+    // For personal rules, we'll force delivery of a real email. This effect
+    // is stronger than notification preferences, so you get an actual email
+    // even if your preferences are set to "Notify" or "Ignore".
+
+    return $this->applyEmail(array($phid), $force = true);
+  }
+
+  public function getHeraldActionStandardType() {
+    return self::STANDARD_NONE;
+  }
+
+  public function renderActionDescription($value) {
+    return pht('Send an email to rule author.');
+  }
+
+}

src/applications/phriction/herald/PhrictionDocumentHeraldAdapter.php

@@ -48,24 +48,6 @@ final class PhrictionDocumentHeraldAdapter extends HeraldAdapter {
     }
   }
 
-  public function getActions($rule_type) {
-    switch ($rule_type) {
-      case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL:
-        return array_merge(
-          array(
-            self::ACTION_EMAIL,
-          ),
-          parent::getActions($rule_type));
-      case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL:
-        return array_merge(
-          array(
-            self::ACTION_EMAIL,
-          ),
-          parent::getActions($rule_type));
-    }
-  }
-
-
   public function getHeraldName() {
     return pht('Wiki Document %d', $this->getDocument()->getID());
   }

src/applications/subscriptions/herald/PhabricatorSubscriptionsAddSelfHeraldAction.php

@@ -9,14 +9,6 @@ final class PhabricatorSubscriptionsAddSelfHeraldAction
     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);
   }

src/applications/subscriptions/herald/PhabricatorSubscriptionsAddSubscribersHeraldAction.php

@@ -9,14 +9,6 @@ final class PhabricatorSubscriptionsAddSubscribersHeraldAction
     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);
   }

src/applications/subscriptions/herald/PhabricatorSubscriptionsHeraldAction.php

@@ -12,6 +12,14 @@ abstract class PhabricatorSubscriptionsHeraldAction
   const DO_SUBSCRIBED = 'do.subscribed';
   const DO_UNSUBSCRIBED = 'do.unsubscribed';
 
+  public function getActionGroupKey() {
+    return HeraldSupportActionGroup::ACTIONGROUPKEY;
+  }
+
+  public function supportsObject($object) {
+    return ($object instanceof PhabricatorSubscribableInterface);
+  }
+
   protected function applySubscribe(array $phids, $is_add) {
     $adapter = $this->getAdapter();
 

src/applications/subscriptions/herald/PhabricatorSubscriptionsRemoveSelfHeraldAction.php

@@ -9,14 +9,6 @@ final class PhabricatorSubscriptionsRemoveSelfHeraldAction
     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);
   }

src/applications/subscriptions/herald/PhabricatorSubscriptionsRemoveSubscribersHeraldAction.php

@@ -9,14 +9,6 @@ final class PhabricatorSubscriptionsRemoveSubscribersHeraldAction
     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);
   }

src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php

@@ -1290,6 +1290,19 @@ final class PhabricatorUSEnglishTranslation
         'Removed subscribers: %2$s.',
       ),
 
+      'Queued email to be delivered to %s target(s): %s.' => array(
+        'Queued email to be delivered to target: %2$s.',
+        'Queued email to be delivered to targets: %2$s.',
+      ),
+
+      'Queued email to be delivered to %s target(s), ignoring their '.
+      'notification preferences: %s.' => array(
+        'Queued email to be delivered to target, ignoring notification '.
+        'preferences: %2$s.',
+        'Queued email to be delivered to targets, ignoring notification '.
+        'preferences: %2$s.',
+      ),
+
     );
   }