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: add backend validation for form feedback submission #3941

Merged
merged 23 commits into from
Jul 4, 2022
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
115604e
feat: add new feedback api route
hanstirtaputra Jun 1, 2022
86193e3
feat: link Angular FE to new feedback endpoint
hanstirtaputra Jun 2, 2022
9116f92
refactor: feedback controller
hanstirtaputra Jun 2, 2022
b2c0755
refactor: feedback controller log meta
hanstirtaputra Jun 2, 2022
23bcb2a
feat: add tests for services
hanstirtaputra Jun 2, 2022
36736ef
feat: add test for new feedback endpoint
hanstirtaputra Jun 2, 2022
5f0a5ea
fix: update failing FE tests
hanstirtaputra Jun 2, 2022
b3a8cf5
fix: update error status code duplicate feedback
hanstirtaputra Jun 3, 2022
62fd745
refactor: rename formSubmissionId to submissionId
hanstirtaputra Jun 7, 2022
3d87a52
refactor: feedback controller submitFormFeedbackV2
hanstirtaputra Jun 7, 2022
4c802f5
fix: update according to PR comments
hanstirtaputra Jun 7, 2022
05151d2
refactor: move submission.feedback.routes tests to feedback.routes
hanstirtaputra Jun 7, 2022
32db644
chore: update TODO comments to link issue number
hanstirtaputra Jun 7, 2022
e20cd53
refactor: rename feedback.error file and some constants
hanstirtaputra Jun 13, 2022
9dc9969
chore: add TODO comments linking to issue #3964
hanstirtaputra Jun 13, 2022
7bd48f1
fix: update feedback related error messages
hanstirtaputra Jun 13, 2022
da35ef5
refactor: update feedback model index syntax
hanstirtaputra Jun 14, 2022
ce041e2
chore: add TODO comments for feedback tests
hanstirtaputra Jun 14, 2022
59cf037
fix: remove unecessary formId field in hasNoPreviousFeedback
hanstirtaputra Jun 14, 2022
9b5b6aa
fix: update FormFeedbackBase submissionId field to optional
hanstirtaputra Jun 15, 2022
41b6cd0
refactor: form feedback tests to merge into one describe route
hanstirtaputra Jun 15, 2022
c0d6450
refactor: move submissions.feedback route to feedback route
hanstirtaputra Jun 15, 2022
a694bfd
fix: update log message for form feedback submission
hanstirtaputra Jul 4, 2022
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
3 changes: 3 additions & 0 deletions shared/types/form/form_feedback.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { Merge } from 'type-fest'
import { DateString } from '../generic'
import { SubmissionResponseDto } from '../submission'
import { FormDto } from './form'

export type SubmitFormFeedbackBodyDto = {
Expand All @@ -17,6 +18,8 @@ export type FormFeedbackBase = {
formId: FormDto['_id']
created?: Date
lastModified?: Date
// TODO #3964: Update to not optional once we fully migrate to /submissions/{submissionId}/feedback endpoint
submissionId?: SubmissionResponseDto['submissionId']
}

// Convert to serialized version.
Expand Down
11 changes: 11 additions & 0 deletions src/app/models/form_feedback.server.model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { Mongoose, QueryCursor, Schema } from 'mongoose'
import { IFormFeedbackModel, IFormFeedbackSchema } from '../../types'

import { FORM_SCHEMA_ID } from './form.server.model'
import { SUBMISSION_SCHEMA_ID } from './submission.server.model'

export const FORM_FEEDBACK_SCHEMA_ID = 'FormFeedback'
export const FORM_FEEDBACK_COLLECTION_NAME = 'formfeedback'
Expand All @@ -14,6 +15,12 @@ const FormFeedbackSchema = new Schema<IFormFeedbackSchema, IFormFeedbackModel>(
ref: FORM_SCHEMA_ID,
required: true,
},
submissionId: {
type: Schema.Types.ObjectId,
ref: SUBMISSION_SCHEMA_ID,
// TODO #3964: Update to true once we fully migrate to /submissions/{submissionId}/feedback endpoint
required: false,
},
rating: {
type: Number,
min: 1,
Expand All @@ -34,6 +41,10 @@ const FormFeedbackSchema = new Schema<IFormFeedbackSchema, IFormFeedbackModel>(
},
)

FormFeedbackSchema.index({
submissionId: 1,
})

/**
* Returns a cursor for all feedback for the form with formId.
* @param formId the form id to return the submissions cursor for
Expand Down
59 changes: 59 additions & 0 deletions src/app/modules/feedback/__tests__/feedback.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import dbHandler from 'tests/unit/backend/helpers/jest-db'

import { FormFeedbackMetaDto } from '../../../../../shared/types'
import { DatabaseError } from '../../core/core.errors'
import { DuplicateFeedbackSubmissionError } from '../feedback.errors'
import * as FeedbackService from '../feedback.service'

const FormFeedback = getFormFeedbackModel(mongoose)
Expand Down Expand Up @@ -248,4 +249,62 @@ describe('feedback.service', () => {
expect(actualResult._unsafeUnwrapErr()).toBeInstanceOf(DatabaseError)
})
})

describe('hasNoPreviousFeedback', () => {
const MOCK_FORM_ID = new ObjectId().toHexString()
const MOCK_SUBMISSION_ID = new ObjectId().toHexString()

beforeEach(async () => {
await dbHandler.clearCollection(FormFeedback.collection.name)
})
afterEach(() => jest.clearAllMocks())

it('should return DuplicateFeedbackSubmissionError when feedback already exists for given submissionId', async () => {
await FormFeedback.create({
comment: `test feedback`,
formId: MOCK_FORM_ID,
submissionId: MOCK_SUBMISSION_ID,
rating: 5,
})

const actualResult = await FeedbackService.hasNoPreviousFeedback(
MOCK_SUBMISSION_ID,
)

expect(actualResult.isErr()).toEqual(true)
expect(actualResult._unsafeUnwrapErr()).toBeInstanceOf(
DuplicateFeedbackSubmissionError,
)
})

it('should return true if there is no existing feedback for given submissionId', async () => {
await FormFeedback.create({
comment: `test feedback`,
formId: MOCK_FORM_ID,
rating: 5,
})

const actualResult = await FeedbackService.hasNoPreviousFeedback(
new ObjectId().toHexString(),
)

expect(actualResult.isOk()).toEqual(true)
expect(actualResult._unsafeUnwrap()).toEqual(true)
})

it('should return DatabaseError when error occurs whilst querying database', async () => {
const existSpy = jest.spyOn(FormFeedback, 'exists')
existSpy.mockImplementationOnce(() => Promise.reject(new Error('boom')))

const actualResult = await FeedbackService.hasNoPreviousFeedback(
MOCK_SUBMISSION_ID,
)

expect(existSpy).toHaveBeenCalledWith({
submissionId: MOCK_SUBMISSION_ID,
})
expect(actualResult.isErr()).toEqual(true)
expect(actualResult._unsafeUnwrapErr()).toBeInstanceOf(DatabaseError)
})
})
})
95 changes: 95 additions & 0 deletions src/app/modules/feedback/feedback.controller.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
import { celebrate, Joi, Segments } from 'celebrate'
import { StatusCodes } from 'http-status-codes'
import { ErrorDto, PrivateFormErrorDto } from 'shared/types'

import { createLoggerWithLabel } from '../../config/logger'
import { createReqMeta } from '../../utils/request'
import { ControllerHandler } from '../core/core.types'
import { PrivateFormError } from '../form/form.errors'
import * as FormService from '../form/form.service'
import * as PublicFormService from '../form/public-form/public-form.service'
import * as SubmissionService from '../submission/submission.service'

import * as FeedbackService from './feedback.service'
import { mapRouteError } from './feedback.util'

const logger = createLoggerWithLabel(module)

const validateSubmitFormFeedbackParams = celebrate({
[Segments.BODY]: Joi.object()
.keys({
rating: Joi.number().min(1).max(5).cast('string').required(),
comment: Joi.string().allow('').required(),
})
// Allow other keys for backwards compability as frontend might put extra keys in the body.
.unknown(true),
})

/**
* Handler for POST api/v3/forms/:formId/submissions/:submissionId/feedback endpoint
* @precondition formId and submissionId should be present in req.params.
* @precondition Joi validation should enforce shape of req.body before this handler is invoked.
*
* @returns 200 if feedback was successfully saved
* @returns 404 if form with formId does not exist or is private, or submissionId does not exist
* @returns 422 if duplicate feedback with the same submissionId and formId exists
* @returns 410 if form has been archived
* @returns 500 if database error occurs
*
* TODO #3964: Rename to `submitFormFeedback` once we fully migrate feedback endpoint to /submissions/{submissionId}/feedback
*/
const submitFormFeedbackV2: ControllerHandler<
mantariksh marked this conversation as resolved.
Show resolved Hide resolved
{ formId: string; submissionId: string },
{ message: string } | ErrorDto | PrivateFormErrorDto,
{ rating: number; comment: string }
> = async (req, res) => {
const { formId, submissionId } = req.params
const { rating, comment } = req.body
const logMeta = {
action: 'submitFormFeedbackV2',
...createReqMeta(req),
formId,
submissionId,
}

return SubmissionService.doesSubmissionIdExist(submissionId)
.andThen(() => FeedbackService.hasNoPreviousFeedback(submissionId))
.andThen(() => FormService.retrieveFullFormById(formId))
.andThen((form) => FormService.isFormPublic(form).map(() => form))
.andThen((form) => {
return PublicFormService.insertFormFeedback({
formId: form._id,
submissionId: submissionId,
rating,
comment,
}).map(() =>
res
.status(StatusCodes.OK)
.json({ message: 'Successfully submitted feedback' }),
)
})
.mapErr((error) => {
const { errorMessage, statusCode } = mapRouteError(error)
logger.error({
message: errorMessage,
mantariksh marked this conversation as resolved.
Show resolved Hide resolved
meta: logMeta,
error,
})

// Specialized error response for PrivateFormError.
if (error instanceof PrivateFormError) {
return res.status(statusCode).json({
message: error.message,
// Flag to prevent default 404 subtext ("please check link") from showing.
isPageFound: true,
formTitle: error.formTitle,
})
}
return res.status(statusCode).json({ message: errorMessage })
})
}

export const handleSubmitFormFeedback = [
validateSubmitFormFeedbackParams,
submitFormFeedbackV2,
] as ControllerHandler[]
15 changes: 15 additions & 0 deletions src/app/modules/feedback/feedback.errors.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import { ApplicationError } from '../core/core.errors'

export class InvalidSubmissionIdError extends ApplicationError {
constructor(message = 'Sorry, something went wrong. Please try again.') {
super(message)
}
}

export class DuplicateFeedbackSubmissionError extends ApplicationError {
constructor(
message = 'You have already submitted feedback for this submission. To provide additional feedback, please contact the form administrator.',
) {
super(message)
}
}
42 changes: 41 additions & 1 deletion src/app/modules/feedback/feedback.service.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { isEmpty } from 'lodash'
import moment from 'moment-timezone'
import mongoose from 'mongoose'
import { ResultAsync } from 'neverthrow'
import { errAsync, okAsync, ResultAsync } from 'neverthrow'

import {
FormFeedbackMetaDto,
Expand All @@ -13,6 +13,8 @@ import getFormFeedbackModel from '../../models/form_feedback.server.model'
import { getMongoErrorMessage } from '../../utils/handle-mongo-error'
import { DatabaseError } from '../core/core.errors'

import { DuplicateFeedbackSubmissionError } from './feedback.errors'

const FormFeedbackModel = getFormFeedbackModel(mongoose)
const logger = createLoggerWithLabel(module)

Expand Down Expand Up @@ -111,3 +113,41 @@ export const getFormFeedbacks = (
}
})
}

/**
* Checks if a feedback for the form with formId and submission with submissionId
* already exists.
*
* @param formId the form id of the form that the feedback is for
* @param submissionId the submission id of the form submission that the feedback is for
*
* @returns ok(true) if there is no existing feedback for the form with formId and submission
* with submissionId
* @returns err(InvalidSubmissionIdError) if submissionId does not exist amongst all the form submissions
* @returns err(DatabaseError) if database query errors
*/
export const hasNoPreviousFeedback = (
submissionId: string,
): ResultAsync<true, DuplicateFeedbackSubmissionError | DatabaseError> =>
ResultAsync.fromPromise(
FormFeedbackModel.exists({
mantariksh marked this conversation as resolved.
Show resolved Hide resolved
submissionId: submissionId,
}),
(error) => {
logger.error({
message: 'Error finding feedback documents from database',
meta: {
action: 'hasNoPreviousFeedback',
submissionId,
},
error,
})

return new DatabaseError(getMongoErrorMessage(error))
},
).andThen((hasPreviousFeedback) => {
if (hasPreviousFeedback) {
return errAsync(new DuplicateFeedbackSubmissionError())
}
return okAsync(true as const)
})
66 changes: 66 additions & 0 deletions src/app/modules/feedback/feedback.util.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
import { StatusCodes } from 'http-status-codes'

import { MapRouteError } from '../../../types'
import { createLoggerWithLabel } from '../../config/logger'
import { ApplicationError, DatabaseError } from '../core/core.errors'
import * as FormErrors from '../form/form.errors'

import {
DuplicateFeedbackSubmissionError,
InvalidSubmissionIdError,
} from './feedback.errors'

const logger = createLoggerWithLabel(module)

export const mapRouteError: MapRouteError = (
error: ApplicationError,
coreErrorMessage = 'Sorry, something went wrong. Please refresh and try again.',
) => {
switch (error.constructor) {
case FormErrors.FormNotFoundError:
return {
statusCode: StatusCodes.NOT_FOUND,
errorMessage:
'This form no longer exists, please contact the agency that gave you the form link if you wish to provide feedback.',
}
case FormErrors.FormDeletedError:
return {
statusCode: StatusCodes.GONE,
errorMessage:
'This form has been deleted, so feedback submissions are no longer accepted',
}
case FormErrors.PrivateFormError:
return {
statusCode: StatusCodes.NOT_FOUND,
errorMessage:
'This form has been made private, so feedback submissions are no longer accepted',
}
case InvalidSubmissionIdError:
return {
statusCode: StatusCodes.NOT_FOUND,
errorMessage: error.message,
}
case DuplicateFeedbackSubmissionError:
return {
statusCode: StatusCodes.UNPROCESSABLE_ENTITY,
errorMessage: error.message,
}
case DatabaseError:
return {
statusCode: StatusCodes.INTERNAL_SERVER_ERROR,
errorMessage: coreErrorMessage ?? error.message,
}
default:
logger.error({
message: 'mapRouteError called with unknown error type',
meta: {
action: 'mapRouteError',
},
error,
})
return {
statusCode: StatusCodes.INTERNAL_SERVER_ERROR,
errorMessage: coreErrorMessage,
}
}
}
10 changes: 2 additions & 8 deletions src/app/modules/form/public-form/public-form.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,15 +64,9 @@ const validateSubmitFormFeedbackParams = celebrate({
})

/**
* NOTE: This is exported solely for unit testing
* Handler for POST /:formId/feedback endpoint
* @precondition formId should be present in req.params.
* @precondition Joi validation should enforce shape of req.body before this handler is invoked.
* @deprecated use submitFormFeedbackV2 instead
mantariksh marked this conversation as resolved.
Show resolved Hide resolved
*
* @returns 200 if feedback was successfully saved
* @returns 404 if form with formId does not exist or is private
* @returns 410 if form has been archived
* @returns 500 if database error occurs
* TODO #3964: Cleanup we fully migrate feedback endpoint to /submissions/{submissionId}/feedback
*/
export const submitFormFeedback: ControllerHandler<
{ formId: string },
Expand Down
3 changes: 3 additions & 0 deletions src/app/modules/form/public-form/public-form.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,12 @@ const logger = createLoggerWithLabel(module)
*/
export const insertFormFeedback = ({
formId,
submissionId,
rating,
comment,
}: {
formId: string
submissionId?: string
rating: number
comment?: string
}): ResultAsync<IFormFeedbackSchema, FormNotFoundError | DatabaseError> => {
Expand All @@ -42,6 +44,7 @@ export const insertFormFeedback = ({
return ResultAsync.fromPromise(
FormFeedbackModel.create({
formId,
submissionId,
rating,
comment,
}),
Expand Down
Loading