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

@@ -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,
+            theme_id:,
-            update_components: update_components,
+            update_components:,
+            run_migrations:,
           )
+
         log_theme_change(nil, @theme)
         render json: @theme, status: :created
       rescue RemoteTheme::ImportError => e

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

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

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

spec/fixtures/themes/discourse-test-theme/migrations/settings/0001-some-migration.js

@@ -0,0 +1,3 @@
+export default function migrate(settings) {
+  return settings;
+}

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

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(messages.count).to eq(1)
-        expect(file_change_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: "<b>test</b>")
 
+        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