Merge pull request #6788 from kevinansfield/synchronous-labs-flags

Synchronous feature service

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', this.get(`feature.${this.get('flag')}`));
-            this.set('_flagValue', flagValue);
-        });
     },
 
     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')}]`;
     })

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);
         });

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}`)) {
-                if (this.get(`config.${name}`)) {
+                return this.get(`config.${name}`);
-                    return resolve(this.get(`config.${name}`));
+            }
-                }
 
-                this.get('labs').then((labs) => {
+            return this.get(`labs.${name}`) || false;
-                    resolve(labs[name] || false);
-                });
-            });
         },
         set(key, value) {
-            return this.update(key, value).then((savedValue) => {
+            this.update(key, value);
-                return savedValue;
+            return value;
-            });
         }
     });
 }
@@ -40,62 +32,52 @@ export default Service.extend({
 
     publicAPI: feature('publicAPI'),
 
-    labs: computed('_settings', function () {
+    _settings: null,
-        return this.get('_settings').then((settings) => {
-            return this._parseLabs(settings);
-        });
-    }),
 
-    _settings: computed(function () {
+    labs: computed('_settings.labs', function () {
-        let store = this.get('store');
+        let labs = this.get('_settings.labs');
-
-        return store.queryRecord('setting', {type: 'blog'});
-    }),
-
-    _parseLabs(settings) {
-        let labs = settings.get('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 settings = this.get('_settings');
-            let promises = {
+        let labs = this.get('labs');
-                settings: this.get('_settings'),
-                labs: this.get('labs')
-            };
 
-            RSVP.hash(promises).then(({labs, settings}) => {
+        // set the new labs key value
-                // set the new labs key value
+        set(labs, key, value);
-                set(labs, key, value);
+        // update the 'labs' key of the settings model
-                // update the 'labs' key of the settings model
+        settings.set('labs', JSON.stringify(labs));
-                settings.set('labs', JSON.stringify(labs));
 
-                settings.save().then((savedSettings) => {
+        return settings.save().then(() => {
-                    // replace the cached _settings promise
+            // return the labs key value that we get from the server
-                    this.set('_settings', RSVP.resolve(savedSettings));
+            this.notifyPropertyChange('labs');
+            return this.get(`labs.${key}`);
 
-                    // return the labs key value that we get from the server
+        }).catch((errors) => {
-                    resolve(this._parseLabs(savedSettings).get(key));
+            settings.rollbackAttributes();
+            this.notifyPropertyChange('labs');
 
-                }).catch((errors) => {
+            // we'll always have an errors object unless we hit a
-                    settings.rollbackAttributes();
+            // 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
+            this.get('notifications').showErrors(errors);
-                    // validation error
-                    if (!errors) {
-                        throw new EmberError(`Validation of the feature service settings model failed when updating labs.`);
-                    }
 
-                    this.get('notifications').showErrors(errors);
+            return this.get(`labs.${key}`);
-
-                    resolve(this._parseLabs(settings)[key]);
-                });
-            }).catch(reject);
         });
     }
 });

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;
+            });
+        });
+    }
 });

ghost/admin/tests/integration/components/gh-feature-flag-test.js

@@ -1,42 +1,15 @@
 import { expect } from 'chai';
 import {
-  describeComponent,
+    describeComponent,
-  it
+    it
 } from 'ember-mocha';
 import hbs from 'htmlbars-inline-precompile';
-import FeatureService, {feature} from 'ghost/services/feature';
+import Ember from 'ember';
-import Pretender from 'pretender';
 import wait from 'ember-test-helpers/wait';
 
-function stubSettings(server, labs) {
+const featureStub = Ember.Service.extend({
-    server.get('/ghost/api/v0.1/settings/', function () {
+    testFlag: true
-        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')
-    });
-}
 
 describeComponent(
     'gh-feature-flag',
@@ -48,62 +21,40 @@ describeComponent(
         let server;
 
         beforeEach(function () {
-            server = new Pretender();
+            this.register('service:feature', featureStub);
-        });
+            this.inject.service('feature', {as: 'feature'});
-
-        afterEach(function () {
-            server.shutdown();
         });
 
         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);
-
+            expect(this.$('label input[type="checkbox"]').prop('checked')).to.be.true;
-            return wait().then(() => {
-                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});
+            this.set('feature.testFlag', false);
-            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.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();
+            expect(this.$('label input[type="checkbox"]').prop('checked')).to.be.false;
-            }).then(() => {
-                expect(this.$('label input[type="checkbox"]').prop('checked')).to.be.false;
-            });
         });
     }
 );

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) => {
+            service.fetch().then(() => {
-                expect(labs.testFlag).to.be.true;
+                expect(service.get('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);
-
                 done();
             });
         });
@@ -105,19 +88,9 @@ describeModule(
             let service = this.subject();
             service.get('config').set('testFlag', false);
 
-            let testFlag, labsTestFlag;
+            service.fetch().then(() => {
-
+                expect(service.get('labs.testFlag')).to.be.false;
-            service.get('testFlag').then((result) => {
+                expect(service.get('testFlag')).to.be.false;
-                testFlag = result;
-            });
-
-            service.get('labs').then((labs) => {
-                labsTestFlag = labs.testFlag;
-            });
-
-            return wait().then(() => {
-                expect(labsTestFlag).to.be.false;
-                expect(testFlag).to.be.false;
                 done();
             });
         });
@@ -129,19 +102,9 @@ describeModule(
             let service = this.subject();
             service.get('config').set('testFlag', true);
 
-            let testFlag, labsTestFlag;
+            service.fetch().then(() => {
-
+                expect(service.get('labs.testFlag')).to.be.false;
-            service.get('testFlag').then((result) => {
+                expect(service.get('testFlag')).to.be.true;
-                testFlag = result;
-            });
-
-            service.get('labs').then((labs) => {
-                labsTestFlag = labs.testFlag;
-            });
-
-            return wait().then(() => {
-                expect(labsTestFlag).to.be.false;
-                expect(testFlag).to.be.true;
                 done();
             });
         });
@@ -153,19 +116,9 @@ describeModule(
             let service = this.subject();
             service.get('config').set('testFlag', false);
 
-            let testFlag, labsTestFlag;
+            service.fetch().then(() => {
-
+                expect(service.get('labs.testFlag')).to.be.true;
-            service.get('testFlag').then((result) => {
+                expect(service.get('testFlag')).to.be.true;
-                testFlag = result;
-            });
-
-            service.get('labs').then((labs) => {
-                labsTestFlag = labs.testFlag;
-            });
-
-            return wait().then(() => {
-                expect(labsTestFlag).to.be.true;
-                expect(testFlag).to.be.true;
                 done();
             });
         });
@@ -177,19 +130,9 @@ describeModule(
             let service = this.subject();
             service.get('config').set('testFlag', true);
 
-            let testFlag, labsTestFlag;
+            service.fetch().then(() => {
-
+                expect(service.get('labs.testFlag')).to.be.true;
-            service.get('testFlag').then((result) => {
+                expect(service.get('testFlag')).to.be.true;
-                testFlag = result;
-            });
-
-            service.get('labs').then((labs) => {
-                labsTestFlag = labs.testFlag;
-            });
-
-            return wait().then(() => {
-                expect(labsTestFlag).to.be.true;
-                expect(testFlag).to.be.true;
                 done();
             });
         });
@@ -200,21 +143,16 @@ describeModule(
 
             let service = this.subject();
 
-            run(() => {
+            service.fetch().then(() => {
-                service.get('testFlag').then((testFlag) => {
+                expect(service.get('testFlag')).to.be.false;
-                    expect(testFlag).to.be.false;
+
+                run(() => {
+                    service.set('testFlag', true);
                 });
-            });
 
-            run(() => {
+                return wait().then(() => {
-                service.set('testFlag', true);
+                    expect(server.handlers[1].numberOfCalls).to.equal(1);
-            });
+                    expect(service.get('testFlag')).to.be.true;
-
-            return wait().then(() => {
-                expect(server.handlers[1].numberOfCalls).to.equal(1);
-
-                service.get('testFlag').then((testFlag) => {
-                    expect(testFlag).to.be.true;
                     done();
                 });
             });
@@ -226,23 +164,17 @@ describeModule(
 
             let service = this.subject();
 
-            run(() => {
+            service.fetch().then(() => {
-                service.get('testFlag').then((testFlag) => {
+                expect(service.get('testFlag')).to.be.false;
-                    expect(testFlag).to.be.false;
+
+                run(() => {
+                    service.set('testFlag', true);
                 });
-            });
 
-            run(() => {
+                return wait().then(() => {
-                service.set('testFlag', true);
+                    expect(server.handlers[1].numberOfCalls).to.equal(1);
-            });
+                    expect(service.get('notifications.notifications').length).to.equal(1);
-
+                    expect(service.get('testFlag')).to.be.false;
-            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;
                     done();
                 });
             });
@@ -254,21 +186,21 @@ describeModule(
 
             let service = this.subject();
 
-            run(() => {
+            service.fetch().then(() => {
-                service.get('testFlag').then((testFlag) => {
+                expect(service.get('testFlag')).to.be.false;
-                    expect(testFlag).to.be.false;
+
+                run(() => {
+                    expect(() => {
+                        service.set('testFlag', true);
+                    }, EmberError, 'threw validation error');
                 });
-            });
 
-            run(() => {
+                return wait().then(() => {
-                expect(() => {
+                    // ensure validation is happening before the API is hit
-                    service.set('testFlag', true);
+                    expect(server.handlers[1].numberOfCalls).to.equal(0);
-                }, EmberError, 'Threw validation error');
+                    expect(service.get('testFlag')).to.be.false;
-            });
+                    done();
-
+                });
-            service.get('testFlag').then((testFlag) => {
-                expect(testFlag).to.be.false;
-                done();
             });
         });
     }

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'
         ],