diff --git a/app/controllers/admin/themes_controller.rb b/app/controllers/admin/themes_controller.rb index e530540fab8..88e44ce0776 100644 --- a/app/controllers/admin/themes_controller.rb +++ b/app/controllers/admin/themes_controller.rb @@ -142,15 +142,19 @@ class Admin::ThemesController < Admin::AdminController bundle = params[:bundle] || params[:theme] theme_id = params[:theme_id] update_components = params[:components] + run_migrations = !params[:skip_migrations] + begin @theme = RemoteTheme.update_zipped_theme( bundle.path, bundle.original_filename, user: theme_user, - theme_id: theme_id, - update_components: update_components, + theme_id:, + update_components:, + run_migrations:, ) + log_theme_change(nil, @theme) render json: @theme, status: :created rescue RemoteTheme::ImportError => e diff --git a/app/models/remote_theme.rb b/app/models/remote_theme.rb index 774ab344400..620f5de16aa 100644 --- a/app/models/remote_theme.rb +++ b/app/models/remote_theme.rb @@ -70,13 +70,15 @@ class RemoteTheme < ActiveRecord::Base original_filename, user: Discourse.system_user, theme_id: nil, - update_components: nil + update_components: nil, + run_migrations: true ) update_theme( ThemeStore::ZipImporter.new(filename, original_filename), user:, theme_id:, update_components:, + run_migrations:, ) end @@ -91,7 +93,8 @@ class RemoteTheme < ActiveRecord::Base importer, user: Discourse.system_user, theme_id: nil, - update_components: nil + update_components: nil, + run_migrations: true ) importer.import! @@ -112,8 +115,14 @@ class RemoteTheme < ActiveRecord::Base remote_theme.remote_url = "" do_update_child_components = false + theme.transaction do - remote_theme.update_from_remote(importer, skip_update: true, already_in_transaction: true) + remote_theme.update_from_remote( + importer, + skip_update: true, + already_in_transaction: true, + run_migrations:, + ) if existing && update_components.present? && update_components != "none" child_components = child_components.map { |url| ThemeStore::GitImporter.new(url.strip).url } @@ -216,7 +225,8 @@ class RemoteTheme < ActiveRecord::Base importer = nil, skip_update: false, raise_if_theme_save_fails: true, - already_in_transaction: false + already_in_transaction: false, + run_migrations: true ) cleanup = false @@ -356,12 +366,14 @@ class RemoteTheme < ActiveRecord::Base update_theme_color_schemes(theme, theme_info["color_schemes"]) unless theme.component self.save! + if raise_if_theme_save_fails theme.save! else raise ActiveRecord::Rollback if !theme.save end - theme.migrate_settings(start_transaction: false) + + theme.migrate_settings(start_transaction: false) if run_migrations end if already_in_transaction diff --git a/app/models/theme.rb b/app/models/theme.rb index e471e14d36a..94c89d7f6d9 100644 --- a/app/models/theme.rb +++ b/app/models/theme.rb @@ -86,7 +86,7 @@ class Theme < ActiveRecord::Base :user, :color_scheme, :theme_translation_overrides, - theme_fields: :upload, + theme_fields: %i[upload theme_settings_migration], ) end @@ -130,6 +130,7 @@ class Theme < ActiveRecord::Base remove_from_cache! ColorScheme.hex_cache.clear + notify_theme_change(with_scheme: notify_with_scheme) if theme_setting_requests_refresh @@ -647,13 +648,14 @@ class Theme < ActiveRecord::Base def settings field = settings_field return [] unless field && field.error.nil? - settings = [] + ThemeSettingsParser .new(field) .load do |name, default, type, opts| settings << ThemeSettingsManager.create(name, default, type, self, opts) end + settings end @@ -847,6 +849,7 @@ class Theme < ActiveRecord::Base self.theme_settings.destroy_all final_result = results.last + final_result[:settings_after].each do |key, val| self.update_setting(key.to_sym, val) rescue Discourse::NotFound @@ -874,7 +877,8 @@ class Theme < ActiveRecord::Base record.calculate_diff(res[:settings_before], res[:settings_after]) record.save! end - self.save! + + self.reload end if start_transaction diff --git a/app/serializers/theme_field_serializer.rb b/app/serializers/theme_field_serializer.rb index 927b4305e25..19d0905e329 100644 --- a/app/serializers/theme_field_serializer.rb +++ b/app/serializers/theme_field_serializer.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class ThemeFieldSerializer < ApplicationSerializer - attributes :name, :target, :value, :error, :type_id, :upload_id, :url, :filename + attributes :name, :target, :value, :error, :type_id, :upload_id, :url, :filename, :migrated def include_url? object.upload @@ -34,4 +34,12 @@ class ThemeFieldSerializer < ApplicationSerializer def include_error? object.error.present? end + + def migrated + !!object.theme_settings_migration + end + + def include_migrated? + target == "migrations" + end end diff --git a/spec/fixtures/themes/discourse-test-theme.zip b/spec/fixtures/themes/discourse-test-theme.zip index a6aed16c9b4..8baa5a7eee4 100644 Binary files a/spec/fixtures/themes/discourse-test-theme.zip and b/spec/fixtures/themes/discourse-test-theme.zip differ diff --git a/spec/fixtures/themes/discourse-test-theme/migrations/settings/0001-some-migration.js b/spec/fixtures/themes/discourse-test-theme/migrations/settings/0001-some-migration.js new file mode 100644 index 00000000000..9c8e61826d1 --- /dev/null +++ b/spec/fixtures/themes/discourse-test-theme/migrations/settings/0001-some-migration.js @@ -0,0 +1,3 @@ +export default function migrate(settings) { + return settings; +} diff --git a/spec/models/remote_theme_spec.rb b/spec/models/remote_theme_spec.rb index 81b7fa47fcb..b3fcdccd1f8 100644 --- a/spec/models/remote_theme_spec.rb +++ b/spec/models/remote_theme_spec.rb @@ -494,7 +494,7 @@ RSpec.describe RemoteTheme do theme = RemoteTheme.import_theme_from_directory(theme_dir) expect(theme.name).to eq("Header Icons") - expect(theme.theme_fields.count).to eq(5) + expect(theme.theme_fields.count).to eq(6) end end end diff --git a/spec/requests/admin/themes_controller_spec.rb b/spec/requests/admin/themes_controller_spec.rb index 0277703fb29..4f24080cff1 100644 --- a/spec/requests/admin/themes_controller_spec.rb +++ b/spec/requests/admin/themes_controller_spec.rb @@ -243,6 +243,7 @@ RSpec.describe Admin::ThemesController do } JS ) + repo_url = MockGitImporter.register("https://example.com/initial_repo.git", repo_path) post "/admin/themes/import.json", params: { remote: repo_url } @@ -336,7 +337,7 @@ RSpec.describe Admin::ThemesController do json = response.parsed_body expect(json["theme"]["name"]).to eq("Header Icons") - expect(json["theme"]["theme_fields"].length).to eq(5) + expect(json["theme"]["theme_fields"].length).to eq(6) expect(json["theme"]["auto_update"]).to eq(false) expect(UserHistory.where(action: UserHistory.actions[:change_theme]).count).to eq(1) end @@ -347,30 +348,45 @@ RSpec.describe Admin::ThemesController do other_existing_theme = Fabricate(:theme, name: "Some other name") messages = - MessageBus.track_publish do + MessageBus.track_publish("/file-change") do expect do post "/admin/themes/import.json", params: { bundle: theme_archive, theme_id: other_existing_theme.id, } + + expect(response.status).to eq(201) end.not_to change { Theme.count } end - expect(response.status).to eq(201) + json = response.parsed_body # Ensure only one refresh message is sent. # More than 1 is wasteful, and can trigger unusual race conditions in the client # If this test fails, it probably means `theme.save` is being called twice - check any 'autosave' relations - file_change_messages = messages.filter { |m| m[:channel] == "/file-change" } - expect(file_change_messages.count).to eq(1) + expect(messages.count).to eq(1) expect(json["theme"]["name"]).to eq("Some other name") expect(json["theme"]["id"]).to eq(other_existing_theme.id) - expect(json["theme"]["theme_fields"].length).to eq(5) + expect(json["theme"]["theme_fields"].length).to eq(6) expect(UserHistory.where(action: UserHistory.actions[:change_theme]).count).to eq(1) end + it "does not run migrations when importing a theme from an archive and `skip_settings_migrations` params is present" do + other_existing_theme = Fabricate(:theme, name: "Some other name") + + post "/admin/themes/import.json", + params: { + bundle: theme_archive, + theme_id: other_existing_theme.id, + skip_migrations: true, + } + + expect(response.status).to eq(201) + expect(other_existing_theme.theme_settings_migrations.exists?).to eq(false) + end + it "creates a new theme when id specified as nil" do # Used by theme CLI existing_theme = Fabricate(:theme, name: "Header Icons") @@ -383,7 +399,7 @@ RSpec.describe Admin::ThemesController do expect(json["theme"]["name"]).to eq("Header Icons") expect(json["theme"]["id"]).not_to eq(existing_theme.id) - expect(json["theme"]["theme_fields"].length).to eq(5) + expect(json["theme"]["theme_fields"].length).to eq(6) expect(json["theme"]["auto_update"]).to eq(false) expect(UserHistory.where(action: UserHistory.actions[:change_theme]).count).to eq(1) end @@ -423,6 +439,12 @@ RSpec.describe Admin::ThemesController do theme.set_field(target: :common, name: :scss, value: ".body{color: black;}") theme.set_field(target: :desktop, name: :after_header, value: "test") + theme.set_field( + target: :migrations, + name: "0001-some-migration", + value: "export default function migrate(settings) { return settings; }", + ) + theme.remote_theme = RemoteTheme.new( remote_url: "awesome.git", @@ -444,7 +466,14 @@ RSpec.describe Admin::ThemesController do expect(json["extras"]["color_schemes"].length).to eq(1) theme_json = json["themes"].find { |t| t["id"] == theme.id } - expect(theme_json["theme_fields"].length).to eq(2) + expect(theme_json["theme_fields"].length).to eq(3) + + expect( + theme_json["theme_fields"].find { |theme_field| theme_field["target"] == "migrations" }[ + "migrated" + ], + ).to eq(false) + expect(theme_json["remote_theme"]["remote_version"]).to eq("7") end end