From 8c7d305cd53602b73cbace1dc196974986f92f99 Mon Sep 17 00:00:00 2001 From: Hannah Wolfe Date: Wed, 16 Aug 2017 11:06:30 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Fixed=20channel=20context=20to?= =?UTF-8?q?=20be=20based=20on=20res.locals=20(#8910)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit closes #8907, closes #8908 - Add a bunch of tests to detect these breakages! - Then change all the places where req.channelConfig was still being used --- core/server/controllers/frontend/context.js | 4 +- core/test/functional/routes/channel_spec.js | 26 +++++ .../controllers/frontend/channels_spec.js | 104 +++++++++++++++--- .../unit/controllers/frontend/context_spec.js | 4 +- 4 files changed, 120 insertions(+), 18 deletions(-) diff --git a/core/server/controllers/frontend/context.js b/core/server/controllers/frontend/context.js index 0c0a3ec6ba..276cf3e939 100644 --- a/core/server/controllers/frontend/context.js +++ b/core/server/controllers/frontend/context.js @@ -53,8 +53,8 @@ function setResponseContext(req, res, data) { } // Each page can only have at most one of these - if (req.channelConfig) { - res.locals.context.push(req.channelConfig.name); + if (res.locals.channel) { + res.locals.context.push(res.locals.channel.name); } else if (privatePattern.test(res.locals.relativeUrl)) { res.locals.context.push('private'); } else if (subscribePattern.test(res.locals.relativeUrl) && labs.isSet('subscribers') === true) { diff --git a/core/test/functional/routes/channel_spec.js b/core/test/functional/routes/channel_spec.js index 6477e35715..9b1ada1089 100644 --- a/core/test/functional/routes/channel_spec.js +++ b/core/test/functional/routes/channel_spec.js @@ -251,6 +251,32 @@ describe('Channel Routes', function () { after(testUtils.teardown); + it('should return HTML for valid route', function (done) { + request.get('/tag/getting-started/') + .expect(200) + .expect('Content-Type', /html/) + .expect('Content-Type', /html/) + .expect('Cache-Control', testUtils.cacheRules.public) + .expect(200) + .end(function (err, res) { + if (err) { + return done(err); + } + + var $ = cheerio.load(res.text); + + should.not.exist(res.headers['x-cache-invalidate']); + should.not.exist(res.headers['X-CSRF-Token']); + should.not.exist(res.headers['set-cookie']); + should.exist(res.headers.date); + + // @TODO: use theme from fixtures and don't rely on content/themes/casper + $('body').attr('class').should.eql('tag-template tag-getting-started'); + + done(); + }); + }); + it('should 404 for /tag/ route', function (done) { request.get('/tag/') .expect('Cache-Control', testUtils.cacheRules.private) diff --git a/core/test/unit/controllers/frontend/channels_spec.js b/core/test/unit/controllers/frontend/channels_spec.js index 8b9ac26ce1..2d0684250c 100644 --- a/core/test/unit/controllers/frontend/channels_spec.js +++ b/core/test/unit/controllers/frontend/channels_spec.js @@ -30,13 +30,20 @@ describe('Channels', function () { function testChannelRender(props, assertions, done) { res = { redirect: sandbox.spy(), - locals: {} + locals: { + // Fake the ghost locals middleware, which doesn't happen when calling a channel router directly + relativeUrl: props.url + } }; - res.render = function (view) { - assertions(view); - res.redirect.called.should.be.false(); - done(); + res.render = function (view, data) { + try { + assertions.call(this, view, data); + res.redirect.called.should.be.false(); + done(); + } catch (err) { + done(err); + } }; _.extend(req, props); @@ -49,13 +56,20 @@ describe('Channels', function () { res = { render: sandbox.spy(), set: sandbox.spy(), - locals: {} + locals: { + // Fake the ghost locals middleware, which doesn't happen when calling a channel router directly + relativeUrl: props.url + } }; res.redirect = function (status, path) { - assertions(status, path); - res.render.called.should.be.false(); - done(); + try { + assertions.call(this, status, path); + res.render.called.should.be.false(); + done(); + } catch (err) { + done(err); + } }; _.extend(req, props); @@ -69,16 +83,23 @@ describe('Channels', function () { redirect: sandbox.spy(), render: sandbox.spy(), set: sandbox.spy(), - locals: {} + locals: { + // Fake the ghost locals middleware, which doesn't happen when calling a channel router directly + relativeUrl: props.url + } }; _.extend(req, props); channelRouter(req, res, function (empty) { - assertions(empty); - res.redirect.called.should.be.false(); - res.render.called.should.be.false(); - done(); + try { + assertions.call(this, empty); + res.redirect.called.should.be.false(); + res.render.called.should.be.false(); + done(); + } catch (err) { + done(err); + } }); } @@ -141,6 +162,11 @@ describe('Channels', function () { testChannelRender({url: '/'}, function (view) { should.exist(view); view.should.eql('index'); + + should.exist(this.locals); + this.locals.should.have.property('context').which.is.an.Array(); + this.locals.context.should.containEql('index'); + postAPIStub.calledOnce.should.be.true(); }, done); }); @@ -151,6 +177,11 @@ describe('Channels', function () { testChannelRender({url: '/'}, function (view) { should.exist(view); view.should.eql('home'); + + should.exist(this.locals); + this.locals.should.have.property('context').which.is.an.Array(); + this.locals.context.should.containEql('index'); + postAPIStub.calledOnce.should.be.true(); }, done); }); @@ -160,6 +191,11 @@ describe('Channels', function () { testChannelRender({url: '/page/2/'}, function (view) { should.exist(view); view.should.eql('index'); + + should.exist(this.locals); + this.locals.should.have.property('context').which.is.an.Array(); + this.locals.context.should.containEql('index'); + postAPIStub.calledOnce.should.be.true(); }, done); }); @@ -168,6 +204,11 @@ describe('Channels', function () { testChannelRender({url: '/page/2/'}, function (view) { should.exist(view); view.should.eql('index'); + + should.exist(this.locals); + this.locals.should.have.property('context').which.is.an.Array(); + this.locals.context.should.containEql('index'); + postAPIStub.calledOnce.should.be.true(); }, done); }); @@ -176,6 +217,11 @@ describe('Channels', function () { testChannelRender({url: '/page/3/'}, function (view) { should.exist(view); view.should.eql('index'); + + should.exist(this.locals); + this.locals.should.have.property('context').which.is.an.Array(); + this.locals.context.should.containEql('index'); + postAPIStub.calledOnce.should.be.true(); }, done); }); @@ -261,6 +307,11 @@ describe('Channels', function () { testChannelRender({url: '/tag/my-tag/'}, function (view) { should.exist(view); view.should.eql('index'); + + should.exist(this.locals); + this.locals.should.have.property('context').which.is.an.Array(); + this.locals.context.should.containEql('tag'); + postAPIStub.calledOnce.should.be.true(); tagAPIStub.calledOnce.should.be.true(); }, done); @@ -272,6 +323,11 @@ describe('Channels', function () { testChannelRender({url: '/tag/my-tag/'}, function (view) { should.exist(view); view.should.eql('tag'); + + should.exist(this.locals); + this.locals.should.have.property('context').which.is.an.Array(); + this.locals.context.should.containEql('tag'); + postAPIStub.calledOnce.should.be.true(); tagAPIStub.calledOnce.should.be.true(); }, done); @@ -284,6 +340,11 @@ describe('Channels', function () { testChannelRender({url: '/tag/my-tag/'}, function (view) { should.exist(view); view.should.eql('tag-my-tag'); + + should.exist(this.locals); + this.locals.should.have.property('context').which.is.an.Array(); + this.locals.context.should.containEql('tag'); + postAPIStub.calledOnce.should.be.true(); tagAPIStub.calledOnce.should.be.true(); }, done); @@ -304,6 +365,11 @@ describe('Channels', function () { testChannelRender({url: '/tag/my-tag/page/2/'}, function (view) { should.exist(view); view.should.eql('tag'); + + should.exist(this.locals); + this.locals.should.have.property('context').which.is.an.Array(); + this.locals.context.should.containEql('tag'); + postAPIStub.calledOnce.should.be.true(); }, done); }); @@ -315,6 +381,11 @@ describe('Channels', function () { testChannelRender({url: '/tag/my-tag/page/2/'}, function (view) { should.exist(view); view.should.eql('tag-my-tag'); + + should.exist(this.locals); + this.locals.should.have.property('context').which.is.an.Array(); + this.locals.context.should.containEql('tag'); + postAPIStub.calledOnce.should.be.true(); }, done); }); @@ -331,6 +402,11 @@ describe('Channels', function () { testChannelRender({url: '/tag/my-tag/page/3/'}, function (view) { should.exist(view); view.should.eql('index'); + + should.exist(this.locals); + this.locals.should.have.property('context').which.is.an.Array(); + this.locals.context.should.containEql('tag'); + postAPIStub.calledOnce.should.be.true(); }, done); }); diff --git a/core/test/unit/controllers/frontend/context_spec.js b/core/test/unit/controllers/frontend/context_spec.js index d6ef7b5eb4..fc4ae5951c 100644 --- a/core/test/unit/controllers/frontend/context_spec.js +++ b/core/test/unit/controllers/frontend/context_spec.js @@ -38,11 +38,11 @@ describe('Contexts', function () { res.locals.relativeUrl = url; if (channel && _.isString(channel)) { - req.channelConfig = channelConfig.get(channel); + res.locals.channel = channelConfig.get(channel); } else if (channel && _.isNumber(channel)) { pageParam = channel; } else if (channel) { - req.channelConfig = channel; + res.locals.channel = channel; } if (pageParam) {