From bfea830d09fdeb5fff027a98ad83197891f42476 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 17 Feb 2012 22:57:07 -0800 Subject: [PATCH] Add email preferences to receive fewer less-important notifications Summary: A few similar requests have come in across several tools and use cases that I think this does a reasonable job of resolving. We currently send one email for each update an object receives, but these aren't always appreciated: - Asana does post-commit review via Differential, so the "committed" mails are useless. - Quora wants to make project category edits to bugs without spamming people attached to them. - Some users in general are very sensitive to email volumes, and this gives us a good way to reduce the volumes without incurring the complexity of delayed-send-batching. The technical mechanism is basically: - Mail may optionally have "mail tags", which indicate content in the mail (e.g., "maniphest-priority, maniphest-cc, maniphest-comment" for a mail which contains a priority change, a CC change, and a comment). - If a mail has tags, remove any recipients who have opted out of all the tags. - Some tags can't be opted out of via the UI, so this ensures that important email is still delivered (e.g., cc + assign + comment is always delivered because you can't opt out of "assign" or "comment"). Test Plan: - Disabled all mail tags in the web UI. - Used test console to send myself mail with an opt-outable tag, it was immediately dropped. - Used test console to send myself mail with an opt-outable tag and a custom tag, it was delivered. - Made Differential updates affecting CCs with and without comments, got appropriate delivery. - Made Maniphest updates affecting project, priority and CCs with and without comments, got appropriate delivery. - Verified mail headers in all cases. Reviewers: btrahan Reviewed By: btrahan CC: aran, epriestley, moskov Maniphest Tasks: T616, T855 Differential Revision: https://secure.phabricator.com/D1635 --- src/__phutil_library_map__.php | 3 + .../mail/base/DifferentialMail.php | 11 ++- .../mail/comment/DifferentialCommentMail.php | 29 ++++++ .../differential/mail/comment/__init__.php | 1 + .../ManiphestTransactionEditor.php | 31 ++++++- .../maniphest/editor/transaction/__init__.php | 1 + .../constants/base/MetaMTAConstants.php | 21 +++++ .../metamta/constants/base/__init__.php | 10 +++ .../MetaMTANotificationType.php | 32 +++++++ .../constants/notificationtype/__init__.php | 12 +++ .../send/PhabricatorMetaMTASendController.php | 9 +- .../storage/mail/PhabricatorMetaMTAMail.php | 77 +++++++++++++++- ...EmailPreferenceSettingsPanelController.php | 89 ++++++++++++++++++- .../settings/panels/emailpref/__init__.php | 2 + .../PhabricatorUserPreferences.php | 1 + 15 files changed, 322 insertions(+), 7 deletions(-) create mode 100644 src/applications/metamta/constants/base/MetaMTAConstants.php create mode 100644 src/applications/metamta/constants/base/__init__.php create mode 100644 src/applications/metamta/constants/notificationtype/MetaMTANotificationType.php create mode 100644 src/applications/metamta/constants/notificationtype/__init__.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 0c4945f6dd..40b384796e 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -430,6 +430,8 @@ phutil_register_library_map(array( 'ManiphestTransactionSaveController' => 'applications/maniphest/controller/transactionsave', 'ManiphestTransactionType' => 'applications/maniphest/constants/transactiontype', 'ManiphestView' => 'applications/maniphest/view/base', + 'MetaMTAConstants' => 'applications/metamta/constants/base', + 'MetaMTANotificationType' => 'applications/metamta/constants/notificationtype', 'Phabricator404Controller' => 'applications/base/controller/404', 'PhabricatorAuditActionConstants' => 'applications/audit/constants/action', 'PhabricatorAuditComment' => 'applications/audit/storage/auditcomment', @@ -1184,6 +1186,7 @@ phutil_register_library_map(array( 'ManiphestTransactionSaveController' => 'ManiphestController', 'ManiphestTransactionType' => 'ManiphestConstants', 'ManiphestView' => 'AphrontView', + 'MetaMTANotificationType' => 'MetaMTAConstants', 'Phabricator404Controller' => 'PhabricatorController', 'PhabricatorAuditComment' => 'PhabricatorAuditDAO', 'PhabricatorAuditController' => 'PhabricatorController', diff --git a/src/applications/differential/mail/base/DifferentialMail.php b/src/applications/differential/mail/base/DifferentialMail.php index 70fcc3bfb4..2a4351a9f0 100644 --- a/src/applications/differential/mail/base/DifferentialMail.php +++ b/src/applications/differential/mail/base/DifferentialMail.php @@ -1,7 +1,7 @@ setIsBulk(true); $template->setRelatedPHID($this->getRevision()->getPHID()); + $mailtags = $this->getMailTags(); + if ($mailtags) { + $template->setMailTags($mailtags); + } + $phids = array(); foreach ($to_phids as $phid) { $phids[$phid] = true; @@ -134,6 +139,10 @@ abstract class DifferentialMail { } } + protected function getMailTags() { + return array(); + } + protected function getSubjectPrefix() { return PhabricatorEnv::getEnvConfig('metamta.differential.subject-prefix'); } diff --git a/src/applications/differential/mail/comment/DifferentialCommentMail.php b/src/applications/differential/mail/comment/DifferentialCommentMail.php index 39139d662e..397ecd5f2e 100644 --- a/src/applications/differential/mail/comment/DifferentialCommentMail.php +++ b/src/applications/differential/mail/comment/DifferentialCommentMail.php @@ -44,6 +44,35 @@ class DifferentialCommentMail extends DifferentialMail { } + protected function getMailTags() { + $comment = $this->getComment(); + $action = $comment->getAction(); + + $tags = array(); + switch ($action) { + case DifferentialAction::ACTION_ADDCCS: + $tags[] = MetaMTANotificationType::TYPE_DIFFERENTIAL_CC; + break; + case DifferentialAction::ACTION_COMMIT: + $tags[] = MetaMTANotificationType::TYPE_DIFFERENTIAL_COMMITTED; + break; + } + + if (strlen(trim($comment->getContent()))) { + switch ($action) { + case DifferentialAction::ACTION_COMMIT: + // Commit comments are auto-generated and not especially interesting, + // so don't tag them as having a comment. + break; + default: + $tags[] = MetaMTANotificationType::TYPE_DIFFERENTIAL_COMMENT; + break; + } + } + + return $tags; + } + protected function renderSubject() { $verb = ucwords($this->getVerb()); $revision = $this->getRevision(); diff --git a/src/applications/differential/mail/comment/__init__.php b/src/applications/differential/mail/comment/__init__.php index b624a2bd37..2dc867eda3 100644 --- a/src/applications/differential/mail/comment/__init__.php +++ b/src/applications/differential/mail/comment/__init__.php @@ -11,6 +11,7 @@ phutil_require_module('arcanist', 'differential/constants/revisionstatus'); phutil_require_module('phabricator', 'applications/differential/constants/action'); phutil_require_module('phabricator', 'applications/differential/mail/base'); phutil_require_module('phabricator', 'applications/differential/storage/comment'); +phutil_require_module('phabricator', 'applications/metamta/constants/notificationtype'); phutil_require_module('phabricator', 'applications/phid/handle/data'); phutil_require_module('phabricator', 'infrastructure/env'); diff --git a/src/applications/maniphest/editor/transaction/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/transaction/ManiphestTransactionEditor.php index 6ac800617f..21a24d8926 100644 --- a/src/applications/maniphest/editor/transaction/ManiphestTransactionEditor.php +++ b/src/applications/maniphest/editor/transaction/ManiphestTransactionEditor.php @@ -1,7 +1,7 @@ getSubjectPrefix(); $subject = trim("{$prefix} [{$action}] T{$task_id}: {$title}"); + $mailtags = $this->getMailTags($transactions); + $template = id(new PhabricatorMetaMTAMail()) ->setSubject($subject) ->setFrom($transaction->getAuthorPHID()) @@ -261,6 +263,7 @@ class ManiphestTransactionEditor { ->setThreadID($thread_id, $is_create) ->setRelatedPHID($task->getPHID()) ->setIsBulk(true) + ->setMailTags($mailtags) ->setBody($body); $mails = $reply_handler->multiplexMail( @@ -349,4 +352,30 @@ class ManiphestTransactionEditor { return $is_create; } + private function getMailTags(array $transactions) { + $tags = array(); + foreach ($transactions as $xaction) { + switch ($xaction->getTransactionType()) { + case ManiphestTransactionType::TYPE_CCS: + $tags[] = MetaMTANotificationType::TYPE_MANIPHEST_CC; + break; + case ManiphestTransactionType::TYPE_PROJECTS: + $tags[] = MetaMTANotificationType::TYPE_MANIPHEST_PROJECTS; + break; + case ManiphestTransactionType::TYPE_PRIORITY: + $tags[] = MetaMTANotificationType::TYPE_MANIPHEST_PRIORITY; + break; + default: + $tags[] = MetaMTANotificationType::TYPE_MANIPHEST_OTHER; + break; + } + + if ($xaction->hasComments()) { + $tags[] = MetaMTANotificationType::TYPE_MANIPHEST_COMMENT; + } + } + + return array_unique($tags); + } + } diff --git a/src/applications/maniphest/editor/transaction/__init__.php b/src/applications/maniphest/editor/transaction/__init__.php index 09a4a08fb1..1f6fa0b5be 100644 --- a/src/applications/maniphest/editor/transaction/__init__.php +++ b/src/applications/maniphest/editor/transaction/__init__.php @@ -12,6 +12,7 @@ phutil_require_module('phabricator', 'applications/maniphest/constants/action'); phutil_require_module('phabricator', 'applications/maniphest/constants/status'); phutil_require_module('phabricator', 'applications/maniphest/constants/transactiontype'); phutil_require_module('phabricator', 'applications/maniphest/view/transactiondetail'); +phutil_require_module('phabricator', 'applications/metamta/constants/notificationtype'); phutil_require_module('phabricator', 'applications/metamta/storage/mail'); phutil_require_module('phabricator', 'applications/phid/handle/data'); phutil_require_module('phabricator', 'applications/search/index/indexer/maniphest'); diff --git a/src/applications/metamta/constants/base/MetaMTAConstants.php b/src/applications/metamta/constants/base/MetaMTAConstants.php new file mode 100644 index 0000000000..15b55adaba --- /dev/null +++ b/src/applications/metamta/constants/base/MetaMTAConstants.php @@ -0,0 +1,21 @@ +setSimulatedFailureCount($request->getInt('failures')); $mail->setIsHTML($request->getInt('html')); $mail->setIsBulk($request->getInt('bulk')); + $mail->setMailTags($request->getStrList('mailtags')); $mail->save(); if ($request->getInt('immediately')) { $mail->sendNow(); @@ -117,6 +118,12 @@ class PhabricatorMetaMTASendController extends PhabricatorMetaMTAController { id(new AphrontFormTextAreaControl()) ->setLabel('Body') ->setName('body')) + ->appendChild( + id(new AphrontFormTextControl()) + ->setLabel('Mail Tags') + ->setName('mailtags') + ->setCaption( + 'Example: differential-cc, differential-comment')) ->appendChild( id(new AphrontFormDragAndDropUploadControl()) ->setLabel('Attach Files') diff --git a/src/applications/metamta/storage/mail/PhabricatorMetaMTAMail.php b/src/applications/metamta/storage/mail/PhabricatorMetaMTAMail.php index aa2bec6211..c85e76ca88 100644 --- a/src/applications/metamta/storage/mail/PhabricatorMetaMTAMail.php +++ b/src/applications/metamta/storage/mail/PhabricatorMetaMTAMail.php @@ -63,6 +63,20 @@ class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO { return idx($this->parameters, $param); } + /** + * Set tags (@{class:MetaMTANotificationType} constants) which identify the + * content of this mail in a general way. These tags are used to allow users + * to opt out of receiving certain types of mail, like updates when a task's + * projects change. + * + * @param list List of @{class:MetaMTANotificationType} constants. + * @return this + */ + public function setMailTags(array $tags) { + $this->setParam('mailtags', $tags); + return $this; + } + /** * In Gmail, conversations will be broken if you reply to a thread and the * server sends back a response without referencing your Message-ID, even if @@ -310,8 +324,8 @@ class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO { $reply_to_name = idx($params, 'reply-to-name', ''); unset($params['reply-to-name']); - $add_cc = null; - $add_to = null; + $add_cc = array(); + $add_to = array(); foreach ($params as $key => $value) { switch ($key) { @@ -418,6 +432,9 @@ class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO { $thread_index = $this->generateThreadIndex($value, $is_first); $mailer->addHeader('Thread-Index', $thread_index); break; + case 'mailtags': + // Handled below. + break; default: // Just discard. } @@ -425,6 +442,60 @@ class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO { $mailer->addHeader('X-Mail-Transport-Agent', 'MetaMTA'); + + // If the message has mailtags, filter out any recipients who don't want + // to receive this type of mail. + $mailtags = $this->getParam('mailtags'); + if ($mailtags && ($add_to || $add_cc)) { + + $tag_header = array(); + foreach ($mailtags as $mailtag) { + $tag_header[] = '<'.$mailtag.'>'; + } + $tag_header = implode(', ', $tag_header); + $mailer->addHeader('X-Phabricator-Mail-Tags', $tag_header); + + $exclude = array(); + + $all_recipients = array_merge( + array_keys($add_to), + array_keys($add_cc)); + + $all_prefs = id(new PhabricatorUserPreferences())->loadAllWhere( + 'userPHID in (%Ls)', + $all_recipients); + $all_prefs = mpull($all_prefs, null, 'getUserPHID'); + + foreach ($all_recipients as $recipient) { + $prefs = idx($all_prefs, $recipient); + if (!$prefs) { + continue; + } + + $user_mailtags = $prefs->getPreference( + PhabricatorUserPreferences::PREFERENCE_MAILTAGS, + array()); + + // The user must have elected to receive mail for at least one + // of the mailtags. + $send = false; + foreach ($mailtags as $tag) { + if (idx($user_mailtags, $tag, true)) { + $send = true; + break; + } + } + + if (!$send) { + $exclude[$recipient] = true; + } + } + + $add_to = array_diff_key($add_to, $exclude); + $add_cc = array_diff_key($add_cc, $exclude); + } + + if ($add_to) { $mailer->addTos($add_to); if ($add_cc) { @@ -535,7 +606,7 @@ class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO { if (isset($exclude[$phid])) { continue; } - $emails[] = $handles[$phid]->getEmail(); + $emails[$phid] = $handles[$phid]->getEmail(); } return $emails; diff --git a/src/applications/people/controller/settings/panels/emailpref/PhabricatorUserEmailPreferenceSettingsPanelController.php b/src/applications/people/controller/settings/panels/emailpref/PhabricatorUserEmailPreferenceSettingsPanelController.php index 31af4d02f7..b4632d64ab 100644 --- a/src/applications/people/controller/settings/panels/emailpref/PhabricatorUserEmailPreferenceSettingsPanelController.php +++ b/src/applications/people/controller/settings/panels/emailpref/PhabricatorUserEmailPreferenceSettingsPanelController.php @@ -43,6 +43,14 @@ class PhabricatorUserEmailPreferenceSettingsPanelController $pref_no_self_mail, $request->getStr($pref_no_self_mail)); + + $new_tags = $request->getArr('mailtags'); + $mailtags = $preferences->getPreference('mailtags', array()); + foreach ($this->getMailTags() as $key => $label) { + $mailtags[$key] = (bool)idx($new_tags, $key, false); + } + $preferences->setPreference('mailtags', $mailtags); + $preferences->save(); return id(new AphrontRedirectResponse()) @@ -105,10 +113,38 @@ class PhabricatorUserEmailPreferenceSettingsPanelController )) ->setValue($re_prefix_value)); + $form + ->appendChild( + '
'. + '

'. + 'You can customize what mail you receive from Phabricator here.'. + '

'. + '

'. + 'NOTE: If an update makes several changes (like '. + 'adding CCs to a task, closing it, and adding a comment) you will '. + 'still receive an email as long as at least one of the changes '. + 'is set to notify you.'. + '

' + ); + + $mailtags = $preferences->getPreference('mailtags', array()); + + $form + ->appendChild( + $this->buildMailTagCheckboxes( + $this->getDifferentialMailTags(), + $mailtags) + ->setLabel('Differential')) + ->appendChild( + $this->buildMailTagCheckboxes( + $this->getManiphestMailTags(), + $mailtags) + ->setLabel('Maniphest')); + $form ->appendChild( id(new AphrontFormSubmitControl()) - ->setValue('Save')); + ->setValue('Save Preferences')); $panel = new AphrontPanelView(); $panel->setHeader('Email Preferences'); @@ -122,4 +158,55 @@ class PhabricatorUserEmailPreferenceSettingsPanelController $panel, )); } + + private function getMailTags() { + return array( + MetaMTANotificationType::TYPE_DIFFERENTIAL_CC => + "Send me email when a revision's CCs change.", + MetaMTANotificationType::TYPE_DIFFERENTIAL_COMMITTED => + "Send me email when a revision is committed.", + MetaMTANotificationType::TYPE_MANIPHEST_PROJECTS => + "Send me email when a task's associated projects change.", + MetaMTANotificationType::TYPE_MANIPHEST_PRIORITY => + "Send me email when a task's priority changes.", + MetaMTANotificationType::TYPE_MANIPHEST_CC => + "Send me email when a task's CCs change.", + ); + } + + private function getManiphestMailTags() { + return array_select_keys( + $this->getMailTags(), + array( + MetaMTANotificationType::TYPE_MANIPHEST_PROJECTS, + MetaMTANotificationType::TYPE_MANIPHEST_PRIORITY, + MetaMTANotificationType::TYPE_MANIPHEST_CC, + )); + } + + private function getDifferentialMailTags() { + return array_select_keys( + $this->getMailTags(), + array( + MetaMTANotificationType::TYPE_DIFFERENTIAL_CC, + MetaMTANotificationType::TYPE_DIFFERENTIAL_COMMITTED, + )); + } + + private function buildMailTagCheckboxes( + array $tags, + array $prefs) { + + $control = new AphrontFormCheckboxControl(); + foreach ($tags as $key => $label) { + $control->addCheckbox( + 'mailtags['.$key.']', + 1, + $label, + idx($prefs, $key, 1)); + } + + return $control; + } + } diff --git a/src/applications/people/controller/settings/panels/emailpref/__init__.php b/src/applications/people/controller/settings/panels/emailpref/__init__.php index 8a7376d88d..c6bf80fd00 100644 --- a/src/applications/people/controller/settings/panels/emailpref/__init__.php +++ b/src/applications/people/controller/settings/panels/emailpref/__init__.php @@ -7,10 +7,12 @@ phutil_require_module('phabricator', 'aphront/response/redirect'); +phutil_require_module('phabricator', 'applications/metamta/constants/notificationtype'); phutil_require_module('phabricator', 'applications/people/controller/settings/panels/base'); phutil_require_module('phabricator', 'applications/people/storage/preferences'); phutil_require_module('phabricator', 'infrastructure/env'); phutil_require_module('phabricator', 'view/form/base'); +phutil_require_module('phabricator', 'view/form/control/checkbox'); phutil_require_module('phabricator', 'view/form/control/select'); phutil_require_module('phabricator', 'view/form/control/submit'); phutil_require_module('phabricator', 'view/form/error'); diff --git a/src/applications/people/storage/preferences/PhabricatorUserPreferences.php b/src/applications/people/storage/preferences/PhabricatorUserPreferences.php index 193aca39e4..89aa6c9780 100644 --- a/src/applications/people/storage/preferences/PhabricatorUserPreferences.php +++ b/src/applications/people/storage/preferences/PhabricatorUserPreferences.php @@ -24,6 +24,7 @@ class PhabricatorUserPreferences extends PhabricatorUserDAO { const PREFERENCE_RE_PREFIX = 're-prefix'; const PREFERENCE_NO_SELF_MAIL = 'self-mail'; + const PREFERENCE_MAILTAGS = 'mailtags'; protected $userPHID; protected $preferences = array();