From a37a19b55ced25372df56e92b0f6089f7021dddb Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Tue, 14 Jul 2020 13:07:54 -0400 Subject: [PATCH] REFACTOR: Remove less effective method of injecting `siteSettings` --- .../discourse/app/models/bookmark.js | 1 - .../discourse/app/models/composer.js | 1 - .../javascripts/discourse/app/models/post.js | 14 +----- test/javascripts/helpers/component-test.js | 5 +- test/javascripts/helpers/create-store.js | 3 +- test/javascripts/helpers/qunit-helpers.js | 18 +++----- test/javascripts/helpers/site-settings.js | 46 +++++++++++++++---- test/javascripts/test_helper.js | 6 ++- 8 files changed, 55 insertions(+), 39 deletions(-) diff --git a/app/assets/javascripts/discourse/app/models/bookmark.js b/app/assets/javascripts/discourse/app/models/bookmark.js index ba5a34777d8..585fa7a868f 100644 --- a/app/assets/javascripts/discourse/app/models/bookmark.js +++ b/app/assets/javascripts/discourse/app/models/bookmark.js @@ -171,7 +171,6 @@ const Bookmark = RestModel.extend({ Bookmark.reopenClass({ create(args) { args = args || {}; - args.siteSettings = args.siteSettings || Discourse.SiteSettings; args.currentUser = args.currentUser || Discourse.currentUser; return this._super(args); } diff --git a/app/assets/javascripts/discourse/app/models/composer.js b/app/assets/javascripts/discourse/app/models/composer.js index f4d302775ad..571cfe268c0 100644 --- a/app/assets/javascripts/discourse/app/models/composer.js +++ b/app/assets/javascripts/discourse/app/models/composer.js @@ -1211,7 +1211,6 @@ Composer.reopenClass({ args = args || {}; args.user = args.user || User.current(); args.site = args.site || Site.current(); - args.siteSettings = args.siteSettings || Discourse.SiteSettings; return this._super(args); }, diff --git a/app/assets/javascripts/discourse/app/models/post.js b/app/assets/javascripts/discourse/app/models/post.js index 35b600aae2e..18bc421fea4 100644 --- a/app/assets/javascripts/discourse/app/models/post.js +++ b/app/assets/javascripts/discourse/app/models/post.js @@ -1,6 +1,6 @@ import I18n from "I18n"; import discourseComputed from "discourse-common/utils/decorators"; -import EmberObject, { computed, get } from "@ember/object"; +import EmberObject, { get } from "@ember/object"; import { isEmpty } from "@ember/utils"; import { equal, and, or, not } from "@ember/object/computed"; import { ajax } from "discourse/lib/ajax"; @@ -18,18 +18,6 @@ import User from "discourse/models/user"; import showModal from "discourse/lib/show-modal"; const Post = RestModel.extend({ - // TODO: Remove this once one instantiate all `Discourse.Post` models via the store. - siteSettings: computed({ - get() { - return Discourse.SiteSettings; - }, - - // prevents model created from json to overridde this property - set() { - return Discourse.SiteSettings; - } - }), - @discourseComputed("url") shareUrl(url) { const user = User.current(); diff --git a/test/javascripts/helpers/component-test.js b/test/javascripts/helpers/component-test.js index 0330f6e91a1..5322a8113b6 100644 --- a/test/javascripts/helpers/component-test.js +++ b/test/javascripts/helpers/component-test.js @@ -4,6 +4,7 @@ import { autoLoadModules } from "discourse/initializers/auto-load-modules"; import TopicTrackingState from "discourse/models/topic-tracking-state"; import User from "discourse/models/user"; import Site from "discourse/models/site"; +import { currentSettings } from "helpers/site-settings"; export default function(name, opts) { opts = opts || {}; @@ -15,7 +16,7 @@ export default function(name, opts) { test(name, function(assert) { this.site = Site.current(); - this.registry.register("site-settings:main", Discourse.SiteSettings, { + this.registry.register("site-settings:main", currentSettings(), { instantiate: false }); this.registry.register("capabilities:main", EmberObject); @@ -25,7 +26,7 @@ export default function(name, opts) { this.registry.injection("component", "capabilities", "capabilities:main"); this.registry.injection("component", "site", "site:main"); - this.siteSettings = Discourse.SiteSettings; + this.siteSettings = currentSettings(); autoLoadModules(this.registry, this.registry); diff --git a/test/javascripts/helpers/create-store.js b/test/javascripts/helpers/create-store.js index 5385a48fd05..c4758165098 100644 --- a/test/javascripts/helpers/create-store.js +++ b/test/javascripts/helpers/create-store.js @@ -4,6 +4,7 @@ import KeyValueStore from "discourse/lib/key-value-store"; import TopicListAdapter from "discourse/adapters/topic-list"; import TopicTrackingState from "discourse/models/topic-tracking-state"; import { buildResolver } from "discourse-common/resolver"; +import { currentSettings } from "helpers/site-settings"; const CatAdapter = RestAdapter.extend({ primaryKey: "cat_id" @@ -40,7 +41,7 @@ export default function(customLookup = () => {}) { return this._tracker; } if (type === "site-settings:main") { - this._settings = this._settings || Discourse.SiteSettings; + this._settings = this._settings || currentSettings(); return this._settings; } return customLookup(type); diff --git a/test/javascripts/helpers/qunit-helpers.js b/test/javascripts/helpers/qunit-helpers.js index dca1e4b4f47..aed99d6634d 100644 --- a/test/javascripts/helpers/qunit-helpers.js +++ b/test/javascripts/helpers/qunit-helpers.js @@ -25,6 +25,7 @@ import { resetCustomPostMessageCallbacks } from "discourse/controllers/topic"; import { _clearSnapshots } from "select-kit/components/composer-actions"; import User from "discourse/models/user"; import { mapRoutes } from "discourse/mapping-router"; +import { currentSettings, mergeSettings } from "helpers/site-settings"; export function currentUser() { return User.create(sessionFixtures["/session/current.json"].current_user); @@ -107,7 +108,7 @@ export function controllerModule(name, args = {}) { setup() { this.registry.register("router:main", mapRoutes()); let controller = this.subject(); - controller.siteSettings = Discourse.SiteSettings; + controller.siteSettings = currentSettings(); if (args.setupController) { args.setupController(controller); } @@ -119,8 +120,7 @@ export function controllerModule(name, args = {}) { export function discourseModule(name, hooks) { QUnit.module(name, { beforeEach() { - // Give us an API to change site settings without `Discourse.SiteSettings` in tests - this.siteSettings = Discourse.SiteSettings; + this.siteSettings = currentSettings(); if (hooks && hooks.beforeEach) { hooks.beforeEach.call(this); } @@ -161,16 +161,12 @@ export function acceptance(name, options) { } if (options.settings) { - Discourse.SiteSettings = jQuery.extend( - true, - Discourse.SiteSettings, - options.settings - ); + mergeSettings(options.settings); } - this.siteSettings = Discourse.SiteSettings; + this.siteSettings = currentSettings(); if (options.site) { - resetSite(Discourse.SiteSettings, options.site); + resetSite(currentSettings(), options.site); } clearOutletCache(); @@ -186,7 +182,7 @@ export function acceptance(name, options) { flushMap(); localStorage.clear(); User.resetCurrent(); - resetSite(Discourse.SiteSettings); + resetSite(currentSettings()); resetExtraClasses(); clearOutletCache(); clearHTMLCache(); diff --git a/test/javascripts/helpers/site-settings.js b/test/javascripts/helpers/site-settings.js index 4d1181dbaf9..477dbefcd0e 100644 --- a/test/javascripts/helpers/site-settings.js +++ b/test/javascripts/helpers/site-settings.js @@ -1,6 +1,4 @@ -// discourse-skip-module - -Discourse.SiteSettingsOriginal = { +const ORIGINAL_SETTINGS = { title: "QUnit Discourse Tests", site_logo_url: "/assets/logo.png", site_logo_url: "/assets/logo.png", @@ -103,8 +101,40 @@ Discourse.SiteSettingsOriginal = { secure_media: false }; -Discourse.SiteSettings = jQuery.extend( - true, - {}, - Discourse.SiteSettingsOriginal -); +let siteSettings = Object.assign({}, ORIGINAL_SETTINGS); +Discourse.SiteSettings = 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]); + } + } + return siteSettings; +} + +export function resetSettings() { + for (let p in siteSettings) { + if (siteSettings.hasOwnProperty(p)) { + let v = ORIGINAL_SETTINGS[p]; + typeof v !== "undefined" ? setValue(p, v) : delete siteSettings[p]; + } + } + return siteSettings; +} diff --git a/test/javascripts/test_helper.js b/test/javascripts/test_helper.js index f20b2251ed5..256ed45c8b4 100644 --- a/test/javascripts/test_helper.js +++ b/test/javascripts/test_helper.js @@ -42,6 +42,8 @@ // //= require jquery.magnific-popup.min.js +let resetSettings = require("helpers/site-settings").resetSettings; + const buildResolver = require("discourse-common/resolver").buildResolver; window.setResolver(buildResolver("discourse").create({ namespace: Discourse })); @@ -104,6 +106,7 @@ function resetSite(siteSettings, extras) { } QUnit.testStart(function(ctx) { + resetSettings(); server = createPretender.default; createPretender.applyDefaultHandlers(server); server.handlers = []; @@ -149,8 +152,7 @@ QUnit.testStart(function(ctx) { ); } - // Allow our tests to change site settings and have them reset before the next test - Discourse.SiteSettings = dup(Discourse.SiteSettingsOriginal); + resetSettings(); let getURL = require("discourse-common/lib/get-url"); getURL.setupURL(null, "http://localhost:3000", "");