DEV: refactor setting object mixin to helper class (#30529)

This PR moves the logic from the setting-object mixin to a helper class. I've opted to maintain the interface of the previous classes (ThemeSettings / SiteSetting) that used the mixed-in methods through aliases so that we limit the amount of changes here (these are referenced in the string form and across classes/templates).

Another option we may consider in future if we want to optimize for performance is straight duplication which would trade off this overhead of aliasing/chaining calls through the helper for some duplicate code - only 2 models require these methods at the time of this PR.

app/assets/javascripts/admin/addon/lib/setting-object-helper.js

@@ -0,0 +1,78 @@
+import { dependentKeyCompat } from "@ember/object/compat";
+import { isPresent } from "@ember/utils";
+import { deepEqual } from "discourse-common/lib/object";
+import { i18n } from "discourse-i18n";
+
+export default class SettingObjectHelper {
+  constructor(settingObj) {
+    this.settingObj = settingObj;
+  }
+
+  @dependentKeyCompat
+  get overridden() {
+    let val = this.settingObj.get("value");
+    let defaultVal = this.settingObj.get("default");
+
+    if (val === null) {
+      val = "";
+    }
+    if (defaultVal === null) {
+      defaultVal = "";
+    }
+
+    return !deepEqual(val, defaultVal);
+  }
+
+  @dependentKeyCompat
+  get computedValueProperty() {
+    if (isPresent(this.settingObj.get("valueProperty"))) {
+      return this.settingObj.get("valueProperty");
+    }
+
+    if (isPresent(this.validValues.get("firstObject.value"))) {
+      return "value";
+    }
+    return null;
+  }
+
+  @dependentKeyCompat
+  get computedNameProperty() {
+    if (isPresent(this.settingObj.get("nameProperty"))) {
+      return this.settingObj.get("nameProperty");
+    }
+
+    if (isPresent(this.validValues.get("firstObject.name"))) {
+      return "name";
+    }
+    return null;
+  }
+
+  @dependentKeyCompat
+  get validValues() {
+    const originalValidValues = this.settingObj.get("valid_values");
+    const values = [];
+    const translateNames = this.settingObj.translate_names;
+
+    (originalValidValues || []).forEach((v) => {
+      if (v.name && v.name.length > 0 && translateNames) {
+        values.addObject({ name: i18n(v.name), value: v.value });
+      } else {
+        values.addObject(v);
+      }
+    });
+    return values;
+  }
+
+  @dependentKeyCompat
+  get allowsNone() {
+    if (this.settingObj.get("valid_values")?.includes("")) {
+      return "admin.settings.none";
+    }
+    return undefined;
+  }
+
+  @dependentKeyCompat
+  get anyValue() {
+    return this.settingObj.get("allow_any");
+  }
+}

app/assets/javascripts/admin/addon/mixins/setting-object.js

@@ -1,74 +0,0 @@
-import { computed } from "@ember/object";
-import { readOnly } from "@ember/object/computed";
-import Mixin from "@ember/object/mixin";
-import { isPresent } from "@ember/utils";
-import { deepEqual } from "discourse-common/lib/object";
-import { i18n } from "discourse-i18n";
-
-export default Mixin.create({
-  overridden: computed("value", "default", function () {
-    let val = this.value;
-    let defaultVal = this.default;
-
-    if (val === null) {
-      val = "";
-    }
-    if (defaultVal === null) {
-      defaultVal = "";
-    }
-
-    return !deepEqual(val, defaultVal);
-  }),
-
-  computedValueProperty: computed(
-    "valueProperty",
-    "validValues.[]",
-    function () {
-      if (isPresent(this.valueProperty)) {
-        return this.valueProperty;
-      }
-
-      if (isPresent(this.validValues.get("firstObject.value"))) {
-        return "value";
-      } else {
-        return null;
-      }
-    }
-  ),
-
-  computedNameProperty: computed("nameProperty", "validValues.[]", function () {
-    if (isPresent(this.nameProperty)) {
-      return this.nameProperty;
-    }
-
-    if (isPresent(this.validValues.get("firstObject.name"))) {
-      return "name";
-    } else {
-      return null;
-    }
-  }),
-
-  validValues: computed("valid_values", function () {
-    const validValues = this.valid_values;
-
-    const values = [];
-    const translateNames = this.translate_names;
-
-    (validValues || []).forEach((v) => {
-      if (v.name && v.name.length > 0 && translateNames) {
-        values.addObject({ name: i18n(v.name), value: v.value });
-      } else {
-        values.addObject(v);
-      }
-    });
-    return values;
-  }),
-
-  allowsNone: computed("valid_values", function () {
-    if (this.valid_values?.includes("")) {
-      return "admin.settings.none";
-    }
-  }),
-
-  anyValue: readOnly("allow_any"),
-});

app/assets/javascripts/admin/addon/models/site-setting.js

@@ -1,10 +1,11 @@
 import EmberObject from "@ember/object";
+import { alias } from "@ember/object/computed";
 import { ajax } from "discourse/lib/ajax";
 import discourseComputed from "discourse-common/utils/decorators";
 import { i18n } from "discourse-i18n";
-import Setting from "admin/mixins/setting-object";
+import SettingObjectHelper from "admin/lib/setting-object-helper";
 
-export default class SiteSetting extends EmberObject.extend(Setting) {
+export default class SiteSetting extends EmberObject {
   static findAll(params = {}) {
     return ajax("/admin/site_settings", { data: params }).then(function (
       settings
@@ -39,6 +40,15 @@ export default class SiteSetting extends EmberObject.extend(Setting) {
     return ajax(`/admin/site_settings/${key}`, { type: "PUT", data });
   }
 
+  settingObjectHelper = new SettingObjectHelper(this);
+
+  @alias("settingObjectHelper.overridden") overridden;
+  @alias("settingObjectHelper.computedValueProperty") computedValueProperty;
+  @alias("settingObjectHelper.computedNameProperty") computedNameProperty;
+  @alias("settingObjectHelper.validValues") validValues;
+  @alias("settingObjectHelper.allowsNone") allowsNone;
+  @alias("settingObjectHelper.anyValue") anyValue;
+
   @discourseComputed("setting")
   staffLogFilter(setting) {
     if (!setting) {

app/assets/javascripts/admin/addon/models/theme-settings.js

@@ -1,9 +1,19 @@
 import EmberObject from "@ember/object";
+import { alias } from "@ember/object/computed";
 import { ajax } from "discourse/lib/ajax";
 import { popupAjaxError } from "discourse/lib/ajax-error";
-import Setting from "admin/mixins/setting-object";
+import SettingObjectHelper from "admin/lib/setting-object-helper";
+
+export default class ThemeSettings extends EmberObject {
+  settingObjectHelper = new SettingObjectHelper(this);
+
+  @alias("settingObjectHelper.overridden") overridden;
+  @alias("settingObjectHelper.computedValueProperty") computedValueProperty;
+  @alias("settingObjectHelper.computedNameProperty") computedNameProperty;
+  @alias("settingObjectHelper.validValues") validValues;
+  @alias("settingObjectHelper.allowsNone") allowsNone;
+  @alias("settingObjectHelper.anyValue") anyValue;
 
-export default class ThemeSettings extends EmberObject.extend(Setting) {
   updateSetting(themeId, newValue) {
     if (this.objects_schema) {
       newValue = JSON.stringify(newValue);

app/assets/javascripts/discourse/tests/unit/lib/setting-object-helper-test.js

@@ -0,0 +1,54 @@
+import EmberObject from "@ember/object";
+import { setupTest } from "ember-qunit";
+import { module, test } from "qunit";
+import SettingObjectHelper from "admin/lib/setting-object-helper";
+
+module("Unit | Lib | setting-object-helper", function (hooks) {
+  setupTest(hooks);
+
+  test("flat array", function (assert) {
+    const settingObj = EmberObject.create({
+      valid_values: ["foo", "bar"],
+    });
+
+    const helper = new SettingObjectHelper(settingObj);
+
+    assert.strictEqual(helper.computedValueProperty, null);
+    assert.strictEqual(helper.computedNameProperty, null);
+  });
+
+  test("object", function (assert) {
+    const settingObj = EmberObject.create({
+      valid_values: [{ value: "foo", name: "bar" }],
+    });
+
+    const helper = new SettingObjectHelper(settingObj);
+
+    assert.strictEqual(helper.computedValueProperty, "value");
+    assert.strictEqual(helper.computedNameProperty, "name");
+  });
+
+  test("no values", function (assert) {
+    const settingObj = EmberObject.create({
+      valid_values: [],
+    });
+
+    const helper = new SettingObjectHelper(settingObj);
+
+    assert.strictEqual(helper.computedValueProperty, null);
+    assert.strictEqual(helper.computedNameProperty, null);
+  });
+
+  test("value/name properties defined", function (assert) {
+    const settingObj = EmberObject.create({
+      valueProperty: "foo",
+      nameProperty: "bar",
+      valid_values: [],
+    });
+
+    const helper = new SettingObjectHelper(settingObj);
+
+    assert.strictEqual(helper.computedValueProperty, "foo");
+    assert.strictEqual(helper.computedNameProperty, "bar");
+  });
+});

app/assets/javascripts/discourse/tests/unit/mixins/setting-object-test.js

@@ -1,54 +0,0 @@
-import EmberObject from "@ember/object";
-import { setupTest } from "ember-qunit";
-import { module, test } from "qunit";
-import Setting from "admin/mixins/setting-object";
-
-module("Unit | Mixin | setting-object", function (hooks) {
-  setupTest(hooks);
-
-  test("flat array", function (assert) {
-    const FooSetting = EmberObject.extend(Setting);
-
-    const fooSettingInstance = FooSetting.create({
-      valid_values: ["foo", "bar"],
-    });
-
-    assert.strictEqual(fooSettingInstance.computedValueProperty, null);
-    assert.strictEqual(fooSettingInstance.computedNameProperty, null);
-  });
-
-  test("object", function (assert) {
-    const FooSetting = EmberObject.extend(Setting);
-
-    const fooSettingInstance = FooSetting.create({
-      valid_values: [{ value: "foo", name: "bar" }],
-    });
-
-    assert.strictEqual(fooSettingInstance.computedValueProperty, "value");
-    assert.strictEqual(fooSettingInstance.computedNameProperty, "name");
-  });
-
-  test("no values", function (assert) {
-    const FooSetting = EmberObject.extend(Setting);
-
-    const fooSettingInstance = FooSetting.create({
-      valid_values: [],
-    });
-
-    assert.strictEqual(fooSettingInstance.computedValueProperty, null);
-    assert.strictEqual(fooSettingInstance.computedNameProperty, null);
-  });
-
-  test("value/name properties defined", function (assert) {
-    const FooSetting = EmberObject.extend(Setting);
-
-    const fooSettingInstance = FooSetting.create({
-      valueProperty: "foo",
-      nameProperty: "bar",
-      valid_values: [],
-    });
-
-    assert.strictEqual(fooSettingInstance.computedValueProperty, "foo");
-    assert.strictEqual(fooSettingInstance.computedNameProperty, "bar");
-  });
-});