From c49dba12a01e50dd4766a5e2bda27a561bd0d757 Mon Sep 17 00:00:00 2001 From: Hannah Wolfe Date: Sun, 27 Aug 2017 18:00:32 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Added=20error=20handling=20to=20?= =?UTF-8?q?prev/next=20post=20helpers?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit refs #8703 - On API error, call inverse with a data error, the same as the get helper --- core/server/helpers/get.js | 3 +- core/server/helpers/prev_next.js | 52 ++++-- core/server/translations/en.json | 2 +- .../unit/server_helpers/next_post_spec.js | 174 ++++++++++++------ 4 files changed, 160 insertions(+), 71 deletions(-) diff --git a/core/server/helpers/get.js b/core/server/helpers/get.js index f9a19ecb0c..b23f21ad86 100644 --- a/core/server/helpers/get.js +++ b/core/server/helpers/get.js @@ -103,7 +103,7 @@ get = function get(resource, options) { apiMethod; if (!options.fn) { - data.error = i18n.t('warnings.helpers.get.mustBeCalledAsBlock'); + data.error = i18n.t('warnings.helpers.get.mustBeCalledAsBlock', {helperName: 'get'}); logging.warn(data.error); return Promise.resolve(); } @@ -136,6 +136,7 @@ get = function get(resource, options) { blockParams: blockParams }); }).catch(function error(err) { + logging.error(err); data.error = err.message; return options.inverse(self, {data: data}); }); diff --git a/core/server/helpers/prev_next.js b/core/server/helpers/prev_next.js index 0d566f47bc..d30daec233 100644 --- a/core/server/helpers/prev_next.js +++ b/core/server/helpers/prev_next.js @@ -6,23 +6,36 @@ var proxy = require('./proxy'), Promise = require('bluebird'), + logging = proxy.logging, + i18n = proxy.i18n, + createFrame = proxy.hbs.handlebars.createFrame, + api = proxy.api, isPost = proxy.checks.isPost, fetch; -fetch = function fetch(apiOptions, options) { - return api.posts.read(apiOptions).then(function (result) { - var related = result.posts[0]; +fetch = function fetch(apiOptions, options, data) { + var self = this; - if (related.previous) { - return options.fn(related.previous); - } else if (related.next) { - return options.fn(related.next); - } else { - return options.inverse(this); - } - }); + return api.posts + .read(apiOptions) + .then(function handleSuccess(result) { + var related = result.posts[0]; + + if (related.previous) { + return options.fn(related.previous, {data: data}); + } else if (related.next) { + return options.fn(related.next, {data: data}); + } else { + return options.inverse(self, {data: data}); + } + }) + .catch(function handleError(err) { + logging.error(err); + data.error = err.message; + return options.inverse(self, {data: data}); + }); }; // If prevNext method is called without valid post data then we must return a promise, if there is valid post data @@ -31,14 +44,21 @@ fetch = function fetch(apiOptions, options) { module.exports = function prevNext(options) { options = options || {}; - var apiOptions = { - include: options.name === 'prev_post' ? 'previous,previous.author,previous.tags' : 'next,next.author,next.tags' - }; + var data = createFrame(options.data), + apiOptions = { + include: options.name === 'prev_post' ? 'previous,previous.author,previous.tags' : 'next,next.author,next.tags' + }; + + if (!options.fn) { + data.error = i18n.t('warnings.helpers.mustBeCalledAsBlock', {helperName: options.name}); + logging.warn(data.error); + return Promise.resolve(); + } if (isPost(this) && this.status === 'published') { apiOptions.slug = this.slug; - return fetch(apiOptions, options); + return fetch(apiOptions, options, data); } else { - return Promise.resolve(options.inverse(this)); + return Promise.resolve(options.inverse(this, {data: data})); } }; diff --git a/core/server/translations/en.json b/core/server/translations/en.json index 6e4f7cbd63..a9aa9962f5 100644 --- a/core/server/translations/en.json +++ b/core/server/translations/en.json @@ -487,11 +487,11 @@ "helperNotAvailable": "The \\{\\{{helperName}\\}\\} helper is not available.", "flagMustBeEnabled": "The {flagName} flag must be enabled in labs if you wish to use the \\{\\{{helperName}\\}\\} helper.", "seeLink": "See {url}", + "mustBeCalledAsBlock": "The \\{\\{{helperName}\\}\\} helper must be called as a block. E.g. \\{\\{#{helperName}\\}\\} \\{\\{/{helperName}\\}\\}", "foreach": { "iteratorNeeded": "Need to pass an iterator to #foreach" }, "get": { - "mustBeCalledAsBlock": "Get helper must be called as a block", "invalidResource": "Invalid resource given to get helper" }, "has": { diff --git a/core/test/unit/server_helpers/next_post_spec.js b/core/test/unit/server_helpers/next_post_spec.js index bf28f7b86c..365c7df120 100644 --- a/core/test/unit/server_helpers/next_post_spec.js +++ b/core/test/unit/server_helpers/next_post_spec.js @@ -5,6 +5,7 @@ var should = require('should'), // jshint ignore:line // Stuff we are testing helpers = require('../../../server/helpers'), api = require('../../../server/api'), + errors = require('../../../server/errors'), sandbox = sinon.sandbox.create(); @@ -31,25 +32,29 @@ describe('{{next_post}} helper', function () { inverse = sinon.spy(), optionsData = {name: 'next_post', fn: fn, inverse: inverse}; - helpers.prev_post.call({ - html: 'content', - status: 'published', - mobiledoc: markdownToMobiledoc('ff'), - title: 'post2', - slug: 'current', - created_at: new Date(0), - url: '/current/' - }, optionsData).then(function () { - fn.calledOnce.should.be.true(); - inverse.calledOnce.should.be.false(); + helpers.prev_post + .call({ + html: 'content', + status: 'published', + mobiledoc: markdownToMobiledoc('ff'), + title: 'post2', + slug: 'current', + created_at: new Date(0), + url: '/current/' + }, optionsData) + .then(function () { + fn.calledOnce.should.be.true(); + inverse.calledOnce.should.be.false(); - readPostStub.calledOnce.should.be.true(); - readPostStub.firstCall.args[0].include.should.eql('next,next.author,next.tags'); - done(); - }).catch(function (err) { - console.log('err ', err); - done(err); - }); + console.log(fn.firstCall.args); + fn.firstCall.args.should.have.lengthOf(2); + fn.firstCall.args[0].should.have.properties('slug', 'title'); + fn.firstCall.args[1].should.be.an.Object().and.have.property('data'); + readPostStub.calledOnce.should.be.true(); + readPostStub.firstCall.args[0].include.should.eql('next,next.author,next.tags'); + done(); + }) + .catch(done); }); }); @@ -67,20 +72,27 @@ describe('{{next_post}} helper', function () { inverse = sinon.spy(), optionsData = {name: 'next_post', fn: fn, inverse: inverse}; - helpers.prev_post.call({ - html: 'content', - mobiledoc: markdownToMobiledoc('ff'), - title: 'post2', - slug: 'current', - created_at: new Date(0), - url: '/current/' - }, optionsData).then(function () { - fn.called.should.be.false(); - inverse.called.should.be.true(); - done(); - }).catch(function (err) { - done(err); - }); + helpers.prev_post + .call({ + html: 'content', + mobiledoc: markdownToMobiledoc('ff'), + title: 'post2', + slug: 'current', + created_at: new Date(0), + url: '/current/' + }, optionsData) + .then(function () { + fn.called.should.be.false(); + inverse.called.should.be.true(); + + console.log(inverse.firstCall.args); + inverse.firstCall.args.should.have.lengthOf(2); + inverse.firstCall.args[0].should.have.properties('slug', 'title'); + inverse.firstCall.args[1].should.be.an.Object().and.have.property('data'); + + done(); + }) + .catch(done); }); }); @@ -98,14 +110,15 @@ describe('{{next_post}} helper', function () { inverse = sinon.spy(), optionsData = {name: 'next_post', fn: fn, inverse: inverse}; - helpers.prev_post.call({}, optionsData).then(function () { - fn.called.should.be.false(); - inverse.called.should.be.true(); - readPostStub.called.should.be.false(); - done(); - }).catch(function (err) { - done(err); - }); + helpers.prev_post + .call({}, optionsData) + .then(function () { + fn.called.should.be.false(); + inverse.called.should.be.true(); + readPostStub.called.should.be.false(); + done(); + }) + .catch(done); }); }); @@ -125,22 +138,77 @@ describe('{{next_post}} helper', function () { inverse = sinon.spy(), optionsData = {name: 'next_post', fn: fn, inverse: inverse}; - helpers.prev_post.call({ - html: 'content', - status: 'published', - mobiledoc: markdownToMobiledoc('ff'), - title: 'post2', - slug: 'current', - created_at: new Date(0), - url: '/current/' - }, optionsData) + helpers.prev_post + .call({ + html: 'content', + status: 'draft', + mobiledoc: markdownToMobiledoc('ff'), + title: 'post2', + slug: 'current', + created_at: new Date(0), + url: '/current/' + }, optionsData) .then(function () { - fn.called.should.be.true(); - inverse.called.should.be.false(); + fn.called.should.be.false(); + inverse.called.should.be.true(); done(); - }).catch(function (err) { - done(err); + }) + .catch(done); + }); + }); + + describe('general error handling', function () { + beforeEach(function () { + readPostStub = sandbox.stub(api.posts, 'read', function () { + return Promise.reject(new errors.NotFoundError({message: 'Something wasn\'t found'})); }); }); + + it('should handle error from the API', function (done) { + var fn = sinon.spy(), + inverse = sinon.spy(), + optionsData = {name: 'next_post', fn: fn, inverse: inverse}; + + helpers.prev_post + .call({ + html: 'content', + status: 'published', + mobiledoc: markdownToMobiledoc('ff'), + title: 'post2', + slug: 'current', + created_at: new Date(0), + url: '/current/' + }, optionsData) + .then(function () { + fn.called.should.be.false(); + inverse.calledOnce.should.be.true(); + + inverse.firstCall.args[1].should.be.an.Object().and.have.property('data'); + inverse.firstCall.args[1].data.should.be.an.Object().and.have.property('error'); + inverse.firstCall.args[1].data.error.should.match(/^Something wasn't found/); + + done(); + }) + .catch(done); + }); + + it('should show warning for call without any options', function (done) { + var fn = sinon.spy(), + inverse = sinon.spy(), + optionsData = {name: 'next_post'}; + + helpers.prev_post + .call( + {}, + optionsData + ) + .then(function () { + fn.called.should.be.false(); + inverse.called.should.be.false(); + + done(); + }) + .catch(done); + }); }); });