diff --git a/src/applications/audit/editor/PhabricatorAuditEditor.php b/src/applications/audit/editor/PhabricatorAuditEditor.php --- a/src/applications/audit/editor/PhabricatorAuditEditor.php +++ b/src/applications/audit/editor/PhabricatorAuditEditor.php @@ -6,7 +6,6 @@ const MAX_FILES_SHOWN_IN_EMAIL = 1000; private $auditReasonMap = array(); - private $heraldEmailPHIDs = array(); private $affectedFiles; private $rawPatch; @@ -627,9 +626,6 @@ protected function getMailTo(PhabricatorLiskDAO $object) { $phids = array(); - if ($this->heraldEmailPHIDs) { - $phids = $this->heraldEmailPHIDs; - } if ($object->getAuthorPHID()) { $phids[] = $object->getAuthorPHID(); @@ -924,8 +920,6 @@ ->setTransactionType(PhabricatorTransactions::TYPE_SUBSCRIBERS) ->setNewValue($add_ccs); - $this->heraldEmailPHIDs = $adapter->getEmailPHIDs(); - HarbormasterBuildable::applyBuildPlans( $object->getPHID(), $object->getRepository()->getPHID(), diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/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 @@ 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 @@ } } - // Save extra email PHIDs for later. - $this->heraldEmailPHIDs = $adapter->getEmailPHIDs(); - // Apply build plans. HarbormasterBuildable::applyBuildPlans( $adapter->getDiff()->getPHID(), diff --git a/src/applications/herald/adapter/HeraldAdapter.php b/src/applications/herald/adapter/HeraldAdapter.php --- a/src/applications/herald/adapter/HeraldAdapter.php +++ b/src/applications/herald/adapter/HeraldAdapter.php @@ -110,11 +110,16 @@ 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 @@ 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( diff --git a/src/applications/maniphest/editor/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/ManiphestTransactionEditor.php --- a/src/applications/maniphest/editor/ManiphestTransactionEditor.php +++ b/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 @@ ); } - 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 @@ HeraldAdapter $adapter, HeraldTranscript $transcript) { - $this->heraldEmailPHIDs = $adapter->getEmailPHIDs(); $xactions = array(); $cc_phids = $adapter->getCcPHIDs(); diff --git a/src/applications/metamta/query/PhabricatorMetaMTAActor.php b/src/applications/metamta/query/PhabricatorMetaMTAActor.php --- a/src/applications/metamta/query/PhabricatorMetaMTAActor.php +++ b/src/applications/metamta/query/PhabricatorMetaMTAActor.php @@ -16,6 +16,7 @@ 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 @@ '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)); diff --git a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php --- a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php +++ b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php @@ -141,6 +141,15 @@ 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 @@ } 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 @@ $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 @@ $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 @@ } } + // 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; } diff --git a/src/applications/phriction/editor/PhrictionTransactionEditor.php b/src/applications/phriction/editor/PhrictionTransactionEditor.php --- a/src/applications/phriction/editor/PhrictionTransactionEditor.php +++ b/src/applications/phriction/editor/PhrictionTransactionEditor.php @@ -13,7 +13,6 @@ private $skipAncestorCheck; private $contentVersion; private $processContentVersionError = true; - private $heraldEmailPHIDs = array(); public function setDescription($description) { $this->description = $description; @@ -369,16 +368,6 @@ ); } - 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 @@ ->setNewValue(array('+' => $value)); } - $this->heraldEmailPHIDs = $adapter->getEmailPHIDs(); - return $xactions; } diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -1967,8 +1967,15 @@ return; } - $email_to = array_filter(array_unique($this->getMailTo($object))); - $email_cc = array_filter(array_unique($this->getMailCC($object))); + $email_force = array(); + $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 @@ ->setThreadID($this->getMailThreadID($object), $this->getIsNewObject()) ->setRelatedPHID($object->getPHID()) ->setExcludeMailRecipientPHIDs($this->getExcludeMailRecipientPHIDs()) + ->setForceHeraldMailRecipientPHIDs($email_force) ->setMailTags($mail_tags) ->setIsBulk(true) ->setBody($body->render())