From c0e26c65e03e1d4398b0fa842de6bccb3fe1d84c Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 6 Apr 2015 10:01:11 -0700 Subject: [PATCH] Make mail delivery reasons code-based; include positive and negative reasons Summary: Ref T7731. Looking forward to T5791, I eventually anticipate writing an interface which looks like a webmail UI where users can review mail they've been sent and understand why they recieved (or did not receive) the mail. Roughly like `bin/mail list-outbound` / `bin/mail show-outbound` work today, but policy-aware (so you can only see messages where delivery was attempted to you). We currently record a list of "reasons" why a mail is undeliverable, but this list is string-based (so it can not be translated once we start persisting it) and has only negative reasons (so it can not be used to fully understand reasons for delivery or nondelivery). Make it code-based (so it can be translated) and allow both positive and negative reasons to be listed (so positive reasons can be understood). Test Plan: Used `bin/mail show-outbound` to review mail delivery reasons, including the positive reason we currently have (forced delivery of authentication mail). Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T7731 Differential Revision: https://secure.phabricator.com/D12297 --- ...atorMailManagementShowOutboundWorkflow.php | 7 +- .../metamta/query/PhabricatorMetaMTAActor.php | 71 ++++++++++++++++++- .../query/PhabricatorMetaMTAActorQuery.php | 26 +++---- .../storage/PhabricatorMetaMTAMail.php | 27 ++----- 4 files changed, 88 insertions(+), 43 deletions(-) diff --git a/src/applications/metamta/management/PhabricatorMailManagementShowOutboundWorkflow.php b/src/applications/metamta/management/PhabricatorMailManagementShowOutboundWorkflow.php index bf4b962e29..3b798259b4 100644 --- a/src/applications/metamta/management/PhabricatorMailManagementShowOutboundWorkflow.php +++ b/src/applications/metamta/management/PhabricatorMailManagementShowOutboundWorkflow.php @@ -127,9 +127,10 @@ final class PhabricatorMailManagementShowOutboundWorkflow $info[] = ' '.coalesce($actor->getName(), $actor->getPHID()); } else { $info[] = '! '.coalesce($actor->getName(), $actor->getPHID()); - foreach ($actor->getUndeliverableReasons() as $reason) { - $info[] = ' - '.$reason; - } + } + foreach ($actor->getDeliverabilityReasons() as $reason) { + $desc = PhabricatorMetaMTAActor::getReasonDescription($reason); + $info[] = ' - '.$desc; } } diff --git a/src/applications/metamta/query/PhabricatorMetaMTAActor.php b/src/applications/metamta/query/PhabricatorMetaMTAActor.php index 8580a7bc3b..e314570a50 100644 --- a/src/applications/metamta/query/PhabricatorMetaMTAActor.php +++ b/src/applications/metamta/query/PhabricatorMetaMTAActor.php @@ -2,9 +2,25 @@ final class PhabricatorMetaMTAActor { + const STATUS_DELIVERABLE = 'deliverable'; + const STATUS_UNDELIVERABLE = 'undeliverable'; + + const REASON_UNLOADABLE = 'unloadable'; + const REASON_UNMAILABLE = 'unmailable'; + const REASON_NO_ADDRESS = 'noaddress'; + const REASON_DISABLED = 'disabled'; + const REASON_MAIL_DISABLED = 'maildisabled'; + const REASON_EXTERNAL_TYPE = 'exernaltype'; + const REASON_RESPONSE = 'response'; + const REASON_SELF = 'self'; + const REASON_MAILTAGS = 'mailtags'; + const REASON_BOT = 'bot'; + const REASON_FORCE = 'force'; + private $phid; private $emailAddress; private $name; + private $status = self::STATUS_DELIVERABLE; private $reasons = array(); public function setName($name) { @@ -36,15 +52,66 @@ final class PhabricatorMetaMTAActor { public function setUndeliverable($reason) { $this->reasons[] = $reason; + $this->status = self::STATUS_UNDELIVERABLE; + return $this; + } + + public function setDeliverable($reason) { + $this->reasons[] = $reason; + $this->status = self::STATUS_DELIVERABLE; return $this; } public function isDeliverable() { - return empty($this->reasons); + return ($this->status === self::STATUS_DELIVERABLE); } - public function getUndeliverableReasons() { + public function getDeliverabilityReasons() { return $this->reasons; } + public static function getReasonDescription($reason) { + $descriptions = array( + self::REASON_DISABLED => pht( + 'This user is disabled; disabled users do not receive mail.'), + self::REASON_BOT => pht( + 'This user is a bot; bot accounts do not receive mail.'), + self::REASON_NO_ADDRESS => pht( + 'Unable to load an email address for this PHID.'), + self::REASON_EXTERNAL_TYPE => pht( + 'Only external accounts of type "email" are deliverable; this '. + 'account has a different type.'), + self::REASON_UNMAILABLE => pht( + 'This PHID type does not correspond to a mailable object.'), + self::REASON_RESPONSE => pht( + 'This message is a response to another email message, and this '. + 'recipient received the original email message, so we are not '. + 'sending them this substantially similar message (for example, '. + 'the sender used "Reply All" instead of "Reply" in response to '. + 'mail from Phabricator).'), + self::REASON_SELF => pht( + 'This recipient is the user whose actions caused delivery of '. + 'this message, but they have set preferences so they do not '. + 'receive mail about their own actions (Settings > Email '. + 'Preferences > Self Actions).'), + self::REASON_MAIL_DISABLED => pht( + 'This recipient has disabled all email notifications '. + '(Settings > Email Preferences > Email Notifications).'), + self::REASON_MAILTAGS => pht( + 'This mail has tags which control which users receive it, and '. + 'this recipient has not elected to receive mail with any of '. + 'the tags on this message (Settings > Email Preferences).'), + self::REASON_UNLOADABLE => pht( + 'Unable to load user record for this PHID.'), + self::REASON_FORCE => pht( + 'Delivery of this mail is forced and ignores deliver preferences. '. + 'Mail which uses forced delivery is usually related to account '. + 'management or authentication. For example, password reset email '. + 'ignores mail preferences.'), + ); + + return idx($descriptions, $reason, pht('Unknown Reason ("%s")', $reason)); + } + + } diff --git a/src/applications/metamta/query/PhabricatorMetaMTAActorQuery.php b/src/applications/metamta/query/PhabricatorMetaMTAActorQuery.php index 58178edb1f..1f593472a6 100644 --- a/src/applications/metamta/query/PhabricatorMetaMTAActorQuery.php +++ b/src/applications/metamta/query/PhabricatorMetaMTAActorQuery.php @@ -70,17 +70,14 @@ final class PhabricatorMetaMTAActorQuery extends PhabricatorQuery { $user = idx($users, $phid); if (!$user) { - $actor->setUndeliverable( - pht('Unable to load user record for this PHID.')); + $actor->setUndeliverable(PhabricatorMetaMTAActor::REASON_UNLOADABLE); } else { $actor->setName($this->getUserName($user)); if ($user->getIsDisabled()) { - $actor->setUndeliverable( - pht('This user is disabled; disabled users do not receive mail.')); + $actor->setUndeliverable(PhabricatorMetaMTAActor::REASON_DISABLED); } if ($user->getIsSystemAgent()) { - $actor->setUndeliverable( - pht('This user is a bot; bot accounts do not receive mail.')); + $actor->setUndeliverable(PhabricatorMetaMTAActor::REASON_BOT); } // NOTE: We do send email to unapproved users, and to unverified users, @@ -91,8 +88,7 @@ final class PhabricatorMetaMTAActorQuery extends PhabricatorQuery { $email = idx($emails, $phid); if (!$email) { - $actor->setUndeliverable( - pht('Unable to load email record for this PHID.')); + $actor->setUndeliverable(PhabricatorMetaMTAActor::REASON_NO_ADDRESS); } else { $actor->setEmailAddress($email->getAddress()); } @@ -113,18 +109,14 @@ final class PhabricatorMetaMTAActorQuery extends PhabricatorQuery { $xuser = idx($xusers, $phid); if (!$xuser) { - $actor->setUndeliverable( - pht('Unable to load external user record for this PHID.')); + $actor->setUndeliverable(PhabricatorMetaMTAActor::REASON_UNLOADABLE); continue; } $actor->setName($xuser->getDisplayName()); if ($xuser->getAccountType() != 'email') { - $actor->setUndeliverable( - pht( - 'Only external accounts of type "email" are deliverable; this '. - 'account has a different type.')); + $actor->setUndeliverable(PhabricatorMetaMTAActor::REASON_EXTERNAL_TYPE); continue; } @@ -146,9 +138,7 @@ final class PhabricatorMetaMTAActorQuery extends PhabricatorQuery { $list = idx($lists, $phid); if (!$list) { - $actor->setUndeliverable( - pht( - 'Unable to load mailing list record for this PHID.')); + $actor->setUndeliverable(PhabricatorMetaMTAActor::REASON_UNLOADABLE); continue; } @@ -160,7 +150,7 @@ final class PhabricatorMetaMTAActorQuery extends PhabricatorQuery { private function loadUnknownActors(array $actors, array $phids) { foreach ($phids as $phid) { $actor = $actors[$phid]; - $actor->setUndeliverable(pht('This PHID type is not mailable.')); + $actor->setUndeliverable(PhabricatorMetaMTAActor::REASON_UNMAILABLE); } } diff --git a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php index 7bf19a935a..5d5cccf372 100644 --- a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php +++ b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php @@ -852,6 +852,9 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO { if ($this->getForceDelivery()) { // If we're forcing delivery, skip all the opt-out checks. + foreach ($actors as $actor) { + $actor->setDeliverable(PhabricatorMetaMTAActor::REASON_FORCE); + } return $actors; } @@ -861,13 +864,7 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO { if (!$actor) { continue; } - $actor->setUndeliverable( - pht( - 'This message is a response to another email message, and this '. - 'recipient received the original email message, so we are not '. - 'sending them this substantially similar message (for example, '. - 'the sender used "Reply All" instead of "Reply" in response to '. - 'mail from Phabricator).')); + $actor->setUndeliverable(PhabricatorMetaMTAActor::REASON_RESPONSE); } // Exclude the actor if their preferences are set. @@ -885,12 +882,7 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO { ->loadPreferences() ->getPreference($pref_key); if ($exclude_self) { - $from_actor->setUndeliverable( - pht( - 'This recipient is the user whose actions caused delivery of '. - 'this message, but they have set preferences so they do not '. - 'receive mail about their own actions (Settings > Email '. - 'Preferences > Self Actions).')); + $from_actor->setUndeliverable(PhabricatorMetaMTAActor::REASON_SELF); } } } @@ -907,9 +899,7 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO { false); if ($exclude) { $actors[$phid]->setUndeliverable( - pht( - 'This recipient has disabled all email notifications '. - '(Settings > Email Preferences > Email Notifications).')); + PhabricatorMetaMTAActor::REASON_MAIL_DISABLED); } } @@ -937,10 +927,7 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO { if (!$send) { $actors[$phid]->setUndeliverable( - pht( - 'This mail has tags which control which users receive it, and '. - 'this recipient has not elected to receive mail with any of '. - 'the tags on this message (Settings > Email Preferences).')); + PhabricatorMetaMTAActor::REASON_MAILTAGS); } } }