⚡❓ Make AMP optional (#7830)

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

core/server/api/settings.js

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

core/server/apps/amp/index.js

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

core/server/apps/amp/lib/router.js

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

core/server/apps/amp/tests/router_spec.js

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

core/server/controllers/frontend/index.js

@@ -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, '/'));

core/server/controllers/frontend/post-lookup.js

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

core/server/data/schema/default-settings.json

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

core/server/helpers/ghost_head.js

@@ -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) + '" />');
             }

core/test/functional/routes/frontend_spec.js

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

core/test/unit/controllers/frontend/post-lookup_spec.js

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