diff --git a/jest.config.js b/jest.config.js index 9e0ce3ef05..3e2e02efb7 100644 --- a/jest.config.js +++ b/jest.config.js @@ -9,7 +9,7 @@ module.exports = { // Needed to use @shelf/jest-mongodb preset. ...tsJestPreset.transform, }, - collectCoverageFrom: ['./src/**/*.{ts,js}'], + collectCoverageFrom: ['./src/**/*.{ts,js}', '!**/__tests__/**'], coveragePathIgnorePatterns: ['./node_modules/', './tests'], coverageThreshold: { global: { diff --git a/src/app/constants/mail.ts b/src/app/constants/mail.ts index e10a0a8f6b..8d5c9f01ff 100644 --- a/src/app/constants/mail.ts +++ b/src/app/constants/mail.ts @@ -14,10 +14,10 @@ export const EMAIL_HEADERS = { } // Types of emails we send -export const EMAIL_TYPES = { - adminResponse: 'Admin (response)', - loginOtp: 'Login OTP', - verificationOtp: 'Verification OTP', - emailConfirmation: 'Email confirmation', - adminBounce: 'Admin (bounce notification)', +export enum EmailType { + AdminResponse = 'Admin (response)', + LoginOtp = 'Login OTP', + VerificationOtp = 'Verification OTP', + EmailConfirmation = 'Email confirmation', + AdminBounce = 'Admin (bounce notification)', } diff --git a/tests/unit/backend/helpers/bounce.ts b/src/app/modules/bounce/__tests__/bounce-test-helpers.ts similarity index 83% rename from tests/unit/backend/helpers/bounce.ts rename to src/app/modules/bounce/__tests__/bounce-test-helpers.ts index 4b69b3a71a..2ea9aba172 100644 --- a/tests/unit/backend/helpers/bounce.ts +++ b/src/app/modules/bounce/__tests__/bounce-test-helpers.ts @@ -26,6 +26,7 @@ const makeEmailNotification = ( formId: ObjectId, submissionId: ObjectId, recipientList: string[], + emailType: 'Admin (response)' | 'Email confirmation', ): IEmailNotification => { return { notificationType, @@ -43,7 +44,7 @@ const makeEmailNotification = ( }, { name: 'X-Formsg-Email-Type', - value: 'Admin (response)', + value: emailType, }, ], commonHeaders: { @@ -61,9 +62,16 @@ export const makeBounceNotification = ( recipientList: string[] = [], bouncedList: string[] = [], bounceType: 'Transient' | 'Permanent' = 'Permanent', + emailType: 'Admin (response)' | 'Email confirmation' = 'Admin (response)', ): ISnsNotification => { const Message = merge( - makeEmailNotification('Bounce', formId, submissionId, recipientList), + makeEmailNotification( + 'Bounce', + formId, + submissionId, + recipientList, + emailType, + ), { bounce: { bounceType, @@ -83,9 +91,16 @@ export const makeDeliveryNotification = ( submissionId: ObjectId = new ObjectId(), recipientList: string[] = [], deliveredList: string[] = [], + emailType: 'Admin (response)' | 'Email confirmation' = 'Admin (response)', ): ISnsNotification => { const Message = merge( - makeEmailNotification('Delivery', formId, submissionId, recipientList), + makeEmailNotification( + 'Delivery', + formId, + submissionId, + recipientList, + emailType, + ), { delivery: { recipients: deliveredList, diff --git a/tests/unit/backend/modules/bounce/bounce.controller.spec.ts b/src/app/modules/bounce/__tests__/bounce.controller.spec.ts similarity index 100% rename from tests/unit/backend/modules/bounce/bounce.controller.spec.ts rename to src/app/modules/bounce/__tests__/bounce.controller.spec.ts diff --git a/tests/unit/backend/models/bounce.server.model.spec.ts b/src/app/modules/bounce/__tests__/bounce.model.spec.ts similarity index 97% rename from tests/unit/backend/models/bounce.server.model.spec.ts rename to src/app/modules/bounce/__tests__/bounce.model.spec.ts index 25a81a6f91..0475c12b0d 100644 --- a/tests/unit/backend/models/bounce.server.model.spec.ts +++ b/src/app/modules/bounce/__tests__/bounce.model.spec.ts @@ -1,15 +1,15 @@ import { ObjectId } from 'bson' import { omit, pick } from 'lodash' import mongoose from 'mongoose' +import dbHandler from 'tests/unit/backend/helpers/jest-db' -import getBounceModel from 'src/app/models/bounce.server.model' +import getBounceModel from 'src/app/modules/bounce/bounce.model' import { extractBounceObject, makeBounceNotification, makeDeliveryNotification, -} from '../helpers/bounce' -import dbHandler from '../helpers/jest-db' +} from './bounce-test-helpers' const Bounce = getBounceModel(mongoose) diff --git a/tests/unit/backend/modules/bounce/bounce.service.spec.ts b/src/app/modules/bounce/__tests__/bounce.service.spec.ts similarity index 73% rename from tests/unit/backend/modules/bounce/bounce.service.spec.ts rename to src/app/modules/bounce/__tests__/bounce.service.spec.ts index b6806163f8..90df58a7a2 100644 --- a/tests/unit/backend/modules/bounce/bounce.service.spec.ts +++ b/src/app/modules/bounce/__tests__/bounce.service.spec.ts @@ -4,31 +4,34 @@ import crypto from 'crypto' import dedent from 'dedent' import { cloneDeep, omit } from 'lodash' import mongoose from 'mongoose' +import dbHandler from 'tests/unit/backend/helpers/jest-db' +import getMockLogger, { + resetMockLogger, +} from 'tests/unit/backend/helpers/jest-logger' import { mocked } from 'ts-jest/utils' import * as LoggerModule from 'src/config/logger' -import { ISnsNotification } from 'src/types' +import { IBounceNotification, ISnsNotification } from 'src/types' import { extractBounceObject, makeBounceNotification, makeDeliveryNotification, MOCK_SNS_BODY, -} from '../../helpers/bounce' -import dbHandler from '../../helpers/jest-db' -import getMockLogger, { resetMockLogger } from '../../helpers/jest-logger' +} from './bounce-test-helpers' jest.mock('axios') const mockAxios = mocked(axios, true) jest.mock('src/config/logger') const MockLoggerModule = mocked(LoggerModule, true) +const mockShortTermLogger = getMockLogger() const mockLogger = getMockLogger() -MockLoggerModule.createCloudWatchLogger.mockReturnValue(mockLogger) -MockLoggerModule.createLoggerWithLabel.mockReturnValue(getMockLogger()) +MockLoggerModule.createCloudWatchLogger.mockReturnValue(mockShortTermLogger) +MockLoggerModule.createLoggerWithLabel.mockReturnValue(mockLogger) // Import modules which depend on config last so that mocks get imported correctly // eslint-disable-next-line import/first -import getBounceModel from 'src/app/models/bounce.server.model' +import getBounceModel from 'src/app/modules/bounce/bounce.model' // eslint-disable-next-line import/first import { isValidSnsRequest, @@ -111,11 +114,17 @@ describe('updateBounces', () => { 'email3@example.com', ] - beforeAll(async () => await dbHandler.connect()) + beforeAll(async () => { + await dbHandler.connect() + // Avoid being affected by other modules + resetMockLogger(mockLogger) + resetMockLogger(mockShortTermLogger) + }) afterEach(async () => { await dbHandler.clearDatabase() resetMockLogger(mockLogger) + resetMockLogger(mockShortTermLogger) }) afterAll(async () => await dbHandler.closeDatabase()) @@ -136,9 +145,11 @@ describe('updateBounces', () => { email, hasBounced: false, })) - expect(mockLogger.info).toHaveBeenCalledWith( - JSON.parse(notification.Message), - ) + expect(mockLogger.info.mock.calls[0][0]).toMatchObject({ + meta: { + ...JSON.parse(notification.Message), + }, + }) expect(mockLogger.warn).not.toHaveBeenCalled() expect(omit(actualBounce, 'expireAt')).toEqual({ formId, @@ -169,9 +180,11 @@ describe('updateBounces', () => { email, hasBounced: bounces[email], })) - expect(mockLogger.info).toHaveBeenCalledWith( - JSON.parse(notification.Message), - ) + expect(mockLogger.info.mock.calls[0][0]).toMatchObject({ + meta: { + ...JSON.parse(notification.Message), + }, + }) expect(mockLogger.warn).not.toHaveBeenCalled() expect(omit(actualBounce, 'expireAt')).toEqual({ formId, @@ -190,6 +203,9 @@ describe('updateBounces', () => { recipientList, recipientList, ) + const parsedNotification: IBounceNotification = JSON.parse( + notification.Message, + ) await updateBounces(notification) const actualBounceDoc = await Bounce.findOne({ formId }) const actualBounce = extractBounceObject(actualBounceDoc) @@ -197,11 +213,20 @@ describe('updateBounces', () => { email, hasBounced: true, })) - expect(mockLogger.info).toHaveBeenCalledWith( - JSON.parse(notification.Message), - ) + expect(mockLogger.info.mock.calls[0][0]).toMatchObject({ + meta: { + ...parsedNotification, + }, + }) expect(mockLogger.warn.mock.calls[0][0]).toMatchObject({ - type: 'CRITICAL BOUNCE', + message: 'Critical bounce', + meta: { + action: 'updateBounces', + hasAlarmed: false, + formId: formId.toHexString(), + submissionId: submissionId.toHexString(), + bounceInfo: parsedNotification.bounce, + }, }) expect(omit(actualBounce, 'expireAt')).toEqual({ formId, @@ -236,12 +261,16 @@ describe('updateBounces', () => { })) // There should only be one document after 2 notifications expect(actualBounceCursor.length).toBe(1) - expect(mockLogger.info.mock.calls[0][0]).toEqual( - JSON.parse(notification1.Message), - ) - expect(mockLogger.info.mock.calls[1][0]).toEqual( - JSON.parse(notification2.Message), - ) + expect(mockLogger.info.mock.calls[0][0]).toMatchObject({ + meta: { + ...JSON.parse(notification1.Message), + }, + }) + expect(mockLogger.info.mock.calls[1][0]).toMatchObject({ + meta: { + ...JSON.parse(notification2.Message), + }, + }) expect(mockLogger.warn).not.toHaveBeenCalled() expect(omit(actualBounce, 'expireAt')).toEqual({ formId, @@ -281,12 +310,16 @@ describe('updateBounces', () => { })) // There should only be one document after 2 notifications expect(actualBounceCursor.length).toBe(1) - expect(mockLogger.info.mock.calls[0][0]).toEqual( - JSON.parse(notification1.Message), - ) - expect(mockLogger.info.mock.calls[1][0]).toEqual( - JSON.parse(notification2.Message), - ) + expect(mockLogger.info.mock.calls[0][0]).toMatchObject({ + meta: { + ...JSON.parse(notification1.Message), + }, + }) + expect(mockLogger.info.mock.calls[1][0]).toMatchObject({ + meta: { + ...JSON.parse(notification2.Message), + }, + }) expect(mockLogger.warn).not.toHaveBeenCalled() expect(omit(actualBounce, 'expireAt')).toEqual({ formId, @@ -298,19 +331,26 @@ describe('updateBounces', () => { it('should save correctly when there are consecutive critical bounce notifications', async () => { const formId = new ObjectId() - const submissionId = new ObjectId() + const submissionId1 = new ObjectId() + const submissionId2 = new ObjectId() const notification1 = makeBounceNotification( formId, - submissionId, + submissionId1, recipientList, recipientList.slice(0, 1), // First email bounced ) + const parsedNotification1: IBounceNotification = JSON.parse( + notification1.Message, + ) const notification2 = makeBounceNotification( formId, - submissionId, + submissionId2, recipientList, recipientList.slice(1, 3), // Second and third email bounced ) + const parsedNotification2: IBounceNotification = JSON.parse( + notification2.Message, + ) await updateBounces(notification1) await updateBounces(notification2) const actualBounceCursor = await Bounce.find({ formId }) @@ -321,14 +361,25 @@ describe('updateBounces', () => { })) // There should only be one document after 2 notifications expect(actualBounceCursor.length).toBe(1) - expect(mockLogger.info.mock.calls[0][0]).toEqual( - JSON.parse(notification1.Message), - ) - expect(mockLogger.info.mock.calls[1][0]).toEqual( - JSON.parse(notification2.Message), - ) + expect(mockLogger.info.mock.calls[0][0]).toMatchObject({ + meta: { + ...parsedNotification1, + }, + }) + expect(mockLogger.info.mock.calls[1][0]).toMatchObject({ + meta: { + ...parsedNotification2, + }, + }) expect(mockLogger.warn.mock.calls[0][0]).toMatchObject({ - type: 'CRITICAL BOUNCE', + message: 'Critical bounce', + meta: { + action: 'updateBounces', + hasAlarmed: false, + formId: formId.toHexString(), + submissionId: submissionId2.toHexString(), + bounceInfo: parsedNotification2.bounce, + }, }) expect(omit(actualBounce, 'expireAt')).toEqual({ formId, @@ -368,12 +419,12 @@ describe('updateBounces', () => { })) // There should only be one document after 2 notifications expect(actualBounceCursor.length).toBe(1) - expect(mockLogger.info.mock.calls[0][0]).toEqual( - JSON.parse(notification1.Message), - ) - expect(mockLogger.info.mock.calls[1][0]).toEqual( - JSON.parse(notification2.Message), - ) + expect(mockLogger.info.mock.calls[0][0]).toMatchObject({ + meta: { ...JSON.parse(notification1.Message) }, + }) + expect(mockLogger.info.mock.calls[1][0]).toMatchObject({ + meta: { ...JSON.parse(notification2.Message) }, + }) expect(mockLogger.warn).not.toHaveBeenCalled() expect(omit(actualBounce, 'expireAt')).toEqual({ formId, @@ -413,12 +464,12 @@ describe('updateBounces', () => { })) // There should only be one document after 2 notifications expect(actualBounceCursor.length).toBe(1) - expect(mockLogger.info.mock.calls[0][0]).toEqual( - JSON.parse(notification1.Message), - ) - expect(mockLogger.info.mock.calls[1][0]).toEqual( - JSON.parse(notification2.Message), - ) + expect(mockLogger.info.mock.calls[0][0]).toMatchObject({ + meta: { ...JSON.parse(notification1.Message) }, + }) + expect(mockLogger.info.mock.calls[1][0]).toMatchObject({ + meta: { ...JSON.parse(notification2.Message) }, + }) expect(mockLogger.warn).not.toHaveBeenCalled() expect(omit(actualBounce, 'expireAt')).toEqual({ formId, @@ -453,12 +504,12 @@ describe('updateBounces', () => { })) // There should only be one document after 2 notifications expect(actualBounceCursor.length).toBe(1) - expect(mockLogger.info.mock.calls[0][0]).toEqual( - JSON.parse(notification1.Message), - ) - expect(mockLogger.info.mock.calls[1][0]).toEqual( - JSON.parse(notification2.Message), - ) + expect(mockLogger.info.mock.calls[0][0]).toMatchObject({ + meta: { ...JSON.parse(notification1.Message) }, + }) + expect(mockLogger.info.mock.calls[1][0]).toMatchObject({ + meta: { ...JSON.parse(notification2.Message) }, + }) expect(mockLogger.warn).not.toHaveBeenCalled() expect(omit(actualBounce, 'expireAt')).toEqual({ formId, @@ -468,7 +519,7 @@ describe('updateBounces', () => { expect(actualBounce.expireAt).toBeInstanceOf(Date) }) - it('should not log critical bounces when hasAlarmed is true', async () => { + it('should log a second critical bounce with hasAlarmed true', async () => { const formId = new ObjectId() const submissionId1 = new ObjectId() const submissionId2 = new ObjectId() @@ -478,12 +529,18 @@ describe('updateBounces', () => { recipientList, recipientList, ) + const parsedNotification1: IBounceNotification = JSON.parse( + notification1.Message, + ) const notification2 = makeBounceNotification( formId, submissionId2, recipientList, recipientList, ) + const parsedNotification2: IBounceNotification = JSON.parse( + notification2.Message, + ) await updateBounces(notification1) await updateBounces(notification2) const actualBounceCursor = await Bounce.find({ formId }) @@ -494,14 +551,32 @@ describe('updateBounces', () => { })) // There should only be one document after 2 notifications expect(actualBounceCursor.length).toBe(1) - expect(mockLogger.info.mock.calls[0][0]).toEqual( - JSON.parse(notification1.Message), - ) - expect(mockLogger.info.mock.calls[1][0]).toEqual( - JSON.parse(notification2.Message), - ) - // Expect only 1 call to logger.warn - expect(mockLogger.warn.mock.calls.length).toBe(1) + expect(mockLogger.info.mock.calls[0][0]).toMatchObject({ + meta: { ...parsedNotification1 }, + }) + expect(mockLogger.info.mock.calls[1][0]).toMatchObject({ + meta: { ...parsedNotification2 }, + }) + expect(mockLogger.warn.mock.calls[0][0]).toMatchObject({ + message: 'Critical bounce', + meta: { + action: 'updateBounces', + hasAlarmed: false, + formId: formId.toHexString(), + submissionId: submissionId1.toHexString(), + bounceInfo: parsedNotification1.bounce, + }, + }) + expect(mockLogger.warn.mock.calls[1][0]).toMatchObject({ + message: 'Critical bounce', + meta: { + action: 'updateBounces', + hasAlarmed: true, + formId: formId.toHexString(), + submissionId: submissionId2.toHexString(), + bounceInfo: parsedNotification2.bounce, + }, + }) expect(omit(actualBounce, 'expireAt')).toEqual({ formId, hasAlarmed: true, @@ -509,4 +584,23 @@ describe('updateBounces', () => { }) expect(actualBounce.expireAt).toBeInstanceOf(Date) }) + + it('should log email confirmations to short-term logs', async () => { + const formId = new ObjectId() + const submissionId = new ObjectId() + const notification = makeBounceNotification( + formId, + submissionId, + recipientList, + recipientList, + 'Transient', + 'Email confirmation', + ) + await updateBounces(notification) + expect(mockLogger.info).not.toHaveBeenCalled() + expect(mockLogger.warn).not.toHaveBeenCalled() + expect(mockShortTermLogger.info).toHaveBeenCalledWith( + JSON.parse(notification.Message), + ) + }) }) diff --git a/src/app/models/bounce.server.model.ts b/src/app/modules/bounce/bounce.model.ts similarity index 78% rename from src/app/models/bounce.server.model.ts rename to src/app/modules/bounce/bounce.model.ts index 19efb85bbd..affb7c9f45 100644 --- a/src/app/models/bounce.server.model.ts +++ b/src/app/modules/bounce/bounce.model.ts @@ -2,18 +2,22 @@ import { get } from 'lodash' import { Model, Mongoose, Schema } from 'mongoose' import validator from 'validator' -import { bounceLifeSpan } from '../../config/config' +import { bounceLifeSpan } from '../../../config/config' import { IBounceNotification, IBounceSchema, IEmailNotification, - isBounceNotification, - isDeliveryNotification, ISingleBounce, -} from '../../types' -import { EMAIL_HEADERS, EMAIL_TYPES } from '../constants/mail' +} from '../../../types' +import { EMAIL_HEADERS, EmailType } from '../../constants/mail' +import { FORM_SCHEMA_ID } from '../../models/form.server.model' -import { FORM_SCHEMA_ID } from './form.server.model' +import { + extractHeader, + hasEmailBounced, + isBounceNotification, + isDeliveryNotification, +} from './bounce.util' export const BOUNCE_SCHEMA_ID = 'Bounce' @@ -58,26 +62,6 @@ const BounceSchema = new Schema({ }) BounceSchema.index({ expireAt: 1 }, { expireAfterSeconds: 0 }) -// Helper function for methods. -// Extracts custom headers which we send with all emails, such as form ID, submission ID -// and email type (admin response, email confirmation OTP etc). -const extractHeader = (body: IEmailNotification, header: string): string => { - return get(body, 'mail.headers').find( - (mailHeader) => mailHeader.name.toLowerCase() === header.toLowerCase(), - )?.value -} - -// Helper function for methods. -// Whether a bounce notification says a given email has bounced -const hasEmailBounced = ( - bounceInfo: IBounceNotification, - email: string, -): boolean => { - return get(bounceInfo, 'bounce.bouncedRecipients').some( - (emailInfo) => emailInfo.emailAddress === email, - ) -} - // Create a new Bounce document from an SNS notification. // More info on format of SNS notifications: // https://docs.aws.amazon.com/sns/latest/dg/sns-verify-signature-of-message.html @@ -88,7 +72,7 @@ BounceSchema.statics.fromSnsNotification = function ( const emailType = extractHeader(snsInfo, EMAIL_HEADERS.emailType) const formId = extractHeader(snsInfo, EMAIL_HEADERS.formId) // We only care about admin emails - if (emailType !== EMAIL_TYPES.adminResponse || !formId) { + if (emailType !== EmailType.AdminResponse || !formId) { return null } const isBounce = isBounceNotification(snsInfo) @@ -117,7 +101,6 @@ BounceSchema.methods.merge = function ( latestBounces: IBounceSchema, snsInfo: IEmailNotification, ): void { - const isDelivery = isDeliveryNotification(snsInfo) this.bounces.forEach((oldBounce) => { // If we were previously notified that a given email has bounced, // we want to retain that information @@ -131,8 +114,8 @@ BounceSchema.methods.merge = function ( // a false in latestBounces doesn't guarantee that the email was // delivered, only that the email has not bounced yet. const hasSubsequentlySucceeded = - isDelivery && - get(snsInfo, 'delivery.recipients').includes(oldBounce.email) + isDeliveryNotification(snsInfo) && + snsInfo.delivery.recipients.includes(oldBounce.email) if (matchedLatestBounce) { // Set the latest bounce status based on the latest notification matchedLatestBounce.hasBounced = !hasSubsequentlySucceeded @@ -142,6 +125,12 @@ BounceSchema.methods.merge = function ( this.bounces = latestBounces.bounces } +BounceSchema.methods.isCriticalBounce = function ( + this: IBounceSchema, +): boolean { + return this.bounces.every((emailInfo) => emailInfo.hasBounced) +} + const getBounceModel = (db: Mongoose): IBounceModel => { try { return db.model(BOUNCE_SCHEMA_ID) as IBounceModel diff --git a/src/app/modules/bounce/bounce.service.ts b/src/app/modules/bounce/bounce.service.ts index 082b3238e7..288884b504 100644 --- a/src/app/modules/bounce/bounce.service.ts +++ b/src/app/modules/bounce/bounce.service.ts @@ -3,16 +3,23 @@ import crypto from 'crypto' import { isEmpty } from 'lodash' import mongoose from 'mongoose' -import { createCloudWatchLogger } from '../../../config/logger' +import { + createCloudWatchLogger, + createLoggerWithLabel, +} from '../../../config/logger' import { IBounceNotification, IBounceSchema, IEmailNotification, ISnsNotification, } from '../../../types' -import getBounceModel from '../../models/bounce.server.model' +import { EMAIL_HEADERS, EmailType } from '../../constants/mail' + +import getBounceModel from './bounce.model' +import { extractHeader, isBounceNotification } from './bounce.util' -const logger = createCloudWatchLogger('email') +const logger = createLoggerWithLabel(module) +const shortTermLogger = createCloudWatchLogger('email') const Bounce = getBounceModel(mongoose) // Note that these need to be ordered in order to generate @@ -101,24 +108,48 @@ export const isValidSnsRequest = async ( // Writes a log message if all recipients have bounced const logCriticalBounce = ( bounceDoc: IBounceSchema, - formId: string, - notification: IEmailNotification, + submissionId: string, + bounceInfo: IBounceNotification['bounce'] | undefined, ): void => { - if ( - !bounceDoc.hasAlarmed && - bounceDoc.bounces.every((emailInfo) => emailInfo.hasBounced) - ) { + if (bounceDoc.isCriticalBounce()) { logger.warn({ - type: 'CRITICAL BOUNCE', - formId, - recipients: bounceDoc.bounces.map((emailInfo) => emailInfo.email), - // We know for sure that critical bounces can only happen because of bounce - // notifications, so this casting is okay - bounceInfo: (notification as IBounceNotification).bounce, + message: 'Critical bounce', + meta: { + action: 'updateBounces', + hasAlarmed: bounceDoc.hasAlarmed, + formId: String(bounceDoc.formId), + submissionId, + recipients: bounceDoc.bounces.map((emailInfo) => emailInfo.email), + // We know for sure that critical bounces can only happen because of bounce + // notifications, so we don't expect this to be undefined + bounceInfo: bounceInfo, + }, }) - // We don't want a flood of logs and alarms, so we use this to limit the rate of - // critical bounce logs for each form ID + // TODO (private #31): autoemail and set hasAlarmed to true. Currently + // hasAlarmed is a dangling key. bounceDoc.hasAlarmed = true + // TODO (private #31): convert bounceType to enum. + } +} + +/** + * Logs the raw notification to the relevant log group. + * @param notification The parsed SNS notification + */ +const logEmailNotification = (notification: IEmailNotification): void => { + if ( + extractHeader(notification, EMAIL_HEADERS.emailType) === + EmailType.EmailConfirmation + ) { + shortTermLogger.info(notification) + } else { + logger.info({ + message: 'Email notification', + meta: { + action: 'updateBounces', + ...notification, + }, + }) } } @@ -129,18 +160,21 @@ const logCriticalBounce = ( export const updateBounces = async (body: ISnsNotification): Promise => { const notification: IEmailNotification = JSON.parse(body.Message) // This is the crucial log statement which allows us to debug bounce-related - // issues, as it logs all the details about deliveries and bounces - logger.info(notification) + // issues, as it logs all the details about deliveries and bounces. Email + // confirmation info goes to the short-term log group so we do not store + // form fillers' information for too long, and everything else goes into the + // main log group. + logEmailNotification(notification) const latestBounces = Bounce.fromSnsNotification(notification) if (!latestBounces) return const formId = latestBounces.formId + const submissionId = extractHeader(notification, EMAIL_HEADERS.submissionId) + const bounceInfo = isBounceNotification(notification) && notification.bounce const oldBounces = await Bounce.findOne({ formId }) if (oldBounces) { oldBounces.merge(latestBounces, notification) - logCriticalBounce(oldBounces, formId, notification) - await oldBounces.save() - } else { - logCriticalBounce(latestBounces, formId, notification) - await latestBounces.save() } + const bounce = oldBounces ?? latestBounces + logCriticalBounce(bounce, submissionId, bounceInfo) + await bounce.save() } diff --git a/src/app/modules/bounce/bounce.util.ts b/src/app/modules/bounce/bounce.util.ts new file mode 100644 index 0000000000..36e2eb3cf7 --- /dev/null +++ b/src/app/modules/bounce/bounce.util.ts @@ -0,0 +1,43 @@ +import { + IBounceNotification, + IDeliveryNotification, + IEmailNotification, +} from 'src/types' +/** + * Extracts custom headers which we send with all emails, such as form ID, submission ID + * and email type (admin response, email confirmation OTP etc). + * @param body Body of SNS notification + * @param header Key of header to extract + */ +export const extractHeader = ( + body: IEmailNotification, + header: string, +): string => { + return body.mail.headers.find( + (mailHeader) => mailHeader.name.toLowerCase() === header.toLowerCase(), + )?.value +} + +/** + * Whether a bounce notification says a given email has bounced. + * @param bounceInfo Bounce notification from SNS + * @param email Email address to check + */ +export const hasEmailBounced = ( + bounceInfo: IBounceNotification, + email: string, +): boolean => { + return bounceInfo.bounce.bouncedRecipients.some( + (emailInfo) => emailInfo.emailAddress === email, + ) +} + +// If an email notification is for bounces +export const isBounceNotification = ( + body: IEmailNotification, +): body is IBounceNotification => body.notificationType === 'Bounce' + +// If an email notification is for successful delivery +export const isDeliveryNotification = ( + body: IEmailNotification, +): body is IDeliveryNotification => body.notificationType === 'Delivery' diff --git a/src/app/services/mail.service.ts b/src/app/services/mail.service.ts index 2f0afaf0a0..c319649eea 100644 --- a/src/app/services/mail.service.ts +++ b/src/app/services/mail.service.ts @@ -15,7 +15,7 @@ import { IPopulatedForm, ISubmissionSchema, } from '../../types' -import { EMAIL_HEADERS, EMAIL_TYPES } from '../constants/mail' +import { EMAIL_HEADERS, EmailType } from '../constants/mail' import { generateAutoreplyHtml, generateAutoreplyPdf, @@ -257,7 +257,7 @@ export class MailService { headers: { [EMAIL_HEADERS.formId]: String(form._id), [EMAIL_HEADERS.submissionId]: submission.id, - [EMAIL_HEADERS.emailType]: EMAIL_TYPES.emailConfirmation, + [EMAIL_HEADERS.emailType]: EmailType.EmailConfirmation, }, } @@ -291,7 +291,7 @@ export class MailService { otp, }), headers: { - [EMAIL_HEADERS.emailType]: EMAIL_TYPES.verificationOtp, + [EMAIL_HEADERS.emailType]: EmailType.VerificationOtp, }, } // Error gets caught in getNewOtp @@ -324,7 +324,7 @@ export class MailService { otp, }), headers: { - [EMAIL_HEADERS.emailType]: EMAIL_TYPES.loginOtp, + [EMAIL_HEADERS.emailType]: EmailType.LoginOtp, }, } @@ -399,7 +399,7 @@ export class MailService { headers: { [EMAIL_HEADERS.formId]: String(form._id), [EMAIL_HEADERS.submissionId]: refNo, - [EMAIL_HEADERS.emailType]: EMAIL_TYPES.adminResponse, + [EMAIL_HEADERS.emailType]: EmailType.AdminResponse, }, // replyTo options only allow string format. replyTo: replyToEmails?.join(', '), diff --git a/src/types/bounce.ts b/src/types/bounce.ts index 75f1c5ec04..6a4cea5286 100644 --- a/src/types/bounce.ts +++ b/src/types/bounce.ts @@ -18,4 +18,5 @@ export interface IBounce { export interface IBounceSchema extends IBounce, Document { merge: (latestBounces: IBounceSchema, snsInfo: IEmailNotification) => void + isCriticalBounce: () => boolean } diff --git a/src/types/sns.ts b/src/types/sns.ts index a3873fbdb0..6202f72160 100644 --- a/src/types/sns.ts +++ b/src/types/sns.ts @@ -52,13 +52,3 @@ export interface IDeliveryNotification extends IEmailNotification { recipients: string[] } } - -// If an email notification is for bounces -export const isBounceNotification = ( - body: IEmailNotification, -): body is IBounceNotification => body.notificationType === 'Bounce' - -// If an email notification is for successful delivery -export const isDeliveryNotification = ( - body: IEmailNotification, -): body is IDeliveryNotification => body.notificationType === 'Delivery' diff --git a/tsconfig.build.json b/tsconfig.build.json index 1a236a6031..e3e631c9ac 100644 --- a/tsconfig.build.json +++ b/tsconfig.build.json @@ -1,5 +1,5 @@ // Main tsconfig for building, prevents tests from being compiled. { "extends": "./tsconfig.json", - "exclude": ["tests/", "**/*.spec.ts", "src/public/"] + "exclude": ["tests/", "**/*.spec.ts", "**/__tests__/", "src/public/"] }