Split renderChannel into controller + renderer (#9218)

refs #5091, refs #9192

- render channel was always a weird file
- now it's clearly 2 things
- we're slowly getting towards closing #5091... 🎉
- added some extra tests

core/server/controllers/channel.js

@@ -0,0 +1,46 @@
+var _ = require('lodash'),
+    errors = require('../errors'),
+    i18n = require('../i18n'),
+    filters = require('../filters'),
+    safeString = require('../utils/index').safeString,
+    handleError = require('./frontend/error'),
+    fetchData = require('./frontend/fetch-data'),
+    setRequestIsSecure = require('./frontend/secure'),
+    renderChannel = require('./frontend/render-channel');
+
+module.exports = function channelController(req, res, next) {
+    // Parse the parameters we need from the URL
+    var channelOpts = res.locals.channel,
+        pageParam = req.params.page !== undefined ? req.params.page : 1,
+        slugParam = req.params.slug ? safeString(req.params.slug) : undefined;
+
+    // Ensure we at least have an empty object for postOptions
+    channelOpts.postOptions = channelOpts.postOptions || {};
+    // Set page on postOptions for the query made later
+    channelOpts.postOptions.page = pageParam;
+    channelOpts.slugParam = slugParam;
+
+    // Call fetchData to get everything we need from the API
+    return fetchData(channelOpts).then(function handleResult(result) {
+        // If page is greater than number of pages we have, go straight to 404
+        if (pageParam > result.meta.pagination.pages) {
+            return next(new errors.NotFoundError({message: i18n.t('errors.errors.pageNotFound')}));
+        }
+
+        // Format data 1
+        // @TODO: figure out if this can be removed, it's supposed to ensure that absolutely URLs get generated
+        // correctly for the various objects, but I believe it doesn't work and a different approach is needed.
+        setRequestIsSecure(req, result.posts);
+        _.each(result.data, function (data) {
+            setRequestIsSecure(req, data);
+        });
+
+        // @TODO: properly design these filters
+        filters.doFilter('prePostsRender', result.posts, res.locals)
+            .then(function (posts) {
+                result.posts = posts;
+                return result;
+            })
+            .then(renderChannel(req, res));
+    }).catch(handleError(next));
+};

core/server/controllers/channels/router.js

@@ -6,7 +6,7 @@ var express = require('express'),
     rss     = require('../../data/xml/rss'),
     utils   = require('../../utils'),
     channelLoader = require('./loader'),
-    renderChannel = require('../frontend/render-channel'),
+    channelController = require('../channel'),
     rssRouter,
     channelRouter,
     channelsRouter;
@@ -70,11 +70,11 @@ channelRouter = function channelRouter(channel) {
         pageRoute = utils.url.urlJoin('/', config.get('routeKeywords').page, ':page(\\d+)/'),
         middleware = [channelConfigMiddleware(channel)];
 
-    channelRouter.get(baseRoute, middleware, renderChannel);
+    channelRouter.get(baseRoute, middleware, channelController);
 
     if (channel.isPaged) {
         channelRouter.param('page', handlePageParam);
-        channelRouter.get(pageRoute, middleware, renderChannel);
+        channelRouter.get(pageRoute, middleware, channelController);
     }
 
     if (channel.hasRSS) {

core/server/controllers/frontend/render-channel.js

@@ -1,56 +1,17 @@
 var debug = require('ghost-ignition').debug('channels:render'),
-    _ = require('lodash'),
-    errors = require('../../errors'),
-    i18n = require('../../i18n'),
-    filters = require('../../filters'),
-    safeString = require('../../utils/index').safeString,
-    handleError = require('./error'),
-    fetchData = require('./fetch-data'),
     formatResponse = require('./format-response'),
     setResponseContext = require('./context'),
-    setRequestIsSecure = require('./secure'),
     templates = require('./templates');
 
-function renderChannel(req, res, next) {
+module.exports = function renderChannel(req, res) {
     debug('renderChannel called');
-    // Parse the parameters we need from the URL
-    var channelOpts = res.locals.channel,
-        pageParam = req.params.page !== undefined ? req.params.page : 1,
-        slugParam = req.params.slug ? safeString(req.params.slug) : undefined;
+    return function renderChannel(result) {
+        var view = templates.channel(res.locals.channel);
 
-    // Ensure we at least have an empty object for postOptions
-    channelOpts.postOptions = channelOpts.postOptions || {};
-    // Set page on postOptions for the query made later
-    channelOpts.postOptions.page = pageParam;
-    channelOpts.slugParam = slugParam;
+        result = formatResponse.channel(result);
 
-    // Call fetchData to get everything we need from the API
-    return fetchData(channelOpts).then(function handleResult(result) {
-        // If page is greater than number of pages we have, go straight to 404
-        if (pageParam > result.meta.pagination.pages) {
-            return next(new errors.NotFoundError({message: i18n.t('errors.errors.pageNotFound')}));
-        }
-
-        // @TODO: figure out if this can be removed, it's supposed to ensure that absolutely URLs get generated
-        // correctly for the various objects, but I believe it doesn't work and a different approach is needed.
-        setRequestIsSecure(req, result.posts);
-        _.each(result.data, function (data) {
-            setRequestIsSecure(req, data);
-        });
-
-        // @TODO: properly design these filters
-        filters.doFilter('prePostsRender', result.posts, res.locals).then(function then(posts) {
-            var view = templates.channel(channelOpts);
-
-            // Do final data formatting and then render
-            result.posts = posts;
-            result = formatResponse.channel(result);
-
-            setResponseContext(req, res);
-            debug('Rendering view: ' + view);
-            res.render(view, result);
-        });
-    }).catch(handleError(next));
-}
-
-module.exports = renderChannel;
+        setResponseContext(req, res);
+        debug('Rendering view: ' + view);
+        res.render(view, result);
+    };
+};

core/server/controllers/preview.js

@@ -5,7 +5,7 @@ var api = require('../api'),
     renderPost = require('./frontend/render-post'),
     setRequestIsSecure = require('./frontend/secure');
 
-module.exports = function preview(req, res, next) {
+module.exports = function previewController(req, res, next) {
     var params = {
         uuid: req.params.uuid,
         status: 'all',

core/server/controllers/single.js

@@ -5,7 +5,7 @@ var utils = require('../utils'),
     renderPost = require('./frontend/render-post'),
     setRequestIsSecure = require('./frontend/secure');
 
-module.exports = function single(req, res, next) {
+module.exports = function singleController(req, res, next) {
     // Query database to find post
     return postLookup(req.path).then(function then(lookup) {
         var post = lookup ? lookup.post : false;

core/test/unit/controllers/channels/custom_channels_spec.js

@@ -11,10 +11,11 @@ var should = require('should'),  // jshint ignore:line
     sandbox = sinon.sandbox.create();
 
 /**
- * These tests are a bit weird,
- * need to test express private API
- * Would be better to be testing our own internal API
- * E.g. setupRSS.calledOnce, rather than router stack!
+ * These tests are a bit weird, because we are testing the express private API
+ * Would be better to be testing our own internal API E.g. setupRSS.calledOnce, rather than router stack!
+ *
+ * This is partly because router_spec.js is testing a full stack of behaviour, including the loader
+ * And we need to differentiate more between testing the default channels, and channels in general
  */
 describe('Custom Channels', function () {
     var channelLoaderStub;
@@ -52,7 +53,7 @@ describe('Custom Channels', function () {
         routeStack[0].should.be.a.DispatchLayer({
             route: {
                 path: '/',
-                stack: ['doChannelConfig', 'renderChannel']
+                stack: ['doChannelConfig', 'channelController']
             }
         });
 
@@ -61,7 +62,7 @@ describe('Custom Channels', function () {
             keys: ['page'],
             route: {
                 path: '/page/:page(\\d+)/',
-                stack: ['doChannelConfig', 'renderChannel']
+                stack: ['doChannelConfig', 'channelController']
             }
         });
 
@@ -114,7 +115,7 @@ describe('Custom Channels', function () {
         routeStack[0].should.be.a.DispatchLayer({
             route: {
                 path: '/',
-                stack: ['doChannelConfig', 'renderChannel']
+                stack: ['doChannelConfig', 'channelController']
             }
         });
 
@@ -123,7 +124,7 @@ describe('Custom Channels', function () {
             keys: ['page'],
             route: {
                 path: '/page/:page(\\d+)/',
-                stack: ['doChannelConfig', 'renderChannel']
+                stack: ['doChannelConfig', 'channelController']
             }
         });
 
@@ -159,7 +160,7 @@ describe('Custom Channels', function () {
         routeStack[0].should.be.a.DispatchLayer({
             route: {
                 path: '/',
-                stack: ['doChannelConfig', 'renderChannel']
+                stack: ['doChannelConfig', 'channelController']
             }
         });
 
@@ -168,7 +169,7 @@ describe('Custom Channels', function () {
             keys: ['page'],
             route: {
                 path: '/page/:page(\\d+)/',
-                stack: ['doChannelConfig', 'renderChannel']
+                stack: ['doChannelConfig', 'channelController']
             }
         });
 
@@ -200,7 +201,7 @@ describe('Custom Channels', function () {
         routeStack[0].should.be.a.DispatchLayer({
             route: {
                 path: '/',
-                stack: ['doChannelConfig', 'renderChannel']
+                stack: ['doChannelConfig', 'channelController']
             }
         });
 

core/test/unit/controllers/channels/loader_spec.js

@@ -2,30 +2,44 @@ var should = require('should'),  // jshint ignore:line
     _ = require('lodash'),
     rewire = require('rewire'),
     channelUtils = require('../../../utils/channelUtils'),
-    channelLoader = rewire('../../../../server/controllers/channels/loader'),
-    Channel = channelUtils.Channel;
+    channelLoader = rewire('../../../../server/controllers/channels/loader');
 
-describe('Channel Config', function () {
-    var channelReset;
+describe('Channels', function () {
+    describe('Loader', function () {
+        // This is a bit of a weird test. because the loader functionality is TEMPORARY
+        // If you have a local config, that gets loaded instead of the default.
+        // This just tests that either way, we get a JSON object.
+        it('should load a JSON object', function () {
+            var loadConfig = channelLoader.__get__('loadConfig'),
+                result = loadConfig();
 
-    before(function () {
-        channelReset = channelLoader.__set__('loadConfig', function () {
-            return channelUtils.getDefaultChannels();
+            result.should.be.an.Object();
         });
     });
 
-    after(function () {
-        channelReset();
-    });
+    describe('Default config', function () {
+        var channelReset;
 
-    it('should build a list of channels', function () {
-        var channels = channelLoader.list();
-        channels.should.be.an.Array().with.lengthOf(3);
+        before(function () {
+            // Change the channels we get returned
+            channelReset = channelLoader.__set__('loadConfig', function () {
+                return channelUtils.getDefaultChannels();
+            });
+        });
 
-        _.map(channels, 'name').should.eql(['index', 'tag', 'author']);
+        after(function () {
+            channelReset();
+        });
 
-        _.each(channels, function (channel) {
-            channel.should.be.a.Channel();
+        it('[default] list() should return a list of channel objects', function () {
+            var channels = channelLoader.list();
+            channels.should.be.an.Array().with.lengthOf(3);
+
+            _.map(channels, 'name').should.eql(['index', 'tag', 'author']);
+
+            _.each(channels, function (channel) {
+                channel.should.be.a.Channel();
+            });
         });
     });
 });

core/test/unit/controllers/channels/router_spec.js

@@ -9,6 +9,13 @@ var should = require('should'),
     themes = require('../../../../server/themes'),
     sandbox = sinon.sandbox.create();
 
+/**
+ * Note: this tests the following all in one go:
+ * - Channel Router controllers/channels/router
+ * - Channel Controller controllers/channel.js
+ * - Channel Renderer controllers/frontend/render-channel.js
+ * This is because the refactor is in progress!
+ */
 describe('Channels', function () {
     var channelRouter, req, res, hasTemplateStub, themeConfigStub;
 
@@ -168,6 +175,7 @@ describe('Channels', function () {
                 this.locals.context.should.containEql('index');
 
                 postAPIStub.calledOnce.should.be.true();
+                postAPIStub.calledWith({page: 1, limit: 5, include: 'author,tags'}).should.be.true();
             }, done);
         });
 
@@ -183,6 +191,7 @@ describe('Channels', function () {
                 this.locals.context.should.containEql('index');
 
                 postAPIStub.calledOnce.should.be.true();
+                postAPIStub.calledWith({page: 1, limit: 5, include: 'author,tags'}).should.be.true();
             }, done);
         });
 
@@ -197,6 +206,7 @@ describe('Channels', function () {
                     this.locals.context.should.containEql('index');
 
                     postAPIStub.calledOnce.should.be.true();
+                    postAPIStub.calledWith({page: 2, limit: 5, include: 'author,tags'}).should.be.true();
                 }, done);
             });
 
@@ -210,6 +220,7 @@ describe('Channels', function () {
                     this.locals.context.should.containEql('index');
 
                     postAPIStub.calledOnce.should.be.true();
+                    postAPIStub.calledWith({page: 2, limit: 5, include: 'author,tags'}).should.be.true();
                 }, done);
             });
 
@@ -223,6 +234,7 @@ describe('Channels', function () {
                     this.locals.context.should.containEql('index');
 
                     postAPIStub.calledOnce.should.be.true();
+                    postAPIStub.calledWith({page: 3, limit: 5, include: 'author,tags'}).should.be.true();
                 }, done);
             });
 
@@ -313,7 +325,9 @@ describe('Channels', function () {
                 this.locals.context.should.containEql('tag');
 
                 postAPIStub.calledOnce.should.be.true();
+                postAPIStub.calledWith({filter: 'tags:\'my-tag\'+tags.visibility:public', page: 1, limit: 5, include: 'author,tags'}).should.be.true();
                 tagAPIStub.calledOnce.should.be.true();
+                tagAPIStub.calledWith({slug: 'my-tag', visibility: 'public'}).should.be.true();
             }, done);
         });
 
@@ -329,7 +343,9 @@ describe('Channels', function () {
                 this.locals.context.should.containEql('tag');
 
                 postAPIStub.calledOnce.should.be.true();
+                postAPIStub.calledWith({filter: 'tags:\'my-tag\'+tags.visibility:public', page: 1, limit: 5, include: 'author,tags'}).should.be.true();
                 tagAPIStub.calledOnce.should.be.true();
+                tagAPIStub.calledWith({slug: 'my-tag', visibility: 'public'}).should.be.true();
             }, done);
         });
 
@@ -346,7 +362,9 @@ describe('Channels', function () {
                 this.locals.context.should.containEql('tag');
 
                 postAPIStub.calledOnce.should.be.true();
+                postAPIStub.calledWith({filter: 'tags:\'my-tag\'+tags.visibility:public', page: 1, limit: 5, include: 'author,tags'}).should.be.true();
                 tagAPIStub.calledOnce.should.be.true();
+                tagAPIStub.calledWith({slug: 'my-tag', visibility: 'public'}).should.be.true();
             }, done);
         });
 
@@ -371,6 +389,9 @@ describe('Channels', function () {
                     this.locals.context.should.containEql('tag');
 
                     postAPIStub.calledOnce.should.be.true();
+                    postAPIStub.calledWith({filter: 'tags:\'my-tag\'+tags.visibility:public', page: 2, limit: 5, include: 'author,tags'}).should.be.true();
+                    tagAPIStub.calledOnce.should.be.true();
+                    tagAPIStub.calledWith({slug: 'my-tag', visibility: 'public'}).should.be.true();
                 }, done);
             });
 
@@ -387,6 +408,9 @@ describe('Channels', function () {
                     this.locals.context.should.containEql('tag');
 
                     postAPIStub.calledOnce.should.be.true();
+                    postAPIStub.calledWith({filter: 'tags:\'my-tag\'+tags.visibility:public', page: 2, limit: 5, include: 'author,tags'}).should.be.true();
+                    tagAPIStub.calledOnce.should.be.true();
+                    tagAPIStub.calledWith({slug: 'my-tag', visibility: 'public'}).should.be.true();
                 }, done);
             });
 
@@ -408,6 +432,9 @@ describe('Channels', function () {
                     this.locals.context.should.containEql('tag');
 
                     postAPIStub.calledOnce.should.be.true();
+                    postAPIStub.calledWith({filter: 'tags:\'my-tag\'+tags.visibility:public', page: 3, limit: 5, include: 'author,tags'}).should.be.true();
+                    tagAPIStub.calledOnce.should.be.true();
+                    tagAPIStub.calledWith({slug: 'my-tag', visibility: 'public'}).should.be.true();
                 }, done);
             });
 

core/test/unit/controllers/frontend/render-channel_spec.js

@@ -1,55 +0,0 @@
-/*jshint expr:true*/
-var should = require('should'), // jshint ignore:line
-    sinon = require('sinon'),
-    rewire = require('rewire'),
-
-    channelUtils = require('../../../utils/channelUtils'),
-
-    // stuff being tested
-    renderChannel = rewire('../../../../server/controllers/frontend/render-channel'),
-
-    sandbox = sinon.sandbox.create(),
-    originalFetchData;
-
-describe('Render Channel', function () {
-    beforeEach(function () {
-        originalFetchData = renderChannel.__get__('fetchData');
-    });
-
-    afterEach(function () {
-        sandbox.restore();
-
-        renderChannel.__set__('fetchData', originalFetchData);
-    });
-
-    describe('Tag config', function () {
-        var req = {
-                params: {}
-            },
-            res = {
-                locals: {
-                    channel: channelUtils.getTestChannel('tag')
-                }
-            },
-            promise = {
-                then: function () {
-                    return {
-                        catch: function () {
-                        }
-                    };
-                }
-            };
-
-        it('should return correct tag config', function () {
-            renderChannel.__set__('fetchData', function (channelOpts) {
-                channelOpts.name.should.eql('tag');
-                channelOpts.postOptions.filter.should.eql('tags:\'%s\'+tags.visibility:public');
-                channelOpts.data.tag.options.should.eql({slug: '%s', visibility: 'public'});
-
-                return promise;
-            });
-
-            renderChannel(req, res);
-        });
-    });
-});