From 4ee2fcd8699cc57da20d8b1194f8683bdf8d08de Mon Sep 17 00:00:00 2001 From: Hannah Wolfe Date: Tue, 5 Apr 2022 13:12:31 +0100 Subject: [PATCH] Moved frontend data helpers into their own service - Some of the helpers inside the routing service would be better suited to their own service - These two helpers fetchData and entryLookup talk to the API to get data & so make a decent start for a data service - The data service would be the single point of contact with the API for the frontend - Doing this now cos I'm moving some files around ahead of deleting things for 5.0 --- core/frontend/apps/amp/lib/router.js | 9 ++-- .../{routing/helpers => data}/entry-lookup.js | 6 +-- .../{routing/helpers => data}/fetch-data.js | 6 +-- core/frontend/services/data/index.js | 4 ++ .../services/routing/controllers/channel.js | 3 +- .../routing/controllers/collection.js | 3 +- .../services/routing/controllers/entry.js | 3 +- .../services/routing/controllers/rss.js | 3 +- .../services/routing/helpers/index.js | 8 ---- test/unit/frontend/apps/amp/router.test.js | 9 ++-- .../helpers => data}/entry-lookup.test.js | 41 ++++++++++++------- .../helpers => data}/fetch-data.test.js | 21 +++++----- .../routing/controllers/channel.test.js | 3 +- .../routing/controllers/collection.test.js | 3 +- .../routing/controllers/entry.test.js | 3 +- .../services/routing/controllers/rss.test.js | 4 +- 16 files changed, 74 insertions(+), 55 deletions(-) rename core/frontend/services/{routing/helpers => data}/entry-lookup.js (89%) rename core/frontend/services/{routing/helpers => data}/fetch-data.js (95%) create mode 100644 core/frontend/services/data/index.js rename test/unit/frontend/services/{routing/helpers => data}/entry-lookup.test.js (67%) rename test/unit/frontend/services/{routing/helpers => data}/fetch-data.test.js (89%) diff --git a/core/frontend/apps/amp/lib/router.js b/core/frontend/apps/amp/lib/router.js index a7e7538bde..ec0ea25eb2 100644 --- a/core/frontend/apps/amp/lib/router.js +++ b/core/frontend/apps/amp/lib/router.js @@ -7,6 +7,7 @@ const errors = require('@tryghost/errors'); // Dirty requires const urlService = require('../../../../server/services/url'); +const dataService = require('../../../services/data'); const helpers = require('../../../services/routing/helpers'); const templateName = 'amp'; @@ -23,15 +24,15 @@ function _renderer(req, res, next) { // Renderer begin // Format data - let data = req.body || {}; + let body = req.body || {}; // CASE: we only support amp pages for posts that are not static pages - if (!data.post || data.post.page) { + if (!body.post || body.post.page) { return next(new errors.NotFoundError({message: tpl(messages.pageNotFound)})); } // Render Call - return helpers.renderer(req, res, data); + return helpers.renderer(req, res, body); } // This here is a controller. @@ -71,7 +72,7 @@ function getPostData(req, res, next) { // @NOTE: amp is not supported for static pages // @TODO: https://github.com/TryGhost/Ghost/issues/10548 - helpers.entryLookup(urlWithoutSubdirectoryWithoutAmp, {permalinks, query: {controller: 'postsPublic', resource: 'posts'}}, res.locals) + dataService.entryLookup(urlWithoutSubdirectoryWithoutAmp, {permalinks, query: {controller: 'postsPublic', resource: 'posts'}}, res.locals) .then((result) => { if (result && result.entry) { req.body.post = result.entry; diff --git a/core/frontend/services/routing/helpers/entry-lookup.js b/core/frontend/services/data/entry-lookup.js similarity index 89% rename from core/frontend/services/routing/helpers/entry-lookup.js rename to core/frontend/services/data/entry-lookup.js index f8645ab682..75dcf47da2 100644 --- a/core/frontend/services/routing/helpers/entry-lookup.js +++ b/core/frontend/services/data/entry-lookup.js @@ -1,11 +1,11 @@ const _ = require('lodash'); const Promise = require('bluebird'); const url = require('url'); -const debug = require('@tryghost/debug')('services:routing:helpers:entry-lookup'); +const debug = require('@tryghost/debug')('services:data:entry-lookup'); const routeMatch = require('path-match')(); /** - * @description Query API for a single entry/resource. + * Query API for a single entry/resource. * @param {String} postUrl * @param {Object} routerOptions * @param {Object} locals @@ -14,7 +14,7 @@ const routeMatch = require('path-match')(); function entryLookup(postUrl, routerOptions, locals) { debug(postUrl); - const api = require('../../proxy').api[locals.apiVersion]; + const api = require('../proxy').api[locals.apiVersion]; const targetPath = url.parse(postUrl).path; const permalinks = routerOptions.permalinks; let isEditURL = false; diff --git a/core/frontend/services/routing/helpers/fetch-data.js b/core/frontend/services/data/fetch-data.js similarity index 95% rename from core/frontend/services/routing/helpers/fetch-data.js rename to core/frontend/services/data/fetch-data.js index caea7b2a19..4f662376e0 100644 --- a/core/frontend/services/routing/helpers/fetch-data.js +++ b/core/frontend/services/data/fetch-data.js @@ -34,7 +34,7 @@ const defaultPostQuery = _.cloneDeep(queryDefaults); defaultPostQuery.options = defaultQueryOptions.options; /** - * @description Process query request. + * Process query request. * * Takes a 'query' object, ensures that type, resource and options are set * Replaces occurrences of `%s` in options with slugParam @@ -45,7 +45,7 @@ defaultPostQuery.options = defaultQueryOptions.options; * @returns {Promise} */ function processQuery(query, slugParam, locals) { - const api = require('../../proxy').api[locals.apiVersion]; + const api = require('../proxy').api[locals.apiVersion]; query = _.cloneDeep(query); @@ -62,7 +62,7 @@ function processQuery(query, slugParam, locals) { } /** - * @description Fetch data from API helper for controllers. + * Fetch data from API helper for controllers. * * Calls out to get posts per page, builds the final posts query & builds any additional queries * Wraps the queries using Promise.props to ensure it gets named responses diff --git a/core/frontend/services/data/index.js b/core/frontend/services/data/index.js new file mode 100644 index 0000000000..45d05143d2 --- /dev/null +++ b/core/frontend/services/data/index.js @@ -0,0 +1,4 @@ +module.exports = { + entryLookup: require('./entry-lookup'), + fetchData: require('./fetch-data') +}; diff --git a/core/frontend/services/routing/controllers/channel.js b/core/frontend/services/routing/controllers/channel.js index f0331de176..43a0188770 100644 --- a/core/frontend/services/routing/controllers/channel.js +++ b/core/frontend/services/routing/controllers/channel.js @@ -4,6 +4,7 @@ const tpl = require('@tryghost/tpl'); const errors = require('@tryghost/errors'); const security = require('@tryghost/security'); const themeEngine = require('../../theme-engine'); +const dataService = require('../../data'); const helpers = require('../helpers'); const messages = { @@ -50,7 +51,7 @@ module.exports = function channelController(req, res, next) { } } - return helpers.fetchData(pathOptions, res.routerOptions, res.locals) + return dataService.fetchData(pathOptions, res.routerOptions, res.locals) .then(function handleResult(result) { // CASE: requested page is greater than number of pages we have if (pathOptions.page > result.meta.pagination.pages) { diff --git a/core/frontend/services/routing/controllers/collection.js b/core/frontend/services/routing/controllers/collection.js index 6bee3ca4db..04ace71008 100644 --- a/core/frontend/services/routing/controllers/collection.js +++ b/core/frontend/services/routing/controllers/collection.js @@ -6,6 +6,7 @@ const security = require('@tryghost/security'); const {routerManager} = require('../'); const themeEngine = require('../../theme-engine'); const helpers = require('../helpers'); +const dataService = require('../../data'); const messages = { pageNotFound: 'Page not found.' @@ -49,7 +50,7 @@ module.exports = function collectionController(req, res, next) { } debug('fetching data'); - return helpers.fetchData(pathOptions, res.routerOptions, res.locals) + return dataService.fetchData(pathOptions, res.routerOptions, res.locals) .then(function handleResult(result) { // CASE: requested page is greater than number of pages we have if (pathOptions.page > result.meta.pagination.pages) { diff --git a/core/frontend/services/routing/controllers/entry.js b/core/frontend/services/routing/controllers/entry.js index f2fd45f66b..ff9df24b46 100644 --- a/core/frontend/services/routing/controllers/entry.js +++ b/core/frontend/services/routing/controllers/entry.js @@ -3,6 +3,7 @@ const url = require('url'); const config = require('../../../../shared/config'); const {routerManager} = require('../'); const urlUtils = require('../../../../shared/url-utils'); +const dataService = require('../../data'); const helpers = require('../helpers'); /** @@ -15,7 +16,7 @@ const helpers = require('../helpers'); module.exports = function entryController(req, res, next) { debug('entryController', res.routerOptions); - return helpers.entryLookup(req.path, res.routerOptions, res.locals) + return dataService.entryLookup(req.path, res.routerOptions, res.locals) .then(function then(lookup) { // Format data 1 const entry = lookup ? lookup.entry : false; diff --git a/core/frontend/services/routing/controllers/rss.js b/core/frontend/services/routing/controllers/rss.js index 091160d379..b0e29bc1f4 100644 --- a/core/frontend/services/routing/controllers/rss.js +++ b/core/frontend/services/routing/controllers/rss.js @@ -5,6 +5,7 @@ const security = require('@tryghost/security'); const settingsCache = require('../../../../shared/settings-cache'); const rssService = require('../../rss'); const helpers = require('../helpers'); +const dataService = require('../../data'); // @TODO: is this really correct? Should we be using meta data title? function getTitle(relatedData) { @@ -35,7 +36,7 @@ module.exports = function rssController(req, res, next) { // @TODO: This belongs to the rss service O_o const baseUrl = url.parse(req.originalUrl).pathname; - helpers.fetchData(pathOptions, res.routerOptions, res.locals) + dataService.fetchData(pathOptions, res.routerOptions, res.locals) .then(function formatResult(result) { const response = _.pick(result, ['posts', 'meta']); diff --git a/core/frontend/services/routing/helpers/index.js b/core/frontend/services/routing/helpers/index.js index cd2e51082c..bd40696fc9 100644 --- a/core/frontend/services/routing/helpers/index.js +++ b/core/frontend/services/routing/helpers/index.js @@ -1,12 +1,4 @@ module.exports = { - get entryLookup() { - return require('./entry-lookup'); - }, - - get fetchData() { - return require('./fetch-data'); - }, - get renderEntries() { return require('./render-entries'); }, diff --git a/test/unit/frontend/apps/amp/router.test.js b/test/unit/frontend/apps/amp/router.test.js index 4f01750ddb..9c687577aa 100644 --- a/test/unit/frontend/apps/amp/router.test.js +++ b/test/unit/frontend/apps/amp/router.test.js @@ -5,6 +5,7 @@ const path = require('path'); const ampController = require('../../../../../core/frontend/apps/amp/lib/router'); const urlService = require('../../../../../core/server/services/url'); const helpers = require('../../../../../core/frontend/services/routing/helpers'); +const dataService = require('../../../../../core/frontend/services/data'); const testUtils = require('../../../../utils'); const configUtils = require('../../../../utils/configUtils'); @@ -107,7 +108,7 @@ describe('Unit - apps/amp/lib/router', function () { entryLookupStub = sinon.stub(); - sinon.stub(helpers, 'entryLookup').get(function () { + sinon.stub(dataService, 'entryLookup').get(function () { return entryLookupStub; }); @@ -123,7 +124,7 @@ describe('Unit - apps/amp/lib/router', function () { urlService.getPermalinkByUrl.withArgs('/welcome/').returns('/:slug/'); - helpers.entryLookup.withArgs('/welcome/', {permalinks: '/:slug/', query: {controller: 'postsPublic', resource: 'posts'}}) + dataService.entryLookup.withArgs('/welcome/', {permalinks: '/:slug/', query: {controller: 'postsPublic', resource: 'posts'}}) .resolves({ entry: post }); @@ -140,7 +141,7 @@ describe('Unit - apps/amp/lib/router', function () { urlService.getPermalinkByUrl.withArgs('/welcome/').returns('/:slug/'); - helpers.entryLookup.withArgs('/welcome/', {permalinks: '/:slug/', query: {controller: 'postsPublic', resource: 'posts'}}).resolves({ + dataService.entryLookup.withArgs('/welcome/', {permalinks: '/:slug/', query: {controller: 'postsPublic', resource: 'posts'}}).resolves({ entry: post }); @@ -155,7 +156,7 @@ describe('Unit - apps/amp/lib/router', function () { urlService.getPermalinkByUrl.withArgs('/welcome/').returns('/:slug/'); - helpers.entryLookup.withArgs('/welcome/', {permalinks: '/:slug/', query: {controller: 'postsPublic', resource: 'posts'}}) + dataService.entryLookup.withArgs('/welcome/', {permalinks: '/:slug/', query: {controller: 'postsPublic', resource: 'posts'}}) .rejects(new errors.NotFoundError()); ampController.getPostData(req, res, function (err) { diff --git a/test/unit/frontend/services/routing/helpers/entry-lookup.test.js b/test/unit/frontend/services/data/entry-lookup.test.js similarity index 67% rename from test/unit/frontend/services/routing/helpers/entry-lookup.test.js rename to test/unit/frontend/services/data/entry-lookup.test.js index 97563da550..72a6b22213 100644 --- a/test/unit/frontend/services/routing/helpers/entry-lookup.test.js +++ b/test/unit/frontend/services/data/entry-lookup.test.js @@ -1,11 +1,12 @@ const should = require('should'); const sinon = require('sinon'); -const Promise = require('bluebird'); -const testUtils = require('../../../../../utils'); -const api = require('../../../../../../core/server/api'); -const helpers = require('../../../../../../core/frontend/services/routing/helpers'); -describe('Unit - services/routing/helpers/entry-lookup', function () { +const API_VERSION = 'canary'; +const api = require('../../../../../core/server/api')[API_VERSION]; +const data = require('../../../../../core/frontend/services/data'); +const testUtils = require('../../../../utils'); + +describe('Unit - frontend/data/entry-lookup', function () { let locals; afterEach(function () { @@ -35,25 +36,25 @@ describe('Unit - services/routing/helpers/entry-lookup', function () { pages: pages }); - sinon.stub(api.canary, 'posts').get(() => { + sinon.stub(api, 'posts').get(() => { return { read: postsReadStub }; }); - sinon.stub(api.canary, 'pagesPublic').get(() => { + sinon.stub(api, 'pagesPublic').get(() => { return { read: pagesReadStub }; }); - locals = {apiVersion: 'canary'}; + locals = {apiVersion: API_VERSION}; }); it('ensure pages controller is triggered', function () { const testUrl = 'http://127.0.0.1:2369' + pages[0].url; - return helpers.entryLookup(testUrl, routerOptions, locals).then(function (lookup) { + return data.entryLookup(testUrl, routerOptions, locals).then(function (lookup) { postsReadStub.called.should.be.false(); pagesReadStub.calledOnce.should.be.true(); should.exist(lookup.entry); @@ -65,7 +66,7 @@ describe('Unit - services/routing/helpers/entry-lookup', function () { describe('posts', function () { const routerOptions = { - permalinks: '/:slug/', + permalinks: '/:slug/:options(edit)?/', query: {controller: 'posts', resource: 'posts'} }; @@ -86,25 +87,25 @@ describe('Unit - services/routing/helpers/entry-lookup', function () { posts: posts }); - sinon.stub(api.canary, 'posts').get(() => { + sinon.stub(api, 'posts').get(() => { return { read: postsReadStub }; }); - sinon.stub(api.canary, 'pagesPublic').get(() => { + sinon.stub(api, 'pagesPublic').get(() => { return { read: pagesReadStub }; }); - locals = {apiVersion: 'canary'}; + locals = {apiVersion: API_VERSION}; }); it('ensure posts controller is triggered', function () { const testUrl = 'http://127.0.0.1:2369' + posts[0].url; - return helpers.entryLookup(testUrl, routerOptions, locals).then(function (lookup) { + return data.entryLookup(testUrl, routerOptions, locals).then(function (lookup) { postsReadStub.calledOnce.should.be.true(); pagesReadStub.called.should.be.false(); should.exist(lookup.entry); @@ -112,5 +113,17 @@ describe('Unit - services/routing/helpers/entry-lookup', function () { lookup.isEditURL.should.be.false(); }); }); + + it('can handle an edit url', function () { + const testUrl = `http://127.0.0.1:2369${posts[0].url}edit/`; + + return data.entryLookup(testUrl, routerOptions, locals).then(function (lookup) { + postsReadStub.calledOnce.should.be.true(); + pagesReadStub.called.should.be.false(); + should.exist(lookup.entry); + lookup.entry.should.have.property('url', posts[0].url); + lookup.isEditURL.should.be.true(); + }); + }); }); }); diff --git a/test/unit/frontend/services/routing/helpers/fetch-data.test.js b/test/unit/frontend/services/data/fetch-data.test.js similarity index 89% rename from test/unit/frontend/services/routing/helpers/fetch-data.test.js rename to test/unit/frontend/services/data/fetch-data.test.js index 66a9a5a03a..5626c05a03 100644 --- a/test/unit/frontend/services/routing/helpers/fetch-data.test.js +++ b/test/unit/frontend/services/data/fetch-data.test.js @@ -1,11 +1,12 @@ const should = require('should'); const sinon = require('sinon'); -const API_VERSION = 'canary'; -const api = require('../../../../../../core/server/api')[API_VERSION]; -const helpers = require('../../../../../../core/frontend/services/routing/helpers'); -const testUtils = require('../../../../../utils'); -describe('Unit - services/routing/helpers/fetch-data', function () { +const API_VERSION = 'canary'; +const api = require('../../../../../core/server/api')[API_VERSION]; +const data = require('../../../../../core/frontend/services/data'); +const testUtils = require('../../../../utils'); + +describe('Unit - frontend/data/fetch-data', function () { let posts; let tags; let locals; @@ -56,7 +57,7 @@ describe('Unit - services/routing/helpers/fetch-data', function () { }); it('should handle no options', function (done) { - helpers.fetchData(null, null, locals).then(function (result) { + data.fetchData(null, null, locals).then(function (result) { should.exist(result); result.should.be.an.Object().with.properties('posts', 'meta'); result.should.not.have.property('data'); @@ -71,7 +72,7 @@ describe('Unit - services/routing/helpers/fetch-data', function () { }); it('should handle path options with page/limit', function (done) { - helpers.fetchData({page: 2, limit: 10}, null, locals).then(function (result) { + data.fetchData({page: 2, limit: 10}, null, locals).then(function (result) { should.exist(result); result.should.be.an.Object().with.properties('posts', 'meta'); result.should.not.have.property('data'); @@ -104,7 +105,7 @@ describe('Unit - services/routing/helpers/fetch-data', function () { } }; - helpers.fetchData(pathOptions, routerOptions, locals).then(function (result) { + data.fetchData(pathOptions, routerOptions, locals).then(function (result) { should.exist(result); result.should.be.an.Object().with.properties('posts', 'meta', 'data'); result.data.should.be.an.Object().with.properties('featured'); @@ -135,7 +136,7 @@ describe('Unit - services/routing/helpers/fetch-data', function () { } }; - helpers.fetchData(pathOptions, routerOptions, locals).then(function (result) { + data.fetchData(pathOptions, routerOptions, locals).then(function (result) { should.exist(result); result.should.be.an.Object().with.properties('posts', 'meta', 'data'); @@ -170,7 +171,7 @@ describe('Unit - services/routing/helpers/fetch-data', function () { } }; - helpers.fetchData(pathOptions, routerOptions, locals).then(function (result) { + data.fetchData(pathOptions, routerOptions, locals).then(function (result) { should.exist(result); result.should.be.an.Object().with.properties('posts', 'meta', 'data'); result.data.should.be.an.Object().with.properties('tag'); diff --git a/test/unit/frontend/services/routing/controllers/channel.test.js b/test/unit/frontend/services/routing/controllers/channel.test.js index 6f1e0f3a24..bf87782c2f 100644 --- a/test/unit/frontend/services/routing/controllers/channel.test.js +++ b/test/unit/frontend/services/routing/controllers/channel.test.js @@ -6,6 +6,7 @@ const security = require('@tryghost/security'); const themeEngine = require('../../../../../../core/frontend/services/theme-engine'); const controllers = require('../../../../../../core/frontend/services/routing/controllers'); const helpers = require('../../../../../../core/frontend/services/routing/helpers'); +const dataService = require('../../../../../../core/frontend/services/data'); function failTest(done) { return function (err) { @@ -34,7 +35,7 @@ describe('Unit - services/routing/controllers/channel', function () { fetchDataStub = sinon.stub(); renderStub = sinon.stub(); - sinon.stub(helpers, 'fetchData').get(function () { + sinon.stub(dataService, 'fetchData').get(function () { return fetchDataStub; }); diff --git a/test/unit/frontend/services/routing/controllers/collection.test.js b/test/unit/frontend/services/routing/controllers/collection.test.js index 7fefafd51a..04d69ab91b 100644 --- a/test/unit/frontend/services/routing/controllers/collection.test.js +++ b/test/unit/frontend/services/routing/controllers/collection.test.js @@ -7,6 +7,7 @@ const themeEngine = require('../../../../../../core/frontend/services/theme-engi const routerManager = require('../../../../../../core/frontend/services/routing/').routerManager; const controllers = require('../../../../../../core/frontend/services/routing/controllers'); const helpers = require('../../../../../../core/frontend/services/routing/helpers'); +const dataService = require('../../../../../../core/frontend/services/data'); function failTest(done) { return function (err) { @@ -36,7 +37,7 @@ describe('Unit - services/routing/controllers/collection', function () { fetchDataStub = sinon.stub(); renderStub = sinon.stub(); - sinon.stub(helpers, 'fetchData').get(function () { + sinon.stub(dataService, 'fetchData').get(function () { return fetchDataStub; }); diff --git a/test/unit/frontend/services/routing/controllers/entry.test.js b/test/unit/frontend/services/routing/controllers/entry.test.js index e46bbc4563..44aa3557ff 100644 --- a/test/unit/frontend/services/routing/controllers/entry.test.js +++ b/test/unit/frontend/services/routing/controllers/entry.test.js @@ -6,6 +6,7 @@ const urlUtils = require('../../../../../../core/shared/url-utils'); const routerManager = require('../../../../../../core/frontend/services/routing/').routerManager; const controllers = require('../../../../../../core/frontend/services/routing/controllers'); const helpers = require('../../../../../../core/frontend/services/routing/helpers'); +const dataService = require('../../../../../../core/frontend/services/data'); const EDITOR_URL = `/#/editor/post/`; describe('Unit - services/routing/controllers/entry', function () { @@ -27,7 +28,7 @@ describe('Unit - services/routing/controllers/entry', function () { entryLookUpStub = sinon.stub(); renderStub = sinon.stub(); - sinon.stub(helpers, 'entryLookup').get(function () { + sinon.stub(dataService, 'entryLookup').get(function () { return entryLookUpStub; }); diff --git a/test/unit/frontend/services/routing/controllers/rss.test.js b/test/unit/frontend/services/routing/controllers/rss.test.js index ecb6263491..7cb8bdeddd 100644 --- a/test/unit/frontend/services/routing/controllers/rss.test.js +++ b/test/unit/frontend/services/routing/controllers/rss.test.js @@ -3,7 +3,7 @@ const testUtils = require('../../../../../utils'); const security = require('@tryghost/security'); const settingsCache = require('../../../../../../core/shared/settings-cache'); const controllers = require('../../../../../../core/frontend/services/routing/controllers'); -const helpers = require('../../../../../../core/frontend/services/routing/helpers'); +const dataService = require('../../../../../../core/frontend/services/data'); const rssService = require('../../../../../../core/frontend/services/rss'); // Helper function to prevent unit tests @@ -43,7 +43,7 @@ describe('Unit - services/routing/controllers/rss', function () { next = sinon.stub(); fetchDataStub = sinon.stub(); - sinon.stub(helpers, 'fetchData').get(function () { + sinon.stub(dataService, 'fetchData').get(function () { return fetchDataStub; });