From 59f1d01381e3be937b441bbcbe3f18c96351c41d Mon Sep 17 00:00:00 2001 From: Jarek Radosz Date: Thu, 17 Nov 2022 18:44:23 +0100 Subject: [PATCH] DEV: Convert `SiteSettings` to a tracked object (#19015) TrackedObject allows us to reference SiteSettings in autotracking contexts (e.g. JS getters referenced from a Glimmer template) without the need for EmberObject's `get()` function. TrackedObject is backwards-compatible with Ember's legacy reactivity model, so it can be referenced in things like computed properties. Co-authored-by: David Taylor --- .../app/controllers/create-account.js | 7 ++-- .../subscribe-user-notifications.js | 6 +-- .../discourse/app/services/site-settings.js | 3 +- app/assets/javascripts/discourse/package.json | 1 + .../acceptance/admin-badges-show-test.js | 3 +- .../discourse/tests/helpers/site-settings.js | 41 ++++++------------- .../tests/unit/services/site-settings-test.js | 37 +++++++++++++++++ .../addon/components/mini-tag-chooser.js | 2 +- .../addon/components/tag-chooser.js | 6 +-- .../addon/components/tag-group-chooser.js | 2 +- app/assets/javascripts/yarn.lock | 28 ++++++++++++- 11 files changed, 90 insertions(+), 46 deletions(-) create mode 100644 app/assets/javascripts/discourse/tests/unit/services/site-settings-test.js diff --git a/app/assets/javascripts/discourse/app/controllers/create-account.js b/app/assets/javascripts/discourse/app/controllers/create-account.js index 370f0c210f7..997a72afd53 100644 --- a/app/assets/javascripts/discourse/app/controllers/create-account.js +++ b/app/assets/javascripts/discourse/app/controllers/create-account.js @@ -116,8 +116,7 @@ export default Controller.extend( @discourseComputed fullnameRequired() { return ( - this.get("siteSettings.full_name_required") || - this.get("siteSettings.enable_names") + this.siteSettings.full_name_required || this.siteSettings.enable_names ); }, @@ -129,9 +128,9 @@ export default Controller.extend( @discourseComputed disclaimerHtml() { return I18n.t("create_account.disclaimer", { - tos_link: this.get("siteSettings.tos_url") || getURL("/tos"), + tos_link: this.siteSettings.tos_url || getURL("/tos"), privacy_link: - this.get("siteSettings.privacy_policy_url") || getURL("/privacy"), + this.siteSettings.privacy_policy_url || getURL("/privacy"), }); }, diff --git a/app/assets/javascripts/discourse/app/initializers/subscribe-user-notifications.js b/app/assets/javascripts/discourse/app/initializers/subscribe-user-notifications.js index 80160979285..908c88f0db6 100644 --- a/app/assets/javascripts/discourse/app/initializers/subscribe-user-notifications.js +++ b/app/assets/javascripts/discourse/app/initializers/subscribe-user-notifications.js @@ -1,4 +1,3 @@ -import { set } from "@ember/object"; // Subscribes to user events on the message bus import { alertChannel, @@ -161,8 +160,9 @@ export default { ); }); - bus.subscribe("/client_settings", (data) => - set(siteSettings, data.name, data.value) + bus.subscribe( + "/client_settings", + (data) => (siteSettings[data.name] = data.value) ); if (!isTesting()) { diff --git a/app/assets/javascripts/discourse/app/services/site-settings.js b/app/assets/javascripts/discourse/app/services/site-settings.js index f550943d0a4..e57c90719c0 100644 --- a/app/assets/javascripts/discourse/app/services/site-settings.js +++ b/app/assets/javascripts/discourse/app/services/site-settings.js @@ -1,9 +1,10 @@ import PreloadStore from "discourse/lib/preload-store"; +import { TrackedObject } from "@ember-compat/tracked-built-ins"; export default class SiteSettingsService { static isServiceFactory = true; static create() { - return PreloadStore.get("siteSettings"); + return new TrackedObject(PreloadStore.get("siteSettings")); } } diff --git a/app/assets/javascripts/discourse/package.json b/app/assets/javascripts/discourse/package.json index 35a9728a6e8..e8473750b2c 100644 --- a/app/assets/javascripts/discourse/package.json +++ b/app/assets/javascripts/discourse/package.json @@ -20,6 +20,7 @@ "@babel/standalone": "^7.20.4", "@discourse/backburner.js": "^2.7.1-0", "@discourse/itsatrap": "^2.0.10", + "@ember-compat/tracked-built-ins": "^0.9.1", "@ember/jquery": "^2.0.0", "@ember/optional-features": "^2.0.0", "@ember/render-modifiers": "^2.0.4", diff --git a/app/assets/javascripts/discourse/tests/acceptance/admin-badges-show-test.js b/app/assets/javascripts/discourse/tests/acceptance/admin-badges-show-test.js index 2464e770b93..7789f953b11 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/admin-badges-show-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/admin-badges-show-test.js @@ -5,7 +5,6 @@ import { } from "discourse/tests/helpers/qunit-helpers"; import { click, fillIn, settled, visit } from "@ember/test-helpers"; import { test } from "qunit"; -import { set } from "@ember/object"; acceptance("Admin - Badges - Show", function (needs) { needs.user(); @@ -41,7 +40,7 @@ acceptance("Admin - Badges - Show", function (needs) { // SQL fields assert.false(exists("label[for=query]"), "sql input is hidden by default"); - set(this.siteSettings, "enable_badge_sql", true); + this.siteSettings.enable_badge_sql = true; await settled(); assert.true(exists("label[for=query]"), "sql input shows when enabled"); diff --git a/app/assets/javascripts/discourse/tests/helpers/site-settings.js b/app/assets/javascripts/discourse/tests/helpers/site-settings.js index 9f64e064ca0..23dc3b2e781 100644 --- a/app/assets/javascripts/discourse/tests/helpers/site-settings.js +++ b/app/assets/javascripts/discourse/tests/helpers/site-settings.js @@ -1,3 +1,5 @@ +import { TrackedObject } from "@ember-compat/tracked-built-ins"; + const CLIENT_SETTING_TEST_OVERRIDES = { title: "QUnit Discourse Tests", site_logo_url: "/assets/logo.png", @@ -15,46 +17,27 @@ const CLIENT_SETTING_TEST_OVERRIDES = { // Note, CLIENT_SITE_SETTINGS_WITH_DEFAULTS is generated by the site-settings-plugin, // writing to test-site-settings.js via the ember-cli-build pipeline. -const ORIGINAL_CLIENT_SITE_SETTINGS = Object.assign( - {}, +const ORIGINAL_CLIENT_SITE_SETTINGS = { // eslint-disable-next-line no-undef - CLIENT_SITE_SETTINGS_WITH_DEFAULTS, - CLIENT_SETTING_TEST_OVERRIDES -); -let siteSettings = Object.assign({}, ORIGINAL_CLIENT_SITE_SETTINGS); + ...CLIENT_SITE_SETTINGS_WITH_DEFAULTS, + ...CLIENT_SETTING_TEST_OVERRIDES, +}; + +let siteSettings; export function currentSettings() { return siteSettings; } -// In debug mode, Ember will decorate objects with setters that remind you to use -// this.set() because they are bound (even if you use `unbound` or `readonly` in templates!). -// Site settings are only ever changed in tests and these warnings are not wanted, so we'll -// strip them when resetting our settings between tests. -function setValue(k, v) { - let desc = Object.getOwnPropertyDescriptor(siteSettings, k); - if (desc && !desc.writable) { - Object.defineProperty(siteSettings, k, { writable: true }); - } - siteSettings[k] = v; -} - export function mergeSettings(other) { - for (let p in other) { - if (other.hasOwnProperty(p)) { - setValue(p, other[p]); - } + for (const key of Object.keys(other)) { + siteSettings[key] = other[key]; } + return siteSettings; } export function resetSettings() { - for (let p in siteSettings) { - if (siteSettings.hasOwnProperty(p)) { - // eslint-disable-next-line no-undef - let v = ORIGINAL_CLIENT_SITE_SETTINGS[p]; - typeof v !== "undefined" ? setValue(p, v) : delete siteSettings[p]; - } - } + siteSettings = new TrackedObject(ORIGINAL_CLIENT_SITE_SETTINGS); return siteSettings; } diff --git a/app/assets/javascripts/discourse/tests/unit/services/site-settings-test.js b/app/assets/javascripts/discourse/tests/unit/services/site-settings-test.js new file mode 100644 index 00000000000..971964b57b7 --- /dev/null +++ b/app/assets/javascripts/discourse/tests/unit/services/site-settings-test.js @@ -0,0 +1,37 @@ +import { module, test } from "qunit"; +import { setupTest } from "ember-qunit"; +import { getOwner } from "discourse-common/lib/get-owner"; +import { inject as service } from "@ember/service"; +import EmberObject, { computed } from "@ember/object"; + +class TestClass extends EmberObject { + @service siteSettings; + + @computed("siteSettings.title") + get text() { + return `The title: ${this.siteSettings.title}`; + } +} + +module("Unit | Service | site-settings", function (hooks) { + setupTest(hooks); + + hooks.beforeEach(function () { + this.siteSettings = getOwner(this).lookup("service:site-settings"); + }); + + test("contains settings", function (assert) { + assert.ok(this.siteSettings.title); + }); + + test("notifies getters", function (assert) { + this.siteSettings.title = "original"; + + getOwner(this).register("test-class:main", TestClass); + const object = getOwner(this).lookup("test-class:main"); + assert.strictEqual(object.text, "The title: original"); + + this.siteSettings.title = "updated"; + assert.strictEqual(object.text, "The title: updated"); + }); +}); diff --git a/app/assets/javascripts/select-kit/addon/components/mini-tag-chooser.js b/app/assets/javascripts/select-kit/addon/components/mini-tag-chooser.js index b21a37e8c0a..d5e1263a2e5 100644 --- a/app/assets/javascripts/select-kit/addon/components/mini-tag-chooser.js +++ b/app/assets/javascripts/select-kit/addon/components/mini-tag-chooser.js @@ -98,7 +98,7 @@ export default MultiSelectComponent.extend(TagsMixin, { termMatchErrorMessage: json.forbidden_message, }); - if (context.get("siteSettings.tags_sort_alphabetically")) { + if (context.siteSettings.tags_sort_alphabetically) { results = results.sort((a, b) => a.text.localeCompare(b.text)); } diff --git a/app/assets/javascripts/select-kit/addon/components/tag-chooser.js b/app/assets/javascripts/select-kit/addon/components/tag-chooser.js index e4c0c840d60..16909e6cf93 100644 --- a/app/assets/javascripts/select-kit/addon/components/tag-chooser.js +++ b/app/assets/javascripts/select-kit/addon/components/tag-chooser.js @@ -36,7 +36,7 @@ export default MultiSelectComponent.extend(TagsMixin, { return parseInt( this.options.limit || this.options.maximum || - this.get("siteSettings.max_tags_per_topic"), + this.siteSettings.max_tags_per_topic, 10 ); } @@ -80,7 +80,7 @@ export default MultiSelectComponent.extend(TagsMixin, { const data = { q: query, - limit: this.get("siteSettings.max_tag_search_results"), + limit: this.siteSettings.max_tag_search_results, categoryId: this.categoryId, }; @@ -122,7 +122,7 @@ export default MultiSelectComponent.extend(TagsMixin, { }); } - if (context.get("siteSettings.tags_sort_alphabetically")) { + if (context.siteSettings.tags_sort_alphabetically) { results = results.sort((a, b) => a.id > b.id); } diff --git a/app/assets/javascripts/select-kit/addon/components/tag-group-chooser.js b/app/assets/javascripts/select-kit/addon/components/tag-group-chooser.js index 8d2b8b999a2..57d51f272fb 100644 --- a/app/assets/javascripts/select-kit/addon/components/tag-group-chooser.js +++ b/app/assets/javascripts/select-kit/addon/components/tag-group-chooser.js @@ -31,7 +31,7 @@ export default MultiSelectComponent.extend(TagsMixin, { search(query) { const data = { q: query, - limit: this.get("siteSettings.max_tag_search_results"), + limit: this.siteSettings.max_tag_search_results, }; return this.searchTags( diff --git a/app/assets/javascripts/yarn.lock b/app/assets/javascripts/yarn.lock index dcce013f8de..55815569800 100644 --- a/app/assets/javascripts/yarn.lock +++ b/app/assets/javascripts/yarn.lock @@ -1012,6 +1012,14 @@ resolved "https://registry.yarnpkg.com/@discourse/itsatrap/-/itsatrap-2.0.10.tgz#c7e750eeb32b54e769e952c4ecc472213eb1385a" integrity sha512-Jn1gdiyHMGUsmUfLFf4Q7VnTAv0l7NePbegU6pKhKHEmbzV3FosGxq30fTOYgVyTS1bxqGjlA6LvQttJpv3ROw== +"@ember-compat/tracked-built-ins@^0.9.1": + version "0.9.1" + resolved "https://registry.yarnpkg.com/@ember-compat/tracked-built-ins/-/tracked-built-ins-0.9.1.tgz#4cc97c1841425fbf812ef3c63c00ab4790fc32a0" + integrity sha512-A1uYo6EZY6CaKeZoXogxtyJCjz2V0+TXxOZCMJFQHHVuMA1DxYqZoZ25juRWIQpR8r77gTnITtYW2uxBg8vGRw== + dependencies: + "@embroider/addon-shim" "^1.8.3" + ember-tracked-storage-polyfill "^1.0.0" + "@ember-data/rfc395-data@^0.0.4": version "0.0.4" resolved "https://registry.yarnpkg.com/@ember-data/rfc395-data/-/rfc395-data-0.0.4.tgz#ecb86efdf5d7733a76ff14ea651a1b0ed1f8a843" @@ -1078,6 +1086,14 @@ ember-cli-version-checker "^5.1.2" semver "^7.3.5" +"@embroider/addon-shim@^1.8.3": + version "1.8.3" + resolved "https://registry.yarnpkg.com/@embroider/addon-shim/-/addon-shim-1.8.3.tgz#2368510b8ce42d50d02cb3289c32e260dfa34bd9" + integrity sha512-7pyHwzT6ESXc3nZsB8rfnirLkUhQWdvj6CkYH+0MUPN74mX4rslf7pnBqZE/KZkW3uBIaBYvU8fxi0hcKC/Paw== + dependencies: + "@embroider/shared-internals" "^1.8.3" + semver "^7.3.5" + "@embroider/macros@1.8.3": version "1.8.3" resolved "https://registry.yarnpkg.com/@embroider/macros/-/macros-1.8.3.tgz#2f0961ab8871f6ad819630208031d705b357757e" @@ -1106,7 +1122,7 @@ resolve "^1.20.0" semver "^7.3.2" -"@embroider/shared-internals@1.8.3", "@embroider/shared-internals@^1.0.0": +"@embroider/shared-internals@1.8.3", "@embroider/shared-internals@^1.0.0", "@embroider/shared-internals@^1.8.3": version "1.8.3" resolved "https://registry.yarnpkg.com/@embroider/shared-internals/-/shared-internals-1.8.3.tgz#52d868dc80016e9fe983552c0e516f437bf9b9f9" integrity sha512-N5Gho6Qk8z5u+mxLCcMYAoQMbN4MmH+z2jXwQHVs859bxuZTxwF6kKtsybDAASCtd2YGxEmzcc1Ja/wM28824w== @@ -4368,7 +4384,7 @@ ember-cli-babel-plugin-helpers@^1.0.0, ember-cli-babel-plugin-helpers@^1.1.1: resolved "https://registry.yarnpkg.com/ember-cli-babel-plugin-helpers/-/ember-cli-babel-plugin-helpers-1.1.1.tgz#5016b80cdef37036c4282eef2d863e1d73576879" integrity sha512-sKvOiPNHr5F/60NLd7SFzMpYPte/nnGkq/tMIfXejfKHIhaiIkYFqX8Z9UFTKWLLn+V7NOaby6niNPZUdvKCRw== -ember-cli-babel@^7.0.0, ember-cli-babel@^7.10.0, ember-cli-babel@^7.13.0, ember-cli-babel@^7.13.2, ember-cli-babel@^7.21.0, ember-cli-babel@^7.22.1, ember-cli-babel@^7.23.0, ember-cli-babel@^7.23.1, ember-cli-babel@^7.26.10, ember-cli-babel@^7.26.11, ember-cli-babel@^7.26.4, ember-cli-babel@^7.26.6, ember-cli-babel@^7.7.3: +ember-cli-babel@^7.0.0, ember-cli-babel@^7.10.0, ember-cli-babel@^7.13.0, ember-cli-babel@^7.13.2, ember-cli-babel@^7.21.0, ember-cli-babel@^7.22.1, ember-cli-babel@^7.23.0, ember-cli-babel@^7.23.1, ember-cli-babel@^7.26.10, ember-cli-babel@^7.26.11, ember-cli-babel@^7.26.3, ember-cli-babel@^7.26.4, ember-cli-babel@^7.26.6, ember-cli-babel@^7.7.3: version "7.26.11" resolved "https://registry.yarnpkg.com/ember-cli-babel/-/ember-cli-babel-7.26.11.tgz#50da0fe4dcd99aada499843940fec75076249a9f" integrity sha512-JJYeYjiz/JTn34q7F5DSOjkkZqy8qwFOOxXfE6pe9yEJqWGu4qErKxlz8I22JoVEQ/aBUO+OcKTpmctvykM9YA== @@ -4905,6 +4921,14 @@ ember-test-selectors@^6.0.0: ember-cli-babel "^7.26.4" ember-cli-version-checker "^5.1.2" +ember-tracked-storage-polyfill@^1.0.0: + version "1.0.0" + resolved "https://registry.yarnpkg.com/ember-tracked-storage-polyfill/-/ember-tracked-storage-polyfill-1.0.0.tgz#84d307a1e4badc5f84dca681db2cfea9bdee8a77" + integrity sha512-eL7lZat68E6P/D7b9UoTB5bB5Oh/0aju0Z7PCMi3aTwhaydRaxloE7TGrTRYU+NdJuyNVZXeGyxFxn2frvd3TA== + dependencies: + ember-cli-babel "^7.26.3" + ember-cli-htmlbars "^5.7.1" + emoji-regex@^8.0.0: version "8.0.0" resolved "https://registry.yarnpkg.com/emoji-regex/-/emoji-regex-8.0.0.tgz#e818fd69ce5ccfcb404594f842963bf53164cc37"