From 10c214c14836b77640fd49112ca4c6ed17bf1165 Mon Sep 17 00:00:00 2001 From: Fabien 'egg' O'Carroll Date: Fri, 14 Jan 2022 18:55:48 +0200 Subject: [PATCH] Switched AMP to be 'off' by default in all new Ghost instances (#13907) refs https://github.com/TryGhost/Team/issues/1189 Support for AMP is slowly in decline, and makes developing new cards trickier, since AMP no longer has an effect of SEO we're going to disable it by default as a first step toward moving away from it. Co-authored-by: Thibaut Patel --- core/server/data/schema/default-settings.json | 2 +- test/e2e-api/admin/settings.test.js | 2 +- test/e2e-frontend/advanced_url_config.test.js | 19 ++++- test/e2e-frontend/default_routes.test.js | 79 +++++++++++-------- .../api-vs-frontend/canary.test.js | 46 ++++++++--- .../api-vs-frontend/v2.test.js | 46 ++++++++--- .../api-vs-frontend/v3.test.js | 46 ++++++++--- test/regression/site/frontend.test.js | 34 ++++++-- .../unit/server/data/schema/integrity.test.js | 2 +- 9 files changed, 191 insertions(+), 85 deletions(-) diff --git a/core/server/data/schema/default-settings.json b/core/server/data/schema/default-settings.json index 066a8d2473..5ebcdfbdfc 100644 --- a/core/server/data/schema/default-settings.json +++ b/core/server/data/schema/default-settings.json @@ -397,7 +397,7 @@ }, "amp": { "amp": { - "defaultValue": "true", + "defaultValue": "false", "validations": { "isIn": [["true", "false"]] }, diff --git a/test/e2e-api/admin/settings.test.js b/test/e2e-api/admin/settings.test.js index 5e72dca61f..5503727ace 100644 --- a/test/e2e-api/admin/settings.test.js +++ b/test/e2e-api/admin/settings.test.js @@ -35,7 +35,7 @@ describe('Settings API', function () { localUtils.API.checkResponse(jsonResponse, 'settings'); JSON.parse(_.find(jsonResponse.settings, {key: 'unsplash'}).value).should.eql(true); - JSON.parse(_.find(jsonResponse.settings, {key: 'amp'}).value).should.eql(true); + JSON.parse(_.find(jsonResponse.settings, {key: 'amp'}).value).should.eql(false); should.not.exist(_.find(jsonResponse.settings, {key: 'permalinks'})); should.not.exist(_.find(jsonResponse.settings, {key: 'ghost_head'})); should.not.exist(_.find(jsonResponse.settings, {key: 'ghost_foot'})); diff --git a/test/e2e-frontend/advanced_url_config.test.js b/test/e2e-frontend/advanced_url_config.test.js index ea52b69c66..a557e9d0f3 100644 --- a/test/e2e-frontend/advanced_url_config.test.js +++ b/test/e2e-frontend/advanced_url_config.test.js @@ -1,8 +1,10 @@ const should = require('should'); +const sinon = require('sinon'); const supertest = require('supertest'); const testUtils = require('../utils'); const configUtils = require('../utils/configUtils'); const urlUtils = require('../utils/urlUtils'); +const settings = require('../../core/shared/settings-cache'); let request; @@ -31,6 +33,10 @@ describe('Advanced URL Configurations', function () { urlUtils.restore(); }); + afterEach(function () { + sinon.restore(); + }); + it('http://localhost should 404', async function () { await request.get('/') .expect(404); @@ -67,11 +73,22 @@ describe('Advanced URL Configurations', function () { .expect(404); }); - it('/blog/welcome/amp/ should 200', async function () { + it('/blog/welcome/amp/ should 200 if amp is enabled', async function () { + sinon.stub(settings, 'get').callsFake(function (key, ...rest) { + if (key === 'amp') { + return true; + } + return settings.get.wrappedMethod.call(settings, key, ...rest); + }); await request.get('/blog/welcome/amp/') .expect(200); }); + it('/blog/welcome/amp/ should 301', async function () { + await request.get('/blog/welcome/amp/') + .expect(301); + }); + it('/welcome/amp/ should 404', async function () { await request.get('/welcome/amp/') .expect(404); diff --git a/test/e2e-frontend/default_routes.test.js b/test/e2e-frontend/default_routes.test.js index 88d0fbe3a2..f3038dd640 100644 --- a/test/e2e-frontend/default_routes.test.js +++ b/test/e2e-frontend/default_routes.test.js @@ -190,52 +190,63 @@ describe('Default Frontend routing', function () { }); describe('AMP post', function () { - it('should respond with html for valid url', async function () { - await request.get('/welcome/amp/') - .expect('Content-Type', /html/) - .expect('Cache-Control', testUtils.cacheRules.public) - .expect(200) - .expect(assertCorrectFrontendHeaders) - .expect((res) => { - const $ = cheerio.load(res.text); - - $('.post-title').text().should.equal('Start here for a quick overview of everything you need to know'); - - $('.content .post').length.should.equal(1); - $('.powered').text().should.equal(' Published with Ghost'); - $('body.amp-template').length.should.equal(1); - $('article.post').length.should.equal(1); - - $('style[amp-custom]').length.should.equal(1); - - // This asserts we should have some content (and not [object Promise] !) - $('.post-content p').length.should.be.greaterThan(0); - - res.text.should.containEql(':root {--ghost-accent-color: #FF1A75;}'); - res.text.should.not.containEql('__GHOST_URL__'); + describe('AMP Enabled', function () { + beforeEach(function () { + sinon.stub(settingsCache, 'get').callsFake(function (key, options) { + if (key === 'amp' && !options) { + return true; + } + return origCache.get(key, options); }); - }); + }); + it('should respond with html for valid url', async function () { + await request.get('/welcome/amp/') + .expect('Content-Type', /html/) + .expect('Cache-Control', testUtils.cacheRules.public) + .expect(200) + .expect(assertCorrectFrontendHeaders) + .expect((res) => { + const $ = cheerio.load(res.text); - it('should not work with date permalinks', async function () { - // get today's date - const date = moment().format('YYYY/MM/DD'); + $('.post-title').text().should.equal('Start here for a quick overview of everything you need to know'); - await request.get('/' + date + '/welcome/amp/') - .expect('Cache-Control', testUtils.cacheRules.private) - .expect(404) - .expect(/Page not found/) - .expect(assertCorrectFrontendHeaders); + $('.content .post').length.should.equal(1); + $('.powered').text().should.equal(' Published with Ghost'); + $('body.amp-template').length.should.equal(1); + $('article.post').length.should.equal(1); + + $('style[amp-custom]').length.should.equal(1); + + // This asserts we should have some content (and not [object Promise] !) + $('.post-content p').length.should.be.greaterThan(0); + + res.text.should.containEql(':root {--ghost-accent-color: #FF1A75;}'); + res.text.should.not.containEql('__GHOST_URL__'); + }); + }); + + it('should not work with date permalinks', async function () { + // get today's date + const date = moment().format('YYYY/MM/DD'); + + await request.get('/' + date + '/welcome/amp/') + .expect('Cache-Control', testUtils.cacheRules.private) + .expect(404) + .expect(/Page not found/) + .expect(assertCorrectFrontendHeaders); + }); }); describe('AMP Disabled', function () { - it('/amp/ should redirect to regular post, including any query params', async function () { + beforeEach(function () { sinon.stub(settingsCache, 'get').callsFake(function (key, options) { if (key === 'amp' && !options) { return false; } return origCache.get(key, options); }); - + }); + it('/amp/ should redirect to regular post, including any query params', async function () { await request.get('/welcome/amp/?q=a') .expect('Location', '/welcome/?q=a') .expect(301) diff --git a/test/regression/mock-express-style/api-vs-frontend/canary.test.js b/test/regression/mock-express-style/api-vs-frontend/canary.test.js index c28eddff9f..8c4331825c 100644 --- a/test/regression/mock-express-style/api-vs-frontend/canary.test.js +++ b/test/regression/mock-express-style/api-vs-frontend/canary.test.js @@ -66,20 +66,40 @@ describe('Integration - Web - Site canary', function () { }); }); - it('serve amp', function () { - const req = { - secure: true, - method: 'GET', - url: '/html-ipsum/amp/', - host: 'example.com' - }; + describe('AMP enabled', function () { + it('serve amp', function () { + localUtils.defaultMocks(sinon, {amp: true}); + const req = { + secure: true, + method: 'GET', + url: '/html-ipsum/amp/', + host: 'example.com' + }; - return localUtils.mockExpress.invoke(app, req) - .then(function (response) { - response.statusCode.should.eql(200); - response.template.should.match(/amp\.hbs/); - response.body.should.match(/

HTML Ipsum Presents<\/h1>/); - }); + return localUtils.mockExpress.invoke(app, req) + .then(function (response) { + response.statusCode.should.eql(200); + response.template.should.match(/amp\.hbs/); + response.body.should.match(/

HTML Ipsum Presents<\/h1>/); + }); + }); + }); + + describe('AMP disabled', function () { + it('serve amp', function () { + localUtils.defaultMocks(sinon, {amp: false}); + const req = { + secure: true, + method: 'GET', + url: '/html-ipsum/amp/', + host: 'example.com' + }; + + return localUtils.mockExpress.invoke(app, req) + .then(function (response) { + response.statusCode.should.eql(301); + }); + }); }); it('post not found', function () { diff --git a/test/regression/mock-express-style/api-vs-frontend/v2.test.js b/test/regression/mock-express-style/api-vs-frontend/v2.test.js index a1e3a0d79f..fb13030ab5 100644 --- a/test/regression/mock-express-style/api-vs-frontend/v2.test.js +++ b/test/regression/mock-express-style/api-vs-frontend/v2.test.js @@ -66,20 +66,40 @@ describe('Integration - Web - Site v2', function () { }); }); - it('serve amp', function () { - const req = { - secure: true, - method: 'GET', - url: '/html-ipsum/amp/', - host: 'example.com' - }; + describe('AMP enabled', function () { + it('serve amp', function () { + localUtils.defaultMocks(sinon, {amp: true}); + const req = { + secure: true, + method: 'GET', + url: '/html-ipsum/amp/', + host: 'example.com' + }; - return localUtils.mockExpress.invoke(app, req) - .then(function (response) { - response.statusCode.should.eql(200); - response.template.should.match(/amp\.hbs/); - response.body.should.match(/

HTML Ipsum Presents<\/h1>/); - }); + return localUtils.mockExpress.invoke(app, req) + .then(function (response) { + response.statusCode.should.eql(200); + response.template.should.match(/amp\.hbs/); + response.body.should.match(/

HTML Ipsum Presents<\/h1>/); + }); + }); + }); + + describe('AMP disabled', function () { + it('serve amp', function () { + localUtils.defaultMocks(sinon, {amp: false}); + const req = { + secure: true, + method: 'GET', + url: '/html-ipsum/amp/', + host: 'example.com' + }; + + return localUtils.mockExpress.invoke(app, req) + .then(function (response) { + response.statusCode.should.eql(301); + }); + }); }); it('post not found', function () { diff --git a/test/regression/mock-express-style/api-vs-frontend/v3.test.js b/test/regression/mock-express-style/api-vs-frontend/v3.test.js index 0f12cc17f4..1509992b85 100644 --- a/test/regression/mock-express-style/api-vs-frontend/v3.test.js +++ b/test/regression/mock-express-style/api-vs-frontend/v3.test.js @@ -66,20 +66,40 @@ describe('Integration - Web - Site v3', function () { }); }); - it('serve amp', function () { - const req = { - secure: true, - method: 'GET', - url: '/html-ipsum/amp/', - host: 'example.com' - }; + describe('AMP enabled', function () { + it('serve amp', function () { + localUtils.defaultMocks(sinon, {amp: true}); + const req = { + secure: true, + method: 'GET', + url: '/html-ipsum/amp/', + host: 'example.com' + }; - return localUtils.mockExpress.invoke(app, req) - .then(function (response) { - response.statusCode.should.eql(200); - response.template.should.match(/amp\.hbs/); - response.body.should.match(/

HTML Ipsum Presents<\/h1>/); - }); + return localUtils.mockExpress.invoke(app, req) + .then(function (response) { + response.statusCode.should.eql(200); + response.template.should.match(/amp\.hbs/); + response.body.should.match(/

HTML Ipsum Presents<\/h1>/); + }); + }); + }); + + describe('AMP disabled', function () { + it('serve amp', function () { + localUtils.defaultMocks(sinon, {amp: false}); + const req = { + secure: true, + method: 'GET', + url: '/html-ipsum/amp/', + host: 'example.com' + }; + + return localUtils.mockExpress.invoke(app, req) + .then(function (response) { + response.statusCode.should.eql(301); + }); + }); }); it('post not found', function () { diff --git a/test/regression/site/frontend.test.js b/test/regression/site/frontend.test.js index fafc37cf2b..5bf23530e0 100644 --- a/test/regression/site/frontend.test.js +++ b/test/regression/site/frontend.test.js @@ -11,6 +11,7 @@ const _ = require('lodash'); const testUtils = require('../../utils'); const configUtils = require('../../utils/configUtils'); const config = require('../../../core/shared/config'); +const settingsCache = require('../../../core/shared/settings-cache'); let request; describe('Frontend Routing', function () { @@ -218,14 +219,31 @@ describe('Frontend Routing', function () { }); describe('amp', function () { - it('should 404 for amp parameter', function (done) { - // NOTE: only post pages are supported so the router doesn't have a way to distinguish if - // the request was done after AMP 'Page' or 'Post' - request.get('/static-page-test/amp/') - .expect('Cache-Control', testUtils.cacheRules.private) - .expect(404) - .expect(/Post not found/) - .end(doEnd(done)); + describe('amp enabled', function (){ + beforeEach(function () { + sinon.stub(settingsCache, 'get').withArgs('amp').returns(true); + }); + it('should 404 for amp parameter', function (done) { + // NOTE: only post pages are supported so the router doesn't have a way to distinguish if + // the request was done after AMP 'Page' or 'Post' + request.get('/static-page-test/amp/') + .expect('Cache-Control', testUtils.cacheRules.private) + .expect(404) + .expect(/Post not found/) + .end(doEnd(done)); + }); + }); + describe('amp disabled', function (){ + beforeEach(function () { + sinon.stub(settingsCache, 'get').withArgs('amp').returns(false); + }); + it('should 301 for amp parameter', function (done) { + // NOTE: only post pages are supported so the router doesn't have a way to distinguish if + // the request was done after AMP 'Page' or 'Post' + request.get('/static-page-test/amp/') + .expect(301) + .end(doEnd(done)); + }); }); }); }); diff --git a/test/unit/server/data/schema/integrity.test.js b/test/unit/server/data/schema/integrity.test.js index 15dddb6b4e..6f3077ae58 100644 --- a/test/unit/server/data/schema/integrity.test.js +++ b/test/unit/server/data/schema/integrity.test.js @@ -37,7 +37,7 @@ describe('DB version integrity', function () { // Only these variables should need updating const currentSchemaHash = 'e649797a5de92d417744f6f2623c79cf'; const currentFixturesHash = '07d4b0c4cf159b34344a6b5e88c74e9f'; - const currentSettingsHash = 'b06316ebbf62381158e1c46c97c0b77a'; + const currentSettingsHash = 'd73b63e33153c9256bca42ebfd376779'; const currentRoutesHash = '3d180d52c663d173a6be791ef411ed01'; // If this test is failing, then it is likely a change has been made that requires a DB version bump,