From f6e1fb53253c30d420003fc9f75e6cfc68f43410 Mon Sep 17 00:00:00 2001 From: Peter Hudec Date: Thu, 12 Dec 2024 11:39:21 +0000 Subject: [PATCH] Set flashMessages once per request instead of calling getMessages() arbitrarily --- .../referrals/send-referral/controllers.js | 4 +- src/middleware/__test__/user-locals.test.js | 108 ++++-------------- src/middleware/react-global-props.js | 2 +- src/middleware/user-locals.js | 44 +++---- src/server.js | 2 +- src/templates/_macros/common/local-header.njk | 2 +- 6 files changed, 51 insertions(+), 111 deletions(-) diff --git a/src/apps/companies/apps/referrals/send-referral/controllers.js b/src/apps/companies/apps/referrals/send-referral/controllers.js index 41048158ccb..094385424c5 100644 --- a/src/apps/companies/apps/referrals/send-referral/controllers.js +++ b/src/apps/companies/apps/referrals/send-referral/controllers.js @@ -26,7 +26,9 @@ function renderSendReferralForm(req, res) { companyId: id, cancelUrl: urls.companies.detail(id), sendingAdviserTeamName, - flashMessages: res.locals.getMessages(), + + // TODO: This should not be necessary as flashMessages are included in global props + // flashMessages: res.locals.flashMessages, }, } ) diff --git a/src/middleware/__test__/user-locals.test.js b/src/middleware/__test__/user-locals.test.js index 278858b1cc3..ace496818ec 100644 --- a/src/middleware/__test__/user-locals.test.js +++ b/src/middleware/__test__/user-locals.test.js @@ -1,93 +1,29 @@ -const proxyquire = require('proxyquire') -const { faker } = require('@faker-js/faker') - -const words = faker.lorem.words -const modulePath = '../user-locals' - -describe('user-locals middleware', () => { - let req, res, next - beforeEach(() => { - res = { - locals: {}, +const { parseFlashMessages } = require('../user-locals') + +describe('parseFlashMessages', () => { + it('valid JSON in success:with-body', () => { + const BODY = { heading: 'foo bar', body: 'baz bing' } + const RAW_FLASH_MESSAGES = { + success: ['foo', '{"test":1}'], + error: ['bar', 'baz'], + 'success:with-body': [JSON.stringify(BODY)], } - next = sinon.spy() - }) - describe('#getMessages', () => { - afterEach(() => { - expect(next).to.have.been.called + expect(parseFlashMessages(RAW_FLASH_MESSAGES)).to.deep.equal({ + ...RAW_FLASH_MESSAGES, + 'success:with-body': [BODY], }) + }) - context('With no messages in the flash', () => { - it('returns the values', () => { - const userLocals = require(modulePath) - const flash = sinon.stub().returns({}) - req = { - path: '', - flash, - } - userLocals(req, res, next) - - const messages = res.locals.getMessages() - - expect(flash).to.have.been.calledWith() - expect(messages).to.deep.equal({}) - }) - }) - - context('With valid messages in the flash', () => { - it('returns the values', () => { - const userLocals = require(modulePath) - const flashData = { - success: [words(5), '{"test":1}'], - error: [words(5), words(5)], - 'success:with-body': [ - JSON.stringify({ heading: words(2), body: words(10) }), - ], - } - const flash = sinon.stub().returns(flashData) - req = { - path: '', - flash, - } - - userLocals(req, res, next) - - const messages = res.locals.getMessages() - - expect(messages).to.equal(flashData) - expect(typeof messages['success:with-body'][0]).to.equal('object') - expect(typeof messages.success[1]).to.equal('string') - expect(flash).to.have.been.calledWith() - }) - }) - - context('With invalid JSON messages in the flash', () => { - it('returns the string and reports the error', () => { - const captureException = sinon.spy() - const userLocals = proxyquire(modulePath, { - '../lib/reporter': { - captureException, - }, - }) - const flashData = { - 'success:with-body': [`heading: ${words(2)}`], - } - const flash = sinon.stub().returns(flashData) - req = { - path: '', - flash, - } - - userLocals(req, res, next) - - const messages = res.locals.getMessages() + it('invalid JSON in success:with-body', () => { + const RAW_FLASH_MESSAGES = { + success: ['foo', '{"test":1}'], + error: ['bar', 'baz'], + 'success:with-body': ["I'm no valid JSON"], + } - expect(messages).to.equal(flashData) - expect(typeof messages['success:with-body'][0]).to.equal('string') - expect(flash).to.have.been.calledWith() - expect(captureException).to.have.been.called - }) - }) + expect(parseFlashMessages(RAW_FLASH_MESSAGES)).to.deep.equal( + RAW_FLASH_MESSAGES + ) }) }) diff --git a/src/middleware/react-global-props.js b/src/middleware/react-global-props.js index a069c5bfd8a..c6737c35bdb 100644 --- a/src/middleware/react-global-props.js +++ b/src/middleware/react-global-props.js @@ -9,7 +9,7 @@ const transformModulePermissions = (modules) => module.exports = () => { return function reactGlobalProps(req, res, next) { res.locals.globalProps = { - flashMessages: res.locals.getMessages(), + flashMessages: res.locals.flashMessages, sentryDsn: config.sentryDsn, sentryEnvironment: config.sentryEnvironment, csrfToken: req.csrfToken(), diff --git a/src/middleware/user-locals.js b/src/middleware/user-locals.js index ec46d8ce1ed..f9ba51c4388 100644 --- a/src/middleware/user-locals.js +++ b/src/middleware/user-locals.js @@ -27,7 +27,15 @@ function convertValueToJson(value) { } } -module.exports = (req, res, next) => { +const parseFlashMessages = (rawFlashMessages) => + Object.fromEntries( + Object.entries(rawFlashMessages).map(([k, v]) => [ + k, + k.endsWith(':with-body') ? v.map(convertValueToJson) : v, + ]) + ) + +const userLocals = (req, res, next) => { const userPermissions = get(res, 'locals.user.permissions') const userProfile = config.oauth.bypassSSO ? null @@ -37,28 +45,22 @@ module.exports = (req, res, next) => { filterNonPermittedItem(userPermissions) ) - Object.assign(res.locals, { - PERMITTED_APPLICATIONS: config.oauth.bypassSSO - ? [{ key: 'datahub-crm' }] - : permittedApplications, - ALLOWED_APPS: permittedNavItems.reduce((apps, { headerKey }) => { - headerKey && apps.push(headerKey) - return apps - }, []), - ACTIVE_KEY: getActiveHeaderKey(req.path, permittedNavItems), - - getMessages() { - const items = req.flash() + res.locals.PERMITTED_APPLICATIONS = config.oauth.bypassSSO + ? [{ key: 'datahub-crm' }] + : permittedApplications - for (const [key, values] of Object.entries(items)) { - if (key.endsWith(':with-body')) { - items[key] = values.map(convertValueToJson) - } - } + res.locals.ALLOWED_APPS = permittedNavItems.reduce((apps, { headerKey }) => { + headerKey && apps.push(headerKey) + return apps + }, []) - return items - }, - }) + res.locals.ACTIVE_KEY = getActiveHeaderKey(req.path, permittedNavItems) + res.locals.flashMessages = parseFlashMessages(req.flash()) next() } + +module.exports = { + parseFlashMessages, + userLocals, +} diff --git a/src/server.js b/src/server.js index 4c161763695..0962944400c 100644 --- a/src/server.js +++ b/src/server.js @@ -20,7 +20,7 @@ const currentJourney = require('./modules/form/current-journey') const nunjucks = require('./config/nunjucks') const headers = require('./middleware/headers') const locals = require('./middleware/locals') -const userLocals = require('./middleware/user-locals') +const { userLocals } = require('./middleware/user-locals') const user = require('./middleware/user') const auth = require('./middleware/auth') const store = require('./middleware/store') diff --git a/src/templates/_macros/common/local-header.njk b/src/templates/_macros/common/local-header.njk index 15517e18401..437d64c621f 100644 --- a/src/templates/_macros/common/local-header.njk +++ b/src/templates/_macros/common/local-header.njk @@ -14,7 +14,7 @@ #} {% macro LocalHeader(props) %} {% set breadcrumbs = props.breadcrumbs | default(getBreadcrumbs()) %} - {% set messages = props.messages | default(getMessages() if getMessages) %} + {% set messages = props.messages | default(flashMessages) %} {% set modifier = props.modifier | concat('') | reverse | join(' c-local-header--') if props.modifier %} {% if props %}