Frontend controller refactor & test improvements

no issue

- Split context out of frontend controller
- Add 100% test coverage for context
- Add tests for preview & improve other bits of coverage
- Further refactors (WIP) will make it easier to reach 100% coverage on the frontend

core/server/controllers/frontend/context.js

@@ -0,0 +1,60 @@
+/**
+ * # Response context
+ *
+ * Figures out which context we are currently serving. The biggest challenge with determining this
+ * is that the only way to determine whether or not we are a post, or a page, is with data after all the
+ * data for the template has been retrieved.
+ *
+ * Contexts are determined based on 3 pieces of information
+ * 1. res.locals.relativeUrl - which never includes the subdirectory
+ * 2. req.params.page - always has the page parameter, regardless of if the URL contains a keyword (RSS pages don't)
+ * 3. data - used for telling the difference between posts and pages
+ */
+
+var config = require('../../config'),
+
+    // Context patterns, should eventually come from Channel configuration
+    tagPattern = new RegExp('^\\/' + config.routeKeywords.tag + '\\/.+'),
+    authorPattern = new RegExp('^\\/' + config.routeKeywords.author + '\\/.+'),
+    privatePattern = new RegExp('^\\/' + config.routeKeywords.private + '\\/'),
+    indexPattern = new RegExp('^\\/' + config.routeKeywords.page + '\\/'),
+    rssPattern = new RegExp('^\\/rss\\/'),
+    homePattern = new RegExp('^\\/$');
+
+function setResponseContext(req, res, data) {
+    var pageParam = req.params && req.params.page !== undefined ? parseInt(req.params.page, 10) : 1;
+
+    res.locals = res.locals || {};
+    res.locals.context = [];
+
+    // If we don't have a relativeUrl, we can't detect the context, so return
+    if (!res.locals.relativeUrl) {
+        return;
+    }
+
+    // paged context
+    if (!isNaN(pageParam) && pageParam > 1) {
+        res.locals.context.push('paged');
+    }
+
+    if (indexPattern.test(res.locals.relativeUrl)) {
+        res.locals.context.push('index');
+    } else if (homePattern.test(res.locals.relativeUrl)) {
+        res.locals.context.push('home');
+        res.locals.context.push('index');
+    } else if (rssPattern.test(res.locals.relativeUrl)) {
+        res.locals.context.push('rss');
+    } else if (privatePattern.test(res.locals.relativeUrl)) {
+        res.locals.context.push('private');
+    } else if (tagPattern.test(res.locals.relativeUrl)) {
+        res.locals.context.push('tag');
+    } else if (authorPattern.test(res.locals.relativeUrl)) {
+        res.locals.context.push('author');
+    } else if (data && data.post && data.post.page) {
+        res.locals.context.push('page');
+    } else {
+        res.locals.context.push('post');
+    }
+}
+
+module.exports = setResponseContext;

core/server/controllers/frontend/index.js

@@ -5,16 +5,17 @@
 /*global require, module */
 
 var _           = require('lodash'),
-    api         = require('../api'),
+    api         = require('../../api'),
-    rss         = require('../data/xml/rss'),
+    rss         = require('../../data/xml/rss'),
     path        = require('path'),
-    config      = require('../config'),
+    config      = require('../../config'),
-    errors      = require('../errors'),
+    errors      = require('../../errors'),
-    filters     = require('../filters'),
+    filters     = require('../../filters'),
     Promise     = require('bluebird'),
-    template    = require('../helpers/template'),
+    template    = require('../../helpers/template'),
     routeMatch  = require('path-match')(),
-    safeString  = require('../utils/index').safeString,
+    safeString  = require('../../utils/index').safeString,
+    setResponseContext = require('./context'),
 
     frontendControllers,
     staticPostPermalink = routeMatch('/:slug/:edit?');
@@ -69,42 +70,6 @@ function handleError(next) {
     };
 }
 
-function setResponseContext(req, res, data) {
-    var contexts = [],
-        pageParam = req.params.page !== undefined ? parseInt(req.params.page, 10) : 1,
-        tagPattern = new RegExp('^\\/' + config.routeKeywords.tag + '\\/.+'),
-        authorPattern = new RegExp('^\\/' + config.routeKeywords.author + '\\/.+'),
-        privatePattern = new RegExp('^\\/' + config.routeKeywords.private + '\\/'),
-        indexPattern = new RegExp('^\\/' + config.routeKeywords.page + '\\/'),
-        homePattern = new RegExp('^\\/$');
-
-    // paged context
-    if (!isNaN(pageParam) && pageParam > 1) {
-        contexts.push('paged');
-    }
-
-    if (indexPattern.test(res.locals.relativeUrl)) {
-        contexts.push('index');
-    } else if (homePattern.test(res.locals.relativeUrl)) {
-        contexts.push('home');
-        contexts.push('index');
-    } else if (/^\/rss\//.test(res.locals.relativeUrl)) {
-        contexts.push('rss');
-    } else if (privatePattern.test(res.locals.relativeUrl)) {
-        contexts.push('private');
-    } else if (tagPattern.test(res.locals.relativeUrl)) {
-        contexts.push('tag');
-    } else if (authorPattern.test(res.locals.relativeUrl)) {
-        contexts.push('author');
-    } else if (data && data.post && data.post.page) {
-        contexts.push('page');
-    } else {
-        contexts.push('post');
-    }
-
-    res.locals.context = contexts;
-}
-
 // Add Request context parameter to the data object
 // to be passed down to the templates
 function setReqCtx(req, data) {

core/test/unit/controllers/frontend/context_spec.js

@@ -0,0 +1,248 @@
+/*globals describe, beforeEach, it*/
+var should   = require('should'),
+
+    // Stuff we are testing
+    setResponseContext = require('../../../../server/controllers/frontend/context');
+
+describe('Contexts', function () {
+    var req, res, data;
+
+    beforeEach(function () {
+        req = {
+            params: {}
+        };
+        res = {
+            locals: {}
+        };
+        data = {};
+    });
+
+    describe('Unknown', function () {
+        it('should return empty array with no error if all parameters are empty', function () {
+            // Reset all parameters to empty;
+            req = {};
+            res = {};
+            data = {};
+            setResponseContext(req, res, data);
+
+            should.exist(res.locals.context);
+            res.locals.context.should.be.an.Array.with.lengthOf(0);
+        });
+
+        it('should return empty array with no error with basic parameters', function () {
+            // BeforeEach sets each of these to the bare minimum that should be provided for determining context
+            setResponseContext(req, res, data);
+
+            should.exist(res.locals.context);
+            res.locals.context.should.be.an.Array.with.lengthOf(0);
+        });
+    });
+
+    describe('Index', function () {
+        it('should correctly identify `/` as index', function () {
+            // Setup test by setting relativeUrl
+            res.locals.relativeUrl = '/';
+
+            setResponseContext(req, res, data);
+
+            should.exist(res.locals.context);
+            res.locals.context.should.be.an.Array.with.lengthOf(2);
+            res.locals.context[0].should.eql('home');
+            res.locals.context[1].should.eql('index');
+        });
+
+        it('should correctly identify `/` as home & index', function () {
+            // Setup test by setting relativeUrl
+            res.locals.relativeUrl = '/';
+
+            setResponseContext(req, res, data);
+
+            should.exist(res.locals.context);
+            res.locals.context.should.be.an.Array.with.lengthOf(2);
+            res.locals.context[0].should.eql('home');
+            res.locals.context[1].should.eql('index');
+        });
+
+        it('will not identify `/page/2/` as index & paged without page param', function () {
+            // Setup test by setting relativeUrl
+            res.locals.relativeUrl = '/page/2/';
+
+            setResponseContext(req, res, data);
+
+            should.exist(res.locals.context);
+            res.locals.context.should.be.an.Array.with.lengthOf(1);
+            res.locals.context[0].should.eql('index');
+        });
+
+        it('should identify `/page/2/` as index & paged with page param', function () {
+            // Setup test by setting relativeUrl
+            res.locals.relativeUrl = '/page/2/';
+            req.params.page = 2;
+
+            setResponseContext(req, res, data);
+
+            should.exist(res.locals.context);
+            res.locals.context.should.be.an.Array.with.lengthOf(2);
+            res.locals.context[0].should.eql('paged');
+            res.locals.context[1].should.eql('index');
+        });
+    });
+
+    describe('RSS', function () {
+        it('should correctly identify `/rss/` as rss', function () {
+            // Setup test by setting relativeUrl
+            res.locals.relativeUrl = '/rss/';
+
+            setResponseContext(req, res, data);
+
+            should.exist(res.locals.context);
+            res.locals.context.should.be.an.Array.with.lengthOf(1);
+            res.locals.context[0].should.eql('rss');
+        });
+
+        it('will not identify `/rss/2/` as rss & paged without page param', function () {
+            // Setup test by setting relativeUrl
+            res.locals.relativeUrl = '/rss/2/';
+
+            setResponseContext(req, res, data);
+
+            should.exist(res.locals.context);
+            res.locals.context.should.be.an.Array.with.lengthOf(1);
+            res.locals.context[0].should.eql('rss');
+        });
+
+        it('should correctly identify `/rss/2/` as rss & paged with page param', function () {
+            // Setup test by setting relativeUrl
+            res.locals.relativeUrl = '/rss/2/';
+            req.params.page = 2;
+
+            setResponseContext(req, res, data);
+
+            should.exist(res.locals.context);
+            res.locals.context.should.be.an.Array.with.lengthOf(2);
+            res.locals.context[0].should.eql('paged');
+            res.locals.context[1].should.eql('rss');
+        });
+    });
+
+    describe('Tag', function () {
+        it('should correctly identify `/tag/getting-started/` as tag', function () {
+            // Setup test by setting relativeUrl
+            res.locals.relativeUrl = '/tag/getting-started/';
+
+            setResponseContext(req, res, data);
+
+            should.exist(res.locals.context);
+            res.locals.context.should.be.an.Array.with.lengthOf(1);
+            res.locals.context[0].should.eql('tag');
+        });
+
+        it('should not identify just `/tag/` as being the tag context', function () {
+            // Setup test by setting relativeUrl
+            res.locals.relativeUrl = '/tag/';
+
+            setResponseContext(req, res, data);
+
+            should.exist(res.locals.context);
+            res.locals.context.should.be.an.Array.with.lengthOf(1);
+            res.locals.context[0].should.eql('post');
+        });
+
+        it('will not identify `/tag/getting-started/page/2/ as paged without page param', function () {
+            // Setup test by setting relativeUrl
+            res.locals.relativeUrl = '/tag/getting-started/page/2/';
+
+            setResponseContext(req, res, data);
+
+            should.exist(res.locals.context);
+            res.locals.context.should.be.an.Array.with.lengthOf(1);
+            res.locals.context[0].should.eql('tag');
+        });
+
+        it('should correctly identify `/tag/getting-started/page/2/ as paged with page param', function () {
+            // Setup test by setting relativeUrl
+            res.locals.relativeUrl = '/tag/getting-started/page/2/';
+            req.params.page = 2;
+
+            setResponseContext(req, res, data);
+
+            should.exist(res.locals.context);
+            res.locals.context.should.be.an.Array.with.lengthOf(2);
+            res.locals.context[0].should.eql('paged');
+            res.locals.context[1].should.eql('tag');
+        });
+    });
+
+    describe('Author', function () {
+        it('should correctly identify `/author/pat/` as author', function () {
+            // Setup test by setting relativeUrl
+            res.locals.relativeUrl = '/author/pat/';
+
+            setResponseContext(req, res, data);
+
+            should.exist(res.locals.context);
+            res.locals.context.should.be.an.Array.with.lengthOf(1);
+            res.locals.context[0].should.eql('author');
+        });
+
+        it('should not identify just `/author/` as being the author context', function () {
+            // Setup test by setting relativeUrl
+            res.locals.relativeUrl = '/author/';
+
+            setResponseContext(req, res, data);
+
+            should.exist(res.locals.context);
+            res.locals.context.should.be.an.Array.with.lengthOf(1);
+            res.locals.context[0].should.eql('post');
+        });
+
+        it('will not identify `/author/pat/page/2/ as paged without page param', function () {
+            // Setup test by setting relativeUrl
+            res.locals.relativeUrl = '/author/pat/page/2/';
+
+            setResponseContext(req, res, data);
+
+            should.exist(res.locals.context);
+            res.locals.context.should.be.an.Array.with.lengthOf(1);
+            res.locals.context[0].should.eql('author');
+        });
+
+        it('should correctly identify `/author/pat/page/2/ as paged with page param', function () {
+            // Setup test by setting relativeUrl
+            res.locals.relativeUrl = '/author/pat/page/2/';
+            req.params.page = 2;
+
+            setResponseContext(req, res, data);
+
+            should.exist(res.locals.context);
+            res.locals.context.should.be.an.Array.with.lengthOf(2);
+            res.locals.context[0].should.eql('paged');
+            res.locals.context[1].should.eql('author');
+        });
+    });
+
+    describe('Posts & Pages', function () {
+        it('should correctly identify a post', function () {
+            // Setup test by setting relativeUrl
+            res.locals.relativeUrl = '/welcome-to-ghost/';
+
+            setResponseContext(req, res, data);
+
+            should.exist(res.locals.context);
+            res.locals.context.should.be.an.Array.with.lengthOf(1);
+            res.locals.context[0].should.eql('post');
+        });
+
+        it('should correctly idenfity a page', function () {
+            // Setup test by setting relativeUrl
+            res.locals.relativeUrl = '/about/';
+            data.post = {page: true};
+
+            setResponseContext(req, res, data);
+
+            should.exist(res.locals.context);
+            res.locals.context.should.be.an.Array.with.lengthOf(1);
+            res.locals.context[0].should.eql('page');
+        });
+    });
+});

core/test/unit/controllers/frontend/index_spec.js

@@ -8,12 +8,12 @@ var moment   = require('moment'),
     _        = require('lodash'),
 
 // Stuff we are testing
-    api      = require('../../server/api'),
+    api      = require('../../../../server/api'),
 
-    frontend = rewire('../../server/controllers/frontend'),
+    frontend = rewire('../../../../server/controllers/frontend'),
-    config   = require('../../server/config'),
+    config   = require('../../../../server/config'),
     origConfig = _.cloneDeep(config),
-    defaultConfig  = require('../../../config.example')[process.env.NODE_ENV];
+    defaultConfig  = require('../../../../../config.example')[process.env.NODE_ENV];
 
 // To stop jshint complaining
 should.equal(true, true);
@@ -31,7 +31,7 @@ describe('Frontend Controller', function () {
         sandbox = sinon.sandbox.create();
 
         // Reset frontend controller for next test
-        frontend = rewire('../../server/controllers/frontend');
+        frontend = rewire('../../../../server/controllers/frontend');
     });
 
     afterEach(function () {
@@ -43,7 +43,8 @@ describe('Frontend Controller', function () {
     // from failing via timeout when they
     // should just immediately fail
     function failTest(done, msg) {
-        return function () {
+        return function (stuf) {
+            console.log(msg, stuf);
             done(new Error(msg));
         };
     }
@@ -574,9 +575,10 @@ describe('Frontend Controller', function () {
 
         beforeEach(function () {
             sandbox.stub(api.posts, 'read', function (args) {
-                return Promise.resolve(_.find(mockPosts, function (mock) {
+                var post = _.find(mockPosts, function (mock) {
                     return mock.posts[0].slug === args.slug;
-                }));
+                });
+                return Promise.resolve(post || {posts: []});
             });
 
             apiSettingsStub = sandbox.stub(api.settings, 'read');
@@ -945,6 +947,24 @@ describe('Frontend Controller', function () {
                         done();
                     });
                 });
+
+                it('should call next if post is not found', function (done) {
+                    var req = {
+                            path: '/unknown/'
+                        },
+                        res = {
+                            locals: {},
+                            render: sinon.spy(),
+                            redirect: sinon.spy()
+                        };
+
+                    frontend.single(req, res, function (err) {
+                        should.not.exist(err);
+                        res.render.called.should.be.false;
+                        res.redirect.called.should.be.false;
+                        done();
+                    });
+                });
             });
 
             describe('permalink set to date', function () {
@@ -1331,6 +1351,38 @@ describe('Frontend Controller', function () {
                     });
                 });
             });
+
+            describe('permalink set to custom format no slash', function () {
+                beforeEach(function () {
+                    apiSettingsStub.withArgs('permalinks').returns(Promise.resolve({
+                        settings: [{
+                            value: '/:year/:slug'
+                        }]
+                    }));
+
+                    var date = moment(mockPosts[1].posts[0].published_at).format('YYYY');
+                    mockPosts[1].posts[0].url = '/' + date + '/' + mockPosts[1].posts[0].slug + '/';
+                });
+
+                // Handle Edit append
+                it('will redirect post to admin edit page via /:year/:id/edit/', function (done) {
+                    var date = moment(mockPosts[1].posts[0].published_at).format('YYYY'),
+                        req = {
+                            path: '/' + [date, mockPosts[1].posts[0].slug, 'edit'].join('/') + '/'
+                        },
+                        res = {
+                            locals: {},
+                            render: sinon.spy(),
+                            redirect: function (arg) {
+                                res.render.called.should.be.false;
+                                arg.should.eql(adminEditPagePath + mockPosts[1].posts[0].id + '/');
+                                done();
+                            }
+                        };
+
+                    frontend.single(req, res, failTest(done));
+                });
+            });
         });
     });
 
@@ -1342,7 +1394,8 @@ describe('Frontend Controller', function () {
                 route: {path: '/private/?r=/'},
                 query: {r: ''},
                 params: {}
-            },
+            };
+
             config = {
                 paths: {
                     adminViews: '/core/server/views',
@@ -1406,4 +1459,177 @@ describe('Frontend Controller', function () {
             frontend.private(req, res, failTest(done));
         });
     });
+
+    describe('preview', function () {
+        var mockPosts = [{
+            posts: [{
+                status: 'draft',
+                uuid: 'abc-1234-01',
+                id: 1,
+                title: 'Test static page',
+                slug: 'test-static-page',
+                markdown: 'Test static page content',
+                page: 1,
+                author: {
+                    id: 1,
+                    name: 'Test User',
+                    slug: 'test',
+                    email: 'test@ghost.org'
+                },
+                url: '/test-static-page/'
+            }]
+        }, {
+            posts: [{
+                status: 'draft',
+                uuid: 'abc-1234-02',
+                id: 2,
+                title: 'Test normal post',
+                slug: 'test-normal-post',
+                markdown: 'The test normal post content',
+                page: 0,
+                author: {
+                    id: 1,
+                    name: 'Test User',
+                    slug: 'test',
+                    email: 'test@ghost.org'
+                }
+            }]
+        }, {
+            posts: [{
+                status: 'published',
+                uuid: 'abc-1234-03',
+                id: 3,
+                title: 'Getting started',
+                slug: 'about',
+                markdown: 'This is a blog post',
+                page: 0,
+                published_at: new Date('2014/1/30').getTime(),
+                author: {
+                    id: 1,
+                    name: 'Test User',
+                    slug: 'test',
+                    email: 'test@ghost.org'
+                },
+                url: '/getting-started/'
+            }]
+        }];
+
+        beforeEach(function () {
+            sandbox.stub(api.posts, 'read', function (args) {
+                var post = _.find(mockPosts, function (mock) {
+                    return mock.posts[0].uuid === args.uuid;
+                });
+                return Promise.resolve(post || {posts: []});
+            });
+
+            apiSettingsStub = sandbox.stub(api.settings, 'read');
+
+            apiSettingsStub.withArgs(sinon.match.has('key', 'activeTheme')).returns(Promise.resolve({
+                settings: [{
+                    key: 'activeTheme',
+                    value: 'casper'
+                }]
+            }));
+
+            frontend.__set__('config', {
+                paths: {
+                    subdir: '',
+                    availableThemes: {
+                        casper: {
+                            assets: null,
+                            'default.hbs': '/content/themes/casper/default.hbs',
+                            'index.hbs': '/content/themes/casper/index.hbs',
+                            'page.hbs': '/content/themes/casper/page.hbs',
+                            'page-about.hbs': '/content/themes/casper/page-about.hbs',
+                            'post.hbs': '/content/themes/casper/post.hbs'
+                        }
+                    }
+                },
+                routeKeywords: {
+                    page: 'page',
+                    tag: 'tag',
+                    author: 'author'
+                },
+                urlFor: function (type, posts) {
+                    return posts.post.url;
+                }
+            });
+        });
+
+        it('should render draft post', function (done) {
+            var req = {
+                    params: {
+                        uuid: 'abc-1234-02'
+                    }
+                },
+                res = {
+                    render: function (view, context) {
+                        view.should.equal('post');
+                        should.exist(context.post);
+                        context.post.should.equal(mockPosts[1].posts[0]);
+                        done();
+                    }
+                };
+
+            frontend.preview(req, res, failTest(done));
+        });
+
+        it('should render draft page', function (done) {
+            var req = {
+                    params: {
+                        uuid: 'abc-1234-01'
+                    }
+                },
+                res = {
+                    render: function (view, context) {
+                        view.should.equal('page');
+                        should.exist(context.post);
+                        context.post.should.equal(mockPosts[0].posts[0]);
+                        done();
+                    }
+                };
+
+            frontend.preview(req, res, failTest(done));
+        });
+
+        it('should call next if post is not found', function (done) {
+            var req = {
+                    params: {
+                        uuid: 'abc-1234-04'
+                    }
+                },
+                res = {
+                    locals: {},
+                    render: sinon.spy(),
+                    redirect: sinon.spy()
+                };
+
+            frontend.preview(req, res, function (err) {
+                should.not.exist(err);
+                res.render.called.should.be.false;
+                res.redirect.called.should.be.false;
+                done();
+            });
+        });
+
+        it('should call redirect if post is published', function (done) {
+            var req = {
+                    params: {
+                        uuid: 'abc-1234-03'
+                    }
+                },
+                res = {
+                    locals: {},
+                    render: sinon.spy(),
+                    redirect: function (status, url) {
+                        res.render.called.should.be.false;
+                        status.should.eql(301);
+                        url.should.eql('/getting-started/');
+                        done();
+                    }
+                };
+
+            frontend.preview(req, res, failTest(done));
+        });
+    });
 });