DEV: Allow typed objects theme settings to be saved via settings editor (#26100)

Why this change? On the `/admin/customize/themes/<:id>` route, we allow admins to edit all settings via a settings editor. Prior to this change, trying to edit and save a typed objects theme settings will result in an error on the server.
2024-03-11 08:42:12 +08:00 · 2024-03-11 08:42:12 +08:00 · 8d4f405da4
parent 17a60be189
commit 8d4f405da4
11 changed files with 171 additions and 34 deletions
--- a/app/assets/javascripts/admin/addon/adapters/theme.js
+++ b/app/assets/javascripts/admin/addon/adapters/theme.js
@ -1,4 +1,5 @@
 import RestAdapter from "discourse/adapters/rest";
+import ThemeSettings from "admin/models/theme-settings";

 export default class Theme extends RestAdapter {
  jsonMode = true;
@ -8,9 +9,11 @@ export default class Theme extends RestAdapter {

  afterFindAll(results) {
    let map = {};
+
    results.forEach((theme) => {
      map[theme.id] = theme;
    });
+
    results.forEach((theme) => {
      let mapped = theme.get("child_themes") || [];
      mapped = mapped.map((t) => map[t.id]);
@ -19,7 +22,15 @@ export default class Theme extends RestAdapter {
      let mappedParents = theme.get("parent_themes") || [];
      mappedParents = mappedParents.map((t) => map[t.id]);
      theme.set("parentThemes", mappedParents);
+
+      if (theme.settings) {
+        theme.set(
+          "settings",
+          theme.settings.map((setting) => ThemeSettings.create(setting))
+        );
+      }
    });
+
    return results;
  }
 }
--- a/app/assets/javascripts/admin/addon/components/theme-setting-editor.js
+++ b/app/assets/javascripts/admin/addon/components/theme-setting-editor.js
@ -1,17 +1,10 @@
-import { ajax } from "discourse/lib/ajax";
-import { url } from "discourse/lib/computed";
 import SiteSettingComponent from "./site-setting";

 export default class extends SiteSettingComponent {
-  @url("model.id", "/admin/themes/%@/setting") updateUrl;
-
  _save() {
-    return ajax(this.updateUrl, {
-      type: "PUT",
-      data: {
-        name: this.setting.setting,
-        value: this.get("buffered.value"),
-      },
-    });
+    return this.setting.updateSetting(
+      this.model.id,
+      this.get("buffered.value")
+    );
  }
 }
--- a/app/assets/javascripts/admin/addon/components/theme-settings-editor.js
+++ b/app/assets/javascripts/admin/addon/components/theme-settings-editor.js
@ -2,7 +2,6 @@ import Component from "@glimmer/component";
 import { tracked } from "@glimmer/tracking";
 import { action } from "@ember/object";
 import { service } from "@ember/service";
-import { ajax } from "discourse/lib/ajax";
 import I18n from "discourse-i18n";

 export default class ThemeSettingsEditor extends Component {
@ -52,6 +51,7 @@ export default class ThemeSettingsEditor extends Component {
    if (!this.theme) {
      return null;
    }
+
    return this.theme.settings.map((setting) => ({
      setting: setting.setting,
      value: setting.value,
@ -86,11 +86,14 @@ export default class ThemeSettingsEditor extends Component {
    this.saving = true;
    this.errors = [];
    this.success = "";
+
    if (!this.editedContent) {
      // no changes.
      return;
    }
+
    let newSettings = "";
+
    try {
      newSettings = JSON.parse(this.editedContent);
    } catch (e) {
@ -104,6 +107,7 @@ export default class ThemeSettingsEditor extends Component {
      this.saving = false;
      return;
    }
+
    if (!this.validateSettingsKeys(newSettings)) {
      this.errors = [
        ...this.errors,
@ -119,14 +123,17 @@ export default class ThemeSettingsEditor extends Component {
    const originalNames = this.theme
      ? this.theme.settings.map((setting) => setting.setting)
      : [];
+
    const newNames = newSettings.map((setting) => setting.setting);
    const deletedNames = originalNames.filter(
      (originalName) => !newNames.find((newName) => newName === originalName)
    );
+
    const addedNames = newNames.filter(
      (newName) =>
        !originalNames.find((originalName) => originalName === newName)
    );
+
    if (deletedNames.length) {
      this.errors = [
        ...this.errors,
@ -136,6 +143,7 @@ export default class ThemeSettingsEditor extends Component {
        },
      ];
    }
+
    if (addedNames.length) {
      this.errors = [
        ...this.errors,
@ -151,41 +159,49 @@ export default class ThemeSettingsEditor extends Component {
      return;
    }

-    const changedSettings = newSettings.filter((newSetting) => {
+    const changedSettings = [];
+
+    newSettings.forEach((newSetting) => {
      const originalSetting = this.theme.settings.find(
        (_originalSetting) => _originalSetting.setting === newSetting.setting
      );
-      return originalSetting.value !== newSetting.value;
+
+      if (originalSetting.value !== newSetting.value) {
+        changedSettings.push({
+          originalSetting,
+          value: newSetting.value,
+        });
+      }
    });
-    for (let setting of changedSettings) {
+
+    for (let changedSetting of changedSettings) {
      try {
-        await this.saveSetting(this.theme.id, setting);
+        await this.saveSetting(this.theme.id, changedSetting);
      } catch (err) {
        const errorObjects = JSON.parse(err.jqXHR.responseText).errors.map(
          (error) => ({
-            setting: setting.setting,
+            setting: changedSetting.originalSetting.setting,
            errorMessage: error,
          })
        );
+
        this.errors = [...this.errors, ...errorObjects];
      }
    }
+
    if (this.errors.length === 0) {
      this.editedContent = null;
    }
+
    this.saving = false;
    this.dialog.cancel();
    this.customizeThemeShowController.send("routeRefreshModel");
  }

-  async saveSetting(themeId, setting) {
-    const updateUrl = `/admin/themes/${themeId}/setting`;
-    return await ajax(updateUrl, {
-      type: "PUT",
-      data: {
-        name: setting.setting,
-        value: setting.value,
-      },
-    });
+  async saveSetting(themeId, changedSetting) {
+    return await changedSetting.originalSetting.updateSetting(
+      themeId,
+      changedSetting.value
+    );
  }
 }
--- a/app/assets/javascripts/admin/addon/controllers/admin-customize-themes-show.js
+++ b/app/assets/javascripts/admin/addon/controllers/admin-customize-themes-show.js
@ -6,6 +6,7 @@ import {
  mapBy,
  match,
  notEmpty,
+  readOnly,
 } from "@ember/object/computed";
 import { service } from "@ember/service";
 import { popupAjaxError } from "discourse/lib/ajax-error";
@ -43,6 +44,7 @@ export default class AdminCustomizeThemesShowController extends Controller {
  @notEmpty("settings") hasSettings;
  @notEmpty("translations") hasTranslations;
  @match("model.remote_theme.remote_url", /^http(s)?:\/\//) sourceIsHttp;
+  @readOnly("model.settings") settings;

  @discourseComputed("model.component", "model.remote_theme")
  showCheckboxes() {
@ -146,11 +148,6 @@ export default class AdminCustomizeThemesShowController extends Controller {
    return `admin.customize.theme.convert_${type}_tooltip`;
  }

-  @discourseComputed("model.settings")
-  settings(settings) {
-    return settings.map((setting) => ThemeSettings.create(setting));
-  }
-
  @discourseComputed("model.translations")
  translations(translations) {
    return translations.map((setting) =>
--- a/app/assets/javascripts/admin/addon/models/theme-settings.js
+++ b/app/assets/javascripts/admin/addon/models/theme-settings.js
@ -1,4 +1,19 @@
 import EmberObject from "@ember/object";
+import { ajax } from "discourse/lib/ajax";
 import Setting from "admin/mixins/setting-object";

-export default class ThemeSettings extends EmberObject.extend(Setting) {}
+export default class ThemeSettings extends EmberObject.extend(Setting) {
+  updateSetting(themeId, newValue) {
+    if (this.objects_schema) {
+      newValue = JSON.stringify(newValue);
+    }
+
+    return ajax(`/admin/themes/${themeId}/setting`, {
+      type: "PUT",
+      data: {
+        name: this.setting,
+        value: newValue,
+      },
+    });
+  }
+}
--- a/lib/theme_settings_manager/objects.rb
+++ b/lib/theme_settings_manager/objects.rb
@ -6,6 +6,7 @@ class ThemeSettingsManager::Objects < ThemeSettingsManager
  end

  def value=(objects)
+    objects = JSON.parse(objects) if objects.is_a?(::String)
    ensure_is_valid_value!(objects)
    record = has_record? ? update_record!(json_value: objects) : create_record!(json_value: objects)
    theme.reload
--- a/spec/requests/admin/themes_controller_spec.rb
+++ b/spec/requests/admin/themes_controller_spec.rb
@ -1040,7 +1040,7 @@ RSpec.describe Admin::ThemesController do
  end

  describe "#update_single_setting" do
-    let(:theme) { Fabricate(:theme) }
+    fab!(:theme)

    before do
      theme.set_field(target: :settings, name: :yaml, value: "bg: red")
@ -1063,6 +1063,38 @@ RSpec.describe Admin::ThemesController do
        expect(user_history.action).to eq(UserHistory.actions[:change_theme_setting])
      end

+      it "should be able to update a theme setting of `objects` typed" do
+        SiteSetting.experimental_objects_type_for_theme_settings = true
+
+        field =
+          theme.set_field(
+            target: :settings,
+            name: "yaml",
+            value: File.read("#{Rails.root}/spec/fixtures/theme_settings/objects_settings.yaml"),
+          )
+
+        theme.save!
+
+        put "/admin/themes/#{theme.id}/setting.json",
+            params: {
+              name: "objects_setting",
+              value: [
+                { name: "new_section", links: [{ name: "new link", url: "https://some.url.com" }] },
+              ].to_json,
+            }
+
+        expect(response.status).to eq(200)
+
+        expect(theme.settings[:objects_setting].value).to eq(
+          [
+            {
+              "name" => "new_section",
+              "links" => [{ "name" => "new link", "url" => "https://some.url.com" }],
+            },
+          ],
+        )
+      end
+
      it "should clear a theme setting" do
        put "/admin/themes/#{theme.id}/setting.json", params: { name: "bg" }
        theme.reload
--- a/spec/system/admin_customize_themes_spec.rb
+++ b/spec/system/admin_customize_themes_spec.rb
@ -110,5 +110,43 @@ describe "Admin Customize Themes", type: :system do
        "/admin/customize/themes/#{theme.id}/schema/objects_setting",
      )
    end
+
+    it "allows an admin to edit a theme setting of objects type via the settings editor" do
+      visit "/admin/customize/themes/#{theme.id}"
+
+      theme_settings_editor = admin_customize_themes_page.click_theme_settings_editor_button
+
+      theme_settings_editor.fill_in(<<~SETTING)
+      [
+        {
+          "setting": "objects_setting",
+          "value": [
+            {
+              "name": "new section",
+              "links": [
+                {
+                  "name": "new link",
+                  "url": "https://example.com"
+                }
+              ]
+            }
+          ]
+        }
+      ]
+      SETTING
+
+      theme_settings_editor.save
+
+      try_until_success do
+        expect(theme.reload.settings[:objects_setting].value).to eq(
+          [
+            {
+              "links" => [{ "name" => "new link", "url" => "https://example.com" }],
+              "name" => "new section",
+            },
+          ],
+        )
+      end
+    end
  end
 end
--- a/spec/system/page_objects/components/ace_editor.rb
+++ b/spec/system/page_objects/components/ace_editor.rb
@ -9,6 +9,9 @@ module PageObjects
      end

      def fill_input(content)
+        # Clear the input before filling it in because capybara's fill_in method doesn't seem to replace existing content
+        # unless the content is a blank string.
+        editor_input.fill_in(with: "")
        editor_input.fill_in(with: content)
        self
      end
@ -18,7 +21,10 @@ module PageObjects
      end

      def editor_input
-        find(".ace-wrapper .ace_text-input", visible: false)
+        find(".ace-wrapper .ace:not(.hidden)", visible: true).find(
+          ".ace_text-input",
+          visible: false,
+        )
      end
    end
  end
--- a/spec/system/page_objects/components/admin_theme_settings_editor.rb
+++ b/spec/system/page_objects/components/admin_theme_settings_editor.rb
@ -0,0 +1,23 @@
+# frozen_string_literal: true
+
+module PageObjects
+  module Components
+    class AdminThemeSettingsEditor < Base
+      def fill_in(settings)
+        editor.fill_input(settings)
+        self
+      end
+
+      def save
+        click_button(I18n.t("admin_js.admin.customize.theme.save"))
+        self
+      end
+
+      private
+
+      def editor
+        @editor ||= within(".settings-editor") { AceEditor.new }
+      end
+    end
+  end
+end
--- a/spec/system/page_objects/pages/admin_customize_themes.rb
+++ b/spec/system/page_objects/pages/admin_customize_themes.rb
@ -42,6 +42,11 @@ module PageObjects
      def click_edit_objects_theme_setting_button(setting_name)
        find(".theme-setting[data-setting=\"#{setting_name}\"] .setting-value-edit-button").click
      end
+
+      def click_theme_settings_editor_button
+        click_button(I18n.t("admin_js.admin.customize.theme.settings_editor"))
+        PageObjects::Components::AdminThemeSettingsEditor.new
+      end
    end
  end
 end