tqt
/
Ghost
mirror of https://github.com/TryGhost/Ghost.git
2
1
Fork 0
Code Issues 1 Releases Wiki Activity

Simplified AMP internal app 

refs #9192

- The AMP app is nothing more than a custom controller - this will come clear soon
- Moved enabled/disabled logic into router
- Removed error-related code, as this wasn't used
- Changed logic for static pages to be based on req.body, not context
- Improved the tests to match
This commit is contained in:
Hannah Wolfe 2017-11-08 07:37:04 +00:00
parent 86c6cec433
commit 474e9234a6
3 changed files with 59 additions and 105 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

15
core/server/apps/amp/index.js
View File

@ -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();
});
}
};

44
core/server/apps/amp/lib/router.js
View File

@ -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;

105
core/test/unit/apps/amp/router_spec.js
View File

@ -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);