Infrastructure/ghost
0
Fork 0
mirror of https://github.com/TryGhost/Ghost.git synced 2025-01-06 22:40:14 -05:00
Code Issues Activity

⚡❓ Make AMP optional (#7830)

Browse source 
closes #7769

Because Google AMP is bitching around and shows errors in Googles' webmaster tools for missing post images and blog icons, we decided to make AMP optional. It will be enabled by default, but can be disabled in general settings. Once disabled, the `amp` route doesn't work anymore.

This PR contains the back end changes for Ghost-alpha:
- Adds `amp` to settings table incl default setting `true`
- Adds `amp` value to our settings cache
- Changes the route handling of AMP app to check for the `amp` setting first.
- Adds tests to check the route handling and ghost_head output
- Includes changes to `post-lookup.js` as done by @kirrg001 in #7842
This commit is contained in:
Aileen Nowak 2017-01-17 22:40:06 +07:00 committed by Katharina Irrgang
parent 8d88f5b6a5
commit 2f3081fa9f
11 changed files with 132 additions and 61 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

1
core/server/api/settings.js
View file

@ -66,6 +66,7 @@ updateConfigCache = function () {
config.set('theme:facebook', (settingsCache.facebook && settingsCache.facebook.value) || '');
config.set('theme:timezone', (settingsCache.activeTimezone && settingsCache.activeTimezone.value) || config.get('theme').timezone);
config.set('theme:url', config.get('url') ? generalUtils.url.urlJoin(config.get('url'), '/') : '');
config.set('theme:amp', (settingsCache.amp && settingsCache.amp.value === 'true'));
_.each(labsValue, function (value, key) {
config.set('labs:' + key, value);

4
core/server/apps/amp/index.js
View file

@ -1,8 +1,6 @@
var router = require('./lib/router'),
registerHelpers = require('./lib/helpers'),
// Dirty requires
config = require('../../config');
config = require('../../config');
module.exports = {
activate: function activate(ghost) {

39
core/server/apps/amp/lib/router.js
View file

@ -2,8 +2,10 @@ var path = require('path'),
express = require('express'),
_ = require('lodash'),
ampRouter = express.Router(),
i18n = require('../../../i18n'),
// Dirty requires
config = require('../../../config'),
errors = require('../../../errors'),
templates = require('../../../controllers/frontend/templates'),
postLookup = require('../../../controllers/frontend/post-lookup'),
@ -12,7 +14,7 @@ var path = require('path'),
function controller(req, res, next) {
var defaultView = path.resolve(__dirname, 'views', 'amp.hbs'),
paths = templates.getActiveThemePaths(req.app.get('activeTheme')),
data = req.amp;
data = req.body || {};
if (res.error) {
data.error = res.error;
@ -33,32 +35,53 @@ function controller(req, res, next) {
}
function getPostData(req, res, next) {
// Create a req property where we can store our data
req.amp = {};
req.body = req.body || {};
postLookup(res.locals.relativeUrl)
.then(function (result) {
if (result && result.post) {
req.amp.post = result.post;
req.body.post = result.post;
}
next();
})
.catch(function (err) {
if (err instanceof errors.NotFoundError) {
return next(err);
}
next(err);
});
}
function checkIfAMPIsEnabled(req, res, next) {
var ampIsEnabled = config.get('theme: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 blog is setup on a subdirectory.
*/
return next(new errors.NotFoundError({message: i18n.t('errors.errors.pageNotFound')}));
}
// AMP frontend route
ampRouter.route('/')
.get(
getPostData,
checkIfAMPIsEnabled,
controller
);
module.exports = ampRouter;
module.exports.controller = controller;
module.exports.getPostData = getPostData;
module.exports.checkIfAMPIsEnabled = checkIfAMPIsEnabled;

15
core/server/apps/amp/tests/router_spec.js
View file

@ -44,7 +44,8 @@ describe('AMP Controller', function () {
configUtils.set({
theme: {
permalinks: '/:slug/'
permalinks: '/:slug/',
amp: true
}
});
});
@ -165,18 +166,16 @@ describe('AMP getPostData', function () {
postLookupStub.returns(new Promise.resolve({
post: {
id: '1',
slug: 'welcome-to-ghost',
isAmpURL: true
slug: 'welcome-to-ghost'
}
}));
ampController.__set__('postLookup', postLookupStub);
ampController.getPostData(req, res, function () {
req.amp.post.should.be.eql({
req.body.post.should.be.eql({
id: '1',
slug: 'welcome-to-ghost',
isAmpURL: true
slug: 'welcome-to-ghost'
}
);
done();
@ -197,7 +196,7 @@ describe('AMP getPostData', function () {
err.message.should.be.eql('not found');
err.statusCode.should.be.eql(404);
err.errorType.should.be.eql('NotFoundError');
req.amp.should.be.eql({});
req.body.should.be.eql({});
done();
});
});
@ -210,7 +209,7 @@ describe('AMP getPostData', function () {
ampController.getPostData(req, res, function (err) {
should.exist(err);
err.should.be.eql('not found');
req.amp.should.be.eql({});
req.body.should.be.eql({});
done();
});
});

5
core/server/controllers/frontend/index.js
View file

@ -69,6 +69,11 @@ frontendControllers = {
return next();
}
// CASE: postlookup can detect options for example /edit, unknown options get ignored and end in 404
if (lookup.isUnknownOption) {
return next();
}
// CASE: last param is of url is /edit, redirect to admin
if (lookup.isEditURL) {
return res.redirect(utils.url.urlJoin(utils.url.getSubdir(), '/ghost/editor', post.id, '/'));

21
core/server/controllers/frontend/post-lookup.js
View file

@ -16,7 +16,6 @@ function postLookup(postUrl) {
postPermalink = config.get('theme').permalinks,
pagePermalink = '/:slug/',
isEditURL = false,
isAmpURL = false,
matchFuncPost,
matchFuncPage,
postParams,
@ -38,23 +37,12 @@ function postLookup(postUrl) {
}
// If params contains options, and it is equal to 'edit', this is an edit URL
// If params contains options, and it is equal to 'amp', this is an amp URL
if (params.options && params.options.toLowerCase() === 'edit') {
isEditURL = true;
} else if (params.options && params.options.toLowerCase() === 'amp') {
isAmpURL = true;
} else if (params.options !== undefined) {
// Unknown string in URL, return empty
return Promise.resolve();
}
// Sanitize params we're going to use to lookup the post.
params = _.pick(params, 'slug', 'id');
// Add author & tag
params.include = 'author,tags';
// Query database to find post
return api.posts.read(params).then(function then(result) {
return api.posts.read(_.extend(_.pick(params, 'slug', 'id'), {include: 'author,tags'})).then(function then(result) {
var post = result.posts[0];
if (!post) {
@ -71,15 +59,10 @@ function postLookup(postUrl) {
return Promise.resolve();
}
// We don't support AMP for static pages yet
if (post.page && isAmpURL) {
return Promise.resolve();
}
return {
post: post,
isEditURL: isEditURL,
isAmpURL: isAmpURL
isUnknownOption: isEditURL ? false : params.options ? true : false
};
});
}

3
core/server/data/schema/default-settings.json
View file

@ -65,6 +65,9 @@
"notContains": "/ghost/"
}
},
"amp": {
"defaultValue" : "true"
},
"ghost_head": {
"defaultValue" : ""
},

3
core/server/helpers/ghost_head.js
View file

@ -98,7 +98,8 @@ function ghost_head(options) {
escapeExpression(metaData.canonicalUrl) + '" />');
head.push('<meta name="referrer" content="' + referrerPolicy + '" />');
if (_.includes(context, 'post') && !_.includes(context, 'amp')) {
// show amp link in post when 1. we are not on the amp page and 2. amp is enabled
if (_.includes(context, 'post') && !_.includes(context, 'amp') && config.get('theme:amp')) {
head.push('<link rel="amphtml" href="' +
escapeExpression(metaData.ampUrl) + '" />');
}

30
core/test/functional/routes/frontend_spec.js
View file

@ -8,6 +8,7 @@ var request = require('supertest'),
moment = require('moment'),
cheerio = require('cheerio'),
testUtils = require('../../utils'),
configUtils = require('../../utils/configUtils'),
ghost = testUtils.startGhost;
describe('Frontend Routing', function () {
@ -309,6 +310,19 @@ describe('Frontend Routing', function () {
.expect(/Page not found/)
.end(doEnd(done));
});
it('should not render AMP, when AMP is disabled', function (done) {
after(function () {
configUtils.restore();
});
configUtils.set('theme:amp', false);
request.get('/welcome-to-ghost/amp/')
.expect(404)
.expect(/Page not found/)
.end(doEnd(done));
});
});
describe('Static assets', function () {
@ -534,6 +548,17 @@ describe('Frontend Routing', function () {
.expect(200)
.end(doEnd(done));
});
it('/blog/welcome-to-ghost/amp/ should 200', function (done) {
after(function () {
configUtils.restore();
});
configUtils.set('theme:amp', true);
request.get('/blog/welcome-to-ghost/amp/')
.expect(200)
.end(doEnd(done));
});
});
describe('Subdirectory (with slash)', function () {
@ -612,6 +637,11 @@ describe('Frontend Routing', function () {
});
it('/blog/welcome-to-ghost/amp/ should 200', function (done) {
after(function () {
configUtils.restore();
});
configUtils.set('theme:amp', true);
request.get('/blog/welcome-to-ghost/amp/')
.expect(200)
.end(doEnd(done));

20
core/test/unit/controllers/frontend/post-lookup_spec.js
View file

@ -40,7 +40,6 @@ describe('postLookup', function () {
should.exist(lookup.post);
lookup.post.should.have.property('url', '/welcome-to-ghost/');
lookup.isEditURL.should.be.false();
lookup.isAmpURL.should.be.false();
done();
}).catch(done);
@ -54,7 +53,6 @@ describe('postLookup', function () {
should.exist(lookup.post);
lookup.post.should.have.property('url', '/welcome-to-ghost/');
lookup.isEditURL.should.be.false();
lookup.isAmpURL.should.be.false();
done();
}).catch(done);
@ -116,7 +114,6 @@ describe('postLookup', function () {
should.exist(lookup.post);
lookup.post.should.have.property('url', '/2016/01/01/welcome-to-ghost/');
lookup.isEditURL.should.be.false();
lookup.isAmpURL.should.be.false();
done();
}).catch(done);
@ -130,7 +127,6 @@ describe('postLookup', function () {
should.exist(lookup.post);
lookup.post.should.have.property('url', '/2016/01/01/welcome-to-ghost/');
lookup.isEditURL.should.be.false();
lookup.isAmpURL.should.be.false();
done();
}).catch(done);
@ -154,7 +150,6 @@ describe('postLookup', function () {
postLookup(testUrl).then(function (lookup) {
lookup.post.should.have.property('url', '/welcome-to-ghost/');
lookup.isEditURL.should.be.true();
lookup.isAmpURL.should.be.false();
done();
}).catch(done);
});
@ -165,7 +160,6 @@ describe('postLookup', function () {
postLookup(testUrl).then(function (lookup) {
lookup.post.should.have.property('url', '/welcome-to-ghost/');
lookup.isEditURL.should.be.true();
lookup.isAmpURL.should.be.false();
done();
}).catch(done);
});
@ -192,7 +186,8 @@ describe('postLookup', function () {
var testUrl = '/welcome-to-ghost/notedit/';
postLookup(testUrl).then(function (lookup) {
should.not.exist(lookup);
lookup.post.should.have.property('url', '/welcome-to-ghost/');
lookup.isUnknownOption.should.eql(true);
done();
}).catch(done);
});
@ -213,7 +208,6 @@ describe('postLookup', function () {
postLookup(testUrl).then(function (lookup) {
lookup.post.should.have.property('url', '/welcome-to-ghost/');
lookup.isAmpURL.should.be.true();
lookup.isEditURL.should.be.false();
done();
}).catch(done);
@ -224,7 +218,6 @@ describe('postLookup', function () {
postLookup(testUrl).then(function (lookup) {
lookup.post.should.have.property('url', '/welcome-to-ghost/');
lookup.isAmpURL.should.be.true();
lookup.isEditURL.should.be.false();
done();
}).catch(done);
@ -247,14 +240,5 @@ describe('postLookup', function () {
done();
}).catch(done);
});
it('cannot lookup relative url: /:slug/notamp/', function (done) {
var testUrl = '/welcome-to-ghost/notamp/';
postLookup(testUrl).then(function (lookup) {
should.not.exist(lookup);
done();
}).catch(done);
});
});
});

52
core/test/unit/server_helpers/ghost_head_spec.js
View file

@ -52,7 +52,8 @@ describe('{{ghost_head}} helper', function () {
theme: {
title: 'Ghost',
description: 'blog description',
cover: '/content/images/blog-cover.png'
cover: '/content/images/blog-cover.png',
amp: true
}
});
});
@ -866,7 +867,8 @@ describe('{{ghost_head}} helper', function () {
theme: {
title: 'Ghost',
description: 'blog description',
cover: '/content/images/blog-cover.png'
cover: '/content/images/blog-cover.png',
amp: true
}
});
});
@ -894,7 +896,8 @@ describe('{{ghost_head}} helper', function () {
theme: {
title: 'Ghost',
description: 'blog description',
cover: '/content/images/blog-cover.png'
cover: '/content/images/blog-cover.png',
amp: true
},
referrerPolicy: 'origin'
});
@ -920,7 +923,8 @@ describe('{{ghost_head}} helper', function () {
theme: {
title: 'Ghost',
description: 'blog description',
cover: '/content/images/blog-cover.png'
cover: '/content/images/blog-cover.png',
amp: true
},
privacy: {
useStructuredData: false
@ -1091,4 +1095,44 @@ describe('{{ghost_head}} helper', function () {
});
});
});
describe('amp is disabled', function () {
beforeEach(function () {
configUtils.set({
url: 'http://testurl.com/',
theme: {
amp: false
}
});
});
it('does not contain amphtml link', function (done) {
var post = {
meta_description: 'blog description',
title: 'Welcome to Ghost',
image: 'content/images/test-image.png',
published_at: moment('2008-05-31T19:18:15').toISOString(),
updated_at: moment('2014-10-06T15:23:54').toISOString(),
tags: [{name: 'tag1'}, {name: 'tag2'}, {name: 'tag3'}],
author: {
name: 'Author name',
url: 'http//:testauthorurl.com',
slug: 'Author',
image: 'content/images/test-author-image.png',
website: 'http://authorwebsite.com',
facebook: 'testuser',
twitter: '@testuser'
}
};
helpers.ghost_head.call(
{relativeUrl: '/post/', safeVersion: '0.3', context: ['post'], post: post},
{data: {root: {context: ['post']}}}
).then(function (rendered) {
should.exist(rendered);
rendered.string.should.not.match(/<link rel="amphtml"/);
done();
}).catch(done);
});
});
});