From ce4a9510bf8104f417f6f59ffe3d92cfd1bac2d9 Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Mon, 28 Sep 2020 11:14:10 +0800 Subject: [PATCH 01/14] feat(FeatureManager): add stronger typings and export type from index --- .../verification/verification.factory.ts | 3 +-- src/config/feature-manager/index.ts | 2 ++ src/config/feature-manager/types.ts | 5 +++++ .../util/FeatureManager.class.ts | 20 +++++++++---------- src/config/formsg-sdk.ts | 3 +-- src/loaders/express/helmet.ts | 3 +-- src/loaders/express/locals.ts | 3 +-- 7 files changed, 21 insertions(+), 18 deletions(-) diff --git a/src/app/modules/verification/verification.factory.ts b/src/app/modules/verification/verification.factory.ts index 3914e633b9..0e7d4cd8e6 100644 --- a/src/app/modules/verification/verification.factory.ts +++ b/src/app/modules/verification/verification.factory.ts @@ -1,8 +1,7 @@ import { RequestHandler } from 'express' import { StatusCodes } from 'http-status-codes' -import featureManager from '../../../config/feature-manager' -import { FeatureNames } from '../../../config/feature-manager/types' +import featureManager, { FeatureNames } from '../../../config/feature-manager' import * as verification from './verification.controller' diff --git a/src/config/feature-manager/index.ts b/src/config/feature-manager/index.ts index cf39394a8a..eaf41a85d4 100644 --- a/src/config/feature-manager/index.ts +++ b/src/config/feature-manager/index.ts @@ -8,6 +8,8 @@ import spcpMyInfo from './spcp-myinfo.config' import verifiedFields from './verified-fields.config' import webhookVerifiedContent from './webhook-verified-content.config' +export * from './types' + const featureManager = new FeatureManager() // Register features and associated middleware/fallbacks diff --git a/src/config/feature-manager/types.ts b/src/config/feature-manager/types.ts index d9e6a98c75..89de1fd588 100644 --- a/src/config/feature-manager/types.ts +++ b/src/config/feature-manager/types.ts @@ -80,6 +80,11 @@ export interface IFeatureManager { [FeatureNames.WebhookVerifiedContent]: IWebhookVerifiedContent } +export interface RegisteredFeature { + isEnabled: boolean + props: IFeatureManager[T] +} + export interface RegisterableFeature { name: K schema: Schema diff --git a/src/config/feature-manager/util/FeatureManager.class.ts b/src/config/feature-manager/util/FeatureManager.class.ts index 09e5aaa84b..e0492ca4fa 100644 --- a/src/config/feature-manager/util/FeatureManager.class.ts +++ b/src/config/feature-manager/util/FeatureManager.class.ts @@ -3,16 +3,16 @@ import validator from 'convict-format-with-validator' import _ from 'lodash' import { createLoggerWithLabel } from '../../logger' -import { FeatureNames, IFeatureManager, RegisterableFeature } from '../types' +import { + FeatureNames, + IFeatureManager, + RegisterableFeature, + RegisteredFeature, +} from '../types' const logger = createLoggerWithLabel(module) convict.addFormat(validator.url) -interface RegisteredFeature { - isEnabled: boolean - props: IFeatureManager[T] -} - export default class FeatureManager { public states: Partial> // Map some feature names to some env vars @@ -96,9 +96,9 @@ export default class FeatureManager { * Return props registered for requested feature * @param name the feature to return properties for */ - props(name: K) { + props(name: K): IFeatureManager[K] { if (this.states[name] !== undefined) { - return this.properties[name] + return this.properties[name] as IFeatureManager[K] } // Not enabled or not in state. throw new Error(`A feature called ${name} does not exist`) @@ -109,11 +109,11 @@ export default class FeatureManager { * and whether requested feature is enabled * @param name the name of the feature to return */ - get(name: FeatureNames): RegisteredFeature { + get(name: K): RegisteredFeature { return { isEnabled: this.isEnabled(name), // TODO (#317): remove usage of non-null assertion - props: this.props(name)!, + props: this.props(name), } } } diff --git a/src/config/formsg-sdk.ts b/src/config/formsg-sdk.ts index 84aa64ea8e..5dda108394 100644 --- a/src/config/formsg-sdk.ts +++ b/src/config/formsg-sdk.ts @@ -3,9 +3,8 @@ import { get } from 'lodash' import * as vfnConstants from '../shared/util/verification' -import { FeatureNames } from './feature-manager/types' import { formsgSdkMode } from './config' -import featureManager from './feature-manager' +import featureManager, { FeatureNames } from './feature-manager' const formsgSdk = formsgSdkPackage({ webhookSecretKey: get( diff --git a/src/loaders/express/helmet.ts b/src/loaders/express/helmet.ts index 008c0b909a..05e6da84a6 100644 --- a/src/loaders/express/helmet.ts +++ b/src/loaders/express/helmet.ts @@ -4,8 +4,7 @@ import { ContentSecurityPolicyOptions } from 'helmet/dist/middlewares/content-se import { get } from 'lodash' import config from '../../config/config' -import featureManager from '../../config/feature-manager' -import { FeatureNames } from '../../config/feature-manager/types' +import featureManager, { FeatureNames } from '../../config/feature-manager' const helmetMiddlewares = () => { // Only add the "Strict-Transport-Security" header if request is https. diff --git a/src/loaders/express/locals.ts b/src/loaders/express/locals.ts index 5439d35184..a3e5fe77f2 100644 --- a/src/loaders/express/locals.ts +++ b/src/loaders/express/locals.ts @@ -2,8 +2,7 @@ import ejs from 'ejs' import { get } from 'lodash' import config from '../../config/config' -import featureManager from '../../config/feature-manager' -import { FeatureNames } from '../../config/feature-manager/types' +import featureManager, { FeatureNames } from '../../config/feature-manager' // Construct js with environment variables needed by frontend const frontendVars = { From cbe7eb519991b2a693e86daad12620c4ed154a80 Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Tue, 29 Sep 2020 20:42:02 +0800 Subject: [PATCH 02/14] refactor: move sms related files into its own services/sms directory --- src/app/factories/sms.factory.js | 19 ++--- src/app/services/sms/sms.errors.ts | 9 +++ src/app/services/{ => sms}/sms.service.ts | 70 +++++++------------ .../services/sms/sms.types.ts} | 21 +++++- .../sms}/sms_count.server.model.ts | 10 +-- src/types/index.ts | 1 - 6 files changed, 64 insertions(+), 66 deletions(-) create mode 100644 src/app/services/sms/sms.errors.ts rename src/app/services/{ => sms}/sms.service.ts (86%) rename src/{types/sms_count.ts => app/services/sms/sms.types.ts} (77%) rename src/app/{models => services/sms}/sms_count.server.model.ts (91%) diff --git a/src/app/factories/sms.factory.js b/src/app/factories/sms.factory.js index fd07ba659e..1c4424ea62 100644 --- a/src/app/factories/sms.factory.js +++ b/src/app/factories/sms.factory.js @@ -1,6 +1,9 @@ const twilio = require('twilio') +const { + sendVerificationOtp, + sendAdminContactOtp, +} = require('../services/sms/sms.service') const featureManager = require('../../config/feature-manager').default -const smsService = require('../services/sms.service') const smsFactory = ({ isEnabled, props }) => { let twilioClient @@ -18,12 +21,7 @@ const smsFactory = ({ isEnabled, props }) => { return { async sendVerificationOtp(recipient, otp, formId) { if (isEnabled) { - return smsService.sendVerificationOtp( - recipient, - otp, - formId, - twilioConfig, - ) + return sendVerificationOtp(recipient, otp, formId, twilioConfig) } else { throw new Error( `Verification OTP has not been configured to be sent for mobile fields`, @@ -32,12 +30,7 @@ const smsFactory = ({ isEnabled, props }) => { }, async sendAdminContactOtp(recipient, otp, userId) { if (isEnabled) { - return smsService.sendAdminContactOtp( - recipient, - otp, - userId, - twilioConfig, - ) + return sendAdminContactOtp(recipient, otp, userId, twilioConfig) } else { throw new Error(`Send Admin Contact OTP has not been enabled`) } diff --git a/src/app/services/sms/sms.errors.ts b/src/app/services/sms/sms.errors.ts new file mode 100644 index 0000000000..041619b378 --- /dev/null +++ b/src/app/services/sms/sms.errors.ts @@ -0,0 +1,9 @@ +import { ApplicationError } from 'src/app/modules/core/core.errors' + +export class SmsSendError extends ApplicationError { + constructor( + message = 'Error sending OTP. Please try again later and if the problem persists, contact us.', + ) { + super(message) + } +} diff --git a/src/app/services/sms.service.ts b/src/app/services/sms/sms.service.ts similarity index 86% rename from src/app/services/sms.service.ts rename to src/app/services/sms/sms.service.ts index 316a4d31c8..fd7372791b 100644 --- a/src/app/services/sms.service.ts +++ b/src/app/services/sms/sms.service.ts @@ -3,19 +3,22 @@ import mongoose from 'mongoose' import NodeCache from 'node-cache' import Twilio from 'twilio' -import config from '../../config/config' -import { createLoggerWithLabel } from '../../config/logger' -import { isPhoneNumber } from '../../shared/util/phone-num-validation' -import { VfnErrors } from '../../shared/util/verification' +import config from '../../../config/config' +import { createLoggerWithLabel } from '../../../config/logger' +import { isPhoneNumber } from '../../../shared/util/phone-num-validation' +import { VfnErrors } from '../../../shared/util/verification' +import { AdminContactOtpData, FormOtpData } from '../../../types' +import getFormModel from '../../models/form.server.model' + +import getSmsCountModel from './sms_count.server.model' +import { SmsSendError } from './sms.errors' import { - AdminContactOtpData, - FormOtpData, LogSmsParams, LogType, SmsType, -} from '../../types' -import getFormModel from '../models/form.server.model' -import getSmsCountModel from '../models/sms_count.server.model' + TwilioConfig, + TwilioCredentials, +} from './sms.types' const logger = createLoggerWithLabel(module) const SmsCount = getSmsCountModel(mongoose) @@ -28,13 +31,6 @@ const secretsManager = new SecretsManager({ region: config.aws.region }) // credentials, or wait 10 seconds before. const twilioClientCache = new NodeCache({ deleteOnExpire: true, stdTTL: 10 }) -type TwilioCredentials = { - accountSid: string - apiKey: string - apiSecret: string - messagingServiceSid: string -} - /** * Retrieves credentials from secrets manager * @param msgSrvcName The name of credential stored in the secret manager. @@ -71,11 +67,6 @@ const getCredentials = async ( return null } -type TwilioConfig = { - client: Twilio.Twilio - msgSrvcSid: string -} - /** * * @param msgSrvcName The name of credential stored in the secret manager @@ -179,7 +170,17 @@ const send = async ( // Sent but with error code. // Throw error to be caught in catch block. if (!sid || errorCode) { - throw new TwilioError(errorMessage, errorCode, status) + const smsSendError = new SmsSendError(errorMessage) + logger.error({ + message: 'Error sending SMS OTP via Twilio', + meta: { + action: 'send', + errorCode, + status, + }, + error: smsSendError, + }) + throw smsSendError } // Log success @@ -249,7 +250,7 @@ const send = async ( * @param formId Form id for logging. * */ -const sendVerificationOtp = async ( +export const sendVerificationOtp = async ( recipient: string, otp: string, formId: string, @@ -282,7 +283,7 @@ const sendVerificationOtp = async ( return send(twilioData, otpData, recipient, message, SmsType.verification) } -const sendAdminContactOtp = async ( +export const sendAdminContactOtp = async ( recipient: string, otp: string, userId: string, @@ -304,24 +305,3 @@ const sendAdminContactOtp = async ( return send(defaultConfig, otpData, recipient, message, SmsType.adminContact) } - -class TwilioError extends Error { - code: number - status: string - - constructor(message: string, code: number, status: string) { - super(message) - this.code = code - this.status = status - this.name = this.constructor.name - - // Set the prototype explicitly. - // See https://github.com/facebook/jest/issues/8279 - Object.setPrototypeOf(this, TwilioError.prototype) - } -} - -module.exports = { - sendVerificationOtp, - sendAdminContactOtp, -} diff --git a/src/types/sms_count.ts b/src/app/services/sms/sms.types.ts similarity index 77% rename from src/types/sms_count.ts rename to src/app/services/sms/sms.types.ts index 48ca223996..a5d52222f3 100644 --- a/src/types/sms_count.ts +++ b/src/app/services/sms/sms.types.ts @@ -1,7 +1,12 @@ import { Document, Model } from 'mongoose' +import { Twilio } from 'twilio' -import { FormOtpData, IFormSchema } from './form' -import { AdminContactOtpData, IUserSchema } from './user' +import { + AdminContactOtpData, + FormOtpData, + IFormSchema, + IUserSchema, +} from 'src/types' export enum SmsType { verification = 'VERIFICATION', @@ -50,3 +55,15 @@ export type IAdminContactSmsCountSchema = ISmsCountSchema export interface ISmsCountModel extends Model { logSms: (logParams: LogSmsParams) => Promise } + +export type TwilioCredentials = { + accountSid: string + apiKey: string + apiSecret: string + messagingServiceSid: string +} + +export type TwilioConfig = { + client: Twilio + msgSrvcSid: string +} diff --git a/src/app/models/sms_count.server.model.ts b/src/app/services/sms/sms_count.server.model.ts similarity index 91% rename from src/app/models/sms_count.server.model.ts rename to src/app/services/sms/sms_count.server.model.ts index f702563f2b..c0e2299d96 100644 --- a/src/app/models/sms_count.server.model.ts +++ b/src/app/services/sms/sms_count.server.model.ts @@ -1,5 +1,8 @@ import { Mongoose, Schema } from 'mongoose' +import { FORM_SCHEMA_ID } from '../../models/form.server.model' +import { USER_SCHEMA_ID } from '../../models/user.server.model' + import { IAdminContactSmsCountSchema, ISmsCount, @@ -9,10 +12,7 @@ import { LogSmsParams, LogType, SmsType, -} from '../../types' - -import { FORM_SCHEMA_ID } from './form.server.model' -import { USER_SCHEMA_ID } from './user.server.model' +} from './sms.types' const SMS_COUNT_SCHEMA_NAME = 'SmsCount' @@ -101,7 +101,7 @@ const compileSmsCountModel = (db: Mongoose) => { * @param db The mongoose instance to retrieve the SmsCount model from * @returns The SmsCount model */ -const getSmsCountModel = (db: Mongoose) => { +const getSmsCountModel = (db: Mongoose): ISmsCountModel => { try { return db.model(SMS_COUNT_SCHEMA_NAME) as ISmsCountModel } catch { diff --git a/src/types/index.ts b/src/types/index.ts index b1e4ab0b98..ae47076e39 100644 --- a/src/types/index.ts +++ b/src/types/index.ts @@ -10,7 +10,6 @@ export * from './login' export * from './mail' export * from './myinfo_hash' export * from './response' -export * from './sms_count' export * from './sns' export * from './submission' export * from './token' From d7856faea3fbfe705f8a575dcbec3b05c7f587e5 Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Tue, 29 Sep 2020 20:48:06 +0800 Subject: [PATCH 03/14] tests(Sms): move sms_count specs into nested __tests__ directory --- .../sms/__tests__}/sms_count.server.model.spec.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) rename {tests/unit/backend/models => src/app/services/sms/__tests__}/sms_count.server.model.spec.ts (96%) diff --git a/tests/unit/backend/models/sms_count.server.model.spec.ts b/src/app/services/sms/__tests__/sms_count.server.model.spec.ts similarity index 96% rename from tests/unit/backend/models/sms_count.server.model.spec.ts rename to src/app/services/sms/__tests__/sms_count.server.model.spec.ts index a3faddf2d2..49b51e2525 100644 --- a/tests/unit/backend/models/sms_count.server.model.spec.ts +++ b/src/app/services/sms/__tests__/sms_count.server.model.spec.ts @@ -1,11 +1,11 @@ +/* eslint-disable @typescript-eslint/ban-ts-comment */ import { ObjectId } from 'bson' import { cloneDeep, merge, omit } from 'lodash' import mongoose from 'mongoose' +import dbHandler from 'tests/unit/backend/helpers/jest-db' -import getSmsCountModel from 'src/app/models/sms_count.server.model' -import { IVerificationSmsCount, LogType, SmsType } from 'src/types' - -import dbHandler from '../helpers/jest-db' +import getSmsCountModel from '../sms_count.server.model' +import { IVerificationSmsCount, LogType, SmsType } from '../sms.types' const SmsCount = getSmsCountModel(mongoose) @@ -194,7 +194,7 @@ describe('SmsCount', () => { form: MOCK_FORM_ID, }).lean() - expect(actualLog!._id).toBeDefined() + expect(actualLog?._id).toBeDefined() // Retrieve object and compare to params, remove indeterministic keys const actualSavedObject = omit(actualLog, ['_id', 'createdAt', '__v']) expect(actualSavedObject).toEqual(expectedLog) @@ -220,7 +220,7 @@ describe('SmsCount', () => { form: MOCK_FORM_ID, }).lean() - expect(actualLog!._id).toBeDefined() + expect(actualLog?._id).toBeDefined() // Retrieve object and compare to params, remove indeterministic keys const actualSavedObject = omit(actualLog, ['_id', 'createdAt', '__v']) expect(actualSavedObject).toEqual(expectedLog) From 253626e2b33b644de7030b247d63b76cdab3bbad Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Tue, 29 Sep 2020 20:58:53 +0800 Subject: [PATCH 04/14] fix(sms): correct absolute import --- src/app/services/sms/sms.errors.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/services/sms/sms.errors.ts b/src/app/services/sms/sms.errors.ts index 041619b378..95ddd9171c 100644 --- a/src/app/services/sms/sms.errors.ts +++ b/src/app/services/sms/sms.errors.ts @@ -1,4 +1,4 @@ -import { ApplicationError } from 'src/app/modules/core/core.errors' +import { ApplicationError } from '../../modules/core/core.errors' export class SmsSendError extends ApplicationError { constructor( From 9293b02e6586f27b671ffa26a7aa24ffc51ca18e Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Tue, 29 Sep 2020 21:04:53 +0800 Subject: [PATCH 05/14] feat(SmsFactory): migrate sms factory to Typescript --- src/app/factories/sms.factory.js | 41 ------------- src/app/modules/user/user.controller.ts | 2 +- .../verification/verification.service.ts | 10 ++-- src/app/services/sms/sms.factory.ts | 59 +++++++++++++++++++ src/app/services/sms/sms.service.ts | 8 +++ .../modules/user/user.controller.spec.ts | 4 +- .../verification/verification.service.spec.ts | 6 +- 7 files changed, 77 insertions(+), 53 deletions(-) delete mode 100644 src/app/factories/sms.factory.js create mode 100644 src/app/services/sms/sms.factory.ts diff --git a/src/app/factories/sms.factory.js b/src/app/factories/sms.factory.js deleted file mode 100644 index 1c4424ea62..0000000000 --- a/src/app/factories/sms.factory.js +++ /dev/null @@ -1,41 +0,0 @@ -const twilio = require('twilio') -const { - sendVerificationOtp, - sendAdminContactOtp, -} = require('../services/sms/sms.service') -const featureManager = require('../../config/feature-manager').default - -const smsFactory = ({ isEnabled, props }) => { - let twilioClient - if (isEnabled && props) { - twilioClient = twilio(props.twilioApiKey, props.twilioApiSecret, { - accountSid: props.twilioAccountSid, - }) - } - - let twilioConfig = { - msgSrvcSid: props && props.twilioMsgSrvcSid, - client: twilioClient, - } - - return { - async sendVerificationOtp(recipient, otp, formId) { - if (isEnabled) { - return sendVerificationOtp(recipient, otp, formId, twilioConfig) - } else { - throw new Error( - `Verification OTP has not been configured to be sent for mobile fields`, - ) - } - }, - async sendAdminContactOtp(recipient, otp, userId) { - if (isEnabled) { - return sendAdminContactOtp(recipient, otp, userId, twilioConfig) - } else { - throw new Error(`Send Admin Contact OTP has not been enabled`) - } - }, - } -} - -module.exports = smsFactory(featureManager.get('sms')) diff --git a/src/app/modules/user/user.controller.ts b/src/app/modules/user/user.controller.ts index c35b35cf30..09d80903b2 100644 --- a/src/app/modules/user/user.controller.ts +++ b/src/app/modules/user/user.controller.ts @@ -5,7 +5,7 @@ import { StatusCodes } from 'http-status-codes' import { createLoggerWithLabel } from '../../../config/logger' import { IPopulatedUser } from '../../../types' -import SmsFactory from '../../factories/sms.factory' +import { SmsFactory } from '../../services/sms/sms.factory' import { ApplicationError } from '../core/core.errors' import { diff --git a/src/app/modules/verification/verification.service.ts b/src/app/modules/verification/verification.service.ts index 90272a8662..549147dd3f 100644 --- a/src/app/modules/verification/verification.service.ts +++ b/src/app/modules/verification/verification.service.ts @@ -12,10 +12,10 @@ import { IVerificationFieldSchema, IVerificationSchema, } from '../../../types' -import smsFactory from '../../factories/sms.factory' import getFormModel from '../../models/form.server.model' import getVerificationModel from '../../models/verification.server.model' import MailService from '../../services/mail.service' +import { SmsFactory } from '../../services/sms/sms.factory' import { generateOtp } from '../../utils/otp' import { ITransaction } from './verification.types' @@ -257,17 +257,15 @@ const sendOTPForField = async ( field: IVerificationFieldSchema, recipient: string, otp: string, -): Promise => { +): Promise => { const { fieldType } = field switch (fieldType) { case 'mobile': // call sms - it should validate the recipient - await smsFactory.sendVerificationOtp(recipient, otp, formId) - break + return SmsFactory.sendVerificationOtp(recipient, otp, formId) case 'email': // call email - it should validate the recipient - await MailService.sendVerificationOtp(recipient, otp) - break + return MailService.sendVerificationOtp(recipient, otp) default: throw new Error(`sendOTPForField: ${fieldType} is unsupported`) } diff --git a/src/app/services/sms/sms.factory.ts b/src/app/services/sms/sms.factory.ts new file mode 100644 index 0000000000..d4964cca34 --- /dev/null +++ b/src/app/services/sms/sms.factory.ts @@ -0,0 +1,59 @@ +import Twilio from 'twilio' + +import FeatureManager, { FeatureNames } from '../../../config/feature-manager' + +import { sendAdminContactOtp, sendVerificationOtp } from './sms.service' +import { TwilioConfig } from './sms.types' + +interface ISmsFactory { + sendVerificationOtp: ( + recipient: string, + otp: string, + formId: string, + ) => ReturnType + sendAdminContactOtp: ( + recipient: string, + otp: string, + userId: string, + ) => ReturnType +} + +const createSmsFactory = (): ISmsFactory => { + const smsFeature = FeatureManager.get(FeatureNames.Sms) + + if (!smsFeature.isEnabled) { + const errorMessage = 'SMS feature must be enabled in Feature Manager first' + return { + sendAdminContactOtp: () => { + throw new Error(`sendAdminContactOtp: ${errorMessage}`) + }, + sendVerificationOtp: () => { + throw new Error(`sendVerificationOtp: ${errorMessage}`) + }, + } + } + + const { + twilioAccountSid, + twilioApiKey, + twilioApiSecret, + twilioMsgSrvcSid, + } = smsFeature.props + + const twilioClient = Twilio(twilioApiKey, twilioApiSecret, { + accountSid: twilioAccountSid, + }) + const twilioConfig: TwilioConfig = { + msgSrvcSid: twilioMsgSrvcSid, + client: twilioClient, + } + + return { + sendVerificationOtp: (recipient, otp, formId) => + sendVerificationOtp(recipient, otp, formId, twilioConfig), + sendAdminContactOtp: (recipient, otp, userId) => + sendAdminContactOtp(recipient, otp, userId, twilioConfig), + } +} + +export const SmsFactory = createSmsFactory() diff --git a/src/app/services/sms/sms.service.ts b/src/app/services/sms/sms.service.ts index fd7372791b..3d3ef21fa9 100644 --- a/src/app/services/sms/sms.service.ts +++ b/src/app/services/sms/sms.service.ts @@ -201,6 +201,14 @@ const send = async ( }) }) + logger.info({ + message: 'Successfully sent sms', + meta: { + action: 'send', + otpData, + }, + }) + return true }) .catch((err) => { diff --git a/tests/unit/backend/modules/user/user.controller.spec.ts b/tests/unit/backend/modules/user/user.controller.spec.ts index d057cfdc02..011a021149 100644 --- a/tests/unit/backend/modules/user/user.controller.spec.ts +++ b/tests/unit/backend/modules/user/user.controller.spec.ts @@ -1,16 +1,16 @@ import { StatusCodes } from 'http-status-codes' import { mocked } from 'ts-jest/utils' -import SmsFactory from 'src/app/factories/sms.factory' import * as UserController from 'src/app/modules/user/user.controller' import { InvalidOtpError } from 'src/app/modules/user/user.errors' import * as UserService from 'src/app/modules/user/user.service' +import { SmsFactory } from 'src/app/services/sms/sms.factory' import { IPopulatedUser, IUser, IUserSchema } from 'src/types' import expressHandler from '../../helpers/jest-express' jest.mock('src/app/modules/user/user.service') -jest.mock('src/app/factories/sms.factory') +jest.mock('src/app/services/sms/sms.factory') const MockUserService = mocked(UserService) const MockSmsFactory = mocked(SmsFactory) diff --git a/tests/unit/backend/modules/verification/verification.service.spec.ts b/tests/unit/backend/modules/verification/verification.service.spec.ts index babde3bff3..5df8847826 100644 --- a/tests/unit/backend/modules/verification/verification.service.spec.ts +++ b/tests/unit/backend/modules/verification/verification.service.spec.ts @@ -3,7 +3,6 @@ import { ObjectId } from 'bson' import mongoose from 'mongoose' import { mocked } from 'ts-jest/utils' -import smsFactory from 'src/app/factories/sms.factory' import getFormModel from 'src/app/models/form.server.model' import getVerificationModel from 'src/app/models/verification.server.model' import { @@ -14,6 +13,7 @@ import { verifyOtp, } from 'src/app/modules/verification/verification.service' import MailService from 'src/app/services/mail.service' +import { SmsFactory } from 'src/app/services/sms/sms.factory' import { generateOtp } from 'src/app/utils/otp' import formsgSdk from 'src/config/formsg-sdk' import { BasicField, IUserSchema, IVerificationSchema } from 'src/types' @@ -29,8 +29,8 @@ jest.mock('src/app/utils/otp') const MockGenerateOtp = mocked(generateOtp, true) jest.mock('src/config/formsg-sdk') const MockFormsgSdk = mocked(formsgSdk, true) -jest.mock('src/app/factories/sms.factory') -const MockSmsFactory = mocked(smsFactory, true) +jest.mock('src/app/services/sms/sms.factory') +const MockSmsFactory = mocked(SmsFactory, true) jest.mock('src/app/services/mail.service') const MockMailService = mocked(MailService, true) jest.mock('bcrypt') From 098f73afc2eb1a8688a5bbd239774ff3136ac299 Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Tue, 29 Sep 2020 21:30:38 +0800 Subject: [PATCH 06/14] feat: add more info to SmsSendError for Twilio error codes and status --- src/app/services/sms/sms.errors.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/app/services/sms/sms.errors.ts b/src/app/services/sms/sms.errors.ts index 95ddd9171c..a7f11c906b 100644 --- a/src/app/services/sms/sms.errors.ts +++ b/src/app/services/sms/sms.errors.ts @@ -1,9 +1,16 @@ import { ApplicationError } from '../../modules/core/core.errors' export class SmsSendError extends ApplicationError { + code?: number + sendStatus: unknown + constructor( message = 'Error sending OTP. Please try again later and if the problem persists, contact us.', + code?: number, + sendStatus?: unknown, ) { super(message) + this.code = code + this.sendStatus = sendStatus } } From 43c9c9712b4a5eb0534dc9112c51a0afe9e5450b Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Tue, 29 Sep 2020 21:32:49 +0800 Subject: [PATCH 07/14] test(sms): migrate SmsService tests to Typescript --- src/app/models/form.server.model.ts | 4 +- .../sms/__tests__/sms.service.spec.ts | 146 ++++++++++++++++++ src/app/services/sms/sms.service.ts | 31 +--- .../unit/backend/services/sms.service.spec.js | 130 ---------------- 4 files changed, 153 insertions(+), 158 deletions(-) create mode 100644 src/app/services/sms/__tests__/sms.service.spec.ts delete mode 100644 tests/unit/backend/services/sms.service.spec.js diff --git a/src/app/models/form.server.model.ts b/src/app/models/form.server.model.ts index 5beb51b95a..b94d9e749b 100644 --- a/src/app/models/form.server.model.ts +++ b/src/app/models/form.server.model.ts @@ -92,7 +92,7 @@ export interface IFormModel extends Model { deactivateById(formId: string): Promise } -type IEncryptedFormModel = Model +type IEncryptedFormModel = Model & IFormModel const EncryptedFormSchema = new Schema({ publicKey: { @@ -101,7 +101,7 @@ const EncryptedFormSchema = new Schema({ }, }) -type IEmailFormModel = Model +type IEmailFormModel = Model & IFormModel const EmailFormSchema = new Schema({ emails: { diff --git a/src/app/services/sms/__tests__/sms.service.spec.ts b/src/app/services/sms/__tests__/sms.service.spec.ts new file mode 100644 index 0000000000..0abf3cdffe --- /dev/null +++ b/src/app/services/sms/__tests__/sms.service.spec.ts @@ -0,0 +1,146 @@ +import mongoose from 'mongoose' +import dbHandler from 'tests/unit/backend/helpers/jest-db' + +import getFormModel from 'src/app/models/form.server.model' +import { VfnErrors } from 'src/shared/util/verification' +import { FormOtpData, IFormSchema, ResponseMode } from 'src/types' + +import getSmsCountModel from '../sms_count.server.model' +import * as SmsService from '../sms.service' +import { LogType, SmsType, TwilioConfig } from '../sms.types' + +const FormModel = getFormModel(mongoose) +const SmsCountModel = getSmsCountModel(mongoose) + +// Test numbers provided by Twilio: +// https://www.twilio.com/docs/iam/test-credentials +const TWILIO_TEST_NUMBER = '+15005550006' + +const MOCK_MSG_SRVC_SID = 'mockMsgSrvcSid' + +describe('sms.service', () => { + const MOCK_VALID_CONFIG = ({ + msgSrvcSid: MOCK_MSG_SRVC_SID, + client: { + messages: { + create: jest.fn().mockResolvedValue({ + status: 'testStatus', + sid: 'testSid', + }), + }, + }, + } as unknown) as TwilioConfig + + const MOCK_INVALID_CONFIG = ({ + msgSrvcSid: MOCK_MSG_SRVC_SID, + client: { + messages: { + create: jest.fn().mockResolvedValue({ + status: 'testStatus', + sid: undefined, + errorCode: 21211, + }), + }, + }, + } as unknown) as TwilioConfig + + beforeAll(async () => await dbHandler.connect()) + afterEach(async () => await dbHandler.clearDatabase()) + afterAll(async () => await dbHandler.closeDatabase()) + + describe('sendVerificationOtp', () => { + let testForm: IFormSchema + let mockOtpData: FormOtpData + + beforeEach(async () => { + const { user } = await dbHandler.insertFormCollectionReqs() + + const emailForm = await FormModel.create({ + title: 'Test Form', + emails: [user.email], + admin: user._id, + responseMode: ResponseMode.Email, + }) + + mockOtpData = { + form: emailForm._id, + formAdmin: { + email: user.email, + userId: user._id, + }, + } + + testForm = emailForm + }) + + it('should throw error when retrieved otpData is null', async () => { + // Arrange + // Return null on Form method + jest.spyOn(FormModel, 'getOtpData').mockResolvedValueOnce(null) + + // Act + const actualPromise = SmsService.sendVerificationOtp( + /* recipient= */ TWILIO_TEST_NUMBER, + /* otp= */ '111111', + /* formId= */ testForm._id, + /* defaultConfig= */ MOCK_VALID_CONFIG, + ) + + await expect(actualPromise).rejects.toThrowError() + }) + + it('should log and send verification OTP when sending has no errors', async () => { + // Arrange + jest.spyOn(FormModel, 'getOtpData').mockResolvedValueOnce(mockOtpData) + const smsCountSpy = jest.spyOn(SmsCountModel, 'logSms') + + // Act + const actualPromise = SmsService.sendVerificationOtp( + /* recipient= */ TWILIO_TEST_NUMBER, + /* otp= */ '111111', + /* formId= */ testForm._id, + /* defaultConfig= */ MOCK_VALID_CONFIG, + ) + + // Assert + // Should resolve to true + await expect(actualPromise).resolves.toEqual(true) + // Logging should also have happened. + const expectedLogParams = { + otpData: mockOtpData, + msgSrvcSid: MOCK_MSG_SRVC_SID, + smsType: SmsType.verification, + logType: LogType.success, + } + expect(smsCountSpy).toHaveBeenCalledWith(expectedLogParams) + }) + + it('should log failure and throw error when sms fails to send', async () => { + // Arrange + jest.spyOn(FormModel, 'getOtpData').mockResolvedValueOnce(mockOtpData) + const smsCountSpy = jest.spyOn(SmsCountModel, 'logSms') + + // Act + const actualPromise = SmsService.sendVerificationOtp( + /* recipient= */ TWILIO_TEST_NUMBER, + /* otp= */ '111111', + /* formId= */ testForm._id, + /* defaultConfig= */ MOCK_INVALID_CONFIG, + ) + + // Assert + const expectedError = new Error(VfnErrors.InvalidMobileNumber) + expectedError.name = VfnErrors.SendOtpFailed + + await expect(actualPromise).rejects.toThrow(expectedError) + // Logging should also have happened. + const expectedLogParams = { + otpData: mockOtpData, + msgSrvcSid: MOCK_MSG_SRVC_SID, + smsType: SmsType.verification, + logType: LogType.failure, + } + expect(smsCountSpy).toHaveBeenCalledWith(expectedLogParams) + }) + }) +}) diff --git a/src/app/services/sms/sms.service.ts b/src/app/services/sms/sms.service.ts index 3d3ef21fa9..aef1b41aa7 100644 --- a/src/app/services/sms/sms.service.ts +++ b/src/app/services/sms/sms.service.ts @@ -126,17 +126,6 @@ const getTwilio = async ( return defaultConfig } -/** - * Retrieve the relevant data required to send an OTP from given formId - * @param formId The form to retrieve data from - * @returns Relevant OTP data containing the linked messaging service name (if available), and form details such as its id and the admin. - * - */ -const getOtpDataFromForm = async (formId: string) => { - const otpData = await Form.getOtpData(formId) - return otpData -} - /** * Sends a message to a valid phone number * @param twilioConfig The configuration used to send OTPs with @@ -170,17 +159,7 @@ const send = async ( // Sent but with error code. // Throw error to be caught in catch block. if (!sid || errorCode) { - const smsSendError = new SmsSendError(errorMessage) - logger.error({ - message: 'Error sending SMS OTP via Twilio', - meta: { - action: 'send', - errorCode, - status, - }, - error: smsSendError, - }) - throw smsSendError + throw new SmsSendError(errorMessage, errorCode, status) } // Log success @@ -241,7 +220,7 @@ const send = async ( // Invalid number error code, throw a more reasonable error for error // handling. // See https://www.twilio.com/docs/api/errors/21211 - if (err.code === 21211) { + if (err?.code === 21211) { const invalidOtpError = new Error(VfnErrors.InvalidMobileNumber) invalidOtpError.name = VfnErrors.SendOtpFailed throw invalidOtpError @@ -263,7 +242,7 @@ export const sendVerificationOtp = async ( otp: string, formId: string, defaultConfig: TwilioConfig, -) => { +): Promise => { logger.info({ message: `Sending verification OTP for ${formId}`, meta: { @@ -271,7 +250,7 @@ export const sendVerificationOtp = async ( formId, }, }) - const otpData = await getOtpDataFromForm(formId) + const otpData = await Form.getOtpData(formId) if (!otpData) { const errMsg = `Unable to retrieve otpData from ${formId}` @@ -296,7 +275,7 @@ export const sendAdminContactOtp = async ( otp: string, userId: string, defaultConfig: TwilioConfig, -) => { +): Promise => { logger.info({ message: `Sending admin contact verification OTP for ${userId}`, meta: { diff --git a/tests/unit/backend/services/sms.service.spec.js b/tests/unit/backend/services/sms.service.spec.js deleted file mode 100644 index b62845fc38..0000000000 --- a/tests/unit/backend/services/sms.service.spec.js +++ /dev/null @@ -1,130 +0,0 @@ -const dbHandler = require('../helpers/db-handler') -const { VfnErrors } = spec('dist/backend/shared/util/verification') -const { SmsType, LogType } = spec('dist/backend/types') - -const Form = dbHandler.makeModel('form.server.model', 'Form') -const SmsCount = dbHandler.makeModel('sms_count.server.model', 'SmsCount') - -const SmsService = spec('dist/backend/app/services/sms.service') - -// Test numbers provided by Twilio: -// https://www.twilio.com/docs/iam/test-credentials -const TWILIO_TEST_NUMBER = '+15005550006' - -const MOCK_MSG_SRVC_SID = 'mockMsgSrvcSid' - -describe('sms.service', () => { - const MOCK_VALID_CONFIG = { - msgSrvcSid: MOCK_MSG_SRVC_SID, - client: { - messages: { - create: jasmine.createSpy().and.resolveTo({ - status: 'testStatus', - sid: 'testSid', - }), - }, - }, - } - - const MOCK_INVALID_CONFIG = { - msgSrvcSid: MOCK_MSG_SRVC_SID, - client: { - messages: { - create: jasmine.createSpy().and.resolveTo({ - status: 'testStatus', - sid: undefined, - errorCode: 21211, - }), - }, - }, - } - - beforeAll(async () => await dbHandler.connect()) - afterEach(async () => await dbHandler.clearDatabase()) - afterAll(async () => await dbHandler.closeDatabase()) - - describe('sendVerificationOtp', () => { - let testForm - let mockOtpData - beforeEach(async () => { - const { form, user } = await dbHandler.preloadCollections() - - mockOtpData = { - form: form._id, - formAdmin: { - email: user.email, - userId: user._id, - }, - } - - testForm = form - }) - it('should throw error if otpData is null', async () => { - // Arrange - // Return null on Form method - spyOn(Form, 'getOtpData').and.returnValue(null) - - await expectAsync( - SmsService.sendVerificationOtp( - /* recipient= */ TWILIO_TEST_NUMBER, - /* otp= */ '111111', - /* formId= */ testForm._id, - /* defaultConfig= */ MOCK_VALID_CONFIG, - ), - ).toBeRejectedWithError() - }) - - it('should log and send verification OTP if twilio has no errors', async () => { - // Arrange - spyOn(Form, 'getOtpData').and.returnValue(mockOtpData) - const smsCountSpy = spyOn(SmsCount, 'logSms').and.callThrough() - // Act + Assert - await expectAsync( - SmsService.sendVerificationOtp( - /* recipient= */ TWILIO_TEST_NUMBER, - /* otp= */ '111111', - /* formId= */ testForm._id, - /* defaultConfig= */ MOCK_VALID_CONFIG, - ), - // Should resolve to true - ).toBeResolvedTo(true) - - // Logging should also have happened. - const expectedLogParams = { - otpData: mockOtpData, - msgSrvcSid: MOCK_MSG_SRVC_SID, - smsType: SmsType.verification, - logType: LogType.success, - } - expect(smsCountSpy).toHaveBeenCalledWith(expectedLogParams) - }) - - it('should log failure and throw error if twilio failed to send', async () => { - // Arrange - spyOn(Form, 'getOtpData').and.returnValue(mockOtpData) - const smsCountSpy = spyOn(SmsCount, 'logSms').and.callThrough() - - // Act + Assert - const expectedError = new Error(VfnErrors.InvalidMobileNumber) - expectedError.name = VfnErrors.SendOtpFailed - - await expectAsync( - SmsService.sendVerificationOtp( - /* recipient= */ TWILIO_TEST_NUMBER, - /* otp= */ '111111', - /* formId= */ testForm._id, - /* defaultConfig= */ MOCK_INVALID_CONFIG, - ), - ).toBeRejectedWith(expectedError) - - // Logging should also have happened. - const expectedLogParams = { - otpData: mockOtpData, - msgSrvcSid: MOCK_MSG_SRVC_SID, - smsType: SmsType.verification, - logType: LogType.failure, - } - expect(smsCountSpy).toHaveBeenCalledWith(expectedLogParams) - }) - }) -}) From 72f62fa6c111c8679a2572caf8bdbd5a9c27095b Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Tue, 29 Sep 2020 21:54:56 +0800 Subject: [PATCH 08/14] test(sms): add tests for sendAdminContactOtp fn --- .../sms/__tests__/sms.service.spec.ts | 131 +++++++++++++----- src/app/services/sms/sms.service.ts | 1 + 2 files changed, 94 insertions(+), 38 deletions(-) diff --git a/src/app/services/sms/__tests__/sms.service.spec.ts b/src/app/services/sms/__tests__/sms.service.spec.ts index 0abf3cdffe..0496e118ca 100644 --- a/src/app/services/sms/__tests__/sms.service.spec.ts +++ b/src/app/services/sms/__tests__/sms.service.spec.ts @@ -3,7 +3,7 @@ import dbHandler from 'tests/unit/backend/helpers/jest-db' import getFormModel from 'src/app/models/form.server.model' import { VfnErrors } from 'src/shared/util/verification' -import { FormOtpData, IFormSchema, ResponseMode } from 'src/types' +import { FormOtpData, IFormSchema, IUserSchema, ResponseMode } from 'src/types' import getSmsCountModel from '../sms_count.server.model' import * as SmsService from '../sms.service' @@ -18,59 +18,64 @@ const TWILIO_TEST_NUMBER = '+15005550006' const MOCK_MSG_SRVC_SID = 'mockMsgSrvcSid' -describe('sms.service', () => { - const MOCK_VALID_CONFIG = ({ - msgSrvcSid: MOCK_MSG_SRVC_SID, - client: { - messages: { - create: jest.fn().mockResolvedValue({ - status: 'testStatus', - sid: 'testSid', - }), - }, +const MOCK_VALID_CONFIG = ({ + msgSrvcSid: MOCK_MSG_SRVC_SID, + client: { + messages: { + create: jest.fn().mockResolvedValue({ + status: 'testStatus', + sid: 'testSid', + }), }, - } as unknown) as TwilioConfig - - const MOCK_INVALID_CONFIG = ({ - msgSrvcSid: MOCK_MSG_SRVC_SID, - client: { - messages: { - create: jest.fn().mockResolvedValue({ - status: 'testStatus', - sid: undefined, - errorCode: 21211, - }), - }, + }, +} as unknown) as TwilioConfig + +const MOCK_INVALID_CONFIG = ({ + msgSrvcSid: MOCK_MSG_SRVC_SID, + client: { + messages: { + create: jest.fn().mockResolvedValue({ + status: 'testStatus', + sid: undefined, + errorCode: 21211, + }), }, - } as unknown) as TwilioConfig + }, +} as unknown) as TwilioConfig + +const smsCountSpy = jest.spyOn(SmsCountModel, 'logSms') + +describe('sms.service', () => { + let testUser: IUserSchema beforeAll(async () => await dbHandler.connect()) + beforeEach(async () => { + const { user } = await dbHandler.insertFormCollectionReqs() + testUser = user + smsCountSpy.mockClear() + }) afterEach(async () => await dbHandler.clearDatabase()) afterAll(async () => await dbHandler.closeDatabase()) describe('sendVerificationOtp', () => { - let testForm: IFormSchema let mockOtpData: FormOtpData + let testForm: IFormSchema beforeEach(async () => { - const { user } = await dbHandler.insertFormCollectionReqs() - - const emailForm = await FormModel.create({ + testForm = await FormModel.create({ title: 'Test Form', - emails: [user.email], - admin: user._id, + emails: [testUser.email], + admin: testUser._id, responseMode: ResponseMode.Email, }) mockOtpData = { - form: emailForm._id, + form: testForm._id, formAdmin: { - email: user.email, - userId: user._id, + email: testUser.email, + userId: testUser._id, }, } - - testForm = emailForm }) it('should throw error when retrieved otpData is null', async () => { @@ -92,7 +97,6 @@ describe('sms.service', () => { it('should log and send verification OTP when sending has no errors', async () => { // Arrange jest.spyOn(FormModel, 'getOtpData').mockResolvedValueOnce(mockOtpData) - const smsCountSpy = jest.spyOn(SmsCountModel, 'logSms') // Act const actualPromise = SmsService.sendVerificationOtp( @@ -115,10 +119,9 @@ describe('sms.service', () => { expect(smsCountSpy).toHaveBeenCalledWith(expectedLogParams) }) - it('should log failure and throw error when sms fails to send', async () => { + it('should log failure and throw error when verification OTP fails to send', async () => { // Arrange jest.spyOn(FormModel, 'getOtpData').mockResolvedValueOnce(mockOtpData) - const smsCountSpy = jest.spyOn(SmsCountModel, 'logSms') // Act const actualPromise = SmsService.sendVerificationOtp( @@ -143,4 +146,56 @@ describe('sms.service', () => { expect(smsCountSpy).toHaveBeenCalledWith(expectedLogParams) }) }) + + describe('sendAdminContactOtp', () => { + it('should log and send contact OTP when sending has no errors', async () => { + // Act + const actualPromise = SmsService.sendAdminContactOtp( + /* recipient= */ TWILIO_TEST_NUMBER, + /* otp= */ '111111', + /* userId= */ testUser._id, + /* defaultConfig= */ MOCK_VALID_CONFIG, + ) + + // Assert + // Should resolve to true + await expect(actualPromise).resolves.toEqual(true) + // Logging should also have happened. + const expectedLogParams = { + otpData: { + admin: testUser._id, + }, + msgSrvcSid: MOCK_MSG_SRVC_SID, + smsType: SmsType.adminContact, + logType: LogType.success, + } + expect(smsCountSpy).toHaveBeenCalledWith(expectedLogParams) + }) + }) + + it('should log failure and throw error when contact OTP fails to send', async () => { + // Act + const actualPromise = SmsService.sendAdminContactOtp( + /* recipient= */ TWILIO_TEST_NUMBER, + /* otp= */ '111111', + /* userId= */ testUser._id, + /* defaultConfig= */ MOCK_INVALID_CONFIG, + ) + + // Assert + const expectedError = new Error(VfnErrors.InvalidMobileNumber) + expectedError.name = VfnErrors.SendOtpFailed + + await expect(actualPromise).rejects.toThrow(expectedError) + // Logging should also have happened. + const expectedLogParams = { + otpData: { + admin: testUser._id, + }, + msgSrvcSid: MOCK_MSG_SRVC_SID, + smsType: SmsType.adminContact, + logType: LogType.failure, + } + expect(smsCountSpy).toHaveBeenCalledWith(expectedLogParams) + }) }) diff --git a/src/app/services/sms/sms.service.ts b/src/app/services/sms/sms.service.ts index aef1b41aa7..4d7c7f7963 100644 --- a/src/app/services/sms/sms.service.ts +++ b/src/app/services/sms/sms.service.ts @@ -169,6 +169,7 @@ const send = async ( msgSrvcSid, logType: LogType.success, } + SmsCount.logSms(logParams).catch((err) => { logger.error({ message: 'Error logging sms count to database', From 0cc008e3b62496a51ef28f500fc9c434b6af305f Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Tue, 29 Sep 2020 23:08:34 +0800 Subject: [PATCH 09/14] test(SmsFactory): add tests for sms feature disabled state --- .../sms/__tests__/sms.factory.spec.ts | 40 +++++++++++++++++++ src/app/services/sms/sms.factory.ts | 14 +++++-- 2 files changed, 50 insertions(+), 4 deletions(-) create mode 100644 src/app/services/sms/__tests__/sms.factory.spec.ts diff --git a/src/app/services/sms/__tests__/sms.factory.spec.ts b/src/app/services/sms/__tests__/sms.factory.spec.ts new file mode 100644 index 0000000000..56869fbad7 --- /dev/null +++ b/src/app/services/sms/__tests__/sms.factory.spec.ts @@ -0,0 +1,40 @@ +import { + FeatureNames, + ISms, + RegisteredFeature, +} from 'src/config/feature-manager' + +import { createSmsFactory } from '../sms.factory' + +describe('sms.factory', () => { + describe('sms feature disabled', () => { + const MOCK_DISABLED_SMS_FEATURE: RegisteredFeature = { + isEnabled: false, + props: {} as ISms, + } + + const SmsFactory = createSmsFactory(MOCK_DISABLED_SMS_FEATURE) + + it('should throw error when invoking sendAdminContactOtp', () => { + // Act + const invocation = () => + SmsFactory.sendAdminContactOtp('anything', 'anything', 'anything') + + // Assert + expect(invocation).toThrowError( + 'sendAdminContactOtp: SMS feature must be enabled in Feature Manager first', + ) + }) + + it('should throw error when invoking sendVerificationOtp', () => { + // Act + const invocation = () => + SmsFactory.sendVerificationOtp('anything', 'anything', 'anything') + + // Assert + expect(invocation).toThrowError( + 'sendVerificationOtp: SMS feature must be enabled in Feature Manager first', + ) + }) + }) +}) diff --git a/src/app/services/sms/sms.factory.ts b/src/app/services/sms/sms.factory.ts index d4964cca34..430e6767c8 100644 --- a/src/app/services/sms/sms.factory.ts +++ b/src/app/services/sms/sms.factory.ts @@ -1,6 +1,9 @@ import Twilio from 'twilio' -import FeatureManager, { FeatureNames } from '../../../config/feature-manager' +import FeatureManager, { + FeatureNames, + RegisteredFeature, +} from '../../../config/feature-manager' import { sendAdminContactOtp, sendVerificationOtp } from './sms.service' import { TwilioConfig } from './sms.types' @@ -18,9 +21,12 @@ interface ISmsFactory { ) => ReturnType } -const createSmsFactory = (): ISmsFactory => { - const smsFeature = FeatureManager.get(FeatureNames.Sms) +const smsFeature = FeatureManager.get(FeatureNames.Sms) +// Exported for testing. +export const createSmsFactory = ( + smsFeature: RegisteredFeature, +): ISmsFactory => { if (!smsFeature.isEnabled) { const errorMessage = 'SMS feature must be enabled in Feature Manager first' return { @@ -56,4 +62,4 @@ const createSmsFactory = (): ISmsFactory => { } } -export const SmsFactory = createSmsFactory() +export const SmsFactory = createSmsFactory(smsFeature) From 46c5457b85dd3432a2e9112b8d7049c90542ed77 Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Tue, 29 Sep 2020 23:37:47 +0800 Subject: [PATCH 10/14] test(SmsFactory): add tests for sms feature enabled state --- .../sms/__tests__/sms.factory.spec.ts | 87 +++++++++++++++++++ 1 file changed, 87 insertions(+) diff --git a/src/app/services/sms/__tests__/sms.factory.spec.ts b/src/app/services/sms/__tests__/sms.factory.spec.ts index 56869fbad7..89c59e4626 100644 --- a/src/app/services/sms/__tests__/sms.factory.spec.ts +++ b/src/app/services/sms/__tests__/sms.factory.spec.ts @@ -1,3 +1,5 @@ +import Twilio from 'twilio' + import { FeatureNames, ISms, @@ -5,8 +7,23 @@ import { } from 'src/config/feature-manager' import { createSmsFactory } from '../sms.factory' +import * as SmsService from '../sms.service' +import { TwilioConfig } from '../sms.types' + +// This is hoisted and thus a const cannot be passed in. +jest.mock('twilio', () => + jest.fn().mockImplementation(() => ({ + mocked: 'this is mocked', + })), +) + +const MOCKED_TWILIO = ({ + mocked: 'this is mocked', +} as unknown) as Twilio.Twilio describe('sms.factory', () => { + beforeEach(() => jest.clearAllMocks()) + describe('sms feature disabled', () => { const MOCK_DISABLED_SMS_FEATURE: RegisteredFeature = { isEnabled: false, @@ -37,4 +54,74 @@ describe('sms.factory', () => { ) }) }) + + describe('sms feature enabled', () => { + const MOCK_ENABLED_SMS_FEATURE: RegisteredFeature = { + isEnabled: true, + props: { + twilioAccountSid: 'ACrandomTwilioSid', + twilioApiKey: 'SKrandomTwilioAPIKEY', + twilioApiSecret: 'this is a super secret', + twilioMsgSrvcSid: 'formsg-is-great-pleasehelpme', + }, + } + + const SmsFactory = createSmsFactory(MOCK_ENABLED_SMS_FEATURE) + + it('should call SmsService counterpart when invoking sendAdminContactOtp', async () => { + // Arrange + const serviceContactSpy = jest + .spyOn(SmsService, 'sendAdminContactOtp') + .mockResolvedValue(true) + + const mockArguments: Parameters = [ + 'mockRecipient', + 'mockOtp', + 'mockFormId', + ] + + // Act + await SmsFactory.sendAdminContactOtp(...mockArguments) + + // Assert + const expectedTwilioConfig: TwilioConfig = { + msgSrvcSid: MOCK_ENABLED_SMS_FEATURE.props.twilioMsgSrvcSid, + client: MOCKED_TWILIO, + } + + expect(serviceContactSpy).toHaveBeenCalledTimes(1) + expect(serviceContactSpy).toHaveBeenCalledWith( + ...mockArguments, + expectedTwilioConfig, + ) + }) + + it('should call SmsService counterpart when invoking sendVerificationOtp', async () => { + // Arrange + const serviceVfnSpy = jest + .spyOn(SmsService, 'sendVerificationOtp') + .mockResolvedValue(true) + + const mockArguments: Parameters = [ + 'mockRecipient', + 'mockOtp', + 'mockUserId', + ] + + // Act + await SmsFactory.sendVerificationOtp(...mockArguments) + + // Assert + const expectedTwilioConfig: TwilioConfig = { + msgSrvcSid: MOCK_ENABLED_SMS_FEATURE.props.twilioMsgSrvcSid, + client: MOCKED_TWILIO, + } + + expect(serviceVfnSpy).toHaveBeenCalledTimes(1) + expect(serviceVfnSpy).toHaveBeenCalledWith( + ...mockArguments, + expectedTwilioConfig, + ) + }) + }) }) From 641af5ece0e23e72dfe1ad7a5705ebb5624e3c06 Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Fri, 2 Oct 2020 16:24:18 +0800 Subject: [PATCH 11/14] feat(MailService): sendNodeMail now resolves true on successful send --- src/app/services/mail.service.ts | 24 ++-- .../backend/services/mail.service.spec.ts | 106 +++++++----------- 2 files changed, 54 insertions(+), 76 deletions(-) diff --git a/src/app/services/mail.service.ts b/src/app/services/mail.service.ts index e58d7948cc..93aa35f190 100644 --- a/src/app/services/mail.service.ts +++ b/src/app/services/mail.service.ts @@ -107,7 +107,10 @@ export class MailService { * @param mail Mail data to send with * @param sendOptions Extra options to better identify mail, such as form or mail id. */ - #sendNodeMail = async (mail: MailOptions, sendOptions?: SendMailOptions) => { + #sendNodeMail = async ( + mail: MailOptions, + sendOptions?: SendMailOptions, + ): Promise => { const logMeta = { action: '#sendNodeMail', mailId: sendOptions?.mailId, @@ -141,12 +144,12 @@ export class MailService { }) try { - const response = await this.#transporter.sendMail(mail) + await this.#transporter.sendMail(mail) logger.info({ message: `Mail successfully sent on attempt ${attemptNum}`, meta: logMeta, }) - return response + return true } catch (err) { // Pass errors to the callback logger.error({ @@ -182,7 +185,7 @@ export class MailService { form, submission, index, - }: SendSingleAutoreplyMailArgs) => { + }: SendSingleAutoreplyMailArgs): Promise => { const emailSubject = autoReplyMailData.subject || `Thank you for submitting ${form.title}` // Sender's name appearing after "("" symbol gets truncated. Escaping it @@ -229,7 +232,10 @@ export class MailService { * @param otp the otp to send * @throws error if mail fails, to be handled by the caller */ - sendVerificationOtp = async (recipient: string, otp: string) => { + sendVerificationOtp = async ( + recipient: string, + otp: string, + ): Promise => { // TODO(#42): Remove param guards once whole backend is TypeScript. if (!otp) { throw new Error('OTP is missing.') @@ -269,7 +275,7 @@ export class MailService { recipient: string otp: string ipAddress: string - }): ResultAsync => { + }): ResultAsync => { return generateLoginOtpHtml({ appName: this.#appName, appUrl: this.#appUrl, @@ -326,7 +332,7 @@ export class MailService { bounceType: BounceType | undefined formTitle: string formId: string - }) => { + }): Promise => { const htmlData: BounceNotificationHtmlData = { formTitle, formLink: `${this.#appUrl}/${formId}`, @@ -374,7 +380,7 @@ export class MailService { question: string answer: string | number }[] - }) => { + }): Promise => { const refNo = submission.id const formTitle = form.title const submissionTime = moment(submission.created) @@ -447,7 +453,7 @@ export class MailService { responsesData, autoReplyMailDatas, attachments = [], - }: SendAutoReplyEmailsArgs) => { + }: SendAutoReplyEmailsArgs): Promise[]> => { // Data to render both the submission details mail HTML body PDF. const renderData: AutoreplySummaryRenderData = { refNo: submission.id, diff --git a/tests/unit/backend/services/mail.service.spec.ts b/tests/unit/backend/services/mail.service.spec.ts index 2f5811d02e..b517cd0b35 100644 --- a/tests/unit/backend/services/mail.service.spec.ts +++ b/tests/unit/backend/services/mail.service.spec.ts @@ -91,8 +91,7 @@ describe('mail.service', () => { it('should send verification otp successfully', async () => { // Arrange // sendMail should return mocked success response - const mockedResponse = 'mockedSuccessResponse' - sendMailSpy.mockResolvedValueOnce(mockedResponse) + sendMailSpy.mockResolvedValueOnce('mockedSuccessResponse') const expectedArgument = await generateExpectedArg() @@ -103,7 +102,7 @@ describe('mail.service', () => { ) // Assert - await expect(pendingSend).resolves.toEqual(mockedResponse) + await expect(pendingSend).resolves.toEqual(true) // Check arguments passed to sendNodeMail expect(sendMailSpy).toHaveBeenCalledTimes(1) expect(sendMailSpy).toHaveBeenCalledWith(expectedArgument) @@ -126,14 +125,13 @@ describe('mail.service', () => { it('should autoretry when 4xx error is thrown by sendNodeMail and pass if second try passes', async () => { // Arrange // sendMail should return mocked success response - const mockedResponse = 'mockedSuccessResponse' const mock4xxReject = { responseCode: 454, message: 'oh no something went wrong', } sendMailSpy .mockRejectedValueOnce(mock4xxReject) - .mockReturnValueOnce(mockedResponse) + .mockReturnValueOnce('mockedSuccessResponse') const expectedArgument = await generateExpectedArg() @@ -144,7 +142,7 @@ describe('mail.service', () => { ) // Assert - await expect(pendingSendVerification).resolves.toEqual(mockedResponse) + await expect(pendingSendVerification).resolves.toEqual(true) // Check arguments passed to sendNodeMail // Should have been called two times since it rejected the first one and // resolved @@ -232,8 +230,7 @@ describe('mail.service', () => { it('should send login otp successfully', async () => { // Arrange // sendMail should return mocked success response - const mockedResponse = 'mockedSuccessResponse' - sendMailSpy.mockResolvedValueOnce(mockedResponse) + sendMailSpy.mockResolvedValueOnce('mockedSuccessResponse') const expectedArgument = await generateExpectedArg() @@ -246,7 +243,7 @@ describe('mail.service', () => { // Assert expect(actualResult.isOk()).toBe(true) - expect(actualResult._unsafeUnwrap()).toEqual(mockedResponse) + expect(actualResult._unsafeUnwrap()).toEqual(true) // Check arguments passed to sendNodeMail expect(sendMailSpy).toHaveBeenCalledTimes(1) expect(sendMailSpy).toHaveBeenCalledWith(expectedArgument) @@ -270,14 +267,13 @@ describe('mail.service', () => { it('should autoretry when 4xx error is thrown by sendNodeMail and pass if second try passes', async () => { // Arrange // sendMail should return mocked success response - const mockedResponse = 'mockedSuccessResponse' const mock4xxReject = { responseCode: 454, message: 'oh no something went wrong', } sendMailSpy .mockRejectedValueOnce(mock4xxReject) - .mockReturnValueOnce(mockedResponse) + .mockReturnValueOnce('mockedSuccessResponse') const expectedArgument = await generateExpectedArg() @@ -290,7 +286,7 @@ describe('mail.service', () => { // Assert expect(actualResult.isOk()).toBe(true) - expect(actualResult._unsafeUnwrap()).toEqual(mockedResponse) + expect(actualResult._unsafeUnwrap()).toEqual(true) // Check arguments passed to sendNodeMail // Should have been called two times since it rejected the first one and // resolved @@ -430,8 +426,7 @@ describe('mail.service', () => { it('should send submission mail to admin successfully if form.emails is an array with a single string', async () => { // Arrange // sendMail should return mocked success response - const mockedResponse = 'mockedSuccessResponse' - sendMailSpy.mockResolvedValueOnce(mockedResponse) + sendMailSpy.mockResolvedValueOnce('mockedSuccessResponse') const expectedArgument = generateExpectedArgWithToField( MOCK_VALID_SUBMISSION_PARAMS.form.emails, @@ -443,7 +438,7 @@ describe('mail.service', () => { ) // Assert - await expect(pendingSend).resolves.toEqual(mockedResponse) + await expect(pendingSend).resolves.toEqual(true) // Check arguments passed to sendNodeMail expect(sendMailSpy).toHaveBeenCalledTimes(1) expect(sendMailSpy).toHaveBeenCalledWith(expectedArgument) @@ -452,8 +447,7 @@ describe('mail.service', () => { it('should send submission mail to admin successfully if form.emails is an array with a single comma separated string', async () => { // Arrange // sendMail should return mocked success response - const mockedResponse = 'mockedSuccessResponse' - sendMailSpy.mockResolvedValueOnce(mockedResponse) + sendMailSpy.mockResolvedValueOnce('mockedSuccessResponse') const formEmailsCommaSeparated = [ `${MOCK_VALID_EMAIL}, ${MOCK_VALID_EMAIL_2}`, @@ -469,7 +463,7 @@ describe('mail.service', () => { const pendingSend = mailService.sendSubmissionToAdmin(modifiedParams) // Assert - await expect(pendingSend).resolves.toEqual(mockedResponse) + await expect(pendingSend).resolves.toEqual(true) // Check arguments passed to sendNodeMail expect(sendMailSpy).toHaveBeenCalledTimes(1) expect(sendMailSpy).toHaveBeenCalledWith(expectedArgument) @@ -477,8 +471,7 @@ describe('mail.service', () => { it('should send submission mail to admin successfully if form.emails is an array with multiple emails', async () => { // Arrange - const mockedResponse = 'mockedSuccessResponse' - sendMailSpy.mockResolvedValueOnce(mockedResponse) + sendMailSpy.mockResolvedValueOnce('mockedSuccessResponse') const formMultipleEmailsArray = [MOCK_VALID_EMAIL, MOCK_VALID_EMAIL_2] const modifiedParams = cloneDeep(MOCK_VALID_SUBMISSION_PARAMS) @@ -492,7 +485,7 @@ describe('mail.service', () => { const pendingSend = mailService.sendSubmissionToAdmin(modifiedParams) // Assert - await expect(pendingSend).resolves.toEqual(mockedResponse) + await expect(pendingSend).resolves.toEqual(true) // Check arguments passed to sendNodeMail expect(sendMailSpy).toHaveBeenCalledTimes(1) expect(sendMailSpy).toHaveBeenCalledWith(expectedArgument) @@ -500,8 +493,7 @@ describe('mail.service', () => { it('should send submission mail to admin successfully if form.emails is an array with a mixture of emails and comma separated emails strings', async () => { // Arrange - const mockedResponse = 'mockedSuccessResponse' - sendMailSpy.mockResolvedValueOnce(mockedResponse) + sendMailSpy.mockResolvedValueOnce('mockedSuccessResponse') const formEmailsMixture = [ `${MOCK_VALID_EMAIL}, ${MOCK_VALID_EMAIL_2}`, @@ -516,7 +508,7 @@ describe('mail.service', () => { const pendingSend = mailService.sendSubmissionToAdmin(modifiedParams) // Assert - await expect(pendingSend).resolves.toEqual(mockedResponse) + await expect(pendingSend).resolves.toEqual(true) // Check arguments passed to sendNodeMail expect(sendMailSpy).toHaveBeenCalledTimes(1) expect(sendMailSpy).toHaveBeenCalledWith(expectedArgument) @@ -524,8 +516,7 @@ describe('mail.service', () => { it('should send submission mail to admin successfully if form.emails is an array with a mixture of emails, semi-colon, and comma separated emails strings', async () => { // Arrange - const mockedResponse = 'mockedSuccessResponse' - sendMailSpy.mockResolvedValueOnce(mockedResponse) + sendMailSpy.mockResolvedValueOnce('mockedSuccessResponse') const formEmailsMixture = [ `${MOCK_VALID_EMAIL}, ${MOCK_VALID_EMAIL_2}`, @@ -540,7 +531,7 @@ describe('mail.service', () => { const pendingSend = mailService.sendSubmissionToAdmin(modifiedParams) // Assert - await expect(pendingSend).resolves.toEqual(mockedResponse) + await expect(pendingSend).resolves.toEqual(true) // Check arguments passed to sendNodeMail expect(sendMailSpy).toHaveBeenCalledTimes(1) expect(sendMailSpy).toHaveBeenCalledWith(expectedArgument) @@ -576,10 +567,9 @@ describe('mail.service', () => { responseCode: 454, message: 'oh no something went wrong', } - const mockedResponse = 'mockedSuccessResponse' sendMailSpy .mockRejectedValueOnce(mock4xxReject) - .mockReturnValueOnce(mockedResponse) + .mockReturnValueOnce('mockedSuccessResponse') const formEmailsMixture = [ `${MOCK_VALID_EMAIL}, ${MOCK_VALID_EMAIL_2}`, @@ -596,7 +586,7 @@ describe('mail.service', () => { ) // Assert - await expect(pendingSendSubmission).resolves.toEqual(mockedResponse) + await expect(pendingSendSubmission).resolves.toEqual(true) // Check arguments passed to sendNodeMail // Should have been called two times since it rejected the first one and // resolved @@ -731,12 +721,9 @@ describe('mail.service', () => { it('should send single autoreply mail successfully with defaults', async () => { // Arrange - const mockedResponse = 'mockedSuccessResponse' - sendMailSpy.mockResolvedValueOnce(mockedResponse) + sendMailSpy.mockResolvedValueOnce('mockedSuccessResponse') - const expectedResponse = await Promise.allSettled([ - Promise.resolve(mockedResponse), - ]) + const expectedResponse = await Promise.allSettled([Promise.resolve(true)]) // Act const pendingSend = mailService.sendAutoReplyEmails(MOCK_AUTOREPLY_PARAMS) @@ -751,7 +738,7 @@ describe('mail.service', () => { it('should send array of multiple autoreply mails successfully with defaults', async () => { // Arrange const firstMockedResponse = 'mockedSuccessResponse1' - const secondMockedResponse = 'mockedSuccessResponse1' + const secondMockedResponse = 'mockedSuccessResponse2' sendMailSpy .mockResolvedValueOnce(firstMockedResponse) .mockResolvedValueOnce(secondMockedResponse) @@ -765,8 +752,8 @@ describe('mail.service', () => { secondExpectedArg.to = MOCK_VALID_EMAIL_3 const expectedResponse = await Promise.allSettled([ - Promise.resolve(firstMockedResponse), - Promise.resolve(secondMockedResponse), + Promise.resolve(true), + Promise.resolve(true), ]) // Act @@ -783,8 +770,7 @@ describe('mail.service', () => { it('should send single autoreply mail successfully with custom autoreply subject', async () => { // Arrange - const mockedResponse = 'mockedSuccessResponse' - sendMailSpy.mockResolvedValueOnce(mockedResponse) + sendMailSpy.mockResolvedValueOnce('mockedSuccessResponse') const customSubject = 'customSubject' const customDataParams = cloneDeep(MOCK_AUTOREPLY_PARAMS) @@ -792,9 +778,7 @@ describe('mail.service', () => { const expectedArg = { ...defaultExpectedArg, subject: customSubject } - const expectedResponse = await Promise.allSettled([ - Promise.resolve(mockedResponse), - ]) + const expectedResponse = await Promise.allSettled([Promise.resolve(true)]) // Act const pendingSend = mailService.sendAutoReplyEmails(customDataParams) @@ -808,8 +792,7 @@ describe('mail.service', () => { it('should send single autoreply mail successfully with custom autoreply sender', async () => { // Arrange - const mockedResponse = 'mockedSuccessResponse' - sendMailSpy.mockResolvedValueOnce(mockedResponse) + sendMailSpy.mockResolvedValueOnce('mockedSuccessResponse') const customSender = 'customSender@example.com' const customDataParams = cloneDeep(MOCK_AUTOREPLY_PARAMS) @@ -819,9 +802,7 @@ describe('mail.service', () => { ...defaultExpectedArg, from: `${customSender} <${MOCK_SENDER_EMAIL}>`, } - const expectedResponse = await Promise.allSettled([ - Promise.resolve(mockedResponse), - ]) + const expectedResponse = await Promise.allSettled([Promise.resolve(true)]) // Act const pendingSend = mailService.sendAutoReplyEmails(customDataParams) @@ -835,8 +816,7 @@ describe('mail.service', () => { it('should send single autoreply mail with attachment if autoReply.includeFormSummary is true', async () => { // Arrange - const mockedResponse = 'mockedSuccessResponse' - sendMailSpy.mockResolvedValueOnce(mockedResponse) + sendMailSpy.mockResolvedValueOnce('mockedSuccessResponse') const customDataParams = cloneDeep(MOCK_AUTOREPLY_PARAMS) customDataParams.autoReplyMailDatas[0].includeFormSummary = true @@ -867,9 +847,7 @@ describe('mail.service', () => { }, ], } - const expectedResponse = await Promise.allSettled([ - Promise.resolve(mockedResponse), - ]) + const expectedResponse = await Promise.allSettled([Promise.resolve(true)]) // Act const pendingSend = mailService.sendAutoReplyEmails(customDataParams) @@ -903,19 +881,16 @@ describe('mail.service', () => { responseCode: 454, message: 'oh no something went wrong', } - const mockedResponse = 'mockedSuccessResponse' sendMailSpy .mockRejectedValueOnce(mock4xxReject) - .mockReturnValueOnce(mockedResponse) + .mockReturnValueOnce('mockedSuccessResponse') const customSubject = 'customSubject' const customDataParams = cloneDeep(MOCK_AUTOREPLY_PARAMS) customDataParams.autoReplyMailDatas[0].subject = customSubject const expectedArg = { ...defaultExpectedArg, subject: customSubject } - const expectedResponse = await Promise.allSettled([ - Promise.resolve(mockedResponse), - ]) + const expectedResponse = await Promise.allSettled([Promise.resolve(true)]) // Act const pendingSend = mailService.sendAutoReplyEmails(customDataParams) @@ -1024,8 +999,7 @@ describe('mail.service', () => { it('should send permanent bounce notification successfully', async () => { // Arrange // sendMail should return mocked success response - const mockedResponse = 'mockedSuccessResponse' - sendMailSpy.mockResolvedValueOnce(mockedResponse) + sendMailSpy.mockResolvedValueOnce('mockedSuccessResponse') // Act const sent = await mailService.sendBounceNotification({ @@ -1037,7 +1011,7 @@ describe('mail.service', () => { }) const expectedArgs = await generateExpectedArg(BounceType.Permanent) // Assert - expect(sent).toEqual(mockedResponse) + expect(sent).toEqual(true) // Check arguments passed to sendNodeMail expect(sendMailSpy).toHaveBeenCalledTimes(1) expect(sendMailSpy).toHaveBeenCalledWith(expectedArgs) @@ -1046,8 +1020,7 @@ describe('mail.service', () => { it('should send transient bounce notification successfully', async () => { // Arrange // sendMail should return mocked success response - const mockedResponse = 'mockedSuccessResponse' - sendMailSpy.mockResolvedValueOnce(mockedResponse) + sendMailSpy.mockResolvedValueOnce('mockedSuccessResponse') // Act const sent = await mailService.sendBounceNotification({ @@ -1059,7 +1032,7 @@ describe('mail.service', () => { }) const expectedArgs = await generateExpectedArg(BounceType.Transient) // Assert - expect(sent).toEqual(mockedResponse) + expect(sent).toEqual(true) // Check arguments passed to sendNodeMail expect(sendMailSpy).toHaveBeenCalledTimes(1) expect(sendMailSpy).toHaveBeenCalledWith(expectedArgs) @@ -1085,14 +1058,13 @@ describe('mail.service', () => { it('should autoretry when 4xx error is thrown by sendNodeMail and pass if second try passes', async () => { // Arrange // sendMail should return mocked success response - const mockedResponse = 'mockedSuccessResponse' const mock4xxReject = { responseCode: 454, message: 'oh no something went wrong', } sendMailSpy .mockRejectedValueOnce(mock4xxReject) - .mockReturnValueOnce(mockedResponse) + .mockReturnValueOnce('mockedSuccessResponse') // Act const sent = await mailService.sendBounceNotification({ @@ -1105,7 +1077,7 @@ describe('mail.service', () => { const expectedArgs = await generateExpectedArg(MOCK_BOUNCE_TYPE) // Assert - expect(sent).toEqual(mockedResponse) + expect(sent).toEqual(true) // Check arguments passed to sendNodeMail // Should have been called two times since it rejected the first one and // resolved From 91ea92c311090ac9d442f1322c5a57d2f212a491 Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Fri, 2 Oct 2020 16:26:18 +0800 Subject: [PATCH 12/14] feat(VfnService): update sendOTPForField's return type to boolean --- src/app/modules/verification/verification.service.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/modules/verification/verification.service.ts b/src/app/modules/verification/verification.service.ts index 549147dd3f..bf9402c9ef 100644 --- a/src/app/modules/verification/verification.service.ts +++ b/src/app/modules/verification/verification.service.ts @@ -257,7 +257,7 @@ const sendOTPForField = async ( field: IVerificationFieldSchema, recipient: string, otp: string, -): Promise => { +): Promise => { const { fieldType } = field switch (fieldType) { case 'mobile': From 6b519681a30765b424b3ada61531909eb0fe8d09 Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Fri, 2 Oct 2020 16:29:39 +0800 Subject: [PATCH 13/14] feat(FeatureManager): remove usage of non-null assertion --- src/config/feature-manager/util/FeatureManager.class.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/config/feature-manager/util/FeatureManager.class.ts b/src/config/feature-manager/util/FeatureManager.class.ts index e0492ca4fa..77e5f7f146 100644 --- a/src/config/feature-manager/util/FeatureManager.class.ts +++ b/src/config/feature-manager/util/FeatureManager.class.ts @@ -112,7 +112,6 @@ export default class FeatureManager { get(name: K): RegisteredFeature { return { isEnabled: this.isEnabled(name), - // TODO (#317): remove usage of non-null assertion props: this.props(name), } } From d5603c74d8800a4c3b6fa7062fa25ecb773ddaa6 Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Fri, 2 Oct 2020 17:20:17 +0800 Subject: [PATCH 14/14] chore: trigger travis