From c20a6aa7f7d38cf18b9eb4c5e565d472d4605300 Mon Sep 17 00:00:00 2001 From: Hannah Wolfe Date: Mon, 23 Oct 2017 11:42:48 +0100 Subject: [PATCH] Improve channels router code (#9166) refs #5091 - There is very little that changes here, just code readability - However I've expanded out the tests getting ready to be able to test more deeply as I refactor the routing --- core/server/controllers/frontend/channels.js | 94 +++--- .../frontend/custom_channels_spec.js | 313 +++++++++++++++--- 2 files changed, 326 insertions(+), 81 deletions(-) diff --git a/core/server/controllers/frontend/channels.js b/core/server/controllers/frontend/channels.js index d10b9f190f..dc2ec37fbb 100644 --- a/core/server/controllers/frontend/channels.js +++ b/core/server/controllers/frontend/channels.js @@ -8,7 +8,7 @@ var express = require('express'), channelConfig = require('./channel-config'), renderChannel = require('./render-channel'), rssRouter, - channelRouter; + channelsRouter; function handlePageParam(req, res, next, page) { var pageRegex = new RegExp('/' + config.get('routeKeywords').page + '/(.*)?/'), @@ -33,19 +33,28 @@ function handlePageParam(req, res, next, page) { } } -rssRouter = function rssRouter(channelConfig) { - function rssConfigMiddleware(req, res, next) { - res.locals.channel.isRSS = true; - next(); - } +function rssConfigMiddleware(req, res, next) { + res.locals.channel.isRSS = true; + next(); +} +function channelConfigMiddleware(channel) { + return function doChannelConfig(req, res, next) { + res.locals.channel = _.cloneDeep(channel); + next(); + }; +} + +rssRouter = function rssRouter(channelMiddleware) { // @TODO move this to an RSS module var router = express.Router({mergeParams: true}), - stack = [channelConfig, rssConfigMiddleware, rss], - baseRoute = '/rss/'; + baseRoute = '/rss/', + pageRoute = utils.url.urlJoin(baseRoute, ':page(\\d+)/'); - router.get(baseRoute, stack); - router.get(utils.url.urlJoin(baseRoute, ':page(\\d+)/'), stack); + // @TODO figure out how to collapse this into a single rule + router.get(baseRoute, channelMiddleware, rssConfigMiddleware, rss); + router.get(pageRoute, channelMiddleware, rssConfigMiddleware, rss); + // Extra redirect rule router.get('/feed/', function redirectToRSS(req, res) { return utils.url.redirect301(res, utils.url.urlJoin(utils.url.getSubdir(), req.baseUrl, baseRoute)); }); @@ -54,47 +63,44 @@ rssRouter = function rssRouter(channelConfig) { return router; }; -channelRouter = function router() { - function channelConfigMiddleware(channel) { - return function doChannelConfig(req, res, next) { - res.locals.channel = _.cloneDeep(channel); - next(); - }; +function buildChannelRouter(channel) { + var channelRouter = express.Router({mergeParams: true}), + baseRoute = '/', + pageRoute = utils.url.urlJoin('/', config.get('routeKeywords').page, ':page(\\d+)/'), + middleware = [channelConfigMiddleware(channel)]; + + // @TODO figure out how to collapse this into a single rule + channelRouter.get(baseRoute, middleware, renderChannel); + + // @TODO improve config and add defaults to make this simpler + if (channel.paged !== false) { + channelRouter.param('page', handlePageParam); + channelRouter.get(pageRoute, middleware, renderChannel); } - var channelsRouter = express.Router({mergeParams: true}), - baseRoute = '/', - pageRoute = utils.url.urlJoin('/', config.get('routeKeywords').page, ':page(\\d+)/'); + // @TODO improve config and add defaults to make this simpler + if (channel.rss !== false) { + channelRouter.use(rssRouter(middleware)); + } + + if (channel.editRedirect) { + channelRouter.get('/edit/', function redirect(req, res) { + utils.url.redirectToAdmin(302, res, channel.editRedirect.replace(':slug', req.params.slug)); + }); + } + + return channelRouter; +} + +channelsRouter = function router() { + var channelsRouter = express.Router({mergeParams: true}); _.each(channelConfig.list(), function (channel) { - var channelRouter = express.Router({mergeParams: true}), - configChannel = channelConfigMiddleware(channel); - - // @TODO figure out how to collapse this into a single rule - channelRouter.get(baseRoute, configChannel, renderChannel); - - // @TODO improve config and add defaults to make this simpler - if (channel.paged !== false) { - channelRouter.param('page', handlePageParam); - channelRouter.get(pageRoute, configChannel, renderChannel); - } - - // @TODO improve config and add defaults to make this simpler - if (channel.rss !== false) { - channelRouter.use(rssRouter(configChannel)); - } - - if (channel.editRedirect) { - channelRouter.get('/edit/', function redirect(req, res) { - utils.url.redirectToAdmin(302, res, channel.editRedirect.replace(':slug', req.params.slug)); - }); - } - // Mount this channel router on the parent channels router - channelsRouter.use(channel.route, channelRouter); + channelsRouter.use(channel.route, buildChannelRouter(channel)); }); return channelsRouter; }; -module.exports.router = channelRouter; +module.exports.router = channelsRouter; diff --git a/core/test/unit/controllers/frontend/custom_channels_spec.js b/core/test/unit/controllers/frontend/custom_channels_spec.js index 85f4345587..0e3a75f499 100644 --- a/core/test/unit/controllers/frontend/custom_channels_spec.js +++ b/core/test/unit/controllers/frontend/custom_channels_spec.js @@ -1,5 +1,6 @@ var should = require('should'), // jshint ignore:line sinon = require('sinon'), + _ = require('lodash'), // Stuff we are testing channelConfig = require('../../../../server/controllers/frontend/channel-config'), @@ -7,6 +8,121 @@ var should = require('should'), // jshint ignore:line sandbox = sinon.sandbox.create(); +/** + * Assertions on the express API + */ +should.Assertion.add('ExpressRouter', function (options) { + options = options || {}; + + this.params = {operator: 'to be a valid Express Router'}; + this.obj.should.be.a.Function(); + this.obj.name.should.eql('router'); + this.obj.should.have.property('mergeParams', true); + this.obj.should.have.property('strict', undefined); + this.obj.should.have.property('stack'); + + if (options.params) { + // Verify the params function! + this.obj.params.should.have.property(options.params.key); + this.obj.params[options.params.key][0].name.should.eql(options.params.value); + } + + this.obj.stack.should.be.an.Array(); + if (options.stackLength) { + this.obj.stack.should.have.lengthOf(options.stackLength); + } +}); + +should.Assertion.add('Layer', function () { + this.params = {operator: 'to be a valid Express Layer'}; + + this.obj.should.be.an.Object().with.properties(['handle', 'name', 'params', 'path', 'keys', 'regexp', 'route']); +}); + +should.Assertion.add('RouterLayer', function (options) { + options = options || {}; + + this.params = {operator: 'to be a valid Express Layer, with Router as handle'}; + + this.obj.should.be.a.Layer(); + this.obj.name.should.eql('router'); + this.obj.handle.should.be.an.ExpressRouter(options); + + if (options.regexp) { + this.obj.regexp.toString().should.match(options.regexp); + } +}); + +should.Assertion.add('DispatchLayer', function (options) { + options = options || {}; + + this.params = {operator: 'to be a valid Express Layer, with Dispatch as handle'}; + this.obj.should.be.a.Layer(); + this.obj.name.should.eql('bound dispatch'); + + if (options.regexp) { + this.obj.regexp.toString().should.match(options.regexp); + } + + if (options.keys) { + this.obj.keys.should.be.an.Array().with.lengthOf(options.keys.length); + _.map(this.obj.keys, 'name').should.eql(options.keys); + } else { + this.obj.keys.should.be.an.Array().with.lengthOf(0); + } + + this.obj.route.should.be.an.Object().with.properties(['path', 'stack', 'methods']); + if (options.route && options.route.path) { + this.obj.route.path.should.eql(options.route.path); + } + + if (options.route.stack) { + this.obj.route.stack.should.be.an.Array().with.lengthOf(options.route.stack.length); + _.map(this.obj.route.stack, 'name').should.eql(options.route.stack); + } else { + this.obj.route.stack.should.be.an.Array(); + } +}); + +should.Assertion.add('RSSRouter', function () { + this.params = {operator: 'to be a valid RSS Router'}; + + this.obj.should.be.a.RouterLayer({ + stackLength: 3, + params: { + key: 'page', + value: 'handlePageParam' + } + }); + + var routeStack = this.obj.handle.stack; + + // Layer 1 should be the handler for /rss/ + routeStack[0].should.be.a.DispatchLayer({ + route: { + path: '/rss/', + stack: ['doChannelConfig', 'rssConfigMiddleware', 'generate'] + } + }); + + // Layer 2 should be the handler for pagination + routeStack[1].should.be.a.DispatchLayer({ + keys: ['page'], + route: { + path: '/rss/:page(\\d+)/', + stack: ['doChannelConfig', 'rssConfigMiddleware', 'generate'] + } + }); + + // // Layer 3 should be a handler for the extra /feed/ url + routeStack[2].should.be.a.DispatchLayer({ + route: { + path: '/feed/', + stack: ['redirectToRSS'] + } + }); +}); + /** * These tests are a bit weird, * need to test express private API @@ -26,23 +142,108 @@ describe('Custom Channels', function () { } }); - var channelRouter = channels.router(), - topRouter = channelRouter.stack[0], - routeStack = topRouter.handle.stack, - rssRouter = routeStack[2].handle.stack; + var channelsRouter = channels.router(), + firstChannel, + routeStack; - topRouter.regexp.toString().should.match(/\\\/home\\\//); - routeStack.should.have.lengthOf(3); + channelsRouter.should.be.an.ExpressRouter({stackLength: 1}); + firstChannel = channelsRouter.stack[0]; + firstChannel.should.be.a.RouterLayer({ + stackLength: 3, + regexp: /\\\/home\\\//, + params: { + key: 'page', + value: 'handlePageParam' + } + }); - // Route 1 should be / - routeStack[0].route.path.should.eql('/'); - // Route 2 should be pagination - routeStack[1].route.path.should.eql('/page/:page(\\d+)/'); - // Route 3 should be a whole new router for RSS - rssRouter.should.have.lengthOf(3); - rssRouter[0].route.path.should.eql('/rss/'); - rssRouter[1].route.path.should.eql('/rss/:page(\\d+)/'); - rssRouter[2].route.path.should.eql('/feed/'); + // The stack underneath our first channel router + routeStack = firstChannel.handle.stack; + + // Layer 1 should be the handler for / + routeStack[0].should.be.a.DispatchLayer({ + route: { + path: '/', + stack: ['doChannelConfig', 'renderChannel'] + } + }); + + // Layer 2 should be the handler for pagination + routeStack[1].should.be.a.DispatchLayer({ + keys: ['page'], + route: { + path: '/page/:page(\\d+)/', + stack: ['doChannelConfig', 'renderChannel'] + } + }); + + // Layer 3 should be a whole new router for RSS + routeStack[2].should.be.an.RSSRouter(); + }); + + it('allow multiple channels to be defined', function () { + sandbox.stub(channelConfig, 'list').returns({ + home: { + name: 'home', + route: '/home/' + }, + featured: { + name: 'featured', + route: '/featured/', + postOptions: { + filter: 'featured:true' + } + } + }); + + var channelsRouter = channels.router(), + firstChannel, + secondChannel, + routeStack; + + channelsRouter.should.be.an.ExpressRouter({stackLength: 2}); + firstChannel = channelsRouter.stack[0]; + secondChannel = channelsRouter.stack[1]; + firstChannel.should.be.a.RouterLayer({ + stackLength: 3, + regexp: /\\\/home\\\//, + params: { + key: 'page', + value: 'handlePageParam' + } + }); + + secondChannel.should.be.a.RouterLayer({ + stackLength: 3, + regexp: /\\\/featured\\\//, + params: { + key: 'page', + value: 'handlePageParam' + } + }); + + // The stack underneath our first channel router + routeStack = secondChannel.handle.stack; + + // Layer 1 should be the handler for / + routeStack[0].should.be.a.DispatchLayer({ + route: { + path: '/', + stack: ['doChannelConfig', 'renderChannel'] + } + }); + + // Layer 2 should be the handler for pagination + routeStack[1].should.be.a.DispatchLayer({ + keys: ['page'], + route: { + path: '/page/:page(\\d+)/', + stack: ['doChannelConfig', 'renderChannel'] + } + }); + + // Layer 3 should be a whole new router for RSS + routeStack[2].should.be.an.RSSRouter(); }); it('allows rss to be disabled', function () { @@ -54,17 +255,43 @@ describe('Custom Channels', function () { } }); - var channelRouter = channels.router(), - topRouter = channelRouter.stack[0], - routeStack = topRouter.handle.stack; + var channelsRouter = channels.router(), + firstChannel, + routeStack; - topRouter.regexp.toString().should.match(/\\\/home\\\//); - routeStack.should.have.lengthOf(2); + channelsRouter.should.be.an.ExpressRouter({stackLength: 1}); + firstChannel = channelsRouter.stack[0]; + firstChannel.should.be.a.RouterLayer({ + stackLength: 2, + regexp: /\\\/home\\\//, + params: { + key: 'page', + value: 'handlePageParam' + } + }); - // Route 1 should be / - routeStack[0].route.path.should.eql('/'); - // Route 2 should be pagination - routeStack[1].route.path.should.eql('/page/:page(\\d+)/'); + // The stack underneath our first channel router + routeStack = firstChannel.handle.stack; + + // Layer 1 should be the handler for / + routeStack[0].should.be.a.DispatchLayer({ + route: { + path: '/', + stack: ['doChannelConfig', 'renderChannel'] + } + }); + + // Layer 2 should be the handler for pagination + routeStack[1].should.be.a.DispatchLayer({ + keys: ['page'], + route: { + path: '/page/:page(\\d+)/', + stack: ['doChannelConfig', 'renderChannel'] + } + }); + + // Layer 3 does not exist + should.not.exist(routeStack[2]); }); it('allows pagination to be disabled', function () { @@ -76,20 +303,32 @@ describe('Custom Channels', function () { } }); - var channelRouter = channels.router(), - topRouter = channelRouter.stack[0], - routeStack = topRouter.handle.stack, - rssRouter = routeStack[1].handle.stack; + var channelsRouter = channels.router(), + firstChannel, + routeStack; - topRouter.regexp.toString().should.match(/\\\/home\\\//); - routeStack.should.have.lengthOf(2); + channelsRouter.should.be.an.ExpressRouter({stackLength: 1}); + firstChannel = channelsRouter.stack[0]; + firstChannel.should.be.a.RouterLayer({ + stackLength: 2, + regexp: /\\\/home\\\// + }); - // Route 1 should be / - routeStack[0].route.path.should.eql('/'); - // Route 2 should be a whole new router for RSS - rssRouter.should.have.lengthOf(3); - rssRouter[0].route.path.should.eql('/rss/'); - rssRouter[1].route.path.should.eql('/rss/:page(\\d+)/'); - rssRouter[2].route.path.should.eql('/feed/'); + // The stack underneath our first channel router + routeStack = firstChannel.handle.stack; + + // Layer 1 should be the handler for / + routeStack[0].should.be.a.DispatchLayer({ + route: { + path: '/', + stack: ['doChannelConfig', 'renderChannel'] + } + }); + + // Layer 2 should be the handler for /rss/ + routeStack[1].should.be.an.RSSRouter(); + + // Layer 3 does not exist + should.not.exist(routeStack[2]); }); });