From 2e450212503e80e07c55821d5ba68d6370e98d04 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 14 Jun 2016 11:49:59 -0700 Subject: [PATCH] Fix several issues with email-related global preferences Summary: Ref T11098. Mixture of issues here: - Similar problem to D16112, where users with no settings at all could fail to fall back to the global defaults. - I made `UserPreferencesQuery` responsible for building defaults instead to simplify this, since we have 4 or 5 callsites which need to do it and they aren't easily reducible. - Handle cases where `metamta.one-mail-per-recipient` is off (and thus users can not have any custom settings) more explicitly. - When `metamta.one-mail-per-recipient` is off, remove the "Email Format" panel for users only -- administrators can still access it in global preferences. Test Plan: - Deleted a user's preferences, changed globals, purged cache, made sure defaults reflected global defaults. - Changed global mail tags, sent mail to the user, verified it was dropped in accordinace with global settings. - Changed user's settings to get the mail instead, verified mail was sent. - Toggled user's Re / Vary settings, verified mail subject lines reflected user settings. - Disabled `metamta.one-mail-per-recipient`, verified user "Email Format" panel vanished. - Edited "Email Format" in single-mail-mode in global prefs as an administrator. - Sent more mail, verified mail respected new global settings. Reviewers: chad Reviewed By: chad Maniphest Tasks: T11098 Differential Revision: https://secure.phabricator.com/D16118 --- .../feed/PhabricatorFeedStoryPublisher.php | 1 + .../storage/PhabricatorMetaMTAMail.php | 26 ++++++----------- .../PhabricatorUserPreferencesCacheType.php | 19 ++---------- .../PhabricatorSettingsMainController.php | 4 +++ .../PhabricatorEmailFormatSettingsPanel.php | 8 +++++ .../panel/PhabricatorSettingsPanel.php | 12 ++++++++ .../query/PhabricatorUserPreferencesQuery.php | 29 ++++++++++++++++++- .../setting/PhabricatorEmailFormatSetting.php | 4 --- .../PhabricatorEmailRePrefixSetting.php | 4 --- .../PhabricatorEmailVarySubjectsSetting.php | 4 --- .../storage/PhabricatorUserPreferences.php | 28 ++++++++++-------- 11 files changed, 81 insertions(+), 58 deletions(-) diff --git a/src/applications/feed/PhabricatorFeedStoryPublisher.php b/src/applications/feed/PhabricatorFeedStoryPublisher.php index 3b59edc37b..8d018c61b3 100644 --- a/src/applications/feed/PhabricatorFeedStoryPublisher.php +++ b/src/applications/feed/PhabricatorFeedStoryPublisher.php @@ -215,6 +215,7 @@ final class PhabricatorFeedStoryPublisher extends Phobject { $all_prefs = id(new PhabricatorUserPreferencesQuery()) ->setViewer(PhabricatorUser::getOmnipotentUser()) ->withUserPHIDs($phids) + ->needSyntheticPreferences(true) ->execute(); $all_prefs = mpull($all_prefs, null, 'getUserPHID'); } diff --git a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php index ceea3f0429..0c90e43832 100644 --- a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php +++ b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php @@ -877,6 +877,7 @@ final class PhabricatorMetaMTAMail $all_prefs = id(new PhabricatorUserPreferencesQuery()) ->setViewer(PhabricatorUser::getOmnipotentUser()) ->withUserPHIDs($actor_phids) + ->needSyntheticPreferences(true) ->execute(); $all_prefs = mpull($all_prefs, null, 'getUserPHID'); @@ -1105,29 +1106,20 @@ final class PhabricatorMetaMTAMail private function loadPreferences($target_phid) { - if (!self::shouldMultiplexAllMail()) { - $target_phid = null; - } + $viewer = PhabricatorUser::getOmnipotentUser(); - if ($target_phid) { + if (self::shouldMultiplexAllMail()) { $preferences = id(new PhabricatorUserPreferencesQuery()) - ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->setViewer($viewer) ->withUserPHIDs(array($target_phid)) + ->needSyntheticPreferences(true) ->executeOne(); - } else { - $preferences = null; + if ($preferences) { + return $preferences; + } } - // TODO: Here, we would load global preferences once they exist. - - if (!$preferences) { - // If we haven't found suitable preferences yet, return an empty object - // which implicitly has all the default values. - $preferences = id(new PhabricatorUserPreferences()) - ->attachUser(new PhabricatorUser()); - } - - return $preferences; + return PhabricatorUserPreferences::loadGlobalPreferences($viewer); } private function shouldAddRePrefix(PhabricatorUserPreferences $preferences) { diff --git a/src/applications/people/cache/PhabricatorUserPreferencesCacheType.php b/src/applications/people/cache/PhabricatorUserPreferencesCacheType.php index 015a24cb0f..7fee680def 100644 --- a/src/applications/people/cache/PhabricatorUserPreferencesCacheType.php +++ b/src/applications/people/cache/PhabricatorUserPreferencesCacheType.php @@ -29,29 +29,16 @@ final class PhabricatorUserPreferencesCacheType $preferences = id(new PhabricatorUserPreferencesQuery()) ->setViewer($viewer) - ->withUserPHIDs($user_phids) + ->withUsers($users) + ->needSyntheticPreferences(true) ->execute(); $preferences = mpull($preferences, null, 'getUserPHID'); - // If some users don't have settings of their own yet, we need to load - // the global default settings to generate caches for them. - if (count($preferences) < count($user_phids)) { - $global = id(new PhabricatorUserPreferencesQuery()) - ->setViewer($viewer) - ->withBuiltinKeys( - array( - PhabricatorUserPreferences::BUILTIN_GLOBAL_DEFAULT, - )) - ->executeOne(); - } else { - $global = null; - } - $all_settings = PhabricatorSetting::getAllSettings(); $settings = array(); foreach ($users as $user_phid => $user) { - $preference = idx($preferences, $user_phid, $global); + $preference = idx($preferences, $user_phid); if (!$preference) { continue; diff --git a/src/applications/settings/controller/PhabricatorSettingsMainController.php b/src/applications/settings/controller/PhabricatorSettingsMainController.php index 2a368d8650..fada4a0937 100644 --- a/src/applications/settings/controller/PhabricatorSettingsMainController.php +++ b/src/applications/settings/controller/PhabricatorSettingsMainController.php @@ -152,6 +152,10 @@ final class PhabricatorSettingsMainController if (!$this->isSelf() && !$panel->isManagementPanel()) { continue; } + + if ($this->isSelf() && !$panel->isUserPanel()) { + continue; + } } if (!empty($result[$key])) { diff --git a/src/applications/settings/panel/PhabricatorEmailFormatSettingsPanel.php b/src/applications/settings/panel/PhabricatorEmailFormatSettingsPanel.php index 5cab5dea41..107816f2eb 100644 --- a/src/applications/settings/panel/PhabricatorEmailFormatSettingsPanel.php +++ b/src/applications/settings/panel/PhabricatorEmailFormatSettingsPanel.php @@ -13,7 +13,15 @@ final class PhabricatorEmailFormatSettingsPanel return PhabricatorSettingsEmailPanelGroup::PANELGROUPKEY; } + public function isUserPanel() { + return PhabricatorMetaMTAMail::shouldMultiplexAllMail(); + } + public function isManagementPanel() { + if (!$this->isUserPanel()) { + return false; + } + if ($this->getUser()->getIsMailingList()) { return true; } diff --git a/src/applications/settings/panel/PhabricatorSettingsPanel.php b/src/applications/settings/panel/PhabricatorSettingsPanel.php index b66b03f9c8..7d86eaf243 100644 --- a/src/applications/settings/panel/PhabricatorSettingsPanel.php +++ b/src/applications/settings/panel/PhabricatorSettingsPanel.php @@ -154,6 +154,18 @@ abstract class PhabricatorSettingsPanel extends Phobject { } + /** + * Return true if this panel is available to users while editing their own + * settings. + * + * @return bool True to enable management on behalf of a user. + * @task config + */ + public function isUserPanel() { + return true; + } + + /** * Return true if this panel is available to administrators while managing * bot and mailing list accounts. diff --git a/src/applications/settings/query/PhabricatorUserPreferencesQuery.php b/src/applications/settings/query/PhabricatorUserPreferencesQuery.php index 280ca3eea6..de4887cbb8 100644 --- a/src/applications/settings/query/PhabricatorUserPreferencesQuery.php +++ b/src/applications/settings/query/PhabricatorUserPreferencesQuery.php @@ -9,6 +9,7 @@ final class PhabricatorUserPreferencesQuery private $builtinKeys; private $hasUserPHID; private $users = array(); + private $synthetic; public function withIDs(array $ids) { $this->ids = $ids; @@ -42,12 +43,38 @@ final class PhabricatorUserPreferencesQuery return $this; } + /** + * Always return preferences for every queried user. + * + * If no settings exist for a user, a new empty settings object with + * appropriate defaults is returned. + * + * @param bool True to generat synthetic preferences for missing users. + */ + public function needSyntheticPreferences($synthetic) { + $this->synthetic = $synthetic; + return $this; + } + public function newResultObject() { return new PhabricatorUserPreferences(); } protected function loadPage() { - return $this->loadStandardPage($this->newResultObject()); + $preferences = $this->loadStandardPage($this->newResultObject()); + + if ($this->synthetic) { + $user_map = mpull($preferences, null, 'getUserPHID'); + foreach ($this->userPHIDs as $user_phid) { + if (isset($user_map[$user_phid])) { + continue; + } + $preferences[] = $this->newResultObject() + ->setUserPHID($user_phid); + } + } + + return $preferences; } protected function willFilterPage(array $prefs) { diff --git a/src/applications/settings/setting/PhabricatorEmailFormatSetting.php b/src/applications/settings/setting/PhabricatorEmailFormatSetting.php index 0cf0db5d74..333d85c6f4 100644 --- a/src/applications/settings/setting/PhabricatorEmailFormatSetting.php +++ b/src/applications/settings/setting/PhabricatorEmailFormatSetting.php @@ -20,10 +20,6 @@ final class PhabricatorEmailFormatSetting return 100; } - protected function isEnabledForViewer(PhabricatorUser $viewer) { - return PhabricatorMetaMTAMail::shouldMultiplexAllMail(); - } - protected function getControlInstructions() { return pht( 'You can opt to receive plain text email from Phabricator instead '. diff --git a/src/applications/settings/setting/PhabricatorEmailRePrefixSetting.php b/src/applications/settings/setting/PhabricatorEmailRePrefixSetting.php index 7b04ef80c3..5e70b731cd 100644 --- a/src/applications/settings/setting/PhabricatorEmailRePrefixSetting.php +++ b/src/applications/settings/setting/PhabricatorEmailRePrefixSetting.php @@ -20,10 +20,6 @@ final class PhabricatorEmailRePrefixSetting return 200; } - protected function isEnabledForViewer(PhabricatorUser $viewer) { - return PhabricatorMetaMTAMail::shouldMultiplexAllMail(); - } - protected function getControlInstructions() { return pht( 'The **Add "Re:" Prefix** setting adds "Re:" in front of all messages, '. diff --git a/src/applications/settings/setting/PhabricatorEmailVarySubjectsSetting.php b/src/applications/settings/setting/PhabricatorEmailVarySubjectsSetting.php index 0c6b73907b..1c088d9411 100644 --- a/src/applications/settings/setting/PhabricatorEmailVarySubjectsSetting.php +++ b/src/applications/settings/setting/PhabricatorEmailVarySubjectsSetting.php @@ -20,10 +20,6 @@ final class PhabricatorEmailVarySubjectsSetting return 300; } - protected function isEnabledForViewer(PhabricatorUser $viewer) { - return PhabricatorMetaMTAMail::shouldMultiplexAllMail(); - } - protected function getControlInstructions() { return pht( 'With **Vary Subjects** enabled, most mail subject lines will include '. diff --git a/src/applications/settings/storage/PhabricatorUserPreferences.php b/src/applications/settings/storage/PhabricatorUserPreferences.php index b03365bd32..5ed360ca2c 100644 --- a/src/applications/settings/storage/PhabricatorUserPreferences.php +++ b/src/applications/settings/storage/PhabricatorUserPreferences.php @@ -122,31 +122,35 @@ final class PhabricatorUserPreferences * @param PhabricatorUser User to load or create preferences for. */ public static function loadUserPreferences(PhabricatorUser $user) { - $preferences = id(new PhabricatorUserPreferencesQuery()) + return id(new PhabricatorUserPreferencesQuery()) ->setViewer($user) ->withUsers(array($user)) + ->needSyntheticPreferences(true) ->executeOne(); - if ($preferences) { - return $preferences; - } - - $preferences = id(new self()) - ->setUserPHID($user->getPHID()) - ->attachUser($user); + } + /** + * Load or create a global preferences object. + * + * If no global preferences exist, an empty preferences object is returned. + * + * @param PhabricatorUser Viewing user. + */ + public static function loadGlobalPreferences(PhabricatorUser $viewer) { $global = id(new PhabricatorUserPreferencesQuery()) - ->setViewer($user) + ->setViewer($viewer) ->withBuiltinKeys( array( self::BUILTIN_GLOBAL_DEFAULT, )) ->executeOne(); - if ($global) { - $preferences->attachDefaultSettings($global); + if (!$global) { + $global = id(new self()) + ->attachUser(new PhabricatorUser()); } - return $preferences; + return $global; } public function newTransaction($key, $value) {