Use a custom transform to simplify navigation settings

no issue
- moves the `NavItem` object from the navigation controller to an explicit `NavigationItem` model file
- adds a custom transform `navigation-settings` that transforms the navigation settings JSON string to/from an array of `NavigationItem` objects
- simplifies the `settings/navigation` controller as it no longer has to export it's own internal model and handle serialization and deserialization

This pattern should also help simplify the apps/slack integration code if implemented there.

ghost/admin/app/controllers/settings/navigation.js

@@ -1,43 +1,13 @@
 import Ember from 'ember';
-import DS from 'ember-data';
 import SettingsSaveMixin from 'ghost/mixins/settings-save';
-import ValidationEngine from 'ghost/mixins/validation-engine';
+import NavigationItem from 'ghost/models/navigation-item';
 
 const {
     Controller,
     RSVP,
     computed,
-    inject: {service},
-    isBlank
+    inject: {service}
 } = Ember;
-const {Errors} = DS;
-const emberA = Ember.A;
-
-export const NavItem = Ember.Object.extend(ValidationEngine, {
-    label: '',
-    url: '',
-    isNew: false,
-
-    validationType: 'navItem',
-
-    isComplete: computed('label', 'url', function () {
-        let {label, url} = this.getProperties('label', 'url');
-
-        return !isBlank(label) && !isBlank(url);
-    }),
-
-    isBlank: computed('label', 'url', function () {
-        let {label, url} = this.getProperties('label', 'url');
-
-        return isBlank(label) && isBlank(url);
-    }),
-
-    init() {
-        this._super(...arguments);
-        this.set('errors', Errors.create());
-        this.set('hasValidated', emberA());
-    }
-});
 
 export default Controller.extend(SettingsSaveMixin, {
     config: service(),
@@ -51,33 +21,16 @@ export default Controller.extend(SettingsSaveMixin, {
         return url.slice(-1) !== '/' ? `${url}/` : url;
     }),
 
-    navigationItems: computed('model.navigation', function () {
-        let navItems;
-
-        try {
-            navItems = JSON.parse(this.get('model.navigation') || [{}]);
-        } catch (e) {
-            navItems = [{}];
-        }
-
-        navItems = navItems.map((item) => {
-            return NavItem.create(item);
-        });
-
-        return navItems;
-    }),
-
     init() {
         this._super(...arguments);
-        this.set('newNavItem', NavItem.create({isNew: true}));
+        this.set('newNavItem', NavigationItem.create({isNew: true}));
     },
 
     save() {
-        let navItems = this.get('navigationItems');
+        let navItems = this.get('model.navigation');
         let newNavItem = this.get('newNavItem');
         let notifications = this.get('notifications');
         let validationPromises = [];
-        let navSetting;
 
         if (!newNavItem.get('isBlank')) {
             validationPromises.pushObject(this.send('addItem'));
@@ -88,19 +41,6 @@ export default Controller.extend(SettingsSaveMixin, {
         });
 
         return RSVP.all(validationPromises).then(() => {
-            navSetting = navItems.map((item) => {
-                let label = item.get('label').trim();
-                let url = item.get('url').trim();
-
-                return {label, url};
-            }).compact();
-
-            this.set('model.navigation', JSON.stringify(navSetting));
-
-            // trigger change event because even if the final JSON is unchanged
-            // we need to have navigationItems recomputed.
-            this.get('model').notifyPropertyChange('navigation');
-
             return this.get('model').save().catch((err) => {
                 notifications.showErrors(err);
             });
@@ -110,12 +50,12 @@ export default Controller.extend(SettingsSaveMixin, {
     },
 
     addNewNavItem() {
-        let navItems = this.get('navigationItems');
+        let navItems = this.get('model.navigation');
         let newNavItem = this.get('newNavItem');
 
         newNavItem.set('isNew', false);
         navItems.pushObject(newNavItem);
-        this.set('newNavItem', NavItem.create({isNew: true}));
+        this.set('newNavItem', NavigationItem.create({isNew: true}));
     },
 
     actions: {
@@ -137,13 +77,13 @@ export default Controller.extend(SettingsSaveMixin, {
                 return;
             }
 
-            let navItems = this.get('navigationItems');
+            let navItems = this.get('model.navigation');
 
             navItems.removeObject(item);
         },
 
         reorderItems(navItems) {
-            this.set('navigationItems', navItems);
+            this.set('model.navigation', navItems);
         },
 
         updateUrl(url, navItem) {
@@ -155,7 +95,7 @@ export default Controller.extend(SettingsSaveMixin, {
         },
 
         reset() {
-            this.set('newNavItem', NavItem.create({isNew: true}));
+            this.set('newNavItem', NavigationItem.create({isNew: true}));
         }
     }
 });

ghost/admin/app/models/navigation-item.js

@@ -0,0 +1,27 @@
+import Ember from 'ember';
+import ValidationEngine from 'ghost/mixins/validation-engine';
+
+const {
+    computed,
+    isBlank
+} = Ember;
+
+export default Ember.Object.extend(ValidationEngine, {
+    label: '',
+    url: '',
+    isNew: false,
+
+    validationType: 'navItem',
+
+    isComplete: computed('label', 'url', function () {
+        let {label, url} = this.getProperties('label', 'url');
+
+        return !isBlank(label) && !isBlank(url);
+    }),
+
+    isBlank: computed('label', 'url', function () {
+        let {label, url} = this.getProperties('label', 'url');
+
+        return isBlank(label) && isBlank(url);
+    })
+});

ghost/admin/app/models/setting.js

@@ -19,7 +19,7 @@ export default Model.extend(ValidationEngine, {
     ghost_head: attr('string'),
     ghost_foot: attr('string'),
     labs: attr('string'),
-    navigation: attr('string'),
+    navigation: attr('navigation-settings'),
     isPrivate: attr('boolean'),
     password: attr('string')
 });

ghost/admin/app/templates/settings/navigation.hbs

@@ -9,7 +9,7 @@
     <section class="view-container">
         <form id="settings-navigation" class="gh-blognav" novalidate="novalidate">
             {{#sortable-group onChange=(action 'reorderItems') as |group|}}
-                {{#each navigationItems as |navItem|}}
+                {{#each model.navigation as |navItem|}}
                     {{gh-navitem navItem=navItem baseUrl=blogUrl addItem="addItem" deleteItem="deleteItem" updateUrl="updateUrl" group=group}}
                 {{/each}}
             {{/sortable-group}}

ghost/admin/app/transforms/navigation-settings.js

@@ -0,0 +1,41 @@
+import Ember from 'ember';
+import Transform from 'ember-data/transform';
+import NavigationItem from 'ghost/models/navigation-item';
+
+const {isArray} = Ember;
+const emberA = Ember.A;
+
+export default Transform.extend({
+    deserialize(serialized) {
+        let navItems, settingsArray;
+
+        try {
+            settingsArray = JSON.parse(serialized) || [];
+        } catch (e) {
+            settingsArray = [];
+        }
+
+        navItems = settingsArray.map((itemDetails) => {
+            return NavigationItem.create(itemDetails);
+        });
+
+        return emberA(navItems);
+    },
+
+    serialize(deserialized) {
+        let settingsArray;
+
+        if (isArray(deserialized)) {
+            settingsArray = deserialized.map((item) => {
+                let label = item.get('label').trim();
+                let url = item.get('url').trim();
+
+                return {label, url};
+            }).compact();
+        } else {
+            settingsArray = [];
+        }
+
+        return JSON.stringify(settingsArray);
+    }
+});

ghost/admin/tests/integration/components/gh-navigation-test.js

@@ -3,7 +3,7 @@ import { expect } from 'chai';
 import { describeComponent, it } from 'ember-mocha';
 import hbs from 'htmlbars-inline-precompile';
 import Ember from 'ember';
-import { NavItem } from 'ghost/controllers/settings/navigation';
+import NavItem from 'ghost/models/navigation-item';
 
 const {run} = Ember;
 

ghost/admin/tests/integration/components/gh-navitem-test.js

@@ -3,7 +3,7 @@ import { expect } from 'chai';
 import { describeComponent, it } from 'ember-mocha';
 import hbs from 'htmlbars-inline-precompile';
 import Ember from 'ember';
-import { NavItem } from 'ghost/controllers/settings/navigation';
+import NavItem from 'ghost/models/navigation-item';
 
 const {run} = Ember;
 

ghost/admin/tests/unit/controllers/settings/navigation-test.js

@@ -2,7 +2,7 @@
 import { expect, assert } from 'chai';
 import { describeModule, it } from 'ember-mocha';
 import Ember from 'ember';
-import { NavItem } from 'ghost/controllers/settings/navigation';
+import NavItem from 'ghost/models/navigation-item';
 
 const {run} = Ember;
 
@@ -22,7 +22,7 @@ describeModule(
     'Unit: Controller: settings/navigation',
     {
         // Specify the other units that are required for this test.
-        needs: ['service:config', 'service:notifications']
+        needs: ['service:config', 'service:notifications', 'model:navigation-item']
     },
     function () {
         it('blogUrl: captures config and ensures trailing slash', function () {
@@ -46,35 +46,23 @@ describeModule(
             expect(ctrl.get('blogUrl')).to.equal('http://localhost:2368/blog/');
         });
 
-        it('navigationItems: generates list of NavItems', function () {
-            let ctrl = this.subject();
-
-            run(() => {
-                ctrl.set('model', Ember.Object.create({navigation: navSettingJSON}));
-                expect(ctrl.get('navigationItems.length')).to.equal(8);
-                expect(ctrl.get('navigationItems.firstObject.label')).to.equal('Home');
-                expect(ctrl.get('navigationItems.firstObject.url')).to.equal('/');
-                expect(ctrl.get('navigationItems.firstObject.isNew')).to.be.false;
-            });
-        });
-
         it('save: validates nav items', function (done) {
             let ctrl = this.subject();
 
             run(() => {
-                ctrl.set('model', Ember.Object.create({navigation: `[
-                    {"label":"First",   "url":"/"},
-                    {"label":"",        "url":"/second"},
-                    {"label":"Third",   "url":""}
-                ]`}));
+                ctrl.set('model', Ember.Object.create({navigation: [
+                    NavItem.create({label: 'First',   url: '/'}),
+                    NavItem.create({label: '',        url: '/second'}),
+                    NavItem.create({label: 'Third',   url: ''})
+                ]}));
                 // blank item won't get added because the last item is incomplete
-                expect(ctrl.get('navigationItems.length')).to.equal(3);
+                expect(ctrl.get('model.navigation.length')).to.equal(3);
 
                 ctrl.save().then(function passedValidation() {
                     assert(false, 'navigationItems weren\'t validated on save');
                     done();
                 }).catch(function failedValidation() {
-                    let navItems = ctrl.get('navigationItems');
+                    let navItems = ctrl.get('model.navigation');
                     expect(navItems[0].get('errors').toArray()).to.be.empty;
                     expect(navItems[1].get('errors.firstObject.attribute')).to.equal('label');
                     expect(navItems[2].get('errors.firstObject.attribute')).to.equal('url');
@@ -87,61 +75,34 @@ describeModule(
             let ctrl = this.subject();
 
             run(() => {
-                ctrl.set('model', Ember.Object.create({navigation: `[
-                    {"label":"First",   "url":"/"},
-                    {"label":"",        "url":""}
-                ]`}));
+                ctrl.set('model', Ember.Object.create({navigation: [
+                    NavItem.create({label: 'First',   url: '/'}),
+                    NavItem.create({label: '',        url: ''})
+                ]}));
 
-                expect(ctrl.get('navigationItems.length')).to.equal(2);
+                expect(ctrl.get('model.navigation.length')).to.equal(2);
 
                 ctrl.save().then(function passedValidation() {
                     assert(false, 'navigationItems weren\'t validated on save');
                     done();
                 }).catch(function failedValidation() {
-                    let navItems = ctrl.get('navigationItems');
+                    let navItems = ctrl.get('model.navigation');
                     expect(navItems[0].get('errors').toArray()).to.be.empty;
                     done();
                 });
             });
         });
 
-        it('save: generates new navigation JSON', function (done) {
-            let ctrl = this.subject();
-            let model = Ember.Object.create({navigation: {}});
-            let expectedJSON = `[{"label":"New","url":"/new"}]`;
-
-            model.save = function () {
-                return new Ember.RSVP.Promise((resolve, reject) => {
-                    return resolve(this);
-                });
-            };
-
-            run(() => {
-                ctrl.set('model', model);
-
-                // remove inserted blank item so validation works
-                ctrl.get('navigationItems').removeObject(ctrl.get('navigationItems.firstObject'));
-                // add new object
-                ctrl.get('navigationItems').addObject(NavItem.create({label: 'New', url: '/new'}));
-
-                ctrl.save().then(function success() {
-                    expect(ctrl.get('model.navigation')).to.equal(expectedJSON);
-                    done();
-                }, function failure() {
-                    assert(false, 'save failed with valid data');
-                    done();
-                });
-            });
-        });
-
         it('action - addItem: adds item to navigationItems', function () {
             let ctrl = this.subject();
 
             run(() => {
-                ctrl.set('navigationItems', [NavItem.create({label: 'First', url: '/first', last: true})]);
+                ctrl.set('model', Ember.Object.create({navigation: [
+                    NavItem.create({label: 'First', url: '/first', last: true})
+                ]}));
             });
 
-            expect(ctrl.get('navigationItems.length')).to.equal(1);
+            expect(ctrl.get('model.navigation.length')).to.equal(1);
 
             ctrl.set('newNavItem.label', 'New');
             ctrl.set('newNavItem.url', '/new');
@@ -150,10 +111,10 @@ describeModule(
                 ctrl.send('addItem');
             });
 
-            expect(ctrl.get('navigationItems.length')).to.equal(2);
-            expect(ctrl.get('navigationItems.lastObject.label')).to.equal('New');
-            expect(ctrl.get('navigationItems.lastObject.url')).to.equal('/new');
-            expect(ctrl.get('navigationItems.lastObject.isNew')).to.be.false;
+            expect(ctrl.get('model.navigation.length')).to.equal(2);
+            expect(ctrl.get('model.navigation.lastObject.label')).to.equal('New');
+            expect(ctrl.get('model.navigation.lastObject.url')).to.equal('/new');
+            expect(ctrl.get('model.navigation.lastObject.isNew')).to.be.false;
             expect(ctrl.get('newNavItem.label')).to.be.blank;
             expect(ctrl.get('newNavItem.url')).to.be.blank;
             expect(ctrl.get('newNavItem.isNew')).to.be.true;
@@ -163,10 +124,12 @@ describeModule(
             let ctrl = this.subject();
 
             run(() => {
-                ctrl.set('navigationItems', [NavItem.create({label: '', url: '', last: true})]);
-                expect(ctrl.get('navigationItems.length')).to.equal(1);
+                ctrl.set('model', Ember.Object.create({navigation: [
+                    NavItem.create({label: '', url: '', last: true})
+                ]}));
+                expect(ctrl.get('model.navigation.length')).to.equal(1);
                 ctrl.send('addItem');
-                expect(ctrl.get('navigationItems.length')).to.equal(1);
+                expect(ctrl.get('model.navigation.length')).to.equal(1);
             });
         });
 
@@ -178,10 +141,10 @@ describeModule(
             ];
 
             run(() => {
-                ctrl.set('navigationItems', navItems);
-                expect(ctrl.get('navigationItems').mapBy('label')).to.deep.equal(['First', 'Second']);
-                ctrl.send('deleteItem', ctrl.get('navigationItems.firstObject'));
-                expect(ctrl.get('navigationItems').mapBy('label')).to.deep.equal(['Second']);
+                ctrl.set('model', Ember.Object.create({navigation: navItems}));
+                expect(ctrl.get('model.navigation').mapBy('label')).to.deep.equal(['First', 'Second']);
+                ctrl.send('deleteItem', ctrl.get('model.navigation.firstObject'));
+                expect(ctrl.get('model.navigation').mapBy('label')).to.deep.equal(['Second']);
             });
         });
 
@@ -193,10 +156,10 @@ describeModule(
             ];
 
             run(() => {
-                ctrl.set('navigationItems', navItems);
-                expect(ctrl.get('navigationItems').mapBy('label')).to.deep.equal(['First', 'Second']);
+                ctrl.set('model', Ember.Object.create({navigation: navItems}));
+                expect(ctrl.get('model.navigation').mapBy('label')).to.deep.equal(['First', 'Second']);
                 ctrl.send('reorderItems', navItems.reverseObjects());
-                expect(ctrl.get('navigationItems').mapBy('label')).to.deep.equal(['Second', 'First']);
+                expect(ctrl.get('model.navigation').mapBy('label')).to.deep.equal(['Second', 'First']);
             });
         });
 
@@ -208,10 +171,10 @@ describeModule(
             ];
 
             run(() => {
-                ctrl.set('navigationItems', navItems);
-                expect(ctrl.get('navigationItems').mapBy('url')).to.deep.equal(['/first', '/second']);
-                ctrl.send('updateUrl', '/new', ctrl.get('navigationItems.firstObject'));
-                expect(ctrl.get('navigationItems').mapBy('url')).to.deep.equal(['/new', '/second']);
+                ctrl.set('model', Ember.Object.create({navigation: navItems}));
+                expect(ctrl.get('model.navigation').mapBy('url')).to.deep.equal(['/first', '/second']);
+                ctrl.send('updateUrl', '/new', ctrl.get('model.navigation.firstObject'));
+                expect(ctrl.get('model.navigation').mapBy('url')).to.deep.equal(['/new', '/second']);
             });
         });
     }

ghost/admin/tests/unit/models/navigation-item-test.js

@@ -0,0 +1,67 @@
+/* jshint expr:true */
+import { expect } from 'chai';
+import { describeModule, it } from 'ember-mocha';
+
+describeModule(
+    'model:navigation-item',
+    'Unit: Model: navigation-item',
+    {
+        // Specify the other units that are required for this test.
+        needs: []
+    },
+    function() {
+        it('isComplete is true when label and url are filled', function () {
+            let model = this.subject();
+
+            model.set('label', 'test');
+            model.set('url', 'test');
+
+            expect(model.get('isComplete')).to.be.true;
+        });
+
+        it('isComplete is false when label is blank', function () {
+            let model = this.subject();
+
+            model.set('label', '');
+            model.set('url', 'test');
+
+            expect(model.get('isComplete')).to.be.false;
+        });
+
+        it('isComplete is false when url is blank', function () {
+            let model = this.subject();
+
+            model.set('label', 'test');
+            model.set('url', '');
+
+            expect(model.get('isComplete')).to.be.false;
+        });
+
+        it('isBlank is true when label and url are blank', function () {
+            let model = this.subject();
+
+            model.set('label', '');
+            model.set('url', '');
+
+            expect(model.get('isBlank')).to.be.true;
+        });
+
+        it('isBlank is false when label is present', function () {
+            let model = this.subject();
+
+            model.set('label', 'test');
+            model.set('url', '');
+
+            expect(model.get('isBlank')).to.be.false;
+        });
+
+        it('isBlank is false when url is present', function () {
+            let model = this.subject();
+
+            model.set('label', '');
+            model.set('url', 'test');
+
+            expect(model.get('isBlank')).to.be.false;
+        });
+    }
+);

ghost/admin/tests/unit/transforms/navigation-settings-test.js

@@ -0,0 +1,42 @@
+/* jshint expr:true */
+import { expect } from 'chai';
+import { describeModule, it } from 'ember-mocha';
+import Ember from 'ember';
+import NavigationItem from 'ghost/models/navigation-item';
+
+const emberA = Ember.A;
+
+describeModule(
+    'transform:navigation-settings',
+    'Unit: Transform: navigation-settings',
+    {
+        // Specify the other units that are required for this test.
+        // needs: ['transform:foo']
+    },
+    function() {
+        it('deserializes navigation json', function () {
+            let transform = this.subject();
+            let serialized = '[{"label":"One","url":"/one"},{"label":"Two","url":"/two"}]';
+            let result = transform.deserialize(serialized);
+
+            expect(result.length).to.equal(2);
+            expect(result[0]).to.be.instanceof(NavigationItem);
+            expect(result[0].get('label')).to.equal('One');
+            expect(result[0].get('url')).to.equal('/one');
+            expect(result[1]).to.be.instanceof(NavigationItem);
+            expect(result[1].get('label')).to.equal('Two');
+            expect(result[1].get('url')).to.equal('/two');
+        });
+
+        it('serializes array of NavigationItems', function () {
+            let transform = this.subject();
+            let deserialized = emberA([
+                NavigationItem.create({label: 'One', url: '/one'}),
+                NavigationItem.create({label: 'Two', url: '/two'})
+            ]);
+            let result = transform.serialize(deserialized);
+
+            expect(result).to.equal('[{"label":"One","url":"/one"},{"label":"Two","url":"/two"}]');
+        });
+    }
+);

ghost/admin/tests/unit/validators/nav-item-test.js

@@ -5,7 +5,7 @@ import {
     it
 } from 'mocha';
 import validator from 'ghost/validators/nav-item';
-import { NavItem } from 'ghost/controllers/settings/navigation';
+import NavItem from 'ghost/models/navigation-item';
 
 const testInvalidUrl = function (url) {
     let navItem = NavItem.create({url});