From b1478599a8f8ceec1794cd98fc53bd72a7590349 Mon Sep 17 00:00:00 2001 From: Stephen Daly Date: Thu, 14 Jan 2021 17:53:08 +0000 Subject: [PATCH] PP-7583 Remove unnecessary getEmailNotifications middleware This middleware made an unnecessary call to connector to get the account when this has already been retrieved in previously run middleware. It then duplicated properties already on the account object with new names. Ideally we should be using a model to restructure the data in an appropriate way when we retrieve the account from connector, but for now just access this account data from the original place it lives on the get account response in controllers that use it. --- .../email-notifications.controller.js | 28 ++-- .../settings/get.settings.controller.js | 6 +- app/middleware/get-email-notification.js | 15 --- app/routes.js | 33 +++-- app/services/email.service.js | 18 --- .../middleware/get-email-notification.test.js | 64 --------- test/unit/services/email.service.test.js | 125 ------------------ 7 files changed, 33 insertions(+), 256 deletions(-) delete mode 100644 app/middleware/get-email-notification.js delete mode 100644 test/unit/middleware/get-email-notification.test.js delete mode 100644 test/unit/services/email.service.test.js diff --git a/app/controllers/email-notifications/email-notifications.controller.js b/app/controllers/email-notifications/email-notifications.controller.js index 52fd8ce7b6..4e6a1fa3d4 100644 --- a/app/controllers/email-notifications/email-notifications.controller.js +++ b/app/controllers/email-notifications/email-notifications.controller.js @@ -48,7 +48,7 @@ module.exports.collectionEmailIndex = (req, res) => { optional: 'OPTIONAL', no: 'OFF' }, - emailCollectionMode: req.account.emailCollectionMode + emailCollectionMode: req.account.email_collection_mode }) } @@ -65,8 +65,8 @@ module.exports.collectionEmailUpdate = (req, res) => { module.exports.confirmationEmailIndex = (req, res) => { showConfirmationEmail(req, res, 'confirmation-email-toggle', { - confirmationEnabled: req.account.emailEnabled, - emailCollectionMode: req.account.emailCollectionMode + confirmationEnabled: req.account.email_notifications.PAYMENT_CONFIRMED.enabled, + emailCollectionMode: req.account.email_collection_mode }) } @@ -89,8 +89,8 @@ module.exports.confirmationEmailOff = (req, res) => { module.exports.refundEmailIndex = (req, res) => { showRefundEmail(req, res, 'refund-email-toggle', { - refundEmailEnabled: req.account.refundEmailEnabled, - emailCollectionMode: req.account.emailCollectionMode + refundEmailEnabled: req.account.email_notifications.REFUND_ISSUED && req.account.email_notifications.REFUND_ISSUED.enabled, + emailCollectionMode: req.account.email_collection_mode }) } @@ -108,28 +108,28 @@ module.exports.refundEmailUpdate = (req, res) => { module.exports.index = (req, res) => { showEmail(req, res, 'index', { confirmationTabActive: true, - customEmailText: req.account.customEmailText, + customEmailText: req.account.email_notifications.PAYMENT_CONFIRMED.template_body, serviceName: req.account.service_name, - emailEnabled: req.account.emailEnabled, - emailCollectionMode: req.account.emailCollectionMode, - refundEmailEnabled: req.account.refundEmailEnabled + emailEnabled: req.account.email_notifications.PAYMENT_CONFIRMED.enabled, + emailCollectionMode: req.account.email_collection_mode, + refundEmailEnabled: req.account.email_notifications.REFUND_ISSUED && req.account.email_notifications.REFUND_ISSUED.enabled }) } module.exports.indexRefundTabEnabled = (req, res) => { showEmail(req, res, 'index', { confirmationTabActive: false, - customEmailText: req.account.customEmailText, + customEmailText: req.account.email_notifications.PAYMENT_CONFIRMED.template_body, serviceName: req.account.service_name, - emailEnabled: req.account.emailEnabled, - emailCollectionMode: req.account.emailCollectionMode, - refundEmailEnabled: req.account.refundEmailEnabled + emailEnabled: req.account.email_notifications.PAYMENT_CONFIRMED.enabled, + emailCollectionMode: req.account.email_collection_mode, + refundEmailEnabled: req.account.email_notifications.REFUND_ISSUED && req.account.email_notifications.REFUND_ISSUED.enabled }) } module.exports.edit = (req, res) => { showEmail(req, res, 'edit', { - customEmailText: req.account.customEmailText, + customEmailText: req.account.email_notifications.PAYMENT_CONFIRMED.template_body, serviceName: req.account.service_name }) } diff --git a/app/controllers/settings/get.settings.controller.js b/app/controllers/settings/get.settings.controller.js index 33b17571c0..a300330b05 100644 --- a/app/controllers/settings/get.settings.controller.js +++ b/app/controllers/settings/get.settings.controller.js @@ -14,9 +14,9 @@ module.exports = (req, res) => { supports3ds: req.account.supports3ds, requires3ds: req.account.requires3ds, collectBillingAddress: req.service.collectBillingAddress, - emailCollectionMode: humaniseEmailMode(req.account.emailCollectionMode), - confirmationEmailEnabled: req.account.emailEnabled, - refundEmailEnabled: req.account.refundEmailEnabled, + emailCollectionMode: humaniseEmailMode(req.account.email_collection_mode), + confirmationEmailEnabled: req.account.email_notifications.PAYMENT_CONFIRMED.enabled, + refundEmailEnabled: req.account.email_notifications.REFUND_ISSUED && req.account.email_notifications.REFUND_ISSUED.enabled, allowMoto: req.account.allow_moto, motoMaskCardNumberInputEnabled: req.account.moto_mask_card_number_input, motoMaskSecurityCodeInputEnabled: req.account.moto_mask_card_security_code_input diff --git a/app/middleware/get-email-notification.js b/app/middleware/get-email-notification.js deleted file mode 100644 index 9620046fce..0000000000 --- a/app/middleware/get-email-notification.js +++ /dev/null @@ -1,15 +0,0 @@ -'use strict' -const { renderErrorView } = require('../utils/response.js') -const { getEmailSettings } = require('../services/email.service.js') -const _ = require('lodash') -const CORRELATION_HEADER = require('../utils/correlation-header.js').CORRELATION_HEADER - -module.exports = function (req, res, next) { - const correlationId = req.headers[CORRELATION_HEADER] - return getEmailSettings(req.account.gateway_account_id, correlationId) - .then(emailData => { - req.account = _.merge(req.account, emailData) - next() - }) - .catch(() => renderErrorView(req, res)) -} diff --git a/app/routes.js b/app/routes.js index f6ea04b342..4a8291823e 100644 --- a/app/routes.js +++ b/app/routes.js @@ -14,7 +14,6 @@ const getServiceAndAccount = require('./middleware/get-service-and-gateway-accou // Middleware const { lockOutDisabledUsers, enforceUserAuthenticated, enforceUserFirstFactor, redirectLoggedInUser } = require('./services/auth.service') const { validateAndRefreshCsrf, ensureSessionHasCsrfSecret } = require('./middleware/csrf') -const getEmailNotification = require('./middleware/get-email-notification') const getAccount = require('./middleware/get-gateway-account') const hasServices = require('./middleware/has-services') const resolveService = require('./middleware/resolve-service') @@ -92,14 +91,14 @@ const { requestToGoLive, policyPages, stripeSetup, stripe, settings, yourPsp, allServiceTransactions, payouts } = paths -const { +const { digitalWallet, emailNotifications, paymentTypes, toggle3ds, toggleBillingAddress, toggleMotoMaskCardNumberAndSecurityCode - } = paths.account +} = paths.account // Exports module.exports.generateRoute = generateRoute @@ -200,7 +199,7 @@ module.exports.bind = function (app) { app.use(authenticatedPaths, enforceUserAuthenticated, validateAndRefreshCsrf) // Enforce authentication on all get requests app.use(authenticatedPaths.filter(item => !lodash.values(serviceSwitcher).includes(item)), hasServices) // Require services everywhere but the switcher page - app.get(settings.index, permission('transactions-details:read'), getAccount, getEmailNotification, settingsController.index) + app.get(settings.index, permission('transactions-details:read'), getAccount, settingsController.index) // TRANSACTIONS app.get(transactions.index, permission('transactions:read'), getAccount, paymentMethodIsCard, transactionsListController) @@ -253,19 +252,19 @@ module.exports.bind = function (app) { account.post(digitalWallet.googlePay, permission('payment-types:update'), paymentMethodIsCard, digitalWalletController.postGooglePay) // EMAIL - account.get(emailNotifications.index, permission('email-notification-template:read'), getEmailNotification, paymentMethodIsCard, emailNotificationsController.index) - account.get(emailNotifications.indexRefundTabEnabled, permission('email-notification-template:read'), getEmailNotification, paymentMethodIsCard, emailNotificationsController.indexRefundTabEnabled) - account.get(emailNotifications.edit, permission('email-notification-paragraph:update'), getEmailNotification, paymentMethodIsCard, emailNotificationsController.edit) - account.post(emailNotifications.confirm, permission('email-notification-paragraph:update'), getEmailNotification, paymentMethodIsCard, emailNotificationsController.confirm) - account.post(emailNotifications.update, permission('email-notification-paragraph:update'), getEmailNotification, paymentMethodIsCard, emailNotificationsController.update) - account.get(emailNotifications.collection, permission('email-notification-template:read'), getEmailNotification, paymentMethodIsCard, emailNotificationsController.collectionEmailIndex) - account.post(emailNotifications.collection, permission('email-notification-toggle:update'), getEmailNotification, paymentMethodIsCard, emailNotificationsController.collectionEmailUpdate) - account.get(emailNotifications.confirmation, permission('email-notification-template:read'), getEmailNotification, paymentMethodIsCard, emailNotificationsController.confirmationEmailIndex) - account.post(emailNotifications.confirmation, permission('email-notification-toggle:update'), getEmailNotification, paymentMethodIsCard, emailNotificationsController.confirmationEmailUpdate) - account.post(emailNotifications.off, permission('email-notification-toggle:update'), getEmailNotification, paymentMethodIsCard, emailNotificationsController.confirmationEmailOff) - account.post(emailNotifications.on, permission('email-notification-toggle:update'), getEmailNotification, paymentMethodIsCard, emailNotificationsController.confirmationEmailOn) - account.get(emailNotifications.refund, permission('email-notification-template:read'), getEmailNotification, paymentMethodIsCard, emailNotificationsController.refundEmailIndex) - account.post(emailNotifications.refund, permission('email-notification-toggle:update'), getEmailNotification, paymentMethodIsCard, emailNotificationsController.refundEmailUpdate) + account.get(emailNotifications.index, permission('email-notification-template:read'), paymentMethodIsCard, emailNotificationsController.index) + account.get(emailNotifications.indexRefundTabEnabled, permission('email-notification-template:read'), paymentMethodIsCard, emailNotificationsController.indexRefundTabEnabled) + account.get(emailNotifications.edit, permission('email-notification-paragraph:update'), paymentMethodIsCard, emailNotificationsController.edit) + account.post(emailNotifications.confirm, permission('email-notification-paragraph:update'), paymentMethodIsCard, emailNotificationsController.confirm) + account.post(emailNotifications.update, permission('email-notification-paragraph:update'), paymentMethodIsCard, emailNotificationsController.update) + account.get(emailNotifications.collection, permission('email-notification-template:read'), paymentMethodIsCard, emailNotificationsController.collectionEmailIndex) + account.post(emailNotifications.collection, permission('email-notification-toggle:update'), paymentMethodIsCard, emailNotificationsController.collectionEmailUpdate) + account.get(emailNotifications.confirmation, permission('email-notification-template:read'), paymentMethodIsCard, emailNotificationsController.confirmationEmailIndex) + account.post(emailNotifications.confirmation, permission('email-notification-toggle:update'), paymentMethodIsCard, emailNotificationsController.confirmationEmailUpdate) + account.post(emailNotifications.off, permission('email-notification-toggle:update'), paymentMethodIsCard, emailNotificationsController.confirmationEmailOff) + account.post(emailNotifications.on, permission('email-notification-toggle:update'), paymentMethodIsCard, emailNotificationsController.confirmationEmailOn) + account.get(emailNotifications.refund, permission('email-notification-template:read'), paymentMethodIsCard, emailNotificationsController.refundEmailIndex) + account.post(emailNotifications.refund, permission('email-notification-toggle:update'), paymentMethodIsCard, emailNotificationsController.refundEmailUpdate) // SERVICE SWITCHER app.get(serviceSwitcher.index, myServicesController.getIndex) diff --git a/app/services/email.service.js b/app/services/email.service.js index badf132e3f..7d7c19827d 100644 --- a/app/services/email.service.js +++ b/app/services/email.service.js @@ -5,23 +5,6 @@ const ConnectorClient = require('./clients/connector.client.js').ConnectorClient const connectorClient = new ConnectorClient(process.env.CONNECTOR_URL) -const getEmailSettings = async function (accountID, correlationId) { - try { - const data = await connectorClient.getAccount({ - gatewayAccountId: accountID, - correlationId: correlationId - }) - return { - customEmailText: data.email_notifications.PAYMENT_CONFIRMED.template_body, - emailEnabled: data.email_notifications.PAYMENT_CONFIRMED.enabled, - emailCollectionMode: data.email_collection_mode, - refundEmailEnabled: data.email_notifications.REFUND_ISSUED && data.email_notifications.REFUND_ISSUED.enabled - } - } catch (err) { - clientFailure(err, 'GET', false) - } -} - const updateConfirmationTemplate = async function (accountID, emailText, correlationId) { try { const patch = { 'op': 'replace', 'path': '/payment_confirmed/template_body', 'value': emailText } @@ -89,7 +72,6 @@ const clientFailure = function (err, methodType, isPatchEndpoint) { } module.exports = { - getEmailSettings, updateConfirmationTemplate, setEmailCollectionMode, setConfirmationEnabled, diff --git a/test/unit/middleware/get-email-notification.test.js b/test/unit/middleware/get-email-notification.test.js deleted file mode 100644 index 8f7da595a0..0000000000 --- a/test/unit/middleware/get-email-notification.test.js +++ /dev/null @@ -1,64 +0,0 @@ -'use strict' - -const path = require('path') -const assert = require('assert') -const sinon = require('sinon') -const chai = require('chai') -const { expect } = chai -const chaiAsPromised = require('chai-as-promised') -const proxyquire = require('proxyquire') - -chai.use(chaiAsPromised) - -describe('retrieve email notification template', function () { - const response = { - status: () => { - }, - render: () => { - }, - setHeader: () => { - } - } - let render - let next - - beforeEach(function () { - render = sinon.stub(response, 'render') - next = sinon.spy() - }) - - afterEach(function () { - render.restore() - }) - - it('should call the error view if connector call fails', function (done) { - const retrieveEmailNotification = require(path.join(__dirname, '/../../../app/middleware/get-email-notification.js')) - const req = { account: { gateway_account_id: 1 }, headers: {} } - retrieveEmailNotification(req, response, next) - setTimeout(function () { - expect(next.notCalled).to.be.true // eslint-disable-line - assert(render.calledWith('error', { message: 'There is a problem with the payments platform' })) - done() - }, 100) - }) - - it('should merge account with email notification template data and call next on success', function (done) { - const emailStub = { - getEmailSettings: function () { - return Promise.resolve({ customEmailText: 'hello', emailEnabled: true }) - } - } - const retrieveEmailNotification = proxyquire(path.join(__dirname, '/../../../app/middleware/get-email-notification.js'), { - '../services/email.service.js': emailStub - }) - const req = { account: { gateway_account_id: 1 }, headers: {} } - retrieveEmailNotification(req, response, next).should.be.fulfilled.then(function () { - expect(req.account).to.deep.equal({ - customEmailText: 'hello', - 'gateway_account_id': 1, - 'emailEnabled': true - }) - expect(next.called).to.be.true // eslint-disable-line - }).should.notify(done) - }) -}) diff --git a/test/unit/services/email.service.test.js b/test/unit/services/email.service.test.js deleted file mode 100644 index ed1e058007..0000000000 --- a/test/unit/services/email.service.test.js +++ /dev/null @@ -1,125 +0,0 @@ -'use strict' - -const path = require('path') -const expect = require('chai').expect -const _ = require('lodash') -const proxyquire = require('proxyquire') - -function getEmailService (connectorClientStub) { - return proxyquire(path.join(__dirname, '/../../../app/services/email.service.js'), { - '../services/clients/connector.client.js': connectorClientStub - }) -} - -const connectorClientSuccessStub = { - ConnectorClient: function () { - return { - updateConfirmationEmail: function () { - return Promise.resolve() - }, - updateConfirmationEmailEnabled: function () { - return Promise.resolve() - }, - getAccount: function () { - return Promise.resolve( - { - gateway_account_id: 31, - service_name: '8b9370c1a83c4d71a538a1691236acc2', - type: 'test', - analytics_id: '8b02c7e542e74423aa9e6d0f0628fd58', - email_collection_mode: 'MANDATORY', - email_notifications: { - PAYMENT_CONFIRMED: { - version: 1, - enabled: true, - template_body: 'template here' - }, - REFUND_ISSUED: { - version: 1, - enabled: true - } - } - }) - } - } - } -} - -const connectorClientErrorsStub = { - ConnectorClient: function () { - return { - updateConfirmationEmail: function () { - return Promise.reject(new Error('connection error')) - }, - updateConfirmationEmailEnabled: function () { - return Promise.reject(new Error('connection error')) - }, - getAccount: function () { - return Promise.reject(new Error('connection error')) - } - } - } -} - -describe('email notification', function () { - describe('getting the template body', function () { - describe('when connector returns an error', function () { - it('should throw error', function () { - const emailService = getEmailService(connectorClientErrorsStub) - return expect(emailService.getEmailSettings(123, 'some-unique-id')) - .to.be.rejectedWith('Calling connector to get/patch account data threw exception') - }) - }) - - describe('when connector returns correctly', function () { - it('should return the correct promise', function () { - const emailService = getEmailService(connectorClientSuccessStub) - return expect(emailService.getEmailSettings(123, 'some-unique-id')) - .to.be.fulfilled.then(function (response) { - expect(response).to.deep.equal({ - customEmailText: 'template here', - emailEnabled: true, - emailCollectionMode: 'MANDATORY', - refundEmailEnabled: true - }) - }) - }) - }) - }) - - describe('updating the email notification template body', function () { - describe('when connector is unavailable', function () { - it('should throw error', function () { - const emailService = getEmailService(connectorClientErrorsStub) - return expect(emailService.updateConfirmationTemplate(123, 'some-unique-id')) - .to.be.rejectedWith('Calling connector to update email notifications for an account threw exception') - }) - }) - - describe('when connector returns correctly', function () { - it('should update the email notification template body', function () { - const emailService = getEmailService(connectorClientSuccessStub) - return expect(emailService.updateConfirmationTemplate(123, 'some-unique-id')).to.be.fulfilled - }) - }) - }) - - describe('enabling/disabling email notifications', function () { - _.each([true, false], function (toggle) { - describe('when connector is unavailable', function () { - it('should throw error', function () { - const emailService = getEmailService(connectorClientErrorsStub) - return expect(emailService.setConfirmationEnabled(123, toggle, 'some-unique-id')) - .to.be.rejectedWith('Calling connector to update email notifications for an account threw exception') - }) - }) - - describe('when connector returns correctly', function () { - it('should disable email notifications', function () { - const emailService = getEmailService(connectorClientSuccessStub) - return expect(emailService.setConfirmationEnabled(123, true, 'some-unique-id')).to.be.fulfilled - }) - }) - }) - }) -})