From 59839e428f3dc49213fedfd8b67acb9f2a5ec69a Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Thu, 11 Jan 2024 14:04:02 +0800 Subject: [PATCH] DEV: Add `skip_migrations` param when importing remote theme (#25218) Why this change? Importing theme with the `bundle` params is used mainly by `discourse_theme` CLI in the development environment. However, we do not want migrations to automatically run in the development environment and instead want the developer to be intentional about running theme migrations. As such, this commit adds support for a `skip_migrations` param when importing a theme with the `bundle` params. This commit also adds a `migrated` attribute for migrations theme fields to indicate whether a migrations theme field has been migrated or not. --- app/controllers/admin/themes_controller.rb | 8 +++- app/models/remote_theme.rb | 22 +++++++-- app/models/theme.rb | 10 ++-- app/serializers/theme_field_serializer.rb | 10 +++- spec/fixtures/themes/discourse-test-theme.zip | Bin 4734 -> 5947 bytes .../settings/0001-some-migration.js | 3 ++ spec/models/remote_theme_spec.rb | 2 +- spec/requests/admin/themes_controller_spec.rb | 45 ++++++++++++++---- 8 files changed, 80 insertions(+), 20 deletions(-) create mode 100644 spec/fixtures/themes/discourse-test-theme/migrations/settings/0001-some-migration.js 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 a6aed16c9b4ffe3d8cd478b32e418ac5ac097325..8baa5a7eee49b51ce37d882412fe644f0669557c 100644 GIT binary patch delta 2369 zcmbu9UuaTM9LJA;N;mV)O;@eh9Bt;c`Ii7sX^#Baa{e=n~XkV^YqgAjkI6Vhjc^CV$Z zAaD<~I)U91jzZNOf0$-XFJsC2c0p2wEDS6#%6WbDa!OT zl$quc*2%Ce&s&zh274oP>9j#ErryN2~fj&a_w*Hu=O$9!*AUuQ(Nw- zI$$PyY)yrbUFZz%xW*v&_i@cf=#9qF!`A)!8_9Hf_`Z5GWxlLTPGtIA!YMgp>t zqEgpjNzC7MQhtt~@^ZWe?&-j2MYoD!vt&CZ=`VKJ8?b9cc{uHtt(1Q5;S3hz zNdlLO_joX5RXeU^8ekU0m4vMNj~&9U^V0My$(ef18rcwE1iQeq7YKG6VOBpAya<^W z5NFaNrQ#01NWR%3orRn7o?J5`bTh%mGRs*mNgd|$YF)71A-c0m?Ra22e)vFG5gQw! zaAg2$R$A@aV3u#4n0p5#F~taGMPNdtwadKTJz%X(5QG}RRGT`AWuquv^6sz>1R>koNqg&O_=Ov6D2RYV%8mIi<9C@!VsUY5NwGe*IKmzd zIuP3?{t!U3Y_l4pFsC@SSki<#&{lvKP0!>E0SygNX6*4K%fJImP7ZG!85kxv3dvi9 zMsUCrQ-C)k6E4lFD4M;&nkUN(bDCr5M|ku6mal)rfmVtFu{M(aC5`i7`Z+*J4P*u^ zso`^j3X&Ob9pxB-9#F&L0fYl;?)YvK23jr%#D++kmoyfDG@FoT@h8T~4@DFv$Mdn_ zwY=^d-+O(a#lVne&_}V{g$Zha5T*kVjz3$j6uJ+n_aGw!gF1@-2TY)-n+!~!nEJ6t zJ!;4?vw-y5!;>m{3P8A_VwT$NKO77UKY>^W#SNigGk~cU(+LRe@m!Mfu<+MJ(Y~7( c>;+&7Rz!(>RyLrI8MuMan1zACln=xM0FYPzO8@`> 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