github-mirror/discourse
2
0
Fork 0
mirror of https://github.com/discourse/discourse.git synced 2025-06-06 01:34:39 +08:00
Code Issues Actions 1 Packages Projects Releases Wiki Activity

DEV: Move backfill into SiteSetting::Update service (#32037)

Browse Source 
Some site settings support backfilling if the user specified it. This works fine for singular site settings sent to the SiteSettingsController#update endpoint, but with bulk save we need to support this for a list of settings as well.

This change alters the params format for SiteSetting::Update.

It also moves the backfill logic into the service.
This commit is contained in:
Ted Johansson
2025-03-28 12:01:56 +08:00
committed by GitHub
parent 654e858df3
commit b40cbfcb76
14 changed files with 237 additions and 129 deletions
Download Patch File Download Diff File Expand all files Collapse all files

44
app/assets/javascripts/admin/addon/mixins/setting-component.js
View File

@ -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(),

39
app/assets/javascripts/discourse/app/lib/constants.js
View File

@ -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 = [

63
app/controllers/admin/config/about_controller.rb
View File

@ -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[

40
app/controllers/admin/config/branding_controller.rb
View File

@ -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

15
app/controllers/admin/site_settings_controller.rb
View File

@ -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

39
app/models/site_setting.rb
View File

@ -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

2
app/services/site_setting/policy/settings_are_configurable.rb
View File

@ -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

4
app/services/site_setting/policy/settings_are_not_deprecated.rb
View File

@ -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

2
app/services/site_setting/policy/settings_are_unshadowed_globally.rb
View File

@ -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

2
app/services/site_setting/policy/settings_are_visible.rb
View File

@ -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

5
app/services/site_setting/policy/values_are_valid.rb
View File

@ -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

83
app/services/site_setting/update.rb
View File

@ -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

2
lib/tasks/javascript.rake
View File

@ -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};

26
spec/services/site_setting/update_spec.rb
View File

@ -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