From d1a061e5a7fbd8c753ee0749a2f75958968d63c8 Mon Sep 17 00:00:00 2001 From: Kevin Ansfield Date: Thu, 3 Nov 2022 10:08:48 +0000 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Fixed=20comped=20subscription=20?= =?UTF-8?q?duration=20drop-down=20sometimes=20not=20being=20visible=20(#15?= =?UTF-8?q?764)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit closes https://github.com/TryGhost/Team/issues/2110 - added failing test showing feature service `@feature` properties weren't autotracking correctly if accessed before authentication+settings fetch occurs - shows labs and feature properties on the feature service are not reacting to changes in the settings service - removing the `@computed` on the `feature.labs` getter stops it being cached but it then fails on the `feature.testFlag` computed property - updated `settings` service to behave as expected with our current version of Ember - inspected the store schema for `Setting` to define the "proxied" properties up-front rather than only after fetching - updated the property definition to use `computed` so we're opting in to the old style reactivity (required adding the `@classic` decorator to pass linting) --- ghost/admin/app/services/settings.js | 42 ++++++++++++------- .../integration/services/feature-test.js | 36 +++++++++++++--- 2 files changed, 57 insertions(+), 21 deletions(-) diff --git a/ghost/admin/app/services/settings.js b/ghost/admin/app/services/settings.js index 5d45a7dd80..c3eb15d6e1 100644 --- a/ghost/admin/app/services/settings.js +++ b/ghost/admin/app/services/settings.js @@ -1,13 +1,16 @@ import Service, {inject as service} from '@ember/service'; +import Setting from '../models/setting'; import ValidationEngine from 'ghost-admin/mixins/validation-engine'; +import classic from 'ember-classic-decorator'; +import {computed, defineProperty, get} from '@ember/object'; import {tracked} from '@glimmer/tracking'; - +@classic export default class SettingsService extends Service.extend(ValidationEngine) { @service store; // will be set to the single Settings model, it's a reference so any later // changes to the settings object in the store will be reflected - settingsModel = null; + @tracked settingsModel = null; validationType = 'setting'; _loadingPromise = null; @@ -16,6 +19,28 @@ export default class SettingsService extends Service.extend(ValidationEngine) { // back from the API rather than local updates @tracked settledIcon = ''; + init() { + super.init(...arguments); + + // TODO: update to use .getSchemaDefinitionService().attributesDefinitionFor({type: 'settings'}); + // in later Ember versions + const attributes = get(Setting, 'attributes'); + + for (const [name] of attributes) { + if (!Object.prototype.hasOwnProperty.call(this, name)) { + // a standard defineProperty here was not autotracking correctly - retry in a later Ember version + defineProperty(this, name, computed(`settingsModel.${name}`, { + get() { + return this.settingsModel?.[name]; + }, + set(keyName, value) { + return this.settingsModel[name] = value; + } + })); + } + } + } + get hasDirtyAttributes() { return this.settingsModel?.hasDirtyAttributes || false; } @@ -54,19 +79,6 @@ export default class SettingsService extends Service.extend(ValidationEngine) { this.settingsModel = settingsModel; this.settledIcon = settingsModel.icon; - settingsModel.eachAttribute((name) => { - if (!Object.prototype.hasOwnProperty.call(this, name)) { - Object.defineProperty(this, name, { - get() { - return this.settingsModel[name]; - }, - set(newValue) { - this.settingsModel[name] = newValue; - } - }); - } - }); - return this; } diff --git a/ghost/admin/tests/integration/services/feature-test.js b/ghost/admin/tests/integration/services/feature-test.js index fc206b4951..062c0938f5 100644 --- a/ghost/admin/tests/integration/services/feature-test.js +++ b/ghost/admin/tests/integration/services/feature-test.js @@ -9,7 +9,9 @@ import {settled} from '@ember/test-helpers'; import {setupTest} from 'ember-mocha'; function stubSettings(server, labs, validSave = true) { - let settings = [ + const site = []; + + const settings = [ { id: '1', type: 'labs', @@ -18,6 +20,10 @@ function stubSettings(server, labs, validSave = true) { } ]; + server.get(`${ghostPaths().apiRoot}/site/`, function () { + return [200, {'Content-Type': 'application/json'}, JSON.stringify({site})]; + }); + server.get(`${ghostPaths().apiRoot}/settings/`, function () { return [200, {'Content-Type': 'application/json'}, JSON.stringify({settings})]; }); @@ -227,7 +233,7 @@ describe('Integration: Service: feature', function () { }); return settled().then(() => { - expect(server.handlers[1].numberOfCalls).to.equal(1); + expect(server.handlers[2].numberOfCalls).to.equal(1); expect(service.get('testFlag')).to.be.true; }); }); @@ -252,7 +258,7 @@ describe('Integration: Service: feature', function () { }); return settled().then(() => { - expect(server.handlers[3].numberOfCalls).to.equal(1); + expect(server.handlers[4].numberOfCalls).to.equal(1); expect(service.get('testUserFlag')).to.be.true; }); }); @@ -279,7 +285,7 @@ describe('Integration: Service: feature', function () { return settled().then(() => { expect( - server.handlers[1].numberOfCalls, + server.handlers[2].numberOfCalls, 'PUT call is made' ).to.equal(1); @@ -340,7 +346,7 @@ describe('Integration: Service: feature', function () { service.get('config').set('testFlag', false); return service.fetch().then(() => { - expect(service.get('testFlag')).to.be.false; + expect(service.get('testFlag'), 'testFlag before set').to.be.false; run(() => { expect(() => { @@ -350,9 +356,27 @@ describe('Integration: Service: feature', function () { return settled().then(() => { // ensure validation is happening before the API is hit - expect(server.handlers[1].numberOfCalls).to.equal(0); + expect(server.handlers[2].numberOfCalls).to.equal(0); expect(service.get('testFlag')).to.be.false; }); }); }); + + it('has correct labs flags when accessed before and after settings load', async function () { + stubSettings(server, {testFlag: true}); + stubUser(server, {}); + + addTestFlag(); + + const settingsService = this.owner.lookup('service:settings'); + const featureService = this.owner.lookup('service:feature'); + + expect(featureService.testFlag, 'testFlag before settings fetch').to.be.false; + + await settingsService.fetch(); + + expect(featureService.settings.labs, 'feature.settings.labs after settings fetch').to.equal('{"testFlag":true}'); + expect(featureService.labs, 'feature.labs after settings fetch').to.deep.equal({testFlag: true}); + expect(featureService.testFlag, 'feature.testFlag after settings fetch').to.be.true; + }); });