Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

feat: Using neverthrow to explicitly handle errors in AuthController #332

Merged
merged 26 commits into from
Sep 22, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
88535df
chore: add neverthrow package for explicit error handling
karrui Sep 16, 2020
7795ca1
feat: add core DatabaseError
karrui Sep 16, 2020
c2eee88
refactor(AuthService): rename and convert getAgencyWithEmail to Result
karrui Sep 16, 2020
2f5cc3c
test(Auth): update tests to use new validateEmailDomain function
karrui Sep 16, 2020
104a08b
refactor(Auth): inline validate middleware into /checkuser endpoint
karrui Sep 16, 2020
6b2d3a2
refactor(Auth): inline validate middleware into /sendotp endpoint
karrui Sep 16, 2020
cc567ff
refactor(Auth): inline validate middleware into /verifyotp endpoint
karrui Sep 16, 2020
2e270f1
refactor(Auth): remove unused auth.middlewares
karrui Sep 16, 2020
d64a605
refactor(Auth): remove unused imports/types after middleware removal
karrui Sep 16, 2020
7104dae
refactor(AuthService): use validateEmailDomain with ResultAsync return
karrui Sep 17, 2020
df6b95e
refactor(AuthService): update createLoginOtp with ResultAsync return
karrui Sep 17, 2020
7a87768
refactor(AuthService): update verifyLoginOtp with ResultAsync return
karrui Sep 17, 2020
7339dbb
fix: fix typo in logger action and remove unused imports
karrui Sep 17, 2020
e7994bd
refactor(MailUtils): update generateLoginOtpHtml to return ResultAsync
karrui Sep 17, 2020
7269bf3
refactor: update sendLoginOtp to return ResultAsync
karrui Sep 17, 2020
a52f370
refactor: update handleLoginSendOtp to chain ResultAsync calls
karrui Sep 17, 2020
4590ec2
refactor(AuthController): update handleLoginVerifyOtp to chain
karrui Sep 17, 2020
e92a63f
fix: remove unused imports and remove null assertion
karrui Sep 17, 2020
a0f5ad2
feat: update comments and tests names to return error instead of throw
karrui Sep 17, 2020
8046f04
chore: update travis.yml
karrui Sep 17, 2020
90fdc5d
feat(AuthController): add explicit undefined check for req.session
karrui Sep 21, 2020
08cd3dc
Merge branch 'develop' into feat/neverthrow
karrui Sep 21, 2020
6171717
refactor(UserService): update upsertUser function to ResultAsync
karrui Sep 21, 2020
3c8da7d
refactor(AuthController): use updated UserService#retrieveUser fn
karrui Sep 21, 2020
f0e0f1b
feat(AuthController): add logging for unknown errors observed
karrui Sep 21, 2020
05d3f58
refactor(Auth): extract mapRouteError into auth.utils file
karrui Sep 21, 2020
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
5 changes: 4 additions & 1 deletion src/app/models/user.server.model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
162 changes: 110 additions & 52 deletions src/app/modules/auth/__tests__/auth.controller.spec.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
import { errAsync, okAsync } from 'neverthrow'
import expressHandler from 'tests/unit/backend/helpers/jest-express'
import { mocked } from 'ts-jest/utils'

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 { InvalidOtpError } from '../auth.errors'
import { InvalidDomainError, InvalidOtpError } from '../auth.errors'
import * as AuthService from '../auth.service'

const VALID_EMAIL = '[email protected]'
Expand All @@ -25,20 +28,39 @@ describe('auth.controller', () => {
})

describe('handleCheckUser', () => {
it('should return 200', async () => {
const MOCK_REQ = expressHandler.mockRequest({
body: { email: '[email protected]' },
})

it('should return 200 when domain is valid', async () => {
// Arrange
const mockRes = expressHandler.mockResponse()
MockAuthService.validateEmailDomain.mockReturnValueOnce(
okAsync(<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.mockReturnValueOnce(
errAsync(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', () => {
Expand All @@ -51,8 +73,11 @@ describe('auth.controller', () => {
// Arrange
const mockRes = expressHandler.mockResponse()
// Mock AuthService and MailService to return without errors
MockAuthService.createLoginOtp.mockResolvedValueOnce(MOCK_OTP)
MockMailService.sendLoginOtp.mockResolvedValueOnce(true)
MockAuthService.validateEmailDomain.mockReturnValueOnce(
okAsync(<IAgencySchema>{}),
)
MockAuthService.createLoginOtp.mockReturnValueOnce(okAsync(MOCK_OTP))
MockMailService.sendLoginOtp.mockReturnValueOnce(okAsync(true))

// Act
await AuthController.handleLoginSendOtp(MOCK_REQ, mockRes, jest.fn())
Expand All @@ -65,12 +90,31 @@ 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.mockReturnValueOnce(
errAsync(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.mockReturnValueOnce(
okAsync(<IAgencySchema>{}),
)
// Mock createLoginOtp failure
MockAuthService.createLoginOtp.mockRejectedValueOnce(
new Error('otp creation error'),
MockAuthService.createLoginOtp.mockReturnValueOnce(
errAsync(new DatabaseError('otp creation error')),
)

// Act
Expand All @@ -89,10 +133,13 @@ describe('auth.controller', () => {
it('should return 500 when there is an error sending login OTP', async () => {
// Arrange
const mockRes = expressHandler.mockResponse()
MockAuthService.validateEmailDomain.mockReturnValueOnce(
okAsync(<IAgencySchema>{}),
)
// Mock createLoginOtp success but sendLoginOtp failure.
MockAuthService.createLoginOtp.mockResolvedValueOnce(MOCK_OTP)
MockMailService.sendLoginOtp.mockRejectedValueOnce(
new Error('send error'),
MockAuthService.createLoginOtp.mockReturnValueOnce(okAsync(MOCK_OTP))
MockMailService.sendLoginOtp.mockReturnValueOnce(
errAsync(new MailSendError('send error')),
)

// Act
Expand All @@ -101,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)
Expand All @@ -122,36 +169,49 @@ 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.verifyLoginOtp.mockResolvedValueOnce(true)
MockUserService.retrieveUser.mockResolvedValueOnce(mockUser)
MockAuthService.validateEmailDomain.mockReturnValueOnce(
okAsync(MOCK_AGENCY),
)
MockAuthService.verifyLoginOtp.mockReturnValueOnce(okAsync(true))
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 422 when verifying login OTP throws an InvalidOtpError', async () => {
it('should return with ApplicationError status and message when retrieving agency returns an ApplicationError', async () => {
// Arrange
// Add agency into locals due to precondition.
const mockRes = expressHandler.mockResponse({
locals: { agency: MOCK_AGENCY },
})
const expectedError = new InvalidDomainError()
const mockRes = expressHandler.mockResponse()
MockAuthService.validateEmailDomain.mockReturnValueOnce(
errAsync(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 returns an InvalidOtpError', async () => {
// Arrange
const mockRes = expressHandler.mockResponse()
const expectedInvalidOtpError = new InvalidOtpError()
MockAuthService.validateEmailDomain.mockReturnValueOnce(
okAsync(MOCK_AGENCY),
)
// Mock error from verifyLoginOtp.
MockAuthService.verifyLoginOtp.mockRejectedValueOnce(
expectedInvalidOtpError,
MockAuthService.verifyLoginOtp.mockReturnValueOnce(
errAsync(expectedInvalidOtpError),
)

// Act
Expand All @@ -165,15 +225,15 @@ 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
// Add agency into locals due to precondition.
const mockRes = expressHandler.mockResponse({
locals: { agency: MOCK_AGENCY },
})
const mockRes = expressHandler.mockResponse()
MockAuthService.validateEmailDomain.mockReturnValueOnce(
okAsync(MOCK_AGENCY),
)
// Mock generic error from verifyLoginOtp.
MockAuthService.verifyLoginOtp.mockRejectedValueOnce(
new Error('generic error'),
MockAuthService.verifyLoginOtp.mockReturnValueOnce(
errAsync(new ApplicationError('generic error')),
)

// Act
Expand All @@ -182,22 +242,22 @@ 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)
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
// Add agency into locals due to precondition.
const mockRes = expressHandler.mockResponse({
locals: { agency: MOCK_AGENCY },
})
MockAuthService.verifyLoginOtp.mockResolvedValueOnce(true)
MockUserService.retrieveUser.mockRejectedValueOnce(
new Error('upsert error'),
const mockRes = expressHandler.mockResponse()
MockAuthService.validateEmailDomain.mockReturnValueOnce(
okAsync(MOCK_AGENCY),
)
MockAuthService.verifyLoginOtp.mockReturnValueOnce(okAsync(true))
MockUserService.retrieveUser.mockReturnValueOnce(
errAsync(new DatabaseError()),
)

// Act
Expand All @@ -207,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)
Expand Down Expand Up @@ -257,7 +315,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()
Expand Down
71 changes: 0 additions & 71 deletions src/app/modules/auth/__tests__/auth.middlewares.spec.ts

This file was deleted.

Loading