Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: migrate SmsFactory to Typescript #387

Merged
merged 14 commits into from
Oct 5, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 0 additions & 48 deletions src/app/factories/sms.factory.js

This file was deleted.

4 changes: 2 additions & 2 deletions src/app/models/form.server.model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ export interface IFormModel extends Model<IFormSchema> {
deactivateById(formId: string): Promise<IFormSchema | null>
}

type IEncryptedFormModel = Model<IEncryptedFormSchema>
type IEncryptedFormModel = Model<IEncryptedFormSchema> & IFormModel

const EncryptedFormSchema = new Schema<IEncryptedFormSchema>({
publicKey: {
Expand All @@ -101,7 +101,7 @@ const EncryptedFormSchema = new Schema<IEncryptedFormSchema>({
},
})

type IEmailFormModel = Model<IEmailFormSchema>
type IEmailFormModel = Model<IEmailFormSchema> & IFormModel

const EmailFormSchema = new Schema<IEmailFormSchema>({
emails: {
Expand Down
2 changes: 1 addition & 1 deletion src/app/modules/user/user.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
3 changes: 1 addition & 2 deletions src/app/modules/verification/verification.factory.ts
Original file line number Diff line number Diff line change
@@ -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'

Expand Down
10 changes: 4 additions & 6 deletions src/app/modules/verification/verification.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down Expand Up @@ -257,17 +257,15 @@ const sendOTPForField = async (
field: IVerificationFieldSchema,
recipient: string,
otp: string,
): Promise<void> => {
): Promise<boolean> => {
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`)
}
Expand Down
24 changes: 15 additions & 9 deletions src/app/services/mail.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<boolean> => {
const logMeta = {
action: '#sendNodeMail',
mailId: sendOptions?.mailId,
Expand Down Expand Up @@ -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({
Expand Down Expand Up @@ -182,7 +185,7 @@ export class MailService {
form,
submission,
index,
}: SendSingleAutoreplyMailArgs) => {
}: SendSingleAutoreplyMailArgs): Promise<boolean> => {
const emailSubject =
autoReplyMailData.subject || `Thank you for submitting ${form.title}`
// Sender's name appearing after "("" symbol gets truncated. Escaping it
Expand Down Expand Up @@ -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<boolean> => {
// TODO(#42): Remove param guards once whole backend is TypeScript.
if (!otp) {
throw new Error('OTP is missing.')
Expand Down Expand Up @@ -269,7 +275,7 @@ export class MailService {
recipient: string
otp: string
ipAddress: string
}): ResultAsync<any, MailSendError> => {
}): ResultAsync<boolean, MailSendError> => {
return generateLoginOtpHtml({
appName: this.#appName,
appUrl: this.#appUrl,
Expand Down Expand Up @@ -326,7 +332,7 @@ export class MailService {
bounceType: BounceType | undefined
formTitle: string
formId: string
}) => {
}): Promise<boolean> => {
const htmlData: BounceNotificationHtmlData = {
formTitle,
formLink: `${this.#appUrl}/${formId}`,
Expand Down Expand Up @@ -374,7 +380,7 @@ export class MailService {
question: string
answer: string | number
}[]
}) => {
}): Promise<boolean> => {
const refNo = submission.id
const formTitle = form.title
const submissionTime = moment(submission.created)
Expand Down Expand Up @@ -447,7 +453,7 @@ export class MailService {
responsesData,
autoReplyMailDatas,
attachments = [],
}: SendAutoReplyEmailsArgs) => {
}: SendAutoReplyEmailsArgs): Promise<PromiseSettledResult<boolean>[]> => {
// Data to render both the submission details mail HTML body PDF.
const renderData: AutoreplySummaryRenderData = {
refNo: submission.id,
Expand Down
127 changes: 127 additions & 0 deletions src/app/services/sms/__tests__/sms.factory.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
import Twilio from 'twilio'

import {
FeatureNames,
ISms,
RegisteredFeature,
} 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<FeatureNames.Sms> = {
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',
)
})
})

describe('sms feature enabled', () => {
const MOCK_ENABLED_SMS_FEATURE: RegisteredFeature<FeatureNames.Sms> = {
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<typeof SmsFactory.sendAdminContactOtp> = [
'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<typeof SmsFactory.sendAdminContactOtp> = [
'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,
)
})
})
})
Loading