From 39dffcb657b8e0b11b3cc0c9a81d0b103814d78a Mon Sep 17 00:00:00 2001
From: Alan Guo Xiang Tan <gxtan1990@gmail.com>
Date: Mon, 5 Feb 2024 14:49:23 +0800
Subject: [PATCH] FIX: Update themes javascript cache after running themes
 migrations (#25564)

Why this change?

This is caused by a regression in
59839e428f3dc49213fedfd8b67acb9f2a5ec69a, where we stopped saving the
`Theme` object because it was unnecessary. However, it resulted in the
`after_save` callback not being called and hence
`Theme#update_javascript_cache!` not being called. As a result, some
sites were reporting that after runing a theme migration, the defaults
for the theme settings were used instead of the settings overrides
stored in the database.

What does this change do?

Add a call to `Theme#update_javascript_cache!` after running theme
migrations.
---
 app/models/theme.rb       |  3 +++
 spec/models/theme_spec.rb | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/app/models/theme.rb b/app/models/theme.rb
index 6fd63b03940..f3cadd0394f 100644
--- a/app/models/theme.rb
+++ b/app/models/theme.rb
@@ -172,7 +172,9 @@ class Theme < ActiveRecord::Base
       js_compiler = ThemeJavascriptCompiler.new(id, name)
       js_compiler.append_tree(all_extra_js)
       settings_hash = build_settings_hash
+
       js_compiler.prepend_settings(settings_hash) if settings_hash.present?
+
       javascript_cache || build_javascript_cache
       javascript_cache.update!(content: js_compiler.content, source_map: js_compiler.source_map)
     else
@@ -879,6 +881,7 @@ class Theme < ActiveRecord::Base
       end
 
       self.reload
+      self.update_javascript_cache!
     end
 
     if start_transaction
diff --git a/spec/models/theme_spec.rb b/spec/models/theme_spec.rb
index 43e225079bd..4ae29e3a6a5 100644
--- a/spec/models/theme_spec.rb
+++ b/spec/models/theme_spec.rb
@@ -1095,11 +1095,42 @@ HTML
       )
     end
 
+    it "updates the theme's javascript cache after running migration" do
+      theme.set_field(target: :extra_js, name: "test.js.es6", value: "const hello = 'world';")
+      theme.save!
+
+      expect(theme.javascript_cache.content).to include('"list_setting":"aa,bb"')
+
+      settings_field.update!(value: <<~YAML)
+        integer_setting: 1
+        list_setting:
+          default: aa|bb
+          type: list
+      YAML
+
+      migration_field.update!(value: <<~JS)
+      export default function migrate(settings) {
+        settings.set("list_setting", "zz|aa");
+        return settings;
+      }
+      JS
+
+      theme.reload
+      theme.migrate_settings
+
+      setting_record = theme.theme_settings.where(name: "list_setting").first
+
+      expect(setting_record.data_type).to eq(ThemeSetting.types[:list])
+      expect(setting_record.value).to eq("zz|aa")
+      expect(theme.javascript_cache.content).to include('"list_setting":"zz|aa"')
+    end
+
     it "allows changing a setting's type" do
       theme.update_setting(:list_setting, "zz,aa")
       theme.save!
 
       setting_record = theme.theme_settings.where(name: "list_setting").first
+
       expect(setting_record.data_type).to eq(ThemeSetting.types[:string])
       expect(setting_record.value).to eq("zz,aa")
 
@@ -1109,12 +1140,14 @@ HTML
           default: aa|bb
           type: list
       YAML
+
       migration_field.update!(value: <<~JS)
         export default function migrate(settings) {
           settings.set("list_setting", "zz|aa");
           return settings;
         }
       JS
+
       theme.reload
 
       theme.migrate_settings