From 91ca4a43e57e60453850a22997cd2f0371a35e7c Mon Sep 17 00:00:00 2001 From: Harry Wolff Date: Mon, 30 Dec 2013 02:03:29 -0500 Subject: [PATCH] Fix routing of posts and static pages closes #1757 and #1773 - switches routes.frontend for posts and pages to use a regex with two capturing groups. This removes the need to dynamically remove an express route at a later point, leaving the decision making to frontend controller. - added unit tests for all routing conditions that can arise for posts and pages. - updated functional tests to also test for same thing in unit tests - removes old code from server/api/index that used to fix this issue, but is no longer needed - removed some un-needed require statements in routes/admin --- core/server/api/index.js | 37 +--- core/server/controllers/frontend.js | 55 ++++-- core/server/routes/admin.js | 4 +- core/server/routes/frontend.js | 24 +-- core/test/functional/base.js | 14 +- core/test/functional/frontend/feed_test.js | 14 +- core/test/functional/frontend/post_test.js | 14 ++ core/test/unit/frontend_spec.js | 187 ++++++++++++++++----- 8 files changed, 232 insertions(+), 117 deletions(-) diff --git a/core/server/api/index.js b/core/server/api/index.js index df261516db..4defee7f55 100644 --- a/core/server/api/index.js +++ b/core/server/api/index.js @@ -3,11 +3,11 @@ var _ = require('underscore'), when = require('when'), + config = require('../config'), errors = require('../errorHandling'), db = require('./db'), settings = require('./settings'), notifications = require('./notifications'), - config = require('../config'), posts = require('./posts'), users = require('./users'), tags = require('./tags'), @@ -49,34 +49,15 @@ requestHandler = function (apiMethod) { var options = _.extend(req.body, req.query, req.params), apiContext = { user: req.session && req.session.user - }, - postRouteIndex, - i; + }; - settings.read('permalinks').then(function (permalinks) { - // If permalinks have changed, find old post route - if (req.body.permalinks && req.body.permalinks !== permalinks) { - for (i = 0; i < req.app.routes.get.length; i += 1) { - if (req.app.routes.get[i].path === config.paths().subdir + permalinks) { - postRouteIndex = i; - break; - } - } - } - - return apiMethod.call(apiContext, options).then(function (result) { - // Reload post route - if (postRouteIndex) { - req.app.get(permalinks, req.app.routes.get.splice(postRouteIndex, 1)[0].callbacks); - } - - invalidateCache(req, res, result); - res.json(result || {}); - }, function (error) { - var errorCode = error.errorCode || 500, - errorMsg = {error: _.isString(error) ? error : (_.isObject(error) ? error.message : 'Unknown API Error')}; - res.json(errorCode, errorMsg); - }); + return apiMethod.call(apiContext, options).then(function (result) { + invalidateCache(req, res, result); + res.json(result || {}); + }, function (error) { + var errorCode = error.errorCode || 500, + errorMsg = {error: _.isString(error) ? error : (_.isObject(error) ? error.message : 'Unknown API Error')}; + res.json(errorCode, errorMsg); }); }; }; diff --git a/core/server/controllers/frontend.js b/core/server/controllers/frontend.js index 22c41cb449..c11c444b28 100644 --- a/core/server/controllers/frontend.js +++ b/core/server/controllers/frontend.js @@ -69,36 +69,55 @@ frontendControllers = { }); }, 'single': function (req, res, next) { - api.posts.read(_.pick(req.params, ['id', 'slug', 'page'])).then(function (post) { - if (post) { + // From route check if a date was parsed + // from the regex + var dateInSlug = req.params[0] !== ''; + when.join( + api.settings.read('permalinks'), + api.posts.read({slug: req.params[1]}) + ).then(function (promises) { + var permalink = promises[0].value, + post = promises[1]; + + function render() { filters.doFilter('prePostsRender', post).then(function (post) { api.settings.read('activeTheme').then(function (activeTheme) { - var paths = config.paths().availableThemes[activeTheme.value]; - if (post.page && paths.hasOwnProperty('page')) { - res.render('page', {post: post}); - } else { - res.render('post', {post: post}); - } + var paths = config.paths().availableThemes[activeTheme.value], + view = post.page && paths.hasOwnProperty('page') ? 'page' : 'post'; + res.render(view, {post: post}); }); }); - } else { - next(); } + if (!post) { + return next(); + } + + // A page can only be rendered when there is no date in the url. + // A post can either be rendered with a date in the url + // depending on the permalink setting. + // For all other conditions return 404. + if (post.page === 1 && dateInSlug === false) { + return render(); + } + + if (post.page === 0) { + // Handle post rendering + if ((permalink === '/:slug/' && dateInSlug === false) || + (permalink !== '/:slug/' && dateInSlug === true)) { + return render(); + } + } + + next(); + + }).otherwise(function (err) { var e = new Error(err.message); e.status = err.errorCode; return next(e); }); }, - 'post': function (req, res, next) { - req.params.page = 0; - return frontendControllers.single(req, res, next); - }, - 'page': function (req, res, next) { - req.params.page = 1; - return frontendControllers.single(req, res, next); - }, 'rss': function (req, res, next) { // Initialize RSS var siteUrl = config().url, diff --git a/core/server/routes/admin.js b/core/server/routes/admin.js index 58f6a6232a..d6c0146a0d 100644 --- a/core/server/routes/admin.js +++ b/core/server/routes/admin.js @@ -1,8 +1,6 @@ var admin = require('../controllers/admin'), - api = require('../api'), config = require('../config'), - middleware = require('../middleware').middleware, - url = require('url'); + middleware = require('../middleware').middleware; module.exports = function (server) { var subdir = config.paths().subdir; diff --git a/core/server/routes/frontend.js b/core/server/routes/frontend.js index 7274fa38bc..2ef505cd33 100644 --- a/core/server/routes/frontend.js +++ b/core/server/routes/frontend.js @@ -1,19 +1,19 @@ -var frontend = require('../controllers/frontend'), - api = require('../api'); +var frontend = require('../controllers/frontend'); + module.exports = function (server) { + /*jslint regexp: true */ + // ### Frontend routes /* TODO: dynamic routing, homepage generator, filters ETC ETC */ server.get('/rss/', frontend.rss); server.get('/rss/:page/', frontend.rss); server.get('/page/:page/', frontend.homepage); + // Only capture the :slug part of the URL + // This regex will always have two capturing groups, + // one for date, and one for the slug. + // Examples: + // Given `/plain-slug/` the req.params would be ['', 'plain-slug'] + // Given `/2012/12/24/plain-slug/` the req.params would be ['2012/12/24', 'plain-slug'] + server.get(/^\/([0-9\/]*)([^\/.]*)\/$/, frontend.single); server.get('/', frontend.homepage); - - api.settings.read('permalinks').then(function (permalinks) { - if (permalinks.value !== '/:slug/') { - server.get('/:slug/', frontend.page); - server.get(permalinks.value, frontend.post); - } else { - server.get(permalinks.value, frontend.single); - } - }); -}; +}; \ No newline at end of file diff --git a/core/test/functional/base.js b/core/test/functional/base.js index 657eb994f6..336d9231e5 100644 --- a/core/test/functional/base.js +++ b/core/test/functional/base.js @@ -209,6 +209,17 @@ CasperTest.Routines = (function () { }, id); } + function togglePermalinks(test) { + casper.thenOpen(url + "ghost/settings/"); + casper.thenClick('#permalinks'); + casper.thenClick('.button-save'); + casper.waitFor(function successNotification() { + return this.evaluate(function () { + return document.querySelectorAll('.js-bb-notification section').length > 0; + }); + }); + } + function _createRunner(fn) { fn.run = function run(test) { var routine = this; @@ -225,7 +236,8 @@ CasperTest.Routines = (function () { register: _createRunner(register), login: _createRunner(login), logout: _createRunner(logout), - deletePost: _createRunner(deletePost) + deletePost: _createRunner(deletePost), + togglePermalinks: _createRunner(togglePermalinks) }; }()); \ No newline at end of file diff --git a/core/test/functional/frontend/feed_test.js b/core/test/functional/frontend/feed_test.js index cb0256cf6a..ecd5f7cc15 100644 --- a/core/test/functional/frontend/feed_test.js +++ b/core/test/functional/frontend/feed_test.js @@ -28,18 +28,8 @@ CasperTest.begin('Ensure that RSS is available', 11, function suite(test) { }); }); -CasperTest.begin('Ensures dated permalinks works with RSS', 4, function suite(test) { - casper.thenOpen(url + "ghost/settings/", function testTitleAndUrl() { - test.assertTitle("Ghost Admin", "Ghost admin has no title"); - test.assertUrlMatch(/ghost\/settings\/general\/$/, "Ghost doesn't require login this time"); - }); - casper.thenClick('#permalinks'); - casper.thenClick('.button-save'); - casper.waitFor(function successNotification() { - return this.evaluate(function () { - return document.querySelectorAll('.js-bb-notification section').length > 0; - }); - }); +CasperTest.begin('Ensures dated permalinks works with RSS', 2, function suite(test) { + CasperTest.Routines.togglePermalinks.run(test); casper.thenOpen(url + 'rss/', function (response) { var content = this.getPageContent(), today = new Date(), diff --git a/core/test/functional/frontend/post_test.js b/core/test/functional/frontend/post_test.js index a0dfdab94b..8957c4b006 100644 --- a/core/test/functional/frontend/post_test.js +++ b/core/test/functional/frontend/post_test.js @@ -3,7 +3,21 @@ */ /*globals CasperTest, casper, __utils__, url, testPost, falseUser, email */ + +// Tests when permalinks is set to date (changed in feed_test) +CasperTest.begin('Post page does not load as slug', 2, function suite(test) { + casper.start(url + 'welcome-to-ghost', function then(response) { + test.assertTitle('404 — Page Not Found', 'The post should return 404 page'); + test.assertElementCount('.content .post', 0, 'There is no post on this page'); + }); +}, true); + +CasperTest.begin('Toggle permalinks', 0, function suite(test) { + CasperTest.Routines.togglePermalinks.run(test); +}); + CasperTest.begin('Post page loads', 3, function suite(test) { + CasperTest.Routines.togglePermalinks.run(test); casper.start(url + 'welcome-to-ghost', function then(response) { test.assertTitle('Welcome to Ghost', 'The post should have a title and it should be "Welcome to Ghost"'); test.assertElementCount('.content .post', 1, 'There is exactly one post on this page'); diff --git a/core/test/unit/frontend_spec.js b/core/test/unit/frontend_spec.js index 0b8f527e41..1963a4280f 100644 --- a/core/test/unit/frontend_spec.js +++ b/core/test/unit/frontend_spec.js @@ -13,7 +13,7 @@ describe('Frontend Controller', function () { var ghost, sandbox, - apiStub; + apiSettingsStub; beforeEach(function () { sandbox = sinon.sandbox.create(); @@ -48,16 +48,16 @@ describe('Frontend Controller', function () { }; beforeEach(function () { - apiStub = sandbox.stub(api.posts , 'read', function (args) { - return when(args.id === 1 ? mockStaticPost : mockPost); + sandbox.stub(api.posts , 'read', function (args) { + return when(args.slug === mockStaticPost.slug ? mockStaticPost : mockPost); }); - sandbox.stub(api.settings , 'read', function () { - return when({ - 'key': 'activeTheme', - 'value': 'casper' - }); - }); + apiSettingsStub = sandbox.stub(api.settings , 'read'); + + apiSettingsStub.withArgs('activeTheme').returns(when({ + 'key': 'activeTheme', + 'value': 'casper' + })); sandbox.stub(config , 'paths', function () { return { @@ -74,47 +74,148 @@ describe('Frontend Controller', function () { }); }); - it('can render a static page', function(done) { - var req = { - params: { - 'id': 1, - 'slug': 'test-static-page' - } - }; + describe('permalink set to slug', function() { + beforeEach(function() { + apiSettingsStub.withArgs('permalinks').returns(when({ + value: '/:slug/' + })); + }); - var res = { - render: function(view, context) { - assert.equal(view, 'page'); - assert(context.post, 'Context object has post attribute'); - assert.equal(context.post, mockStaticPost); - done(); - } - }; + it('can render a static page', function(done) { + var req = { + params: ['', 'test-static-page'] + }; - frontend.single(req, res, null); + var res = { + render: function(view, context) { + assert.equal(view, 'page'); + assert.equal(context.post, mockStaticPost); + done(); + } + }; - }); + frontend.single(req, res, null); + }); - it('can render a normal post', function(done) { - var req = { - params: { - 'id': 2, - 'slug': 'test-normal-post' - } - }; + it('won\'t render a static page accessed as a date url', function(done) { + var req = { + params: ['2012/12/30', 'test-static-page'] + }; - var res = { - render: function(view, context) { - assert.equal(view, 'post'); - assert(context.post, 'Context object has post attribute'); - assert.equal(context.post, mockPost); - done(); - } - }; + var res = { + render: sinon.spy() + }; - frontend.single(req, res, null); + frontend.single(req, res, function() { + res.render.called.should.be.false; + done(); + }); + }); + + it('can render a normal post', function(done) { + var req = { + params: ['', 'test-normal-post'] + }; + + var res = { + render: function(view, context) { + assert.equal(view, 'post'); + assert(context.post, 'Context object has post attribute'); + assert.equal(context.post, mockPost); + done(); + } + }; + + frontend.single(req, res, null); + }); + + it('won\'t render a normal post accessed as a date url', function(done) { + var req = { + params: ['2012/12/30', 'test-normal-post'] + }; + + var res = { + render: sinon.spy() + }; + + frontend.single(req, res, function() { + res.render.called.should.be.false; + done(); + }); + }); + }); + + describe('permalink set to date', function() { + beforeEach(function() { + apiSettingsStub.withArgs('permalinks').returns(when({ + value: '/:year/:month/:day/:slug/' + })); + }); + + it('can render a static page', function(done) { + var req = { + params: ['', 'test-static-page'] + }; + + var res = { + render: function(view, context) { + assert.equal(view, 'page'); + assert.equal(context.post, mockStaticPost); + done(); + } + }; + + frontend.single(req, res, null); + }); + + it('won\'t render a static page accessed as a date url', function(done) { + var req = { + params: ['2012/12/30', 'test-static-page'] + }; + + var res = { + render: sinon.spy() + }; + + frontend.single(req, res, function() { + res.render.called.should.be.false; + done(); + }); + }); + + it('can render a normal post', function(done) { + var req = { + params: ['2012/12/30', 'test-normal-post'] + }; + + var res = { + render: function(view, context) { + assert.equal(view, 'post'); + assert(context.post, 'Context object has post attribute'); + assert.equal(context.post, mockPost); + done(); + } + }; + + frontend.single(req, res, null); + }); + + it('won\'t render a normal post accessed as a slug url', function(done) { + var req = { + params: ['', 'test-normal-post'] + }; + + var res = { + render: sinon.spy() + }; + + frontend.single(req, res, function() { + res.render.called.should.be.false; + done(); + }); + }); + }); - }); }); }); \ No newline at end of file