From b07e7cc70f3ff599873329dd15c41d6640ffb7a9 Mon Sep 17 00:00:00 2001 From: Kelv Date: Mon, 6 Jan 2025 12:02:46 +0800 Subject: [PATCH] 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. --- .../admin/addon/lib/setting-object-helper.js | 78 +++++++++++++++++++ .../admin/addon/mixins/setting-object.js | 74 ------------------ .../admin/addon/models/site-setting.js | 14 +++- .../admin/addon/models/theme-settings.js | 14 +++- .../unit/lib/setting-object-helper-test.js | 54 +++++++++++++ .../tests/unit/mixins/setting-object-test.js | 54 ------------- 6 files changed, 156 insertions(+), 132 deletions(-) create mode 100644 app/assets/javascripts/admin/addon/lib/setting-object-helper.js delete mode 100644 app/assets/javascripts/admin/addon/mixins/setting-object.js create mode 100644 app/assets/javascripts/discourse/tests/unit/lib/setting-object-helper-test.js delete mode 100644 app/assets/javascripts/discourse/tests/unit/mixins/setting-object-test.js diff --git a/app/assets/javascripts/admin/addon/lib/setting-object-helper.js b/app/assets/javascripts/admin/addon/lib/setting-object-helper.js new file mode 100644 index 00000000000..9e67c100d50 --- /dev/null +++ b/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"); + } +} diff --git a/app/assets/javascripts/admin/addon/mixins/setting-object.js b/app/assets/javascripts/admin/addon/mixins/setting-object.js deleted file mode 100644 index 8d6adc48634..00000000000 --- a/app/assets/javascripts/admin/addon/mixins/setting-object.js +++ /dev/null @@ -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"), -}); diff --git a/app/assets/javascripts/admin/addon/models/site-setting.js b/app/assets/javascripts/admin/addon/models/site-setting.js index 9fd3b615bb8..a7b6bd5451f 100644 --- a/app/assets/javascripts/admin/addon/models/site-setting.js +++ b/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) { diff --git a/app/assets/javascripts/admin/addon/models/theme-settings.js b/app/assets/javascripts/admin/addon/models/theme-settings.js index 03ee2a04d86..ddc1f25b27b 100644 --- a/app/assets/javascripts/admin/addon/models/theme-settings.js +++ b/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); diff --git a/app/assets/javascripts/discourse/tests/unit/lib/setting-object-helper-test.js b/app/assets/javascripts/discourse/tests/unit/lib/setting-object-helper-test.js new file mode 100644 index 00000000000..d64444d616f --- /dev/null +++ b/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"); + }); +}); diff --git a/app/assets/javascripts/discourse/tests/unit/mixins/setting-object-test.js b/app/assets/javascripts/discourse/tests/unit/mixins/setting-object-test.js deleted file mode 100644 index 8f9eff917c3..00000000000 --- a/app/assets/javascripts/discourse/tests/unit/mixins/setting-object-test.js +++ /dev/null @@ -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"); - }); -});