Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

refactor(verification): migrate verified field #1866

Merged
merged 11 commits into from
May 14, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -167,64 +167,82 @@ describe('Verification controller', () => {
})
})

describe('handleGetTransactionMetadata', () => {
describe('handleCreateVerificationTransaction', () => {
const MOCK_REQ = expressHandler.mockRequest({
params: { transactionId: MOCK_TRANSACTION_ID },
params: { formId: MOCK_FORM_ID },
})

it('should return metadata when parameters are valid', async () => {
const transactionPublicView = mockTransaction.getPublicView()
MockVerificationFactory.getTransactionMetadata.mockReturnValueOnce(
okAsync(transactionPublicView),
it('should return transaction when parameters are valid', async () => {
MockVerificationFactory.createTransaction.mockReturnValueOnce(
okAsync(mockTransaction),
)

await VerificationController.handleGetTransactionMetadata(
await VerificationController.handleCreateVerificationTransaction(
MOCK_REQ,
mockRes,
jest.fn(),
)

expect(
MockVerificationFactory.getTransactionMetadata,
).toHaveBeenCalledWith(MOCK_TRANSACTION_ID)
expect(MockVerificationFactory.createTransaction).toHaveBeenCalledWith(
MOCK_FORM_ID,
)
expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.CREATED)
expect(mockRes.json).toHaveBeenCalledWith({
transactionId: mockTransaction._id,
expireAt: mockTransaction.expireAt,
})
})

it('should return 200 with empty object when transaction is not created', async () => {
MockVerificationFactory.createTransaction.mockReturnValueOnce(
okAsync(null),
)
await VerificationController.handleCreateVerificationTransaction(
MOCK_REQ,
mockRes,
jest.fn(),
)
expect(MockVerificationFactory.createTransaction).toHaveBeenCalledWith(
MOCK_FORM_ID,
)
expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.OK)
expect(mockRes.json).toHaveBeenCalledWith(transactionPublicView)
expect(mockRes.json).toHaveBeenCalledWith({})
})

it('should return 404 when transaction is not found', async () => {
MockVerificationFactory.getTransactionMetadata.mockReturnValueOnce(
errAsync(new TransactionNotFoundError()),
it('should return 404 when form is not found', async () => {
MockVerificationFactory.createTransaction.mockReturnValueOnce(
errAsync(new FormNotFoundError()),
)

await VerificationController.handleGetTransactionMetadata(
await VerificationController.handleCreateVerificationTransaction(
MOCK_REQ,
mockRes,
jest.fn(),
)

expect(
MockVerificationFactory.getTransactionMetadata,
).toHaveBeenCalledWith(MOCK_TRANSACTION_ID)
expect(MockVerificationFactory.createTransaction).toHaveBeenCalledWith(
MOCK_FORM_ID,
)
expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.NOT_FOUND)
expect(mockRes.json).toHaveBeenCalledWith({
message: expect.any(String),
})
})

it('should return 500 when database error occurs', async () => {
MockVerificationFactory.getTransactionMetadata.mockReturnValueOnce(
MockVerificationFactory.createTransaction.mockReturnValueOnce(
errAsync(new DatabaseError()),
)

await VerificationController.handleGetTransactionMetadata(
await VerificationController.handleCreateVerificationTransaction(
MOCK_REQ,
mockRes,
jest.fn(),
)

expect(
MockVerificationFactory.getTransactionMetadata,
).toHaveBeenCalledWith(MOCK_TRANSACTION_ID)
expect(MockVerificationFactory.createTransaction).toHaveBeenCalledWith(
MOCK_FORM_ID,
)
expect(mockRes.status).toHaveBeenCalledWith(
StatusCodes.INTERNAL_SERVER_ERROR,
)
Expand Down
13 changes: 0 additions & 13 deletions src/app/modules/verification/__tests__/verification.routes.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -158,19 +158,6 @@ describe('verification.routes', () => {
})
})

describe('GET /:transactionId', () => {
it('should return 200 when transactionId is valid', async () => {
const response = await request.get(`/transaction/${mockTransactionId}`)

expect(response.status).toBe(StatusCodes.OK)
expect(response.body).toEqual({
formId: expect.any(String),
expireAt: expect.any(String),
_id: expect.any(String),
})
})
})

describe('POST /:transactionId/reset', () => {
it('should return 400 when fieldId is not provided in body', async () => {
const response = await request.post(
Expand Down
38 changes: 23 additions & 15 deletions src/app/modules/verification/verification.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import { RequestHandler } from 'express'
import { StatusCodes } from 'http-status-codes'

import { SALT_ROUNDS } from '../../../shared/util/verification'
import { PublicTransaction } from '../../../types'
import { ErrorDto } from '../../../types/api'
import { createLoggerWithLabel } from '../../config/logger'
import { generateOtpWithHash } from '../../utils/otp'
Expand All @@ -15,8 +14,10 @@ import { mapRouteError } from './verification.util'
const logger = createLoggerWithLabel(module)

/**
* NOTE: Private handler for POST /transaction
* When a form is loaded publicly, a transaction is created, and populated with the field ids of fields that are verifiable.
* If no fields are verifiable, then it did not create a transaction and returns an empty object.
* @deprecated in favour of handleCreateVerificationTransaction
* @param req
* @param res
* @returns 201 - transaction is created
Expand Down Expand Up @@ -54,29 +55,36 @@ export const handleCreateTransaction: RequestHandler<
}

/**
* Returns a transaction's id and expiry time if it exists
* NOTE: Private handler for POST /forms/:formId/fieldverifications
* When a form is loaded publicly, a transaction is created, and populated with the field ids of fields that are verifiable.
* If no fields are verifiable, then it did not create a transaction and returns an empty object.
* @param req
* @param res
* @returns 201 - transaction is created
* @returns 200 - transaction was not created as no fields were verifiable for the form
*/
export const handleGetTransactionMetadata: RequestHandler<
{
transactionId: string
},
PublicTransaction | ErrorDto
export const handleCreateVerificationTransaction: RequestHandler<
{ formId: string },
Transaction | ErrorDto
> = async (req, res) => {
const { transactionId } = req.params
const { formId } = req.params
const logMeta = {
action: 'handleGetTransactionMetadata',
transactionId,
action: 'handleCreateVerificationTransaction',
formId,
...createReqMeta(req),
}
return VerificationFactory.getTransactionMetadata(transactionId)
.map((publicTransaction) =>
res.status(StatusCodes.OK).json(publicTransaction),
)
return VerificationFactory.createTransaction(formId)
.map((transaction) => {
return transaction
? res.status(StatusCodes.CREATED).json({
expireAt: transaction.expireAt,
transactionId: transaction._id,
})
: res.status(StatusCodes.OK).json({})
})
.mapErr((error) => {
logger.error({
message: 'Error retrieving transaction metadata',
message: 'Error creating transaction',
meta: logMeta,
error,
})
Expand Down
15 changes: 5 additions & 10 deletions src/app/modules/verification/verification.routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@ export const VfnRouter = Router()

const formatOfId = Joi.string().length(24).hex().required()

/**
* Route for POST /transaction
* @body {formId: string}: The form to create the transaction for
* @deprecated in favour of POST /forms/:formId/fieldverifications
*/
VfnRouter.post(
'/',
celebrate({
Expand All @@ -17,16 +22,6 @@ VfnRouter.post(
VerificationController.handleCreateTransaction,
)

VfnRouter.get(
'/:transactionId([a-fA-F0-9]{24})',
celebrate({
[Segments.PARAMS]: Joi.object({
transactionId: formatOfId,
}),
}),
VerificationController.handleGetTransactionMetadata,
)

VfnRouter.post(
'/:transactionId([a-fA-F0-9]{24})/reset',
celebrate({
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
import { StatusCodes } from 'http-status-codes'
import session, { Session } from 'supertest-session'

import { BasicField } from 'src/types'

import { setupApp } from 'tests/integration/helpers/express-setup'
import { generateDefaultField } from 'tests/unit/backend/helpers/generate-form-data'
import dbHandler from 'tests/unit/backend/helpers/jest-db'

import { PublicFormsVerificationRouter } from '../public-forms.verification.routes'

const verificationApp = setupApp('/forms', PublicFormsVerificationRouter)

jest.mock('nodemailer', () => ({
createTransport: jest.fn().mockReturnValue({
sendMail: jest.fn().mockResolvedValue(true),
}),
}))
// Default Twilio export is a function
jest.mock('twilio', () => () => ({
messages: {
create: jest.fn().mockResolvedValue({
sid: 'mockSid',
}),
},
}))

describe('public-forms.verification.routes', () => {
let mockEmptyFormId: string
let mockVerifiableFormId: string
let request: Session

beforeAll(async () => await dbHandler.connect())

beforeEach(async () => {
request = session(verificationApp)
jest.clearAllMocks()
await dbHandler.clearDatabase()

// Form without verifiable fields
const { form: emptyForm } = await dbHandler.insertEmailForm()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may want to inline this since this is only used in a single test. Same for the verifiable form

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i didn't do this cos it'll be reused for the related endpoints in the issues @__@ if it's an issue, i can change it!

mockEmptyFormId = String(emptyForm._id)

// Form with verifiable fields
const emailField = generateDefaultField(BasicField.Email, {
isVerifiable: true,
})
const mobileField = generateDefaultField(BasicField.Mobile, {
isVerifiable: true,
})
const { form: verifiableForm } = await dbHandler.insertEmailForm({
// Alternative mail domain so as not to clash with emptyForm
mailDomain: 'test2.gov.sg',
formOptions: {
form_fields: [emailField, mobileField],
},
})
mockVerifiableFormId = String(verifiableForm._id)
})

afterAll(async () => await dbHandler.closeDatabase())

describe('POST /forms/:formId/fieldverifications', () => {
it('should return 404 when formId is malformed', async () => {
// Act
const response = await request.post(`/forms/malformed/fieldverifications`)

// Assert
expect(response.status).toBe(StatusCodes.NOT_FOUND)
})

it('should return 200 when form has no verifiable fields', async () => {
// Act
const response = await request
// ID of form with no verifiable fields
.post(`/forms/${mockEmptyFormId}/fieldverifications`)

// Assert
expect(response.status).toBe(StatusCodes.OK)
seaerchin marked this conversation as resolved.
Show resolved Hide resolved
expect(response.body).toEqual({})
})

it('should return 201 when form has verifiable fields', async () => {
// Act
const response = await request
// ID of form with verifiable fields
.post(`/forms/${mockVerifiableFormId}/fieldverifications`)

// Assert
expect(response.status).toBe(StatusCodes.CREATED)
expect(response.body).toEqual({
transactionId: expect.any(String),
expireAt: expect.any(String),
})
})
})
})
2 changes: 2 additions & 0 deletions src/app/routes/api/v3/forms/public-forms.routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,12 @@ import { PublicFormsAuthRouter } from './public-forms.auth.routes'
import { PublicFormsFeedbackRouter } from './public-forms.feedback.routes'
import { PublicFormsFormRouter } from './public-forms.form.routes'
import { PublicFormsSubmissionsRouter } from './public-forms.submissions.routes'
import { PublicFormsVerificationRouter } from './public-forms.verification.routes'

export const PublicFormsRouter = Router()

PublicFormsRouter.use(PublicFormsSubmissionsRouter)
PublicFormsRouter.use(PublicFormsFeedbackRouter)
PublicFormsRouter.use(PublicFormsFormRouter)
PublicFormsRouter.use(PublicFormsAuthRouter)
PublicFormsRouter.use(PublicFormsVerificationRouter)
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import { Router } from 'express'

import * as VerificationController from '../../../../modules/verification/verification.controller'

export const PublicFormsVerificationRouter = Router()

PublicFormsVerificationRouter.route(
'/:formId([a-fA-F0-9]{24})/fieldverifications',
).post(VerificationController.handleCreateVerificationTransaction)
14 changes: 10 additions & 4 deletions src/public/services/FieldVerificationService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,15 @@ export type FetchNewTransactionResponse =

type VerifiedFieldSignature = Opaque<string, 'VerifiedFieldSignature'>

/** Exported for testing */
/** Exported for testing
* @deprecated
*/
export const TRANSACTION_ENDPOINT = '/transaction'
seaerchin marked this conversation as resolved.
Show resolved Hide resolved

/** Exported for testing */
export const FORM_API_PREFIX = '/api/v3/forms'
export const VERIFICATION_ENDPOINT = 'fieldverifications'
seaerchin marked this conversation as resolved.
Show resolved Hide resolved

/**
* Create a transaction for given form.
* @param formId The id of the form to create a transaction for
Expand All @@ -25,9 +31,9 @@ export const createTransactionForForm = async (
formId: string,
): Promise<FetchNewTransactionResponse> => {
return axios
.post<FetchNewTransactionResponse>(TRANSACTION_ENDPOINT, {
formId,
})
.post<FetchNewTransactionResponse>(
`${FORM_API_PREFIX}/${formId}/${VERIFICATION_ENDPOINT}`,
)
.then(({ data }) => data)
}

Expand Down
Loading