From 7f730dde785304325123f2e6d7634e40e9a54ec1 Mon Sep 17 00:00:00 2001 From: seaerchin <44049504+seaerchin@users.noreply.github.com> Date: Wed, 14 Jul 2021 10:53:42 +0800 Subject: [PATCH] feat(sms-limiting): sms counts retrieval (#2276) * feat(sms_count.server.model): adds new isOnboardedAcc key and scripts to add/delete the key * refactor(sms.types): adds property that checks if a sms is sent using formsg's credentials * refactor(sms_count.server.model): adds pre-save hook to automatically set property * feat(sms.types): updated typings of IVerificationsmscount * test(sms_count.server): fixed tests and added extra tests for isOnboardedAccount * feat(sms.service): adds new method to retrieve free sms counts of a user * test(sms.service): adds new unit tests for sms count retrieval * test(sms_count.server.model): updated test case for a client using default credentials * feat(sms.controller): added new sms controller iwth method to retrieve counts and msgsrvcid fix(sms.controller.ts): added mapRouteError for sms controller * feat(routes): exposes new route at sms/:userId/:formId * fix(sms.controller): removed sms count retrieval requiring the userId as it sohuld be for the admin * refactor(sms): count retrieval no longer required userId * test(sms.controller): added unit tests for controller method to retrieve counts * fix(sms.controller): changed to relative imports in spec to avoid e2e tests breaking * refactor(sms_count): sms service wraps model count documents call * test(sms.service): reworked tests to account for mocking of db * refactor(sms.controller): removed extra middleware from sms controller * test(sms.controller.spec): renamed sms controller's exposed method in tests * refactor(sms.routes): removed unused sms route * refactor(sms.controller): changed counts retrieval to be solely focused on counts * feat(admin-forms.form.routes): adds new route for retrieving free sms ocunts for admin of a form * refactor(sms_count): shifts default msgsrvcid to be from config, helper method defined in test * test(sms.controller.spec): updated tests to use mocks * test(sms_count.server.model.spec): updated db tests to use smsConfig * fix(v3.routes): removed extra import statement * refactor(types): renamed smscounts meta and shifted it to api renamed to smsCountsDto because it is used to transfer data b/w fe and be. against giving it a more specific name because this is a type signifying a transfer of sms count data. context for what this sms counts actually means should be given by the caller * test(sms_count.server.model.spec): updated test to use sms_config * docs(sms): updated docs for sms counts retrieval in controller and route * test(sms.service.spec): made tests more explicit * refactor(admin-form.controller): shifts over handler for count retrieval from sms to admin-form * test(admin-form.controller.spec): shifts over associated tests * refactor(sms_count.server.model.spec): changed tests structure * chore(sms.util): removed unneeded method * refactor(sms.types): added in docs for retrieveFreeSmsCounts * fix(admin-forms.form.routes): removed erroneous import * fix(sms.service): adds error logging --- .../__tests__/admin-form.controller.spec.ts | 138 ++++++++++++++++++ .../form/admin-form/admin-form.controller.ts | 45 ++++++ .../v3/admin/forms/admin-forms.form.routes.ts | 14 ++ .../sms/__tests__/sms.service.spec.ts | 36 ++++- .../__tests__/sms_count.server.model.spec.ts | 56 ++++++- src/app/services/sms/sms.service.ts | 32 +++- src/app/services/sms/sms.types.ts | 10 +- .../services/sms/sms_count.server.model.ts | 23 +++ 8 files changed, 348 insertions(+), 6 deletions(-) diff --git a/src/app/modules/form/admin-form/__tests__/admin-form.controller.spec.ts b/src/app/modules/form/admin-form/__tests__/admin-form.controller.spec.ts index 05a20e680d..eda333a258 100644 --- a/src/app/modules/form/admin-form/__tests__/admin-form.controller.spec.ts +++ b/src/app/modules/form/admin-form/__tests__/admin-form.controller.spec.ts @@ -77,6 +77,7 @@ import { } from 'tests/unit/backend/helpers/generate-form-data' import expressHandler from 'tests/unit/backend/helpers/jest-express' +import * as SmsService from '../../../../services/sms/sms.service' import * as UserService from '../../../user/user.service' import { ForbiddenFormError, @@ -133,6 +134,8 @@ jest.mock('../../../user/user.service') const MockUserService = mocked(UserService) jest.mock('src/app/services/mail/mail.service') const MockMailService = mocked(MailService) +jest.mock('../../../../services/sms/sms.service') +const MockSmsService = mocked(SmsService) describe('admin-form.controller', () => { beforeEach(() => jest.clearAllMocks()) @@ -9798,4 +9801,139 @@ describe('admin-form.controller', () => { expect(MockAdminFormService.getFormField).not.toHaveBeenCalled() }) }) + + describe('handleGetFreeSmsCountForFormAdmin', () => { + const mockForm = { + admin: new ObjectId().toHexString(), + } as unknown as IFormSchema + const VERIFICATION_SMS_COUNT = 3 + + beforeAll(() => { + MockFormService.retrieveFormById.mockReturnValue(okAsync(mockForm)) + MockSmsService.retrieveFreeSmsCounts.mockReturnValue( + okAsync(VERIFICATION_SMS_COUNT), + ) + }) + + it('should retrieve counts and msgSrvcId when the user and the form exist', async () => { + // Arrange + const MOCK_REQ = expressHandler.mockRequest({ + params: { + formId: mockForm._id, + }, + session: { + user: { + _id: 'exists', + }, + }, + }) + const mockRes = expressHandler.mockResponse() + const expected = VERIFICATION_SMS_COUNT + + // Act + await AdminFormController.handleGetFreeSmsCountForFormAdmin( + MOCK_REQ, + mockRes, + jest.fn(), + ) + + // Assert + expect(mockRes.status).toBeCalledWith(200) + expect(mockRes.json).toBeCalledWith(expected) + }) + + it('should return 404 when the form is not found in the database', async () => { + // Arrange + const MOCK_REQ = expressHandler.mockRequest({ + params: { + formId: new ObjectId().toHexString(), + }, + session: { + user: { + _id: 'exists', + }, + }, + }) + MockFormService.retrieveFormById.mockReturnValueOnce( + errAsync(new FormNotFoundError()), + ) + const mockRes = expressHandler.mockResponse() + const expected = { + message: 'Form not found', + } + + // Act + await AdminFormController.handleGetFreeSmsCountForFormAdmin( + MOCK_REQ, + mockRes, + jest.fn(), + ) + + // Assert + expect(mockRes.status).toBeCalledWith(404) + expect(mockRes.json).toBeCalledWith(expected) + }) + + it('should return 500 when a database error occurs during form retrieval', async () => { + // Arrange + const MOCK_REQ = expressHandler.mockRequest({ + params: { + formId: mockForm._id, + }, + session: { + user: { + _id: 'exists', + }, + }, + }) + const mockRes = expressHandler.mockResponse() + const retrieveSpy = jest.spyOn(FormService, 'retrieveFormById') + retrieveSpy.mockReturnValueOnce(errAsync(new DatabaseError())) + const expected = { + message: 'Something went wrong. Please try again.', + } + + // Act + await AdminFormController.handleGetFreeSmsCountForFormAdmin( + MOCK_REQ, + mockRes, + jest.fn(), + ) + + // Assert + expect(mockRes.status).toBeCalledWith(500) + expect(mockRes.json).toBeCalledWith(expected) + }) + + it('should return 500 when a database error occurs during count retrieval', async () => { + // Arrange + const MOCK_REQ = expressHandler.mockRequest({ + params: { + formId: mockForm._id, + }, + session: { + user: { + _id: 'exists', + }, + }, + }) + const mockRes = expressHandler.mockResponse() + const retrieveSpy = jest.spyOn(SmsService, 'retrieveFreeSmsCounts') + retrieveSpy.mockReturnValueOnce(errAsync(new DatabaseError())) + const expected = { + message: 'Something went wrong. Please try again.', + } + + // Act + await AdminFormController.handleGetFreeSmsCountForFormAdmin( + MOCK_REQ, + mockRes, + jest.fn(), + ) + + // Assert + expect(mockRes.status).toBeCalledWith(500) + expect(mockRes.json).toBeCalledWith(expected) + }) + }) }) diff --git a/src/app/modules/form/admin-form/admin-form.controller.ts b/src/app/modules/form/admin-form/admin-form.controller.ts index e815ab5af4..c43bf6f8c0 100644 --- a/src/app/modules/form/admin-form/admin-form.controller.ts +++ b/src/app/modules/form/admin-form/admin-form.controller.ts @@ -40,6 +40,7 @@ import { } from '../../../../types/api' import { createLoggerWithLabel } from '../../../config/logger' import MailService from '../../../services/mail/mail.service' +import * as SmsService from '../../../services/sms/sms.service' import { createReqMeta } from '../../../utils/request' import * as AuthService from '../../auth/auth.service' import { @@ -2380,3 +2381,47 @@ export const handleUpdateStartPage = [ }), _handleUpdateStartPage, ] as ControllerHandler[] + +/** + * Handler to retrieve the free sms counts used by a form's administrator + * This is the controller for GET /admin/forms/:formId/verified-sms/count/free + * @param formId The id of the form to retrieve the free sms counts for + * @returns 200 with free sms counts when successful + * @returns 404 when the formId is not found in the database + * @returns 500 when a database error occurs during retrieval + */ +export const handleGetFreeSmsCountForFormAdmin: ControllerHandler< + { + formId: string + }, + ErrorDto | number +> = (req, res) => { + const { formId } = req.params + const logMeta = { + action: 'handleGetFreeSmsCountForFormAdmin', + ...createReqMeta(req), + formId, + } + + // Step 1: Check that the form exists + return ( + FormService.retrieveFormById(formId) + // Step 2: Retrieve the free sms count + .andThen(({ admin }) => { + return SmsService.retrieveFreeSmsCounts(String(admin)) + }) + // Step 3: Map/MapErr accordingly + .map((freeSmsCountForAdmin) => + res.status(StatusCodes.OK).json(freeSmsCountForAdmin), + ) + .mapErr((error) => { + logger.error({ + message: 'Error while retrieving sms counts for user', + meta: logMeta, + error, + }) + const { statusCode, errorMessage } = mapRouteError(error) + return res.status(statusCode).json({ message: errorMessage }) + }) + ) +} diff --git a/src/app/routes/api/v3/admin/forms/admin-forms.form.routes.ts b/src/app/routes/api/v3/admin/forms/admin-forms.form.routes.ts index 16e07aa942..3e0b0b2cbd 100644 --- a/src/app/routes/api/v3/admin/forms/admin-forms.form.routes.ts +++ b/src/app/routes/api/v3/admin/forms/admin-forms.form.routes.ts @@ -197,3 +197,17 @@ AdminFormsFormRouter.put( '/:formId([a-fA-F0-9]{24})/start-page', AdminFormController.handleUpdateStartPage, ) + +/** + * Retrieves the free sms counts used by a form's administrator + * @security session + * + * @returns 200 with the free sms counts + * @returns 401 when user does not exist in session + * @returns 404 when the formId is not found in the database + * @returns 500 when a database error occurs during retrieval + */ +AdminFormsFormRouter.get( + '/:formId([a-fA-F0-9]{24})/verified-sms/count/free', + AdminFormController.handleGetFreeSmsCountForFormAdmin, +) diff --git a/src/app/services/sms/__tests__/sms.service.spec.ts b/src/app/services/sms/__tests__/sms.service.spec.ts index 4579a0344c..67323987cc 100644 --- a/src/app/services/sms/__tests__/sms.service.spec.ts +++ b/src/app/services/sms/__tests__/sms.service.spec.ts @@ -2,7 +2,11 @@ import { ObjectId } from 'bson' import mongoose from 'mongoose' import getFormModel from 'src/app/models/form.server.model' -import { MalformedParametersError } from 'src/app/modules/core/core.errors' +import { + DatabaseError, + MalformedParametersError, +} from 'src/app/modules/core/core.errors' +import { getMongoErrorMessage } from 'src/app/utils/handle-mongo-error' import { VfnErrors } from 'src/shared/util/verification' import { FormOtpData, IFormSchema, IUserSchema, ResponseMode } from 'src/types' @@ -351,6 +355,36 @@ describe('sms.service', () => { }) }) + describe('retrieveFreeSmsCounts', () => { + const VERIFICATION_SMS_COUNT = 3 + + it('should retrieve sms counts correctly for a specified user', async () => { + // Arrange + const retrieveSpy = jest.spyOn(SmsCountModel, 'retrieveFreeSmsCounts') + retrieveSpy.mockResolvedValueOnce(VERIFICATION_SMS_COUNT) + + // Act + const actual = await SmsService.retrieveFreeSmsCounts(testUser._id) + + // Assert + expect(actual._unsafeUnwrap()).toBe(VERIFICATION_SMS_COUNT) + }) + + it('should return a database error when retrieval fails', async () => { + // Arrange + const retrieveSpy = jest.spyOn(SmsCountModel, 'retrieveFreeSmsCounts') + retrieveSpy.mockRejectedValueOnce('ohno') + + // Act + const actual = await SmsService.retrieveFreeSmsCounts(testUser._id) + + // Assert + expect(actual._unsafeUnwrapErr()).toEqual( + new DatabaseError(getMongoErrorMessage('ohno')), + ) + }) + }) + it('should log failure and throw error when contact OTP fails to send', async () => { // Act const actualResult = await SmsService.sendAdminContactOtp( diff --git a/src/app/services/sms/__tests__/sms_count.server.model.spec.ts b/src/app/services/sms/__tests__/sms_count.server.model.spec.ts index ad0d66b67f..e599c04c54 100644 --- a/src/app/services/sms/__tests__/sms_count.server.model.spec.ts +++ b/src/app/services/sms/__tests__/sms_count.server.model.spec.ts @@ -5,6 +5,7 @@ import mongoose from 'mongoose' import dbHandler from 'tests/unit/backend/helpers/jest-db' +import { smsConfig } from '../../../config/features/sms.config' import { IVerificationSmsCount, LogType, SmsType } from '../sms.types' import getSmsCountModel from '../sms_count.server.model' @@ -230,9 +231,22 @@ describe('SmsCount', () => { }) describe('VerificationCount Schema', () => { + const twilioMsgSrvcSid = smsConfig.twilioMsgSrvcSid + + beforeAll(() => { + smsConfig.twilioMsgSrvcSid = MOCK_MSG_SRVC_SID + }) + + afterAll(() => { + smsConfig.twilioMsgSrvcSid = twilioMsgSrvcSid + }) + it('should create and save successfully', async () => { // Arrange const smsCountParams = createVerificationSmsCountParams() + const expected = merge(smsCountParams, { + isOnboardedAccount: false, + }) // Act const validSmsCount = new SmsCount(smsCountParams) @@ -249,7 +263,7 @@ describe('SmsCount', () => { 'createdAt', '__v', ]) - expect(actualSavedObject).toEqual(smsCountParams) + expect(actualSavedObject).toEqual(expected) }) it('should save successfully, but not save fields that is not defined in the schema', async () => { @@ -260,6 +274,9 @@ describe('SmsCount', () => { extra: 'somethingExtra', }, ) + const expected = merge(omit(smsCountParamsWithExtra, 'extra'), { + isOnboardedAccount: false, + }) // Act const validSmsCount = new SmsCount(smsCountParamsWithExtra) @@ -278,7 +295,38 @@ describe('SmsCount', () => { 'createdAt', '__v', ]) - expect(actualSavedObject).toEqual(omit(smsCountParamsWithExtra, 'extra')) + expect(actualSavedObject).toEqual(expected) + }) + + it('should save successfully and set isOnboarded to true when the credentials are different from default', async () => { + // Arrange + const verificationParams = merge( + createVerificationSmsCountParams({ + logType: LogType.success, + smsType: SmsType.Verification, + }), + { msgSrvcSid: 'i am different' }, + ) + + // Act + const validSmsCount = new SmsCount(verificationParams) + const saved = await validSmsCount.save() + + // Assert + // All fields should exist + // Object Id should be defined when successfully saved to MongoDB. + expect(saved._id).toBeDefined() + expect(saved.createdAt).toBeInstanceOf(Date) + // Retrieve object and compare to params, remove indeterministic keys + const actualSavedObject = omit(saved.toObject(), [ + '_id', + 'createdAt', + '__v', + ]) + expect(omit(actualSavedObject, 'isOnboardedAccount')).toEqual( + verificationParams, + ) + expect(actualSavedObject.isOnboardedAccount).toBe(true) }) it('should reject if form key is missing', async () => { @@ -566,7 +614,6 @@ const createVerificationSmsCountParams = ({ smsCountParams.logType = logType smsCountParams.smsType = smsType smsCountParams.msgSrvcSid = MOCK_MSG_SRVC_SID - return smsCountParams } @@ -589,6 +636,9 @@ const logAndReturnExpectedLog = async ({ msgSrvcSid: MOCK_MSG_SRVC_SID, smsType, logType, + ...(smsType === SmsType.Verification && { + isOnboardedAccount: !(MOCK_MSG_SRVC_SID === smsConfig.twilioMsgSrvcSid), + }), } return expectedLog diff --git a/src/app/services/sms/sms.service.ts b/src/app/services/sms/sms.service.ts index 489ad34d99..bbf60d6f71 100644 --- a/src/app/services/sms/sms.service.ts +++ b/src/app/services/sms/sms.service.ts @@ -12,8 +12,12 @@ import getFormModel from '../../models/form.server.model' import { DatabaseError, MalformedParametersError, + PossibleDatabaseError, } from '../../modules/core/core.errors' -import { getMongoErrorMessage } from '../../utils/handle-mongo-error' +import { + getMongoErrorMessage, + transformMongoError, +} from '../../utils/handle-mongo-error' import { InvalidNumberError, SmsSendError } from './sms.errors' import { @@ -461,3 +465,29 @@ export const sendBouncedSubmissionSms = ( SmsType.BouncedSubmission, ) } + +/** + * Retrieves the free sms count for a particular user + * @param userId The id of the user to retrieve the sms counts for + * @returns ok(count) when retrieval is successful + * @returns err(error) when retrieval fails due to a database error + */ +export const retrieveFreeSmsCounts = ( + userId: string, +): ResultAsync => { + return ResultAsync.fromPromise( + SmsCount.retrieveFreeSmsCounts(userId), + (error) => { + logger.error({ + message: `Retrieving free sms counts failed for ${userId}`, + meta: { + action: 'retrieveFreeSmsCounts', + userId, + error, + }, + }) + + return transformMongoError(error) + }, + ) +} diff --git a/src/app/services/sms/sms.types.ts b/src/app/services/sms/sms.types.ts index 438837e53f..60fcf49fa5 100644 --- a/src/app/services/sms/sms.types.ts +++ b/src/app/services/sms/sms.types.ts @@ -60,9 +60,12 @@ export interface IVerificationSmsCount extends ISmsCount { email: string userId: IUserSchema['_id'] } + isOnboardedAccount: boolean } -export type IVerificationSmsCountSchema = ISmsCountSchema +export type IVerificationSmsCountSchema = ISmsCountSchema & { + isOnboardedAccount: boolean +} export interface IAdminContactSmsCount extends ISmsCount { admin: IUserSchema['_id'] @@ -88,6 +91,11 @@ export interface IBouncedSubmissionSmsCountSchema export interface ISmsCountModel extends Model { logSms: (logParams: LogSmsParams) => Promise + /** + * Counts the number of sms which an admin has sent using default (formSG) credentials. + * NOTE: This counts across all forms which an admin has. + */ + retrieveFreeSmsCounts: (userId: string) => Promise } export type TwilioCredentials = { diff --git a/src/app/services/sms/sms_count.server.model.ts b/src/app/services/sms/sms_count.server.model.ts index 66d8176f15..c509168b1a 100644 --- a/src/app/services/sms/sms_count.server.model.ts +++ b/src/app/services/sms/sms_count.server.model.ts @@ -2,6 +2,7 @@ import { parsePhoneNumberFromString } from 'libphonenumber-js/mobile' import { Mongoose, Schema } from 'mongoose' import validator from 'validator' +import { smsConfig } from '../../../app/config/features/sms.config' import { FORM_SCHEMA_ID } from '../../models/form.server.model' import { USER_SCHEMA_ID } from '../../models/user.server.model' @@ -34,8 +35,20 @@ const VerificationSmsCountSchema = new Schema({ required: true, }, }, + isOnboardedAccount: { + type: Boolean, + }, }) +VerificationSmsCountSchema.pre( + 'save', + function (next) { + const formTwilioId = smsConfig.twilioMsgSrvcSid + this.isOnboardedAccount = !(this.msgSrvcSid === formTwilioId) + return next() + }, +) + const AdminContactSmsCountSchema = new Schema({ admin: { type: Schema.Types.ObjectId, @@ -125,6 +138,16 @@ const compileSmsCountModel = (db: Mongoose) => { return smsCount.save() } + SmsCountSchema.statics.retrieveFreeSmsCounts = async function ( + userId: string, + ) { + return this.countDocuments({ + 'formAdmin.userId': userId, + smsType: SmsType.Verification, + isOnboardedAccount: false, + }).exec() + } + const SmsCountModel = db.model( SMS_COUNT_SCHEMA_NAME, SmsCountSchema,