diff --git a/app/controllers/admin/site_settings_controller.rb b/app/controllers/admin/site_settings_controller.rb index cb89bf844c4..03095795748 100644 --- a/app/controllers/admin/site_settings_controller.rb +++ b/app/controllers/admin/site_settings_controller.rb @@ -7,12 +7,15 @@ class Admin::SiteSettingsController < Admin::AdminController def index params.permit(:categories, :plugin) + params.permit(:filter_names, []) render_json_dump( site_settings: SiteSetting.all_settings( filter_categories: params[:categories], filter_plugin: params[:plugin], + filter_names: params[:filter_names], + include_locale_setting: params[:filter_names].blank?, ), ) end @@ -20,8 +23,8 @@ class Admin::SiteSettingsController < Admin::AdminController def update params.require(:id) id = params[:id] + update_existing_users = params[:update_existing_user].present? value = params[id] - value.strip! if value.is_a?(String) new_setting_name = SiteSettings::DeprecatedSettings::SETTINGS.find do |old_name, new_name, override, _| @@ -37,127 +40,25 @@ class Admin::SiteSettingsController < Admin::AdminController id = new_setting_name if new_setting_name - raise_access_hidden_setting(id) - - case SiteSetting.type_supervisor.get_type(id) - when :integer - value = value.tr("^-0-9", "") - when :file_size_restriction - value = value.tr("^0-9", "").to_i - when :uploaded_image_list - value = value.blank? ? "" : Upload.get_from_urls(value.split("|")).to_a - end - - value = Upload.get_from_url(value) || "" if SiteSetting.type_supervisor.get_type(id) == :upload - - update_existing_users = params[:update_existing_user].present? previous_value = value_or_default(SiteSetting.get(id)) if update_existing_users - SiteSetting.set_and_log(id, value, current_user) + with_service(UpdateSiteSetting, setting_name: id, new_value: value) do + on_success do + value = result.new_value + SiteSettingUpdateExistingUsers.call(id, value, previous_value) if update_existing_users - if update_existing_users - new_value = value_or_default(value) + render body: nil + end - if (user_option = user_options[id.to_sym]).present? - if user_option == "text_size_key" - previous_value = UserOption.text_sizes[previous_value.to_sym] - new_value = UserOption.text_sizes[new_value.to_sym] - elsif user_option == "title_count_mode_key" - previous_value = UserOption.title_count_modes[previous_value.to_sym] - new_value = UserOption.title_count_modes[new_value.to_sym] - end + on_failed_policy(:setting_is_visible) do + raise Discourse::InvalidParameters, I18n.t("errors.site_settings.site_setting_is_hidden") + end - attrs = { user_option => new_value } - attrs[:email_digests] = (new_value.to_i != 0) if id == "default_email_digest_frequency" - - UserOption.human_users.where(user_option => previous_value).update_all(attrs) - elsif id.start_with?("default_categories_") - previous_category_ids = previous_value.split("|") - new_category_ids = new_value.split("|") - - notification_level = category_notification_level(id) - - categories_to_unwatch = previous_category_ids - new_category_ids - CategoryUser.where( - category_id: categories_to_unwatch, - notification_level: notification_level, - ).delete_all - TopicUser - .joins(:topic) - .where( - notification_level: TopicUser.notification_levels[:watching], - notifications_reason_id: TopicUser.notification_reasons[:auto_watch_category], - topics: { - category_id: categories_to_unwatch, - }, - ) - .update_all(notification_level: TopicUser.notification_levels[:regular]) - - (new_category_ids - previous_category_ids).each do |category_id| - skip_user_ids = CategoryUser.where(category_id: category_id).pluck(:user_id) - - User - .real - .where(staged: false) - .where.not(id: skip_user_ids) - .select(:id) - .find_in_batches do |users| - category_users = [] - users.each do |user| - category_users << { - category_id: category_id, - user_id: user.id, - notification_level: notification_level, - } - end - CategoryUser.insert_all!(category_users) - end - end - elsif id.start_with?("default_tags_") - previous_tag_ids = Tag.where(name: previous_value.split("|")).pluck(:id) - new_tag_ids = Tag.where(name: new_value.split("|")).pluck(:id) - now = Time.zone.now - - notification_level = tag_notification_level(id) - - TagUser.where( - tag_id: (previous_tag_ids - new_tag_ids), - notification_level: notification_level, - ).delete_all - - (new_tag_ids - previous_tag_ids).each do |tag_id| - skip_user_ids = TagUser.where(tag_id: tag_id).pluck(:user_id) - - User - .real - .where(staged: false) - .where.not(id: skip_user_ids) - .select(:id) - .find_in_batches do |users| - tag_users = [] - users.each do |user| - tag_users << { - tag_id: tag_id, - user_id: user.id, - notification_level: notification_level, - created_at: now, - updated_at: now, - } - end - TagUser.insert_all!(tag_users) - end - end - elsif is_sidebar_default_setting?(id) - Jobs.enqueue( - :backfill_sidebar_site_settings, - setting_name: id, - previous_value: previous_value, - new_value: new_value, - ) + on_failed_policy(:setting_is_configurable) do + raise Discourse::InvalidParameters, + I18n.t("errors.site_settings.site_setting_is_unconfigurable") end end - - render body: nil end def user_count @@ -170,7 +71,7 @@ class Admin::SiteSettingsController < Admin::AdminController previous_value = value_or_default(SiteSetting.public_send(id)) json = {} - if (user_option = user_options[id.to_sym]).present? + if (user_option = SiteSettingUpdateExistingUsers.user_options[id.to_sym]).present? if user_option == "text_size_key" previous_value = UserOption.text_sizes[previous_value.to_sym] elsif user_option == "title_count_mode_key" @@ -182,7 +83,7 @@ class Admin::SiteSettingsController < Admin::AdminController previous_category_ids = previous_value.split("|") new_category_ids = new_value.split("|") - notification_level = category_notification_level(id) + notification_level = SiteSettingUpdateExistingUsers.category_notification_level(id) user_ids = CategoryUser @@ -210,7 +111,7 @@ class Admin::SiteSettingsController < Admin::AdminController previous_tag_ids = Tag.where(name: previous_value.split("|")).pluck(:id) new_tag_ids = Tag.where(name: new_value.split("|")).pluck(:id) - notification_level = tag_notification_level(id) + notification_level = SiteSettingUpdateExistingUsers.tag_notification_level(id) user_ids = TagUser @@ -228,7 +129,7 @@ class Admin::SiteSettingsController < Admin::AdminController .pluck("users.id") json[:user_count] = user_ids.uniq.count - elsif is_sidebar_default_setting?(id) + elsif SiteSettingUpdateExistingUsers.is_sidebar_default_setting?(id) json[:user_count] = SidebarSiteSettingsBackfiller.new( id, previous_value: previous_value, @@ -241,38 +142,6 @@ class Admin::SiteSettingsController < Admin::AdminController private - def is_sidebar_default_setting?(setting_name) - %w[default_navigation_menu_categories default_navigation_menu_tags].include?(setting_name.to_s) - end - - def user_options - { - default_email_mailing_list_mode: "mailing_list_mode", - default_email_mailing_list_mode_frequency: "mailing_list_mode_frequency", - default_email_level: "email_level", - default_email_messages_level: "email_messages_level", - default_topics_automatic_unpin: "automatically_unpin_topics", - default_email_previous_replies: "email_previous_replies", - default_email_in_reply_to: "email_in_reply_to", - default_other_enable_quoting: "enable_quoting", - default_other_enable_defer: "enable_defer", - default_other_external_links_in_new_tab: "external_links_in_new_tab", - default_other_dynamic_favicon: "dynamic_favicon", - default_other_new_topic_duration_minutes: "new_topic_duration_minutes", - default_other_auto_track_topics_after_msecs: "auto_track_topics_after_msecs", - default_other_notification_level_when_replying: "notification_level_when_replying", - default_other_like_notification_frequency: "like_notification_frequency", - default_other_skip_new_user_tips: "skip_new_user_tips", - default_email_digest_frequency: "digest_after_minutes", - default_include_tl0_in_digests: "include_tl0_in_digests", - default_text_size: "text_size_key", - default_title_count_mode: "title_count_mode_key", - default_hide_profile_and_presence: "hide_profile_and_presence", - default_sidebar_link_to_filtered_list: "sidebar_link_to_filtered_list", - default_sidebar_show_count_of_new_items: "sidebar_show_count_of_new_items", - } - end - def raise_access_hidden_setting(id) id = id.to_sym @@ -285,34 +154,6 @@ class Admin::SiteSettingsController < Admin::AdminController end end - def tag_notification_level(id) - case id - when "default_tags_watching" - NotificationLevels.all[:watching] - when "default_tags_tracking" - NotificationLevels.all[:tracking] - when "default_tags_muted" - NotificationLevels.all[:muted] - when "default_tags_watching_first_post" - NotificationLevels.all[:watching_first_post] - end - end - - def category_notification_level(id) - case id - when "default_categories_watching" - NotificationLevels.all[:watching] - when "default_categories_tracking" - NotificationLevels.all[:tracking] - when "default_categories_muted" - NotificationLevels.all[:muted] - when "default_categories_watching_first_post" - NotificationLevels.all[:watching_first_post] - when "default_categories_normal" - NotificationLevels.all[:regular] - end - end - def value_or_default(value) value.nil? ? "" : value end diff --git a/app/services/site_setting_update_existing_users.rb b/app/services/site_setting_update_existing_users.rb new file mode 100644 index 00000000000..32a6fecb8f1 --- /dev/null +++ b/app/services/site_setting_update_existing_users.rb @@ -0,0 +1,165 @@ +# frozen_string_literal: true + +class SiteSettingUpdateExistingUsers + def self.call(id, value, previous_value) + new_value = value.nil? ? "" : value + + if (user_option = self.user_options[id.to_sym]).present? + if user_option == "text_size_key" + previous_value = UserOption.text_sizes[previous_value.to_sym] + new_value = UserOption.text_sizes[new_value.to_sym] + elsif user_option == "title_count_mode_key" + previous_value = UserOption.title_count_modes[previous_value.to_sym] + new_value = UserOption.title_count_modes[new_value.to_sym] + end + + attrs = { user_option => new_value } + attrs[:email_digests] = (new_value.to_i != 0) if id == "default_email_digest_frequency" + + UserOption.human_users.where(user_option => previous_value).update_all(attrs) + elsif id.start_with?("default_categories_") + previous_category_ids = previous_value.split("|") + new_category_ids = new_value.split("|") + + notification_level = self.category_notification_level(id) + + categories_to_unwatch = previous_category_ids - new_category_ids + CategoryUser.where( + category_id: categories_to_unwatch, + notification_level: notification_level, + ).delete_all + TopicUser + .joins(:topic) + .where( + notification_level: TopicUser.notification_levels[:watching], + notifications_reason_id: TopicUser.notification_reasons[:auto_watch_category], + topics: { + category_id: categories_to_unwatch, + }, + ) + .update_all(notification_level: TopicUser.notification_levels[:regular]) + + (new_category_ids - previous_category_ids).each do |category_id| + skip_user_ids = CategoryUser.where(category_id: category_id).pluck(:user_id) + + User + .real + .where(staged: false) + .where.not(id: skip_user_ids) + .select(:id) + .find_in_batches do |users| + category_users = [] + users.each do |user| + category_users << { + category_id: category_id, + user_id: user.id, + notification_level: notification_level, + } + end + CategoryUser.insert_all!(category_users) + end + end + elsif id.start_with?("default_tags_") + previous_tag_ids = Tag.where(name: previous_value.split("|")).pluck(:id) + new_tag_ids = Tag.where(name: new_value.split("|")).pluck(:id) + now = Time.zone.now + + notification_level = self.tag_notification_level(id) + + TagUser.where( + tag_id: (previous_tag_ids - new_tag_ids), + notification_level: notification_level, + ).delete_all + + (new_tag_ids - previous_tag_ids).each do |tag_id| + skip_user_ids = TagUser.where(tag_id: tag_id).pluck(:user_id) + + User + .real + .where(staged: false) + .where.not(id: skip_user_ids) + .select(:id) + .find_in_batches do |users| + tag_users = [] + users.each do |user| + tag_users << { + tag_id: tag_id, + user_id: user.id, + notification_level: notification_level, + created_at: now, + updated_at: now, + } + end + TagUser.insert_all!(tag_users) + end + end + elsif self.is_sidebar_default_setting?(id) + Jobs.enqueue( + :backfill_sidebar_site_settings, + setting_name: id, + previous_value: previous_value, + new_value: new_value, + ) + end + end + + def self.user_options + { + default_email_mailing_list_mode: "mailing_list_mode", + default_email_mailing_list_mode_frequency: "mailing_list_mode_frequency", + default_email_level: "email_level", + default_email_messages_level: "email_messages_level", + default_topics_automatic_unpin: "automatically_unpin_topics", + default_email_previous_replies: "email_previous_replies", + default_email_in_reply_to: "email_in_reply_to", + default_other_enable_quoting: "enable_quoting", + default_other_enable_defer: "enable_defer", + default_other_external_links_in_new_tab: "external_links_in_new_tab", + default_other_dynamic_favicon: "dynamic_favicon", + default_other_new_topic_duration_minutes: "new_topic_duration_minutes", + default_other_auto_track_topics_after_msecs: "auto_track_topics_after_msecs", + default_other_notification_level_when_replying: "notification_level_when_replying", + default_other_like_notification_frequency: "like_notification_frequency", + default_other_skip_new_user_tips: "skip_new_user_tips", + default_email_digest_frequency: "digest_after_minutes", + default_include_tl0_in_digests: "include_tl0_in_digests", + default_text_size: "text_size_key", + default_title_count_mode: "title_count_mode_key", + default_hide_profile_and_presence: "hide_profile_and_presence", + default_sidebar_link_to_filtered_list: "sidebar_link_to_filtered_list", + default_sidebar_show_count_of_new_items: "sidebar_show_count_of_new_items", + } + end + + def self.category_notification_level(id) + case id + when "default_categories_watching" + NotificationLevels.all[:watching] + when "default_categories_tracking" + NotificationLevels.all[:tracking] + when "default_categories_muted" + NotificationLevels.all[:muted] + when "default_categories_watching_first_post" + NotificationLevels.all[:watching_first_post] + when "default_categories_normal" + NotificationLevels.all[:regular] + end + end + + def self.tag_notification_level(id) + case id + when "default_tags_watching" + NotificationLevels.all[:watching] + when "default_tags_tracking" + NotificationLevels.all[:tracking] + when "default_tags_muted" + NotificationLevels.all[:muted] + when "default_tags_watching_first_post" + NotificationLevels.all[:watching_first_post] + end + end + + def self.is_sidebar_default_setting?(setting_name) + %w[default_navigation_menu_categories default_navigation_menu_tags].include?(setting_name.to_s) + end +end diff --git a/app/services/update_site_setting.rb b/app/services/update_site_setting.rb new file mode 100644 index 00000000000..be896e4767d --- /dev/null +++ b/app/services/update_site_setting.rb @@ -0,0 +1,65 @@ +# frozen_string_literal: true + +class UpdateSiteSetting + include Service::Base + + policy :current_user_is_admin + + contract + + step :convert_name_to_sym + + policy :setting_is_visible + policy :setting_is_configurable + + step :cleanup_value + step :save + + class Contract + attribute :setting_name + attribute :new_value + attribute :allow_changing_hidden, :boolean, default: false + + validates :setting_name, presence: true + end + + private + + def convert_name_to_sym(setting_name:) + context.setting_name = setting_name.to_sym + end + + def current_user_is_admin(guardian:) + guardian.is_admin? + end + + def setting_is_visible(setting_name:) + context.allow_changing_hidden || !SiteSetting.hidden_settings.include?(setting_name) + end + + def setting_is_configurable(setting_name:) + return true if !SiteSetting.plugins[setting_name] + + Discourse.plugins_by_name[SiteSetting.plugins[setting_name]].configurable? + end + + def cleanup_value(setting_name:, new_value:) + new_value = new_value.strip if new_value.is_a?(String) + + case SiteSetting.type_supervisor.get_type(setting_name) + when :integer + new_value = new_value.tr("^-0-9", "").to_i if new_value.is_a?(String) + when :file_size_restriction + new_value = new_value.tr("^0-9", "").to_i if new_value.is_a?(String) + when :uploaded_image_list + new_value = new_value.blank? ? "" : Upload.get_from_urls(new_value.split("|")).to_a + when :upload + new_value = Upload.get_from_url(new_value) || "" + end + context.new_value = new_value + end + + def save(setting_name:, new_value:, guardian:) + SiteSetting.set_and_log(setting_name, new_value, guardian.user) + end +end diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index b32205169e5..44c92c69451 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -333,6 +333,8 @@ en: site_settings: invalid_site_setting: "No setting named '%{name}' exists" invalid_category_id: "You specified a category that does not exist" + site_setting_is_hidden: "You are not allowed to change hidden settings" + site_setting_is_unconfigurable: "You are not allowed to change unconfigurable settings" invalid_choice: one: "'%{name}' is an invalid choice." other: "'%{name}' are invalid choices." diff --git a/lib/site_setting_extension.rb b/lib/site_setting_extension.rb index 006f78f2dc8..3f890a61d40 100644 --- a/lib/site_setting_extension.rb +++ b/lib/site_setting_extension.rb @@ -185,7 +185,8 @@ module SiteSettingExtension include_locale_setting: true, only_overridden: false, filter_categories: nil, - filter_plugin: nil + filter_plugin: nil, + filter_names: nil ) locale_setting_hash = { setting: "default_locale", @@ -256,6 +257,13 @@ module SiteSettingExtension true end end + .select do |setting| + if filter_names + filter_names.include?(setting[:setting].to_s) + else + true + end + end .unshift(include_locale_setting && !only_overridden ? locale_setting_hash : nil) .compact end diff --git a/spec/requests/admin/site_settings_controller_spec.rb b/spec/requests/admin/site_settings_controller_spec.rb index 178b87b0803..6a4271132c2 100644 --- a/spec/requests/admin/site_settings_controller_spec.rb +++ b/spec/requests/admin/site_settings_controller_spec.rb @@ -20,6 +20,25 @@ RSpec.describe Admin::SiteSettingsController do expect(locale.length).to eq(1) end + + describe "the filter_names param" do + it "only returns settings that are specified in the filter_names param" do + get "/admin/site_settings.json", + params: { + filter_names: %w[title site_description notification_email], + } + + expect(response.status).to eq(200) + + json = response.parsed_body + expect(json["site_settings"].size).to eq(3) + expect(json["site_settings"].map { |s| s["setting"] }).to contain_exactly( + "title", + "site_description", + "notification_email", + ) + end + end end shared_examples "site settings inaccessible" do diff --git a/spec/services/update_site_setting_spec.rb b/spec/services/update_site_setting_spec.rb new file mode 100644 index 00000000000..7040b6dea19 --- /dev/null +++ b/spec/services/update_site_setting_spec.rb @@ -0,0 +1,71 @@ +# frozen_string_literal: true + +describe(UpdateSiteSetting) do + fab!(:admin) + + def call_service(name, value, user: admin, allow_changing_hidden: false) + described_class.call( + setting_name: name, + new_value: value, + guardian: user.guardian, + allow_changing_hidden:, + ) + end + + context "when setting_name is blank" do + it "fails the service contract" do + expect(call_service(nil, "blah whatever")).to fail_a_contract + + expect(call_service(:"", "blah whatever")).to fail_a_contract + end + end + + context "when a non-admin user tries to change a setting" do + it "fails the current_user_is_admin policy" do + expect(call_service(:title, "some new title", user: Fabricate(:moderator))).to fail_a_policy( + :current_user_is_admin, + ) + expect(SiteSetting.title).not_to eq("some new title") + end + end + + context "when the user changes a hidden setting" do + context "when allow_changing_hidden is false" do + it "fails the setting_is_visible policy" do + expect(call_service(:max_category_nesting, 3)).to fail_a_policy(:setting_is_visible) + expect(SiteSetting.max_category_nesting).not_to eq(3) + end + end + + context "when allow_changing_hidden is true" do + it "updates the specified setting" do + expect(call_service(:max_category_nesting, 3, allow_changing_hidden: true)).to be_success + expect(SiteSetting.max_category_nesting).to eq(3) + end + end + end + + context "when the user changes a visible setting" do + it "updates the specified setting" do + expect(call_service(:title, "hello this is title")).to be_success + expect(SiteSetting.title).to eq("hello this is title") + end + + it "cleans up the new setting value before using it" do + expect(call_service(:suggested_topics, "308viu")).to be_success + expect(SiteSetting.suggested_topics).to eq(308) + + expect(call_service(:max_image_size_kb, "8zf843")).to be_success + expect(SiteSetting.max_image_size_kb).to eq(8843) + end + + it "creates an entry in the staff action logs" do + expect do expect(call_service(:max_image_size_kb, 44_543)).to be_success end.to change { + UserHistory.where( + action: UserHistory.actions[:change_site_setting], + subject: "max_image_size_kb", + ).count + }.by(1) + end + end +end