From 243fcb6ffc039d8e4e73d7db3fec47b53715bd72 Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Fri, 3 May 2024 06:29:18 +0800 Subject: [PATCH] DEV: Introduce `run_theme_migration` spec helper in test environment (#26845) This commit introduces the `run_theme_migration` spec helper to allow theme developers to write RSpec tests for theme migrations. For example, this allows the following RSpec test to be written in themes: ``` RSpec.describe "0003-migrate-small-links-setting migration" do let!(:theme) { upload_theme_component } it "should set target property to `_blank` if previous target component is not valid or empty" do theme.theme_settings.create!( name: "small_links", theme: theme, data_type: ThemeSetting.types[:string], value: "some text, #|some text 2, #, invalid target", ) run_theme_migration(theme, "0003-migrate-small-links-setting") expect(theme.settings[:small_links].value).to eq( [ { "text" => "some text", "url" => "#", "target" => "_blank" }, { "text" => "some text 2", "url" => "#", "target" => "_blank" }, ], ) end end ``` This change is being introduced because we realised that writting just javascript tests for the migrations is insufficient since javascript tests do not ensure that the migrated theme settings can actually be successfully saved into the database. Hence, we are introduce this helper as a way for theme developers to write "end-to-end" migrations tests. --- app/models/theme.rb | 11 +++- app/models/theme_settings_migration.rb | 2 +- .../theme_settings_migrations_runner.rb | 11 ++-- spec/models/theme_spec.rb | 53 ++++++++++++++- spec/support/helpers.rb | 66 +++++++++++++++++++ spec/support/system_helpers.rb | 51 -------------- 6 files changed, 133 insertions(+), 61 deletions(-) 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