diff --git a/core/server/apps/amp/index.js b/core/server/apps/amp/index.js index ffec7b1412..7673efd7c5 100644 --- a/core/server/apps/amp/index.js +++ b/core/server/apps/amp/index.js @@ -1,11 +1,20 @@ var router = require('./lib/router'), - registerHelpers = require('./lib/helpers'), - config = require('../../config'); + registerHelpers = require('./lib/helpers'), + + // Dirty requires + config = require('../../config'), + settingsCache = require('../../settings/cache'); module.exports = { activate: function activate(ghost) { registerHelpers(ghost); - ghost.routeService.registerRouter('*/' + config.get('routeKeywords').amp + '/', router); + ghost.routeService.registerRouter('*/' + config.get('routeKeywords').amp + '/', function settingsEnabledRouter(req, res, next) { + if (settingsCache.get('amp') === true) { + return router.apply(this, arguments); + } + + next(); + }); } }; diff --git a/core/server/apps/amp/lib/router.js b/core/server/apps/amp/lib/router.js index 67330ef945..1ef64d44ef 100644 --- a/core/server/apps/amp/lib/router.js +++ b/core/server/apps/amp/lib/router.js @@ -1,12 +1,10 @@ var path = require('path'), express = require('express'), - _ = require('lodash'), ampRouter = express.Router(), i18n = require('../../../i18n'), // Dirty requires errors = require('../../../errors'), - settingsCache = require('../../../settings/cache'), templates = require('../../../controllers/frontend/templates'), postLookup = require('../../../controllers/frontend/post-lookup'), setResponseContext = require('../../../controllers/frontend/context'); @@ -17,19 +15,13 @@ function controller(req, res, next) { view = templates.pickTemplate(templateName, defaultTemplate), data = req.body || {}; - if (res.error) { - data.error = res.error; + // CASE: we only support amp pages for posts that are not static pages + if (!data.post || data.post.page) { + return next(new errors.NotFoundError({message: i18n.t('errors.errors.pageNotFound')})); } setResponseContext(req, res, data); - // Context check: - // Our context must be ['post', 'amp'], otherwise we won't render the template - // This prevents AMP from being rendered for pages - if (_.intersection(res.locals.context, ['post', 'amp']).length < 2) { - return next(); - } - return res.render(view, data); } @@ -44,44 +36,16 @@ function getPostData(req, res, next) { next(); }) - .catch(function (err) { - next(err); - }); -} - -function checkIfAMPIsEnabled(req, res, next) { - var ampIsEnabled = settingsCache.get('amp'); - - if (ampIsEnabled) { - return next(); - } - - // CASE: we don't support amp pages for static pages - if (req.body.post && req.body.post.page) { - return next(new errors.NotFoundError({message: i18n.t('errors.errors.pageNotFound')})); - } - - /** - * CASE: amp is disabled, we serve 404 - * - * Alternatively we could redirect to the original post, as the user can enable/disable AMP every time. - * - * If we would call `next()`, express jumps to the frontend controller (server/controllers/frontend/index.js fn single) - * and tries to lookup the post (again) and checks whether the post url equals the requested url (post.url !== req.path). - * This check would fail if the site is setup on a subdirectory. - */ - return next(new errors.NotFoundError({message: i18n.t('errors.errors.pageNotFound')})); + .catch(next); } // AMP frontend route ampRouter.route('/') .get( getPostData, - checkIfAMPIsEnabled, controller ); module.exports = ampRouter; module.exports.controller = controller; module.exports.getPostData = getPostData; -module.exports.checkIfAMPIsEnabled = checkIfAMPIsEnabled; diff --git a/core/test/unit/apps/amp/router_spec.js b/core/test/unit/apps/amp/router_spec.js index b369560743..ef6be01884 100644 --- a/core/test/unit/apps/amp/router_spec.js +++ b/core/test/unit/apps/amp/router_spec.js @@ -24,7 +24,6 @@ describe('AMP Controller', function () { var res, req, defaultPath, - setResponseContextStub, hasTemplateStub; beforeEach(function () { @@ -46,7 +45,12 @@ describe('AMP Controller', function () { route: {path: '/'}, query: {r: ''}, params: {}, - amp: {} + amp: {}, + body: { + post: { + title: 'test' + } + } }; defaultPath = path.join(configUtils.config.get('paths').appRoot, '/core/server/apps/amp/lib/views/amp.hbs'); @@ -65,11 +69,9 @@ describe('AMP Controller', function () { }); it('should render default amp page when theme has no amp template', function (done) { - setResponseContextStub = sandbox.stub(); - ampController.__set__('setResponseContext', setResponseContextStub); - - res.render = function (view) { + res.render = function (view, data) { view.should.eql(defaultPath); + data.should.eql({post: {title: 'test'}}); done(); }; @@ -79,69 +81,52 @@ describe('AMP Controller', function () { it('should render theme amp page when theme has amp template', function (done) { hasTemplateStub.withArgs('amp').returns(true); - setResponseContextStub = sandbox.stub(); - ampController.__set__('setResponseContext', setResponseContextStub); - - res.render = function (view) { + res.render = function (view, data) { view.should.eql('amp'); + data.should.eql({post: {title: 'test'}}); done(); }; ampController.controller(req, res, failTest(done)); }); - it('should render with error when error is passed in', function (done) { - res.error = 'Test Error'; + it('throws 404 when req.body has no post', function (done) { + req.body = {}; - setResponseContextStub = sandbox.stub(); - ampController.__set__('setResponseContext', setResponseContextStub); - - res.render = function (view, context) { - view.should.eql(defaultPath); - context.should.eql({error: 'Test Error'}); + ampController.controller(req, res, function (err) { + should.exist(err); + should.exist(err.message); + should.exist(err.statusCode); + should.exist(err.errorType); + err.message.should.be.eql('Page not found'); + err.statusCode.should.be.eql(404); + err.errorType.should.be.eql('NotFoundError'); done(); + }); + }); + + it('throws 404 when req.body.post is a page', function (done) { + req.body = { + post: { + page: true + } }; - ampController.controller(req, res, failTest(done)); - }); - - it('does not render amp page when amp context is missing', function (done) { - var renderSpy; - - setResponseContextStub = sandbox.stub(); - ampController.__set__('setResponseContext', setResponseContextStub); - - res.locals.context = ['post']; - res.render = sandbox.spy(function () { + ampController.controller(req, res, function (err) { + should.exist(err); + should.exist(err.message); + should.exist(err.statusCode); + should.exist(err.errorType); + err.message.should.be.eql('Page not found'); + err.statusCode.should.be.eql(404); + err.errorType.should.be.eql('NotFoundError'); done(); }); - - renderSpy = res.render; - - ampController.controller(req, res, failTest(done)); - renderSpy.called.should.be.false(); - }); - - it('does not render amp page when context is other than amp and post', function (done) { - var renderSpy; - - setResponseContextStub = sandbox.stub(); - ampController.__set__('setResponseContext', setResponseContextStub); - - res.locals.context = ['amp', 'page']; - res.render = sandbox.spy(function () { - done(); - }); - - renderSpy = res.render; - - ampController.controller(req, res, failTest(done)); - renderSpy.called.should.be.false(); }); }); describe('AMP getPostData', function () { - var res, req, postLookupStub, next; + var res, req, postLookupStub, resetPostLookup, next; beforeEach(function () { res = { @@ -157,14 +142,17 @@ describe('AMP getPostData', function () { }; next = function () {}; + + postLookupStub = sandbox.stub(); + resetPostLookup = ampController.__set__('postLookup', postLookupStub); }); afterEach(function () { sandbox.restore(); + resetPostLookup(); }); it('should successfully get the post data from slug', function (done) { - postLookupStub = sandbox.stub(); postLookupStub.returns(new Promise.resolve({ post: { id: '1', @@ -172,8 +160,6 @@ describe('AMP getPostData', function () { } })); - ampController.__set__('postLookup', postLookupStub); - ampController.getPostData(req, res, function () { req.body.post.should.be.eql({ id: '1', @@ -185,11 +171,8 @@ describe('AMP getPostData', function () { }); it('should return error if postlookup returns NotFoundError', function (done) { - postLookupStub = sandbox.stub(); postLookupStub.returns(new Promise.reject(new errors.NotFoundError({message: 'not found'}))); - ampController.__set__('postLookup', postLookupStub); - ampController.getPostData(req, res, function (err) { should.exist(err); should.exist(err.message); @@ -202,11 +185,9 @@ describe('AMP getPostData', function () { done(); }); }); - it('should return error and if postlookup returns error', function (done) { - postLookupStub = sandbox.stub(); - postLookupStub.returns(new Promise.reject('not found')); - ampController.__set__('postLookup', postLookupStub); + it('should return error and if postlookup returns error', function (done) { + postLookupStub.returns(new Promise.reject('not found')); ampController.getPostData(req, res, function (err) { should.exist(err);