DEV: Partially refactor themes controller to services (#33141)

This commit starts to introduce services to replace actions in the
ThemesController. We will start with the low langing fruit:

* Create
* Destroy
* BulkDestroy
* GetTranslations

Endpoints like #import and #update will be much harder.
Also, the https://github.com/discourse/discourse-theme-creator plugin
overrides some of these controller actions directly, so we need
to be careful.
This commit is contained in:
Martin Brennan 2025-06-23 10:14:58 +10:00 committed by GitHub
parent 5902e1141c
commit 4b947d2404
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
13 changed files with 699 additions and 61 deletions

View File

@ -192,25 +192,19 @@ class Admin::ThemesController < Admin::AdminController
end
def create
ban_in_allowlist_mode!
@theme =
Theme.new(
name: theme_params[:name],
user_id: theme_user.id,
user_selectable: theme_params[:user_selectable] || false,
color_scheme_id: theme_params[:color_scheme_id],
component: [true, "true"].include?(theme_params[:component]),
)
set_fields
respond_to do |format|
if @theme.save
update_default_theme
log_theme_change(nil, @theme)
format.json { render json: serialize_data(@theme, ThemeSerializer), status: :created }
else
format.json { render json: @theme.errors, status: :unprocessable_entity }
Themes::Create.call(
params: theme_params.to_unsafe_h.merge(user_id: theme_user.id),
guardian:,
) do
on_success { |theme:| render json: serialize_data(theme, ThemeSerializer), status: :created }
on_failed_contract do |contract|
render json: failed_json.merge(errors: contract.errors.full_messages), status: 400
end
on_failed_policy(:ensure_remote_themes_are_not_allowlisted) { raise Discourse::InvalidAccess }
on_model_errors { |theme:| render json: theme.errors, status: :unprocessable_entity }
on_model_not_found(:theme) do |result|
raise Discourse::NotFound if !result.exception
render json: failed_json.merge(errors: result.exception.message), status: 400
end
end
end
@ -280,27 +274,23 @@ class Admin::ThemesController < Admin::AdminController
end
def destroy
raise Discourse::InvalidAccess if params[:id].to_i.negative?
@theme = Theme.find_by(id: params[:id])
raise Discourse::InvalidParameters.new(:id) unless @theme
StaffActionLogger.new(current_user).log_theme_destroy(@theme)
@theme.destroy
respond_to { |format| format.json { head :no_content } }
Themes::Destroy.call(service_params) do
on_success { render json: {}, status: :no_content }
on_failed_contract do |contract|
render json: failed_json.merge(errors: contract.errors.full_messages), status: 400
end
on_model_not_found(:theme) { raise Discourse::NotFound }
end
end
def bulk_destroy
params[:theme_ids] = params[:theme_ids].filter { |id| id.to_i.positive? }
themes = Theme.where(id: params[:theme_ids])
raise Discourse::InvalidParameters.new(:id) if themes.blank?
ActiveRecord::Base.transaction do
themes.each { |theme| StaffActionLogger.new(current_user).log_theme_destroy(theme) }
themes.destroy_all
Themes::BulkDestroy.call(service_params) do
on_success { render json: {}, status: :no_content }
on_failed_contract do |contract|
render json: failed_json.merge(errors: contract.errors.full_messages), status: 400
end
on_model_not_found(:themes) { raise Discourse::NotFound }
end
respond_to { |format| format.json { head :no_content } }
end
def show
@ -326,22 +316,14 @@ class Admin::ThemesController < Admin::AdminController
end
def get_translations
params.require(:locale)
if I18n.available_locales.exclude?(params[:locale].to_sym)
raise Discourse::InvalidParameters.new(:locale)
end
I18n.locale = params[:locale]
@theme = Theme.find_by(id: params[:id])
raise Discourse::InvalidParameters.new(:id) unless @theme
translations =
@theme.translations.map do |translation|
{ key: translation.key, value: translation.value, default: translation.default }
Themes::GetTranslations.call(service_params) do
on_success { |translations:| render(json: success_json.merge(translations:)) }
on_failed_contract do |contract|
render json: failed_json.merge(errors: contract.errors.full_messages), status: 400
end
render json: { translations: translations }, status: :ok
on_failed_policy(:validate_locale) { raise Discourse::InvalidParameters.new(:locale) }
on_model_not_found(:theme) { raise Discourse::NotFound }
end
end
def update_single_setting
@ -408,7 +390,7 @@ class Admin::ThemesController < Admin::AdminController
def update_default_theme
if theme_params.key?(:default)
is_default = theme_params[:default].to_s == "true"
if @theme.id == SiteSetting.default_theme_id && !is_default
if @theme.default? && !is_default
Theme.clear_default!
elsif is_default
@theme.set_default!

View File

@ -21,6 +21,12 @@ class Theme < ActiveRecord::Base
class SettingsMigrationError < StandardError
end
class InvalidFieldTargetError < StandardError
end
class InvalidFieldTypeError < StandardError
end
attr_accessor :child_components
attr_accessor :skip_child_components_update
@ -630,11 +636,21 @@ class Theme < ActiveRecord::Base
name = name.to_s
target_id = Theme.targets[target.to_sym]
raise "Unknown target #{target} passed to set field" unless target_id
if target_id.blank?
raise InvalidFieldTargetError.new("Unknown target #{target} passed to set field")
end
type_id ||=
type ? ThemeField.types[type.to_sym] : ThemeField.guess_type(name: name, target: target)
raise "Unknown type #{type} passed to set field" unless type_id
if type_id.blank?
if type.present?
raise InvalidFieldTypeError.new("Unknown type #{type} passed to set field")
else
raise InvalidFieldTypeError.new(
"No type could be guessed for field #{name} for target #{target}",
)
end
end
value ||= ""

View File

@ -3,7 +3,8 @@
class ThemeField < ActiveRecord::Base
MIGRATION_NAME_PART_MAX_LENGTH = 150
# This string is not 'secret'. It's just randomized to avoid accidental clashes with genuine theme field content.
# This string is not 'secret'. It's just randomized to avoid accidental
# clashes with genuine theme field content.
CSP_NONCE_PLACEHOLDER = "__CSP__NONCE__PLACEHOLDER__f72bff1b1768168a34ee092ce759f192__"
belongs_to :upload

View File

@ -0,0 +1,57 @@
# frozen_string_literal: true
# Destroys multiple themes and logs the staff action. Related records are destroyed
# by ActiveRecord dependent: :destroy. Cannot be used to destroy system themes.
#
# @example
# Themes::Destroy.call(
# guardian: guardian,
# params: {
# theme_ids: [theme_1.id, theme_2.id],
# }
# )
#
class Themes::BulkDestroy
include Service::Base
# @!method self.call(guardian:, params:)
# @param [guardian] guardian
# @param [hash] params
# @option params [array] :theme_ids The ids of the themes to destroy, must be positive integers.
# @return [service::base::context]
params do
attribute :theme_ids, :array
validates :theme_ids, presence: true, length: { minimum: 1, maximum: 50 }
validate :theme_ids_must_be_positive, if: -> { theme_ids.present? }
before_validation { self.theme_ids = theme_ids.map(&:to_i).uniq if theme_ids.present? }
def theme_ids_must_be_positive
return if theme_ids.all?(&:positive?)
errors.add(:theme_ids, I18n.t("errors.messages.must_all_be_positive"))
end
end
model :themes
transaction do
step :log_themes_destroy
step :destroy_themes
end
private
def fetch_themes(params:)
Theme.where(id: params.theme_ids)
end
def log_themes_destroy(themes:, guardian:)
staff_action_logger = StaffActionLogger.new(guardian.user)
themes.each { |theme| staff_action_logger.log_theme_destroy(theme) }
end
def destroy_themes(themes:)
themes.destroy_all
end
end

View File

@ -0,0 +1,85 @@
# frozen_string_literal: true
# Creates a new theme with the provided parameters. Themes can be created
# with various attributes including name, user selectability, color scheme,
# and theme fields.
#
# Also used to create theme components.
#
# The theme can optionally be set as the default theme, overriding SiteSetting.default_theme_id.
# The theme will then be used for all users on the site who haven't specifically set their
# theme preference.
#
# @example
# Themes::Create.call(
# guardian: guardian,
# params: {
# name: "My Theme",
# user_selectable: true,
# color_scheme_id: 1,
# component: false,
# theme_fields: [
# { name: "header", target: "common", value: "content", type_id: 1 }
# ],
# default: false
# }
# )
#
class Themes::Create
include Service::Base
# @!method self.call(guardian:, params:)
# @param [Guardian] guardian
# @param [Hash] params
# @option params String :name The name of the theme
# @option params Integer :user_id The ID of the user creating the theme
# @option params [Boolean] :user_selectable Whether the theme can be selected by users
# @option params [Integer] :color_scheme_id The ID of the color palette to use
# @option params [Boolean] :component Whether this is a theme component. These cannot be user_selectable or have a color_scheme_id
# @option params [Array] :theme_fields Array of theme field attributes
# @option params [Boolean] :default Whether to set this as the default theme
# @return [Service::Base::Context]
params do
attribute :name, :string
attribute :user_id, :integer
attribute :user_selectable, :boolean, default: false
attribute :color_scheme_id, :integer
attribute :component, :boolean, default: false
attribute :theme_fields, :array
attribute :default, :boolean, default: false
validates :name, presence: true
validates :user_id, presence: true
validates :theme_fields, length: { maximum: 100 }
end
policy :ensure_remote_themes_are_not_allowlisted
transaction do
model :theme, :create_theme
step :update_default_theme
step :log_theme_change
end
private
def ensure_remote_themes_are_not_allowlisted
Theme.allowed_remote_theme_ids.nil?
end
def create_theme(params:)
Theme.create(
params.slice(:name, :user_id, :user_selectable, :color_scheme_id, :component),
) { |theme| params.theme_fields.to_a.each { |field| theme.set_field(**field.symbolize_keys) } }
end
def update_default_theme(params:, theme:)
theme.set_default! if params.default
end
def log_theme_change(theme:, guardian:)
StaffActionLogger.new(guardian.user).log_theme_change(nil, theme)
end
end

View File

@ -0,0 +1,50 @@
# frozen_string_literal: true
# Destroys a theme and logs the staff action. Related records are destroyed
# by ActiveRecord dependent: :destroy. Cannot be used to destroy system themes.
#
# @example
# Themes::Destroy.call(
# guardian: guardian,
# params: {
# id: theme.id,
# }
# )
#
class Themes::Destroy
include Service::Base
# @!method self.call(guardian:, params:)
# @param [Guardian] guardian
# @param [Hash] params
# @option params [Integer] :id The ID of the theme to destroy, must be greater than 0.
# @return [Service::Base::Context]
params do
attribute :id, :integer
# Negative theme IDs are for system themes only, which cannot be destroyed.
validates :id, presence: true, numericality: { only_integer: true, greater_than: 0 }
end
model :theme
transaction do
step :destroy_theme
step :log_theme_destroy
end
private
def fetch_theme(params:)
Theme.find_by(id: params.id)
end
def destroy_theme(theme:)
theme.destroy
end
def log_theme_destroy(theme:, guardian:)
StaffActionLogger.new(guardian.user).log_theme_destroy(theme)
end
end

View File

@ -0,0 +1,50 @@
# frozen_string_literal: true
# Gets all of the translation overrides for a theme, which are defined
# in locale yml files for a theme. A ThemeField is created for each locale,
# which in turn creates ThemeTranslationOverride records.
#
# @example
# Themes::GetTranslations.call(
# guardian: guardian,
# params: {
# id: theme.id,
# locale: "en",
# }
# )
#
class Themes::GetTranslations
include Service::Base
params do
attribute :locale, :string
attribute :id, :integer
validates :id, presence: true
validates :locale, presence: true
validate :validate_locale, if: -> { locale.present? }
def validate_locale
return if I18n.available_locales.include?(locale.to_sym)
errors.add(:base, I18n.t("errors.messages.invalid_locale", invalid_locale: locale))
end
end
model :theme
model :translations
private
def fetch_theme(params:)
Theme.find_by(id: params.id)
end
def fetch_translations(theme:, params:)
I18n.with_locale(params.locale) do
theme.translations.map do |translation|
{ key: translation.key, value: translation.value, default: translation.default }
end
end
end
end

View File

@ -283,6 +283,7 @@ en:
format: ! "%{attribute} %{message}"
format_with_full_message: "<b>%{attribute}</b>: %{message}"
messages:
invalid_locale: "%{invalid_locale} is not a valid locale"
too_long_validation:
one: "is limited to %{count} character; you entered %{length}."
other: "is limited to %{count} characters; you entered %{length}."
@ -312,6 +313,7 @@ en:
less_than_or_equal_to: must be less than or equal to %{count}
not_a_number: is not a number
not_an_integer: must be an integer
must_all_be_positive: must all be positive
odd: must be odd
record_invalid: ! "Validation failed: %{errors}"
max_emojis: "can't have more than %{max_emojis_count} emoji"

View File

@ -573,7 +573,7 @@ RSpec.describe Admin::ThemesController do
context "when logged in as an admin" do
before { sign_in(admin) }
it "creates a theme" do
it "creates a theme and theme fields" do
post "/admin/themes.json",
params: {
theme: {
@ -589,6 +589,51 @@ RSpec.describe Admin::ThemesController do
expect(json["theme"]["theme_fields"].length).to eq(1)
expect(UserHistory.where(action: UserHistory.actions[:change_theme]).count).to eq(1)
end
it "can set a theme to default" do
post "/admin/themes.json", params: { theme: { name: "my test name", default: "true" } }
expect(response.status).to eq(201)
json = response.parsed_body
expect(json["theme"]["default"]).to eq(true)
end
context "when creating a theme field with an invalid target" do
it "errors" do
post "/admin/themes.json",
params: {
theme: {
name: "my test name",
theme_fields: [name: "scss", target: "blah", value: "body{color: red;}"],
},
}
expect(response.status).to eq(400)
json = response.parsed_body
expect(json["errors"]).to include("Unknown target blah passed to set field")
end
end
context "when creating a theme field with an invalid type" do
it "errors" do
post "/admin/themes.json",
params: {
theme: {
name: "my test name",
theme_fields: [name: "blahblah", target: "common", value: "body{color: red;}"],
},
}
expect(response.status).to eq(400)
json = response.parsed_body
expect(json["errors"]).to include(
"No type could be guessed for field blahblah for target common",
)
end
end
end
shared_examples "theme creation not allowed" do
@ -619,6 +664,20 @@ RSpec.describe Admin::ThemesController do
include_examples "theme creation not allowed"
end
context "when theme allowlist mode is enabled" do
before do
global_setting :allowed_theme_repos, " https://magic.com/repo.git, https://x.com/git"
end
it "prevents theme creation with 404 error" do
expect do
post "/admin/themes.json", params: { theme: { name: "my test name" } }
end.not_to change { Theme.count }
expect(response.status).to eq(404)
end
end
end
describe "#update" do
@ -1066,7 +1125,7 @@ RSpec.describe Admin::ThemesController do
it "returns the right response when an invalid id is given" do
delete "/admin/themes/9999.json"
expect(response.status).to eq(400)
expect(response.status).to eq(404)
end
it "deletes the field's javascript cache" do
@ -1401,7 +1460,7 @@ RSpec.describe Admin::ThemesController do
get "/admin/themes/#{theme.id}/translations/foo.json"
expect(response.status).to eq(400)
expect(response.parsed_body["errors"]).to include(
I18n.t("invalid_params", message: :locale),
I18n.t("errors.messages.invalid_locale", invalid_locale: "foo"),
)
end
end
@ -1438,19 +1497,24 @@ RSpec.describe Admin::ThemesController do
expect do
delete "/admin/themes/bulk_destroy.json", params: { theme_ids: theme_ids }
end.to change { Theme.count }.by(-2)
expect(response.status).to eq(204)
end
it "does not destroy if any theme is system" do
it "does not destroy any themes if any of them is a system theme" do
theme.update_columns(id: -10)
expect do
delete "/admin/themes/bulk_destroy.json", params: { theme_ids: theme_ids }
end.to change { Theme.count }.by(-1)
expect { theme_2.reload }.to raise_exception(ActiveRecord::RecordNotFound)
end.not_to change { Theme.count }
expect(response.status).to eq(400)
expect(response.parsed_body["errors"]).to eq(
["Theme ids " + I18n.t("errors.messages.must_all_be_positive")],
)
end
it "logs the theme destroy action for each theme" do
StaffActionLogger.any_instance.expects(:log_theme_destroy).twice
delete "/admin/themes/bulk_destroy.json", params: { theme_ids: theme_ids }
expect(response.status).to eq(204)
end
end

View File

@ -0,0 +1,54 @@
# frozen_string_literal: true
RSpec.describe Themes::BulkDestroy do
describe described_class::Contract, type: :model do
it { is_expected.to validate_presence_of(:theme_ids) }
it { is_expected.to allow_values([1], (1..50).to_a).for(:theme_ids) }
it { is_expected.not_to allow_values([], (1..51).to_a).for(:theme_ids) }
it do
is_expected.not_to allow_values([1, 0, -3]).for(:theme_ids).with_message(
/must all be positive/,
)
end
end
describe ".call" do
subject(:result) { described_class.call(params:, **dependencies) }
fab!(:theme_1) { Fabricate(:theme) }
fab!(:theme_2) { Fabricate(:theme) }
fab!(:admin)
let(:params) { { theme_ids: [theme_1.id, theme_2.id] } }
let(:dependencies) { { guardian: admin.guardian } }
context "when data is invalid" do
let(:params) { {} }
it { is_expected.to fail_a_contract }
end
context "when a theme does not exist" do
before do
theme_1.destroy!
theme_2.destroy!
end
it { is_expected.to fail_to_find_a_model(:themes) }
end
context "when everything is ok" do
it { is_expected.to run_successfully }
it "destroys the themes" do
expect { result }.to change { Theme.count }.by(-2)
end
it "logs the theme destroys" do
expect_any_instance_of(StaffActionLogger).to receive(:log_theme_destroy).with(theme_1).once
expect_any_instance_of(StaffActionLogger).to receive(:log_theme_destroy).with(theme_2).once
result
end
end
end
end

View File

@ -0,0 +1,156 @@
# frozen_string_literal: true
RSpec.describe Themes::Create do
describe described_class::Contract, type: :model do
it { is_expected.to validate_presence_of(:name) }
it { is_expected.to validate_presence_of(:user_id) }
it { is_expected.to validate_length_of(:theme_fields).as_array.is_at_least(0).is_at_most(100) }
end
describe ".call" do
subject(:result) { described_class.call(params:, **dependencies) }
fab!(:user)
fab!(:admin)
fab!(:guardian) { admin.guardian }
fab!(:color_scheme)
let(:dependencies) { { guardian: } }
let(:theme_params) do
{
name: "My Cool Theme",
user_id: admin.id,
user_selectable: true,
color_scheme_id: color_scheme.id,
component: false,
theme_fields: [
{
name: "header",
target: "common",
value: "header content",
type_id: ThemeField.types[:html],
},
],
default: false,
}
end
let(:params) { theme_params }
context "when remote themes are allowlisted" do
before do
GlobalSetting.stubs(:allowed_theme_repos).returns(
"https://github.com/discourse/sample-theme",
)
end
it { is_expected.to fail_a_policy(:ensure_remote_themes_are_not_allowlisted) }
end
context "with invalid theme field target" do
let(:params) do
theme_params.merge(
theme_fields: [
{ name: "header", target: "invalid_target", value: "header content", type: "html" },
],
)
end
it { is_expected.to fail_to_find_a_model(:theme) }
end
context "with invalid theme field type" do
let(:params) do
theme_params.merge(
theme_fields: [
{
name: "header",
target: "common",
value: "header content",
type: "blah", # Invalid type
},
],
)
end
it { is_expected.to fail_to_find_a_model(:theme) }
end
context "with invalid component model parameters" do
let(:params) { theme_params.merge(component: true) }
it "fails to create a component" do
expect(result).to fail_with_an_invalid_model(:theme)
expect(result.theme.errors.full_messages).to eq(
[
"Theme components can't have color palettes",
"Theme components can't be user-selectable",
],
)
end
end
context "when everything is ok" do
it { is_expected.to run_successfully }
it "creates a theme with the provided parameters" do
expect(result.theme).to have_attributes(
name: "My Cool Theme",
user_id: admin.id,
user_selectable: true,
color_scheme_id: color_scheme.id,
component: false,
theme_fields: [have_attributes(name: "header", value: "header content")],
)
end
it "logs the theme change" do
expect_any_instance_of(StaffActionLogger).to receive(:log_theme_change).with(
nil,
an_instance_of(Theme),
)
result
end
context "with component param" do
let(:params) do
theme_params.merge(component: true, user_selectable: false, color_scheme_id: nil)
end
it "creates a component" do
expect(result.theme).to be_a_component
end
end
context "with empty theme_fields" do
let(:params) { theme_params.except(:theme_fields) }
it "creates a theme without fields" do
expect(result.theme.theme_fields).to be_empty
end
end
context "when default param is true" do
let(:params) { theme_params.merge(default: true) }
it "sets the theme as default" do
expect(result.theme).to be_default
expect(SiteSetting.default_theme_id).to eq(result.theme.id)
end
context "when there is an existing default theme" do
fab!(:existing_default) { Fabricate(:theme) }
before { existing_default.set_default! }
it "clears the existing default theme" do
expect { result }.to change { existing_default.reload.default? }.to(false)
expect(result.theme).to be_default
expect(SiteSetting.default_theme_id).to eq(result.theme.id)
end
end
end
end
end
end

View File

@ -0,0 +1,44 @@
# frozen_string_literal: true
RSpec.describe Themes::Destroy do
describe described_class::Contract, type: :model do
it { is_expected.to validate_presence_of(:id) }
it { is_expected.to allow_values(1, "1", 42).for(:id) }
it { is_expected.not_to allow_values(-1, 0).for(:id) }
end
describe ".call" do
subject(:result) { described_class.call(params:, **dependencies) }
fab!(:theme)
fab!(:admin)
let(:params) { { id: theme.id } }
let(:dependencies) { { guardian: admin.guardian } }
context "when data is invalid" do
let(:params) { {} }
it { is_expected.to fail_a_contract }
end
context "for invalid theme id" do
before { theme.destroy! }
it { is_expected.to fail_to_find_a_model(:theme) }
end
context "when everything is ok" do
it { is_expected.to run_successfully }
it "destroys the theme" do
expect { result }.to change { Theme.find_by(id: theme.id) }.to(nil)
end
it "logs the theme destroy" do
expect_any_instance_of(StaffActionLogger).to receive(:log_theme_destroy).with(theme)
result
end
end
end
end

View File

@ -0,0 +1,77 @@
# frozen_string_literal: true
RSpec.describe Themes::GetTranslations do
describe described_class::Contract, type: :model do
it { is_expected.to validate_presence_of :id }
it { is_expected.to validate_presence_of :locale }
context "when locale is invalid" do
let(:locale) { "invalid_locale" }
it "invalidates the contract" do
contract = described_class.new(locale: locale)
contract.validate
expect(contract.errors.full_messages).to include(
I18n.t("errors.messages.invalid_locale", invalid_locale: locale),
)
end
end
end
describe ".call" do
subject(:result) { described_class.call(params:) }
fab!(:theme)
fab!(:locale_field_1) do
ThemeField.create!(
theme_id: theme.id,
name: "en",
type_id: ThemeField.types[:yaml],
target_id: Theme.targets[:translations],
value: <<~YAML,
en:
theme_metadata:
description: "Description of my theme"
skip_to_main_content: "Skip to main contentzz"
skip_user_nav: "Skip to profile contentzz"
YAML
)
end
let(:locale) { I18n.available_locales.first.to_s }
let(:params) { { id: theme.id, locale: locale } }
context "when data is invalid" do
let(:params) { {} }
it { is_expected.to fail_a_contract }
end
context "when theme doesn't exist" do
before { theme.destroy! }
it { is_expected.to fail_to_find_a_model(:theme) }
end
context "when everything is ok" do
it { is_expected.to run_successfully }
it "returns the theme translations" do
expect(result.translations).to eq(
[
{
key: "skip_to_main_content",
value: "Skip to main contentzz",
default: "Skip to main contentzz",
},
{
key: "skip_user_nav",
value: "Skip to profile contentzz",
default: "Skip to profile contentzz",
},
],
)
end
end
end
end