From d011f8fdc6c4653132a2a03434ef9404200681a7 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 12 Aug 2014 12:28:29 -0700 Subject: [PATCH] Add a setting to disable all notification email Summary: Ref T5861. Adds an option to opt out of all notification email. We'll still send you password resets, email verifications, etc. Test Plan: {F189484} - Added unit tests. - With preference set to different things, tried to send myself mail. Mail respected preferences. - Sent password reset email, which got through the preference. Reviewers: btrahan, chad Reviewed By: chad Subscribers: rush898, epriestley Maniphest Tasks: T5861 Differential Revision: https://secure.phabricator.com/D10237 --- .../PhabricatorEmailLoginController.php | 5 +- .../LegalpadDocumentSignController.php | 1 + .../storage/PhabricatorMetaMTAMail.php | 46 +++++++++++++++++-- .../PhabricatorMetaMTAReceivedMail.php | 1 + .../PhabricatorMetaMTAMailTestCase.php | 27 ++++++++++- .../PhabricatorPeopleApproveController.php | 1 + .../people/storage/PhabricatorUser.php | 2 + .../people/storage/PhabricatorUserEmail.php | 3 ++ ...abricatorSettingsPanelEmailPreferences.php | 33 ++++++++++++- .../storage/PhabricatorUserPreferences.php | 1 + 10 files changed, 109 insertions(+), 11 deletions(-) diff --git a/src/applications/auth/controller/PhabricatorEmailLoginController.php b/src/applications/auth/controller/PhabricatorEmailLoginController.php index 8b5a6b19c5..92067fc3a3 100644 --- a/src/applications/auth/controller/PhabricatorEmailLoginController.php +++ b/src/applications/auth/controller/PhabricatorEmailLoginController.php @@ -108,12 +108,9 @@ Phabricator EOBODY; } - // NOTE: Don't set the user as 'from', or they may not receive the - // mail if they have the "don't send me email about my own actions" - // preference set. - $mail = id(new PhabricatorMetaMTAMail()) ->setSubject(pht('[Phabricator] Password Reset')) + ->setForceDelivery(true) ->addRawTos(array($target_email->getAddress())) ->setBody($body) ->saveAndSend(); diff --git a/src/applications/legalpad/controller/LegalpadDocumentSignController.php b/src/applications/legalpad/controller/LegalpadDocumentSignController.php index cfd5e19d93..ca0a8c5801 100644 --- a/src/applications/legalpad/controller/LegalpadDocumentSignController.php +++ b/src/applications/legalpad/controller/LegalpadDocumentSignController.php @@ -631,6 +631,7 @@ EOBODY; id(new PhabricatorMetaMTAMail()) ->addRawTos(array($email->getAddress())) ->setSubject(pht('[Legalpad] Signature Verification')) + ->setForceDelivery(true) ->setBody($body) ->setRelatedPHID($signature->getDocumentPHID()) ->saveAndSend(); diff --git a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php index 75ee634b86..cb6bac7262 100644 --- a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php +++ b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php @@ -242,6 +242,25 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO { return $this->getParam('cc', array()); } + /** + * Force delivery of a message, even if recipients have preferences which + * would otherwise drop the message. + * + * This is primarily intended to let users who don't want any email still + * receive things like password resets. + * + * @param bool True to force delivery despite user preferences. + * @return this + */ + public function setForceDelivery($force) { + $this->setParam('force', $force); + return $this; + } + + public function getForceDelivery() { + return $this->getParam('force', false); + } + /** * Flag that this is an auto-generated bulk message and should have bulk * headers added to it if appropriate. Broadly, this means some flavor of @@ -794,6 +813,11 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO { return array(); } + if ($this->getForceDelivery()) { + // If we're forcing delivery, skip all the opt-out checks. + return $actors; + } + // Exclude explicit recipients. foreach ($this->getExcludeMailRecipientPHIDs() as $phid) { $actor = idx($actors, $phid); @@ -834,17 +858,29 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO { } } + $all_prefs = id(new PhabricatorUserPreferences())->loadAllWhere( + 'userPHID in (%Ls)', + $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( + pht( + 'This recipient has disabled all email notifications '. + '(Settings > Email Preferences > Email Notifications).')); + } + } // Exclude all recipients who have set preferences to not receive this type // of email (for example, a user who says they don't want emails about task // CC changes). $tags = $this->getParam('mailtags'); if ($tags) { - $all_prefs = id(new PhabricatorUserPreferences())->loadAllWhere( - 'userPHID in (%Ls)', - $actor_phids); - $all_prefs = mpull($all_prefs, null, 'getUserPHID'); - foreach ($all_prefs as $phid => $prefs) { $user_mailtags = $prefs->getPreference( PhabricatorUserPreferences::PREFERENCE_MAILTAGS, diff --git a/src/applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php b/src/applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php index b43502514a..742b4e1ee4 100644 --- a/src/applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php +++ b/src/applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php @@ -329,6 +329,7 @@ EOBODY $mail = id(new PhabricatorMetaMTAMail()) ->setIsErrorEmail(true) + ->setForceDelivery(true) ->setSubject($title) ->addRawTos(array($from)) ->setBody($body) diff --git a/src/applications/metamta/storage/__tests__/PhabricatorMetaMTAMailTestCase.php b/src/applications/metamta/storage/__tests__/PhabricatorMetaMTAMailTestCase.php index d3575097d5..5822b11a98 100644 --- a/src/applications/metamta/storage/__tests__/PhabricatorMetaMTAMailTestCase.php +++ b/src/applications/metamta/storage/__tests__/PhabricatorMetaMTAMailTestCase.php @@ -72,7 +72,7 @@ final class PhabricatorMetaMTAMailTestCase extends PhabricatorTestCase { '"To" is a recipient.'); - // Test that the "No Self Mail" preference works correctly. + // Test that the "No Self Mail" and "No Mail" preferences work correctly. $mail->setFrom($phid); $this->assertTrue( @@ -92,6 +92,31 @@ final class PhabricatorMetaMTAMailTestCase extends PhabricatorTestCase { PhabricatorUserPreferences::PREFERENCE_NO_SELF_MAIL); $prefs->save(); + $this->assertTrue( + in_array($phid, $mail->buildRecipientList()), + '"From" does not exclude recipients by default.'); + + $prefs->setPreference( + PhabricatorUserPreferences::PREFERENCE_NO_MAIL, + true); + $prefs->save(); + + $this->assertFalse( + in_array($phid, $mail->buildRecipientList()), + '"From" excludes recipients with no-mail set.'); + + $mail->setForceDelivery(true); + + $this->assertTrue( + in_array($phid, $mail->buildRecipientList()), + '"From" includes no-mail recipients when forced.'); + + $mail->setForceDelivery(false); + + $prefs->unsetPreference( + PhabricatorUserPreferences::PREFERENCE_NO_MAIL); + $prefs->save(); + $this->assertTrue( in_array($phid, $mail->buildRecipientList()), '"From" does not exclude recipients by default.'); diff --git a/src/applications/people/controller/PhabricatorPeopleApproveController.php b/src/applications/people/controller/PhabricatorPeopleApproveController.php index 23d43da28d..ce1ac85dde 100644 --- a/src/applications/people/controller/PhabricatorPeopleApproveController.php +++ b/src/applications/people/controller/PhabricatorPeopleApproveController.php @@ -45,6 +45,7 @@ final class PhabricatorPeopleApproveController ->addTos(array($user->getPHID())) ->addCCs(array($admin->getPHID())) ->setSubject('[Phabricator] '.$title) + ->setForceDelivery(true) ->setBody($body) ->saveAndSend(); diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php index 5a0d7e47f1..afd33dc4b0 100644 --- a/src/applications/people/storage/PhabricatorUser.php +++ b/src/applications/people/storage/PhabricatorUser.php @@ -540,6 +540,7 @@ EOBODY; $mail = id(new PhabricatorMetaMTAMail()) ->addTos(array($this->getPHID())) + ->setForceDelivery(true) ->setSubject('[Phabricator] Welcome to Phabricator') ->setBody($body) ->saveAndSend(); @@ -583,6 +584,7 @@ EOBODY; $mail = id(new PhabricatorMetaMTAMail()) ->addTos(array($this->getPHID())) + ->setForceDelivery(true) ->setSubject('[Phabricator] Username Changed') ->setBody($body) ->saveAndSend(); diff --git a/src/applications/people/storage/PhabricatorUserEmail.php b/src/applications/people/storage/PhabricatorUserEmail.php index 31c5377022..c7b5662b58 100644 --- a/src/applications/people/storage/PhabricatorUserEmail.php +++ b/src/applications/people/storage/PhabricatorUserEmail.php @@ -173,6 +173,7 @@ EOBODY; id(new PhabricatorMetaMTAMail()) ->addRawTos(array($address)) + ->setForceDelivery(true) ->setSubject('[Phabricator] Email Verification') ->setBody($body) ->setRelatedPHID($user->getPHID()) @@ -210,6 +211,7 @@ EOBODY; id(new PhabricatorMetaMTAMail()) ->addRawTos(array($old_address)) + ->setForceDelivery(true) ->setSubject('[Phabricator] Primary Address Changed') ->setBody($body) ->setFrom($user->getPHID()) @@ -241,6 +243,7 @@ EOBODY; id(new PhabricatorMetaMTAMail()) ->addRawTos(array($new_address)) + ->setForceDelivery(true) ->setSubject('[Phabricator] Primary Address Changed') ->setBody($body) ->setFrom($user->getPHID()) diff --git a/src/applications/settings/panel/PhabricatorSettingsPanelEmailPreferences.php b/src/applications/settings/panel/PhabricatorSettingsPanelEmailPreferences.php index 1000c3fe13..1f5b3d77b2 100644 --- a/src/applications/settings/panel/PhabricatorSettingsPanelEmailPreferences.php +++ b/src/applications/settings/panel/PhabricatorSettingsPanelEmailPreferences.php @@ -20,10 +20,15 @@ final class PhabricatorSettingsPanelEmailPreferences $preferences = $user->loadPreferences(); + $pref_no_mail = PhabricatorUserPreferences::PREFERENCE_NO_MAIL; $pref_no_self_mail = PhabricatorUserPreferences::PREFERENCE_NO_SELF_MAIL; $errors = array(); if ($request->isFormPost()) { + $preferences->setPreference( + $pref_no_mail, + $request->getStr($pref_no_mail)); + $preferences->setPreference( $pref_no_self_mail, $request->getStr($pref_no_self_mail)); @@ -56,6 +61,33 @@ final class PhabricatorSettingsPanelEmailPreferences $form = new AphrontFormView(); $form ->setUser($user) + ->appendRemarkupInstructions( + pht( + 'These settings let you control how Phabricator notifies you about '. + 'events. You can configure Phabricator to send you an email, '. + 'just send a web notification, or not notify you at all.')) + ->appendRemarkupInstructions( + pht( + 'If you disable **Email Notifications**, Phabricator will never '. + 'send email to notify you about events. This preference overrides '. + 'all your other settings.'. + "\n\n". + "//You may still receive some administrative email, like password ". + "reset email.//")) + ->appendChild( + id(new AphrontFormSelectControl()) + ->setLabel(pht('Email Notifications')) + ->setName($pref_no_mail) + ->setOptions( + array( + '0' => pht('Send me email notifications'), + '1' => pht('Never send email notifications'), + )) + ->setValue($preferences->getPreference($pref_no_mail, 0))) + ->appendRemarkupInstructions( + pht( + 'If you disable **Self Actions**, Phabricator will not notify '. + 'you about actions you take.')) ->appendChild( id(new AphrontFormSelectControl()) ->setLabel(pht('Self Actions')) @@ -65,7 +97,6 @@ final class PhabricatorSettingsPanelEmailPreferences '0' => pht('Send me an email when I take an action'), '1' => pht('Do not send me an email when I take an action'), )) - ->setCaption(pht('You can disable email about your own actions.')) ->setValue($preferences->getPreference($pref_no_self_mail, 0))); $mailtags = $preferences->getPreference('mailtags', array()); diff --git a/src/applications/settings/storage/PhabricatorUserPreferences.php b/src/applications/settings/storage/PhabricatorUserPreferences.php index 78e006089b..f4e931bf6a 100644 --- a/src/applications/settings/storage/PhabricatorUserPreferences.php +++ b/src/applications/settings/storage/PhabricatorUserPreferences.php @@ -12,6 +12,7 @@ final class PhabricatorUserPreferences extends PhabricatorUserDAO { const PREFERENCE_RE_PREFIX = 're-prefix'; const PREFERENCE_NO_SELF_MAIL = 'self-mail'; + const PREFERENCE_NO_MAIL = 'no-mail'; const PREFERENCE_MAILTAGS = 'mailtags'; const PREFERENCE_VARY_SUBJECT = 'vary-subject';