Merge pull request TryGhost#5985 from ErisDS/frontend-split

Further split up frontend controller & improve tests

core/server/controllers/frontend/error.js

@@ -0,0 +1,12 @@
+function handleError(next) {
+    return function handleError(err) {
+        // If we've thrown an error message of type: 'NotFound' then we found no path match.
+        if (err.errorType === 'NotFoundError') {
+            return next();
+        }
+
+        return next(err);
+    };
+}
+
+module.exports = handleError;

core/server/controllers/frontend/fetch-data.js

@@ -0,0 +1,17 @@
+var api = require('../../api');
+
+function fetchData(options) {
+    return api.settings.read('postsPerPage').then(function then(response) {
+        var postPP = response.settings[0],
+            postsPerPage = parseInt(postPP.value, 10);
+
+        // No negative posts per page, must be number
+        if (!isNaN(postsPerPage) && postsPerPage > 0) {
+            options.limit = postsPerPage;
+        }
+        options.include = 'author,tags,fields';
+        return api.posts.browse(options);
+    });
+}
+
+module.exports = fetchData;

core/server/controllers/frontend/format-response.js

@@ -0,0 +1,31 @@
+var _   = require('lodash');
+
+/**
+ * formats variables for handlebars in multi-post contexts.
+ * If extraValues are available, they are merged in the final value
+ * @return {Object} containing page variables
+ */
+function formatPageResponse(posts, page, extraValues) {
+    extraValues = extraValues || {};
+
+    var resp = {
+        posts: posts,
+        pagination: page.meta.pagination
+    };
+    return _.extend(resp, extraValues);
+}
+
+/**
+ * similar to formatPageResponse, but for single post pages
+ * @return {Object} containing page variables
+ */
+function formatResponse(post) {
+    return {
+        post: post
+    };
+}
+
+module.exports = {
+    channel: formatPageResponse,
+    single: formatResponse
+};

core/server/controllers/frontend/index.js

@@ -15,87 +15,16 @@ var _           = require('lodash'),
     template    = require('../../helpers/template'),
     routeMatch  = require('path-match')(),
     safeString  = require('../../utils/index').safeString,
+    handleError = require('./error'),
+    fetchData   = require('./fetch-data'),
+    formatResponse = require('./format-response'),
     setResponseContext = require('./context'),
+    setRequestIsSecure   = require('./secure'),
+    getActiveThemePaths = require('./theme-paths'),
 
     frontendControllers,
     staticPostPermalink = routeMatch('/:slug/:edit?');
 
-function getPostPage(options) {
-    return api.settings.read('postsPerPage').then(function then(response) {
-        var postPP = response.settings[0],
-            postsPerPage = parseInt(postPP.value, 10);
-
-        // No negative posts per page, must be number
-        if (!isNaN(postsPerPage) && postsPerPage > 0) {
-            options.limit = postsPerPage;
-        }
-        options.include = 'author,tags,fields';
-        return api.posts.browse(options);
-    });
-}
-
-/**
- * formats variables for handlebars in multi-post contexts.
- * If extraValues are available, they are merged in the final value
- * @return {Object} containing page variables
- */
-function formatPageResponse(posts, page, extraValues) {
-    extraValues = extraValues || {};
-
-    var resp = {
-        posts: posts,
-        pagination: page.meta.pagination
-    };
-    return _.extend(resp, extraValues);
-}
-
-/**
- * similar to formatPageResponse, but for single post pages
- * @return {Object} containing page variables
- */
-function formatResponse(post) {
-    return {
-        post: post
-    };
-}
-
-function handleError(next) {
-    return function handleError(err) {
-        // If we've thrown an error message of type: 'NotFound' then we found no path match.
-        if (err.errorType === 'NotFoundError') {
-            return next();
-        }
-
-        return next(err);
-    };
-}
-
-// Add Request context parameter to the data object
-// to be passed down to the templates
-function setReqCtx(req, data) {
-    (Array.isArray(data) ? data : [data]).forEach(function forEach(d) {
-        d.secure = req.secure;
-    });
-}
-
-/**
- * Returns the paths object of the active theme via way of a promise.
- * @return {Promise} The promise resolves with the value of the paths.
- */
-function getActiveThemePaths() {
-    return api.settings.read({
-        key: 'activeTheme',
-        context: {
-            internal: true
-        }
-    }).then(function then(response) {
-        var activeTheme = response.settings[0],
-            paths = config.paths.availableThemes[activeTheme.value];
-
-        return paths;
-    });
-}
-
 /*
 * Sets the response context around a post and renders it
 * with the current theme's post view. Used by post preview
@@ -106,7 +35,7 @@ function renderPost(req, res) {
     return function renderPost(post) {
         return getActiveThemePaths().then(function then(paths) {
             var view = template.getThemeViewForPost(paths, post),
-                response = formatResponse(post);
+                response = formatResponse.single(post);
 
             setResponseContext(req, res, response);
             res.render(view, response);
@@ -149,17 +78,17 @@ function renderChannel(channelOpts) {
             return res.redirect(createUrl());
         }
 
-        return getPostPage(options).then(function then(page) {
+        return fetchData(options).then(function then(page) {
             // If page is greater than number of pages we have, redirect to last page
             if (pageParam > page.meta.pagination.pages) {
                 return res.redirect(createUrl(page.meta.pagination.pages));
             }
 
-            setReqCtx(req, page.posts);
+            setRequestIsSecure(req, page.posts);
             if (channelOpts.filter && page.meta.filters[channelOpts.filter]) {
                 filterKey = page.meta.filters[channelOpts.filter];
                 filter = (_.isArray(filterKey)) ? filterKey[0] : filterKey;
-                setReqCtx(req, filter);
+                setRequestIsSecure(req, filter);
             }
 
             filters.doFilter('prePostsRender', page.posts, res.locals).then(function then(posts) {
@@ -183,9 +112,9 @@ function renderChannel(channelOpts) {
                             return next();
                         }
 
-                        result = formatPageResponse(posts, page, extra);
+                        result = formatResponse.channel(posts, page, extra);
                     } else {
-                        result = formatPageResponse(posts, page);
+                        result = formatResponse.channel(posts, page);
                     }
 
                     setResponseContext(req, res);
@@ -232,7 +161,7 @@ frontendControllers = {
                 return res.redirect(301, config.urlFor('post', {post: post}));
             }
 
-            setReqCtx(req, post);
+            setRequestIsSecure(req, post);
 
             filters.doFilter('prePostsRender', post, res.locals)
                 .then(renderPost(req, res));
@@ -301,7 +230,7 @@ frontendControllers = {
                     return Promise.reject(new errors.NotFoundError());
                 }
 
-                setReqCtx(req, post);
+                setRequestIsSecure(req, post);
 
                 filters.doFilter('prePostsRender', post, res.locals)
                     .then(renderPost(req, res));

core/server/controllers/frontend/secure.js

@@ -0,0 +1,10 @@
+// TODO: figure out how to remove the need for this
+// Add Request context parameter to the data object
+// to be passed down to the templates
+function setRequestIsSecure(req, data) {
+    (Array.isArray(data) ? data : [data]).forEach(function forEach(d) {
+        d.secure = req.secure;
+    });
+}
+
+module.exports = setRequestIsSecure;

core/server/controllers/frontend/theme-paths.js

@@ -0,0 +1,22 @@
+var api         = require('../../api'),
+    config      = require('../../config');
+
+/**
+ * Returns the paths object of the active theme via way of a promise.
+ * @return {Promise} The promise resolves with the value of the paths.
+ */
+function getActiveThemePaths() {
+    return api.settings.read({
+        key: 'activeTheme',
+        context: {
+            internal: true
+        }
+    }).then(function then(response) {
+        var activeTheme = response.settings[0],
+            paths = config.paths.availableThemes[activeTheme.value];
+
+        return paths;
+    });
+}
+
+module.exports = getActiveThemePaths;

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

@@ -245,4 +245,17 @@ describe('Contexts', function () {
             res.locals.context[0].should.eql('page');
         });
     });
+
+    describe('Private', function () {
+        it('should correctly identify `/private/` as the private route', function () {
+            // Setup test by setting relativeUrl
+            res.locals.relativeUrl = '/private/?r=';
+
+            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('private');
+        });
+    });
 });

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

@@ -0,0 +1,43 @@
+/*globals describe, beforeEach, afterEach, it*/
+/*jshint expr:true*/
+var should   = require('should'),
+    sinon    = require('sinon'),
+    errors   = require('../../../../server/errors'),
+
+// Stuff we are testing
+    handleError = require('../../../../server/controllers/frontend/error'),
+
+    sandbox = sinon.sandbox.create();
+
+// To stop jshint complaining
+should.equal(true, true);
+
+describe('handleError', function () {
+    var next;
+    beforeEach(function () {
+        next = sandbox.spy();
+    });
+
+    afterEach(function () {
+        sandbox.restore();
+    });
+
+    it('should call next with no args for 404 errors', function () {
+        var notFoundError = new errors.NotFoundError('Something wasn\'t found');
+        handleError(next)(notFoundError);
+
+        next.calledOnce.should.be.true;
+        next.firstCall.args.should.be.empty;
+    });
+
+    it('should call next with error for other errors', function () {
+        var otherError = new errors.MethodNotAllowedError('Something wasn\'t allowed');
+
+        handleError(next)(otherError);
+
+        next.calledOnce.should.be.true;
+        next.firstCall.args.should.have.lengthOf(1);
+        next.firstCall.args[0].should.be.an.Object;
+        next.firstCall.args[0].should.be.instanceof(Error);
+    });
+});

core/test/unit/controllers/frontend/fetch-data_spec.js

@@ -0,0 +1,79 @@
+/*globals describe, beforeEach, afterEach, it*/
+/*jshint expr:true*/
+var should   = require('should'),
+    sinon    = require('sinon'),
+    Promise  = require('bluebird'),
+
+    // Stuff we are testing
+    api      = require('../../../../server/api'),
+    fetchData = require('../../../../server/controllers/frontend/fetch-data'),
+
+    sandbox = sinon.sandbox.create();
+
+describe('fetchData', function () {
+    var apiSettingsStub,
+        apiPostsStub;
+
+    beforeEach(function () {
+        apiPostsStub = sandbox.stub(api.posts, 'browse').returns(new Promise.resolve({}));
+        apiSettingsStub = sandbox.stub(api.settings, 'read');
+    });
+
+    afterEach(function () {
+        sandbox.restore();
+    });
+
+    describe('valid postsPerPage', function () {
+        beforeEach(function () {
+            apiSettingsStub.withArgs('postsPerPage').returns(Promise.resolve({
+                settings: [{
+                    key: 'postsPerPage',
+                    value: '10'
+                }]
+            }));
+        });
+
+        it('Adds limit & includes to options by default', function (done) {
+            fetchData({}).then(function () {
+                apiSettingsStub.calledOnce.should.be.true;
+                apiPostsStub.calledOnce.should.be.true;
+                apiPostsStub.firstCall.args[0].should.be.an.Object;
+                apiPostsStub.firstCall.args[0].should.have.property('include');
+                apiPostsStub.firstCall.args[0].should.have.property('limit', 10);
+                done();
+            }).catch(done);
+        });
+
+        it('Throws error if no options are passed', function (done) {
+            fetchData().then(function () {
+                done('Should have thrown an error here');
+            }).catch(function (err) {
+                should.exist(err);
+                done();
+            });
+        });
+    });
+
+    describe('invalid postsPerPage', function () {
+        beforeEach(function () {
+            apiSettingsStub.withArgs('postsPerPage').returns(Promise.resolve({
+                settings: [{
+                    key: 'postsPerPage',
+                    value: '-1'
+                }]
+            }));
+        });
+
+        it('Will not add limit if postsPerPage is not valid', function (done) {
+            fetchData({}).then(function () {
+                apiSettingsStub.calledOnce.should.be.true;
+                apiPostsStub.calledOnce.should.be.true;
+                apiPostsStub.firstCall.args[0].should.be.an.Object;
+                apiPostsStub.firstCall.args[0].should.have.property('include');
+                apiPostsStub.firstCall.args[0].should.not.have.property('limit');
+
+                done();
+            }).catch(done);
+        });
+    });
+});