diff --git a/app/models/theme.rb b/app/models/theme.rb index 2fcd7c62417..88f794ad56b 100644 --- a/app/models/theme.rb +++ b/app/models/theme.rb @@ -857,10 +857,11 @@ class Theme < ActiveRecord::Base contents end - def migrate_settings(start_transaction: true) + def migrate_settings(start_transaction: true, fields: nil, allow_out_of_sequence_migration: false) block = -> do runner = ThemeSettingsMigrationsRunner.new(self) - results = runner.run + results = + runner.run(fields:, raise_error_on_out_of_sequence: !allow_out_of_sequence_migration) next if results.blank? @@ -893,8 +894,12 @@ class Theme < ActiveRecord::Base name: res[:name], theme_field_id: res[:theme_field_id], ) + record.calculate_diff(res[:settings_before], res[:settings_after]) - record.save! + + # If out of sequence migration is allowed we don't want to raise an error if the record is invalid due to version + # conflicts + allow_out_of_sequence_migration ? record.save : record.save! end self.reload diff --git a/app/models/theme_settings_migration.rb b/app/models/theme_settings_migration.rb index a93d11db670..3cae4b7e969 100644 --- a/app/models/theme_settings_migration.rb +++ b/app/models/theme_settings_migration.rb @@ -4,7 +4,7 @@ class ThemeSettingsMigration < ActiveRecord::Base belongs_to :theme belongs_to :theme_field - validates :theme_id, presence: true + validates :theme_id, presence: true, uniqueness: { scope: :version } validates :theme_field_id, presence: true validates :version, presence: true diff --git a/app/services/theme_settings_migrations_runner.rb b/app/services/theme_settings_migrations_runner.rb index 6a8c04307da..223ceea66f3 100644 --- a/app/services/theme_settings_migrations_runner.rb +++ b/app/services/theme_settings_migrations_runner.rb @@ -53,7 +53,7 @@ class ThemeSettingsMigrationsRunner } JS - private_constant :Migration, :MIGRATION_ENTRY_POINT_JS + private_constant :MIGRATION_ENTRY_POINT_JS def self.loader_js_lib_content @loader_js_lib_content ||= @@ -67,8 +67,8 @@ class ThemeSettingsMigrationsRunner @memory = memory end - def run - fields = lookup_pending_migrations_fields + def run(fields: nil, raise_error_on_out_of_sequence: true) + fields ||= lookup_pending_migrations_fields count = fields.count return [] if count == 0 @@ -80,12 +80,13 @@ class ThemeSettingsMigrationsRunner current_migration_version = @theme.theme_settings_migrations.order(version: :desc).pick(:version) + current_migration_version ||= -Float::INFINITY current_settings = lookup_overriden_settings migrations.map do |migration| - if migration.version <= current_migration_version + if migration.version <= current_migration_version && raise_error_on_out_of_sequence raise_error( "themes.import_error.migrations.out_of_sequence", name: migration.original_name, @@ -94,6 +95,7 @@ class ThemeSettingsMigrationsRunner end migrated_settings = execute(migration, current_settings) + results = { version: migration.version, name: migration.name, @@ -157,6 +159,7 @@ class ThemeSettingsMigrationsRunner .migration_fields .left_joins(:theme_settings_migration) .where(theme_settings_migration: { id: nil }) + .order(created_at: :asc) end def convert_fields_to_migrations(fields) diff --git a/spec/models/theme_spec.rb b/spec/models/theme_spec.rb index 9d77da0e167..9e8faf0dc1d 100644 --- a/spec/models/theme_spec.rb +++ b/spec/models/theme_spec.rb @@ -341,10 +341,10 @@ HTML expect(scss).to include("font-size:30px") # Escapes correctly. If not, compiling this would throw an exception - setting.value = <<~CSS + setting.value = <<~SCSS \#{$fakeinterpolatedvariable} andanothervalue 'withquotes'; margin: 0; - CSS + SCSS theme.set_field(target: :common, name: :scss, value: "body {font-size: quote($font-size)}") theme.save! @@ -1347,6 +1347,55 @@ HTML "deletions" => [{ "key" => "setting_that_will_be_removed", "val" => 1023 }], ) end + + it "does not raise an out of sequence error and does not create `ThemeSettingsMigration` record for out of sequence migration when `allow_out_of_sequence_migration` kwarg is set to true" do + second_migration_field = + Fabricate( + :migration_theme_field, + name: "0001-some-other-migration-name", + theme: theme, + value: <<~JS, + export default function migrate(settings) { + settings.set("integer_setting", 3); + return settings; + } + JS + version: 1, + ) + + expect do theme.migrate_settings end.to raise_error( + Theme::SettingsMigrationError, + /'0001-some-other-migration-name' is out of sequence/, + ) + + expect(theme.get_setting("integer_setting")).to eq(1) + + theme.migrate_settings(allow_out_of_sequence_migration: true) + + expect(theme.theme_settings_migrations.count).to eq(1) + expect(theme.theme_settings_migrations.first.theme_field_id).to eq(migration_field.id) + expect(theme.get_setting("integer_setting")).to eq(3) + end + + it "allows custom migration fields to be run by specifing the `fields` kwarg" do + expect do theme.migrate_settings(fields: []) end.not_to change { + theme.theme_settings_migrations.count + } + + second_migration_field = + Fabricate(:migration_theme_field, theme: theme, value: <<~JS, version: 2) + export default function migrate(settings) { + settings.set("integer_setting", 3); + return settings; + } + JS + + theme.migrate_settings(fields: [second_migration_field]) + + expect(theme.theme_settings_migrations.count).to eq(1) + expect(theme.theme_settings_migrations.first.theme_field_id).to eq(second_migration_field.id) + expect(theme.get_setting("integer_setting")).to eq(3) + end end describe "development experience" do diff --git a/spec/support/helpers.rb b/spec/support/helpers.rb index 3657886dd58..31d49528322 100644 --- a/spec/support/helpers.rb +++ b/spec/support/helpers.rb @@ -229,4 +229,70 @@ module Helpers ensure SearchIndexer.disable end + + # Uploads a theme from a directory. + # + # @param set_theme_as_default [Boolean] Whether to set the uploaded theme as the default theme for the site. Defaults to true. + # + # @return [Theme] The uploaded theme model given by `models/theme.rb`. + # + # @example Upload a theme and set it as default + # upload_theme("/path/to/theme") + def upload_theme(set_theme_as_default: true) + theme = RemoteTheme.import_theme_from_directory(theme_dir_from_caller) + + if theme.component + raise "Uploaded theme is a theme component, please use the `upload_theme_component` method instead." + end + + theme.set_default! if set_theme_as_default + theme + end + + # Uploads a theme component from a directory. + # + # @param parent_theme_id [Integer] The ID of the theme to add the theme component to. Defaults to `SiteSetting.default_theme_id`. + # + # @return [Theme] The uploaded theme model given by `models/theme.rb`. + # + # @example Upload a theme component + # upload_theme_component("/path/to/theme_component") + # + # @example Upload a theme component and add it to a specific theme + # upload_theme_component("/path/to/theme_component", parent_theme_id: 123) + def upload_theme_component(parent_theme_id: SiteSetting.default_theme_id) + theme = RemoteTheme.import_theme_from_directory(theme_dir_from_caller) + + if !theme.component + raise "Uploaded theme is not a theme component, please use the `upload_theme` method instead." + end + + Theme.find(parent_theme_id).child_themes << theme + theme + end + + # Runs named migration for a given theme. + # + # @params [Theme] theme The theme to run the migration for. + # @params [String] migration_name The name of the migration to run. + # + # @return [nil] + # + # @example + # run_theme_migration(theme, "0001-migrate-some-settings") + def run_theme_migration(theme, migration_name) + migration_theme_field = theme.theme_fields.find_by(name: migration_name) + theme.migrate_settings(fields: [migration_theme_field], allow_out_of_sequence_migration: true) + nil + end + + private + + def theme_dir_from_caller + caller.each do |line| + if (split = line.split(%r{/spec/*/.+_spec.rb})).length > 1 + return split.first + end + end + end end diff --git a/spec/support/system_helpers.rb b/spec/support/system_helpers.rb index 3eb1b10967c..22ab72411fe 100644 --- a/spec/support/system_helpers.rb +++ b/spec/support/system_helpers.rb @@ -22,47 +22,6 @@ module SystemHelpers expect(page).to have_content("Signed in to #{user.encoded_username} successfully") end - # Uploads a theme from a directory. - # - # @param set_theme_as_default [Boolean] Whether to set the uploaded theme as the default theme for the site. Defaults to true. - # - # @return [Theme] The uploaded theme model given by `models/theme.rb`. - # - # @example Upload a theme and set it as default - # upload_theme("/path/to/theme") - def upload_theme(set_theme_as_default: true) - theme = RemoteTheme.import_theme_from_directory(theme_dir_from_caller) - - if theme.component - raise "Uploaded theme is a theme component, please use the `upload_theme_component` method instead." - end - - theme.set_default! if set_theme_as_default - theme - end - - # Uploads a theme component from a directory. - # - # @param parent_theme_id [Integer] The ID of the theme to add the theme component to. Defaults to `SiteSetting.default_theme_id`. - # - # @return [Theme] The uploaded theme model given by `models/theme.rb`. - # - # @example Upload a theme component - # upload_theme_component("/path/to/theme_component") - # - # @example Upload a theme component and add it to a specific theme - # upload_theme_component("/path/to/theme_component", parent_theme_id: 123) - def upload_theme_component(parent_theme_id: SiteSetting.default_theme_id) - theme = RemoteTheme.import_theme_from_directory(theme_dir_from_caller) - - if !theme.component - raise "Uploaded theme is not a theme component, please use the `upload_theme` method instead." - end - - Theme.find(parent_theme_id).child_themes << theme - theme - end - def setup_system_test SiteSetting.login_required = false SiteSetting.has_login_hint = false @@ -205,14 +164,4 @@ module SystemHelpers ) end end - - private - - def theme_dir_from_caller - caller.each do |line| - if (split = line.split(%r{/spec/system/.+_spec.rb})).length > 1 - return split.first - end - end - end end