From 2da6673197a04123779e6fbbb545318c9c2435f3 Mon Sep 17 00:00:00 2001 From: Kevin Ansfield Date: Thu, 5 May 2016 16:03:09 +0200 Subject: [PATCH] Synchronous feature service supersedes #6773 - update `feature` service and `gh-feature-flag` component to work synchronously rather than async - use the application route's `afterModel` hook so that settings are loaded before first load - override `session` service's `authenticate` method to load the settings after successful authentication before any other routes are processed --- ghost/admin/app/components/gh-feature-flag.js | 7 +- ghost/admin/app/routes/application.js | 10 ++ ghost/admin/app/services/feature.js | 90 ++++------ ghost/admin/app/services/session.js | 11 +- .../components/gh-feature-flag-test.js | 77 ++------- .../integration/services/feature-test.js | 158 +++++------------- .../unit/components/gh-image-uploader-test.js | 1 + 7 files changed, 118 insertions(+), 236 deletions(-) diff --git a/ghost/admin/app/components/gh-feature-flag.js b/ghost/admin/app/components/gh-feature-flag.js index 541430871e..e744e0f943 100644 --- a/ghost/admin/app/components/gh-feature-flag.js +++ b/ghost/admin/app/components/gh-feature-flag.js @@ -14,14 +14,10 @@ const FeatureFlagComponent = Component.extend({ feature: service(), - isVisible: computed.notEmpty('_flagValue'), - init() { this._super(...arguments); - this.get(`feature.${this.get('flag')}`).then((flagValue) => { - this.set('_flagValue', flagValue); - }); + this.set('_flagValue', this.get(`feature.${this.get('flag')}`)); }, value: computed('_flagValue', { @@ -36,6 +32,7 @@ const FeatureFlagComponent = Component.extend({ for: computed('flag', function () { return `labs-${this.get('flag')}`; }), + name: computed('flag', function () { return `labs[${this.get('flag')}]`; }) diff --git a/ghost/admin/app/routes/application.js b/ghost/admin/app/routes/application.js index faeeba1161..2b79292002 100644 --- a/ghost/admin/app/routes/application.js +++ b/ghost/admin/app/routes/application.js @@ -24,12 +24,19 @@ export default Route.extend(ApplicationRouteMixin, ShortcutsRoute, { shortcuts, config: service(), + feature: service(), dropdown: service(), notifications: service(), afterModel(model, transition) { + this._super(...arguments); + if (this.get('session.isAuthenticated')) { transition.send('loadServerNotifications'); + + // return the feature loading promise so that we block until settings + // are loaded in order for synchronous access everywhere + return this.get('feature').fetch(); } }, @@ -42,7 +49,10 @@ export default Route.extend(ApplicationRouteMixin, ShortcutsRoute, { return; } + // standard ESA post-sign-in redirect this._super(...arguments); + + // trigger post-sign-in background behaviour this.get('session.user').then((user) => { this.send('signedIn', user); }); diff --git a/ghost/admin/app/services/feature.js b/ghost/admin/app/services/feature.js index dff0fc4670..566a3c4511 100644 --- a/ghost/admin/app/services/feature.js +++ b/ghost/admin/app/services/feature.js @@ -1,34 +1,26 @@ import Ember from 'ember'; const { - RSVP, Service, computed, inject: {service}, set } = Ember; -const {Promise} = RSVP; - const EmberError = Ember.Error; export function feature(name) { return computed(`config.${name}`, `labs.${name}`, { get() { - return new Promise((resolve) => { - if (this.get(`config.${name}`)) { - return resolve(this.get(`config.${name}`)); - } + if (this.get(`config.${name}`)) { + return this.get(`config.${name}`); + } - this.get('labs').then((labs) => { - resolve(labs[name] || false); - }); - }); + return this.get(`labs.${name}`) || false; }, set(key, value) { - return this.update(key, value).then((savedValue) => { - return savedValue; - }); + this.update(key, value); + return value; } }); } @@ -40,62 +32,52 @@ export default Service.extend({ publicAPI: feature('publicAPI'), - labs: computed('_settings', function () { - return this.get('_settings').then((settings) => { - return this._parseLabs(settings); - }); - }), + _settings: null, - _settings: computed(function () { - let store = this.get('store'); - - return store.queryRecord('setting', {type: 'blog'}); - }), - - _parseLabs(settings) { - let labs = settings.get('labs'); + labs: computed('_settings.labs', function () { + let labs = this.get('_settings.labs'); try { return JSON.parse(labs) || {}; } catch (e) { return {}; } + }), + + fetch() { + return this.get('store').queryRecord('setting', {type: 'blog'}).then((settings) => { + this.set('_settings', settings); + return true; + }); }, update(key, value) { - return new Promise((resolve, reject) => { - let promises = { - settings: this.get('_settings'), - labs: this.get('labs') - }; + let settings = this.get('_settings'); + let labs = this.get('labs'); - RSVP.hash(promises).then(({labs, settings}) => { - // set the new labs key value - set(labs, key, value); - // update the 'labs' key of the settings model - settings.set('labs', JSON.stringify(labs)); + // set the new labs key value + set(labs, key, value); + // update the 'labs' key of the settings model + settings.set('labs', JSON.stringify(labs)); - settings.save().then((savedSettings) => { - // replace the cached _settings promise - this.set('_settings', RSVP.resolve(savedSettings)); + return settings.save().then(() => { + // return the labs key value that we get from the server + this.notifyPropertyChange('labs'); + return this.get(`labs.${key}`); - // return the labs key value that we get from the server - resolve(this._parseLabs(savedSettings).get(key)); + }).catch((errors) => { + settings.rollbackAttributes(); + this.notifyPropertyChange('labs'); - }).catch((errors) => { - settings.rollbackAttributes(); + // we'll always have an errors object unless we hit a + // validation error + if (!errors) { + throw new EmberError(`Validation of the feature service settings model failed when updating labs.`); + } - // we'll always have an errors object unless we hit a - // validation error - if (!errors) { - throw new EmberError(`Validation of the feature service settings model failed when updating labs.`); - } + this.get('notifications').showErrors(errors); - this.get('notifications').showErrors(errors); - - resolve(this._parseLabs(settings)[key]); - }); - }).catch(reject); + return this.get(`labs.${key}`); }); } }); diff --git a/ghost/admin/app/services/session.js b/ghost/admin/app/services/session.js index af9077935f..bb63134491 100644 --- a/ghost/admin/app/services/session.js +++ b/ghost/admin/app/services/session.js @@ -8,8 +8,17 @@ const { export default SessionService.extend({ store: service(), + feature: service(), user: computed(function () { return this.get('store').findRecord('user', 'me'); - }) + }), + + authenticate() { + return this._super(...arguments).then((authResult) => { + return this.get('feature').fetch().then(() => { + return authResult; + }); + }); + } }); diff --git a/ghost/admin/tests/integration/components/gh-feature-flag-test.js b/ghost/admin/tests/integration/components/gh-feature-flag-test.js index 2552b8bfb2..30813c1fb7 100644 --- a/ghost/admin/tests/integration/components/gh-feature-flag-test.js +++ b/ghost/admin/tests/integration/components/gh-feature-flag-test.js @@ -1,42 +1,15 @@ import { expect } from 'chai'; import { - describeComponent, - it + describeComponent, + it } from 'ember-mocha'; import hbs from 'htmlbars-inline-precompile'; -import FeatureService, {feature} from 'ghost/services/feature'; -import Pretender from 'pretender'; +import Ember from 'ember'; import wait from 'ember-test-helpers/wait'; -function stubSettings(server, labs) { - server.get('/ghost/api/v0.1/settings/', function () { - return [200, {'Content-Type': 'application/json'}, JSON.stringify({settings: [ - { - id: '1', - type: 'blog', - key: 'labs', - value: JSON.stringify(labs) - }, - // postsPerPage is needed to satisfy the validation - { - id: '2', - type: 'blog', - key: 'postsPerPage', - value: 1 - } - ]})]; - }); - - server.put('/ghost/api/v0.1/settings/', function (request) { - return [200, {'Content-Type': 'application/json'}, request.requestBody]; - }); -} - -function addTestFlag() { - FeatureService.reopen({ - testFlag: feature('testFlag') - }); -} +const featureStub = Ember.Service.extend({ + testFlag: true +}); describeComponent( 'gh-feature-flag', @@ -48,62 +21,40 @@ describeComponent( let server; beforeEach(function () { - server = new Pretender(); - }); - - afterEach(function () { - server.shutdown(); + this.register('service:feature', featureStub); + this.inject.service('feature', {as: 'feature'}); }); it('renders properties correctly', function () { - stubSettings(server, {testFlag: true}); - addTestFlag(); - this.render(hbs`{{gh-feature-flag "testFlag"}}`); expect(this.$()).to.have.length(1); expect(this.$('label').attr('for')).to.equal(this.$('input[type="checkbox"]').attr('id')); }); it('renders correctly when flag is set to true', function () { - stubSettings(server, {testFlag: true}); - addTestFlag(); - this.render(hbs`{{gh-feature-flag "testFlag"}}`); expect(this.$()).to.have.length(1); - - return wait().then(() => { - expect(this.$('label input[type="checkbox"]').prop('checked')).to.be.true; - }); + expect(this.$('label input[type="checkbox"]').prop('checked')).to.be.true; }); it('renders correctly when flag is set to false', function () { - stubSettings(server, {testFlag: false}); - addTestFlag(); + this.set('feature.testFlag', false); this.render(hbs`{{gh-feature-flag "testFlag"}}`); expect(this.$()).to.have.length(1); - return wait().then(() => { - expect(this.$('label input[type="checkbox"]').prop('checked')).to.be.false; - }); + expect(this.$('label input[type="checkbox"]').prop('checked')).to.be.false; }); it('updates to reflect changes in flag property', function () { - stubSettings(server, {testFlag: true}); - addTestFlag(); - this.render(hbs`{{gh-feature-flag "testFlag"}}`); expect(this.$()).to.have.length(1); - return wait().then(() => { - expect(this.$('label input[type="checkbox"]').prop('checked')).to.be.true; + expect(this.$('label input[type="checkbox"]').prop('checked')).to.be.true; - this.$('label').click(); + this.$('label').click(); - return wait(); - }).then(() => { - expect(this.$('label input[type="checkbox"]').prop('checked')).to.be.false; - }); + expect(this.$('label input[type="checkbox"]').prop('checked')).to.be.false; }); } ); diff --git a/ghost/admin/tests/integration/services/feature-test.js b/ghost/admin/tests/integration/services/feature-test.js index b4d2209f73..00d2dac7c5 100644 --- a/ghost/admin/tests/integration/services/feature-test.js +++ b/ghost/admin/tests/integration/services/feature-test.js @@ -71,29 +71,12 @@ describeModule( it('loads labs settings correctly', function (done) { stubSettings(server, {testFlag: true}); + addTestFlag(); let service = this.subject(); - service.get('labs').then((labs) => { - expect(labs.testFlag).to.be.true; - done(); - }); - }); - - it('caches the labs promise', function (done) { - stubSettings(server, {testFlag: true}); - - let service = this.subject(); - let calls = [ - service.get('labs'), - service.get('labs'), - service.get('labs') - ]; - - RSVP.all(calls).then(() => { - expect(server.handledRequests.length, 'requests after 3 calls') - .to.equal(1); - + service.fetch().then(() => { + expect(service.get('testFlag')).to.be.true; done(); }); }); @@ -105,19 +88,9 @@ describeModule( let service = this.subject(); service.get('config').set('testFlag', false); - let testFlag, labsTestFlag; - - service.get('testFlag').then((result) => { - testFlag = result; - }); - - service.get('labs').then((labs) => { - labsTestFlag = labs.testFlag; - }); - - return wait().then(() => { - expect(labsTestFlag).to.be.false; - expect(testFlag).to.be.false; + service.fetch().then(() => { + expect(service.get('labs.testFlag')).to.be.false; + expect(service.get('testFlag')).to.be.false; done(); }); }); @@ -129,19 +102,9 @@ describeModule( let service = this.subject(); service.get('config').set('testFlag', true); - let testFlag, labsTestFlag; - - service.get('testFlag').then((result) => { - testFlag = result; - }); - - service.get('labs').then((labs) => { - labsTestFlag = labs.testFlag; - }); - - return wait().then(() => { - expect(labsTestFlag).to.be.false; - expect(testFlag).to.be.true; + service.fetch().then(() => { + expect(service.get('labs.testFlag')).to.be.false; + expect(service.get('testFlag')).to.be.true; done(); }); }); @@ -153,19 +116,9 @@ describeModule( let service = this.subject(); service.get('config').set('testFlag', false); - let testFlag, labsTestFlag; - - service.get('testFlag').then((result) => { - testFlag = result; - }); - - service.get('labs').then((labs) => { - labsTestFlag = labs.testFlag; - }); - - return wait().then(() => { - expect(labsTestFlag).to.be.true; - expect(testFlag).to.be.true; + service.fetch().then(() => { + expect(service.get('labs.testFlag')).to.be.true; + expect(service.get('testFlag')).to.be.true; done(); }); }); @@ -177,19 +130,9 @@ describeModule( let service = this.subject(); service.get('config').set('testFlag', true); - let testFlag, labsTestFlag; - - service.get('testFlag').then((result) => { - testFlag = result; - }); - - service.get('labs').then((labs) => { - labsTestFlag = labs.testFlag; - }); - - return wait().then(() => { - expect(labsTestFlag).to.be.true; - expect(testFlag).to.be.true; + service.fetch().then(() => { + expect(service.get('labs.testFlag')).to.be.true; + expect(service.get('testFlag')).to.be.true; done(); }); }); @@ -200,21 +143,16 @@ describeModule( let service = this.subject(); - run(() => { - service.get('testFlag').then((testFlag) => { - expect(testFlag).to.be.false; + service.fetch().then(() => { + expect(service.get('testFlag')).to.be.false; + + run(() => { + service.set('testFlag', true); }); - }); - run(() => { - service.set('testFlag', true); - }); - - return wait().then(() => { - expect(server.handlers[1].numberOfCalls).to.equal(1); - - service.get('testFlag').then((testFlag) => { - expect(testFlag).to.be.true; + return wait().then(() => { + expect(server.handlers[1].numberOfCalls).to.equal(1); + expect(service.get('testFlag')).to.be.true; done(); }); }); @@ -226,23 +164,17 @@ describeModule( let service = this.subject(); - run(() => { - service.get('testFlag').then((testFlag) => { - expect(testFlag).to.be.false; + service.fetch().then(() => { + expect(service.get('testFlag')).to.be.false; + + run(() => { + service.set('testFlag', true); }); - }); - run(() => { - service.set('testFlag', true); - }); - - return wait().then(() => { - expect(server.handlers[1].numberOfCalls).to.equal(1); - - expect(service.get('notifications.notifications').length).to.equal(1); - - service.get('testFlag').then((testFlag) => { - expect(testFlag).to.be.false; + return wait().then(() => { + expect(server.handlers[1].numberOfCalls).to.equal(1); + expect(service.get('notifications.notifications').length).to.equal(1); + expect(service.get('testFlag')).to.be.false; done(); }); }); @@ -254,21 +186,21 @@ describeModule( let service = this.subject(); - run(() => { - service.get('testFlag').then((testFlag) => { - expect(testFlag).to.be.false; + service.fetch().then(() => { + expect(service.get('testFlag')).to.be.false; + + run(() => { + expect(() => { + service.set('testFlag', true); + }, EmberError, 'threw validation error'); }); - }); - run(() => { - expect(() => { - service.set('testFlag', true); - }, EmberError, 'Threw validation error'); - }); - - service.get('testFlag').then((testFlag) => { - expect(testFlag).to.be.false; - done(); + return wait().then(() => { + // ensure validation is happening before the API is hit + expect(server.handlers[1].numberOfCalls).to.equal(0); + expect(service.get('testFlag')).to.be.false; + done(); + }); }); }); } diff --git a/ghost/admin/tests/unit/components/gh-image-uploader-test.js b/ghost/admin/tests/unit/components/gh-image-uploader-test.js index 6545fff3f4..30bfb0c88a 100644 --- a/ghost/admin/tests/unit/components/gh-image-uploader-test.js +++ b/ghost/admin/tests/unit/components/gh-image-uploader-test.js @@ -50,6 +50,7 @@ describeComponent( 'service:config', 'service:session', 'service:ajax', + 'service:feature', 'component:x-file-input', 'component:one-way-input' ],