From 67482fd19d2c7f3a2624f60c839bb78475d14227 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 2 Jun 2016 17:12:15 -0700 Subject: [PATCH] Continue modernizing application access to user preferences Summary: Ref T4103. This is just incremental cleanup: - Add "internal" settings, which aren't editable via the UI. They can still do validation and run through the normal pathway. Move a couple settings to use this. - Remove `getPreference()` on `PhabricatorUser`, which was a sort of prototype version of `getUserSetting()`. - Make `getUserSetting()` validate setting values before returning them, to improve robustness if we change allowable values later. - Add a user setting cache, since reading user settings was getting fairly expensive on Calendar. - Improve performance of setting validation for timezone setting (don't require building/computing all timezone offsets). - Since we have the cache anyway, make the timezone override a little more general in its approach. - Move editor stuff to use `getUserSetting()`. Test Plan: - Changed search scopes. - Reconciled local and server timezone settings by ignoring and changing timezones. - Changed date/time settings, browsed Calendar, queried date ranges. - Verified editor links generate properly in Diffusion. - Browsed around with time/date settings looking at timestamps. - Grepped for `getPreference()`, nuked all the ones coming off `$user` or `$viewer` that I could find. - Changed accessiblity to high-contrast colors. - Ran all unit tests. - Grepped for removed constants. Reviewers: chad Reviewed By: chad Maniphest Tasks: T4103 Differential Revision: https://secure.phabricator.com/D16015 --- src/__phutil_library_map__.php | 6 + src/aphront/response/AphrontAjaxResponse.php | 4 +- .../people/storage/PhabricatorUser.php | 116 ++++++++++-------- .../PhabricatorSettingsTimezoneController.php | 5 +- .../PhabricatorDateTimeSettingsPanel.php | 2 +- .../setting/PhabricatorInternalSetting.php | 4 + .../setting/PhabricatorSearchScopeSetting.php | 16 +++ .../settings/setting/PhabricatorSetting.php | 4 + ...PhabricatorTimezoneIgnoreOffsetSetting.php | 12 ++ .../setting/PhabricatorTimezoneSetting.php | 25 ++++ .../storage/PhabricatorUserPreferences.php | 3 - .../form/control/AphrontFormDateControl.php | 19 +-- .../control/AphrontFormDateControlValue.php | 10 +- src/view/page/PhabricatorBarePageView.php | 4 +- src/view/page/PhabricatorStandardPageView.php | 9 +- .../menu/PhabricatorMainMenuSearchView.php | 8 +- .../phui/calendar/PHUICalendarListView.php | 10 +- src/view/viewutils.php | 8 +- 18 files changed, 168 insertions(+), 97 deletions(-) create mode 100644 src/applications/settings/setting/PhabricatorInternalSetting.php create mode 100644 src/applications/settings/setting/PhabricatorSearchScopeSetting.php create mode 100644 src/applications/settings/setting/PhabricatorTimezoneIgnoreOffsetSetting.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index fd7426ce7d..28b0344977 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2594,6 +2594,7 @@ phutil_register_library_map(array( 'PhabricatorInlineCommentPreviewController' => 'infrastructure/diff/PhabricatorInlineCommentPreviewController.php', 'PhabricatorInlineSummaryView' => 'infrastructure/diff/view/PhabricatorInlineSummaryView.php', 'PhabricatorInstructionsEditField' => 'applications/transactions/editfield/PhabricatorInstructionsEditField.php', + 'PhabricatorInternalSetting' => 'applications/settings/setting/PhabricatorInternalSetting.php', 'PhabricatorInternationalizationManagementExtractWorkflow' => 'infrastructure/internationalization/management/PhabricatorInternationalizationManagementExtractWorkflow.php', 'PhabricatorInternationalizationManagementWorkflow' => 'infrastructure/internationalization/management/PhabricatorInternationalizationManagementWorkflow.php', 'PhabricatorInvalidConfigSetupCheck' => 'applications/config/check/PhabricatorInvalidConfigSetupCheck.php', @@ -3353,6 +3354,7 @@ phutil_register_library_map(array( 'PhabricatorSearchResultBucketGroup' => 'applications/search/buckets/PhabricatorSearchResultBucketGroup.php', 'PhabricatorSearchResultView' => 'applications/search/view/PhabricatorSearchResultView.php', 'PhabricatorSearchSchemaSpec' => 'applications/search/storage/PhabricatorSearchSchemaSpec.php', + 'PhabricatorSearchScopeSetting' => 'applications/settings/setting/PhabricatorSearchScopeSetting.php', 'PhabricatorSearchSelectController' => 'applications/search/controller/PhabricatorSearchSelectController.php', 'PhabricatorSearchSelectField' => 'applications/search/field/PhabricatorSearchSelectField.php', 'PhabricatorSearchStringListField' => 'applications/search/field/PhabricatorSearchStringListField.php', @@ -3538,6 +3540,7 @@ phutil_register_library_map(array( 'PhabricatorTimeFormatSetting' => 'applications/settings/setting/PhabricatorTimeFormatSetting.php', 'PhabricatorTimeGuard' => 'infrastructure/time/PhabricatorTimeGuard.php', 'PhabricatorTimeTestCase' => 'infrastructure/time/__tests__/PhabricatorTimeTestCase.php', + 'PhabricatorTimezoneIgnoreOffsetSetting' => 'applications/settings/setting/PhabricatorTimezoneIgnoreOffsetSetting.php', 'PhabricatorTimezoneSetting' => 'applications/settings/setting/PhabricatorTimezoneSetting.php', 'PhabricatorTimezoneSetupCheck' => 'applications/config/check/PhabricatorTimezoneSetupCheck.php', 'PhabricatorTitleGlyphsSetting' => 'applications/settings/setting/PhabricatorTitleGlyphsSetting.php', @@ -7196,6 +7199,7 @@ phutil_register_library_map(array( 'PhabricatorInlineCommentPreviewController' => 'PhabricatorController', 'PhabricatorInlineSummaryView' => 'AphrontView', 'PhabricatorInstructionsEditField' => 'PhabricatorEditField', + 'PhabricatorInternalSetting' => 'PhabricatorSetting', 'PhabricatorInternationalizationManagementExtractWorkflow' => 'PhabricatorInternationalizationManagementWorkflow', 'PhabricatorInternationalizationManagementWorkflow' => 'PhabricatorManagementWorkflow', 'PhabricatorInvalidConfigSetupCheck' => 'PhabricatorSetupCheck', @@ -8103,6 +8107,7 @@ phutil_register_library_map(array( 'PhabricatorSearchResultBucketGroup' => 'Phobject', 'PhabricatorSearchResultView' => 'AphrontView', 'PhabricatorSearchSchemaSpec' => 'PhabricatorConfigSchemaSpec', + 'PhabricatorSearchScopeSetting' => 'PhabricatorInternalSetting', 'PhabricatorSearchSelectController' => 'PhabricatorSearchBaseController', 'PhabricatorSearchSelectField' => 'PhabricatorSearchField', 'PhabricatorSearchStringListField' => 'PhabricatorSearchField', @@ -8304,6 +8309,7 @@ phutil_register_library_map(array( 'PhabricatorTimeFormatSetting' => 'PhabricatorSelectSetting', 'PhabricatorTimeGuard' => 'Phobject', 'PhabricatorTimeTestCase' => 'PhabricatorTestCase', + 'PhabricatorTimezoneIgnoreOffsetSetting' => 'PhabricatorInternalSetting', 'PhabricatorTimezoneSetting' => 'PhabricatorOptionGroupSetting', 'PhabricatorTimezoneSetupCheck' => 'PhabricatorSetupCheck', 'PhabricatorTitleGlyphsSetting' => 'PhabricatorSelectSetting', diff --git a/src/aphront/response/AphrontAjaxResponse.php b/src/aphront/response/AphrontAjaxResponse.php index b13a62f268..1ccb3fe97e 100644 --- a/src/aphront/response/AphrontAjaxResponse.php +++ b/src/aphront/response/AphrontAjaxResponse.php @@ -64,8 +64,8 @@ final class AphrontAjaxResponse extends AphrontResponse { if ($request) { $viewer = $request->getViewer(); if ($viewer) { - $postprocessor_key = $viewer->getPreference( - PhabricatorUserPreferences::PREFERENCE_RESOURCE_POSTPROCESSOR); + $postprocessor_key = $viewer->getUserSetting( + PhabricatorAccessibilitySetting::SETTINGKEY); if (strlen($postprocessor_key)) { $response->setPostprocessorKey($postprocessor_key); } diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php index c0559b7e7d..2ba5d2b2cd 100644 --- a/src/applications/people/storage/PhabricatorUser.php +++ b/src/applications/people/storage/PhabricatorUser.php @@ -66,7 +66,9 @@ final class PhabricatorUser private $authorities = array(); private $handlePool; private $csrfSalt; - private $timezoneOverride; + + private $settingCacheKeys = array(); + private $settingCache = array(); protected function readField($field) { switch ($field) { @@ -481,19 +483,50 @@ final class PhabricatorUser public function getUserSetting($key) { + // NOTE: We store available keys and cached values separately to make it + // faster to check for `null` in the cache, which is common. + if (isset($this->settingCacheKeys[$key])) { + return $this->settingCache[$key]; + } + $settings_key = PhabricatorUserPreferencesCacheType::KEY_PREFERENCES; $settings = $this->requireCacheData($settings_key); + $defaults = PhabricatorSetting::getAllEnabledSettings($this); + if (array_key_exists($key, $settings)) { - return $settings[$key]; + $value = $settings[$key]; + + // Make sure the value is valid before we return it. This makes things + // more robust when options are changed or removed. + if (isset($defaults[$key])) { + try { + id(clone $defaults[$key]) + ->setViewer($this) + ->assertValidValue($value); + + $this->settingCacheKeys[$key] = true; + $this->settingCache[$key] = $value; + + return $value; + } catch (Exception $ex) { + // Fall through below and return the default value. + } + } } - $defaults = PhabricatorSetting::getAllEnabledSettings($this); if (isset($defaults[$key])) { - return $defaults[$key]->getSettingDefaultValue(); + $value = id(clone $defaults[$key]) + ->setViewer($this) + ->getSettingDefaultValue(); + } else { + $value = null; } - return null; + $this->settingCacheKeys[$key] = true; + $this->settingCache[$key] = $value; + + return $value; } @@ -510,15 +543,25 @@ final class PhabricatorUser return ($actual == $value); } + + /** + * @task settings + */ + public function clearUserSettingCache() { + $this->settingCacheKeys = array(); + $this->settingCache = array(); + + $settings_key = PhabricatorUserPreferencesCacheType::KEY_PREFERENCES; + $this->clearCacheData($settings_key); + + return $this; + } + public function getTranslation() { return $this->getUserSetting(PhabricatorTranslationSetting::SETTINGKEY); } public function getTimezoneIdentifier() { - if ($this->timezoneOverride) { - return $this->timezoneOverride; - } - return $this->getUserSetting(PhabricatorTimezoneSetting::SETTINGKEY); } @@ -533,7 +576,9 @@ final class PhabricatorUser * @task settings */ public function overrideTimezoneIdentifier($identifier) { - $this->timezoneOverride = $identifier; + $timezone_key = PhabricatorTimezoneSetting::SETTINGKEY; + $this->settingCacheKeys[$timezone_key] = true; + $this->settingCache[$timezone_key] = $identifier; return $this; } @@ -578,17 +623,17 @@ final class PhabricatorUser $line, PhabricatorRepository $repository = null) { - $editor = $this->loadPreferences()->getPreference( - PhabricatorUserPreferences::PREFERENCE_EDITOR); + $editor = $this->getUserSetting(PhabricatorEditorSetting::SETTINGKEY); if (is_array($path)) { - $multiedit = $this->loadPreferences()->getPreference( - PhabricatorUserPreferences::PREFERENCE_MULTIEDIT); + $multi_key = PhabricatorEditorMultipleSetting::SETTINGKEY; + $multiedit = $this->getUserSetting($multi_key); switch ($multiedit) { - case '': + case PhabricatorEditorMultipleSetting::VALUE_SPACES: $path = implode(' ', $path); break; - case 'disable': + case PhabricatorEditorMultipleSetting::VALUE_SINGLE: + default: return null; } } @@ -848,48 +893,13 @@ final class PhabricatorUser $format = 'M j'; } else { // Same year, month and day so show a time of day. - $pref_time = PhabricatorUserPreferences::PREFERENCE_TIME_FORMAT; - $format = $this->getPreference($pref_time); + $pref_time = PhabricatorTimeFormatSetting::SETTINGKEY; + $format = $this->getUserSetting($pref_time); } return $when->format($format); } - public function getPreference($key) { - $preferences = $this->loadPreferences(); - - // TODO: After T4103 and T7707 this should eventually be pushed down the - // stack into modular preference definitions and role profiles. This is - // just fixing T8601 and mildly anticipating those changes. - $value = $preferences->getPreference($key); - - $allowed_values = null; - switch ($key) { - case PhabricatorUserPreferences::PREFERENCE_TIME_FORMAT: - $allowed_values = array( - 'g:i A', - 'H:i', - ); - break; - case PhabricatorUserPreferences::PREFERENCE_DATE_FORMAT: - $allowed_values = array( - 'Y-m-d', - 'n/j/Y', - 'd-m-Y', - ); - break; - } - - if ($allowed_values !== null) { - $allowed_values = array_fuse($allowed_values); - if (empty($allowed_values[$value])) { - $value = head($allowed_values); - } - } - - return $value; - } - public function __toString() { return $this->getUsername(); } diff --git a/src/applications/settings/controller/PhabricatorSettingsTimezoneController.php b/src/applications/settings/controller/PhabricatorSettingsTimezoneController.php index 6b5fb39f22..4f34c39fa0 100644 --- a/src/applications/settings/controller/PhabricatorSettingsTimezoneController.php +++ b/src/applications/settings/controller/PhabricatorSettingsTimezoneController.php @@ -30,7 +30,7 @@ final class PhabricatorSettingsTimezoneController if ($request->isFormPost()) { $timezone = $request->getStr('timezone'); - $pref_ignore = PhabricatorUserPreferences::PREFERENCE_IGNORE_OFFSET; + $pref_ignore = PhabricatorTimezoneIgnoreOffsetSetting::SETTINGKEY; $pref_timezone = PhabricatorTimezoneSetting::SETTINGKEY; $preferences = $viewer->loadPreferences(); @@ -56,8 +56,7 @@ final class PhabricatorSettingsTimezoneController ->setPreference($pref_timezone, $timezone) ->save(); - $viewer->clearCacheData( - PhabricatorUserPreferencesCacheType::KEY_PREFERENCES); + $viewer->clearUserSettingCache(); } } diff --git a/src/applications/settings/panel/PhabricatorDateTimeSettingsPanel.php b/src/applications/settings/panel/PhabricatorDateTimeSettingsPanel.php index 6c21a88894..c9c2645030 100644 --- a/src/applications/settings/panel/PhabricatorDateTimeSettingsPanel.php +++ b/src/applications/settings/panel/PhabricatorDateTimeSettingsPanel.php @@ -22,7 +22,7 @@ final class PhabricatorDateTimeSettingsPanel extends PhabricatorSettingsPanel { $pref_time = PhabricatorUserPreferences::PREFERENCE_TIME_FORMAT; $pref_date = PhabricatorUserPreferences::PREFERENCE_DATE_FORMAT; $pref_week_start = PhabricatorUserPreferences::PREFERENCE_WEEK_START_DAY; - $pref_ignore = PhabricatorUserPreferences::PREFERENCE_IGNORE_OFFSET; + $pref_ignore = PhabricatorTimezoneIgnoreOffsetSetting::SETTINGKEY; $preferences = $user->loadPreferences(); $errors = array(); diff --git a/src/applications/settings/setting/PhabricatorInternalSetting.php b/src/applications/settings/setting/PhabricatorInternalSetting.php new file mode 100644 index 0000000000..af77608f41 --- /dev/null +++ b/src/applications/settings/setting/PhabricatorInternalSetting.php @@ -0,0 +1,4 @@ +validateTransactionValue($value); + } + public function getTransactionNewValue($value) { return $value; } diff --git a/src/applications/settings/setting/PhabricatorTimezoneIgnoreOffsetSetting.php b/src/applications/settings/setting/PhabricatorTimezoneIgnoreOffsetSetting.php new file mode 100644 index 0000000000..3a0223b572 --- /dev/null +++ b/src/applications/settings/setting/PhabricatorTimezoneIgnoreOffsetSetting.php @@ -0,0 +1,12 @@ +formatTime($epoch, 'Y!m!d!g:i A'); + $readable = $this->formatTime($epoch, 'Y!m!d!'.$this->getTimeFormat()); $readable = explode('!', $readable, 4); $year = $readable[0]; @@ -140,13 +140,15 @@ final class AphrontFormDateControl extends AphrontFormControl { } private function getTimeFormat() { - return $this->getViewer() - ->getPreference(PhabricatorUserPreferences::PREFERENCE_TIME_FORMAT); + $viewer = $this->getViewer(); + $time_key = PhabricatorTimeFormatSetting::SETTINGKEY; + return $viewer->getUserSetting($time_key); } private function getDateFormat() { - return $this->getViewer() - ->getPreference(PhabricatorUserPreferences::PREFERENCE_DATE_FORMAT); + $viewer = $this->getViewer(); + $date_key = PhabricatorDateFormatSetting::SETTINGKEY; + return $viewer->getUserSetting($date_key); } private function getTimeInputValue() { @@ -241,7 +243,6 @@ final class AphrontFormDateControl extends AphrontFormControl { 'format' => $this->getTimeFormat(), )); - $time_sel = javelin_tag( 'input', array( @@ -262,9 +263,9 @@ final class AphrontFormDateControl extends AphrontFormControl { ), $time_sel); - $preferences = $this->getViewer()->loadPreferences(); - $pref_week_start = PhabricatorUserPreferences::PREFERENCE_WEEK_START_DAY; - $week_start = $preferences->getPreference($pref_week_start, 0); + $viewer = $this->getViewer(); + $week_key = PhabricatorWeekStartDaySetting::SETTINGKEY; + $week_start = $viewer->getUserSetting($week_key); Javelin::initBehavior('fancy-datepicker', array( 'format' => $this->getDateFormat(), diff --git a/src/view/form/control/AphrontFormDateControlValue.php b/src/view/form/control/AphrontFormDateControlValue.php index f5bfda1cc9..aeb35cdb15 100644 --- a/src/view/form/control/AphrontFormDateControlValue.php +++ b/src/view/form/control/AphrontFormDateControlValue.php @@ -207,13 +207,15 @@ final class AphrontFormDateControlValue extends Phobject { } private function getTimeFormat() { - return $this->getViewer() - ->getPreference(PhabricatorUserPreferences::PREFERENCE_TIME_FORMAT); + $viewer = $this->getViewer(); + $time_key = PhabricatorTimeFormatSetting::SETTINGKEY; + return $viewer->getUserSetting($time_key); } private function getDateFormat() { - return $this->getViewer() - ->getPreference(PhabricatorUserPreferences::PREFERENCE_DATE_FORMAT); + $viewer = $this->getViewer(); + $date_key = PhabricatorDateFormatSetting::SETTINGKEY; + return $viewer->getUserSetting($date_key); } private function getFormattedDateFromDate($date, $time) { diff --git a/src/view/page/PhabricatorBarePageView.php b/src/view/page/PhabricatorBarePageView.php index 6a4444c981..82b4d43faf 100644 --- a/src/view/page/PhabricatorBarePageView.php +++ b/src/view/page/PhabricatorBarePageView.php @@ -130,8 +130,8 @@ class PhabricatorBarePageView extends AphrontPageView { if ($this->getRequest()) { $viewer = $this->getRequest()->getViewer(); if ($viewer) { - $postprocessor_key = $viewer->getPreference( - PhabricatorUserPreferences::PREFERENCE_RESOURCE_POSTPROCESSOR); + $postprocessor_key = $viewer->getUserSetting( + PhabricatorAccessibilitySetting::SETTINGKEY); if (strlen($postprocessor_key)) { $response->setPostProcessorKey($postprocessor_key); } diff --git a/src/view/page/PhabricatorStandardPageView.php b/src/view/page/PhabricatorStandardPageView.php index e2d373ab5a..cdadf7c309 100644 --- a/src/view/page/PhabricatorStandardPageView.php +++ b/src/view/page/PhabricatorStandardPageView.php @@ -225,13 +225,8 @@ final class PhabricatorStandardPageView extends PhabricatorBarePageView if ($user->isLoggedIn()) { $offset = $user->getTimeZoneOffset(); - $preferences = $user->loadPreferences(); - $ignore_key = PhabricatorUserPreferences::PREFERENCE_IGNORE_OFFSET; - - $ignore = $preferences->getPreference($ignore_key); - if (!strlen($ignore)) { - $ignore = null; - } + $ignore_key = PhabricatorTimezoneIgnoreOffsetSetting::SETTINGKEY; + $ignore = $user->getUserSetting($ignore_key); Javelin::initBehavior( 'detect-timezone', diff --git a/src/view/page/menu/PhabricatorMainMenuSearchView.php b/src/view/page/menu/PhabricatorMainMenuSearchView.php index d3c7319f7d..aa1855ce17 100644 --- a/src/view/page/menu/PhabricatorMainMenuSearchView.php +++ b/src/view/page/menu/PhabricatorMainMenuSearchView.php @@ -50,7 +50,7 @@ final class PhabricatorMainMenuSearchView extends AphrontView { ''); $search_datasource = new PhabricatorSearchDatasource(); - $scope_key = PhabricatorUserPreferences::PREFERENCE_SEARCH_SCOPE; + $scope_key = PhabricatorSearchScopeSetting::SETTINGKEY; Javelin::initBehavior( 'phabricator-search-typeahead', @@ -177,10 +177,8 @@ final class PhabricatorMainMenuSearchView extends AphrontView { 'href' => PhabricatorEnv::getDoclink('Search User Guide'), ); - $scope_key = PhabricatorUserPreferences::PREFERENCE_SEARCH_SCOPE; - $current_value = $viewer->loadPreferences()->getPreference( - $scope_key, - 'all'); + $scope_key = PhabricatorSearchScopeSetting::SETTINGKEY; + $current_value = $viewer->getUserSetting($scope_key); $current_icon = 'fa-globe'; foreach ($items as $item) { diff --git a/src/view/phui/calendar/PHUICalendarListView.php b/src/view/phui/calendar/PHUICalendarListView.php index fc2929adae..7dd921d769 100644 --- a/src/view/phui/calendar/PHUICalendarListView.php +++ b/src/view/phui/calendar/PHUICalendarListView.php @@ -147,16 +147,18 @@ final class PHUICalendarListView extends AphrontTagView { } private function getEventTooltip(AphrontCalendarEventView $event) { - $time_pref = $this->getUser() - ->getPreference(PhabricatorUserPreferences::PREFERENCE_TIME_FORMAT); + $viewer = $this->getViewer(); + $time_key = PhabricatorTimeFormatSetting::SETTINGKEY; + $time_pref = $viewer->getUserSetting($time_key); Javelin::initBehavior('phabricator-tooltips'); $start = id(AphrontFormDateControlValue::newFromEpoch( - $this->getUser(), + $viewer, $event->getEpochStart())); + $end = id(AphrontFormDateControlValue::newFromEpoch( - $this->getUser(), + $viewer, $event->getEpochEnd())); $start_date = $start->getDateTime()->format('m d Y'); diff --git a/src/view/viewutils.php b/src/view/viewutils.php index 8d57cd02d0..ea89399e22 100644 --- a/src/view/viewutils.php +++ b/src/view/viewutils.php @@ -31,21 +31,21 @@ function phabricator_relative_date($epoch, $user, $on = false) { } function phabricator_time($epoch, $user) { - $time_key = PhabricatorUserPreferences::PREFERENCE_TIME_FORMAT; + $time_key = PhabricatorTimeFormatSetting::SETTINGKEY; return phabricator_format_local_time( $epoch, $user, - $user->getPreference($time_key)); + $user->getUserSetting($time_key)); } function phabricator_datetime($epoch, $user) { - $time_key = PhabricatorUserPreferences::PREFERENCE_TIME_FORMAT; + $time_key = PhabricatorTimeFormatSetting::SETTINGKEY; return phabricator_format_local_time( $epoch, $user, pht('%s, %s', phutil_date_format($epoch), - $user->getPreference($time_key))); + $user->getUserSetting($time_key))); } /** -- 2.51.2