Allow "send me an email" in personal rules to punch through settings

Summary:
Fixes T7731. When a user writes a "Send me an email" rule, always try send them an email, even if their notification settings would normally downgrade it to a notification.

In particular, this is stronger than these downgrades:

  - Downgrades due to "self actions";
  - downgrades due to "mail tags".

Test Plan:
  - Wrote various Herald rules with "Send me an email" rules.
  - Used `bin/mail list-outbound` / `show-outbound` to vet generated mail.
  - Mail reacted properly to a variety of conditions (disabled accounts, settings, "send me an email" rule, forced delivery).

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7731

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

src/applications/audit/editor/PhabricatorAuditEditor.php

@@ -6,7 +6,6 @@ final class PhabricatorAuditEditor
   const MAX_FILES_SHOWN_IN_EMAIL = 1000;
 
   private $auditReasonMap = array();
-  private $heraldEmailPHIDs = array();
   private $affectedFiles;
   private $rawPatch;
 
@@ -627,9 +626,6 @@ final class PhabricatorAuditEditor
 
   protected function getMailTo(PhabricatorLiskDAO $object) {
     $phids = array();
-    if ($this->heraldEmailPHIDs) {
-      $phids = $this->heraldEmailPHIDs;
-    }
 
     if ($object->getAuthorPHID()) {
       $phids[] = $object->getAuthorPHID();
@@ -924,8 +920,6 @@ final class PhabricatorAuditEditor
       ->setTransactionType(PhabricatorTransactions::TYPE_SUBSCRIBERS)
       ->setNewValue($add_ccs);
 
-    $this->heraldEmailPHIDs = $adapter->getEmailPHIDs();
-
     HarbormasterBuildable::applyBuildPlans(
       $object->getPHID(),
       $object->getRepository()->getPHID(),

src/applications/differential/editor/DifferentialTransactionEditor.php

@@ -3,7 +3,6 @@
 final class DifferentialTransactionEditor
   extends PhabricatorApplicationTransactionEditor {
 
-  private $heraldEmailPHIDs;
   private $changedPriorToCommitURI;
   private $isCloseByCommit;
   private $repositoryPHIDOverride = false;
@@ -1154,18 +1153,6 @@ final class DifferentialTransactionEditor
     return $phids;
   }
 
-  protected function getMailCC(PhabricatorLiskDAO $object) {
-    $phids = parent::getMailCC($object);
-
-    if ($this->heraldEmailPHIDs) {
-      foreach ($this->heraldEmailPHIDs as $phid) {
-        $phids[] = $phid;
-      }
-    }
-
-    return $phids;
-  }
-
   protected function getMailAction(
     PhabricatorLiskDAO $object,
     array $xactions) {
@@ -1730,9 +1717,6 @@ final class DifferentialTransactionEditor
       }
     }
 
-    // Save extra email PHIDs for later.
-    $this->heraldEmailPHIDs = $adapter->getEmailPHIDs();
-
     // Apply build plans.
     HarbormasterBuildable::applyBuildPlans(
       $adapter->getDiff()->getPHID(),

src/applications/herald/adapter/HeraldAdapter.php

@@ -110,11 +110,16 @@ abstract class HeraldAdapter {
   private $customActions = null;
   private $queuedTransactions = array();
   private $emailPHIDs = array();
+  private $forcedEmailPHIDs = array();
 
   public function getEmailPHIDs() {
     return array_values($this->emailPHIDs);
   }
 
+  public function getForcedEmailPHIDs() {
+    return array_values($this->forcedEmailPHIDs);
+  }
+
   public function getCustomActions() {
     if ($this->customActions === null) {
       $custom_actions = id(new PhutilSymbolLoader())
@@ -1038,6 +1043,14 @@ abstract class HeraldAdapter {
 
     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(

src/applications/maniphest/editor/ManiphestTransactionEditor.php

@@ -3,8 +3,6 @@
 final class ManiphestTransactionEditor
   extends PhabricatorApplicationTransactionEditor {
 
-  private $heraldEmailPHIDs = array();
-
   public function getEditorApplicationClass() {
     return 'PhabricatorManiphestApplication';
   }
@@ -394,20 +392,6 @@ final class ManiphestTransactionEditor
     );
   }
 
-  protected function getMailCC(PhabricatorLiskDAO $object) {
-    $phids = array();
-
-    foreach (parent::getMailCC($object) as $phid) {
-      $phids[] = $phid;
-    }
-
-    foreach ($this->heraldEmailPHIDs as $phid) {
-      $phids[] = $phid;
-    }
-
-    return $phids;
-  }
-
   public function getMailTagsMap() {
     return array(
       ManiphestTransaction::MAILTAG_STATUS =>
@@ -521,7 +505,6 @@ final class ManiphestTransactionEditor
     HeraldAdapter $adapter,
     HeraldTranscript $transcript) {
 
-    $this->heraldEmailPHIDs = $adapter->getEmailPHIDs();
     $xactions = array();
 
     $cc_phids = $adapter->getCcPHIDs();

src/applications/metamta/query/PhabricatorMetaMTAActor.php

@@ -16,6 +16,7 @@ final class PhabricatorMetaMTAActor {
   const REASON_MAILTAGS = 'mailtags';
   const REASON_BOT = 'bot';
   const REASON_FORCE = 'force';
+  const REASON_FORCE_HERALD = 'force-herald';
 
   private $phid;
   private $emailAddress;
@@ -108,6 +109,9 @@ final class PhabricatorMetaMTAActor {
         'Mail which uses forced delivery is usually related to account '.
         'management or authentication. For example, password reset email '.
         'ignores mail preferences.'),
+      self::REASON_FORCE_HERALD => pht(
+        'This recipient was added by a "Send me an Email" rule in Herald, '.
+        'which overrides some delivery settings.'),
     );
 
     return idx($descriptions, $reason, pht('Unknown Reason ("%s")', $reason));

src/applications/metamta/storage/PhabricatorMetaMTAMail.php

@@ -141,6 +141,15 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO {
     return $this->getParam('exclude', array());
   }
 
+  public function setForceHeraldMailRecipientPHIDs(array $force) {
+    $this->setParam('herald-force-recipients', $force);
+    return $this;
+  }
+
+  private function getForceHeraldMailRecipientPHIDs() {
+    return $this->getParam('herald-force-recipients', array());
+  }
+
   public function getTranslation(array $objects) {
     $default_translation = PhabricatorEnv::getEnvConfig('translation.provider');
     $return = null;
@@ -851,7 +860,10 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO {
     }
 
     if ($this->getForceDelivery()) {
-      // If we're forcing delivery, skip all the opt-out checks.
+      // If we're forcing delivery, skip all the opt-out checks. We don't
+      // bother annotating reasoning on the mail in this case because it should
+      // always be obvious why the mail hit this rule (e.g., it is a password
+      // reset mail).
       foreach ($actors as $actor) {
         $actor->setDeliverable(PhabricatorMetaMTAActor::REASON_FORCE);
       }
@@ -867,6 +879,22 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO {
       $actor->setUndeliverable(PhabricatorMetaMTAActor::REASON_RESPONSE);
     }
 
+    // Before running more rules, save a list of the actors who were
+    // deliverable before we started running preference-based rules. This stops
+    // us from trying to send mail to disabled users just because a Herald rule
+    // added them, for example.
+    $deliverable = array();
+    foreach ($actors as $phid => $actor) {
+      if ($actor->isDeliverable()) {
+        $deliverable[] = $phid;
+      }
+    }
+
+    // For the rest of the rules, order matters. We're going to run all the
+    // possible rules in order from weakest to strongest, and let the strongest
+    // matching rule win. The weaker rules leave annotations behind which help
+    // users understand why the mail was routed the way it was.
+
     // Exclude the actor if their preferences are set.
     $from_phid = $this->getParam('from');
     $from_actor = idx($actors, $from_phid);
@@ -892,17 +920,6 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO {
       $actor_phids);
     $all_prefs = mpull($all_prefs, null, 'getUserPHID');
 
-    // Exclude recipients who don't want any mail.
-    foreach ($all_prefs as $phid => $prefs) {
-      $exclude = $prefs->getPreference(
-        PhabricatorUserPreferences::PREFERENCE_NO_MAIL,
-        false);
-      if ($exclude) {
-        $actors[$phid]->setUndeliverable(
-          PhabricatorMetaMTAActor::REASON_MAIL_DISABLED);
-      }
-    }
-
     $value_email = PhabricatorUserPreferences::MAILTAG_PREFERENCE_EMAIL;
 
     // Exclude all recipients who have set preferences to not receive this type
@@ -932,6 +949,33 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO {
       }
     }
 
+    // If recipients were initially deliverable and were added by "Send me an
+    // email" Herald rules, annotate them as such and make them deliverable
+    // again, overriding any changes made by the  "self mail" and "mail tags"
+    // settings.
+    $force_recipients = $this->getForceHeraldMailRecipientPHIDs();
+    $force_recipients = array_fuse($force_recipients);
+    if ($force_recipients) {
+      foreach ($deliverable as $phid) {
+        if (isset($force_recipients[$phid])) {
+          $actors[$phid]->setDeliverable(
+            PhabricatorMetaMTAActor::REASON_FORCE_HERALD);
+        }
+      }
+    }
+
+    // Exclude recipients who don't want any mail. This rule is very strong
+    // and runs last.
+    foreach ($all_prefs as $phid => $prefs) {
+      $exclude = $prefs->getPreference(
+        PhabricatorUserPreferences::PREFERENCE_NO_MAIL,
+        false);
+      if ($exclude) {
+        $actors[$phid]->setUndeliverable(
+          PhabricatorMetaMTAActor::REASON_MAIL_DISABLED);
+      }
+    }
+
     return $actors;
   }
 

src/applications/phriction/editor/PhrictionTransactionEditor.php

@@ -13,7 +13,6 @@ final class PhrictionTransactionEditor
   private $skipAncestorCheck;
   private $contentVersion;
   private $processContentVersionError = true;
-  private $heraldEmailPHIDs = array();
 
   public function setDescription($description) {
     $this->description = $description;
@@ -369,16 +368,6 @@ final class PhrictionTransactionEditor
     );
   }
 
-  protected function getMailCC(PhabricatorLiskDAO $object) {
-    $phids = array();
-
-    foreach ($this->heraldEmailPHIDs as $phid) {
-      $phids[] = $phid;
-    }
-
-    return $phids;
-  }
-
   public function getMailTagsMap() {
     return array(
       PhrictionTransaction::MAILTAG_TITLE =>
@@ -801,8 +790,6 @@ final class PhrictionTransactionEditor
         ->setNewValue(array('+' => $value));
     }
 
-    $this->heraldEmailPHIDs = $adapter->getEmailPHIDs();
-
     return $xactions;
   }
 

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php

@@ -1967,8 +1967,15 @@ abstract class PhabricatorApplicationTransactionEditor
       return;
     }
 
-    $email_to = array_filter(array_unique($this->getMailTo($object)));
+    $email_force = array();
-    $email_cc = array_filter(array_unique($this->getMailCC($object)));
+    $email_to = $this->getMailTo($object);
+    $email_cc = $this->getMailCC($object);
+
+    $adapter = $this->getHeraldAdapter();
+    if ($adapter) {
+      $email_cc = array_merge($email_cc, $adapter->getEmailPHIDs());
+      $email_force = $adapter->getForcedEmailPHIDs();
+    }
 
     $phids = array_merge($email_to, $email_cc);
     $handles = id(new PhabricatorHandleQuery())
@@ -1993,6 +2000,7 @@ abstract class PhabricatorApplicationTransactionEditor
       ->setThreadID($this->getMailThreadID($object), $this->getIsNewObject())
       ->setRelatedPHID($object->getPHID())
       ->setExcludeMailRecipientPHIDs($this->getExcludeMailRecipientPHIDs())
+      ->setForceHeraldMailRecipientPHIDs($email_force)
       ->setMailTags($mail_tags)
       ->setIsBulk(true)
       ->setBody($body->render())