From 88535df5b974eec791962bce387592a1832bef5d Mon Sep 17 00:00:00 2001
From: Kar Rui Lau <karrui.lau@gmail.com>
Date: Thu, 17 Sep 2020 00:10:39 +0800
Subject: [PATCH 01/25] chore: add neverthrow package for explicit error
 handling

---
 package-lock.json | 5 +++++
 package.json      | 1 +
 2 files changed, 6 insertions(+)

diff --git a/package-lock.json b/package-lock.json
index 569d1afb3d..7d051a48ac 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -19069,6 +19069,11 @@
       "resolved": "https://registry.npmjs.org/netmask/-/netmask-1.0.6.tgz",
       "integrity": "sha1-ICl+idhvb2QA8lDZ9Pa0wZRfzTU="
     },
+    "neverthrow": {
+      "version": "2.7.1",
+      "resolved": "https://registry.npmjs.org/neverthrow/-/neverthrow-2.7.1.tgz",
+      "integrity": "sha512-tk7vBuxlBcVU6GIXrAOpez2ZpvJxVW1xl2OjVufdVd87g61SjvekY0VBOuTh1EdUf6/gbgQcQujpxXlap0bQ0A=="
+    },
     "ng-infinite-scroll": {
       "version": "1.3.0",
       "resolved": "https://registry.npmjs.org/ng-infinite-scroll/-/ng-infinite-scroll-1.3.0.tgz",
diff --git a/package.json b/package.json
index 0fa33c8b0b..d4b3b2f9e8 100644
--- a/package.json
+++ b/package.json
@@ -135,6 +135,7 @@
     "mongodb-uri": "^0.9.7",
     "mongoose": "^5.10.0",
     "multiparty": ">=4.1.3",
+    "neverthrow": "^2.7.1",
     "ng-infinite-scroll": "^1.3.0",
     "ng-table": "^3.0.1",
     "ngclipboard": "^2.0.0",

From 7795ca1a748aeff88466d62fa7fbe25158345a16 Mon Sep 17 00:00:00 2001
From: Kar Rui Lau <karrui.lau@gmail.com>
Date: Thu, 17 Sep 2020 00:10:48 +0800
Subject: [PATCH 02/25] feat: add core DatabaseError

---
 src/app/modules/core/core.errors.ts | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/app/modules/core/core.errors.ts b/src/app/modules/core/core.errors.ts
index 5c66fdfacb..be485cd06f 100644
--- a/src/app/modules/core/core.errors.ts
+++ b/src/app/modules/core/core.errors.ts
@@ -26,3 +26,9 @@ export abstract class ApplicationError extends Error {
     this.meta = meta
   }
 }
+
+export class DatabaseError extends ApplicationError {
+  constructor(message?: string) {
+    super(message)
+  }
+}

From c2eee88831952c1606a934c9568a73b777ae59ed Mon Sep 17 00:00:00 2001
From: Kar Rui Lau <karrui.lau@gmail.com>
Date: Thu, 17 Sep 2020 00:11:36 +0800
Subject: [PATCH 03/25] refactor(AuthService): rename and convert
 getAgencyWithEmail to Result

---
 src/app/modules/auth/auth.middlewares.ts | 47 ++++++++++++++---------
 src/app/modules/auth/auth.service.ts     | 48 ++++++++++++++++++++----
 2 files changed, 70 insertions(+), 25 deletions(-)

diff --git a/src/app/modules/auth/auth.middlewares.ts b/src/app/modules/auth/auth.middlewares.ts
index 7a19f652e8..9ec00dd5a4 100644
--- a/src/app/modules/auth/auth.middlewares.ts
+++ b/src/app/modules/auth/auth.middlewares.ts
@@ -1,17 +1,36 @@
-import to from 'await-to-js'
 import { RequestHandler } from 'express'
 import { ParamsDictionary } from 'express-serve-static-core'
 import { StatusCodes } from 'http-status-codes'
 
 import { createLoggerWithLabel } from '../../../config/logger'
-import { LINKS } from '../../../shared/constants'
 import { getRequestIp } from '../../utils/request'
-import { ApplicationError } from '../core/core.errors'
+import { ApplicationError, DatabaseError } from '../core/core.errors'
 
+import { InvalidDomainError } from './auth.errors'
 import * as AuthService from './auth.service'
 
 const logger = createLoggerWithLabel(module)
 
+const mapRouteError = (error: ApplicationError) => {
+  switch (error.constructor) {
+    case InvalidDomainError:
+      return {
+        statusCode: StatusCodes.UNAUTHORIZED,
+        errorMessage: error.message,
+      }
+    case DatabaseError:
+      return {
+        statusCode: StatusCodes.INTERNAL_SERVER_ERROR,
+        errorMessage: error.message,
+      }
+    default:
+      return {
+        statusCode: StatusCodes.INTERNAL_SERVER_ERROR,
+        errorMessage: 'Something went wrong. Please try again.',
+      }
+  }
+}
+
 /**
  * Middleware to check if domain of email in the body is from a whitelisted
  * agency.
@@ -27,11 +46,10 @@ export const validateDomain: RequestHandler<
   // Joi validation ensures existence.
   const { email } = req.body
 
-  const [validationError, agency] = await to(
-    AuthService.getAgencyWithEmail(email),
-  )
+  const validateResult = await AuthService.validateEmailDomain(email)
 
-  if (validationError) {
+  if (validateResult.isErr()) {
+    const { error } = validateResult
     logger.error({
       message: 'Domain validation error',
       meta: {
@@ -39,20 +57,13 @@ export const validateDomain: RequestHandler<
         ip: getRequestIp(req),
         email,
       },
-      error: validationError,
+      error,
     })
-    if (validationError instanceof ApplicationError) {
-      return res.status(validationError.status).send(validationError.message)
-    }
-    return res
-      .status(StatusCodes.INTERNAL_SERVER_ERROR)
-      .send(
-        `Unable to validate email domain. If this issue persists, please submit a Support Form at (${LINKS.supportFormLink}).`,
-      )
+    const { errorMessage, statusCode } = mapRouteError(error)
+    return res.status(statusCode).send(errorMessage)
   }
-
   // Pass down agency to next handler.
-  res.locals.agency = agency
+  res.locals.agency = validateResult.value
 
   return next()
 }
diff --git a/src/app/modules/auth/auth.service.ts b/src/app/modules/auth/auth.service.ts
index ac6d34f4b6..349e140fed 100644
--- a/src/app/modules/auth/auth.service.ts
+++ b/src/app/modules/auth/auth.service.ts
@@ -1,14 +1,21 @@
 import bcrypt from 'bcrypt'
 import mongoose from 'mongoose'
+import { err, ok, Result } from 'neverthrow'
 import validator from 'validator'
 
+import { IAgencySchema } from 'src/types'
+
 import config from '../../../config/config'
+import { createLoggerWithLabel } from '../../../config/logger'
+import { LINKS } from '../../../shared/constants'
 import getAgencyModel from '../../models/agency.server.model'
 import getTokenModel from '../../models/token.server.model'
 import { generateOtp } from '../../utils/otp'
+import { DatabaseError } from '../core/core.errors'
 
 import { InvalidDomainError, InvalidOtpError } from './auth.errors'
 
+const logger = createLoggerWithLabel(module)
 const TokenModel = getTokenModel(mongoose)
 const AgencyModel = getAgencyModel(mongoose)
 
@@ -22,19 +29,46 @@ export const MAX_OTP_ATTEMPTS = 10
  * @returns the agency document with the domain of the email only if it is valid.
  * @throws error if database query fails or if agency cannot be found.
  */
-export const getAgencyWithEmail = async (email: string) => {
+export const validateEmailDomain = async (
+  email: string,
+): Promise<Result<IAgencySchema, InvalidDomainError | DatabaseError>> => {
   // Extra guard even if Joi validation has already checked.
   if (!validator.isEmail(email)) {
-    throw new InvalidDomainError()
+    return err(new InvalidDomainError())
   }
 
   const emailDomain = email.split('@').pop()
-  const agency = await AgencyModel.findOne({ emailDomain })
-  if (!agency) {
-    throw new InvalidDomainError()
+  try {
+    const agency = await AgencyModel.findOne({ emailDomain })
+    if (!agency) {
+      const error = new InvalidDomainError()
+      logger.warn({
+        message: 'Agency not found',
+        meta: {
+          action: 'retrieveAgency',
+          emailDomain,
+        },
+        error,
+      })
+      return err(error)
+    }
+    return ok(agency)
+  } catch (error) {
+    logger.error({
+      message: 'Database error when retrieving agency',
+      meta: {
+        action: 'retrieveAgency',
+        emailDomain,
+      },
+      error,
+    })
+
+    return err(
+      new DatabaseError(
+        `Unable to validate email domain. If this issue persists, please submit a Support Form at (${LINKS.supportFormLink})`,
+      ),
+    )
   }
-
-  return agency
 }
 
 /**

From 2f5cc3cab3e5d16dbe25d263ba67bb9c09328aae Mon Sep 17 00:00:00 2001
From: Kar Rui Lau <karrui.lau@gmail.com>
Date: Thu, 17 Sep 2020 00:11:59 +0800
Subject: [PATCH 04/25] test(Auth): update tests to use new validateEmailDomain
 function

---
 .../auth/__tests__/auth.middlewares.spec.ts   | 32 ++++-------------
 .../auth/__tests__/auth.routes.spec.ts        | 35 +++++++++----------
 .../auth/__tests__/auth.service.spec.ts       | 23 +++++++-----
 3 files changed, 38 insertions(+), 52 deletions(-)

diff --git a/src/app/modules/auth/__tests__/auth.middlewares.spec.ts b/src/app/modules/auth/__tests__/auth.middlewares.spec.ts
index 4c3bb824ba..62e0c48afc 100644
--- a/src/app/modules/auth/__tests__/auth.middlewares.spec.ts
+++ b/src/app/modules/auth/__tests__/auth.middlewares.spec.ts
@@ -1,3 +1,4 @@
+import { err, ok } from 'neverthrow'
 import expressHandler from 'tests/unit/backend/helpers/jest-express'
 import { mocked } from 'ts-jest/utils'
 
@@ -20,8 +21,8 @@ describe('auth.middleware', () => {
       // Arrange
       const mockRes = expressHandler.mockResponse()
       const mockNext = jest.fn()
-      MockAuthService.getAgencyWithEmail.mockResolvedValueOnce(
-        {} as IAgencySchema,
+      MockAuthService.validateEmailDomain.mockResolvedValueOnce(
+        ok(<IAgencySchema>{}),
       )
 
       // Act
@@ -31,33 +32,14 @@ describe('auth.middleware', () => {
       expect(mockNext).toBeCalled()
     })
 
-    it('should return 500 when retrieving agency throws non ApplicationError', async () => {
-      // Arrange
-      const mockRes = expressHandler.mockResponse()
-      const mockNext = jest.fn()
-      MockAuthService.getAgencyWithEmail.mockRejectedValueOnce(
-        new Error('some error'),
-      )
-
-      // Act
-      await AuthMiddleware.validateDomain(MOCK_REQ, mockRes, mockNext)
-
-      // Assert
-      expect(mockRes.status).toBeCalledWith(500)
-      expect(mockRes.send).toBeCalledWith(
-        expect.stringContaining(
-          'Unable to validate email domain. If this issue persists, please submit a Support Form',
-        ),
-      )
-      expect(mockNext).not.toBeCalled()
-    })
-
-    it('should return with ApplicationError status and message when retrieving agency throws ApplicationError', async () => {
+    it('should return with ApplicationError status and message when retrieving agency returns an ApplicationError', async () => {
       // Arrange
       const expectedError = new InvalidDomainError()
       const mockRes = expressHandler.mockResponse()
       const mockNext = jest.fn()
-      MockAuthService.getAgencyWithEmail.mockRejectedValueOnce(expectedError)
+      MockAuthService.validateEmailDomain.mockResolvedValueOnce(
+        err(expectedError),
+      )
 
       // Act
       await AuthMiddleware.validateDomain(MOCK_REQ, mockRes, mockNext)
diff --git a/src/app/modules/auth/__tests__/auth.routes.spec.ts b/src/app/modules/auth/__tests__/auth.routes.spec.ts
index b6606de02e..349024cc94 100644
--- a/src/app/modules/auth/__tests__/auth.routes.spec.ts
+++ b/src/app/modules/auth/__tests__/auth.routes.spec.ts
@@ -1,4 +1,5 @@
 import { pick } from 'lodash'
+import { err } from 'neverthrow'
 import supertest from 'supertest'
 import { CookieStore, setupApp } from 'tests/integration/helpers/express-setup'
 import dbHandler from 'tests/unit/backend/helpers/jest-db'
@@ -8,6 +9,7 @@ import MailService from 'src/app/services/mail.service'
 import * as OtpUtils from 'src/app/utils/otp'
 import { IAgencySchema } from 'src/types'
 
+import { DatabaseError } from '../../core/core.errors'
 import * as UserService from '../../user/user.service'
 import { AuthRouter } from '../auth.routes'
 import * as AuthService from '../auth.service'
@@ -82,16 +84,17 @@ describe('auth.routes', () => {
       expect(response.text).toEqual('OK')
     })
 
-    it('should return 500 when validating domain throws an unknown error', async () => {
+    it('should return 500 when validating domain returns a database error', async () => {
       // Arrange
       // Insert agency
       const validDomain = 'example.com'
       const validEmail = `test@${validDomain}`
+      const mockErrorString = 'Unable to validate email domain.'
       await dbHandler.insertDefaultAgency({ mailDomain: validDomain })
 
       const getAgencySpy = jest
-        .spyOn(AuthService, 'getAgencyWithEmail')
-        .mockRejectedValueOnce(new Error('some error occured'))
+        .spyOn(AuthService, 'validateEmailDomain')
+        .mockResolvedValueOnce(err(new DatabaseError(mockErrorString)))
 
       // Act
       const response = await request
@@ -101,9 +104,7 @@ describe('auth.routes', () => {
       // Assert
       expect(getAgencySpy).toBeCalled()
       expect(response.status).toEqual(500)
-      expect(response.text).toEqual(
-        expect.stringContaining('Unable to validate email domain.'),
-      )
+      expect(response.text).toEqual(mockErrorString)
     })
   })
 
@@ -194,11 +195,12 @@ describe('auth.routes', () => {
       )
     })
 
-    it('should return 500 when validating domain throws an unknown error', async () => {
+    it('should return 500 when validating domain returns a database error', async () => {
       // Arrange
+      const mockErrorString = 'Unable to validate email domain.'
       const getAgencySpy = jest
-        .spyOn(AuthService, 'getAgencyWithEmail')
-        .mockRejectedValueOnce(new Error('some error occured'))
+        .spyOn(AuthService, 'validateEmailDomain')
+        .mockResolvedValueOnce(err(new DatabaseError(mockErrorString)))
 
       // Act
       const response = await request
@@ -208,9 +210,7 @@ describe('auth.routes', () => {
       // Assert
       expect(getAgencySpy).toBeCalled()
       expect(response.status).toEqual(500)
-      expect(response.text).toEqual(
-        expect.stringContaining('Unable to validate email domain.'),
-      )
+      expect(response.text).toEqual(mockErrorString)
     })
 
     it('should return 200 when otp is sent successfully', async () => {
@@ -323,11 +323,12 @@ describe('auth.routes', () => {
       )
     })
 
-    it('should return 500 when validating domain throws an unknown error', async () => {
+    it('should return 500 when validating domain returns a database error', async () => {
       // Arrange
+      const mockErrorString = 'Unable to validate email domain.'
       const getAgencySpy = jest
-        .spyOn(AuthService, 'getAgencyWithEmail')
-        .mockRejectedValueOnce(new Error('some error occured'))
+        .spyOn(AuthService, 'validateEmailDomain')
+        .mockResolvedValueOnce(err(new DatabaseError(mockErrorString)))
 
       // Act
       const response = await request
@@ -337,9 +338,7 @@ describe('auth.routes', () => {
       // Assert
       expect(getAgencySpy).toBeCalled()
       expect(response.status).toEqual(500)
-      expect(response.text).toEqual(
-        expect.stringContaining('Unable to validate email domain.'),
-      )
+      expect(response.text).toEqual(mockErrorString)
     })
 
     it('should return 422 when hash does not exist for body.otp', async () => {
diff --git a/src/app/modules/auth/__tests__/auth.service.spec.ts b/src/app/modules/auth/__tests__/auth.service.spec.ts
index 540e0ed17d..e578b8425f 100644
--- a/src/app/modules/auth/__tests__/auth.service.spec.ts
+++ b/src/app/modules/auth/__tests__/auth.service.spec.ts
@@ -36,35 +36,40 @@ describe('auth.service', () => {
 
   afterAll(async () => await dbHandler.closeDatabase())
 
-  describe('getAgencyWithEmail', () => {
+  describe('validateEmailDomain', () => {
     it('should retrieve agency successfully when email is valid and domain is in Agency collection', async () => {
       // Act
-      const actual = await AuthService.getAgencyWithEmail(VALID_EMAIL)
+      const actual = await AuthService.validateEmailDomain(VALID_EMAIL)
 
       // Assert
-      expect(actual.toObject()).toEqual(defaultAgency.toObject())
+      expect(actual.isOk()).toBe(true)
+      expect(actual._unsafeUnwrap().toObject()).toEqual(
+        defaultAgency.toObject(),
+      )
     })
 
-    it('should throw InvalidDomainError when email is invalid', async () => {
+    it('should return InvalidDomainError error result when email is invalid', async () => {
       // Arrange
       const notAnEmail = 'not an email'
 
       // Act
-      const actualPromise = AuthService.getAgencyWithEmail(notAnEmail)
+      const actual = await AuthService.validateEmailDomain(notAnEmail)
 
       // Assert
-      await expect(actualPromise).rejects.toThrowError(InvalidDomainError)
+      expect(actual.isErr()).toBe(true)
+      expect(actual._unsafeUnwrapErr()).toEqual(new InvalidDomainError())
     })
 
-    it('should throw InvalidDomainError when valid email domain is not in Agency collection', async () => {
+    it('should return InvalidDomainError error result when valid email domain is not in Agency collection', async () => {
       // Arrange
       const invalidEmail = 'invalid@example.com'
 
       // Act
-      const actualPromise = AuthService.getAgencyWithEmail(invalidEmail)
+      const actual = await AuthService.validateEmailDomain(invalidEmail)
 
       // Assert
-      await expect(actualPromise).rejects.toThrowError(InvalidDomainError)
+      expect(actual.isErr()).toBe(true)
+      expect(actual._unsafeUnwrapErr()).toEqual(new InvalidDomainError())
     })
   })
 

From 104a08b8f769e1a7c827acd27d869205a951484b Mon Sep 17 00:00:00 2001
From: Kar Rui Lau <karrui.lau@gmail.com>
Date: Thu, 17 Sep 2020 00:24:31 +0800
Subject: [PATCH 05/25] refactor(Auth): inline validate middleware into
 /checkuser endpoint

---
 .../auth/__tests__/auth.controller.spec.ts    | 34 +++++++---
 src/app/modules/auth/auth.controller.ts       | 63 ++++++++++++++++---
 src/app/modules/auth/auth.routes.ts           |  1 -
 3 files changed, 82 insertions(+), 16 deletions(-)

diff --git a/src/app/modules/auth/__tests__/auth.controller.spec.ts b/src/app/modules/auth/__tests__/auth.controller.spec.ts
index 21ba412ef6..65f866b9be 100644
--- a/src/app/modules/auth/__tests__/auth.controller.spec.ts
+++ b/src/app/modules/auth/__tests__/auth.controller.spec.ts
@@ -1,3 +1,4 @@
+import { err, ok } from 'neverthrow'
 import expressHandler from 'tests/unit/backend/helpers/jest-express'
 import { mocked } from 'ts-jest/utils'
 
@@ -6,7 +7,7 @@ import { IAgencySchema, IUserSchema } from 'src/types'
 
 import * as UserService from '../../user/user.service'
 import * as AuthController from '../auth.controller'
-import { InvalidOtpError } from '../auth.errors'
+import { InvalidDomainError, InvalidOtpError } from '../auth.errors'
 import * as AuthService from '../auth.service'
 
 const VALID_EMAIL = 'test@example.com'
@@ -25,20 +26,39 @@ describe('auth.controller', () => {
   })
 
   describe('handleCheckUser', () => {
-    it('should return 200', async () => {
+    const MOCK_REQ = expressHandler.mockRequest({
+      body: { email: 'test@example.com' },
+    })
+
+    it('should return 200 when domain is valid', async () => {
       // Arrange
       const mockRes = expressHandler.mockResponse()
+      MockAuthService.validateEmailDomain.mockResolvedValueOnce(
+        ok(<IAgencySchema>{}),
+      )
 
       // Act
-      await AuthController.handleCheckUser(
-        expressHandler.mockRequest(),
-        mockRes,
-        jest.fn(),
-      )
+      await AuthController.handleCheckUser(MOCK_REQ, mockRes, jest.fn())
 
       // Assert
       expect(mockRes.sendStatus).toBeCalledWith(200)
     })
+
+    it('should return with ApplicationError status and message when retrieving agency returns an ApplicationError', async () => {
+      // Arrange
+      const expectedError = new InvalidDomainError()
+      const mockRes = expressHandler.mockResponse()
+      MockAuthService.validateEmailDomain.mockResolvedValueOnce(
+        err(expectedError),
+      )
+
+      // Act
+      await AuthController.handleCheckUser(MOCK_REQ, mockRes, jest.fn())
+
+      // Assert
+      expect(mockRes.status).toBeCalledWith(expectedError.status)
+      expect(mockRes.send).toBeCalledWith(expectedError.message)
+    })
   })
 
   describe('handleLoginSendOtp', () => {
diff --git a/src/app/modules/auth/auth.controller.ts b/src/app/modules/auth/auth.controller.ts
index 4fa51633a4..c4ead7e360 100644
--- a/src/app/modules/auth/auth.controller.ts
+++ b/src/app/modules/auth/auth.controller.ts
@@ -8,23 +8,70 @@ import { createLoggerWithLabel } from '../../../config/logger'
 import { LINKS } from '../../../shared/constants'
 import MailService from '../../services/mail.service'
 import { getRequestIp } from '../../utils/request'
-import { ApplicationError } from '../core/core.errors'
+import { ApplicationError, DatabaseError } from '../core/core.errors'
 import * as UserService from '../user/user.service'
 
+import { InvalidDomainError } from './auth.errors'
 import * as AuthService from './auth.service'
 import { ResponseAfter, SessionUser } from './auth.types'
 
 const logger = createLoggerWithLabel(module)
 
 /**
- * Precondition: AuthMiddlewares.validateDomain must precede this handler.
- * @returns 200 regardless, assumed to have passed domain validation.
+ * Handler to map ApplicationErrors to their correct status code and error
+ * messages.
+ * @param error The error to retrieve the status codes and error messages
  */
-export const handleCheckUser: RequestHandler = async (
-  _req: Request,
-  res: ResponseAfter['validateDomain'],
-) => {
-  return res.sendStatus(StatusCodes.OK)
+const mapRouteError = (error: ApplicationError) => {
+  switch (error.constructor) {
+    case InvalidDomainError:
+      return {
+        statusCode: StatusCodes.UNAUTHORIZED,
+        errorMessage: error.message,
+      }
+    case DatabaseError:
+      return {
+        statusCode: StatusCodes.INTERNAL_SERVER_ERROR,
+        errorMessage: error.message,
+      }
+    default:
+      return {
+        statusCode: StatusCodes.INTERNAL_SERVER_ERROR,
+        errorMessage: 'Something went wrong. Please try again.',
+      }
+  }
+}
+
+/**
+ * Handler for GET /auth/checkuser endpoint.
+ * @returns 500 when there was an error validating body.email
+ * @returns 401 when domain of body.email is invalid
+ * @returns 200 if domain of body.email is valid
+ */
+export const handleCheckUser: RequestHandler<
+  ParamsDictionary,
+  string,
+  { email: string }
+> = async (req, res) => {
+  // Joi validation ensures existence.
+  const { email } = req.body
+
+  const validateResult = await AuthService.validateEmailDomain(email)
+  validateResult
+    .map(() => res.sendStatus(StatusCodes.OK))
+    .mapErr((error) => {
+      logger.error({
+        message: 'Domain validation error',
+        meta: {
+          action: 'handleCheckUser',
+          ip: getRequestIp(req),
+          email,
+        },
+        error,
+      })
+      const { errorMessage, statusCode } = mapRouteError(error)
+      return res.status(statusCode).send(errorMessage)
+    })
 }
 
 /**
diff --git a/src/app/modules/auth/auth.routes.ts b/src/app/modules/auth/auth.routes.ts
index 8584040a04..729a1b62c9 100644
--- a/src/app/modules/auth/auth.routes.ts
+++ b/src/app/modules/auth/auth.routes.ts
@@ -24,7 +24,6 @@ AuthRouter.post(
         .message('Please enter a valid email'),
     }),
   }),
-  AuthMiddlewares.validateDomain,
   AuthController.handleCheckUser,
 )
 

From 6b2d3a287a2b519ab0ffa0ed1de56a5a7e21b185 Mon Sep 17 00:00:00 2001
From: Kar Rui Lau <karrui.lau@gmail.com>
Date: Thu, 17 Sep 2020 00:42:34 +0800
Subject: [PATCH 06/25] refactor(Auth): inline validate middleware into
 /sendotp endpoint

---
 .../auth/__tests__/auth.controller.spec.ts    | 31 ++++++++++++++++---
 src/app/modules/auth/auth.controller.ts       | 30 +++++++++++++++---
 src/app/modules/auth/auth.routes.ts           |  1 -
 3 files changed, 52 insertions(+), 10 deletions(-)

diff --git a/src/app/modules/auth/__tests__/auth.controller.spec.ts b/src/app/modules/auth/__tests__/auth.controller.spec.ts
index 65f866b9be..efb7218e62 100644
--- a/src/app/modules/auth/__tests__/auth.controller.spec.ts
+++ b/src/app/modules/auth/__tests__/auth.controller.spec.ts
@@ -71,6 +71,9 @@ describe('auth.controller', () => {
       // Arrange
       const mockRes = expressHandler.mockResponse()
       // Mock AuthService and MailService to return without errors
+      MockAuthService.validateEmailDomain.mockResolvedValueOnce(
+        ok(<IAgencySchema>{}),
+      )
       MockAuthService.createLoginOtp.mockResolvedValueOnce(MOCK_OTP)
       MockMailService.sendLoginOtp.mockResolvedValueOnce(true)
 
@@ -85,9 +88,28 @@ describe('auth.controller', () => {
       expect(MockMailService.sendLoginOtp).toHaveBeenCalledTimes(1)
     })
 
+    it('should return with ApplicationError status and message when retrieving agency returns an ApplicationError', async () => {
+      // Arrange
+      const expectedError = new InvalidDomainError()
+      const mockRes = expressHandler.mockResponse()
+      MockAuthService.validateEmailDomain.mockResolvedValueOnce(
+        err(expectedError),
+      )
+
+      // Act
+      await AuthController.handleLoginSendOtp(MOCK_REQ, mockRes, jest.fn())
+
+      // Assert
+      expect(mockRes.status).toBeCalledWith(expectedError.status)
+      expect(mockRes.send).toBeCalledWith(expectedError.message)
+    })
+
     it('should return 500 when there is an error generating login OTP', async () => {
       // Arrange
       const mockRes = expressHandler.mockResponse()
+      MockAuthService.validateEmailDomain.mockResolvedValueOnce(
+        ok(<IAgencySchema>{}),
+      )
       // Mock createLoginOtp failure
       MockAuthService.createLoginOtp.mockRejectedValueOnce(
         new Error('otp creation error'),
@@ -109,6 +131,9 @@ describe('auth.controller', () => {
     it('should return 500 when there is an error sending login OTP', async () => {
       // Arrange
       const mockRes = expressHandler.mockResponse()
+      MockAuthService.validateEmailDomain.mockResolvedValueOnce(
+        ok(<IAgencySchema>{}),
+      )
       // Mock createLoginOtp success but sendLoginOtp failure.
       MockAuthService.createLoginOtp.mockResolvedValueOnce(MOCK_OTP)
       MockMailService.sendLoginOtp.mockRejectedValueOnce(
@@ -142,12 +167,10 @@ describe('auth.controller', () => {
       const mockUser = {
         toObject: () => ({ id: 'imagine this is a user document from the db' }),
       } as IUserSchema
-      // Add agency into locals due to precondition.
-      const mockRes = expressHandler.mockResponse({
-        locals: { agency: MOCK_AGENCY },
-      })
+      const mockRes = expressHandler.mockResponse()
 
       // Mock all service success.
+      MockAuthService.validateEmailDomain.mockResolvedValueOnce(ok(MOCK_AGENCY))
       MockAuthService.verifyLoginOtp.mockResolvedValueOnce(true)
       MockUserService.retrieveUser.mockResolvedValueOnce(mockUser)
 
diff --git a/src/app/modules/auth/auth.controller.ts b/src/app/modules/auth/auth.controller.ts
index c4ead7e360..02e5087902 100644
--- a/src/app/modules/auth/auth.controller.ts
+++ b/src/app/modules/auth/auth.controller.ts
@@ -75,12 +75,16 @@ export const handleCheckUser: RequestHandler<
 }
 
 /**
- * Precondition: AuthMiddlewares.validateDomain must precede this handler.
+ * Handler for POST /auth/sendotp endpoint.
+ * @return 200 when OTP has been been successfully sent
+ * @return 401 when email domain is invalid
+ * @return 500 when unknown errors occurs during generate OTP, or create/send the email that delivers the OTP to the user's email address
  */
-export const handleLoginSendOtp: RequestHandler = async (
-  req: Request<ParamsDictionary, string, { email: string }>,
-  res: ResponseAfter['validateDomain'],
-) => {
+export const handleLoginSendOtp: RequestHandler<
+  ParamsDictionary,
+  string,
+  { email: string }
+> = async (req, res) => {
   // Joi validation ensures existence.
   const { email } = req.body
   const requestIp = getRequestIp(req)
@@ -90,6 +94,22 @@ export const handleLoginSendOtp: RequestHandler = async (
     ip: requestIp,
   }
 
+  const validateResult = await AuthService.validateEmailDomain(email)
+  if (validateResult.isErr()) {
+    const { error } = validateResult
+    logger.error({
+      message: 'Domain validation error',
+      meta: {
+        action: 'handleLoginSendOtp',
+        ip: getRequestIp(req),
+        email,
+      },
+      error,
+    })
+    const { errorMessage, statusCode } = mapRouteError(error)
+    return res.status(statusCode).send(errorMessage)
+  }
+
   // Create OTP.
   const [otpErr, otp] = await to(AuthService.createLoginOtp(email))
 
diff --git a/src/app/modules/auth/auth.routes.ts b/src/app/modules/auth/auth.routes.ts
index 729a1b62c9..16a07a5719 100644
--- a/src/app/modules/auth/auth.routes.ts
+++ b/src/app/modules/auth/auth.routes.ts
@@ -49,7 +49,6 @@ AuthRouter.post(
         .message('Please enter a valid email'),
     }),
   }),
-  AuthMiddlewares.validateDomain,
   AuthController.handleLoginSendOtp,
 )
 

From cc567fff6412eb0b514ccea27d267e7875c563fd Mon Sep 17 00:00:00 2001
From: Kar Rui Lau <karrui.lau@gmail.com>
Date: Thu, 17 Sep 2020 00:51:16 +0800
Subject: [PATCH 07/25] refactor(Auth): inline validate middleware into
 /verifyotp endpoint

---
 .../auth/__tests__/auth.controller.spec.ts    | 34 +++++++++-----
 src/app/modules/auth/auth.controller.ts       | 44 +++++++++++++------
 src/app/modules/auth/auth.routes.ts           |  1 -
 3 files changed, 53 insertions(+), 26 deletions(-)

diff --git a/src/app/modules/auth/__tests__/auth.controller.spec.ts b/src/app/modules/auth/__tests__/auth.controller.spec.ts
index efb7218e62..3558883bec 100644
--- a/src/app/modules/auth/__tests__/auth.controller.spec.ts
+++ b/src/app/modules/auth/__tests__/auth.controller.spec.ts
@@ -185,13 +185,27 @@ describe('auth.controller', () => {
       })
     })
 
+    it('should return with ApplicationError status and message when retrieving agency returns an ApplicationError', async () => {
+      // Arrange
+      const expectedError = new InvalidDomainError()
+      const mockRes = expressHandler.mockResponse()
+      MockAuthService.validateEmailDomain.mockResolvedValueOnce(
+        err(expectedError),
+      )
+
+      // Act
+      await AuthController.handleLoginVerifyOtp(MOCK_REQ, mockRes, jest.fn())
+
+      // Assert
+      expect(mockRes.status).toBeCalledWith(expectedError.status)
+      expect(mockRes.send).toBeCalledWith(expectedError.message)
+    })
+
     it('should return 422 when verifying login OTP throws an InvalidOtpError', async () => {
       // Arrange
-      // Add agency into locals due to precondition.
-      const mockRes = expressHandler.mockResponse({
-        locals: { agency: MOCK_AGENCY },
-      })
+      const mockRes = expressHandler.mockResponse()
       const expectedInvalidOtpError = new InvalidOtpError()
+      MockAuthService.validateEmailDomain.mockResolvedValueOnce(ok(MOCK_AGENCY))
       // Mock error from verifyLoginOtp.
       MockAuthService.verifyLoginOtp.mockRejectedValueOnce(
         expectedInvalidOtpError,
@@ -210,10 +224,8 @@ describe('auth.controller', () => {
 
     it('should return 500 when verifying login OTP throws a non-InvalidOtpError', async () => {
       // Arrange
-      // Add agency into locals due to precondition.
-      const mockRes = expressHandler.mockResponse({
-        locals: { agency: MOCK_AGENCY },
-      })
+      const mockRes = expressHandler.mockResponse()
+      MockAuthService.validateEmailDomain.mockResolvedValueOnce(ok(MOCK_AGENCY))
       // Mock generic error from verifyLoginOtp.
       MockAuthService.verifyLoginOtp.mockRejectedValueOnce(
         new Error('generic error'),
@@ -234,10 +246,8 @@ describe('auth.controller', () => {
 
     it('should return 500 when an error is thrown while upserting user', async () => {
       // Arrange
-      // Add agency into locals due to precondition.
-      const mockRes = expressHandler.mockResponse({
-        locals: { agency: MOCK_AGENCY },
-      })
+      const mockRes = expressHandler.mockResponse()
+      MockAuthService.validateEmailDomain.mockResolvedValueOnce(ok(MOCK_AGENCY))
       MockAuthService.verifyLoginOtp.mockResolvedValueOnce(true)
       MockUserService.retrieveUser.mockRejectedValueOnce(
         new Error('upsert error'),
diff --git a/src/app/modules/auth/auth.controller.ts b/src/app/modules/auth/auth.controller.ts
index 02e5087902..c1bbd0c965 100644
--- a/src/app/modules/auth/auth.controller.ts
+++ b/src/app/modules/auth/auth.controller.ts
@@ -158,20 +158,38 @@ export const handleLoginSendOtp: RequestHandler<
 }
 
 /**
- * Precondition: AuthMiddlewares.validateDomain must precede this handler.
+ *  Handler for POST /auth/verifyotp endpoint.
+ * @returns 200 when user has successfully logged in, with session cookie set
+ * @returns 401 when the email domain is invalid
+ * @returns 422 when the OTP is invalid
+ * @returns 500 when error occurred whilst verifying the OTP
  */
-export const handleLoginVerifyOtp: RequestHandler = async (
-  req: Request<
-    ParamsDictionary,
-    string | SessionUser,
-    { email: string; otp: string }
-  >,
-  res: ResponseAfter['validateDomain'],
-) => {
+export const handleLoginVerifyOtp: RequestHandler<
+  ParamsDictionary,
+  string | SessionUser,
+  { email: string; otp: string }
+> = async (req, res) => {
   // Joi validation ensures existence.
   const { email, otp } = req.body
-  // validateDomain middleware will populate agency.
-  const { agency } = res.locals
+
+  const validateResult = await AuthService.validateEmailDomain(email)
+  if (validateResult.isErr()) {
+    const { error } = validateResult
+    logger.error({
+      message: 'Domain validation error',
+      meta: {
+        action: 'handleLoginVerifyOtp',
+        ip: getRequestIp(req),
+        email,
+      },
+      error,
+    })
+    const { errorMessage, statusCode } = mapRouteError(error)
+    return res.status(statusCode).send(errorMessage)
+  }
+
+  // Since there is no error, agency is retrieved from validation.
+  const agency = validateResult.value
 
   const logMeta = {
     action: 'handleLoginVerifyOtp',
@@ -206,7 +224,7 @@ export const handleLoginVerifyOtp: RequestHandler = async (
   // OTP is valid, proceed to login user.
   try {
     // TODO (#317): remove usage of non-null assertion
-    const user = await UserService.retrieveUser(email, agency!)
+    const user = await UserService.retrieveUser(email, agency)
     // Create user object to return to frontend.
     const userObj = { ...user.toObject(), agency }
 
@@ -249,7 +267,7 @@ export const handleSignout: RequestHandler = async (req, res) => {
     return res.sendStatus(StatusCodes.BAD_REQUEST)
   }
 
-  req.session!.destroy((error) => {
+  req.session?.destroy((error) => {
     if (error) {
       logger.error({
         message: 'Failed to destroy session',
diff --git a/src/app/modules/auth/auth.routes.ts b/src/app/modules/auth/auth.routes.ts
index 16a07a5719..c9823f69d2 100644
--- a/src/app/modules/auth/auth.routes.ts
+++ b/src/app/modules/auth/auth.routes.ts
@@ -79,7 +79,6 @@ AuthRouter.post(
         .message('Please enter a valid otp'),
     }),
   }),
-  AuthMiddlewares.validateDomain,
   AuthController.handleLoginVerifyOtp,
 )
 

From 2e270f1c41a7f877cec871213b7b101a70d7809a Mon Sep 17 00:00:00 2001
From: Kar Rui Lau <karrui.lau@gmail.com>
Date: Thu, 17 Sep 2020 00:52:33 +0800
Subject: [PATCH 08/25] refactor(Auth): remove unused auth.middlewares

---
 .../auth/__tests__/auth.middlewares.spec.ts   | 53 --------------
 src/app/modules/auth/auth.middlewares.ts      | 69 -------------------
 src/app/modules/auth/auth.routes.ts           |  1 -
 3 files changed, 123 deletions(-)
 delete mode 100644 src/app/modules/auth/__tests__/auth.middlewares.spec.ts
 delete mode 100644 src/app/modules/auth/auth.middlewares.ts

diff --git a/src/app/modules/auth/__tests__/auth.middlewares.spec.ts b/src/app/modules/auth/__tests__/auth.middlewares.spec.ts
deleted file mode 100644
index 62e0c48afc..0000000000
--- a/src/app/modules/auth/__tests__/auth.middlewares.spec.ts
+++ /dev/null
@@ -1,53 +0,0 @@
-import { err, ok } from 'neverthrow'
-import expressHandler from 'tests/unit/backend/helpers/jest-express'
-import { mocked } from 'ts-jest/utils'
-
-import { IAgencySchema } from 'src/types'
-
-import { InvalidDomainError } from '../auth.errors'
-import * as AuthMiddleware from '../auth.middlewares'
-import * as AuthService from '../auth.service'
-
-jest.mock('../auth.service')
-const MockAuthService = mocked(AuthService)
-
-describe('auth.middleware', () => {
-  describe('validateDomain', () => {
-    const MOCK_REQ = expressHandler.mockRequest({
-      body: { email: 'test@example.com' },
-    })
-
-    it('should continue without error when domain is valid', async () => {
-      // Arrange
-      const mockRes = expressHandler.mockResponse()
-      const mockNext = jest.fn()
-      MockAuthService.validateEmailDomain.mockResolvedValueOnce(
-        ok(<IAgencySchema>{}),
-      )
-
-      // Act
-      await AuthMiddleware.validateDomain(MOCK_REQ, mockRes, mockNext)
-
-      // Assert
-      expect(mockNext).toBeCalled()
-    })
-
-    it('should return with ApplicationError status and message when retrieving agency returns an ApplicationError', async () => {
-      // Arrange
-      const expectedError = new InvalidDomainError()
-      const mockRes = expressHandler.mockResponse()
-      const mockNext = jest.fn()
-      MockAuthService.validateEmailDomain.mockResolvedValueOnce(
-        err(expectedError),
-      )
-
-      // Act
-      await AuthMiddleware.validateDomain(MOCK_REQ, mockRes, mockNext)
-
-      // Assert
-      expect(mockRes.status).toBeCalledWith(expectedError.status)
-      expect(mockRes.send).toBeCalledWith(expectedError.message)
-      expect(mockNext).not.toBeCalled()
-    })
-  })
-})
diff --git a/src/app/modules/auth/auth.middlewares.ts b/src/app/modules/auth/auth.middlewares.ts
deleted file mode 100644
index 9ec00dd5a4..0000000000
--- a/src/app/modules/auth/auth.middlewares.ts
+++ /dev/null
@@ -1,69 +0,0 @@
-import { RequestHandler } from 'express'
-import { ParamsDictionary } from 'express-serve-static-core'
-import { StatusCodes } from 'http-status-codes'
-
-import { createLoggerWithLabel } from '../../../config/logger'
-import { getRequestIp } from '../../utils/request'
-import { ApplicationError, DatabaseError } from '../core/core.errors'
-
-import { InvalidDomainError } from './auth.errors'
-import * as AuthService from './auth.service'
-
-const logger = createLoggerWithLabel(module)
-
-const mapRouteError = (error: ApplicationError) => {
-  switch (error.constructor) {
-    case InvalidDomainError:
-      return {
-        statusCode: StatusCodes.UNAUTHORIZED,
-        errorMessage: error.message,
-      }
-    case DatabaseError:
-      return {
-        statusCode: StatusCodes.INTERNAL_SERVER_ERROR,
-        errorMessage: error.message,
-      }
-    default:
-      return {
-        statusCode: StatusCodes.INTERNAL_SERVER_ERROR,
-        errorMessage: 'Something went wrong. Please try again.',
-      }
-  }
-}
-
-/**
- * Middleware to check if domain of email in the body is from a whitelisted
- * agency.
- * @returns 500 when there was an error validating email
- * @returns 401 when email domain is invalid
- * @returns sets retrieved agency in `res.locals.agency` and calls next when domain is valid
- */
-export const validateDomain: RequestHandler<
-  ParamsDictionary,
-  string,
-  { email: string }
-> = async (req, res, next) => {
-  // Joi validation ensures existence.
-  const { email } = req.body
-
-  const validateResult = await AuthService.validateEmailDomain(email)
-
-  if (validateResult.isErr()) {
-    const { error } = validateResult
-    logger.error({
-      message: 'Domain validation error',
-      meta: {
-        action: 'validateDomain',
-        ip: getRequestIp(req),
-        email,
-      },
-      error,
-    })
-    const { errorMessage, statusCode } = mapRouteError(error)
-    return res.status(statusCode).send(errorMessage)
-  }
-  // Pass down agency to next handler.
-  res.locals.agency = validateResult.value
-
-  return next()
-}
diff --git a/src/app/modules/auth/auth.routes.ts b/src/app/modules/auth/auth.routes.ts
index c9823f69d2..de7babc84e 100644
--- a/src/app/modules/auth/auth.routes.ts
+++ b/src/app/modules/auth/auth.routes.ts
@@ -2,7 +2,6 @@ import { celebrate, Joi, Segments } from 'celebrate'
 import { Router } from 'express'
 
 import * as AuthController from './auth.controller'
-import * as AuthMiddlewares from './auth.middlewares'
 
 export const AuthRouter = Router()
 

From d64a605c25019daa80a65cfddc0133038c3a1bfc Mon Sep 17 00:00:00 2001
From: Kar Rui Lau <karrui.lau@gmail.com>
Date: Thu, 17 Sep 2020 00:54:07 +0800
Subject: [PATCH 09/25] refactor(Auth): remove unused imports/types after
 middleware removal

---
 src/app/modules/auth/auth.controller.ts |  4 ++--
 src/app/modules/auth/auth.types.ts      | 12 +-----------
 src/app/modules/core/core.types.ts      |  3 ---
 3 files changed, 3 insertions(+), 16 deletions(-)
 delete mode 100644 src/app/modules/core/core.types.ts

diff --git a/src/app/modules/auth/auth.controller.ts b/src/app/modules/auth/auth.controller.ts
index c1bbd0c965..3b29479cfa 100644
--- a/src/app/modules/auth/auth.controller.ts
+++ b/src/app/modules/auth/auth.controller.ts
@@ -1,5 +1,5 @@
 import to from 'await-to-js'
-import { Request, RequestHandler } from 'express'
+import { RequestHandler } from 'express'
 import { ParamsDictionary } from 'express-serve-static-core'
 import { StatusCodes } from 'http-status-codes'
 import { isEmpty } from 'lodash'
@@ -13,7 +13,7 @@ import * as UserService from '../user/user.service'
 
 import { InvalidDomainError } from './auth.errors'
 import * as AuthService from './auth.service'
-import { ResponseAfter, SessionUser } from './auth.types'
+import { SessionUser } from './auth.types'
 
 const logger = createLoggerWithLabel(module)
 
diff --git a/src/app/modules/auth/auth.types.ts b/src/app/modules/auth/auth.types.ts
index 861c0e7a98..ba7e1a50ee 100644
--- a/src/app/modules/auth/auth.types.ts
+++ b/src/app/modules/auth/auth.types.ts
@@ -1,13 +1,3 @@
-import { IAgencySchema, IPopulatedUser } from 'src/types'
-
-import { ResponseWithLocals } from '../core/core.types'
-
-/**
- *  Meta typing for the shape of the Express.Response object after various
- *  middlewares for /auth.
- */
-export type ResponseAfter = {
-  validateDomain: ResponseWithLocals<{ agency?: IAgencySchema }>
-}
+import { IPopulatedUser } from 'src/types'
 
 export type SessionUser = IPopulatedUser
diff --git a/src/app/modules/core/core.types.ts b/src/app/modules/core/core.types.ts
deleted file mode 100644
index 9c1011c90c..0000000000
--- a/src/app/modules/core/core.types.ts
+++ /dev/null
@@ -1,3 +0,0 @@
-import { Response } from 'express'
-
-export type ResponseWithLocals<T> = Omit<Response, 'locals'> & { locals: T }

From 7104daeec69360b745e81e3b5e13479d3a7541b1 Mon Sep 17 00:00:00 2001
From: Kar Rui Lau <karrui.lau@gmail.com>
Date: Thu, 17 Sep 2020 11:02:22 +0800
Subject: [PATCH 10/25] refactor(AuthService): use validateEmailDomain with
 ResultAsync return

---
 .../auth/__tests__/auth.controller.spec.ts    | 46 +++++++++-------
 .../auth/__tests__/auth.routes.spec.ts        |  8 +--
 src/app/modules/auth/auth.controller.ts       |  3 +-
 src/app/modules/auth/auth.service.ts          | 52 +++++++++----------
 src/config/logger.ts                          |  2 +-
 5 files changed, 59 insertions(+), 52 deletions(-)

diff --git a/src/app/modules/auth/__tests__/auth.controller.spec.ts b/src/app/modules/auth/__tests__/auth.controller.spec.ts
index 3558883bec..de1ff1b983 100644
--- a/src/app/modules/auth/__tests__/auth.controller.spec.ts
+++ b/src/app/modules/auth/__tests__/auth.controller.spec.ts
@@ -1,4 +1,4 @@
-import { err, ok } from 'neverthrow'
+import { err, errAsync, ok, okAsync } from 'neverthrow'
 import expressHandler from 'tests/unit/backend/helpers/jest-express'
 import { mocked } from 'ts-jest/utils'
 
@@ -33,8 +33,8 @@ describe('auth.controller', () => {
     it('should return 200 when domain is valid', async () => {
       // Arrange
       const mockRes = expressHandler.mockResponse()
-      MockAuthService.validateEmailDomain.mockResolvedValueOnce(
-        ok(<IAgencySchema>{}),
+      MockAuthService.validateEmailDomain.mockReturnValueOnce(
+        okAsync(<IAgencySchema>{}),
       )
 
       // Act
@@ -48,8 +48,8 @@ describe('auth.controller', () => {
       // Arrange
       const expectedError = new InvalidDomainError()
       const mockRes = expressHandler.mockResponse()
-      MockAuthService.validateEmailDomain.mockResolvedValueOnce(
-        err(expectedError),
+      MockAuthService.validateEmailDomain.mockReturnValueOnce(
+        errAsync(expectedError),
       )
 
       // Act
@@ -71,8 +71,8 @@ describe('auth.controller', () => {
       // Arrange
       const mockRes = expressHandler.mockResponse()
       // Mock AuthService and MailService to return without errors
-      MockAuthService.validateEmailDomain.mockResolvedValueOnce(
-        ok(<IAgencySchema>{}),
+      MockAuthService.validateEmailDomain.mockReturnValueOnce(
+        okAsync(<IAgencySchema>{}),
       )
       MockAuthService.createLoginOtp.mockResolvedValueOnce(MOCK_OTP)
       MockMailService.sendLoginOtp.mockResolvedValueOnce(true)
@@ -92,8 +92,8 @@ describe('auth.controller', () => {
       // Arrange
       const expectedError = new InvalidDomainError()
       const mockRes = expressHandler.mockResponse()
-      MockAuthService.validateEmailDomain.mockResolvedValueOnce(
-        err(expectedError),
+      MockAuthService.validateEmailDomain.mockReturnValueOnce(
+        errAsync(expectedError),
       )
 
       // Act
@@ -107,8 +107,8 @@ describe('auth.controller', () => {
     it('should return 500 when there is an error generating login OTP', async () => {
       // Arrange
       const mockRes = expressHandler.mockResponse()
-      MockAuthService.validateEmailDomain.mockResolvedValueOnce(
-        ok(<IAgencySchema>{}),
+      MockAuthService.validateEmailDomain.mockReturnValueOnce(
+        okAsync(<IAgencySchema>{}),
       )
       // Mock createLoginOtp failure
       MockAuthService.createLoginOtp.mockRejectedValueOnce(
@@ -131,8 +131,8 @@ describe('auth.controller', () => {
     it('should return 500 when there is an error sending login OTP', async () => {
       // Arrange
       const mockRes = expressHandler.mockResponse()
-      MockAuthService.validateEmailDomain.mockResolvedValueOnce(
-        ok(<IAgencySchema>{}),
+      MockAuthService.validateEmailDomain.mockReturnValueOnce(
+        okAsync(<IAgencySchema>{}),
       )
       // Mock createLoginOtp success but sendLoginOtp failure.
       MockAuthService.createLoginOtp.mockResolvedValueOnce(MOCK_OTP)
@@ -170,7 +170,9 @@ describe('auth.controller', () => {
       const mockRes = expressHandler.mockResponse()
 
       // Mock all service success.
-      MockAuthService.validateEmailDomain.mockResolvedValueOnce(ok(MOCK_AGENCY))
+      MockAuthService.validateEmailDomain.mockReturnValueOnce(
+        okAsync(MOCK_AGENCY),
+      )
       MockAuthService.verifyLoginOtp.mockResolvedValueOnce(true)
       MockUserService.retrieveUser.mockResolvedValueOnce(mockUser)
 
@@ -189,8 +191,8 @@ describe('auth.controller', () => {
       // Arrange
       const expectedError = new InvalidDomainError()
       const mockRes = expressHandler.mockResponse()
-      MockAuthService.validateEmailDomain.mockResolvedValueOnce(
-        err(expectedError),
+      MockAuthService.validateEmailDomain.mockReturnValueOnce(
+        errAsync(expectedError),
       )
 
       // Act
@@ -205,7 +207,9 @@ describe('auth.controller', () => {
       // Arrange
       const mockRes = expressHandler.mockResponse()
       const expectedInvalidOtpError = new InvalidOtpError()
-      MockAuthService.validateEmailDomain.mockResolvedValueOnce(ok(MOCK_AGENCY))
+      MockAuthService.validateEmailDomain.mockReturnValueOnce(
+        okAsync(MOCK_AGENCY),
+      )
       // Mock error from verifyLoginOtp.
       MockAuthService.verifyLoginOtp.mockRejectedValueOnce(
         expectedInvalidOtpError,
@@ -225,7 +229,9 @@ describe('auth.controller', () => {
     it('should return 500 when verifying login OTP throws a non-InvalidOtpError', async () => {
       // Arrange
       const mockRes = expressHandler.mockResponse()
-      MockAuthService.validateEmailDomain.mockResolvedValueOnce(ok(MOCK_AGENCY))
+      MockAuthService.validateEmailDomain.mockReturnValueOnce(
+        okAsync(MOCK_AGENCY),
+      )
       // Mock generic error from verifyLoginOtp.
       MockAuthService.verifyLoginOtp.mockRejectedValueOnce(
         new Error('generic error'),
@@ -247,7 +253,9 @@ describe('auth.controller', () => {
     it('should return 500 when an error is thrown while upserting user', async () => {
       // Arrange
       const mockRes = expressHandler.mockResponse()
-      MockAuthService.validateEmailDomain.mockResolvedValueOnce(ok(MOCK_AGENCY))
+      MockAuthService.validateEmailDomain.mockReturnValueOnce(
+        okAsync(MOCK_AGENCY),
+      )
       MockAuthService.verifyLoginOtp.mockResolvedValueOnce(true)
       MockUserService.retrieveUser.mockRejectedValueOnce(
         new Error('upsert error'),
diff --git a/src/app/modules/auth/__tests__/auth.routes.spec.ts b/src/app/modules/auth/__tests__/auth.routes.spec.ts
index 349024cc94..dcd1623d19 100644
--- a/src/app/modules/auth/__tests__/auth.routes.spec.ts
+++ b/src/app/modules/auth/__tests__/auth.routes.spec.ts
@@ -1,5 +1,5 @@
 import { pick } from 'lodash'
-import { err } from 'neverthrow'
+import { err, errAsync } from 'neverthrow'
 import supertest from 'supertest'
 import { CookieStore, setupApp } from 'tests/integration/helpers/express-setup'
 import dbHandler from 'tests/unit/backend/helpers/jest-db'
@@ -94,7 +94,7 @@ describe('auth.routes', () => {
 
       const getAgencySpy = jest
         .spyOn(AuthService, 'validateEmailDomain')
-        .mockResolvedValueOnce(err(new DatabaseError(mockErrorString)))
+        .mockReturnValueOnce(errAsync(new DatabaseError(mockErrorString)))
 
       // Act
       const response = await request
@@ -200,7 +200,7 @@ describe('auth.routes', () => {
       const mockErrorString = 'Unable to validate email domain.'
       const getAgencySpy = jest
         .spyOn(AuthService, 'validateEmailDomain')
-        .mockResolvedValueOnce(err(new DatabaseError(mockErrorString)))
+        .mockReturnValueOnce(errAsync(new DatabaseError(mockErrorString)))
 
       // Act
       const response = await request
@@ -328,7 +328,7 @@ describe('auth.routes', () => {
       const mockErrorString = 'Unable to validate email domain.'
       const getAgencySpy = jest
         .spyOn(AuthService, 'validateEmailDomain')
-        .mockResolvedValueOnce(err(new DatabaseError(mockErrorString)))
+        .mockReturnValueOnce(errAsync(new DatabaseError(mockErrorString)))
 
       // Act
       const response = await request
diff --git a/src/app/modules/auth/auth.controller.ts b/src/app/modules/auth/auth.controller.ts
index 3b29479cfa..0d14860bd8 100644
--- a/src/app/modules/auth/auth.controller.ts
+++ b/src/app/modules/auth/auth.controller.ts
@@ -56,8 +56,7 @@ export const handleCheckUser: RequestHandler<
   // Joi validation ensures existence.
   const { email } = req.body
 
-  const validateResult = await AuthService.validateEmailDomain(email)
-  validateResult
+  return AuthService.validateEmailDomain(email)
     .map(() => res.sendStatus(StatusCodes.OK))
     .mapErr((error) => {
       logger.error({
diff --git a/src/app/modules/auth/auth.service.ts b/src/app/modules/auth/auth.service.ts
index 349e140fed..70aecd3c36 100644
--- a/src/app/modules/auth/auth.service.ts
+++ b/src/app/modules/auth/auth.service.ts
@@ -1,6 +1,6 @@
 import bcrypt from 'bcrypt'
 import mongoose from 'mongoose'
-import { err, ok, Result } from 'neverthrow'
+import { err, errAsync, ok, okAsync, Result, ResultAsync } from 'neverthrow'
 import validator from 'validator'
 
 import { IAgencySchema } from 'src/types'
@@ -29,46 +29,46 @@ export const MAX_OTP_ATTEMPTS = 10
  * @returns the agency document with the domain of the email only if it is valid.
  * @throws error if database query fails or if agency cannot be found.
  */
-export const validateEmailDomain = async (
+export const validateEmailDomain = (
   email: string,
-): Promise<Result<IAgencySchema, InvalidDomainError | DatabaseError>> => {
+): ResultAsync<IAgencySchema, InvalidDomainError | DatabaseError> => {
   // Extra guard even if Joi validation has already checked.
   if (!validator.isEmail(email)) {
-    return err(new InvalidDomainError())
+    return errAsync(new InvalidDomainError())
   }
 
   const emailDomain = email.split('@').pop()
-  try {
-    const agency = await AgencyModel.findOne({ emailDomain })
+  return ResultAsync.fromPromise(
+    AgencyModel.findOne({ emailDomain }).exec(),
+    (error) => {
+      logger.error({
+        message: 'Database error when retrieving agency',
+        meta: {
+          action: 'validateEmailDomain',
+          emailDomain,
+        },
+        error,
+      })
+
+      return new DatabaseError(
+        `Unable to validate email domain. If this issue persists, please submit a Support Form at (${LINKS.supportFormLink})`,
+      )
+    },
+  ).andThen((agency) => {
     if (!agency) {
-      const error = new InvalidDomainError()
+      const noAgencyError = new InvalidDomainError()
       logger.warn({
         message: 'Agency not found',
         meta: {
           action: 'retrieveAgency',
           emailDomain,
         },
-        error,
+        error: noAgencyError,
       })
-      return err(error)
+      return errAsync(noAgencyError)
     }
-    return ok(agency)
-  } catch (error) {
-    logger.error({
-      message: 'Database error when retrieving agency',
-      meta: {
-        action: 'retrieveAgency',
-        emailDomain,
-      },
-      error,
-    })
-
-    return err(
-      new DatabaseError(
-        `Unable to validate email domain. If this issue persists, please submit a Support Form at (${LINKS.supportFormLink})`,
-      ),
-    )
-  }
+    return okAsync(agency)
+  })
 }
 
 /**
diff --git a/src/config/logger.ts b/src/config/logger.ts
index bf4ef0e4b3..2254a39f3e 100644
--- a/src/config/logger.ts
+++ b/src/config/logger.ts
@@ -20,7 +20,7 @@ type CustomLoggerParams = {
     action: string
     [other: string]: any
   }
-  error?: Error
+  error?: unknown
 }
 
 // A variety of helper functions to make winston logging like console logging,

From df6b95e8168f3692cf921e0a6232d87892ec7c90 Mon Sep 17 00:00:00 2001
From: Kar Rui Lau <karrui.lau@gmail.com>
Date: Thu, 17 Sep 2020 11:53:40 +0800
Subject: [PATCH 11/25] refactor(AuthService): update createLoginOtp with
 ResultAsync return

---
 .../auth/__tests__/auth.controller.spec.ts    |  9 +-
 .../auth/__tests__/auth.routes.spec.ts        |  4 +-
 .../auth/__tests__/auth.service.spec.ts       | 10 +-
 src/app/modules/auth/auth.controller.ts       | 10 +-
 src/app/modules/auth/auth.service.ts          | 99 +++++++++++++++----
 src/app/modules/core/core.errors.ts           |  2 +-
 6 files changed, 101 insertions(+), 33 deletions(-)

diff --git a/src/app/modules/auth/__tests__/auth.controller.spec.ts b/src/app/modules/auth/__tests__/auth.controller.spec.ts
index de1ff1b983..26e8ed4dae 100644
--- a/src/app/modules/auth/__tests__/auth.controller.spec.ts
+++ b/src/app/modules/auth/__tests__/auth.controller.spec.ts
@@ -5,6 +5,7 @@ import { mocked } from 'ts-jest/utils'
 import MailService from 'src/app/services/mail.service'
 import { IAgencySchema, IUserSchema } from 'src/types'
 
+import { DatabaseError } from '../../core/core.errors'
 import * as UserService from '../../user/user.service'
 import * as AuthController from '../auth.controller'
 import { InvalidDomainError, InvalidOtpError } from '../auth.errors'
@@ -74,7 +75,7 @@ describe('auth.controller', () => {
       MockAuthService.validateEmailDomain.mockReturnValueOnce(
         okAsync(<IAgencySchema>{}),
       )
-      MockAuthService.createLoginOtp.mockResolvedValueOnce(MOCK_OTP)
+      MockAuthService.createLoginOtp.mockReturnValueOnce(okAsync(MOCK_OTP))
       MockMailService.sendLoginOtp.mockResolvedValueOnce(true)
 
       // Act
@@ -111,8 +112,8 @@ describe('auth.controller', () => {
         okAsync(<IAgencySchema>{}),
       )
       // Mock createLoginOtp failure
-      MockAuthService.createLoginOtp.mockRejectedValueOnce(
-        new Error('otp creation error'),
+      MockAuthService.createLoginOtp.mockReturnValueOnce(
+        errAsync(new DatabaseError('otp creation error')),
       )
 
       // Act
@@ -135,7 +136,7 @@ describe('auth.controller', () => {
         okAsync(<IAgencySchema>{}),
       )
       // Mock createLoginOtp success but sendLoginOtp failure.
-      MockAuthService.createLoginOtp.mockResolvedValueOnce(MOCK_OTP)
+      MockAuthService.createLoginOtp.mockReturnValueOnce(okAsync(MOCK_OTP))
       MockMailService.sendLoginOtp.mockRejectedValueOnce(
         new Error('send error'),
       )
diff --git a/src/app/modules/auth/__tests__/auth.routes.spec.ts b/src/app/modules/auth/__tests__/auth.routes.spec.ts
index dcd1623d19..26a9e0e5bf 100644
--- a/src/app/modules/auth/__tests__/auth.routes.spec.ts
+++ b/src/app/modules/auth/__tests__/auth.routes.spec.ts
@@ -9,7 +9,7 @@ import MailService from 'src/app/services/mail.service'
 import * as OtpUtils from 'src/app/utils/otp'
 import { IAgencySchema } from 'src/types'
 
-import { DatabaseError } from '../../core/core.errors'
+import { ApplicationError, DatabaseError } from '../../core/core.errors'
 import * as UserService from '../../user/user.service'
 import { AuthRouter } from '../auth.routes'
 import * as AuthService from '../auth.service'
@@ -161,7 +161,7 @@ describe('auth.routes', () => {
       // Arrange
       const createLoginOtpSpy = jest
         .spyOn(AuthService, 'createLoginOtp')
-        .mockRejectedValueOnce(new Error('some error'))
+        .mockReturnValueOnce(errAsync(new ApplicationError()))
 
       // Act
       const response = await request
diff --git a/src/app/modules/auth/__tests__/auth.service.spec.ts b/src/app/modules/auth/__tests__/auth.service.spec.ts
index e578b8425f..d29a9d5855 100644
--- a/src/app/modules/auth/__tests__/auth.service.spec.ts
+++ b/src/app/modules/auth/__tests__/auth.service.spec.ts
@@ -80,10 +80,11 @@ describe('auth.service', () => {
       await expect(TokenModel.countDocuments()).resolves.toEqual(0)
 
       // Act
-      const actualOtp = await AuthService.createLoginOtp(VALID_EMAIL)
+      const actualResult = await AuthService.createLoginOtp(VALID_EMAIL)
 
       // Assert
-      expect(actualOtp).toEqual(MOCK_OTP)
+      expect(actualResult.isOk()).toBe(true)
+      expect(actualResult._unsafeUnwrap()).toEqual(MOCK_OTP)
       // Should have new token document inserted.
       await expect(TokenModel.countDocuments()).resolves.toEqual(1)
     })
@@ -93,10 +94,11 @@ describe('auth.service', () => {
       const notAnEmail = 'not an email'
 
       // Act
-      const actualPromise = AuthService.createLoginOtp(notAnEmail)
+      const actualResult = await AuthService.createLoginOtp(notAnEmail)
 
       // Assert
-      await expect(actualPromise).rejects.toThrowError(InvalidDomainError)
+      expect(actualResult.isErr()).toBe(true)
+      expect(actualResult._unsafeUnwrapErr()).toBeInstanceOf(InvalidDomainError)
     })
   })
 
diff --git a/src/app/modules/auth/auth.controller.ts b/src/app/modules/auth/auth.controller.ts
index 0d14860bd8..dfc5a7c83a 100644
--- a/src/app/modules/auth/auth.controller.ts
+++ b/src/app/modules/auth/auth.controller.ts
@@ -110,13 +110,13 @@ export const handleLoginSendOtp: RequestHandler<
   }
 
   // Create OTP.
-  const [otpErr, otp] = await to(AuthService.createLoginOtp(email))
-
-  if (otpErr || !otp) {
+  const otpResult = await AuthService.createLoginOtp(email)
+  if (otpResult.isErr()) {
+    const { error } = otpResult
     logger.error({
       message: 'Error generating OTP',
       meta: logMeta,
-      error: otpErr ?? undefined,
+      error,
     })
     return res
       .status(StatusCodes.INTERNAL_SERVER_ERROR)
@@ -129,7 +129,7 @@ export const handleLoginSendOtp: RequestHandler<
   const [sendErr] = await to(
     MailService.sendLoginOtp({
       recipient: email,
-      otp,
+      otp: otpResult.value,
       ipAddress: requestIp,
     }),
   )
diff --git a/src/app/modules/auth/auth.service.ts b/src/app/modules/auth/auth.service.ts
index 70aecd3c36..77d00ca148 100644
--- a/src/app/modules/auth/auth.service.ts
+++ b/src/app/modules/auth/auth.service.ts
@@ -3,7 +3,7 @@ import mongoose from 'mongoose'
 import { err, errAsync, ok, okAsync, Result, ResultAsync } from 'neverthrow'
 import validator from 'validator'
 
-import { IAgencySchema } from 'src/types'
+import { IAgencySchema, ITokenSchema } from 'src/types'
 
 import config from '../../../config/config'
 import { createLoggerWithLabel } from '../../../config/logger'
@@ -11,7 +11,7 @@ import { LINKS } from '../../../shared/constants'
 import getAgencyModel from '../../models/agency.server.model'
 import getTokenModel from '../../models/token.server.model'
 import { generateOtp } from '../../utils/otp'
-import { DatabaseError } from '../core/core.errors'
+import { ApplicationError, DatabaseError } from '../core/core.errors'
 
 import { InvalidDomainError, InvalidOtpError } from './auth.errors'
 
@@ -26,8 +26,9 @@ export const MAX_OTP_ATTEMPTS = 10
  * Validates the domain of the given email. A domain is valid if it exists in
  * the Agency collection in the database.
  * @param email the email to validate the domain for
- * @returns the agency document with the domain of the email only if it is valid.
- * @throws error if database query fails or if agency cannot be found.
+ * @returns ok(the agency document) with the domain of the email if the domain is valid
+ * @returns err(InvalidDomainError) if the agency document cannot be found
+ * @returns err(DatabaseError) if database query fails
  */
 export const validateEmailDomain = (
   email: string,
@@ -74,25 +75,31 @@ export const validateEmailDomain = (
 /**
  * Creates a login OTP and saves its hash into the Token collection.
  * @param email the email to link the generated otp to
- * @returns the generated OTP if saving into DB is successful
- * @throws {InvalidDomainError} the given email is invalid
- * @throws {Error} if any error occur whilst creating the OTP or insertion of OTP into the database.
+ * @returns ok(the generated OTP) if saving into DB is successful
+ * @returns err(InvalidDomainError) if the given email is invalid
+ * @returns err(ApplicationError) if any error occur whilst hashing the OTP
+ * @returns err(DatabaseError) if error occurs during upsertion of hashed OTP into the database.
  */
-export const createLoginOtp = async (email: string) => {
+export const createLoginOtp = (
+  email: string,
+): ResultAsync<
+  string,
+  ApplicationError | DatabaseError | InvalidDomainError
+> => {
   if (!validator.isEmail(email)) {
-    throw new InvalidDomainError()
+    return errAsync(new InvalidDomainError())
   }
 
   const otp = generateOtp()
-  const hashedOtp = await bcrypt.hash(otp, DEFAULT_SALT_ROUNDS)
 
-  await TokenModel.upsertOtp({
-    email,
-    hashedOtp,
-    expireAt: new Date(Date.now() + config.otpLifeSpan),
-  })
-
-  return otp
+  return (
+    // Step 1: Hash OTP.
+    hashOtp(otp, { email })
+      // Step 2: Upsert otp hash into database.
+      .andThen((hashedOtp) => upsertOtp(email, hashedOtp))
+      // Step 3: Return generated OTP.
+      .map(() => otp)
+  )
 }
 
 /**
@@ -137,3 +144,61 @@ export const verifyLoginOtp = async (otpToVerify: string, email: string) => {
   // Finally return true (as success).
   return true
 }
+
+// Private helper functions
+/**
+ * Hashes the given otp.
+ * @param otpToHash the otp to hash
+ * @param logMeta additional metadata for logging, if available
+ * @returns ok(hashed otp) if the hashing was successful
+ * @returns err(ApplicationError) if hashing error occurs
+ */
+const hashOtp = (otpToHash: string, logMeta: Record<string, unknown> = {}) => {
+  return ResultAsync.fromPromise(
+    bcrypt.hash(otpToHash, DEFAULT_SALT_ROUNDS),
+    (error) => {
+      logger.error({
+        message: 'bcrypt hash otp error',
+        meta: {
+          action: 'hashOtp',
+          ...logMeta,
+        },
+        error,
+      })
+
+      return new ApplicationError()
+    },
+  )
+}
+
+/**
+ * Upserts the given otp hash into the document keyed by the given email
+ * @param email the email to retrieve the current Token document for
+ * @param hashedOtp the otp hash to upsert
+ * @returns ok(upserted Token document) if upsert is successful
+ * @returns err(DatabaseError) if upsert fails
+ */
+const upsertOtp = (
+  email: string,
+  hashedOtp: string,
+): ResultAsync<ITokenSchema, DatabaseError> => {
+  return ResultAsync.fromPromise(
+    TokenModel.upsertOtp({
+      email,
+      hashedOtp,
+      expireAt: new Date(Date.now() + config.otpLifeSpan),
+    }),
+    (error) => {
+      logger.error({
+        message: 'Database upsert OTP error',
+        meta: {
+          action: 'upsertOtp',
+          email,
+        },
+        error,
+      })
+
+      return new DatabaseError()
+    },
+  )
+}
diff --git a/src/app/modules/core/core.errors.ts b/src/app/modules/core/core.errors.ts
index be485cd06f..d6f665ff1f 100644
--- a/src/app/modules/core/core.errors.ts
+++ b/src/app/modules/core/core.errors.ts
@@ -2,7 +2,7 @@
  * A custom base error class that encapsulates the name, message, status code,
  * and logging meta string (if any) for the error.
  */
-export abstract class ApplicationError extends Error {
+export class ApplicationError extends Error {
   /**
    * Http status code for the error to be returned in the response.
    */

From 7a87768f0c26efa0d9de87385d804507442fd45b Mon Sep 17 00:00:00 2001
From: Kar Rui Lau <karrui.lau@gmail.com>
Date: Thu, 17 Sep 2020 14:11:24 +0800
Subject: [PATCH 12/25] refactor(AuthService): update verifyLoginOtp with
 ResultAsync return

---
 .../auth/__tests__/auth.controller.spec.ts    |  14 +-
 .../auth/__tests__/auth.service.spec.ts       |  32 +++-
 src/app/modules/auth/auth.controller.ts       |  48 +++---
 src/app/modules/auth/auth.service.ts          | 148 ++++++++++++++----
 src/types/token.ts                            |   4 +-
 5 files changed, 176 insertions(+), 70 deletions(-)

diff --git a/src/app/modules/auth/__tests__/auth.controller.spec.ts b/src/app/modules/auth/__tests__/auth.controller.spec.ts
index 26e8ed4dae..6dd29f9c96 100644
--- a/src/app/modules/auth/__tests__/auth.controller.spec.ts
+++ b/src/app/modules/auth/__tests__/auth.controller.spec.ts
@@ -5,7 +5,7 @@ import { mocked } from 'ts-jest/utils'
 import MailService from 'src/app/services/mail.service'
 import { IAgencySchema, IUserSchema } from 'src/types'
 
-import { DatabaseError } from '../../core/core.errors'
+import { ApplicationError, DatabaseError } from '../../core/core.errors'
 import * as UserService from '../../user/user.service'
 import * as AuthController from '../auth.controller'
 import { InvalidDomainError, InvalidOtpError } from '../auth.errors'
@@ -174,7 +174,7 @@ describe('auth.controller', () => {
       MockAuthService.validateEmailDomain.mockReturnValueOnce(
         okAsync(MOCK_AGENCY),
       )
-      MockAuthService.verifyLoginOtp.mockResolvedValueOnce(true)
+      MockAuthService.verifyLoginOtp.mockReturnValueOnce(okAsync(true))
       MockUserService.retrieveUser.mockResolvedValueOnce(mockUser)
 
       // Act
@@ -212,8 +212,8 @@ describe('auth.controller', () => {
         okAsync(MOCK_AGENCY),
       )
       // Mock error from verifyLoginOtp.
-      MockAuthService.verifyLoginOtp.mockRejectedValueOnce(
-        expectedInvalidOtpError,
+      MockAuthService.verifyLoginOtp.mockReturnValueOnce(
+        errAsync(expectedInvalidOtpError),
       )
 
       // Act
@@ -234,8 +234,8 @@ describe('auth.controller', () => {
         okAsync(MOCK_AGENCY),
       )
       // Mock generic error from verifyLoginOtp.
-      MockAuthService.verifyLoginOtp.mockRejectedValueOnce(
-        new Error('generic error'),
+      MockAuthService.verifyLoginOtp.mockReturnValueOnce(
+        errAsync(new ApplicationError('generic error')),
       )
 
       // Act
@@ -257,7 +257,7 @@ describe('auth.controller', () => {
       MockAuthService.validateEmailDomain.mockReturnValueOnce(
         okAsync(MOCK_AGENCY),
       )
-      MockAuthService.verifyLoginOtp.mockResolvedValueOnce(true)
+      MockAuthService.verifyLoginOtp.mockReturnValueOnce(okAsync(true))
       MockUserService.retrieveUser.mockRejectedValueOnce(
         new Error('upsert error'),
       )
diff --git a/src/app/modules/auth/__tests__/auth.service.spec.ts b/src/app/modules/auth/__tests__/auth.service.spec.ts
index d29a9d5855..eaab218870 100644
--- a/src/app/modules/auth/__tests__/auth.service.spec.ts
+++ b/src/app/modules/auth/__tests__/auth.service.spec.ts
@@ -110,11 +110,15 @@ describe('auth.service', () => {
       await expect(TokenModel.countDocuments()).resolves.toEqual(1)
 
       // Act
-      const actual = await AuthService.verifyLoginOtp(MOCK_OTP, VALID_EMAIL)
+      const actualResult = await AuthService.verifyLoginOtp(
+        MOCK_OTP,
+        VALID_EMAIL,
+      )
 
       // Assert
       // Resolves successfully.
-      expect(actual).toEqual(true)
+      expect(actualResult.isOk()).toBe(true)
+      expect(actualResult._unsafeUnwrap()).toEqual(true)
       // Token document should be removed.
       await expect(TokenModel.countDocuments()).resolves.toEqual(0)
     })
@@ -125,13 +129,17 @@ describe('auth.service', () => {
       await expect(TokenModel.countDocuments()).resolves.toEqual(0)
 
       // Act
-      const verifyPromise = AuthService.verifyLoginOtp(MOCK_OTP, VALID_EMAIL)
+      const actualResult = await AuthService.verifyLoginOtp(
+        MOCK_OTP,
+        VALID_EMAIL,
+      )
 
       // Assert
       const expectedError = new InvalidOtpError(
         'OTP has expired. Please request for a new OTP.',
       )
-      await expect(verifyPromise).rejects.toThrowError(expectedError)
+      expect(actualResult.isErr()).toBe(true)
+      expect(actualResult._unsafeUnwrapErr()).toEqual(expectedError)
     })
 
     it('should throw InvalidOtpError when verification has been attempted too many times', async () => {
@@ -145,13 +153,17 @@ describe('auth.service', () => {
       )
 
       // Act
-      const verifyPromise = AuthService.verifyLoginOtp(MOCK_OTP, VALID_EMAIL)
+      const actualResult = await AuthService.verifyLoginOtp(
+        MOCK_OTP,
+        VALID_EMAIL,
+      )
 
       // Assert
       const expectedError = new InvalidOtpError(
         'You have hit the max number of attempts. Please request for a new OTP.',
       )
-      await expect(verifyPromise).rejects.toThrowError(expectedError)
+      expect(actualResult.isErr()).toBe(true)
+      expect(actualResult._unsafeUnwrapErr()).toEqual(expectedError)
     })
 
     it('should throw InvalidOtpError when the OTP hash does not match', async () => {
@@ -161,13 +173,17 @@ describe('auth.service', () => {
       const invalidOtp = '654321'
 
       // Act
-      const verifyPromise = AuthService.verifyLoginOtp(invalidOtp, VALID_EMAIL)
+      const actualResult = await AuthService.verifyLoginOtp(
+        invalidOtp,
+        VALID_EMAIL,
+      )
 
       // Assert
       const expectedError = new InvalidOtpError(
         'OTP is invalid. Please try again.',
       )
-      await expect(verifyPromise).rejects.toThrowError(expectedError)
+      expect(actualResult.isErr()).toBe(true)
+      expect(actualResult._unsafeUnwrapErr()).toEqual(expectedError)
     })
   })
 })
diff --git a/src/app/modules/auth/auth.controller.ts b/src/app/modules/auth/auth.controller.ts
index dfc5a7c83a..c8dc1ca1de 100644
--- a/src/app/modules/auth/auth.controller.ts
+++ b/src/app/modules/auth/auth.controller.ts
@@ -11,7 +11,7 @@ import { getRequestIp } from '../../utils/request'
 import { ApplicationError, DatabaseError } from '../core/core.errors'
 import * as UserService from '../user/user.service'
 
-import { InvalidDomainError } from './auth.errors'
+import { InvalidDomainError, InvalidOtpError } from './auth.errors'
 import * as AuthService from './auth.service'
 import { SessionUser } from './auth.types'
 
@@ -21,18 +21,25 @@ const logger = createLoggerWithLabel(module)
  * Handler to map ApplicationErrors to their correct status code and error
  * messages.
  * @param error The error to retrieve the status codes and error messages
+ * @param coreErrorMessage Any error message to return instead of the default core error message, if any
  */
-const mapRouteError = (error: ApplicationError) => {
+const mapRouteError = (error: ApplicationError, coreErrorMessage?: string) => {
   switch (error.constructor) {
     case InvalidDomainError:
       return {
         statusCode: StatusCodes.UNAUTHORIZED,
         errorMessage: error.message,
       }
+    case InvalidOtpError:
+      return {
+        statusCode: StatusCodes.UNPROCESSABLE_ENTITY,
+        errorMessage: error.message,
+      }
+    case ApplicationError:
     case DatabaseError:
       return {
         statusCode: StatusCodes.INTERNAL_SERVER_ERROR,
-        errorMessage: error.message,
+        errorMessage: coreErrorMessage ?? error.message,
       }
     default:
       return {
@@ -118,11 +125,12 @@ export const handleLoginSendOtp: RequestHandler<
       meta: logMeta,
       error,
     })
-    return res
-      .status(StatusCodes.INTERNAL_SERVER_ERROR)
-      .send(
-        'Failed to send login OTP. Please try again later and if the problem persists, contact us.',
-      )
+
+    const { errorMessage, statusCode } = mapRouteError(
+      error,
+      /* coreErrorMessage= */ 'Failed to send login OTP. Please try again later and if the problem persists, contact us.',
+    )
+    return res.status(statusCode).send(errorMessage)
   }
 
   // Send OTP.
@@ -196,28 +204,24 @@ export const handleLoginVerifyOtp: RequestHandler<
     ip: getRequestIp(req),
   }
 
-  const [verifyErr] = await to(AuthService.verifyLoginOtp(otp, email))
+  const verifyResult = await AuthService.verifyLoginOtp(otp, email)
 
-  if (verifyErr) {
+  if (verifyResult.isErr()) {
+    const { error } = verifyResult
     logger.warn({
       message:
-        verifyErr instanceof ApplicationError
+        error instanceof InvalidOtpError
           ? 'Login OTP is invalid'
           : 'Error occurred when trying to validate login OTP',
       meta: logMeta,
-      error: verifyErr,
+      error,
     })
 
-    if (verifyErr instanceof ApplicationError) {
-      return res.status(verifyErr.status).send(verifyErr.message)
-    }
-
-    // Unknown error, return generic error response.
-    return res
-      .status(StatusCodes.INTERNAL_SERVER_ERROR)
-      .send(
-        'Failed to validate OTP. Please try again later and if the problem persists, contact us.',
-      )
+    const { errorMessage, statusCode } = mapRouteError(
+      error,
+      /* coreErrorMessage= */ 'Failed to validate OTP. Please try again later and if the problem persists, contact us.',
+    )
+    return res.status(statusCode).send(errorMessage)
   }
 
   // OTP is valid, proceed to login user.
diff --git a/src/app/modules/auth/auth.service.ts b/src/app/modules/auth/auth.service.ts
index 77d00ca148..9478d5fbaa 100644
--- a/src/app/modules/auth/auth.service.ts
+++ b/src/app/modules/auth/auth.service.ts
@@ -1,6 +1,6 @@
 import bcrypt from 'bcrypt'
 import mongoose from 'mongoose'
-import { err, errAsync, ok, okAsync, Result, ResultAsync } from 'neverthrow'
+import { errAsync, okAsync, ResultAsync } from 'neverthrow'
 import validator from 'validator'
 
 import { IAgencySchema, ITokenSchema } from 'src/types'
@@ -110,39 +110,25 @@ export const createLoginOtp = (
  * returned. Else, the document is kept in the database and an error is thrown.
  * @param otpToVerify the OTP to verify with the hashed counterpart
  * @param email the email used to retrieve the document from the database
- * @returns true on success
- * @throws {InvalidOtpError} if the OTP is invalid or expired.
- * @throws {Error} if any errors occur whilst retrieving from database or comparing hashes.
+ * @returns ok(true) on success
+ * @returns err(InvalidOtpError) if the OTP is invalid or expired
+ * @returns err(ApplicationError) if any error occurs whilst comparing hashes
+ * @returns err(DatabaseError) if any errors occur whilst querying the database
  */
-export const verifyLoginOtp = async (otpToVerify: string, email: string) => {
-  const updatedDocument = await TokenModel.incrementAttemptsByEmail(email)
-
-  // Does not exist, return expired error message.
-  if (!updatedDocument) {
-    throw new InvalidOtpError('OTP has expired. Please request for a new OTP.')
-  }
-
-  // Too many attempts.
-  if (updatedDocument.numOtpAttempts! > MAX_OTP_ATTEMPTS) {
-    throw new InvalidOtpError(
-      'You have hit the max number of attempts. Please request for a new OTP.',
-    )
-  }
-
-  // Compare otp with saved hash.
-  const isOtpMatch = await bcrypt.compare(
-    otpToVerify,
-    updatedDocument.hashedOtp,
+export const verifyLoginOtp = (
+  otpToVerify: string,
+  email: string,
+): ResultAsync<true, InvalidOtpError | DatabaseError | ApplicationError> => {
+  return (
+    // Step 1: Increment login attempts.
+    incrementLoginAttempts(email)
+      // Step 2: Compare otp with saved hash.
+      .andThen(({ hashedOtp }) => compareOtpHash(otpToVerify, hashedOtp))
+      // Step 3: Remove token document from collection since hash matches.
+      .andThen(() => removeTokenOnSuccess(email))
+      // Step 4: Return true (as success).
+      .map(() => true)
   )
-
-  if (!isOtpMatch) {
-    throw new InvalidOtpError('OTP is invalid. Please try again.')
-  }
-
-  // Hashed OTP matches, remove from collection.
-  await TokenModel.findOneAndRemove({ email })
-  // Finally return true (as success).
-  return true
 }
 
 // Private helper functions
@@ -171,6 +157,43 @@ const hashOtp = (otpToHash: string, logMeta: Record<string, unknown> = {}) => {
   )
 }
 
+/**
+ * Compares otp with a given hash.
+ * @param otpToVerify The unhashed OTP to check match for
+ * @param hashedOtp The hashed OTP to check match for
+ * @param logMeta additional metadata for logging, if available
+ * @returns ok(true) if the hash matches
+ * @returns err(ApplicationError) if error occurs whilst comparing hashes
+ * @returns err(InvalidOtpError) if OTP hashes do not match
+ */
+const compareOtpHash = (
+  otpToVerify: string,
+  hashedOtp: string,
+  logMeta: Record<string, unknown> = {},
+): ResultAsync<true, ApplicationError | InvalidOtpError> => {
+  return ResultAsync.fromPromise(
+    bcrypt.compare(otpToVerify, hashedOtp),
+    (error) => {
+      logger.error({
+        message: 'bcrypt compare otp error',
+        meta: {
+          action: 'compareHash',
+          ...logMeta,
+        },
+        error,
+      })
+
+      return new ApplicationError()
+    },
+  ).andThen((isOtpMatch) => {
+    if (!isOtpMatch) {
+      return errAsync(new InvalidOtpError('OTP is invalid. Please try again.'))
+    }
+
+    return okAsync(isOtpMatch)
+  })
+}
+
 /**
  * Upserts the given otp hash into the document keyed by the given email
  * @param email the email to retrieve the current Token document for
@@ -202,3 +225,64 @@ const upsertOtp = (
     },
   )
 }
+
+/**
+ * Increment login attempts for the given email
+ * @param email the email to increment login attempts for
+ * @returns ok(updated document) if increment succeeds
+ * @returns err(DatabaseError) if any database error occurs whilst updating attempts
+ * @returns err(InvalidOtpError) if login has expired or if max number of attempts are reached
+ */
+const incrementLoginAttempts = (
+  email: string,
+): ResultAsync<ITokenSchema, InvalidOtpError | DatabaseError> => {
+  return ResultAsync.fromPromise(
+    TokenModel.incrementAttemptsByEmail(email),
+    (error) => {
+      logger.error({
+        message: 'Database increment OTP error',
+        meta: {
+          action: 'incrementAttempts',
+          email,
+        },
+        error,
+      })
+
+      return new DatabaseError()
+    },
+  ).andThen((upsertedDoc) => {
+    // Document does not exist, return expired error message.
+    if (!upsertedDoc || !upsertedDoc.numOtpAttempts) {
+      return errAsync(
+        new InvalidOtpError('OTP has expired. Please request for a new OTP.'),
+      )
+    }
+    if (upsertedDoc.numOtpAttempts > MAX_OTP_ATTEMPTS) {
+      return errAsync(
+        new InvalidOtpError(
+          'You have hit the max number of attempts. Please request for a new OTP.',
+        ),
+      )
+    }
+
+    return okAsync(upsertedDoc)
+  })
+}
+
+const removeTokenOnSuccess = (email: string) => {
+  return ResultAsync.fromPromise(
+    TokenModel.findOneAndRemove({ email }).exec(),
+    (error) => {
+      logger.error({
+        message: 'Database remove Token document error',
+        meta: {
+          action: 'removeTokenOnSuccess',
+          email,
+        },
+        error,
+      })
+
+      return new DatabaseError()
+    },
+  )
+}
diff --git a/src/types/token.ts b/src/types/token.ts
index f808a833d7..55d4332d71 100644
--- a/src/types/token.ts
+++ b/src/types/token.ts
@@ -18,5 +18,7 @@ export interface ITokenModel extends Model<ITokenSchema> {
     params: Omit<IToken, '_id' | 'numOtpSent'>,
   ) => Promise<ITokenSchema>
 
-  incrementAttemptsByEmail: (email: IToken['email']) => Promise<ITokenSchema>
+  incrementAttemptsByEmail: (
+    email: IToken['email'],
+  ) => Promise<ITokenSchema | null>
 }

From 7339dbbdab3b8b6f65bfed47250962971c79db86 Mon Sep 17 00:00:00 2001
From: Kar Rui Lau <karrui.lau@gmail.com>
Date: Thu, 17 Sep 2020 14:13:33 +0800
Subject: [PATCH 13/25] fix: fix typo in logger action and remove unused
 imports

---
 src/app/modules/auth/__tests__/auth.controller.spec.ts | 2 +-
 src/app/modules/auth/__tests__/auth.routes.spec.ts     | 2 +-
 src/app/modules/auth/auth.controller.ts                | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/app/modules/auth/__tests__/auth.controller.spec.ts b/src/app/modules/auth/__tests__/auth.controller.spec.ts
index 6dd29f9c96..e410e8017d 100644
--- a/src/app/modules/auth/__tests__/auth.controller.spec.ts
+++ b/src/app/modules/auth/__tests__/auth.controller.spec.ts
@@ -1,4 +1,4 @@
-import { err, errAsync, ok, okAsync } from 'neverthrow'
+import { errAsync, okAsync } from 'neverthrow'
 import expressHandler from 'tests/unit/backend/helpers/jest-express'
 import { mocked } from 'ts-jest/utils'
 
diff --git a/src/app/modules/auth/__tests__/auth.routes.spec.ts b/src/app/modules/auth/__tests__/auth.routes.spec.ts
index 26a9e0e5bf..adb416b648 100644
--- a/src/app/modules/auth/__tests__/auth.routes.spec.ts
+++ b/src/app/modules/auth/__tests__/auth.routes.spec.ts
@@ -1,5 +1,5 @@
 import { pick } from 'lodash'
-import { err, errAsync } from 'neverthrow'
+import { errAsync } from 'neverthrow'
 import supertest from 'supertest'
 import { CookieStore, setupApp } from 'tests/integration/helpers/express-setup'
 import dbHandler from 'tests/unit/backend/helpers/jest-db'
diff --git a/src/app/modules/auth/auth.controller.ts b/src/app/modules/auth/auth.controller.ts
index c8dc1ca1de..0cdd43157b 100644
--- a/src/app/modules/auth/auth.controller.ts
+++ b/src/app/modules/auth/auth.controller.ts
@@ -95,7 +95,7 @@ export const handleLoginSendOtp: RequestHandler<
   const { email } = req.body
   const requestIp = getRequestIp(req)
   const logMeta = {
-    action: 'handleSendLoginOtp',
+    action: 'handleLoginSendOtp',
     email,
     ip: requestIp,
   }

From e7994bd32517f16e6af2f711977a8cbb3b2b12a6 Mon Sep 17 00:00:00 2001
From: Kar Rui Lau <karrui.lau@gmail.com>
Date: Thu, 17 Sep 2020 15:04:45 +0800
Subject: [PATCH 14/25] refactor(MailUtils): update generateLoginOtpHtml to
 return ResultAsync

---
 src/app/services/mail.service.ts | 26 ++++++++++++++++++++------
 src/app/utils/mail.ts            | 21 +++++++++++++++++++--
 2 files changed, 39 insertions(+), 8 deletions(-)

diff --git a/src/app/services/mail.service.ts b/src/app/services/mail.service.ts
index 4803b4853b..4fd424d56d 100644
--- a/src/app/services/mail.service.ts
+++ b/src/app/services/mail.service.ts
@@ -313,16 +313,30 @@ export class MailService {
     otp: string
     ipAddress: string
   }) => {
+    const loginHtmlResult = await generateLoginOtpHtml({
+      appName: this.#appName,
+      appUrl: this.#appUrl,
+      ipAddress: ipAddress,
+      otp,
+    })
+
+    if (loginHtmlResult.isErr()) {
+      logger.error({
+        message: 'Error generating login OTP HTML',
+        meta: {
+          action: 'sendLoginOtp',
+        },
+        error: loginHtmlResult.error,
+      })
+
+      throw loginHtmlResult.error
+    }
+
     const mail: MailOptions = {
       to: recipient,
       from: this.#senderFromString,
       subject: `One-Time Password (OTP) for ${this.#appName}`,
-      html: await generateLoginOtpHtml({
-        appName: this.#appName,
-        appUrl: this.#appUrl,
-        ipAddress: ipAddress,
-        otp,
-      }),
+      html: loginHtmlResult.value,
       headers: {
         [EMAIL_HEADERS.emailType]: EmailType.LoginOtp,
       },
diff --git a/src/app/utils/mail.ts b/src/app/utils/mail.ts
index 87e25f420d..26c440204d 100644
--- a/src/app/utils/mail.ts
+++ b/src/app/utils/mail.ts
@@ -1,21 +1,38 @@
 import dedent from 'dedent-js'
 import ejs from 'ejs'
 import { flattenDeep } from 'lodash'
+import { ResultAsync } from 'neverthrow'
 import puppeteer from 'puppeteer-core'
 import validator from 'validator'
 
 import { IFormSchema, ISubmissionSchema } from 'src/types'
 
 import config from '../../config/config'
+import { createLoggerWithLabel } from '../../config/logger'
+
+const logger = createLoggerWithLabel(module)
 
 export const generateLoginOtpHtml = (htmlData: {
   otp: string
   appName: string
   appUrl: string
   ipAddress: string
-}): Promise<string> => {
+}): ResultAsync<string, Error> => {
   const pathToTemplate = `${process.cwd()}/src/app/views/templates/otp-email.server.view.html`
-  return ejs.renderFile(pathToTemplate, htmlData)
+  return ResultAsync.fromPromise(
+    ejs.renderFile(pathToTemplate, htmlData),
+    (error) => {
+      logger.error({
+        message: 'Error occurred whilst rendering login otp ejs',
+        meta: {
+          action: 'generateLoginOtpHtml',
+        },
+        error,
+      })
+
+      return error as Error
+    },
+  )
 }
 
 export const generateVerificationOtpHtml = ({

From 7269bf35b7c7eb6f3cd3fc79aaaa5c42ac559b5c Mon Sep 17 00:00:00 2001
From: Kar Rui Lau <karrui.lau@gmail.com>
Date: Thu, 17 Sep 2020 16:02:40 +0800
Subject: [PATCH 15/25] refactor: update sendLoginOtp to return ResultAsync

---
 src/app/modules/mail/mail.errors.ts           |  9 +++
 src/app/services/mail.service.ts              | 59 ++++++++++---------
 src/app/utils/mail.ts                         |  5 +-
 .../backend/services/mail.service.spec.ts     | 44 ++++++++------
 4 files changed, 70 insertions(+), 47 deletions(-)
 create mode 100644 src/app/modules/mail/mail.errors.ts

diff --git a/src/app/modules/mail/mail.errors.ts b/src/app/modules/mail/mail.errors.ts
new file mode 100644
index 0000000000..a9726ead37
--- /dev/null
+++ b/src/app/modules/mail/mail.errors.ts
@@ -0,0 +1,9 @@
+import { ApplicationError } from '../core/core.errors'
+
+export class MailSendError 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/mail.service.ts b/src/app/services/mail.service.ts
index 4fd424d56d..3183f38e08 100644
--- a/src/app/services/mail.service.ts
+++ b/src/app/services/mail.service.ts
@@ -1,5 +1,6 @@
 import { get, inRange, isEmpty } from 'lodash'
 import moment from 'moment-timezone'
+import { err, errAsync, ResultAsync } from 'neverthrow'
 import Mail from 'nodemailer/lib/mailer'
 import promiseRetry from 'promise-retry'
 import { OperationOptions } from 'retry'
@@ -16,6 +17,7 @@ import {
   ISubmissionSchema,
 } from '../../types'
 import { EMAIL_HEADERS, EmailType } from '../constants/mail'
+import { MailSendError } from '../modules/mail/mail.errors'
 import {
   generateAutoreplyHtml,
   generateAutoreplyPdf,
@@ -302,9 +304,10 @@ export class MailService {
    * Sends a login otp email to a valid email
    * @param recipient the recipient email address
    * @param otp the OTP to send
-   * @throws error if mail fails, to be handled by the caller
+   * @returns ok(never) if sending of mail succeeds
+   * @returns err(MailSendError) if sending of mail fails, to be handled by the caller
    */
-  sendLoginOtp = async ({
+  sendLoginOtp = ({
     recipient,
     otp,
     ipAddress,
@@ -312,37 +315,39 @@ export class MailService {
     recipient: string
     otp: string
     ipAddress: string
-  }) => {
-    const loginHtmlResult = await generateLoginOtpHtml({
+  }): ResultAsync<any, MailSendError> => {
+    return generateLoginOtpHtml({
       appName: this.#appName,
       appUrl: this.#appUrl,
       ipAddress: ipAddress,
       otp,
-    })
-
-    if (loginHtmlResult.isErr()) {
-      logger.error({
-        message: 'Error generating login OTP HTML',
-        meta: {
-          action: 'sendLoginOtp',
+    }).andThen((loginHtml) => {
+      const mail: MailOptions = {
+        to: recipient,
+        from: this.#senderFromString,
+        subject: `One-Time Password (OTP) for ${this.#appName}`,
+        html: loginHtml,
+        headers: {
+          [EMAIL_HEADERS.emailType]: EmailType.LoginOtp,
         },
-        error: loginHtmlResult.error,
-      })
-
-      throw loginHtmlResult.error
-    }
-
-    const mail: MailOptions = {
-      to: recipient,
-      from: this.#senderFromString,
-      subject: `One-Time Password (OTP) for ${this.#appName}`,
-      html: loginHtmlResult.value,
-      headers: {
-        [EMAIL_HEADERS.emailType]: EmailType.LoginOtp,
-      },
-    }
+      }
 
-    return this.#sendNodeMail(mail, { mailId: 'OTP' })
+      return ResultAsync.fromPromise(
+        this.#sendNodeMail(mail, { mailId: 'OTP' }),
+        (error) => {
+          logger.error({
+            message: 'Error sending login OTP to email',
+            meta: {
+              action: 'sendLoginOtp',
+              recipient,
+            },
+            error,
+          })
+
+          return new MailSendError()
+        },
+      )
+    })
   }
 
   /**
diff --git a/src/app/utils/mail.ts b/src/app/utils/mail.ts
index 26c440204d..f8179ed539 100644
--- a/src/app/utils/mail.ts
+++ b/src/app/utils/mail.ts
@@ -9,6 +9,7 @@ import { IFormSchema, ISubmissionSchema } from 'src/types'
 
 import config from '../../config/config'
 import { createLoggerWithLabel } from '../../config/logger'
+import { MailSendError } from '../modules/mail/mail.errors'
 
 const logger = createLoggerWithLabel(module)
 
@@ -17,7 +18,7 @@ export const generateLoginOtpHtml = (htmlData: {
   appName: string
   appUrl: string
   ipAddress: string
-}): ResultAsync<string, Error> => {
+}): ResultAsync<string, MailSendError> => {
   const pathToTemplate = `${process.cwd()}/src/app/views/templates/otp-email.server.view.html`
   return ResultAsync.fromPromise(
     ejs.renderFile(pathToTemplate, htmlData),
@@ -30,7 +31,7 @@ export const generateLoginOtpHtml = (htmlData: {
         error,
       })
 
-      return error as Error
+      return new MailSendError()
     },
   )
 }
diff --git a/tests/unit/backend/services/mail.service.spec.ts b/tests/unit/backend/services/mail.service.spec.ts
index 5193414b88..5c75da652a 100644
--- a/tests/unit/backend/services/mail.service.spec.ts
+++ b/tests/unit/backend/services/mail.service.spec.ts
@@ -3,6 +3,7 @@ import moment from 'moment-timezone'
 import Mail, { Attachment } from 'nodemailer/lib/mailer'
 import { ImportMock } from 'ts-mock-imports'
 
+import { MailSendError } from 'src/app/modules/mail/mail.errors'
 import {
   AutoreplySummaryRenderData,
   MailOptions,
@@ -48,7 +49,7 @@ describe('mail.service', () => {
   })
 
   describe('Constructor', () => {
-    it('should throw error when invalid senderMail param is passed ', () => {
+    it('should throw error when invalid senderMail param is passed', () => {
       // Arrange
       const invalidParams = {
         transporter: mockTransporter,
@@ -211,12 +212,14 @@ describe('mail.service', () => {
         to: MOCK_VALID_EMAIL,
         from: MOCK_SENDER_STRING,
         subject: `One-Time Password (OTP) for ${MOCK_APP_NAME}`,
-        html: await MailUtils.generateLoginOtpHtml({
-          otp: MOCK_OTP,
-          appName: MOCK_APP_NAME,
-          appUrl: MOCK_APP_URL,
-          ipAddress: MOCK_IP,
-        }),
+        html: (
+          await MailUtils.generateLoginOtpHtml({
+            otp: MOCK_OTP,
+            appName: MOCK_APP_NAME,
+            appUrl: MOCK_APP_URL,
+            ipAddress: MOCK_IP,
+          })
+        )._unsafeUnwrap(),
         headers: {
           // Hardcode in tests in case something changes this.
           'X-Formsg-Email-Type': 'Login OTP',
@@ -233,31 +236,33 @@ describe('mail.service', () => {
       const expectedArgument = await generateExpectedArg()
 
       // Act
-      const pendingSend = mailService.sendLoginOtp({
+      const actualResult = await mailService.sendLoginOtp({
         recipient: MOCK_VALID_EMAIL,
         otp: MOCK_OTP,
         ipAddress: MOCK_IP,
       })
 
       // Assert
-      await expect(pendingSend).resolves.toEqual(mockedResponse)
+      expect(actualResult.isOk()).toBe(true)
+      expect(actualResult._unsafeUnwrap()).toEqual(mockedResponse)
       // Check arguments passed to sendNodeMail
       expect(sendMailSpy).toHaveBeenCalledTimes(1)
       expect(sendMailSpy).toHaveBeenCalledWith(expectedArgument)
     })
 
-    it('should reject with error when email is invalid', async () => {
+    it('should return a MailSendError when email is invalid', async () => {
       // Arrange
       const invalidEmail = 'notAnEmail'
 
       // Act
-      const pendingSend = mailService.sendLoginOtp({
+      const actualResult = await mailService.sendLoginOtp({
         recipient: invalidEmail,
         otp: MOCK_OTP,
         ipAddress: MOCK_IP,
       })
       // Assert
-      await expect(pendingSend).rejects.toThrowError('Invalid email error')
+      expect(actualResult.isErr()).toBe(true)
+      expect(actualResult._unsafeUnwrapErr()).toBeInstanceOf(MailSendError)
     })
 
     it('should autoretry when 4xx error is thrown by sendNodeMail and pass if second try passes', async () => {
@@ -275,14 +280,15 @@ describe('mail.service', () => {
       const expectedArgument = await generateExpectedArg()
 
       // Act
-      const pendingSendLoginOtp = mailService.sendLoginOtp({
+      const actualResult = await mailService.sendLoginOtp({
         recipient: MOCK_VALID_EMAIL,
         otp: MOCK_OTP,
         ipAddress: MOCK_IP,
       })
 
       // Assert
-      await expect(pendingSendLoginOtp).resolves.toEqual(mockedResponse)
+      expect(actualResult.isOk()).toBe(true)
+      expect(actualResult._unsafeUnwrap()).toEqual(mockedResponse)
       // Check arguments passed to sendNodeMail
       // Should have been called two times since it rejected the first one and
       // resolved
@@ -301,14 +307,15 @@ describe('mail.service', () => {
       const expectedArgument = await generateExpectedArg()
 
       // Act
-      const pendingSendLoginOtp = mailService.sendLoginOtp({
+      const actualResult = await mailService.sendLoginOtp({
         recipient: MOCK_VALID_EMAIL,
         otp: MOCK_OTP,
         ipAddress: MOCK_IP,
       })
 
       // Assert
-      await expect(pendingSendLoginOtp).rejects.toEqual(mock4xxReject)
+      expect(actualResult.isErr()).toBe(true)
+      expect(actualResult._unsafeUnwrapErr()).toBeInstanceOf(MailSendError)
       // Check arguments passed to sendNodeMail
       // Should have been called MOCK_RETRY_COUNT + 1 times
       expect(sendMailSpy).toHaveBeenCalledTimes(MOCK_RETRY_COUNT + 1)
@@ -329,14 +336,15 @@ describe('mail.service', () => {
       const expectedArgument = await generateExpectedArg()
 
       // Act
-      const pendingSendLoginOtp = mailService.sendLoginOtp({
+      const actualResult = await mailService.sendLoginOtp({
         recipient: MOCK_VALID_EMAIL,
         otp: MOCK_OTP,
         ipAddress: MOCK_IP,
       })
 
       // Assert
-      await expect(pendingSendLoginOtp).rejects.toEqual(mockError)
+      expect(actualResult.isErr()).toBe(true)
+      expect(actualResult._unsafeUnwrapErr()).toBeInstanceOf(MailSendError)
       // Check arguments passed to sendNodeMail
       // Should only invoke two times and stop since the second rejected value
       // is non-4xx error.

From a52f370e440ae27a2c621d89e7a6634bd77e6852 Mon Sep 17 00:00:00 2001
From: Kar Rui Lau <karrui.lau@gmail.com>
Date: Thu, 17 Sep 2020 16:35:25 +0800
Subject: [PATCH 16/25] refactor: update handleLoginSendOtp to chain
 ResultAsync calls

---
 .../auth/__tests__/auth.controller.spec.ts    |  9 +-
 .../auth/__tests__/auth.routes.spec.ts        | 23 ++---
 src/app/modules/auth/auth.controller.ts       | 97 +++++++------------
 3 files changed, 53 insertions(+), 76 deletions(-)

diff --git a/src/app/modules/auth/__tests__/auth.controller.spec.ts b/src/app/modules/auth/__tests__/auth.controller.spec.ts
index e410e8017d..9d9794d024 100644
--- a/src/app/modules/auth/__tests__/auth.controller.spec.ts
+++ b/src/app/modules/auth/__tests__/auth.controller.spec.ts
@@ -6,6 +6,7 @@ import MailService from 'src/app/services/mail.service'
 import { IAgencySchema, IUserSchema } from 'src/types'
 
 import { ApplicationError, DatabaseError } from '../../core/core.errors'
+import { MailSendError } from '../../mail/mail.errors'
 import * as UserService from '../../user/user.service'
 import * as AuthController from '../auth.controller'
 import { InvalidDomainError, InvalidOtpError } from '../auth.errors'
@@ -76,7 +77,7 @@ describe('auth.controller', () => {
         okAsync(<IAgencySchema>{}),
       )
       MockAuthService.createLoginOtp.mockReturnValueOnce(okAsync(MOCK_OTP))
-      MockMailService.sendLoginOtp.mockResolvedValueOnce(true)
+      MockMailService.sendLoginOtp.mockReturnValueOnce(okAsync(true))
 
       // Act
       await AuthController.handleLoginSendOtp(MOCK_REQ, mockRes, jest.fn())
@@ -137,8 +138,8 @@ describe('auth.controller', () => {
       )
       // Mock createLoginOtp success but sendLoginOtp failure.
       MockAuthService.createLoginOtp.mockReturnValueOnce(okAsync(MOCK_OTP))
-      MockMailService.sendLoginOtp.mockRejectedValueOnce(
-        new Error('send error'),
+      MockMailService.sendLoginOtp.mockReturnValueOnce(
+        errAsync(new MailSendError('send error')),
       )
 
       // Act
@@ -147,7 +148,7 @@ describe('auth.controller', () => {
       // Assert
       expect(mockRes.status).toBeCalledWith(500)
       expect(mockRes.send).toBeCalledWith(
-        'Error sending OTP. Please try again later and if the problem persists, contact us.',
+        'Failed to send login OTP. Please try again later and if the problem persists, contact us.',
       )
       // Services should have been invoked.
       expect(MockAuthService.createLoginOtp).toHaveBeenCalledTimes(1)
diff --git a/src/app/modules/auth/__tests__/auth.routes.spec.ts b/src/app/modules/auth/__tests__/auth.routes.spec.ts
index adb416b648..d142236749 100644
--- a/src/app/modules/auth/__tests__/auth.routes.spec.ts
+++ b/src/app/modules/auth/__tests__/auth.routes.spec.ts
@@ -1,5 +1,5 @@
 import { pick } from 'lodash'
-import { errAsync } from 'neverthrow'
+import { errAsync, okAsync } from 'neverthrow'
 import supertest from 'supertest'
 import { CookieStore, setupApp } from 'tests/integration/helpers/express-setup'
 import dbHandler from 'tests/unit/backend/helpers/jest-db'
@@ -10,6 +10,7 @@ import * as OtpUtils from 'src/app/utils/otp'
 import { IAgencySchema } from 'src/types'
 
 import { ApplicationError, DatabaseError } from '../../core/core.errors'
+import { MailSendError } from '../../mail/mail.errors'
 import * as UserService from '../../user/user.service'
 import { AuthRouter } from '../auth.routes'
 import * as AuthService from '../auth.service'
@@ -180,7 +181,7 @@ describe('auth.routes', () => {
       // Arrange
       const sendLoginOtpSpy = jest
         .spyOn(MailService, 'sendLoginOtp')
-        .mockRejectedValueOnce(new Error('some error'))
+        .mockReturnValueOnce(errAsync(new MailSendError('some error')))
 
       // Act
       const response = await request
@@ -191,16 +192,15 @@ describe('auth.routes', () => {
       expect(sendLoginOtpSpy).toHaveBeenCalled()
       expect(response.status).toEqual(500)
       expect(response.text).toEqual(
-        'Error sending OTP. Please try again later and if the problem persists, contact us.',
+        'Failed to send login OTP. Please try again later and if the problem persists, contact us.',
       )
     })
 
     it('should return 500 when validating domain returns a database error', async () => {
       // Arrange
-      const mockErrorString = 'Unable to validate email domain.'
       const getAgencySpy = jest
         .spyOn(AuthService, 'validateEmailDomain')
-        .mockReturnValueOnce(errAsync(new DatabaseError(mockErrorString)))
+        .mockReturnValueOnce(errAsync(new DatabaseError()))
 
       // Act
       const response = await request
@@ -210,14 +210,16 @@ describe('auth.routes', () => {
       // Assert
       expect(getAgencySpy).toBeCalled()
       expect(response.status).toEqual(500)
-      expect(response.text).toEqual(mockErrorString)
+      expect(response.text).toEqual(
+        'Failed to send login OTP. Please try again later and if the problem persists, contact us.',
+      )
     })
 
     it('should return 200 when otp is sent successfully', async () => {
       // Arrange
       const sendLoginOtpSpy = jest
         .spyOn(MailService, 'sendLoginOtp')
-        .mockResolvedValueOnce(true)
+        .mockReturnValueOnce(okAsync(true))
 
       // Act
       const response = await request
@@ -325,10 +327,9 @@ describe('auth.routes', () => {
 
     it('should return 500 when validating domain returns a database error', async () => {
       // Arrange
-      const mockErrorString = 'Unable to validate email domain.'
       const getAgencySpy = jest
         .spyOn(AuthService, 'validateEmailDomain')
-        .mockReturnValueOnce(errAsync(new DatabaseError(mockErrorString)))
+        .mockReturnValueOnce(errAsync(new DatabaseError()))
 
       // Act
       const response = await request
@@ -338,7 +339,7 @@ describe('auth.routes', () => {
       // Assert
       expect(getAgencySpy).toBeCalled()
       expect(response.status).toEqual(500)
-      expect(response.text).toEqual(mockErrorString)
+      expect(response.text).toEqual('Something went wrong. Please try again.')
     })
 
     it('should return 422 when hash does not exist for body.otp', async () => {
@@ -511,7 +512,7 @@ describe('auth.routes', () => {
 // Helper functions
 const requestForOtp = async (email: string) => {
   // Set that so no real mail is sent.
-  jest.spyOn(MailService, 'sendLoginOtp').mockResolvedValue(true)
+  jest.spyOn(MailService, 'sendLoginOtp').mockReturnValue(okAsync(true))
 
   const response = await request.post('/auth/sendotp').send({ email })
   expect(response.text).toEqual(`OTP sent to ${email}!`)
diff --git a/src/app/modules/auth/auth.controller.ts b/src/app/modules/auth/auth.controller.ts
index 0cdd43157b..77b450d0ae 100644
--- a/src/app/modules/auth/auth.controller.ts
+++ b/src/app/modules/auth/auth.controller.ts
@@ -1,4 +1,3 @@
-import to from 'await-to-js'
 import { RequestHandler } from 'express'
 import { ParamsDictionary } from 'express-serve-static-core'
 import { StatusCodes } from 'http-status-codes'
@@ -9,6 +8,7 @@ import { LINKS } from '../../../shared/constants'
 import MailService from '../../services/mail.service'
 import { getRequestIp } from '../../utils/request'
 import { ApplicationError, DatabaseError } from '../core/core.errors'
+import { MailSendError } from '../mail/mail.errors'
 import * as UserService from '../user/user.service'
 
 import { InvalidDomainError, InvalidOtpError } from './auth.errors'
@@ -35,6 +35,7 @@ const mapRouteError = (error: ApplicationError, coreErrorMessage?: string) => {
         statusCode: StatusCodes.UNPROCESSABLE_ENTITY,
         errorMessage: error.message,
       }
+    case MailSendError:
     case ApplicationError:
     case DatabaseError:
       return {
@@ -100,68 +101,42 @@ export const handleLoginSendOtp: RequestHandler<
     ip: requestIp,
   }
 
-  const validateResult = await AuthService.validateEmailDomain(email)
-  if (validateResult.isErr()) {
-    const { error } = validateResult
-    logger.error({
-      message: 'Domain validation error',
-      meta: {
-        action: 'handleLoginSendOtp',
-        ip: getRequestIp(req),
-        email,
-      },
-      error,
-    })
-    const { errorMessage, statusCode } = mapRouteError(error)
-    return res.status(statusCode).send(errorMessage)
-  }
-
-  // Create OTP.
-  const otpResult = await AuthService.createLoginOtp(email)
-  if (otpResult.isErr()) {
-    const { error } = otpResult
-    logger.error({
-      message: 'Error generating OTP',
-      meta: logMeta,
-      error,
-    })
-
-    const { errorMessage, statusCode } = mapRouteError(
-      error,
-      /* coreErrorMessage= */ 'Failed to send login OTP. Please try again later and if the problem persists, contact us.',
-    )
-    return res.status(statusCode).send(errorMessage)
-  }
-
-  // Send OTP.
-  const [sendErr] = await to(
-    MailService.sendLoginOtp({
-      recipient: email,
-      otp: otpResult.value,
-      ipAddress: requestIp,
-    }),
-  )
-  if (sendErr) {
-    logger.error({
-      message: 'Error mailing OTP',
-      meta: logMeta,
-      error: sendErr,
-    })
-
-    return res
-      .status(StatusCodes.INTERNAL_SERVER_ERROR)
-      .send(
-        'Error sending OTP. Please try again later and if the problem persists, contact us.',
+  return (
+    // Step 1: Validate email domain.
+    AuthService.validateEmailDomain(email)
+      // Step 2: Create login OTP.
+      .andThen(() => AuthService.createLoginOtp(email))
+      // Step 3: Send login OTP to email address.
+      .andThen((otp) =>
+        MailService.sendLoginOtp({
+          recipient: email,
+          otp,
+          ipAddress: requestIp,
+        }),
       )
-  }
+      // Step 4a: Successfully sent login otp.
+      .map(() => {
+        logger.info({
+          message: 'Login OTP sent successfully',
+          meta: logMeta,
+        })
 
-  // Successfully sent login otp.
-  logger.info({
-    message: 'Login OTP sent successfully',
-    meta: logMeta,
-  })
-
-  return res.status(StatusCodes.OK).send(`OTP sent to ${email}!`)
+        return res.status(StatusCodes.OK).send(`OTP sent to ${email}!`)
+      })
+      // Step 4b: Error occurred whilst sending otp.
+      .mapErr((error) => {
+        logger.error({
+          message: 'Error sending login OTP',
+          meta: logMeta,
+          error,
+        })
+        const { errorMessage, statusCode } = mapRouteError(
+          error,
+          /* coreErrorMessage=*/ 'Failed to send login OTP. Please try again later and if the problem persists, contact us.',
+        )
+        return res.status(statusCode).send(errorMessage)
+      })
+  )
 }
 
 /**

From 4590ec25e7534ff9400cf4f3fe67737f148f758f Mon Sep 17 00:00:00 2001
From: Kar Rui Lau <karrui.lau@gmail.com>
Date: Thu, 17 Sep 2020 16:48:21 +0800
Subject: [PATCH 17/25] refactor(AuthController): update handleLoginVerifyOtp
 to chain

---
 src/app/modules/auth/auth.controller.ts | 106 +++++++++++-------------
 1 file changed, 49 insertions(+), 57 deletions(-)

diff --git a/src/app/modules/auth/auth.controller.ts b/src/app/modules/auth/auth.controller.ts
index 77b450d0ae..568986c06f 100644
--- a/src/app/modules/auth/auth.controller.ts
+++ b/src/app/modules/auth/auth.controller.ts
@@ -154,16 +154,18 @@ export const handleLoginVerifyOtp: RequestHandler<
   // Joi validation ensures existence.
   const { email, otp } = req.body
 
+  const logMeta = {
+    action: 'handleLoginVerifyOtp',
+    email,
+    ip: getRequestIp(req),
+  }
+
   const validateResult = await AuthService.validateEmailDomain(email)
   if (validateResult.isErr()) {
     const { error } = validateResult
     logger.error({
       message: 'Domain validation error',
-      meta: {
-        action: 'handleLoginVerifyOtp',
-        ip: getRequestIp(req),
-        email,
-      },
+      meta: logMeta,
       error,
     })
     const { errorMessage, statusCode } = mapRouteError(error)
@@ -173,65 +175,55 @@ export const handleLoginVerifyOtp: RequestHandler<
   // Since there is no error, agency is retrieved from validation.
   const agency = validateResult.value
 
-  const logMeta = {
-    action: 'handleLoginVerifyOtp',
-    email,
-    ip: getRequestIp(req),
-  }
-
-  const verifyResult = await AuthService.verifyLoginOtp(otp, email)
-
-  if (verifyResult.isErr()) {
-    const { error } = verifyResult
-    logger.warn({
-      message:
-        error instanceof InvalidOtpError
-          ? 'Login OTP is invalid'
-          : 'Error occurred when trying to validate login OTP',
-      meta: logMeta,
-      error,
-    })
-
-    const { errorMessage, statusCode } = mapRouteError(
-      error,
-      /* coreErrorMessage= */ 'Failed to validate OTP. Please try again later and if the problem persists, contact us.',
-    )
-    return res.status(statusCode).send(errorMessage)
-  }
+  return AuthService.verifyLoginOtp(otp, email)
+    .map(async () => {
+      // OTP is valid, proceed to login user.
+      try {
+        // TODO (#317): remove usage of non-null assertion
+        const user = await UserService.retrieveUser(email, agency)
+        // Create user object to return to frontend.
+        const userObj = { ...user.toObject(), agency }
 
-  // OTP is valid, proceed to login user.
-  try {
-    // TODO (#317): remove usage of non-null assertion
-    const user = await UserService.retrieveUser(email, agency)
-    // Create user object to return to frontend.
-    const userObj = { ...user.toObject(), agency }
+        if (!req.session) {
+          throw new Error('req.session not found')
+        }
 
-    if (!req.session) {
-      throw new Error('req.session not found')
-    }
+        // TODO(#212): Should store only userId in session.
+        // Add user info to session.
+        req.session.user = userObj as SessionUser
+        logger.info({
+          message: `Successfully logged in user ${user.email}`,
+          meta: logMeta,
+        })
 
-    // TODO(#212): Should store only userId in session.
-    // Add user info to session.
-    req.session.user = userObj as SessionUser
-    logger.info({
-      message: `Successfully logged in user ${user.email}`,
-      meta: logMeta,
-    })
+        return res.status(StatusCodes.OK).send(userObj)
+      } catch (err) {
+        logger.error({
+          message: 'Error logging in user',
+          meta: logMeta,
+          error: err,
+        })
 
-    return res.status(StatusCodes.OK).send(userObj)
-  } catch (err) {
-    logger.error({
-      message: 'Error logging in user',
-      meta: logMeta,
-      error: err,
+        return res
+          .status(StatusCodes.INTERNAL_SERVER_ERROR)
+          .send(
+            `User signin failed. Please try again later and if the problem persists, submit our Support Form (${LINKS.supportFormLink}).`,
+          )
+      }
     })
+    .mapErr((error) => {
+      logger.warn({
+        message: 'Error occurred when trying to validate login OTP',
+        meta: logMeta,
+        error,
+      })
 
-    return res
-      .status(StatusCodes.INTERNAL_SERVER_ERROR)
-      .send(
-        `User signin failed. Please try again later and if the problem persists, submit our Support Form (${LINKS.supportFormLink}).`,
+      const { errorMessage, statusCode } = mapRouteError(
+        error,
+        /* coreErrorMessage= */ 'Failed to validate OTP. Please try again later and if the problem persists, contact us.',
       )
-  }
+      return res.status(statusCode).send(errorMessage)
+    })
 }
 
 export const handleSignout: RequestHandler = async (req, res) => {

From e92a63f044966127dbf2a9ff3a8bff7d0e5b733e Mon Sep 17 00:00:00 2001
From: Kar Rui Lau <karrui.lau@gmail.com>
Date: Thu, 17 Sep 2020 16:50:17 +0800
Subject: [PATCH 18/25] fix: remove unused imports and remove null assertion

---
 src/app/services/mail.service.ts                 | 2 +-
 tests/unit/backend/services/mail.service.spec.ts | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/app/services/mail.service.ts b/src/app/services/mail.service.ts
index 3183f38e08..f325d2f536 100644
--- a/src/app/services/mail.service.ts
+++ b/src/app/services/mail.service.ts
@@ -1,6 +1,6 @@
 import { get, inRange, isEmpty } from 'lodash'
 import moment from 'moment-timezone'
-import { err, errAsync, ResultAsync } from 'neverthrow'
+import { ResultAsync } from 'neverthrow'
 import Mail from 'nodemailer/lib/mailer'
 import promiseRetry from 'promise-retry'
 import { OperationOptions } from 'retry'
diff --git a/tests/unit/backend/services/mail.service.spec.ts b/tests/unit/backend/services/mail.service.spec.ts
index 5c75da652a..798022b4ea 100644
--- a/tests/unit/backend/services/mail.service.spec.ts
+++ b/tests/unit/backend/services/mail.service.spec.ts
@@ -858,7 +858,7 @@ describe('mail.service', () => {
         html: expectedMailBody,
         // Attachments should be concatted with mock pdf response
         attachments: [
-          ...MOCK_AUTOREPLY_PARAMS.attachments!,
+          ...(MOCK_AUTOREPLY_PARAMS.attachments ?? []),
           {
             content: MOCK_PDF,
             filename: 'response.pdf',

From a0f5ad294428f77dac8a0f24dbd85f9bfc8a848a Mon Sep 17 00:00:00 2001
From: Kar Rui Lau <karrui.lau@gmail.com>
Date: Thu, 17 Sep 2020 17:10:31 +0800
Subject: [PATCH 19/25] feat: update comments and tests names to return error
 instead of throw

---
 src/app/modules/auth/__tests__/auth.controller.spec.ts | 8 ++++----
 src/app/modules/auth/__tests__/auth.routes.spec.ts     | 2 +-
 src/app/modules/auth/__tests__/auth.service.spec.ts    | 8 ++++----
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/src/app/modules/auth/__tests__/auth.controller.spec.ts b/src/app/modules/auth/__tests__/auth.controller.spec.ts
index 9d9794d024..95661ed6a7 100644
--- a/src/app/modules/auth/__tests__/auth.controller.spec.ts
+++ b/src/app/modules/auth/__tests__/auth.controller.spec.ts
@@ -205,7 +205,7 @@ describe('auth.controller', () => {
       expect(mockRes.send).toBeCalledWith(expectedError.message)
     })
 
-    it('should return 422 when verifying login OTP throws an InvalidOtpError', async () => {
+    it('should return 422 when verifying login OTP returns an InvalidOtpError', async () => {
       // Arrange
       const mockRes = expressHandler.mockResponse()
       const expectedInvalidOtpError = new InvalidOtpError()
@@ -228,7 +228,7 @@ describe('auth.controller', () => {
       expect(MockUserService.retrieveUser).not.toHaveBeenCalled()
     })
 
-    it('should return 500 when verifying login OTP throws a non-InvalidOtpError', async () => {
+    it('should return 500 when verifying login OTP returns a non-InvalidOtpError', async () => {
       // Arrange
       const mockRes = expressHandler.mockResponse()
       MockAuthService.validateEmailDomain.mockReturnValueOnce(
@@ -252,7 +252,7 @@ describe('auth.controller', () => {
       expect(MockUserService.retrieveUser).not.toHaveBeenCalled()
     })
 
-    it('should return 500 when an error is thrown while upserting user', async () => {
+    it('should return 500 when an error is returned while upserting user', async () => {
       // Arrange
       const mockRes = expressHandler.mockResponse()
       MockAuthService.validateEmailDomain.mockReturnValueOnce(
@@ -320,7 +320,7 @@ describe('auth.controller', () => {
       expect(mockRes.sendStatus).toBeCalledWith(400)
     })
 
-    it('should return 500 when error is thrown when destroying session', async () => {
+    it('should return 500 when error is returned when destroying session', async () => {
       // Arrange
       const mockDestroyWithErr = jest
         .fn()
diff --git a/src/app/modules/auth/__tests__/auth.routes.spec.ts b/src/app/modules/auth/__tests__/auth.routes.spec.ts
index d142236749..ee33e65756 100644
--- a/src/app/modules/auth/__tests__/auth.routes.spec.ts
+++ b/src/app/modules/auth/__tests__/auth.routes.spec.ts
@@ -443,7 +443,7 @@ describe('auth.routes', () => {
       // Request for OTP so the hash exists.
       await requestForOtp(VALID_EMAIL)
 
-      // Mock error thrown when creating user
+      // Mock error returned when creating user
       const upsertSpy = jest
         .spyOn(UserService, 'retrieveUser')
         .mockRejectedValueOnce(new Error('some error'))
diff --git a/src/app/modules/auth/__tests__/auth.service.spec.ts b/src/app/modules/auth/__tests__/auth.service.spec.ts
index eaab218870..77d9c8c3c4 100644
--- a/src/app/modules/auth/__tests__/auth.service.spec.ts
+++ b/src/app/modules/auth/__tests__/auth.service.spec.ts
@@ -89,7 +89,7 @@ describe('auth.service', () => {
       await expect(TokenModel.countDocuments()).resolves.toEqual(1)
     })
 
-    it('should throw InvalidDomainError when email is invalid', async () => {
+    it('should return with InvalidDomainError when email is invalid', async () => {
       // Arrange
       const notAnEmail = 'not an email'
 
@@ -123,7 +123,7 @@ describe('auth.service', () => {
       await expect(TokenModel.countDocuments()).resolves.toEqual(0)
     })
 
-    it('should throw InvalidOtpError when Token document cannot be retrieved', async () => {
+    it('should return with InvalidOtpError when Token document cannot be retrieved', async () => {
       // Arrange
       // No OTP requested; should have no documents prior to acting.
       await expect(TokenModel.countDocuments()).resolves.toEqual(0)
@@ -142,7 +142,7 @@ describe('auth.service', () => {
       expect(actualResult._unsafeUnwrapErr()).toEqual(expectedError)
     })
 
-    it('should throw InvalidOtpError when verification has been attempted too many times', async () => {
+    it('should return with InvalidOtpError when verification has been attempted too many times', async () => {
       // Arrange
       // Add a Token document to verify against.
       await AuthService.createLoginOtp(VALID_EMAIL)
@@ -166,7 +166,7 @@ describe('auth.service', () => {
       expect(actualResult._unsafeUnwrapErr()).toEqual(expectedError)
     })
 
-    it('should throw InvalidOtpError when the OTP hash does not match', async () => {
+    it('should return with InvalidOtpError when the OTP hash does not match', async () => {
       // Arrange
       // Add a Token document to verify against.
       await AuthService.createLoginOtp(VALID_EMAIL)

From 8046f040573cbcf7d61b13c7a11d8375a58431ab Mon Sep 17 00:00:00 2001
From: Kar Rui Lau <karrui.lau@gmail.com>
Date: Fri, 18 Sep 2020 00:32:48 +0800
Subject: [PATCH 20/25] chore: update travis.yml

It will now install a pinned version of localstack and use the chrome addon instead of downloading from apt
---
 .travis.yml | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/.travis.yml b/.travis.yml
index 1188c390ba..c5d647b8bb 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -11,10 +11,8 @@ cache:
   - npm
   - pip
 
-# Setup browsers needed for end-to-end tests
-before_install:
-  - sudo apt-get clean && sudo apt-get update
-  - sudo apt-get install -y dpkg google-chrome-stable fluxbox chromium-browser
+addons:
+  chrome: stable
 
 notifications:
   email:
@@ -26,9 +24,8 @@ notifications:
     on_failure: always
 
 before_script:
-  - fluxbox >/dev/null 2>&1 &
   - pyenv global 3.7.1
-  - pip3 install --user localstack[full]
+  - pip3 install 'localstack==0.11.5' --force-reinstall
 
 script:
   - set -e

From 90fdc5d9f2ec998aa2248fed1bcf73556a11e81d Mon Sep 17 00:00:00 2001
From: Kar Rui Lau <karrui.lau@gmail.com>
Date: Mon, 21 Sep 2020 13:02:31 +0800
Subject: [PATCH 21/25] feat(AuthController): add explicit undefined check for
 req.session

---
 src/app/modules/auth/auth.controller.ts    | 4 ++--
 tests/unit/backend/helpers/jest-express.ts | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/app/modules/auth/auth.controller.ts b/src/app/modules/auth/auth.controller.ts
index 568986c06f..f617589c91 100644
--- a/src/app/modules/auth/auth.controller.ts
+++ b/src/app/modules/auth/auth.controller.ts
@@ -227,7 +227,7 @@ export const handleLoginVerifyOtp: RequestHandler<
 }
 
 export const handleSignout: RequestHandler = async (req, res) => {
-  if (isEmpty(req.session)) {
+  if (!req.session || isEmpty(req.session)) {
     logger.error({
       message: 'Attempted to sign out without a session',
       meta: {
@@ -237,7 +237,7 @@ export const handleSignout: RequestHandler = async (req, res) => {
     return res.sendStatus(StatusCodes.BAD_REQUEST)
   }
 
-  req.session?.destroy((error) => {
+  req.session.destroy((error) => {
     if (error) {
       logger.error({
         message: 'Failed to destroy session',
diff --git a/tests/unit/backend/helpers/jest-express.ts b/tests/unit/backend/helpers/jest-express.ts
index ed91e6fed2..051dc86f88 100644
--- a/tests/unit/backend/helpers/jest-express.ts
+++ b/tests/unit/backend/helpers/jest-express.ts
@@ -10,7 +10,7 @@ const mockRequest = <P extends Record<string, string>, B>({
   body?: B
   session?: any
   secure?: boolean
-} = {}) => {
+} = {}): Request<P, any, B> => {
   return {
     body: body ?? {},
     params: params ?? {},

From 617171781ef7dd2030a287855b3a13bb91cf7d6f Mon Sep 17 00:00:00 2001
From: Kar Rui Lau <karrui.lau@gmail.com>
Date: Mon, 21 Sep 2020 13:51:46 +0800
Subject: [PATCH 22/25] refactor(UserService): update upsertUser function to
 ResultAsync

---
 src/app/models/user.server.model.ts           |  5 +-
 src/app/modules/user/user.service.ts          | 47 ++++++----
 src/types/user.ts                             |  9 +-
 .../backend/modules/user/user.service.spec.ts | 86 ++++++++++++++++---
 4 files changed, 120 insertions(+), 27 deletions(-)

diff --git a/src/app/models/user.server.model.ts b/src/app/models/user.server.model.ts
index 32aaad4d37..7b98277af4 100644
--- a/src/app/models/user.server.model.ts
+++ b/src/app/models/user.server.model.ts
@@ -98,7 +98,10 @@ const compileUserModel = (db: Mongoose) => {
         runValidators: true,
         setDefaultsOnInsert: true,
       },
-    )
+    ).populate({
+      path: 'agency',
+      model: AGENCY_SCHEMA_ID,
+    })
   }
 
   return db.model<IUserSchema, IUserModel>(USER_SCHEMA_ID, UserSchema)
diff --git a/src/app/modules/user/user.service.ts b/src/app/modules/user/user.service.ts
index 358953227a..799e24a902 100644
--- a/src/app/modules/user/user.service.ts
+++ b/src/app/modules/user/user.service.ts
@@ -1,6 +1,7 @@
 import to from 'await-to-js'
 import bcrypt from 'bcrypt'
 import mongoose from 'mongoose'
+import { errAsync, ResultAsync } from 'neverthrow'
 import validator from 'validator'
 
 import getAdminVerificationModel from '../../../app/models/admin_verification.server.model'
@@ -8,11 +9,15 @@ import { AGENCY_SCHEMA_ID } from '../../../app/models/agency.server.model'
 import getUserModel from '../../../app/models/user.server.model'
 import { generateOtp } from '../../../app/utils/otp'
 import config from '../../../config/config'
+import { createLoggerWithLabel } from '../../../config/logger'
 import { IAgency, IPopulatedUser, IUserSchema } from '../../../types'
 import { InvalidDomainError } from '../auth/auth.errors'
+import { DatabaseError } from '../core/core.errors'
 
 import { InvalidOtpError, MalformedOtpError } from './user.errors'
 
+const logger = createLoggerWithLabel(module)
+
 const AdminVerificationModel = getAdminVerificationModel(mongoose)
 const UserModel = getUserModel(mongoose)
 
@@ -161,23 +166,35 @@ export const getPopulatedUserById = async (
  * new user document is created and returned.
  * @param email the email of the user to retrieve
  * @param agency the agency document to associate with the user
- * @returns the upserted user document
- * @throws {InvalidDomainError} on invalid email
- * @throws {Error} on upsert failure
+ * @returns ok(upserted user document) populated with agency details
+ * @returns err(InvalidDomainError) when given email is invalid
+ * @returns err(DatabaseError) on upsert failure
  */
-export const retrieveUser = async (email: string, agency: IAgency) => {
+export const retrieveUser = (
+  email: string,
+  agencyId: IAgency['_id'],
+): ResultAsync<IPopulatedUser, InvalidDomainError | DatabaseError> => {
   if (!validator.isEmail(email)) {
-    throw new InvalidDomainError()
+    return errAsync(new InvalidDomainError())
   }
 
-  const admin = await UserModel.upsertUser({
-    email,
-    agency: agency._id,
-  })
-
-  if (!admin) {
-    throw new Error('Failed to upsert user')
-  }
-
-  return admin
+  return ResultAsync.fromPromise(
+    UserModel.upsertUser({
+      email,
+      agency: agencyId,
+    }),
+    (error) => {
+      logger.error({
+        message: 'Database error when upserting user',
+        meta: {
+          action: 'retrieveUser',
+          email,
+          agencyId,
+        },
+        error,
+      })
+
+      return new DatabaseError()
+    },
+  )
 }
diff --git a/src/types/user.ts b/src/types/user.ts
index 01ab753ab1..ed8a7f8762 100644
--- a/src/types/user.ts
+++ b/src/types/user.ts
@@ -20,9 +20,16 @@ export interface IUserSchema extends IUser, Document {
 }
 
 export interface IUserModel extends Model<IUserSchema> {
+  /**
+   * Upsert into User collection with given email and agencyId.
+   * If user with given email does not exist, a new document will be created.
+   * If user with given email already exists, the user's agency will be updated
+   * and populated before being returned.
+   * @returns the user document after upsert with populated agency details
+   */
   upsertUser: (
     upsertParams: Pick<IUser, 'email' | 'agency'>,
-  ) => Promise<IUserSchema>
+  ) => Promise<IPopulatedUser>
 }
 
 export interface IPopulatedUser extends IUserSchema {
diff --git a/tests/unit/backend/modules/user/user.service.spec.ts b/tests/unit/backend/modules/user/user.service.spec.ts
index d14e0a8447..3f3ac51c01 100644
--- a/tests/unit/backend/modules/user/user.service.spec.ts
+++ b/tests/unit/backend/modules/user/user.service.spec.ts
@@ -3,13 +3,16 @@ import mongoose from 'mongoose'
 import { ImportMock } from 'ts-mock-imports'
 
 import getAdminVerificationModel from 'src/app/models/admin_verification.server.model'
+import getUserModel from 'src/app/models/user.server.model'
+import { InvalidDomainError } from 'src/app/modules/auth/auth.errors'
 import * as UserService from 'src/app/modules/user/user.service'
 import * as OtpUtils from 'src/app/utils/otp'
-import { IAgencySchema, IUserSchema } from 'src/types'
+import { IAgencySchema, IPopulatedUser, IUserSchema } from 'src/types'
 
 import dbHandler from '../../helpers/jest-db'
 
 const AdminVerification = getAdminVerificationModel(mongoose)
+const UserModel = getUserModel(mongoose)
 
 describe('user.service', () => {
   // Obtained from Twilio's
@@ -17,27 +20,23 @@ describe('user.service', () => {
   const MOCK_CONTACT = '+15005550006'
   const MOCK_OTP = '123456'
   const USER_ID = new ObjectID()
+  const ALLOWED_DOMAIN = 'test.gov.sg'
 
   let defaultAgency: IAgencySchema
   let defaultUser: IUserSchema
 
-  beforeAll(async () => {
-    await dbHandler.connect()
-
+  beforeAll(async () => await dbHandler.connect())
+  beforeEach(async () => {
     // Insert user into collections.
     const { agency, user } = await dbHandler.insertFormCollectionReqs({
       userId: USER_ID,
+      mailDomain: ALLOWED_DOMAIN,
     })
 
     defaultAgency = agency.toObject()
     defaultUser = user.toObject()
   })
-  beforeEach(
-    async () =>
-      await dbHandler.clearCollection(
-        AdminVerification.collection.collectionName,
-      ),
-  )
+  afterEach(async () => await dbHandler.clearDatabase())
   afterAll(async () => await dbHandler.closeDatabase())
 
   describe('createContactOtp', () => {
@@ -232,4 +231,71 @@ describe('user.service', () => {
       await expect(userPromise).resolves.toBeNull()
     })
   })
+
+  describe('retrieveUser', () => {
+    it('should return InvalidDomainError on invalid email', async () => {
+      // Arrange
+      const notAnEmail = 'not an email'
+
+      // Act
+      const actualResult = await UserService.retrieveUser(
+        notAnEmail,
+        defaultAgency._id,
+      )
+
+      // Assert
+      expect(actualResult.isErr()).toBe(true)
+      expect(actualResult._unsafeUnwrapErr()).toBeInstanceOf(InvalidDomainError)
+    })
+
+    it('should return new User document when user does not yet exist in the collection', async () => {
+      // Arrange
+      const newUserEmail = `newUser@${ALLOWED_DOMAIN}`
+      // Should have the default user document in collection.
+      await expect(UserModel.countDocuments()).resolves.toEqual(1)
+
+      // Act
+      const actualResult = await UserService.retrieveUser(
+        newUserEmail,
+        defaultAgency._id,
+      )
+
+      // Assert
+      const expectedUser: Partial<IPopulatedUser> = {
+        agency: defaultAgency,
+        email: newUserEmail,
+      }
+      expect(actualResult.isOk()).toBe(true)
+      // Should now have 2 user documents
+      await expect(UserModel.countDocuments()).resolves.toEqual(2)
+      expect(actualResult._unsafeUnwrap().toObject()).toEqual(
+        expect.objectContaining(expectedUser),
+      )
+    })
+
+    it('should return existing User document when user already exists', async () => {
+      // Arrange
+      // Should have the default user document in collection.
+      await expect(UserModel.countDocuments()).resolves.toEqual(1)
+      const userEmail = defaultUser.email
+
+      // Act
+      const actualResult = await UserService.retrieveUser(
+        userEmail,
+        defaultAgency._id,
+      )
+
+      // Assert
+      const expectedUser: Partial<IPopulatedUser> = {
+        ...defaultUser,
+        agency: defaultAgency,
+      }
+      expect(actualResult.isOk()).toBe(true)
+      // Should still only have 1 user document.
+      await expect(UserModel.countDocuments()).resolves.toEqual(1)
+      expect(actualResult._unsafeUnwrap().toObject()).toEqual(
+        expect.objectContaining(expectedUser),
+      )
+    })
+  })
 })

From 3c8da7d89614317ca9835e16547b8188b0c7b55c Mon Sep 17 00:00:00 2001
From: Kar Rui Lau <karrui.lau@gmail.com>
Date: Mon, 21 Sep 2020 14:07:44 +0800
Subject: [PATCH 23/25] refactor(AuthController): use updated
 UserService#retrieveUser fn

---
 .../auth/__tests__/auth.controller.spec.ts    | 17 ++---
 .../auth/__tests__/auth.routes.spec.ts        |  4 +-
 src/app/modules/auth/auth.controller.ts       | 64 +++++++++----------
 3 files changed, 38 insertions(+), 47 deletions(-)

diff --git a/src/app/modules/auth/__tests__/auth.controller.spec.ts b/src/app/modules/auth/__tests__/auth.controller.spec.ts
index 95661ed6a7..cde65ca3fc 100644
--- a/src/app/modules/auth/__tests__/auth.controller.spec.ts
+++ b/src/app/modules/auth/__tests__/auth.controller.spec.ts
@@ -176,17 +176,14 @@ describe('auth.controller', () => {
         okAsync(MOCK_AGENCY),
       )
       MockAuthService.verifyLoginOtp.mockReturnValueOnce(okAsync(true))
-      MockUserService.retrieveUser.mockResolvedValueOnce(mockUser)
+      MockUserService.retrieveUser.mockReturnValueOnce(okAsync(mockUser))
 
       // Act
       await AuthController.handleLoginVerifyOtp(MOCK_REQ, mockRes, jest.fn())
 
       // Assert
       expect(mockRes.status).toBeCalledWith(200)
-      expect(mockRes.send).toBeCalledWith({
-        ...mockUser.toObject(),
-        agency: MOCK_AGENCY,
-      })
+      expect(mockRes.send).toBeCalledWith(mockUser.toObject())
     })
 
     it('should return with ApplicationError status and message when retrieving agency returns an ApplicationError', async () => {
@@ -245,7 +242,7 @@ describe('auth.controller', () => {
       // Assert
       expect(mockRes.status).toBeCalledWith(500)
       expect(mockRes.send).toBeCalledWith(
-        'Failed to validate OTP. Please try again later and if the problem persists, contact us.',
+        expect.stringContaining('Failed to process OTP.'),
       )
       // Check that the correct services have been called or not called.
       expect(MockAuthService.verifyLoginOtp).toHaveBeenCalledTimes(1)
@@ -259,8 +256,8 @@ describe('auth.controller', () => {
         okAsync(MOCK_AGENCY),
       )
       MockAuthService.verifyLoginOtp.mockReturnValueOnce(okAsync(true))
-      MockUserService.retrieveUser.mockRejectedValueOnce(
-        new Error('upsert error'),
+      MockUserService.retrieveUser.mockReturnValueOnce(
+        errAsync(new DatabaseError()),
       )
 
       // Act
@@ -270,9 +267,7 @@ describe('auth.controller', () => {
       expect(mockRes.status).toBeCalledWith(500)
       expect(mockRes.send).toBeCalledWith(
         // Use stringContaining here due to dynamic text and out of test scope.
-        expect.stringContaining(
-          'User signin failed. Please try again later and if the problem persists',
-        ),
+        expect.stringContaining('Failed to process OTP.'),
       )
       // Check that the correct services have been called or not called.
       expect(MockAuthService.verifyLoginOtp).toHaveBeenCalledTimes(1)
diff --git a/src/app/modules/auth/__tests__/auth.routes.spec.ts b/src/app/modules/auth/__tests__/auth.routes.spec.ts
index 06c8879602..5c01e21040 100644
--- a/src/app/modules/auth/__tests__/auth.routes.spec.ts
+++ b/src/app/modules/auth/__tests__/auth.routes.spec.ts
@@ -446,7 +446,7 @@ describe('auth.routes', () => {
       // Mock error returned when creating user
       const upsertSpy = jest
         .spyOn(UserService, 'retrieveUser')
-        .mockRejectedValueOnce(new Error('some error'))
+        .mockReturnValueOnce(errAsync(new DatabaseError('some error')))
 
       // Act
       const response = await request
@@ -458,7 +458,7 @@ describe('auth.routes', () => {
       expect(upsertSpy).toBeCalled()
       expect(response.status).toEqual(500)
       expect(response.text).toEqual(
-        expect.stringContaining('User signin failed. Please try again later'),
+        expect.stringContaining('Failed to process OTP.'),
       )
     })
   })
diff --git a/src/app/modules/auth/auth.controller.ts b/src/app/modules/auth/auth.controller.ts
index f617589c91..98d9cee42e 100644
--- a/src/app/modules/auth/auth.controller.ts
+++ b/src/app/modules/auth/auth.controller.ts
@@ -159,6 +159,7 @@ export const handleLoginVerifyOtp: RequestHandler<
     email,
     ip: getRequestIp(req),
   }
+  const coreErrorMessage = `Failed to process OTP. Please try again later and if the problem persists, submit our Support Form (${LINKS.supportFormLink}).`
 
   const validateResult = await AuthService.validateEmailDomain(email)
   if (validateResult.isErr()) {
@@ -175,55 +176,50 @@ export const handleLoginVerifyOtp: RequestHandler<
   // Since there is no error, agency is retrieved from validation.
   const agency = validateResult.value
 
-  return AuthService.verifyLoginOtp(otp, email)
-    .map(async () => {
-      // OTP is valid, proceed to login user.
-      try {
-        // TODO (#317): remove usage of non-null assertion
-        const user = await UserService.retrieveUser(email, agency)
-        // Create user object to return to frontend.
-        const userObj = { ...user.toObject(), agency }
-
+  // Step 1: Verify login OTP.
+  return (
+    AuthService.verifyLoginOtp(otp, email)
+      // Step 2: OTP is valid, retrieve associated user.
+      .andThen(() => UserService.retrieveUser(email, agency._id))
+      // Step 3a: Set session and return user in response.
+      .map((user) => {
         if (!req.session) {
-          throw new Error('req.session not found')
+          logger.error({
+            message: 'Error logging in user; req.session is undefined',
+            meta: logMeta,
+          })
+
+          return res
+            .status(StatusCodes.INTERNAL_SERVER_ERROR)
+            .send(coreErrorMessage)
         }
 
         // TODO(#212): Should store only userId in session.
         // Add user info to session.
-        req.session.user = userObj as SessionUser
+        const userObj = user.toObject() as SessionUser
+        req.session.user = userObj
         logger.info({
           message: `Successfully logged in user ${user.email}`,
           meta: logMeta,
         })
 
         return res.status(StatusCodes.OK).send(userObj)
-      } catch (err) {
-        logger.error({
-          message: 'Error logging in user',
+      })
+      // Step 3b: Error occured in one of the steps.
+      .mapErr((error) => {
+        logger.warn({
+          message: 'Error occurred when trying to validate login OTP',
           meta: logMeta,
-          error: err,
+          error,
         })
 
-        return res
-          .status(StatusCodes.INTERNAL_SERVER_ERROR)
-          .send(
-            `User signin failed. Please try again later and if the problem persists, submit our Support Form (${LINKS.supportFormLink}).`,
-          )
-      }
-    })
-    .mapErr((error) => {
-      logger.warn({
-        message: 'Error occurred when trying to validate login OTP',
-        meta: logMeta,
-        error,
+        const { errorMessage, statusCode } = mapRouteError(
+          error,
+          coreErrorMessage,
+        )
+        return res.status(statusCode).send(errorMessage)
       })
-
-      const { errorMessage, statusCode } = mapRouteError(
-        error,
-        /* coreErrorMessage= */ 'Failed to validate OTP. Please try again later and if the problem persists, contact us.',
-      )
-      return res.status(statusCode).send(errorMessage)
-    })
+  )
 }
 
 export const handleSignout: RequestHandler = async (req, res) => {

From f0e0f1b540f8b88947887644f5baa85fd8cdd72a Mon Sep 17 00:00:00 2001
From: Kar Rui Lau <karrui.lau@gmail.com>
Date: Mon, 21 Sep 2020 14:10:21 +0800
Subject: [PATCH 24/25] feat(AuthController): add logging for unknown errors
 observed

---
 src/app/modules/auth/auth.controller.ts | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/app/modules/auth/auth.controller.ts b/src/app/modules/auth/auth.controller.ts
index 98d9cee42e..729d4ebf0c 100644
--- a/src/app/modules/auth/auth.controller.ts
+++ b/src/app/modules/auth/auth.controller.ts
@@ -43,6 +43,14 @@ const mapRouteError = (error: ApplicationError, coreErrorMessage?: string) => {
         errorMessage: coreErrorMessage ?? error.message,
       }
     default:
+      logger.error({
+        message: 'Unknown route error observed',
+        meta: {
+          action: 'mapRouteError',
+        },
+        error,
+      })
+
       return {
         statusCode: StatusCodes.INTERNAL_SERVER_ERROR,
         errorMessage: 'Something went wrong. Please try again.',

From 05d3f58fa77cadfd92659bedfe093770cc7b82bc Mon Sep 17 00:00:00 2001
From: Kar Rui Lau <karrui.lau@gmail.com>
Date: Mon, 21 Sep 2020 14:17:27 +0800
Subject: [PATCH 25/25] refactor(Auth): extract mapRouteError into auth.utils
 file

---
 src/app/modules/auth/auth.controller.ts | 45 +------------------
 src/app/modules/auth/auth.utils.ts      | 58 +++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 44 deletions(-)
 create mode 100644 src/app/modules/auth/auth.utils.ts

diff --git a/src/app/modules/auth/auth.controller.ts b/src/app/modules/auth/auth.controller.ts
index 729d4ebf0c..c5907ff358 100644
--- a/src/app/modules/auth/auth.controller.ts
+++ b/src/app/modules/auth/auth.controller.ts
@@ -7,57 +7,14 @@ import { createLoggerWithLabel } from '../../../config/logger'
 import { LINKS } from '../../../shared/constants'
 import MailService from '../../services/mail.service'
 import { getRequestIp } from '../../utils/request'
-import { ApplicationError, DatabaseError } from '../core/core.errors'
-import { MailSendError } from '../mail/mail.errors'
 import * as UserService from '../user/user.service'
 
-import { InvalidDomainError, InvalidOtpError } from './auth.errors'
 import * as AuthService from './auth.service'
 import { SessionUser } from './auth.types'
+import { mapRouteError } from './auth.utils'
 
 const logger = createLoggerWithLabel(module)
 
-/**
- * Handler to map ApplicationErrors to their correct status code and error
- * messages.
- * @param error The error to retrieve the status codes and error messages
- * @param coreErrorMessage Any error message to return instead of the default core error message, if any
- */
-const mapRouteError = (error: ApplicationError, coreErrorMessage?: string) => {
-  switch (error.constructor) {
-    case InvalidDomainError:
-      return {
-        statusCode: StatusCodes.UNAUTHORIZED,
-        errorMessage: error.message,
-      }
-    case InvalidOtpError:
-      return {
-        statusCode: StatusCodes.UNPROCESSABLE_ENTITY,
-        errorMessage: error.message,
-      }
-    case MailSendError:
-    case ApplicationError:
-    case DatabaseError:
-      return {
-        statusCode: StatusCodes.INTERNAL_SERVER_ERROR,
-        errorMessage: coreErrorMessage ?? error.message,
-      }
-    default:
-      logger.error({
-        message: 'Unknown route error observed',
-        meta: {
-          action: 'mapRouteError',
-        },
-        error,
-      })
-
-      return {
-        statusCode: StatusCodes.INTERNAL_SERVER_ERROR,
-        errorMessage: 'Something went wrong. Please try again.',
-      }
-  }
-}
-
 /**
  * Handler for GET /auth/checkuser endpoint.
  * @returns 500 when there was an error validating body.email
diff --git a/src/app/modules/auth/auth.utils.ts b/src/app/modules/auth/auth.utils.ts
new file mode 100644
index 0000000000..366717de21
--- /dev/null
+++ b/src/app/modules/auth/auth.utils.ts
@@ -0,0 +1,58 @@
+import { StatusCodes } from 'http-status-codes'
+
+import { createLoggerWithLabel } from '../../../config/logger'
+import { ApplicationError, DatabaseError } from '../core/core.errors'
+import { MailSendError } from '../mail/mail.errors'
+
+import { InvalidDomainError, InvalidOtpError } from './auth.errors'
+
+const logger = createLoggerWithLabel(module)
+
+type ErrorResponseData = {
+  statusCode: StatusCodes
+  errorMessage: string
+}
+
+/**
+ * Handler to map ApplicationErrors to their correct status code and error
+ * messages.
+ * @param error The error to retrieve the status codes and error messages
+ * @param coreErrorMessage Any error message to return instead of the default core error message, if any
+ */
+export const mapRouteError = (
+  error: ApplicationError,
+  coreErrorMessage?: string,
+): ErrorResponseData => {
+  switch (error.constructor) {
+    case InvalidDomainError:
+      return {
+        statusCode: StatusCodes.UNAUTHORIZED,
+        errorMessage: error.message,
+      }
+    case InvalidOtpError:
+      return {
+        statusCode: StatusCodes.UNPROCESSABLE_ENTITY,
+        errorMessage: error.message,
+      }
+    case MailSendError:
+    case ApplicationError:
+    case DatabaseError:
+      return {
+        statusCode: StatusCodes.INTERNAL_SERVER_ERROR,
+        errorMessage: coreErrorMessage ?? error.message,
+      }
+    default:
+      logger.error({
+        message: 'Unknown route error observed',
+        meta: {
+          action: 'mapRouteError',
+        },
+        error,
+      })
+
+      return {
+        statusCode: StatusCodes.INTERNAL_SERVER_ERROR,
+        errorMessage: 'Something went wrong. Please try again.',
+      }
+  }
+}