diff --git a/app/assets/javascripts/admin/addon/mixins/setting-component.js b/app/assets/javascripts/admin/addon/mixins/setting-component.js index 8ffcd1d8489..4af6d3a357c 100644 --- a/app/assets/javascripts/admin/addon/mixins/setting-component.js +++ b/app/assets/javascripts/admin/addon/mixins/setting-component.js @@ -9,7 +9,10 @@ import { Promise } from "rsvp"; import JsonSchemaEditorModal from "discourse/components/modal/json-schema-editor"; import { ajax } from "discourse/lib/ajax"; import { fmt, propertyNotEqual } from "discourse/lib/computed"; -import { SITE_SETTING_REQUIRES_CONFIRMATION_TYPES } from "discourse/lib/constants"; +import { + DEFAULT_USER_PREFERENCES, + SITE_SETTING_REQUIRES_CONFIRMATION_TYPES, +} from "discourse/lib/constants"; import { deepEqual } from "discourse/lib/object"; import { humanizedSettingName } from "discourse/lib/site-settings-utils"; import { splitString } from "discourse/lib/utilities"; @@ -44,45 +47,6 @@ const CUSTOM_TYPES = [ const AUTO_REFRESH_ON_SAVE = ["logo", "logo_small", "large_icon"]; -const DEFAULT_USER_PREFERENCES = [ - "default_email_digest_frequency", - "default_include_tl0_in_digests", - "default_email_level", - "default_email_messages_level", - "default_email_mailing_list_mode", - "default_email_mailing_list_mode_frequency", - "default_email_previous_replies", - "default_email_in_reply_to", - "default_hide_profile", - "default_hide_presence", - "default_other_new_topic_duration_minutes", - "default_other_auto_track_topics_after_msecs", - "default_other_notification_level_when_replying", - "default_other_external_links_in_new_tab", - "default_other_enable_quoting", - "default_other_enable_smart_lists", - "default_other_enable_defer", - "default_other_dynamic_favicon", - "default_other_like_notification_frequency", - "default_other_skip_new_user_tips", - "default_topics_automatic_unpin", - "default_categories_watching", - "default_categories_tracking", - "default_categories_muted", - "default_categories_watching_first_post", - "default_categories_normal", - "default_tags_watching", - "default_tags_tracking", - "default_tags_muted", - "default_tags_watching_first_post", - "default_text_size", - "default_title_count_mode", - "default_navigation_menu_categories", - "default_navigation_menu_tags", - "default_sidebar_link_to_filtered_list", - "default_sidebar_show_count_of_new_items", -]; - export default Mixin.create({ modal: service(), router: service(), diff --git a/app/assets/javascripts/discourse/app/lib/constants.js b/app/assets/javascripts/discourse/app/lib/constants.js index f2fadc2863a..8583697ea2a 100644 --- a/app/assets/javascripts/discourse/app/lib/constants.js +++ b/app/assets/javascripts/discourse/app/lib/constants.js @@ -102,6 +102,45 @@ export const SITE_SETTING_REQUIRES_CONFIRMATION_TYPES = { user_option: "user_option", }; +export const DEFAULT_USER_PREFERENCES = [ + "default_email_digest_frequency", + "default_include_tl0_in_digests", + "default_email_level", + "default_email_messages_level", + "default_email_mailing_list_mode", + "default_email_mailing_list_mode_frequency", + "default_email_previous_replies", + "default_email_in_reply_to", + "default_hide_profile", + "default_hide_presence", + "default_other_new_topic_duration_minutes", + "default_other_auto_track_topics_after_msecs", + "default_other_notification_level_when_replying", + "default_other_external_links_in_new_tab", + "default_other_enable_quoting", + "default_other_enable_smart_lists", + "default_other_enable_defer", + "default_other_dynamic_favicon", + "default_other_like_notification_frequency", + "default_other_skip_new_user_tips", + "default_topics_automatic_unpin", + "default_categories_watching", + "default_categories_tracking", + "default_categories_muted", + "default_categories_watching_first_post", + "default_categories_normal", + "default_tags_watching", + "default_tags_tracking", + "default_tags_muted", + "default_tags_watching_first_post", + "default_text_size", + "default_title_count_mode", + "default_navigation_menu_categories", + "default_navigation_menu_tags", + "default_sidebar_link_to_filtered_list", + "default_sidebar_show_count_of_new_items", +]; + export const MAX_UNOPTIMIZED_CATEGORIES = 1000; export const USER_FIELD_FLAGS = [ diff --git a/app/controllers/admin/config/about_controller.rb b/app/controllers/admin/config/about_controller.rb index ccb6ff2f99f..e760333f0b7 100644 --- a/app/controllers/admin/config/about_controller.rb +++ b/app/controllers/admin/config/about_controller.rb @@ -5,41 +5,62 @@ class Admin::Config::AboutController < Admin::AdminController end def update - settings_map = {} - if general_settings = params[:general_settings] - settings_map[:title] = general_settings[:name] - settings_map[:site_description] = general_settings[:summary] - settings_map[:about_banner_image] = general_settings[:about_banner_image] + settings = [] - settings_map[:extended_site_description] = general_settings[:extended_description] - settings_map[:short_site_description] = general_settings[:community_title] - if settings_map[:extended_site_description].present? - settings_map[:extended_site_description_cooked] = PrettyText.markdown( - settings_map[:extended_site_description], - ) + if general_settings = params[:general_settings] + settings << { setting_name: "title", value: general_settings[:name] } + settings << { setting_name: "site_description", value: general_settings[:summary] } + settings << { + setting_name: "about_banner_image", + value: general_settings[:about_banner_image], + } + + settings << { + setting_name: "extended_site_description", + value: general_settings[:extended_description], + } + settings << { + setting_name: "short_site_description", + value: general_settings[:community_title], + } + + if general_settings[:extended_description].present? + settings << { + setting_name: "extended_site_description_cooked", + value: PrettyText.markdown(general_settings[:extended_description]), + } else - settings_map[:extended_site_description_cooked] = "" + settings << { setting_name: "extended_site_description_cooked", value: "" } end end if contact_information = params[:contact_information] - settings_map[:community_owner] = contact_information[:community_owner] - settings_map[:contact_email] = contact_information[:contact_email] - settings_map[:contact_url] = contact_information[:contact_url] - settings_map[:site_contact_username] = contact_information[:contact_username] - settings_map[:site_contact_group_name] = contact_information[:contact_group_name] + settings << { setting_name: "community_owner", value: contact_information[:community_owner] } + settings << { setting_name: "contact_email", value: contact_information[:contact_email] } + settings << { setting_name: "contact_url", value: contact_information[:contact_url] } + settings << { + setting_name: "site_contact_username", + value: contact_information[:contact_username], + } + settings << { + setting_name: "site_contact_group_name", + value: contact_information[:contact_group_name], + } end if your_organization = params[:your_organization] - settings_map[:company_name] = your_organization[:company_name] - settings_map[:governing_law] = your_organization[:governing_law] - settings_map[:city_for_disputes] = your_organization[:city_for_disputes] + settings << { setting_name: "company_name", value: your_organization[:company_name] } + settings << { setting_name: "governing_law", value: your_organization[:governing_law] } + settings << { + setting_name: "city_for_disputes", + value: your_organization[:city_for_disputes], + } end SiteSetting::Update.call( guardian:, params: { - settings: settings_map, + settings:, }, options: { allow_changing_hidden: %i[ diff --git a/app/controllers/admin/config/branding_controller.rb b/app/controllers/admin/config/branding_controller.rb index 4602d65b9a8..fc39d2ff070 100644 --- a/app/controllers/admin/config/branding_controller.rb +++ b/app/controllers/admin/config/branding_controller.rb @@ -4,27 +4,25 @@ class Admin::Config::BrandingController < Admin::AdminController end def logo - SiteSetting::Update.call( - guardian:, - params: { - settings: { - logo: params[:logo], - logo_dark: params[:logo_dark], - large_icon: params[:large_icon], - favicon: params[:favicon], - logo_small: params[:logo_small], - logo_small_dark: params[:logo_small_dark], - mobile_logo: params[:mobile_logo], - mobile_logo_dark: params[:mobile_logo_dark], - manifest_icon: params[:manifest_icon], - manifest_screenshots: params[:manifest_screenshots], - apple_touch_icon: params[:apple_touch_icon], - digest_logo: params[:digest_logo], - opengraph_image: params[:opengraph_image], - x_summary_large_image: params[:x_summary_large_image], - }, - }, - ) do + settings = + %i[ + logo + logo_dark + large_icon + favicon + logo_small + logo_small_dark + mobile_logo + mobile_logo_dark + manifest_icon + manifest_screenshots + apple_touch_icon + digest_logo + opengraph_image + x_summary_large_image + ].map { |setting| { setting_name: setting, value: params[setting] } } + + SiteSetting::Update.call(guardian:, params: { settings: }) do on_success { render json: success_json } on_failed_policy(:settings_are_visible) do |policy| raise Discourse::InvalidParameters, policy.reason diff --git a/app/controllers/admin/site_settings_controller.rb b/app/controllers/admin/site_settings_controller.rb index 1b109db4f20..b4cb36e982e 100644 --- a/app/controllers/admin/site_settings_controller.rb +++ b/app/controllers/admin/site_settings_controller.rb @@ -20,19 +20,12 @@ class Admin::SiteSettingsController < Admin::AdminController def update params.require(:id) id = params[:id] - update_existing_users = params[:update_existing_user].present? + backfill = params[:update_existing_user] - previous_value = value_or_default(SiteSetting.get(id)) if update_existing_users + settings = [{ setting_name: id, value: params[id], backfill: }] - SiteSetting::Update.call(params: { settings: { id => params[id] } }, guardian:) do - on_success do |params:| - if update_existing_users - params.settings.to_a.each do |setting_name, setting_value| - SiteSettingUpdateExistingUsers.call(setting_name.to_s, setting_value, previous_value) - end - end - render body: nil - end + SiteSetting::Update.call(params: { settings: }, guardian:) do + on_success { render body: nil } on_failed_policy(:settings_are_not_deprecated) do |policy| raise Discourse::InvalidParameters, policy.reason end diff --git a/app/models/site_setting.rb b/app/models/site_setting.rb index e5d58cbf59f..82c89c9c88d 100644 --- a/app/models/site_setting.rb +++ b/app/models/site_setting.rb @@ -16,6 +16,45 @@ class SiteSetting < ActiveRecord::Base trust_levels ] + DEFAULT_USER_PREFERENCES = %w[ + default_email_digest_frequency + default_include_tl0_in_digests + default_email_level + default_email_messages_level + default_email_mailing_list_mode + default_email_mailing_list_mode_frequency + default_email_previous_replies + default_email_in_reply_to + default_hide_profile + default_hide_presence + default_other_new_topic_duration_minutes + default_other_auto_track_topics_after_msecs + default_other_notification_level_when_replying + default_other_external_links_in_new_tab + default_other_enable_quoting + default_other_enable_smart_lists + default_other_enable_defer + default_other_dynamic_favicon + default_other_like_notification_frequency + default_other_skip_new_user_tips + default_topics_automatic_unpin + default_categories_watching + default_categories_tracking + default_categories_muted + default_categories_watching_first_post + default_categories_normal + default_tags_watching + default_tags_tracking + default_tags_muted + default_tags_watching_first_post + default_text_size + default_title_count_mode + default_navigation_menu_categories + default_navigation_menu_tags + default_sidebar_link_to_filtered_list + default_sidebar_show_count_of_new_items + ] + extend GlobalPath extend SiteSettingExtension diff --git a/app/services/site_setting/policy/settings_are_configurable.rb b/app/services/site_setting/policy/settings_are_configurable.rb index c7e8b5243e9..9749e8e98bf 100644 --- a/app/services/site_setting/policy/settings_are_configurable.rb +++ b/app/services/site_setting/policy/settings_are_configurable.rb @@ -4,7 +4,7 @@ class SiteSetting::Policy::SettingsAreConfigurable < Service::PolicyBase delegate :options, :params, to: :context def call - @unconfigurable_settings = params.settings.keys.select(&method(:validate_setting)) + @unconfigurable_settings = params.settings.map(&:name).select(&method(:validate_setting)) @unconfigurable_settings.empty? end diff --git a/app/services/site_setting/policy/settings_are_not_deprecated.rb b/app/services/site_setting/policy/settings_are_not_deprecated.rb index 1ca43e47b10..3a9f9aadb6d 100644 --- a/app/services/site_setting/policy/settings_are_not_deprecated.rb +++ b/app/services/site_setting/policy/settings_are_not_deprecated.rb @@ -5,9 +5,9 @@ class SiteSetting::Policy::SettingsAreNotDeprecated < Service::PolicyBase def call @hard_deprecations = - params.settings.keys.filter_map do |id| + params.settings.filter_map do |setting| SiteSettings::DeprecatedSettings::SETTINGS.find do |old_name, new_name, override, _| - if old_name.to_sym == id + if old_name.to_sym == setting.name if override options.overridden_setting_names[old_name.to_sym] = new_name break diff --git a/app/services/site_setting/policy/settings_are_unshadowed_globally.rb b/app/services/site_setting/policy/settings_are_unshadowed_globally.rb index 2c850618a9f..d987abacd91 100644 --- a/app/services/site_setting/policy/settings_are_unshadowed_globally.rb +++ b/app/services/site_setting/policy/settings_are_unshadowed_globally.rb @@ -4,7 +4,7 @@ class SiteSetting::Policy::SettingsAreUnshadowedGlobally < Service::PolicyBase delegate :options, :params, to: :context def call - @hidden_settings = params.settings.keys.select(&method(:validate_setting)) + @hidden_settings = params.settings.map(&:name).select(&method(:validate_setting)) @hidden_settings.empty? end diff --git a/app/services/site_setting/policy/settings_are_visible.rb b/app/services/site_setting/policy/settings_are_visible.rb index 4bb6ee9e87b..d210d56e623 100644 --- a/app/services/site_setting/policy/settings_are_visible.rb +++ b/app/services/site_setting/policy/settings_are_visible.rb @@ -4,7 +4,7 @@ class SiteSetting::Policy::SettingsAreVisible < Service::PolicyBase delegate :options, :params, to: :context def call - @hidden_settings = params.settings.keys.select(&method(:validate_setting)) + @hidden_settings = params.settings.map(&:name).select(&method(:validate_setting)) @hidden_settings.empty? end diff --git a/app/services/site_setting/policy/values_are_valid.rb b/app/services/site_setting/policy/values_are_valid.rb index 5d325696717..5a29379c077 100644 --- a/app/services/site_setting/policy/values_are_valid.rb +++ b/app/services/site_setting/policy/values_are_valid.rb @@ -15,10 +15,9 @@ class SiteSetting::Policy::ValuesAreValid < Service::PolicyBase private def validate_setting(setting) - setting_name, setting_value = setting - setting_type = SiteSetting.type_supervisor.get_type(setting_name) + setting_type = SiteSetting.type_supervisor.get_type(setting.name) begin - SiteSetting.type_supervisor.validate_value(setting_name, setting_type, setting_value) + SiteSetting.type_supervisor.validate_value(setting.name, setting_type, setting.value) nil rescue Discourse::InvalidParameters => e e.message diff --git a/app/services/site_setting/update.rb b/app/services/site_setting/update.rb index 97c02aa93bd..7a5956240d6 100644 --- a/app/services/site_setting/update.rb +++ b/app/services/site_setting/update.rb @@ -3,6 +3,8 @@ class SiteSetting::Update include Service::Base + Setting = Struct.new(:name, :value, :backfill, :change) + options do attribute :allow_changing_hidden, :array, default: [] attribute :overridden_setting_names, default: {} @@ -14,32 +16,40 @@ class SiteSetting::Update attribute :settings before_validation do - self.settings = self.settings.to_a.map { |key, value| [key.to_sym, value.to_s.strip] }.to_h + self.settings = + self.settings.to_a.map do |setting| + Setting.new( + setting[:setting_name].to_sym, + setting[:value].to_s.strip, + !!setting[:backfill], + nil, + ) + end end validates :settings, presence: true after_validation do self.settings = - self - .settings - .map do |setting_name, value| - value = - case SiteSetting.type_supervisor.get_type(setting_name) - when :integer - value.tr("^-0-9", "").to_i - when :file_size_restriction - value.tr("^0-9", "").to_i - when :uploaded_image_list - value.blank? ? "" : Upload.get_from_urls(value.split("|")).to_a - when :upload - Upload.get_from_url(value) || "" - else - value - end - [setting_name, value] - end - .to_h + self.settings.map do |setting| + raw_value = setting.value + + setting.value = + case SiteSetting.type_supervisor.get_type(setting.name) + when :integer + raw_value.tr("^-0-9", "").to_i + when :file_size_restriction + raw_value.tr("^0-9", "").to_i + when :uploaded_image_list + raw_value.blank? ? "" : Upload.get_from_urls(raw_value.split("|")).to_a + when :upload + Upload.get_from_url(raw_value) || "" + else + raw_value + end + + setting + end end end @@ -49,7 +59,11 @@ class SiteSetting::Update policy :settings_are_visible, class_name: SiteSetting::Policy::SettingsAreVisible policy :settings_are_configurable, class_name: SiteSetting::Policy::SettingsAreConfigurable policy :values_are_valid, class_name: SiteSetting::Policy::ValuesAreValid - transaction { step :save } + + transaction do + step :save + step :backfill + end private @@ -58,12 +72,29 @@ class SiteSetting::Update end def save(params:, options:, guardian:) - params.settings.each do |setting_name, value| - SiteSetting.set_and_log( - options.overridden_setting_names[setting_name] || setting_name, - value, - guardian.user, + params.settings.each do |setting| + setting.change = + SiteSetting.set_and_log( + options.overridden_setting_names[setting.name] || setting.name, + setting.value, + guardian.user, + ) + end + end + + def backfill(params:) + params.settings.each do |setting| + next if !setting.backfill || !default_user_preference?(setting) + + SiteSettingUpdateExistingUsers.call( + setting.name.to_s, + setting.change.new_value, + setting.change.previous_value, ) end end + + def default_user_preference?(setting) + SiteSetting::DEFAULT_USER_PREFERENCES.include?(setting.name.to_s) + end end diff --git a/lib/tasks/javascript.rake b/lib/tasks/javascript.rake index 07cd09c3c33..1bf1e3f3597 100644 --- a/lib/tasks/javascript.rake +++ b/lib/tasks/javascript.rake @@ -168,6 +168,8 @@ task "javascript:update_constants" => :environment do export const SITE_SETTING_REQUIRES_CONFIRMATION_TYPES = #{SiteSettings::TypeSupervisor::REQUIRES_CONFIRMATION_TYPES.to_json}; + export const DEFAULT_USER_PREFERENCES = #{SiteSetting::DEFAULT_USER_PREFERENCES.to_json}; + export const MAX_UNOPTIMIZED_CATEGORIES = #{CategoryList::MAX_UNOPTIMIZED_CATEGORIES}; export const USER_FIELD_FLAGS = #{UserField::FLAG_ATTRIBUTES}; diff --git a/spec/services/site_setting/update_spec.rb b/spec/services/site_setting/update_spec.rb index 01b7097944f..6d45b830644 100644 --- a/spec/services/site_setting/update_spec.rb +++ b/spec/services/site_setting/update_spec.rb @@ -10,7 +10,8 @@ RSpec.describe SiteSetting::Update do fab!(:admin) let(:params) { { settings: } } - let(:settings) { { setting_name => new_value } } + let(:settings) { [{ setting_name: setting_name, value: new_value, backfill: backfill }] } + let(:backfill) { false } let(:options) { { allow_changing_hidden: } } let(:dependencies) { { guardian: } } let(:setting_name) { :title } @@ -114,7 +115,12 @@ RSpec.describe SiteSetting::Update do end context "when one setting is having invalid value" do - let(:settings) { { title: "hello this is title", default_categories_watching: "999999" } } + let(:settings) do + [ + { setting_name: "title", value: "hello this is title" }, + { setting_name: "default_categories_watching", value: "999999" }, + ] + end it { is_expected.to fail_a_policy(:values_are_valid) } @@ -122,5 +128,21 @@ RSpec.describe SiteSetting::Update do expect { result }.not_to change { SiteSetting.title } end end + + context "when backfill is requested" do + let(:settings) do + [ + { setting_name: "default_hide_profile", value: true, backfill: true }, + { setting_name: "default_hide_presence", value: true, backfill: false }, + { setting_name: "title", value: true, backfill: true }, + ] + end + + it "calls the relevant class for backfill" do + SiteSettingUpdateExistingUsers.expects(:call).once.with("default_hide_profile", true, false) + + result + end + end end end