diff --git a/app/assets/javascripts/admin/addon/mixins/setting-component.js b/app/assets/javascripts/admin/addon/mixins/setting-component.js index e3945dd3a74..e47384fd4a5 100644 --- a/app/assets/javascripts/admin/addon/mixins/setting-component.js +++ b/app/assets/javascripts/admin/addon/mixins/setting-component.js @@ -5,9 +5,11 @@ import Mixin from "@ember/object/mixin"; import { service } from "@ember/service"; import { htmlSafe } from "@ember/template"; import { isNone } from "@ember/utils"; +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 { splitString } from "discourse/lib/utilities"; import { deepEqual } from "discourse-common/lib/object"; import discourseComputed, { bind } from "discourse-common/utils/decorators"; @@ -82,6 +84,7 @@ export default Mixin.create({ modal: service(), router: service(), site: service(), + dialog: service(), attributeBindings: ["setting.setting:data-setting"], classNameBindings: [":row", ":setting", "overridden", "typeClass"], validationMessage: null, @@ -204,10 +207,59 @@ export default Mixin.create({ } }, + confirmChanges(settingKey) { + return new Promise((resolve) => { + // Fallback is needed in case the setting does not have a custom confirmation + // prompt/confirm defined. + this.dialog.alert({ + message: I18n.t( + `admin.site_settings.requires_confirmation_messages.${settingKey}.prompt`, + { + translatedFallback: I18n.t( + "admin.site_settings.requires_confirmation_messages.default.prompt" + ), + } + ), + buttons: [ + { + label: I18n.t( + `admin.site_settings.requires_confirmation_messages.${settingKey}.confirm`, + { + translatedFallback: I18n.t( + "admin.site_settings.requires_confirmation_messages.default.confirm" + ), + } + ), + class: "btn-primary", + action: () => resolve(true), + }, + { + label: I18n.t("no_value"), + class: "btn-default", + action: () => resolve(false), + }, + ], + }); + }); + }, + @action async update() { const key = this.buffered.get("setting"); + let confirm = true; + if ( + this.buffered.get("requires_confirmation") === + SITE_SETTING_REQUIRES_CONFIRMATION_TYPES.simple + ) { + confirm = await this.confirmChanges(key); + } + + if (!confirm) { + this.cancel(); + return; + } + if (!DEFAULT_USER_PREFERENCES.includes(key)) { await this.save(); return; diff --git a/app/assets/javascripts/discourse/app/lib/constants.js b/app/assets/javascripts/discourse/app/lib/constants.js index 7cd9f15d8c1..bbc9f9fae81 100644 --- a/app/assets/javascripts/discourse/app/lib/constants.js +++ b/app/assets/javascripts/discourse/app/lib/constants.js @@ -84,9 +84,14 @@ export const TOPIC_VISIBILITY_REASONS = { export const SYSTEM_FLAG_IDS = { like: 2, notify_user: 6, - notify_moderators: 7, off_topic: 3, inappropriate: 4, spam: 8, illegal: 10, + notify_moderators: 7, +}; + +export const SITE_SETTING_REQUIRES_CONFIRMATION_TYPES = { + simple: "simple", + user_option: "user_option", }; diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 5e0ce5edf49..c5405be34cd 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -6795,6 +6795,22 @@ en: add_types_title: "Allow extensions %{types}" add_types_toast: "%{types} file types added" mandatory_group: "Group is mandatory" + requires_confirmation_messages: + default: + prompt: "Changing this setting may have far-reaching or unintended consequences for your site. Are you sure you want to proceed?" + confirm: "Yes, I'm sure" + min_password_length: + prompt: "You’re about to change your password policy. This will affect all users changing their passwords from now on. Are you sure you want to proceed?" + confirm: "Yes, update password policy" + min_admin_password_length: + prompt: "You’re about to change your password policy. This will affect all admins changing their passwords from now on. Are you sure you want to proceed?" + confirm: "Yes, update password policy" + password_unique_charactes: + prompt: "You’re about to change your password policy. This will affect all users changing their passwords from now on. Are you sure you want to proceed?" + confirm: "Yes, update password policy" + block_common_passwords: + prompt: "You’re about to change your password policy. This will affect all users changing their passwords from now on. Are you sure you want to proceed?" + confirm: "Yes, update password policy" badges: title: Badges diff --git a/config/site_settings.yml b/config/site_settings.yml index c77fb42eaaa..f35b9db2b6c 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -630,16 +630,21 @@ users: default: 10 min: 8 max: 500 + requires_confirmation: "simple" min_admin_password_length: client: true default: 15 min: 8 max: 500 + requires_confirmation: "simple" password_unique_characters: default: 6 min: 1 max: 10 - block_common_passwords: true + requires_confirmation: "simple" + block_common_passwords: + default: true + requires_confirmation: "simple" username_change_period: 3 email_editable: client: true diff --git a/lib/site_setting_extension.rb b/lib/site_setting_extension.rb index 3f890a61d40..d7b97eacf08 100644 --- a/lib/site_setting_extension.rb +++ b/lib/site_setting_extension.rb @@ -95,6 +95,10 @@ module SiteSettingExtension @shadowed_settings ||= [] end + def requires_confirmation_settings + @requires_confirmation_settings ||= {} + end + def hidden_settings_provider @hidden_settings_provider ||= SiteSettings::HiddenProvider.new end @@ -244,6 +248,7 @@ module SiteSettingExtension secret: secret_settings.include?(s), placeholder: placeholder(s), mandatory_values: mandatory_values[s], + requires_confirmation: requires_confirmation_settings[s], }.merge!(type_hash) opts[:plugin] = plugins[s] if plugins[s] @@ -654,6 +659,14 @@ module SiteSettingExtension mandatory_values[name] = opts[:mandatory_values] if opts[:mandatory_values] + requires_confirmation_settings[name] = ( + if SiteSettings::TypeSupervisor::REQUIRES_CONFIRMATION_TYPES.values.include?( + opts[:requires_confirmation], + ) + opts[:requires_confirmation] + end + ) + categories[name] = opts[:category] || :uncategorized hidden_settings_provider.add_hidden(name) if opts[:hidden] diff --git a/lib/site_settings/type_supervisor.rb b/lib/site_settings/type_supervisor.rb index 1b6f6c125ee..5d2a458ef1e 100644 --- a/lib/site_settings/type_supervisor.rb +++ b/lib/site_settings/type_supervisor.rb @@ -20,12 +20,15 @@ class SiteSettings::TypeSupervisor list_type textarea json_schema + requires_confirmation ].freeze VALIDATOR_OPTS = %i[min max regex hidden regex_error json_schema].freeze # For plugins, so they can tell if a feature is supported SUPPORTED_TYPES = %i[email username list enum].freeze + REQUIRES_CONFIRMATION_TYPES = { simple: "simple", user_option: "user_option" }.freeze + def self.types @types ||= Enum.new( diff --git a/lib/tasks/javascript.rake b/lib/tasks/javascript.rake index ba68b791aa0..96a36e7d52c 100644 --- a/lib/tasks/javascript.rake +++ b/lib/tasks/javascript.rake @@ -166,6 +166,8 @@ task "javascript:update_constants" => :environment do export const TOPIC_VISIBILITY_REASONS = #{Topic.visibility_reasons.to_json}; export const SYSTEM_FLAG_IDS = #{PostActionType.types.to_json} + + export const SITE_SETTING_REQUIRES_CONFIRMATION_TYPES = #{SiteSettings::TypeSupervisor::REQUIRES_CONFIRMATION_TYPES.to_json} JS pretty_notifications = Notification.types.map { |n| " #{n[0]}: #{n[1]}," }.join("\n") diff --git a/spec/lib/site_setting_extension_spec.rb b/spec/lib/site_setting_extension_spec.rb index 44ac6fb7af6..8aaa1fb45ff 100644 --- a/spec/lib/site_setting_extension_spec.rb +++ b/spec/lib/site_setting_extension_spec.rb @@ -892,6 +892,24 @@ RSpec.describe SiteSettingExtension do end end + describe "requires_confirmation settings" do + it "returns 'simple' for settings that require confirmation with 'simple' type" do + expect( + SiteSetting.all_settings.find { |s| s[:setting] == :min_password_length }[ + :requires_confirmation + ], + ).to eq("simple") + end + + it "returns nil for settings that do not require confirmation" do + expect( + SiteSetting.all_settings.find { |s| s[:setting] == :display_local_time_in_user_card }[ + :requires_confirmation + ], + ).to eq(nil) + end + end + describe "_map extension for list settings" do it "handles splitting group_list settings" do SiteSetting.personal_message_enabled_groups = "1|2" diff --git a/spec/system/admin_site_setting_requires_confirmation_spec.rb b/spec/system/admin_site_setting_requires_confirmation_spec.rb new file mode 100644 index 00000000000..74d883f0fcd --- /dev/null +++ b/spec/system/admin_site_setting_requires_confirmation_spec.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +describe "Admin Site Setting Requires Confirmation", type: :system do + let(:settings_page) { PageObjects::Pages::AdminSiteSettings.new } + let(:dialog) { PageObjects::Components::Dialog.new } + fab!(:admin) + + before do + SiteSetting.min_password_length = 10 + sign_in(admin) + end + + it "requires confirmation and shows the correct message" do + settings_page.visit("min_password_length") + settings_page.change_number_setting("min_password_length", 12) + expect(dialog).to be_open + expect(dialog).to have_content( + I18n.t( + "admin_js.admin.site_settings.requires_confirmation_messages.min_password_length.prompt", + ), + ) + expect(dialog).to have_content( + I18n.t( + "admin_js.admin.site_settings.requires_confirmation_messages.min_password_length.confirm", + ), + ) + dialog.click_yes + expect(dialog).to be_closed + expect(SiteSetting.min_password_length).to eq(12) + end + + it "does not save the new setting value if the admin cancels confirmation" do + settings_page.visit("min_password_length") + settings_page.change_number_setting("min_password_length", 12) + expect(dialog).to be_open + dialog.click_no + expect(dialog).to be_closed + expect(SiteSetting.min_password_length).to eq(10) + end +end diff --git a/spec/system/admin_site_setting_search_spec.rb b/spec/system/admin_site_setting_search_spec.rb index 75c765d58d8..7fec0b12f46 100644 --- a/spec/system/admin_site_setting_search_spec.rb +++ b/spec/system/admin_site_setting_search_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true describe "Admin Site Setting Search", type: :system do - let(:settings_page) { PageObjects::Pages::AdminSettings.new } + let(:settings_page) { PageObjects::Pages::AdminSiteSettings.new } fab!(:admin) before do diff --git a/spec/system/emojis/emoji_deny_list_spec.rb b/spec/system/emojis/emoji_deny_list_spec.rb index f13bb6ee895..90900bde268 100644 --- a/spec/system/emojis/emoji_deny_list_spec.rb +++ b/spec/system/emojis/emoji_deny_list_spec.rb @@ -10,7 +10,7 @@ describe "Emoji deny list", type: :system do describe "when editing admin settings" do before { SiteSetting.emoji_deny_list = "" } - let(:site_settings_page) { PageObjects::Pages::AdminSettings.new } + let(:site_settings_page) { PageObjects::Pages::AdminSiteSettings.new } skip "should allow admin to update emoji deny list" do site_settings_page.visit_category("posting") diff --git a/spec/system/page_objects/pages/admin_settings.rb b/spec/system/page_objects/pages/admin_site_settings.rb similarity index 63% rename from spec/system/page_objects/pages/admin_settings.rb rename to spec/system/page_objects/pages/admin_site_settings.rb index a1b1eefc75a..6b4b70dd51c 100644 --- a/spec/system/page_objects/pages/admin_settings.rb +++ b/spec/system/page_objects/pages/admin_site_settings.rb @@ -2,14 +2,18 @@ module PageObjects module Pages - class AdminSettings < PageObjects::Pages::Base + class AdminSiteSettings < PageObjects::Pages::Base def visit_filtered_plugin_setting(filter) page.visit("/admin/site_settings/category/plugins?filter=#{filter}") self end - def visit - page.visit("/admin/site_settings") + def visit(filter = nil) + if filter.present? + page.visit("/admin/site_settings?filter=#{filter}") + else + page.visit("/admin/site_settings") + end self end @@ -18,17 +22,31 @@ module PageObjects self end + def find_setting(setting_name) + find(".admin-detail .row.setting[data-setting='#{setting_name}']") + end + def toggle_setting(setting_name, text = "") - setting = find(".admin-detail .row.setting[data-setting='#{setting_name}']") + setting = find_setting(setting_name) setting.find(".setting-value span", text: text).click - setting.find(".setting-controls button.ok").click + save_setting(setting) + end + + def change_number_setting(setting_name, value, save_changes = true) + setting = find_setting(setting_name) + setting.fill_in(with: value) + save_setting(setting) if save_changes end def select_from_emoji_list(setting_name, text = "", save_changes = true) setting = find(".admin-detail .row.setting[data-setting='#{setting_name}']") setting.find(".setting-value .value-list > .value button").click setting.find(".setting-value .emoji-picker .emoji[title='#{text}']").click - setting.find(".setting-controls button.ok").click if save_changes + save_setting(setting) if save_changes + end + + def save_setting(setting_element) + setting_element.find(".setting-controls button.ok").click end def values_in_list(setting_name) @@ -67,5 +85,10 @@ module PageObjects assert_selector(".admin-detail .row.setting", minimum: count) end end + + # TODO (martin) Remove this after discourse-topic-voting no longer + # relies on this, it was renamed to AdminSiteSettings. + class AdminSettings < PageObjects::Pages::AdminSiteSettings + end end end