cleanup feature service error handling

Author: acburdine

core/client/app/services/feature.js

@@ -8,6 +8,8 @@ const {
     set
 } = Ember;
 
+const EmberError = Ember.Error;
+
 export function feature(name) {
     return computed(`config.${name}`, `labs.${name}`, {
         get() {
@@ -22,7 +24,7 @@ export function feature(name) {
             });
         },
         set(key, value) {
-            this.update(key, value).then((savedValue) => {
+            return this.update(key, value).then((savedValue) => {
                 return savedValue;
             });
         }
@@ -76,8 +78,13 @@ export default Service.extend({
                     this.set('_settings', savedSettings);
                     resolve(this._parseLabs(savedSettings).get(key));
                 }).catch((errors) => {
-                    this.get('notifications').showErrors(errors);
-                    settings.rollbackAttributes();
+                    if (errors) { // model.save errors, show notifications
+                        this.get('notifications').showErrors(errors);
+                        settings.rollbackAttributes();
+                    } else {
+                        settings.rollbackAttributes();
+                        throw new EmberError(`Validation of the feature service settings model failed when updating labs.`);
+                    }
                     resolve(this._parseLabs(settings)[key]);
                 });
             }).catch(reject);

core/client/tests/integration/services/feature-test.js

@@ -6,30 +6,43 @@ import Pretender from 'pretender';
 import wait from 'ember-test-helpers/wait';
 import FeatureService, {feature} from 'ghost/services/feature';
 import Ember from 'ember';
+import { errorOverride, errorReset } from 'ghost/tests/helpers/adapter-error';
 
 const {merge, run} = Ember;
+const EmberError = Ember.Error;
+
+function stubSettings(server, labs, validSave = true, validSettings = true) {
+    let settings = [
+        {
+            id: '1',
+            type: 'blog',
+            key: 'labs',
+            value: JSON.stringify(labs)
+        }
+    ];
+
+    if (validSettings) {
+        settings.push({
+            id: '2',
+            type: 'blog',
+            key: 'postsPerPage',
+            value: 1
+        });
+    }
 
-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
-            }
-        ]})];
+        return [200, {'Content-Type': 'application/json'}, JSON.stringify({settings})];
     });
 
     server.put('/ghost/api/v0.1/settings/', function (request) {
-        return [200, {'Content-Type': 'application/json'}, request.requestBody];
+        let statusCode = (validSave) ? 200 : 400;
+        let response = (validSave) ? request.requestBody : JSON.stringify({
+            errors: [{
+                message: 'Test Error'
+            }]
+        });
+
+        return [statusCode, {'Content-Type': 'application/json'}, response];
     });
 }
 
@@ -182,15 +195,63 @@ describeModule(
             return wait().then(() => {
                 expect(server.handlers[1].numberOfCalls).to.equal(1);
 
-                // TODO: failing because service.update only sets values on
                 service.get('testFlag').then((testFlag) => {
                     expect(testFlag).to.be.true;
                     done();
                 });
             });
         });
 
-        it('notifies for server errors');
-        it('notifies for validation errors');
+        it('notifies for server errors', function (done) {
+            stubSettings(server, {testFlag: false}, false);
+            addTestFlag();
+
+            let service = this.subject();
+
+            run(() => {
+                service.get('testFlag').then((testFlag) => {
+                    expect(testFlag).to.be.false;
+                });
+            });
+
+            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;
+                    done();
+                });
+            });
+        });
+
+        it('notifies for validation errors', function (done) {
+            stubSettings(server, {testFlag: false}, true, false);
+            addTestFlag();
+
+            let service = this.subject();
+
+            run(() => {
+                service.get('testFlag').then((testFlag) => {
+                    expect(testFlag).to.be.false;
+                });
+            });
+
+            run(() => {
+                expect(() => {
+                    service.set('testFlag', true);
+                }, EmberError, 'Threw validation error');
+            });
+
+            service.get('testFlag').then((testFlag) => {
+                expect(testFlag).to.be.false;
+                done();
+            });
+        });
     }
 );

core/client/tests/unit/validators/tag-settings-test.js

@@ -112,7 +112,6 @@ describe('Unit: Validator: tag-settings', function () {
         nameErrors = tag.get('errors').errorsFor('name').get(0);
         expect(nameErrors.attribute, 'errors.name.attribute').to.equal('name');
         expect(nameErrors.message, 'errors.name.message').to.equal('Tag names can\'t start with commas.');
-        expect(tag.get('errors.length')).to.equal(1);
 
         expect(passed, 'passed').to.be.false;
         expect(tag.get('hasValidated'), 'hasValidated').to.include('name');