From 115604e1a94f6fbfc7be00abaae1fe3825b2a101 Mon Sep 17 00:00:00 2001 From: Hans Tirtaputra Date: Wed, 1 Jun 2022 18:25:49 +0800 Subject: [PATCH 01/23] feat: add new feedback api route --- shared/types/form/form_feedback.ts | 1 + src/app/models/form_feedback.server.model.ts | 7 + .../modules/feedback/feedback.controller.ts | 163 ++++++++++++++++++ src/app/modules/feedback/feedback.service.ts | 24 +++ .../form/public-form/public-form.service.ts | 3 + .../modules/submission/submission.service.ts | 22 +++ .../api/v3/forms/public-forms.routes.ts | 3 + ...ublic-forms.submissions.feedback.routes.ts | 25 +++ 8 files changed, 248 insertions(+) create mode 100644 src/app/modules/feedback/feedback.controller.ts create mode 100644 src/app/routes/api/v3/forms/public-forms.submissions.feedback.routes.ts diff --git a/shared/types/form/form_feedback.ts b/shared/types/form/form_feedback.ts index 840e4ca201..5dbe11102d 100644 --- a/shared/types/form/form_feedback.ts +++ b/shared/types/form/form_feedback.ts @@ -17,6 +17,7 @@ export type FormFeedbackBase = { formId: FormDto['_id'] created?: Date lastModified?: Date + formSubmissionId: string } // Convert to serialized version. diff --git a/src/app/models/form_feedback.server.model.ts b/src/app/models/form_feedback.server.model.ts index 9f4e01554f..506dca7327 100644 --- a/src/app/models/form_feedback.server.model.ts +++ b/src/app/models/form_feedback.server.model.ts @@ -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' @@ -14,6 +15,12 @@ const FormFeedbackSchema = new Schema( ref: FORM_SCHEMA_ID, required: true, }, + formSubmissionId: { + type: Schema.Types.ObjectId, + ref: SUBMISSION_SCHEMA_ID, + // TODO: Update to true once we fully migrate to /submissions/{submissionId}/feedback endpoint + required: false, + }, rating: { type: Number, min: 1, diff --git a/src/app/modules/feedback/feedback.controller.ts b/src/app/modules/feedback/feedback.controller.ts new file mode 100644 index 0000000000..a61d7c2438 --- /dev/null +++ b/src/app/modules/feedback/feedback.controller.ts @@ -0,0 +1,163 @@ +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 { mapRouteError } from '../form/public-form/public-form.utils' +import * as SubmissionService from '../submission/submission.service' + +import * as FeedbackService from './feedback.service' + +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), +}) + +// TODO: Refactor this +export const submitFormSubmissionFeedback: ControllerHandler< + { 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 checkDoesSubmissionIdExistRes = + await SubmissionService.checkDoesSubmissionIdExist(submissionId) + if (checkDoesSubmissionIdExistRes.isErr()) { + const { error } = checkDoesSubmissionIdExistRes + logger.warn({ + message: 'Failed to check if submissionId is valid', + meta: { + action: 'submitFormSubmissionFeedback', + ...createReqMeta(req), + formId, + submissionId, + }, + error, + }) + const { errorMessage, statusCode } = mapRouteError(error) + return res.status(statusCode).json({ message: errorMessage }) + } + + const isSubmissionIdValid = checkDoesSubmissionIdExistRes.value + if (!isSubmissionIdValid) { + return res + .status(StatusCodes.NOT_FOUND) + .json({ message: 'SubmissionId is not valid' }) + } + + const checkHasPreviousFeedbackRes = + await FeedbackService.checkHasPreviousFeedback(formId, submissionId) + if (checkHasPreviousFeedbackRes.isErr()) { + const { error } = checkHasPreviousFeedbackRes + logger.warn({ + message: + 'Failed to check if feedback has already been submitted previously', + meta: { + action: 'submitFormSubmissionFeedback', + ...createReqMeta(req), + formId, + submissionId, + }, + error, + }) + const { errorMessage, statusCode } = mapRouteError(error) + return res.status(statusCode).json({ message: errorMessage }) + } + + const hasPreviousFeedback = checkHasPreviousFeedbackRes.value + if (hasPreviousFeedback) { + return res + .status(StatusCodes.CONFLICT) + .json({ message: 'Multiple feedbacks has already been submitted' }) + } + + const formResult = await FormService.retrieveFullFormById(formId) + if (formResult.isErr()) { + const { error } = formResult + logger.error({ + message: 'Failed to retrieve form', + meta: { + action: 'submitFormSubmissionFeedback', + ...createReqMeta(req), + formId, + submissionId, + }, + error, + }) + const { errorMessage, statusCode } = mapRouteError(error) + return res.status(statusCode).json({ message: errorMessage }) + } + + const form = formResult.value + const isPublicResult = FormService.isFormPublic(form) + if (isPublicResult.isErr()) { + const { error } = isPublicResult + logger.warn({ + message: 'Form is not public', + meta: { + action: 'submitFormSubmissionFeedback', + ...createReqMeta(req), + formId, + submissionId, + }, + error, + }) + const { errorMessage, statusCode } = mapRouteError(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 }) + } + + return PublicFormService.insertFormFeedback({ + formId: form._id, + formSubmissionId: submissionId, + rating, + comment, + }) + .map(() => + res + .status(StatusCodes.OK) + .json({ message: 'Successfully submitted feedback' }), + ) + .mapErr((error) => { + logger.error({ + message: 'Error creating form feedback', + meta: { + action: 'submitFormSubmissionFeedback', + ...createReqMeta(req), + formId, + submissionId, + }, + error, + }) + const { errorMessage, statusCode } = mapRouteError(error) + return res.status(statusCode).json({ message: errorMessage }) + }) +} + +export const handleSubmitFormFeedback = [ + validateSubmitFormFeedbackParams, + submitFormSubmissionFeedback, +] as ControllerHandler[] diff --git a/src/app/modules/feedback/feedback.service.ts b/src/app/modules/feedback/feedback.service.ts index 4bc84203e9..eaf1f9b64f 100644 --- a/src/app/modules/feedback/feedback.service.ts +++ b/src/app/modules/feedback/feedback.service.ts @@ -111,3 +111,27 @@ export const getFormFeedbacks = ( } }) } + +export const checkHasPreviousFeedback = ( + formId: string, + submissionId: string, +): ResultAsync => + ResultAsync.fromPromise( + FormFeedbackModel.exists({ + formId: formId, + formSubmissionId: submissionId, + }), + (error) => { + logger.error({ + message: 'Error finding feedback documents from database', + meta: { + action: 'checkHasPreviousFeedback', + formId, + submissionId, + }, + error, + }) + + return new DatabaseError(getMongoErrorMessage(error)) + }, + ) diff --git a/src/app/modules/form/public-form/public-form.service.ts b/src/app/modules/form/public-form/public-form.service.ts index e3f33934e4..8f54750537 100644 --- a/src/app/modules/form/public-form/public-form.service.ts +++ b/src/app/modules/form/public-form/public-form.service.ts @@ -28,10 +28,12 @@ const logger = createLoggerWithLabel(module) */ export const insertFormFeedback = ({ formId, + formSubmissionId, rating, comment, }: { formId: string + formSubmissionId?: string rating: number comment?: string }): ResultAsync => { @@ -42,6 +44,7 @@ export const insertFormFeedback = ({ return ResultAsync.fromPromise( FormFeedbackModel.create({ formId, + formSubmissionId, rating, comment, }), diff --git a/src/app/modules/submission/submission.service.ts b/src/app/modules/submission/submission.service.ts index 4d86e03638..cea0e7fb0c 100644 --- a/src/app/modules/submission/submission.service.ts +++ b/src/app/modules/submission/submission.service.ts @@ -12,6 +12,7 @@ import getSubmissionModel from '../../models/submission.server.model' import MailService from '../../services/mail/mail.service' import { AutoReplyMailData } from '../../services/mail/mail.types' import { createQueryWithDateParam, isMalformedDate } from '../../utils/date' +import { getMongoErrorMessage } from '../../utils/handle-mongo-error' import { DatabaseError, MalformedParametersError } from '../core/core.errors' import { SendEmailConfirmationError } from './submission.errors' @@ -132,3 +133,24 @@ export const sendEmailConfirmations = ({ return okAsync(true as const) }) } + +export const checkDoesSubmissionIdExist = ( + submissionId: string, +): ResultAsync => + ResultAsync.fromPromise( + SubmissionModel.exists({ + _id: submissionId, + }), + (error) => { + logger.error({ + message: 'Error finding _id from submissions collection in database', + meta: { + action: 'checkDoesSubmissionIdExist', + submissionId, + }, + error, + }) + + return new DatabaseError(getMongoErrorMessage(error)) + }, + ) diff --git a/src/app/routes/api/v3/forms/public-forms.routes.ts b/src/app/routes/api/v3/forms/public-forms.routes.ts index a5db22e138..5af996898e 100644 --- a/src/app/routes/api/v3/forms/public-forms.routes.ts +++ b/src/app/routes/api/v3/forms/public-forms.routes.ts @@ -3,6 +3,7 @@ import { Router } from 'express' import { PublicFormsAuthRouter } from './public-forms.auth.routes' import { PublicFormsFeedbackRouter } from './public-forms.feedback.routes' import { PublicFormsFormRouter } from './public-forms.form.routes' +import { PublicFormSubmissionsFeedbackRouter } from './public-forms.submissions.feedback.routes' import { PublicFormsSubmissionsRouter } from './public-forms.submissions.routes' import { PublicFormsVerificationRouter } from './public-forms.verification.routes' @@ -13,3 +14,5 @@ PublicFormsRouter.use(PublicFormsFeedbackRouter) PublicFormsRouter.use(PublicFormsFormRouter) PublicFormsRouter.use(PublicFormsAuthRouter) PublicFormsRouter.use(PublicFormsVerificationRouter) +// TODO: Cleanup PublicFormsFeedbackRouter once it's fully migrated to PublicFormSubmissionsFeedbackRouter +PublicFormsRouter.use(PublicFormSubmissionsFeedbackRouter) diff --git a/src/app/routes/api/v3/forms/public-forms.submissions.feedback.routes.ts b/src/app/routes/api/v3/forms/public-forms.submissions.feedback.routes.ts new file mode 100644 index 0000000000..267806f38f --- /dev/null +++ b/src/app/routes/api/v3/forms/public-forms.submissions.feedback.routes.ts @@ -0,0 +1,25 @@ +import { Router } from 'express' + +import * as FeedbackController from '../../../../modules/feedback/feedback.controller' + +export const PublicFormSubmissionsFeedbackRouter = Router() + +/** + * Send feedback for a public form + * @route POST /:formId/submissions/:submissionId/feedback + * @group forms - endpoints to serve forms + * @param {string} formId.path.required - the form id + * @param {string} submissionId.path.required - the form submission id + * @param {Feedback.model} feedback.body.required - the user's feedback + * @consumes application/json + * @produces application/json + * @returns 200 if feedback was successfully saved + * @returns 400 if form feedback was malformed and hence cannot be saved + * @returns 404 if form with formId or submissionId does not exist, or form is private + * @returns 409 if form feedback for the submissionId has already been submitted + * @returns 410 if form has been archived + * @returns 500 if database error occurs + */ +PublicFormSubmissionsFeedbackRouter.route( + '/:formId([a-fA-F0-9]{24})/submissions/:submissionId([a-fA-F0-9]{24})/feedback', +).post(FeedbackController.handleSubmitFormFeedback) From 86193e3da3e2dfa4eac975512c66d07d856d2b8b Mon Sep 17 00:00:00 2001 From: Hans Tirtaputra Date: Thu, 2 Jun 2022 11:52:45 +0800 Subject: [PATCH 02/23] feat: link Angular FE to new feedback endpoint --- .../forms/base/components/feedback-form.client.component.js | 5 ++++- .../base/directiveViews/submit-form.directive.view.html | 1 + src/public/services/FormFeedbackService.ts | 4 +++- 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/public/modules/forms/base/components/feedback-form.client.component.js b/src/public/modules/forms/base/components/feedback-form.client.component.js index 3467867f07..af82b929d5 100644 --- a/src/public/modules/forms/base/components/feedback-form.client.component.js +++ b/src/public/modules/forms/base/components/feedback-form.client.component.js @@ -8,6 +8,7 @@ angular.module('forms').component('feedbackFormComponent', { bindings: { isPreview: '<', formId: '@', + submissionId: '@', colorTheme: '@', }, controller: ['Toastr', '$q', feedbackController], @@ -33,7 +34,9 @@ function feedbackController(Toastr, $q) { isPreview: vm.isPreview, } - $q.when(FormFeedbackService.postFeedback(vm.formId, feedback)).then( + $q.when( + FormFeedbackService.postFeedback(vm.formId, vm.submissionId, feedback), + ).then( function () { vm.isSubmitted = true vm.isLoading = false diff --git a/src/public/modules/forms/base/directiveViews/submit-form.directive.view.html b/src/public/modules/forms/base/directiveViews/submit-form.directive.view.html index 30315f52c3..68c06b8220 100644 --- a/src/public/modules/forms/base/directiveViews/submit-form.directive.view.html +++ b/src/public/modules/forms/base/directiveViews/submit-form.directive.view.html @@ -121,6 +121,7 @@ diff --git a/src/public/services/FormFeedbackService.ts b/src/public/services/FormFeedbackService.ts index 7d00bc217d..902b644b23 100644 --- a/src/public/services/FormFeedbackService.ts +++ b/src/public/services/FormFeedbackService.ts @@ -15,16 +15,18 @@ export const ADMIN_FORM_ENDPOINT = '/api/v3/admin/forms' /** * Post feedback for a given form. * @param formId the id of the form to post feedback for + * @param submissionId the id of the form submission to post feedback for * @param feedbackToPost object containing the feedback * @returns success message */ export const postFeedback = async ( formId: string, + submissionId: string, feedbackToPost: SubmitFormFeedbackBodyDto, ): Promise => { return axios .post( - `${PUBLIC_FORM_ENDPOINT}/${formId}/feedback`, + `${PUBLIC_FORM_ENDPOINT}/${formId}/submissions/${submissionId}/feedback`, feedbackToPost, ) .then(({ data }) => data) From 9116f92f338ae14f1a48d1dbf5012ab4527a35c6 Mon Sep 17 00:00:00 2001 From: Hans Tirtaputra Date: Thu, 2 Jun 2022 13:19:12 +0800 Subject: [PATCH 03/23] refactor: feedback controller --- .../modules/feedback/feedback.controller.ts | 23 +++++++++---------- 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/src/app/modules/feedback/feedback.controller.ts b/src/app/modules/feedback/feedback.controller.ts index a61d7c2438..8708f2868e 100644 --- a/src/app/modules/feedback/feedback.controller.ts +++ b/src/app/modules/feedback/feedback.controller.ts @@ -25,8 +25,7 @@ const validateSubmitFormFeedbackParams = celebrate({ .unknown(true), }) -// TODO: Refactor this -export const submitFormSubmissionFeedback: ControllerHandler< +export const submitFormFeedback: ControllerHandler< { formId: string; submissionId: string }, { message: string } | ErrorDto | PrivateFormErrorDto, { rating: number; comment: string } @@ -38,10 +37,10 @@ export const submitFormSubmissionFeedback: ControllerHandler< await SubmissionService.checkDoesSubmissionIdExist(submissionId) if (checkDoesSubmissionIdExistRes.isErr()) { const { error } = checkDoesSubmissionIdExistRes - logger.warn({ - message: 'Failed to check if submissionId is valid', + logger.error({ + message: 'Failed to check if submissionId exists', meta: { - action: 'submitFormSubmissionFeedback', + action: 'submitFormFeedback', ...createReqMeta(req), formId, submissionId, @@ -63,11 +62,11 @@ export const submitFormSubmissionFeedback: ControllerHandler< await FeedbackService.checkHasPreviousFeedback(formId, submissionId) if (checkHasPreviousFeedbackRes.isErr()) { const { error } = checkHasPreviousFeedbackRes - logger.warn({ + logger.error({ message: 'Failed to check if feedback has already been submitted previously', meta: { - action: 'submitFormSubmissionFeedback', + action: 'submitFormFeedback', ...createReqMeta(req), formId, submissionId, @@ -91,7 +90,7 @@ export const submitFormSubmissionFeedback: ControllerHandler< logger.error({ message: 'Failed to retrieve form', meta: { - action: 'submitFormSubmissionFeedback', + action: 'submitFormFeedback', ...createReqMeta(req), formId, submissionId, @@ -106,10 +105,10 @@ export const submitFormSubmissionFeedback: ControllerHandler< const isPublicResult = FormService.isFormPublic(form) if (isPublicResult.isErr()) { const { error } = isPublicResult - logger.warn({ + logger.error({ message: 'Form is not public', meta: { - action: 'submitFormSubmissionFeedback', + action: 'submitFormFeedback', ...createReqMeta(req), formId, submissionId, @@ -145,7 +144,7 @@ export const submitFormSubmissionFeedback: ControllerHandler< logger.error({ message: 'Error creating form feedback', meta: { - action: 'submitFormSubmissionFeedback', + action: 'submitFormFeedback', ...createReqMeta(req), formId, submissionId, @@ -159,5 +158,5 @@ export const submitFormSubmissionFeedback: ControllerHandler< export const handleSubmitFormFeedback = [ validateSubmitFormFeedbackParams, - submitFormSubmissionFeedback, + submitFormFeedback, ] as ControllerHandler[] From b2c07558f19b7938d9cf66ff7770d621e38c5158 Mon Sep 17 00:00:00 2001 From: Hans Tirtaputra Date: Thu, 2 Jun 2022 17:37:38 +0800 Subject: [PATCH 04/23] refactor: feedback controller log meta --- .../modules/feedback/feedback.controller.ts | 41 +++++-------------- 1 file changed, 11 insertions(+), 30 deletions(-) diff --git a/src/app/modules/feedback/feedback.controller.ts b/src/app/modules/feedback/feedback.controller.ts index 8708f2868e..b60a1dc73d 100644 --- a/src/app/modules/feedback/feedback.controller.ts +++ b/src/app/modules/feedback/feedback.controller.ts @@ -32,6 +32,12 @@ export const submitFormFeedback: ControllerHandler< > = async (req, res) => { const { formId, submissionId } = req.params const { rating, comment } = req.body + const logMeta = { + action: 'submitFormFeedback', + ...createReqMeta(req), + formId, + submissionId, + } const checkDoesSubmissionIdExistRes = await SubmissionService.checkDoesSubmissionIdExist(submissionId) @@ -39,12 +45,7 @@ export const submitFormFeedback: ControllerHandler< const { error } = checkDoesSubmissionIdExistRes logger.error({ message: 'Failed to check if submissionId exists', - meta: { - action: 'submitFormFeedback', - ...createReqMeta(req), - formId, - submissionId, - }, + meta: logMeta, error, }) const { errorMessage, statusCode } = mapRouteError(error) @@ -65,12 +66,7 @@ export const submitFormFeedback: ControllerHandler< logger.error({ message: 'Failed to check if feedback has already been submitted previously', - meta: { - action: 'submitFormFeedback', - ...createReqMeta(req), - formId, - submissionId, - }, + meta: logMeta, error, }) const { errorMessage, statusCode } = mapRouteError(error) @@ -89,12 +85,7 @@ export const submitFormFeedback: ControllerHandler< const { error } = formResult logger.error({ message: 'Failed to retrieve form', - meta: { - action: 'submitFormFeedback', - ...createReqMeta(req), - formId, - submissionId, - }, + meta: logMeta, error, }) const { errorMessage, statusCode } = mapRouteError(error) @@ -107,12 +98,7 @@ export const submitFormFeedback: ControllerHandler< const { error } = isPublicResult logger.error({ message: 'Form is not public', - meta: { - action: 'submitFormFeedback', - ...createReqMeta(req), - formId, - submissionId, - }, + meta: logMeta, error, }) const { errorMessage, statusCode } = mapRouteError(error) @@ -143,12 +129,7 @@ export const submitFormFeedback: ControllerHandler< .mapErr((error) => { logger.error({ message: 'Error creating form feedback', - meta: { - action: 'submitFormFeedback', - ...createReqMeta(req), - formId, - submissionId, - }, + meta: logMeta, error, }) const { errorMessage, statusCode } = mapRouteError(error) From 23bcb2a54cb727a588889dfa39c4dcda464250dc Mon Sep 17 00:00:00 2001 From: Hans Tirtaputra Date: Thu, 2 Jun 2022 17:38:35 +0800 Subject: [PATCH 05/23] feat: add tests for services --- .../__tests__/feedback.service.spec.ts | 60 ++++++++++++++++++ .../submission/submission.service.spec.ts | 62 +++++++++++++++++++ 2 files changed, 122 insertions(+) diff --git a/src/app/modules/feedback/__tests__/feedback.service.spec.ts b/src/app/modules/feedback/__tests__/feedback.service.spec.ts index 8da48ad08d..b01dc7c123 100644 --- a/src/app/modules/feedback/__tests__/feedback.service.spec.ts +++ b/src/app/modules/feedback/__tests__/feedback.service.spec.ts @@ -248,4 +248,64 @@ describe('feedback.service', () => { expect(actualResult._unsafeUnwrapErr()).toBeInstanceOf(DatabaseError) }) }) + + describe('checkHasPreviousFeedback', () => { + 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 true when previous feedback exists', async () => { + await FormFeedback.create({ + comment: `test feedback`, + formId: MOCK_FORM_ID, + formSubmissionId: MOCK_SUBMISSION_ID, + rating: 5, + }) + + const actualResult = await FeedbackService.checkHasPreviousFeedback( + MOCK_FORM_ID, + MOCK_SUBMISSION_ID, + ) + + expect(actualResult.isOk()).toEqual(true) + expect(actualResult._unsafeUnwrap()).toEqual(true) + }) + + it('should return false previous feedback does not exist', async () => { + await FormFeedback.create({ + comment: `test feedback`, + formId: MOCK_FORM_ID, + rating: 5, + }) + + const actualResult = await FeedbackService.checkHasPreviousFeedback( + MOCK_FORM_ID, + new ObjectId().toHexString(), + ) + + expect(actualResult.isOk()).toEqual(true) + expect(actualResult._unsafeUnwrap()).toEqual(false) + }) + + 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.checkHasPreviousFeedback( + MOCK_FORM_ID, + MOCK_SUBMISSION_ID, + ) + + expect(existSpy).toHaveBeenCalledWith({ + formId: MOCK_FORM_ID, + formSubmissionId: MOCK_SUBMISSION_ID, + }) + expect(actualResult.isErr()).toEqual(true) + expect(actualResult._unsafeUnwrapErr()).toBeInstanceOf(DatabaseError) + }) + }) }) diff --git a/src/app/modules/submission/__tests__/submission/submission.service.spec.ts b/src/app/modules/submission/__tests__/submission/submission.service.spec.ts index 3361cf9052..388a60d5ef 100644 --- a/src/app/modules/submission/__tests__/submission/submission.service.spec.ts +++ b/src/app/modules/submission/__tests__/submission/submission.service.spec.ts @@ -721,4 +721,66 @@ describe('submission.service', () => { ) }) }) + + describe('checkDoesSubmissionIdExist', () => { + const MOCK_SUBMISSION_ID = MOCK_SUBMISSION._id + + beforeEach(async () => { + await dbHandler.clearCollection(Submission.collection.name) + }) + afterEach(() => jest.clearAllMocks()) + + it('should return true with valid submissionId', async () => { + await Submission.create({ + _id: MOCK_SUBMISSION_ID, + submissionType: SubmissionType.Encrypt, + form: MOCK_FORM_ID, + encryptedContent: 'some random encrypted content', + version: 1, + responseHash: 'hash', + responseSalt: 'salt', + }) + + const actualResult = await SubmissionService.checkDoesSubmissionIdExist( + MOCK_SUBMISSION_ID, + ) + + expect(actualResult.isOk()).toEqual(true) + expect(actualResult._unsafeUnwrap()).toEqual(true) + }) + + it('should return false with invalid submissionId', async () => { + await Submission.create({ + _id: MOCK_SUBMISSION_ID, + submissionType: SubmissionType.Encrypt, + form: MOCK_FORM_ID, + encryptedContent: 'some random encrypted content', + version: 1, + responseHash: 'hash', + responseSalt: 'salt', + }) + + const actualResult = await SubmissionService.checkDoesSubmissionIdExist( + new ObjectId().toHexString(), + ) + + expect(actualResult.isOk()).toEqual(true) + expect(actualResult._unsafeUnwrap()).toEqual(false) + }) + + it('should return DatabaseError when error occurs whilst querying database', async () => { + const existSpy = jest.spyOn(Submission, 'exists') + existSpy.mockImplementationOnce(() => Promise.reject(new Error('boom'))) + + const actualResult = await SubmissionService.checkDoesSubmissionIdExist( + MOCK_SUBMISSION_ID, + ) + + expect(existSpy).toHaveBeenCalledWith({ + _id: MOCK_SUBMISSION_ID, + }) + expect(actualResult.isErr()).toEqual(true) + expect(actualResult._unsafeUnwrapErr()).toBeInstanceOf(DatabaseError) + }) + }) }) From 36736efbc74704086e3211e773f3a0057be6d035 Mon Sep 17 00:00:00 2001 From: Hans Tirtaputra Date: Thu, 2 Jun 2022 17:40:01 +0800 Subject: [PATCH 06/23] feat: add test for new feedback endpoint --- ...-forms.submissions.feedback.routes.spec.ts | 219 ++++++++++++++++++ tests/unit/backend/helpers/jest-db.ts | 58 +++++ 2 files changed, 277 insertions(+) create mode 100644 src/app/routes/api/v3/forms/__tests__/public-forms.submissions.feedback.routes.spec.ts diff --git a/src/app/routes/api/v3/forms/__tests__/public-forms.submissions.feedback.routes.spec.ts b/src/app/routes/api/v3/forms/__tests__/public-forms.submissions.feedback.routes.spec.ts new file mode 100644 index 0000000000..9169fb554a --- /dev/null +++ b/src/app/routes/api/v3/forms/__tests__/public-forms.submissions.feedback.routes.spec.ts @@ -0,0 +1,219 @@ +import { ObjectId } from 'mongodb' +import { errAsync } from 'neverthrow' +import supertest, { Session } from 'supertest-session' + +import { DatabaseError } from 'src/app/modules/core/core.errors' + +import { setupApp } from 'tests/integration/helpers/express-setup' +import { buildCelebrateError } from 'tests/unit/backend/helpers/celebrate' +import dbHandler from 'tests/unit/backend/helpers/jest-db' + +import { FormStatus } from '../../../../../../../shared/types' +import * as FormService from '../../../../../modules/form/form.service' +import { PublicFormsRouter } from '../public-forms.routes' + +const app = setupApp('/forms', PublicFormsRouter) + +describe('public-form.submissions.feedback.routes', () => { + let request: Session + + beforeAll(async () => await dbHandler.connect()) + beforeEach(async () => { + request = supertest(app) + }) + afterEach(async () => { + await dbHandler.clearDatabase() + jest.restoreAllMocks() + }) + afterAll(async () => await dbHandler.closeDatabase()) + describe('POST /forms/:formId/submissions/:submissionId/feedback', () => { + it('should return 200 when feedback was successfully saved', async () => { + const MOCK_FEEDBACK = { + rating: 5, + comment: 'great mock', + } + + const { form } = await dbHandler.insertEmailForm({ + formOptions: { + status: FormStatus.Public, + }, + }) + const { submission } = await dbHandler.insertFormSubmission({ + formId: form._id, + }) + + const expectedResp = JSON.parse( + JSON.stringify({ message: 'Successfully submitted feedback' }), + ) + const actualResp = await request + .post(`/forms/${form._id}/submissions/${submission._id}/feedback`) + .send(MOCK_FEEDBACK) + + expect(actualResp.status).toEqual(200) + expect(actualResp.body).toEqual(expectedResp) + }) + + it('should return 400 when form feedback submitted is malformed', async () => { + const MOCK_FEEDBACK = { rating: 6 } + + const { form } = await dbHandler.insertEmailForm() + const { submission } = await dbHandler.insertFormSubmission({ + formId: form._id, + }) + + const actualResp = await request + .post(`/forms/${form._id}/submissions/${submission._id}/feedback`) + .send(MOCK_FEEDBACK) + + expect(actualResp.status).toEqual(400) + expect(actualResp.body).toEqual( + buildCelebrateError({ + body: { + key: 'rating', + message: '"rating" must be less than or equal to 5', + }, + }), + ) + }) + + it('should return 404 when form is private', async () => { + const MOCK_FEEDBACK = { + rating: 5, + comment: 'great mock', + } + + const { form } = await dbHandler.insertEmailForm() + const { submission } = await dbHandler.insertFormSubmission({ + formId: form._id, + }) + const expectedResp = JSON.parse( + JSON.stringify({ + message: form.inactiveMessage, + formTitle: form.title, + isPageFound: true, + }), + ) + + const actualResp = await request + .post(`/forms/${form._id}/submissions/${submission._id}/feedback`) + .send(MOCK_FEEDBACK) + + expect(actualResp.status).toEqual(404) + expect(actualResp.body).toEqual(expectedResp) + }) + + it('should return 404 when submissionId does not exist', async () => { + const MOCK_FEEDBACK = { + rating: 5, + comment: 'great mock', + } + + const { form } = await dbHandler.insertEmailForm() + const expectedResp = JSON.parse( + JSON.stringify({ + message: 'SubmissionId is not valid', + }), + ) + + const INVALID_SUBMISSION_ID = new ObjectId().toHexString() + const actualResp = await request + .post( + `/forms/${form._id}/submissions/${INVALID_SUBMISSION_ID}/feedback`, + ) + .send(MOCK_FEEDBACK) + + expect(actualResp.status).toEqual(404) + expect(actualResp.body).toEqual(expectedResp) + }) + + it('should return 409 when form feedback for the submissionId has already been submitted', async () => { + const MOCK_FEEDBACK = { + rating: 5, + comment: 'great mock', + } + + const { form } = await dbHandler.insertEmailForm() + const { submission } = await dbHandler.insertFormSubmission({ + formId: form._id, + }) + await dbHandler.insertFormFeedback({ + formId: form._id, + formSubmissionId: submission._id, + }) + + const expectedResp = JSON.parse( + JSON.stringify({ + message: 'Multiple feedbacks has already been submitted', + }), + ) + const actualResp = await request + .post(`/forms/${form._id}/submissions/${submission._id}/feedback`) + .send(MOCK_FEEDBACK) + + expect(actualResp.status).toEqual(409) + expect(actualResp.body).toEqual(expectedResp) + }) + + it('should return 410 when form is archived', async () => { + const MOCK_FEEDBACK = { + rating: 5, + comment: 'great mock', + } + + const { form } = await dbHandler.insertEmailForm({ + formOptions: { + status: FormStatus.Archived, + }, + }) + const { submission } = await dbHandler.insertFormSubmission({ + formId: form._id, + }) + + const expectedResp = JSON.parse( + JSON.stringify({ + message: 'This form is no longer active', + }), + ) + + const actualResp = await request + .post(`/forms/${form._id}/submissions/${submission._id}/feedback`) + .send(MOCK_FEEDBACK) + + expect(actualResp.status).toEqual(410) + expect(actualResp.body).toEqual(expectedResp) + }) + + it('should return 500 when form could not be retrieved due to a database error', async () => { + const MOCK_ERROR_MESSAGE = 'mock me' + const MOCK_FEEDBACK = { + rating: 5, + comment: 'great mock', + } + + const { form } = await dbHandler.insertEmailForm({ + formOptions: { + status: FormStatus.Public, + }, + }) + const { submission } = await dbHandler.insertFormSubmission({ + formId: form._id, + }) + + const expectedResp = JSON.parse( + JSON.stringify({ + message: MOCK_ERROR_MESSAGE, + }), + ) + jest + .spyOn(FormService, 'retrieveFullFormById') + .mockReturnValueOnce(errAsync(new DatabaseError(MOCK_ERROR_MESSAGE))) + + const actualResp = await request + .post(`/forms/${form._id}/submissions/${submission._id}/feedback`) + .send(MOCK_FEEDBACK) + + expect(actualResp.status).toEqual(500) + expect(actualResp.body).toEqual(expectedResp) + }) + }) +}) diff --git a/tests/unit/backend/helpers/jest-db.ts b/tests/unit/backend/helpers/jest-db.ts index 8cdb5d57e0..be0621e6b2 100644 --- a/tests/unit/backend/helpers/jest-db.ts +++ b/tests/unit/backend/helpers/jest-db.ts @@ -7,6 +7,8 @@ import getFormModel, { getEmailFormModel, getEncryptedFormModel, } from 'src/app/models/form.server.model' +import getFormFeedbackModel from 'src/app/models/form_feedback.server.model' +import getSubmissionModel from 'src/app/models/submission.server.model' import getUserModel from 'src/app/models/user.server.model' import { AgencyDocument, @@ -16,8 +18,10 @@ import { IEncryptedForm, IEncryptedFormSchema, IForm, + IFormFeedbackSchema, IFormSchema, IPopulatedForm, + ISubmissionSchema, IUserSchema, } from 'src/types' @@ -269,6 +273,58 @@ const getFullFormById = async ( ): Promise => await getFormModel(mongoose).getFullFormById(formId) +const insertFormSubmission = async ({ + formId, + submissionType = 'encryptSubmission', + version = '1', + encryptedContent = 'encryptedContent', +}: { + formId?: ObjectID + submissionType?: string + version?: string + encryptedContent?: string +} = {}): Promise<{ + submission: ISubmissionSchema +}> => { + const SubmissionFormModel = getSubmissionModel(mongoose) + const submission = await SubmissionFormModel.create({ + form: formId, + submissionType, + encryptedContent, + version, + }) + + return { + submission: submission as ISubmissionSchema, + } +} + +const insertFormFeedback = async ({ + formId, + formSubmissionId, + rating = '5', + comment = 'FormSG rocks!', +}: { + formId?: ObjectID + formSubmissionId?: ObjectID + rating?: string + comment?: string +} = {}): Promise<{ + formFeedback: IFormFeedbackSchema +}> => { + const FormFeedbackModel = getFormFeedbackModel(mongoose) + const formFeedback = await FormFeedbackModel.create({ + formId, + comment, + rating, + formSubmissionId, + }) + + return { + formFeedback: formFeedback as IFormFeedbackSchema, + } +} + const dbHandler = { connect, closeDatabase, @@ -281,6 +337,8 @@ const dbHandler = { insertEncryptForm, insertFormWithMsgSrvcName, getFullFormById, + insertFormSubmission, + insertFormFeedback, } export default dbHandler From 5f0a5ea7af9d36bf8b69d4d0f59e24012a87b363 Mon Sep 17 00:00:00 2001 From: Hans Tirtaputra Date: Thu, 2 Jun 2022 18:03:04 +0800 Subject: [PATCH 07/23] fix: update failing FE tests --- .../services/__tests__/FormFeedbackService.spec.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/public/services/__tests__/FormFeedbackService.spec.ts b/src/public/services/__tests__/FormFeedbackService.spec.ts index 30ab90bd47..ed89b9a558 100644 --- a/src/public/services/__tests__/FormFeedbackService.spec.ts +++ b/src/public/services/__tests__/FormFeedbackService.spec.ts @@ -8,8 +8,10 @@ jest.mock('axios', () => MockAxios) FeedbackCsvGenerator.prototype.addLineFromFeedback = jest.fn() describe('FormFeedbackService', () => { - afterEach(() => jest.clearAllMocks()) const mockFormId = 'mock-form-id' + const mockSubmissionId = 'mock-submission-id' + + afterEach(() => jest.clearAllMocks()) describe('postFeedback', () => { const mockFeedback = { formId: mockFormId, @@ -24,12 +26,13 @@ describe('FormFeedbackService', () => { // Act const actual = await FormFeedbackService.postFeedback( mockFormId, + mockSubmissionId, mockFeedback, ) // Assert expect(MockAxios.post).toHaveBeenCalledWith( - `${FormFeedbackService.PUBLIC_FORM_ENDPOINT}/${mockFormId}/feedback`, + `${FormFeedbackService.PUBLIC_FORM_ENDPOINT}/${mockFormId}/submissions/${mockSubmissionId}/feedback`, mockFeedback, ) expect(actual).toEqual(mockFeedback) @@ -43,13 +46,14 @@ describe('FormFeedbackService', () => { // Act const actualPromise = FormFeedbackService.postFeedback( mockFormId, + mockSubmissionId, mockFeedback, ) // Assert await expect(actualPromise).rejects.toEqual(expected) expect(MockAxios.post).toHaveBeenCalledWith( - `${FormFeedbackService.PUBLIC_FORM_ENDPOINT}/${mockFormId}/feedback`, + `${FormFeedbackService.PUBLIC_FORM_ENDPOINT}/${mockFormId}/submissions/${mockSubmissionId}/feedback`, mockFeedback, ) }) From b3a8cf523b191cd828508234d76a9ff3d1609fb3 Mon Sep 17 00:00:00 2001 From: Hans Tirtaputra Date: Fri, 3 Jun 2022 21:22:01 +0800 Subject: [PATCH 08/23] fix: update error status code duplicate feedback --- src/app/modules/feedback/feedback.controller.ts | 2 +- .../public-forms.submissions.feedback.routes.spec.ts | 4 ++-- .../api/v3/forms/public-forms.submissions.feedback.routes.ts | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/app/modules/feedback/feedback.controller.ts b/src/app/modules/feedback/feedback.controller.ts index b60a1dc73d..2842424f2c 100644 --- a/src/app/modules/feedback/feedback.controller.ts +++ b/src/app/modules/feedback/feedback.controller.ts @@ -76,7 +76,7 @@ export const submitFormFeedback: ControllerHandler< const hasPreviousFeedback = checkHasPreviousFeedbackRes.value if (hasPreviousFeedback) { return res - .status(StatusCodes.CONFLICT) + .status(StatusCodes.UNPROCESSABLE_ENTITY) .json({ message: 'Multiple feedbacks has already been submitted' }) } diff --git a/src/app/routes/api/v3/forms/__tests__/public-forms.submissions.feedback.routes.spec.ts b/src/app/routes/api/v3/forms/__tests__/public-forms.submissions.feedback.routes.spec.ts index 9169fb554a..8971ea24a6 100644 --- a/src/app/routes/api/v3/forms/__tests__/public-forms.submissions.feedback.routes.spec.ts +++ b/src/app/routes/api/v3/forms/__tests__/public-forms.submissions.feedback.routes.spec.ts @@ -126,7 +126,7 @@ describe('public-form.submissions.feedback.routes', () => { expect(actualResp.body).toEqual(expectedResp) }) - it('should return 409 when form feedback for the submissionId has already been submitted', async () => { + it('should return 422 when form feedback for the submissionId has already been submitted', async () => { const MOCK_FEEDBACK = { rating: 5, comment: 'great mock', @@ -150,7 +150,7 @@ describe('public-form.submissions.feedback.routes', () => { .post(`/forms/${form._id}/submissions/${submission._id}/feedback`) .send(MOCK_FEEDBACK) - expect(actualResp.status).toEqual(409) + expect(actualResp.status).toEqual(422) expect(actualResp.body).toEqual(expectedResp) }) diff --git a/src/app/routes/api/v3/forms/public-forms.submissions.feedback.routes.ts b/src/app/routes/api/v3/forms/public-forms.submissions.feedback.routes.ts index 267806f38f..d5dda309d3 100644 --- a/src/app/routes/api/v3/forms/public-forms.submissions.feedback.routes.ts +++ b/src/app/routes/api/v3/forms/public-forms.submissions.feedback.routes.ts @@ -16,8 +16,8 @@ export const PublicFormSubmissionsFeedbackRouter = Router() * @returns 200 if feedback was successfully saved * @returns 400 if form feedback was malformed and hence cannot be saved * @returns 404 if form with formId or submissionId does not exist, or form is private - * @returns 409 if form feedback for the submissionId has already been submitted * @returns 410 if form has been archived + * @returns 422 if form feedback for the submissionId has already been submitted * @returns 500 if database error occurs */ PublicFormSubmissionsFeedbackRouter.route( From 62fd74556a4f27548796670eabc41c8304fa6a8c Mon Sep 17 00:00:00 2001 From: Hans Tirtaputra Date: Tue, 7 Jun 2022 14:44:18 +0800 Subject: [PATCH 09/23] refactor: rename formSubmissionId to submissionId --- shared/types/form/form_feedback.ts | 2 +- src/app/models/form_feedback.server.model.ts | 2 +- src/app/modules/feedback/__tests__/feedback.service.spec.ts | 4 ++-- src/app/modules/feedback/feedback.controller.ts | 2 +- src/app/modules/feedback/feedback.service.ts | 2 +- src/app/modules/form/public-form/public-form.service.ts | 6 +++--- .../public-forms.submissions.feedback.routes.spec.ts | 2 +- tests/unit/backend/helpers/jest-db.ts | 6 +++--- 8 files changed, 13 insertions(+), 13 deletions(-) diff --git a/shared/types/form/form_feedback.ts b/shared/types/form/form_feedback.ts index 5dbe11102d..c98d1fbed7 100644 --- a/shared/types/form/form_feedback.ts +++ b/shared/types/form/form_feedback.ts @@ -17,7 +17,7 @@ export type FormFeedbackBase = { formId: FormDto['_id'] created?: Date lastModified?: Date - formSubmissionId: string + submissionId: string } // Convert to serialized version. diff --git a/src/app/models/form_feedback.server.model.ts b/src/app/models/form_feedback.server.model.ts index 506dca7327..581521c7e0 100644 --- a/src/app/models/form_feedback.server.model.ts +++ b/src/app/models/form_feedback.server.model.ts @@ -15,7 +15,7 @@ const FormFeedbackSchema = new Schema( ref: FORM_SCHEMA_ID, required: true, }, - formSubmissionId: { + submissionId: { type: Schema.Types.ObjectId, ref: SUBMISSION_SCHEMA_ID, // TODO: Update to true once we fully migrate to /submissions/{submissionId}/feedback endpoint diff --git a/src/app/modules/feedback/__tests__/feedback.service.spec.ts b/src/app/modules/feedback/__tests__/feedback.service.spec.ts index b01dc7c123..6df69a49bb 100644 --- a/src/app/modules/feedback/__tests__/feedback.service.spec.ts +++ b/src/app/modules/feedback/__tests__/feedback.service.spec.ts @@ -262,7 +262,7 @@ describe('feedback.service', () => { await FormFeedback.create({ comment: `test feedback`, formId: MOCK_FORM_ID, - formSubmissionId: MOCK_SUBMISSION_ID, + submissionId: MOCK_SUBMISSION_ID, rating: 5, }) @@ -302,7 +302,7 @@ describe('feedback.service', () => { expect(existSpy).toHaveBeenCalledWith({ formId: MOCK_FORM_ID, - formSubmissionId: MOCK_SUBMISSION_ID, + submissionId: MOCK_SUBMISSION_ID, }) expect(actualResult.isErr()).toEqual(true) expect(actualResult._unsafeUnwrapErr()).toBeInstanceOf(DatabaseError) diff --git a/src/app/modules/feedback/feedback.controller.ts b/src/app/modules/feedback/feedback.controller.ts index 2842424f2c..742200335d 100644 --- a/src/app/modules/feedback/feedback.controller.ts +++ b/src/app/modules/feedback/feedback.controller.ts @@ -117,7 +117,7 @@ export const submitFormFeedback: ControllerHandler< return PublicFormService.insertFormFeedback({ formId: form._id, - formSubmissionId: submissionId, + submissionId: submissionId, rating, comment, }) diff --git a/src/app/modules/feedback/feedback.service.ts b/src/app/modules/feedback/feedback.service.ts index eaf1f9b64f..1c954bb595 100644 --- a/src/app/modules/feedback/feedback.service.ts +++ b/src/app/modules/feedback/feedback.service.ts @@ -119,7 +119,7 @@ export const checkHasPreviousFeedback = ( ResultAsync.fromPromise( FormFeedbackModel.exists({ formId: formId, - formSubmissionId: submissionId, + submissionId: submissionId, }), (error) => { logger.error({ diff --git a/src/app/modules/form/public-form/public-form.service.ts b/src/app/modules/form/public-form/public-form.service.ts index 8f54750537..fd41a82cd9 100644 --- a/src/app/modules/form/public-form/public-form.service.ts +++ b/src/app/modules/form/public-form/public-form.service.ts @@ -28,12 +28,12 @@ const logger = createLoggerWithLabel(module) */ export const insertFormFeedback = ({ formId, - formSubmissionId, + submissionId, rating, comment, }: { formId: string - formSubmissionId?: string + submissionId?: string rating: number comment?: string }): ResultAsync => { @@ -44,7 +44,7 @@ export const insertFormFeedback = ({ return ResultAsync.fromPromise( FormFeedbackModel.create({ formId, - formSubmissionId, + submissionId, rating, comment, }), diff --git a/src/app/routes/api/v3/forms/__tests__/public-forms.submissions.feedback.routes.spec.ts b/src/app/routes/api/v3/forms/__tests__/public-forms.submissions.feedback.routes.spec.ts index 8971ea24a6..01b88a971f 100644 --- a/src/app/routes/api/v3/forms/__tests__/public-forms.submissions.feedback.routes.spec.ts +++ b/src/app/routes/api/v3/forms/__tests__/public-forms.submissions.feedback.routes.spec.ts @@ -138,7 +138,7 @@ describe('public-form.submissions.feedback.routes', () => { }) await dbHandler.insertFormFeedback({ formId: form._id, - formSubmissionId: submission._id, + submissionId: submission._id, }) const expectedResp = JSON.parse( diff --git a/tests/unit/backend/helpers/jest-db.ts b/tests/unit/backend/helpers/jest-db.ts index be0621e6b2..d0111f15ef 100644 --- a/tests/unit/backend/helpers/jest-db.ts +++ b/tests/unit/backend/helpers/jest-db.ts @@ -301,12 +301,12 @@ const insertFormSubmission = async ({ const insertFormFeedback = async ({ formId, - formSubmissionId, + submissionId, rating = '5', comment = 'FormSG rocks!', }: { formId?: ObjectID - formSubmissionId?: ObjectID + submissionId?: ObjectID rating?: string comment?: string } = {}): Promise<{ @@ -317,7 +317,7 @@ const insertFormFeedback = async ({ formId, comment, rating, - formSubmissionId, + submissionId, }) return { From 3d87a522efbf8b72b566b162618d0f4282966cd6 Mon Sep 17 00:00:00 2001 From: Hans Tirtaputra Date: Tue, 7 Jun 2022 18:48:30 +0800 Subject: [PATCH 10/23] refactor: feedback controller submitFormFeedbackV2 --- .../__tests__/feedback.service.spec.ts | 21 +-- .../modules/feedback/feedback.controller.ts | 132 ++++++------------ src/app/modules/feedback/feedback.error.ts | 17 +++ src/app/modules/feedback/feedback.service.ts | 17 ++- src/app/modules/feedback/feedback.util.ts | 63 +++++++++ .../submission/submission.service.spec.ts | 11 +- .../modules/submission/submission.service.ts | 10 +- ...-forms.submissions.feedback.routes.spec.ts | 8 +- 8 files changed, 166 insertions(+), 113 deletions(-) create mode 100644 src/app/modules/feedback/feedback.error.ts create mode 100644 src/app/modules/feedback/feedback.util.ts diff --git a/src/app/modules/feedback/__tests__/feedback.service.spec.ts b/src/app/modules/feedback/__tests__/feedback.service.spec.ts index 6df69a49bb..1d7dd67232 100644 --- a/src/app/modules/feedback/__tests__/feedback.service.spec.ts +++ b/src/app/modules/feedback/__tests__/feedback.service.spec.ts @@ -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.error' import * as FeedbackService from '../feedback.service' const FormFeedback = getFormFeedbackModel(mongoose) @@ -249,7 +250,7 @@ describe('feedback.service', () => { }) }) - describe('checkHasPreviousFeedback', () => { + describe('hasNoPreviousFeedback', () => { const MOCK_FORM_ID = new ObjectId().toHexString() const MOCK_SUBMISSION_ID = new ObjectId().toHexString() @@ -258,7 +259,7 @@ describe('feedback.service', () => { }) afterEach(() => jest.clearAllMocks()) - it('should return true when previous feedback exists', async () => { + it('should return DuplicateFeedbackSubmissionError when feedback already exists for given submissionId', async () => { await FormFeedback.create({ comment: `test feedback`, formId: MOCK_FORM_ID, @@ -266,36 +267,38 @@ describe('feedback.service', () => { rating: 5, }) - const actualResult = await FeedbackService.checkHasPreviousFeedback( + const actualResult = await FeedbackService.hasNoPreviousFeedback( MOCK_FORM_ID, MOCK_SUBMISSION_ID, ) - expect(actualResult.isOk()).toEqual(true) - expect(actualResult._unsafeUnwrap()).toEqual(true) + expect(actualResult.isErr()).toEqual(true) + expect(actualResult._unsafeUnwrapErr()).toBeInstanceOf( + DuplicateFeedbackSubmissionError, + ) }) - it('should return false previous feedback does not exist', async () => { + 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.checkHasPreviousFeedback( + const actualResult = await FeedbackService.hasNoPreviousFeedback( MOCK_FORM_ID, new ObjectId().toHexString(), ) expect(actualResult.isOk()).toEqual(true) - expect(actualResult._unsafeUnwrap()).toEqual(false) + 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.checkHasPreviousFeedback( + const actualResult = await FeedbackService.hasNoPreviousFeedback( MOCK_FORM_ID, MOCK_SUBMISSION_ID, ) diff --git a/src/app/modules/feedback/feedback.controller.ts b/src/app/modules/feedback/feedback.controller.ts index 742200335d..9a043f43df 100644 --- a/src/app/modules/feedback/feedback.controller.ts +++ b/src/app/modules/feedback/feedback.controller.ts @@ -8,10 +8,10 @@ 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 { mapRouteError } from '../form/public-form/public-form.utils' import * as SubmissionService from '../submission/submission.service' import * as FeedbackService from './feedback.service' +import { mapRouteError } from './feedback.util' const logger = createLoggerWithLabel(module) @@ -25,7 +25,18 @@ const validateSubmitFormFeedbackParams = celebrate({ .unknown(true), }) -export const submitFormFeedback: ControllerHandler< +/** + * 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 + */ +const submitFormFeedbackV2: ControllerHandler< { formId: string; submissionId: string }, { message: string } | ErrorDto | PrivateFormErrorDto, { rating: number; comment: string } @@ -39,105 +50,44 @@ export const submitFormFeedback: ControllerHandler< submissionId, } - const checkDoesSubmissionIdExistRes = - await SubmissionService.checkDoesSubmissionIdExist(submissionId) - if (checkDoesSubmissionIdExistRes.isErr()) { - const { error } = checkDoesSubmissionIdExistRes - logger.error({ - message: 'Failed to check if submissionId exists', - meta: logMeta, - error, + return SubmissionService.checkDoesSubmissionIdExist(submissionId) + .andThen(() => FeedbackService.hasNoPreviousFeedback(formId, 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' }), + ) }) - const { errorMessage, statusCode } = mapRouteError(error) - return res.status(statusCode).json({ message: errorMessage }) - } - - const isSubmissionIdValid = checkDoesSubmissionIdExistRes.value - if (!isSubmissionIdValid) { - return res - .status(StatusCodes.NOT_FOUND) - .json({ message: 'SubmissionId is not valid' }) - } - - const checkHasPreviousFeedbackRes = - await FeedbackService.checkHasPreviousFeedback(formId, submissionId) - if (checkHasPreviousFeedbackRes.isErr()) { - const { error } = checkHasPreviousFeedbackRes - logger.error({ - message: - 'Failed to check if feedback has already been submitted previously', - meta: logMeta, - error, - }) - const { errorMessage, statusCode } = mapRouteError(error) - return res.status(statusCode).json({ message: errorMessage }) - } - - const hasPreviousFeedback = checkHasPreviousFeedbackRes.value - if (hasPreviousFeedback) { - return res - .status(StatusCodes.UNPROCESSABLE_ENTITY) - .json({ message: 'Multiple feedbacks has already been submitted' }) - } - - const formResult = await FormService.retrieveFullFormById(formId) - if (formResult.isErr()) { - const { error } = formResult - logger.error({ - message: 'Failed to retrieve form', - meta: logMeta, - error, - }) - const { errorMessage, statusCode } = mapRouteError(error) - return res.status(statusCode).json({ message: errorMessage }) - } - - const form = formResult.value - const isPublicResult = FormService.isFormPublic(form) - if (isPublicResult.isErr()) { - const { error } = isPublicResult - logger.error({ - message: 'Form is not public', - meta: logMeta, - error, - }) - const { errorMessage, statusCode } = mapRouteError(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 }) - } - - 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: 'Error creating form feedback', + message: errorMessage, meta: logMeta, error, }) - const { errorMessage, statusCode } = mapRouteError(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, - submitFormFeedback, + submitFormFeedbackV2, ] as ControllerHandler[] diff --git a/src/app/modules/feedback/feedback.error.ts b/src/app/modules/feedback/feedback.error.ts new file mode 100644 index 0000000000..785ba2d11d --- /dev/null +++ b/src/app/modules/feedback/feedback.error.ts @@ -0,0 +1,17 @@ +import { ApplicationError } from '../core/core.errors' + +export class InvalidSubmissionIdError extends ApplicationError { + constructor( + message = 'Feedback submissionId can not be found in the form submissions', + ) { + super(message) + } +} + +export class DuplicateFeedbackSubmissionError extends ApplicationError { + constructor( + message = 'A feedback with the same formId and submissionId already exists', + ) { + super(message) + } +} diff --git a/src/app/modules/feedback/feedback.service.ts b/src/app/modules/feedback/feedback.service.ts index 1c954bb595..4fd0e250b2 100644 --- a/src/app/modules/feedback/feedback.service.ts +++ b/src/app/modules/feedback/feedback.service.ts @@ -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, @@ -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.error' + const FormFeedbackModel = getFormFeedbackModel(mongoose) const logger = createLoggerWithLabel(module) @@ -112,10 +114,10 @@ export const getFormFeedbacks = ( }) } -export const checkHasPreviousFeedback = ( +export const hasNoPreviousFeedback = ( formId: string, submissionId: string, -): ResultAsync => +): ResultAsync => ResultAsync.fromPromise( FormFeedbackModel.exists({ formId: formId, @@ -125,7 +127,7 @@ export const checkHasPreviousFeedback = ( logger.error({ message: 'Error finding feedback documents from database', meta: { - action: 'checkHasPreviousFeedback', + action: 'hasNoPreviousFeedback', formId, submissionId, }, @@ -134,4 +136,9 @@ export const checkHasPreviousFeedback = ( return new DatabaseError(getMongoErrorMessage(error)) }, - ) + ).andThen((hasPreviousFeedback) => { + if (hasPreviousFeedback) { + return errAsync(new DuplicateFeedbackSubmissionError()) + } + return okAsync(true as const) + }) diff --git a/src/app/modules/feedback/feedback.util.ts b/src/app/modules/feedback/feedback.util.ts new file mode 100644 index 0000000000..3457d83227 --- /dev/null +++ b/src/app/modules/feedback/feedback.util.ts @@ -0,0 +1,63 @@ +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.error' + +const logger = createLoggerWithLabel(module) + +export const mapRouteError: MapRouteError = ( + error: ApplicationError, + coreErrorMessage?: string, +) => { + switch (error.constructor) { + case FormErrors.FormNotFoundError: + return { + statusCode: StatusCodes.NOT_FOUND, + errorMessage: error.message, + } + case FormErrors.FormDeletedError: + return { + statusCode: StatusCodes.GONE, + errorMessage: error.message, + } + case FormErrors.PrivateFormError: + return { + statusCode: StatusCodes.NOT_FOUND, + errorMessage: error.message, + } + 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: 'Something went wrong. Please refresh and try again.', + } + } +} diff --git a/src/app/modules/submission/__tests__/submission/submission.service.spec.ts b/src/app/modules/submission/__tests__/submission/submission.service.spec.ts index 388a60d5ef..8fc2a4b052 100644 --- a/src/app/modules/submission/__tests__/submission/submission.service.spec.ts +++ b/src/app/modules/submission/__tests__/submission/submission.service.spec.ts @@ -9,6 +9,7 @@ import { DatabaseError, MalformedParametersError, } from 'src/app/modules/core/core.errors' +import { InvalidSubmissionIdError } from 'src/app/modules/feedback/feedback.error' import * as SubmissionService from 'src/app/modules/submission/submission.service' import MailService from 'src/app/services/mail/mail.service' import { createQueryWithDateParam } from 'src/app/utils/date' @@ -730,7 +731,7 @@ describe('submission.service', () => { }) afterEach(() => jest.clearAllMocks()) - it('should return true with valid submissionId', async () => { + it('should return true if submissionId already exists', async () => { await Submission.create({ _id: MOCK_SUBMISSION_ID, submissionType: SubmissionType.Encrypt, @@ -749,7 +750,7 @@ describe('submission.service', () => { expect(actualResult._unsafeUnwrap()).toEqual(true) }) - it('should return false with invalid submissionId', async () => { + it('should return false if submissionId does not exist', async () => { await Submission.create({ _id: MOCK_SUBMISSION_ID, submissionType: SubmissionType.Encrypt, @@ -764,8 +765,10 @@ describe('submission.service', () => { new ObjectId().toHexString(), ) - expect(actualResult.isOk()).toEqual(true) - expect(actualResult._unsafeUnwrap()).toEqual(false) + expect(actualResult.isErr()).toEqual(true) + expect(actualResult._unsafeUnwrapErr()).toBeInstanceOf( + InvalidSubmissionIdError, + ) }) it('should return DatabaseError when error occurs whilst querying database', async () => { diff --git a/src/app/modules/submission/submission.service.ts b/src/app/modules/submission/submission.service.ts index cea0e7fb0c..e3c38ca945 100644 --- a/src/app/modules/submission/submission.service.ts +++ b/src/app/modules/submission/submission.service.ts @@ -14,6 +14,7 @@ import { AutoReplyMailData } from '../../services/mail/mail.types' import { createQueryWithDateParam, isMalformedDate } from '../../utils/date' import { getMongoErrorMessage } from '../../utils/handle-mongo-error' import { DatabaseError, MalformedParametersError } from '../core/core.errors' +import { InvalidSubmissionIdError } from '../feedback/feedback.error' import { SendEmailConfirmationError } from './submission.errors' @@ -136,7 +137,7 @@ export const sendEmailConfirmations = ({ export const checkDoesSubmissionIdExist = ( submissionId: string, -): ResultAsync => +): ResultAsync => ResultAsync.fromPromise( SubmissionModel.exists({ _id: submissionId, @@ -153,4 +154,9 @@ export const checkDoesSubmissionIdExist = ( return new DatabaseError(getMongoErrorMessage(error)) }, - ) + ).andThen((hasSubmissionId) => { + if (!hasSubmissionId) { + return errAsync(new InvalidSubmissionIdError()) + } + return okAsync(true as const) + }) diff --git a/src/app/routes/api/v3/forms/__tests__/public-forms.submissions.feedback.routes.spec.ts b/src/app/routes/api/v3/forms/__tests__/public-forms.submissions.feedback.routes.spec.ts index 01b88a971f..6aede093c5 100644 --- a/src/app/routes/api/v3/forms/__tests__/public-forms.submissions.feedback.routes.spec.ts +++ b/src/app/routes/api/v3/forms/__tests__/public-forms.submissions.feedback.routes.spec.ts @@ -3,6 +3,10 @@ import { errAsync } from 'neverthrow' import supertest, { Session } from 'supertest-session' import { DatabaseError } from 'src/app/modules/core/core.errors' +import { + DuplicateFeedbackSubmissionError, + InvalidSubmissionIdError, +} from 'src/app/modules/feedback/feedback.error' import { setupApp } from 'tests/integration/helpers/express-setup' import { buildCelebrateError } from 'tests/unit/backend/helpers/celebrate' @@ -111,7 +115,7 @@ describe('public-form.submissions.feedback.routes', () => { const { form } = await dbHandler.insertEmailForm() const expectedResp = JSON.parse( JSON.stringify({ - message: 'SubmissionId is not valid', + message: new InvalidSubmissionIdError().message, }), ) @@ -143,7 +147,7 @@ describe('public-form.submissions.feedback.routes', () => { const expectedResp = JSON.parse( JSON.stringify({ - message: 'Multiple feedbacks has already been submitted', + message: new DuplicateFeedbackSubmissionError().message, }), ) const actualResp = await request From 4c802f54031dfba44bea2f6d6c6df6e5778bab2f Mon Sep 17 00:00:00 2001 From: Hans Tirtaputra Date: Tue, 7 Jun 2022 19:16:08 +0800 Subject: [PATCH 11/23] fix: update according to PR comments --- shared/types/form/form_feedback.ts | 3 ++- src/app/models/form_feedback.server.model.ts | 1 + src/app/modules/feedback/feedback.controller.ts | 2 +- src/app/modules/feedback/feedback.error.ts | 4 ++-- src/app/modules/feedback/feedback.service.ts | 12 ++++++++++++ .../form/public-form/public-form.controller.ts | 10 +--------- .../__tests__/submission/submission.service.spec.ts | 8 ++++---- src/app/modules/submission/submission.service.ts | 13 +++++++++++-- 8 files changed, 34 insertions(+), 19 deletions(-) diff --git a/shared/types/form/form_feedback.ts b/shared/types/form/form_feedback.ts index c98d1fbed7..e37dd449df 100644 --- a/shared/types/form/form_feedback.ts +++ b/shared/types/form/form_feedback.ts @@ -1,5 +1,6 @@ import { Merge } from 'type-fest' import { DateString } from '../generic' +import { SubmissionResponseDto } from '../submission' import { FormDto } from './form' export type SubmitFormFeedbackBodyDto = { @@ -17,7 +18,7 @@ export type FormFeedbackBase = { formId: FormDto['_id'] created?: Date lastModified?: Date - submissionId: string + submissionId: SubmissionResponseDto['submissionId'] } // Convert to serialized version. diff --git a/src/app/models/form_feedback.server.model.ts b/src/app/models/form_feedback.server.model.ts index 581521c7e0..e134cba27d 100644 --- a/src/app/models/form_feedback.server.model.ts +++ b/src/app/models/form_feedback.server.model.ts @@ -20,6 +20,7 @@ const FormFeedbackSchema = new Schema( ref: SUBMISSION_SCHEMA_ID, // TODO: Update to true once we fully migrate to /submissions/{submissionId}/feedback endpoint required: false, + index: true, }, rating: { type: Number, diff --git a/src/app/modules/feedback/feedback.controller.ts b/src/app/modules/feedback/feedback.controller.ts index 9a043f43df..5a90fde70b 100644 --- a/src/app/modules/feedback/feedback.controller.ts +++ b/src/app/modules/feedback/feedback.controller.ts @@ -50,7 +50,7 @@ const submitFormFeedbackV2: ControllerHandler< submissionId, } - return SubmissionService.checkDoesSubmissionIdExist(submissionId) + return SubmissionService.doesSubmissionIdExist(submissionId) .andThen(() => FeedbackService.hasNoPreviousFeedback(formId, submissionId)) .andThen(() => FormService.retrieveFullFormById(formId)) .andThen((form) => FormService.isFormPublic(form).map(() => form)) diff --git a/src/app/modules/feedback/feedback.error.ts b/src/app/modules/feedback/feedback.error.ts index 785ba2d11d..37a2ea4fac 100644 --- a/src/app/modules/feedback/feedback.error.ts +++ b/src/app/modules/feedback/feedback.error.ts @@ -2,7 +2,7 @@ import { ApplicationError } from '../core/core.errors' export class InvalidSubmissionIdError extends ApplicationError { constructor( - message = 'Feedback submissionId can not be found in the form submissions', + message = 'Feedback submissionId can not be found in the form submissions. Please submit a form before you submit a feedback.', ) { super(message) } @@ -10,7 +10,7 @@ export class InvalidSubmissionIdError extends ApplicationError { export class DuplicateFeedbackSubmissionError extends ApplicationError { constructor( - message = 'A feedback with the same formId and submissionId already exists', + message = 'You have already submitted feedback for this submission. To provide additional feedback, please contact the form administrator.', ) { super(message) } diff --git a/src/app/modules/feedback/feedback.service.ts b/src/app/modules/feedback/feedback.service.ts index 4fd0e250b2..98f0508f6b 100644 --- a/src/app/modules/feedback/feedback.service.ts +++ b/src/app/modules/feedback/feedback.service.ts @@ -114,6 +114,18 @@ 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 = ( formId: string, submissionId: string, diff --git a/src/app/modules/form/public-form/public-form.controller.ts b/src/app/modules/form/public-form/public-form.controller.ts index 5a813ba488..690f2222a3 100644 --- a/src/app/modules/form/public-form/public-form.controller.ts +++ b/src/app/modules/form/public-form/public-form.controller.ts @@ -64,15 +64,7 @@ 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. - * - * @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 + * @deprecated use submitFormFeedbackV2 instead */ export const submitFormFeedback: ControllerHandler< { formId: string }, diff --git a/src/app/modules/submission/__tests__/submission/submission.service.spec.ts b/src/app/modules/submission/__tests__/submission/submission.service.spec.ts index 8fc2a4b052..2d481adea4 100644 --- a/src/app/modules/submission/__tests__/submission/submission.service.spec.ts +++ b/src/app/modules/submission/__tests__/submission/submission.service.spec.ts @@ -723,7 +723,7 @@ describe('submission.service', () => { }) }) - describe('checkDoesSubmissionIdExist', () => { + describe('doesSubmissionIdExist', () => { const MOCK_SUBMISSION_ID = MOCK_SUBMISSION._id beforeEach(async () => { @@ -742,7 +742,7 @@ describe('submission.service', () => { responseSalt: 'salt', }) - const actualResult = await SubmissionService.checkDoesSubmissionIdExist( + const actualResult = await SubmissionService.doesSubmissionIdExist( MOCK_SUBMISSION_ID, ) @@ -761,7 +761,7 @@ describe('submission.service', () => { responseSalt: 'salt', }) - const actualResult = await SubmissionService.checkDoesSubmissionIdExist( + const actualResult = await SubmissionService.doesSubmissionIdExist( new ObjectId().toHexString(), ) @@ -775,7 +775,7 @@ describe('submission.service', () => { const existSpy = jest.spyOn(Submission, 'exists') existSpy.mockImplementationOnce(() => Promise.reject(new Error('boom'))) - const actualResult = await SubmissionService.checkDoesSubmissionIdExist( + const actualResult = await SubmissionService.doesSubmissionIdExist( MOCK_SUBMISSION_ID, ) diff --git a/src/app/modules/submission/submission.service.ts b/src/app/modules/submission/submission.service.ts index e3c38ca945..2995e3ec33 100644 --- a/src/app/modules/submission/submission.service.ts +++ b/src/app/modules/submission/submission.service.ts @@ -135,7 +135,16 @@ export const sendEmailConfirmations = ({ }) } -export const checkDoesSubmissionIdExist = ( +/** + * Checks if submissionId exists amongst all the form submissions. + * + * @param submissionId the submission id to find amongst all the form submissions + * + * @returns ok(true) if submission id exists amongst all the form submissions + * @returns err(InvalidSubmissionIdError) if submissionId does not exist amongst all the form submissions + * @returns err(DatabaseError) if database query errors + */ +export const doesSubmissionIdExist = ( submissionId: string, ): ResultAsync => ResultAsync.fromPromise( @@ -146,7 +155,7 @@ export const checkDoesSubmissionIdExist = ( logger.error({ message: 'Error finding _id from submissions collection in database', meta: { - action: 'checkDoesSubmissionIdExist', + action: 'doesSubmissionIdExist', submissionId, }, error, From 05151d2590e86f1ed4af64d66ae2b4ba4686d71a Mon Sep 17 00:00:00 2001 From: Hans Tirtaputra Date: Tue, 7 Jun 2022 19:22:07 +0800 Subject: [PATCH 12/23] refactor: move submission.feedback.routes tests to feedback.routes --- .../public-forms.feedback.routes.spec.ts | 213 +++++++++++++++++ ...-forms.submissions.feedback.routes.spec.ts | 223 ------------------ 2 files changed, 213 insertions(+), 223 deletions(-) delete mode 100644 src/app/routes/api/v3/forms/__tests__/public-forms.submissions.feedback.routes.spec.ts diff --git a/src/app/routes/api/v3/forms/__tests__/public-forms.feedback.routes.spec.ts b/src/app/routes/api/v3/forms/__tests__/public-forms.feedback.routes.spec.ts index b9eb8c34c2..2f41949896 100644 --- a/src/app/routes/api/v3/forms/__tests__/public-forms.feedback.routes.spec.ts +++ b/src/app/routes/api/v3/forms/__tests__/public-forms.feedback.routes.spec.ts @@ -1,7 +1,12 @@ +import { ObjectId } from 'mongodb' import { errAsync } from 'neverthrow' import supertest, { Session } from 'supertest-session' import { DatabaseError } from 'src/app/modules/core/core.errors' +import { + DuplicateFeedbackSubmissionError, + InvalidSubmissionIdError, +} from 'src/app/modules/feedback/feedback.error' import { setupApp } from 'tests/integration/helpers/express-setup' import { buildCelebrateError } from 'tests/unit/backend/helpers/celebrate' @@ -16,6 +21,10 @@ const app = setupApp('/forms', PublicFormsRouter) // Avoid async refresh calls jest.mock('src/app/modules/spcp/sp.oidc.client.ts') +/** + * TODO: Remove 'public-form.feedback.routes' test, and keep 'public-form.submissions.feedback.routes' + * once `/api/v3/forms/{formId}/feedback` route is cleaned up + */ describe('public-form.feedback.routes', () => { let request: Session @@ -160,3 +169,207 @@ describe('public-form.feedback.routes', () => { }) }) }) + +describe('public-form.submissions.feedback.routes', () => { + let request: Session + + beforeAll(async () => await dbHandler.connect()) + beforeEach(async () => { + request = supertest(app) + }) + afterEach(async () => { + await dbHandler.clearDatabase() + jest.restoreAllMocks() + }) + afterAll(async () => await dbHandler.closeDatabase()) + describe('POST /forms/:formId/submissions/:submissionId/feedback', () => { + it('should return 200 when feedback was successfully saved', async () => { + const MOCK_FEEDBACK = { + rating: 5, + comment: 'great mock', + } + + const { form } = await dbHandler.insertEmailForm({ + formOptions: { + status: FormStatus.Public, + }, + }) + const { submission } = await dbHandler.insertFormSubmission({ + formId: form._id, + }) + + const expectedResp = JSON.parse( + JSON.stringify({ message: 'Successfully submitted feedback' }), + ) + const actualResp = await request + .post(`/forms/${form._id}/submissions/${submission._id}/feedback`) + .send(MOCK_FEEDBACK) + + expect(actualResp.status).toEqual(200) + expect(actualResp.body).toEqual(expectedResp) + }) + + it('should return 400 when form feedback submitted is malformed', async () => { + const MOCK_FEEDBACK = { rating: 6 } + + const { form } = await dbHandler.insertEmailForm() + const { submission } = await dbHandler.insertFormSubmission({ + formId: form._id, + }) + + const actualResp = await request + .post(`/forms/${form._id}/submissions/${submission._id}/feedback`) + .send(MOCK_FEEDBACK) + + expect(actualResp.status).toEqual(400) + expect(actualResp.body).toEqual( + buildCelebrateError({ + body: { + key: 'rating', + message: '"rating" must be less than or equal to 5', + }, + }), + ) + }) + + it('should return 404 when form is private', async () => { + const MOCK_FEEDBACK = { + rating: 5, + comment: 'great mock', + } + + const { form } = await dbHandler.insertEmailForm() + const { submission } = await dbHandler.insertFormSubmission({ + formId: form._id, + }) + const expectedResp = JSON.parse( + JSON.stringify({ + message: form.inactiveMessage, + formTitle: form.title, + isPageFound: true, + }), + ) + + const actualResp = await request + .post(`/forms/${form._id}/submissions/${submission._id}/feedback`) + .send(MOCK_FEEDBACK) + + expect(actualResp.status).toEqual(404) + expect(actualResp.body).toEqual(expectedResp) + }) + + it('should return 404 when submissionId does not exist', async () => { + const MOCK_FEEDBACK = { + rating: 5, + comment: 'great mock', + } + + const { form } = await dbHandler.insertEmailForm() + const expectedResp = JSON.parse( + JSON.stringify({ + message: new InvalidSubmissionIdError().message, + }), + ) + + const INVALID_SUBMISSION_ID = new ObjectId().toHexString() + const actualResp = await request + .post( + `/forms/${form._id}/submissions/${INVALID_SUBMISSION_ID}/feedback`, + ) + .send(MOCK_FEEDBACK) + + expect(actualResp.status).toEqual(404) + expect(actualResp.body).toEqual(expectedResp) + }) + + it('should return 422 when form feedback for the submissionId has already been submitted', async () => { + const MOCK_FEEDBACK = { + rating: 5, + comment: 'great mock', + } + + const { form } = await dbHandler.insertEmailForm() + const { submission } = await dbHandler.insertFormSubmission({ + formId: form._id, + }) + await dbHandler.insertFormFeedback({ + formId: form._id, + submissionId: submission._id, + }) + + const expectedResp = JSON.parse( + JSON.stringify({ + message: new DuplicateFeedbackSubmissionError().message, + }), + ) + const actualResp = await request + .post(`/forms/${form._id}/submissions/${submission._id}/feedback`) + .send(MOCK_FEEDBACK) + + expect(actualResp.status).toEqual(422) + expect(actualResp.body).toEqual(expectedResp) + }) + + it('should return 410 when form is archived', async () => { + const MOCK_FEEDBACK = { + rating: 5, + comment: 'great mock', + } + + const { form } = await dbHandler.insertEmailForm({ + formOptions: { + status: FormStatus.Archived, + }, + }) + const { submission } = await dbHandler.insertFormSubmission({ + formId: form._id, + }) + + const expectedResp = JSON.parse( + JSON.stringify({ + message: 'This form is no longer active', + }), + ) + + const actualResp = await request + .post(`/forms/${form._id}/submissions/${submission._id}/feedback`) + .send(MOCK_FEEDBACK) + + expect(actualResp.status).toEqual(410) + expect(actualResp.body).toEqual(expectedResp) + }) + + it('should return 500 when form could not be retrieved due to a database error', async () => { + const MOCK_ERROR_MESSAGE = 'mock me' + const MOCK_FEEDBACK = { + rating: 5, + comment: 'great mock', + } + + const { form } = await dbHandler.insertEmailForm({ + formOptions: { + status: FormStatus.Public, + }, + }) + const { submission } = await dbHandler.insertFormSubmission({ + formId: form._id, + }) + + const expectedResp = JSON.parse( + JSON.stringify({ + message: MOCK_ERROR_MESSAGE, + }), + ) + jest + .spyOn(FormService, 'retrieveFullFormById') + .mockReturnValueOnce(errAsync(new DatabaseError(MOCK_ERROR_MESSAGE))) + + const actualResp = await request + .post(`/forms/${form._id}/submissions/${submission._id}/feedback`) + .send(MOCK_FEEDBACK) + + expect(actualResp.status).toEqual(500) + expect(actualResp.body).toEqual(expectedResp) + }) + }) +}) diff --git a/src/app/routes/api/v3/forms/__tests__/public-forms.submissions.feedback.routes.spec.ts b/src/app/routes/api/v3/forms/__tests__/public-forms.submissions.feedback.routes.spec.ts deleted file mode 100644 index 6aede093c5..0000000000 --- a/src/app/routes/api/v3/forms/__tests__/public-forms.submissions.feedback.routes.spec.ts +++ /dev/null @@ -1,223 +0,0 @@ -import { ObjectId } from 'mongodb' -import { errAsync } from 'neverthrow' -import supertest, { Session } from 'supertest-session' - -import { DatabaseError } from 'src/app/modules/core/core.errors' -import { - DuplicateFeedbackSubmissionError, - InvalidSubmissionIdError, -} from 'src/app/modules/feedback/feedback.error' - -import { setupApp } from 'tests/integration/helpers/express-setup' -import { buildCelebrateError } from 'tests/unit/backend/helpers/celebrate' -import dbHandler from 'tests/unit/backend/helpers/jest-db' - -import { FormStatus } from '../../../../../../../shared/types' -import * as FormService from '../../../../../modules/form/form.service' -import { PublicFormsRouter } from '../public-forms.routes' - -const app = setupApp('/forms', PublicFormsRouter) - -describe('public-form.submissions.feedback.routes', () => { - let request: Session - - beforeAll(async () => await dbHandler.connect()) - beforeEach(async () => { - request = supertest(app) - }) - afterEach(async () => { - await dbHandler.clearDatabase() - jest.restoreAllMocks() - }) - afterAll(async () => await dbHandler.closeDatabase()) - describe('POST /forms/:formId/submissions/:submissionId/feedback', () => { - it('should return 200 when feedback was successfully saved', async () => { - const MOCK_FEEDBACK = { - rating: 5, - comment: 'great mock', - } - - const { form } = await dbHandler.insertEmailForm({ - formOptions: { - status: FormStatus.Public, - }, - }) - const { submission } = await dbHandler.insertFormSubmission({ - formId: form._id, - }) - - const expectedResp = JSON.parse( - JSON.stringify({ message: 'Successfully submitted feedback' }), - ) - const actualResp = await request - .post(`/forms/${form._id}/submissions/${submission._id}/feedback`) - .send(MOCK_FEEDBACK) - - expect(actualResp.status).toEqual(200) - expect(actualResp.body).toEqual(expectedResp) - }) - - it('should return 400 when form feedback submitted is malformed', async () => { - const MOCK_FEEDBACK = { rating: 6 } - - const { form } = await dbHandler.insertEmailForm() - const { submission } = await dbHandler.insertFormSubmission({ - formId: form._id, - }) - - const actualResp = await request - .post(`/forms/${form._id}/submissions/${submission._id}/feedback`) - .send(MOCK_FEEDBACK) - - expect(actualResp.status).toEqual(400) - expect(actualResp.body).toEqual( - buildCelebrateError({ - body: { - key: 'rating', - message: '"rating" must be less than or equal to 5', - }, - }), - ) - }) - - it('should return 404 when form is private', async () => { - const MOCK_FEEDBACK = { - rating: 5, - comment: 'great mock', - } - - const { form } = await dbHandler.insertEmailForm() - const { submission } = await dbHandler.insertFormSubmission({ - formId: form._id, - }) - const expectedResp = JSON.parse( - JSON.stringify({ - message: form.inactiveMessage, - formTitle: form.title, - isPageFound: true, - }), - ) - - const actualResp = await request - .post(`/forms/${form._id}/submissions/${submission._id}/feedback`) - .send(MOCK_FEEDBACK) - - expect(actualResp.status).toEqual(404) - expect(actualResp.body).toEqual(expectedResp) - }) - - it('should return 404 when submissionId does not exist', async () => { - const MOCK_FEEDBACK = { - rating: 5, - comment: 'great mock', - } - - const { form } = await dbHandler.insertEmailForm() - const expectedResp = JSON.parse( - JSON.stringify({ - message: new InvalidSubmissionIdError().message, - }), - ) - - const INVALID_SUBMISSION_ID = new ObjectId().toHexString() - const actualResp = await request - .post( - `/forms/${form._id}/submissions/${INVALID_SUBMISSION_ID}/feedback`, - ) - .send(MOCK_FEEDBACK) - - expect(actualResp.status).toEqual(404) - expect(actualResp.body).toEqual(expectedResp) - }) - - it('should return 422 when form feedback for the submissionId has already been submitted', async () => { - const MOCK_FEEDBACK = { - rating: 5, - comment: 'great mock', - } - - const { form } = await dbHandler.insertEmailForm() - const { submission } = await dbHandler.insertFormSubmission({ - formId: form._id, - }) - await dbHandler.insertFormFeedback({ - formId: form._id, - submissionId: submission._id, - }) - - const expectedResp = JSON.parse( - JSON.stringify({ - message: new DuplicateFeedbackSubmissionError().message, - }), - ) - const actualResp = await request - .post(`/forms/${form._id}/submissions/${submission._id}/feedback`) - .send(MOCK_FEEDBACK) - - expect(actualResp.status).toEqual(422) - expect(actualResp.body).toEqual(expectedResp) - }) - - it('should return 410 when form is archived', async () => { - const MOCK_FEEDBACK = { - rating: 5, - comment: 'great mock', - } - - const { form } = await dbHandler.insertEmailForm({ - formOptions: { - status: FormStatus.Archived, - }, - }) - const { submission } = await dbHandler.insertFormSubmission({ - formId: form._id, - }) - - const expectedResp = JSON.parse( - JSON.stringify({ - message: 'This form is no longer active', - }), - ) - - const actualResp = await request - .post(`/forms/${form._id}/submissions/${submission._id}/feedback`) - .send(MOCK_FEEDBACK) - - expect(actualResp.status).toEqual(410) - expect(actualResp.body).toEqual(expectedResp) - }) - - it('should return 500 when form could not be retrieved due to a database error', async () => { - const MOCK_ERROR_MESSAGE = 'mock me' - const MOCK_FEEDBACK = { - rating: 5, - comment: 'great mock', - } - - const { form } = await dbHandler.insertEmailForm({ - formOptions: { - status: FormStatus.Public, - }, - }) - const { submission } = await dbHandler.insertFormSubmission({ - formId: form._id, - }) - - const expectedResp = JSON.parse( - JSON.stringify({ - message: MOCK_ERROR_MESSAGE, - }), - ) - jest - .spyOn(FormService, 'retrieveFullFormById') - .mockReturnValueOnce(errAsync(new DatabaseError(MOCK_ERROR_MESSAGE))) - - const actualResp = await request - .post(`/forms/${form._id}/submissions/${submission._id}/feedback`) - .send(MOCK_FEEDBACK) - - expect(actualResp.status).toEqual(500) - expect(actualResp.body).toEqual(expectedResp) - }) - }) -}) From 32db644a5c58cb1913aa1195434f000783123937 Mon Sep 17 00:00:00 2001 From: Hans Tirtaputra Date: Tue, 7 Jun 2022 19:29:38 +0800 Subject: [PATCH 13/23] chore: update TODO comments to link issue number --- src/app/models/form_feedback.server.model.ts | 2 +- .../api/v3/forms/__tests__/public-forms.feedback.routes.spec.ts | 2 +- src/app/routes/api/v3/forms/public-forms.routes.ts | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/app/models/form_feedback.server.model.ts b/src/app/models/form_feedback.server.model.ts index e134cba27d..62f1612db3 100644 --- a/src/app/models/form_feedback.server.model.ts +++ b/src/app/models/form_feedback.server.model.ts @@ -18,7 +18,7 @@ const FormFeedbackSchema = new Schema( submissionId: { type: Schema.Types.ObjectId, ref: SUBMISSION_SCHEMA_ID, - // TODO: Update to true once we fully migrate to /submissions/{submissionId}/feedback endpoint + // TODO #3964: Update to true once we fully migrate to /submissions/{submissionId}/feedback endpoint required: false, index: true, }, diff --git a/src/app/routes/api/v3/forms/__tests__/public-forms.feedback.routes.spec.ts b/src/app/routes/api/v3/forms/__tests__/public-forms.feedback.routes.spec.ts index 2f41949896..e18cede60f 100644 --- a/src/app/routes/api/v3/forms/__tests__/public-forms.feedback.routes.spec.ts +++ b/src/app/routes/api/v3/forms/__tests__/public-forms.feedback.routes.spec.ts @@ -22,7 +22,7 @@ const app = setupApp('/forms', PublicFormsRouter) jest.mock('src/app/modules/spcp/sp.oidc.client.ts') /** - * TODO: Remove 'public-form.feedback.routes' test, and keep 'public-form.submissions.feedback.routes' + * TODO #3964: Remove 'public-form.feedback.routes' test, and keep 'public-form.submissions.feedback.routes' * once `/api/v3/forms/{formId}/feedback` route is cleaned up */ describe('public-form.feedback.routes', () => { diff --git a/src/app/routes/api/v3/forms/public-forms.routes.ts b/src/app/routes/api/v3/forms/public-forms.routes.ts index 5af996898e..98459a3bb7 100644 --- a/src/app/routes/api/v3/forms/public-forms.routes.ts +++ b/src/app/routes/api/v3/forms/public-forms.routes.ts @@ -14,5 +14,5 @@ PublicFormsRouter.use(PublicFormsFeedbackRouter) PublicFormsRouter.use(PublicFormsFormRouter) PublicFormsRouter.use(PublicFormsAuthRouter) PublicFormsRouter.use(PublicFormsVerificationRouter) -// TODO: Cleanup PublicFormsFeedbackRouter once it's fully migrated to PublicFormSubmissionsFeedbackRouter +// TODO #3964: Cleanup PublicFormsFeedbackRouter once it's fully migrated to PublicFormSubmissionsFeedbackRouter PublicFormsRouter.use(PublicFormSubmissionsFeedbackRouter) From e20cd53e4354534f890cc23d4d0653871d0d6983 Mon Sep 17 00:00:00 2001 From: Hans Tirtaputra Date: Mon, 13 Jun 2022 18:23:37 +0800 Subject: [PATCH 14/23] refactor: rename feedback.error file and some constants --- src/app/modules/feedback/__tests__/feedback.service.spec.ts | 2 +- .../feedback/{feedback.error.ts => feedback.errors.ts} | 0 src/app/modules/feedback/feedback.service.ts | 2 +- src/app/modules/feedback/feedback.util.ts | 2 +- .../__tests__/submission/submission.service.spec.ts | 2 +- src/app/modules/submission/submission.service.ts | 2 +- .../v3/forms/__tests__/public-forms.feedback.routes.spec.ts | 2 +- tests/unit/backend/helpers/jest-db.ts | 4 ++-- 8 files changed, 8 insertions(+), 8 deletions(-) rename src/app/modules/feedback/{feedback.error.ts => feedback.errors.ts} (100%) diff --git a/src/app/modules/feedback/__tests__/feedback.service.spec.ts b/src/app/modules/feedback/__tests__/feedback.service.spec.ts index 1d7dd67232..e0d5622c72 100644 --- a/src/app/modules/feedback/__tests__/feedback.service.spec.ts +++ b/src/app/modules/feedback/__tests__/feedback.service.spec.ts @@ -10,7 +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.error' +import { DuplicateFeedbackSubmissionError } from '../feedback.errors' import * as FeedbackService from '../feedback.service' const FormFeedback = getFormFeedbackModel(mongoose) diff --git a/src/app/modules/feedback/feedback.error.ts b/src/app/modules/feedback/feedback.errors.ts similarity index 100% rename from src/app/modules/feedback/feedback.error.ts rename to src/app/modules/feedback/feedback.errors.ts diff --git a/src/app/modules/feedback/feedback.service.ts b/src/app/modules/feedback/feedback.service.ts index 98f0508f6b..eb33e06fdc 100644 --- a/src/app/modules/feedback/feedback.service.ts +++ b/src/app/modules/feedback/feedback.service.ts @@ -13,7 +13,7 @@ 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.error' +import { DuplicateFeedbackSubmissionError } from './feedback.errors' const FormFeedbackModel = getFormFeedbackModel(mongoose) const logger = createLoggerWithLabel(module) diff --git a/src/app/modules/feedback/feedback.util.ts b/src/app/modules/feedback/feedback.util.ts index 3457d83227..c2149f7d90 100644 --- a/src/app/modules/feedback/feedback.util.ts +++ b/src/app/modules/feedback/feedback.util.ts @@ -8,7 +8,7 @@ import * as FormErrors from '../form/form.errors' import { DuplicateFeedbackSubmissionError, InvalidSubmissionIdError, -} from './feedback.error' +} from './feedback.errors' const logger = createLoggerWithLabel(module) diff --git a/src/app/modules/submission/__tests__/submission/submission.service.spec.ts b/src/app/modules/submission/__tests__/submission/submission.service.spec.ts index 2d481adea4..2b6e5bfa37 100644 --- a/src/app/modules/submission/__tests__/submission/submission.service.spec.ts +++ b/src/app/modules/submission/__tests__/submission/submission.service.spec.ts @@ -9,7 +9,7 @@ import { DatabaseError, MalformedParametersError, } from 'src/app/modules/core/core.errors' -import { InvalidSubmissionIdError } from 'src/app/modules/feedback/feedback.error' +import { InvalidSubmissionIdError } from 'src/app/modules/feedback/feedback.errors' import * as SubmissionService from 'src/app/modules/submission/submission.service' import MailService from 'src/app/services/mail/mail.service' import { createQueryWithDateParam } from 'src/app/utils/date' diff --git a/src/app/modules/submission/submission.service.ts b/src/app/modules/submission/submission.service.ts index 2995e3ec33..0dbd89a890 100644 --- a/src/app/modules/submission/submission.service.ts +++ b/src/app/modules/submission/submission.service.ts @@ -14,7 +14,7 @@ import { AutoReplyMailData } from '../../services/mail/mail.types' import { createQueryWithDateParam, isMalformedDate } from '../../utils/date' import { getMongoErrorMessage } from '../../utils/handle-mongo-error' import { DatabaseError, MalformedParametersError } from '../core/core.errors' -import { InvalidSubmissionIdError } from '../feedback/feedback.error' +import { InvalidSubmissionIdError } from '../feedback/feedback.errors' import { SendEmailConfirmationError } from './submission.errors' diff --git a/src/app/routes/api/v3/forms/__tests__/public-forms.feedback.routes.spec.ts b/src/app/routes/api/v3/forms/__tests__/public-forms.feedback.routes.spec.ts index e18cede60f..ebe6393377 100644 --- a/src/app/routes/api/v3/forms/__tests__/public-forms.feedback.routes.spec.ts +++ b/src/app/routes/api/v3/forms/__tests__/public-forms.feedback.routes.spec.ts @@ -6,7 +6,7 @@ import { DatabaseError } from 'src/app/modules/core/core.errors' import { DuplicateFeedbackSubmissionError, InvalidSubmissionIdError, -} from 'src/app/modules/feedback/feedback.error' +} from 'src/app/modules/feedback/feedback.errors' import { setupApp } from 'tests/integration/helpers/express-setup' import { buildCelebrateError } from 'tests/unit/backend/helpers/celebrate' diff --git a/tests/unit/backend/helpers/jest-db.ts b/tests/unit/backend/helpers/jest-db.ts index d0111f15ef..d96f5e0d1a 100644 --- a/tests/unit/backend/helpers/jest-db.ts +++ b/tests/unit/backend/helpers/jest-db.ts @@ -286,8 +286,8 @@ const insertFormSubmission = async ({ } = {}): Promise<{ submission: ISubmissionSchema }> => { - const SubmissionFormModel = getSubmissionModel(mongoose) - const submission = await SubmissionFormModel.create({ + const SubmissionModel = getSubmissionModel(mongoose) + const submission = await SubmissionModel.create({ form: formId, submissionType, encryptedContent, From 9dc9969d59d73f42a0d71aad62a4d025038cf38c Mon Sep 17 00:00:00 2001 From: Hans Tirtaputra Date: Mon, 13 Jun 2022 18:29:09 +0800 Subject: [PATCH 15/23] chore: add TODO comments linking to issue #3964 --- src/app/modules/feedback/feedback.controller.ts | 2 ++ src/app/modules/form/public-form/public-form.controller.ts | 2 ++ 2 files changed, 4 insertions(+) diff --git a/src/app/modules/feedback/feedback.controller.ts b/src/app/modules/feedback/feedback.controller.ts index 5a90fde70b..af1df1cf58 100644 --- a/src/app/modules/feedback/feedback.controller.ts +++ b/src/app/modules/feedback/feedback.controller.ts @@ -35,6 +35,8 @@ const validateSubmitFormFeedbackParams = celebrate({ * @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< { formId: string; submissionId: string }, diff --git a/src/app/modules/form/public-form/public-form.controller.ts b/src/app/modules/form/public-form/public-form.controller.ts index 690f2222a3..e28454f831 100644 --- a/src/app/modules/form/public-form/public-form.controller.ts +++ b/src/app/modules/form/public-form/public-form.controller.ts @@ -65,6 +65,8 @@ const validateSubmitFormFeedbackParams = celebrate({ /** * @deprecated use submitFormFeedbackV2 instead + * + * TODO #3964: Cleanup we fully migrate feedback endpoint to /submissions/{submissionId}/feedback */ export const submitFormFeedback: ControllerHandler< { formId: string }, From 7bd48f1660f166e7315afba7a50120c9025ba0d0 Mon Sep 17 00:00:00 2001 From: Hans Tirtaputra Date: Mon, 13 Jun 2022 19:42:39 +0800 Subject: [PATCH 16/23] fix: update feedback related error messages --- src/app/modules/feedback/feedback.controller.ts | 2 +- src/app/modules/feedback/feedback.errors.ts | 4 +--- src/app/modules/feedback/feedback.util.ts | 13 ++++++++----- .../__tests__/public-forms.feedback.routes.spec.ts | 6 ++++-- 4 files changed, 14 insertions(+), 11 deletions(-) diff --git a/src/app/modules/feedback/feedback.controller.ts b/src/app/modules/feedback/feedback.controller.ts index af1df1cf58..3a9a8ab529 100644 --- a/src/app/modules/feedback/feedback.controller.ts +++ b/src/app/modules/feedback/feedback.controller.ts @@ -46,7 +46,7 @@ const submitFormFeedbackV2: ControllerHandler< const { formId, submissionId } = req.params const { rating, comment } = req.body const logMeta = { - action: 'submitFormFeedback', + action: 'submitFormFeedbackV2', ...createReqMeta(req), formId, submissionId, diff --git a/src/app/modules/feedback/feedback.errors.ts b/src/app/modules/feedback/feedback.errors.ts index 37a2ea4fac..024fa33ec1 100644 --- a/src/app/modules/feedback/feedback.errors.ts +++ b/src/app/modules/feedback/feedback.errors.ts @@ -1,9 +1,7 @@ import { ApplicationError } from '../core/core.errors' export class InvalidSubmissionIdError extends ApplicationError { - constructor( - message = 'Feedback submissionId can not be found in the form submissions. Please submit a form before you submit a feedback.', - ) { + constructor(message = 'Sorry, something went wrong. Please try again.') { super(message) } } diff --git a/src/app/modules/feedback/feedback.util.ts b/src/app/modules/feedback/feedback.util.ts index c2149f7d90..243e9849b5 100644 --- a/src/app/modules/feedback/feedback.util.ts +++ b/src/app/modules/feedback/feedback.util.ts @@ -14,23 +14,26 @@ const logger = createLoggerWithLabel(module) export const mapRouteError: MapRouteError = ( error: ApplicationError, - coreErrorMessage?: string, + coreErrorMessage = 'Sorry, something went wrong. Please refresh and try again.', ) => { switch (error.constructor) { case FormErrors.FormNotFoundError: return { statusCode: StatusCodes.NOT_FOUND, - errorMessage: error.message, + 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: error.message, + errorMessage: + 'This form has been deleted, so feedback submissions are no longer accepted', } case FormErrors.PrivateFormError: return { statusCode: StatusCodes.NOT_FOUND, - errorMessage: error.message, + errorMessage: + 'This form has been made private, so feedback submissions are no longer accepted', } case InvalidSubmissionIdError: return { @@ -57,7 +60,7 @@ export const mapRouteError: MapRouteError = ( }) return { statusCode: StatusCodes.INTERNAL_SERVER_ERROR, - errorMessage: 'Something went wrong. Please refresh and try again.', + errorMessage: coreErrorMessage, } } } diff --git a/src/app/routes/api/v3/forms/__tests__/public-forms.feedback.routes.spec.ts b/src/app/routes/api/v3/forms/__tests__/public-forms.feedback.routes.spec.ts index ebe6393377..3f48a1e45e 100644 --- a/src/app/routes/api/v3/forms/__tests__/public-forms.feedback.routes.spec.ts +++ b/src/app/routes/api/v3/forms/__tests__/public-forms.feedback.routes.spec.ts @@ -327,7 +327,8 @@ describe('public-form.submissions.feedback.routes', () => { const expectedResp = JSON.parse( JSON.stringify({ - message: 'This form is no longer active', + message: + 'This form has been deleted, so feedback submissions are no longer accepted', }), ) @@ -340,7 +341,8 @@ describe('public-form.submissions.feedback.routes', () => { }) it('should return 500 when form could not be retrieved due to a database error', async () => { - const MOCK_ERROR_MESSAGE = 'mock me' + const MOCK_ERROR_MESSAGE = + 'Sorry, something went wrong. Please refresh and try again.' const MOCK_FEEDBACK = { rating: 5, comment: 'great mock', From da35ef5478552dd57b0cd6f9ad5443fd2ccb5b29 Mon Sep 17 00:00:00 2001 From: Hans Tirtaputra Date: Tue, 14 Jun 2022 20:07:17 +0800 Subject: [PATCH 17/23] refactor: update feedback model index syntax --- src/app/models/form_feedback.server.model.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/app/models/form_feedback.server.model.ts b/src/app/models/form_feedback.server.model.ts index 62f1612db3..51651f7dcc 100644 --- a/src/app/models/form_feedback.server.model.ts +++ b/src/app/models/form_feedback.server.model.ts @@ -20,7 +20,6 @@ const FormFeedbackSchema = new Schema( ref: SUBMISSION_SCHEMA_ID, // TODO #3964: Update to true once we fully migrate to /submissions/{submissionId}/feedback endpoint required: false, - index: true, }, rating: { type: Number, @@ -42,6 +41,10 @@ const FormFeedbackSchema = new Schema( }, ) +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 From ce041e21152b44f56f3f5353ac57ecfb742e8043 Mon Sep 17 00:00:00 2001 From: Hans Tirtaputra Date: Tue, 14 Jun 2022 20:14:23 +0800 Subject: [PATCH 18/23] chore: add TODO comments for feedback tests --- .../v3/forms/__tests__/public-forms.feedback.routes.spec.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/app/routes/api/v3/forms/__tests__/public-forms.feedback.routes.spec.ts b/src/app/routes/api/v3/forms/__tests__/public-forms.feedback.routes.spec.ts index 3f48a1e45e..02e78cedc5 100644 --- a/src/app/routes/api/v3/forms/__tests__/public-forms.feedback.routes.spec.ts +++ b/src/app/routes/api/v3/forms/__tests__/public-forms.feedback.routes.spec.ts @@ -22,7 +22,7 @@ const app = setupApp('/forms', PublicFormsRouter) jest.mock('src/app/modules/spcp/sp.oidc.client.ts') /** - * TODO #3964: Remove 'public-form.feedback.routes' test, and keep 'public-form.submissions.feedback.routes' + * TODO #3964: Remove 'public-form.feedback.routes' test, and keep 'public-form.submissions.feedback.routes' test * once `/api/v3/forms/{formId}/feedback` route is cleaned up */ describe('public-form.feedback.routes', () => { @@ -170,6 +170,10 @@ describe('public-form.feedback.routes', () => { }) }) +/** + * TODO #3964: Update the `describe` path from 'public-form.submissions.feedback.routes' + * to 'public-form.feedback.routes' once `/api/v3/forms/{formId}/feedback` route is cleaned up + */ describe('public-form.submissions.feedback.routes', () => { let request: Session From 59cf0378a442d27aa551d1a71528d2a1fbddedb1 Mon Sep 17 00:00:00 2001 From: Hans Tirtaputra Date: Tue, 14 Jun 2022 20:22:55 +0800 Subject: [PATCH 19/23] fix: remove unecessary formId field in hasNoPreviousFeedback --- src/app/modules/feedback/__tests__/feedback.service.spec.ts | 4 ---- src/app/modules/feedback/feedback.controller.ts | 2 +- src/app/modules/feedback/feedback.service.ts | 3 --- 3 files changed, 1 insertion(+), 8 deletions(-) diff --git a/src/app/modules/feedback/__tests__/feedback.service.spec.ts b/src/app/modules/feedback/__tests__/feedback.service.spec.ts index e0d5622c72..d38cc983f3 100644 --- a/src/app/modules/feedback/__tests__/feedback.service.spec.ts +++ b/src/app/modules/feedback/__tests__/feedback.service.spec.ts @@ -268,7 +268,6 @@ describe('feedback.service', () => { }) const actualResult = await FeedbackService.hasNoPreviousFeedback( - MOCK_FORM_ID, MOCK_SUBMISSION_ID, ) @@ -286,7 +285,6 @@ describe('feedback.service', () => { }) const actualResult = await FeedbackService.hasNoPreviousFeedback( - MOCK_FORM_ID, new ObjectId().toHexString(), ) @@ -299,12 +297,10 @@ describe('feedback.service', () => { existSpy.mockImplementationOnce(() => Promise.reject(new Error('boom'))) const actualResult = await FeedbackService.hasNoPreviousFeedback( - MOCK_FORM_ID, MOCK_SUBMISSION_ID, ) expect(existSpy).toHaveBeenCalledWith({ - formId: MOCK_FORM_ID, submissionId: MOCK_SUBMISSION_ID, }) expect(actualResult.isErr()).toEqual(true) diff --git a/src/app/modules/feedback/feedback.controller.ts b/src/app/modules/feedback/feedback.controller.ts index 3a9a8ab529..3c1c99f63b 100644 --- a/src/app/modules/feedback/feedback.controller.ts +++ b/src/app/modules/feedback/feedback.controller.ts @@ -53,7 +53,7 @@ const submitFormFeedbackV2: ControllerHandler< } return SubmissionService.doesSubmissionIdExist(submissionId) - .andThen(() => FeedbackService.hasNoPreviousFeedback(formId, submissionId)) + .andThen(() => FeedbackService.hasNoPreviousFeedback(submissionId)) .andThen(() => FormService.retrieveFullFormById(formId)) .andThen((form) => FormService.isFormPublic(form).map(() => form)) .andThen((form) => { diff --git a/src/app/modules/feedback/feedback.service.ts b/src/app/modules/feedback/feedback.service.ts index eb33e06fdc..ce4a033eba 100644 --- a/src/app/modules/feedback/feedback.service.ts +++ b/src/app/modules/feedback/feedback.service.ts @@ -127,12 +127,10 @@ export const getFormFeedbacks = ( * @returns err(DatabaseError) if database query errors */ export const hasNoPreviousFeedback = ( - formId: string, submissionId: string, ): ResultAsync => ResultAsync.fromPromise( FormFeedbackModel.exists({ - formId: formId, submissionId: submissionId, }), (error) => { @@ -140,7 +138,6 @@ export const hasNoPreviousFeedback = ( message: 'Error finding feedback documents from database', meta: { action: 'hasNoPreviousFeedback', - formId, submissionId, }, error, From 9b5b6aa4eedb88f3c92669a8cd6cd4dd0c44deba Mon Sep 17 00:00:00 2001 From: Hans Tirtaputra Date: Thu, 16 Jun 2022 00:50:34 +0800 Subject: [PATCH 20/23] fix: update FormFeedbackBase submissionId field to optional --- shared/types/form/form_feedback.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/shared/types/form/form_feedback.ts b/shared/types/form/form_feedback.ts index e37dd449df..0fcc504740 100644 --- a/shared/types/form/form_feedback.ts +++ b/shared/types/form/form_feedback.ts @@ -18,7 +18,8 @@ export type FormFeedbackBase = { formId: FormDto['_id'] created?: Date lastModified?: Date - submissionId: SubmissionResponseDto['submissionId'] + // TODO #3964: Update to not optional once we fully migrate to /submissions/{submissionId}/feedback endpoint + submissionId?: SubmissionResponseDto['submissionId'] } // Convert to serialized version. From 41b6cd00777e849933aaa2c332b3fddf05ba2403 Mon Sep 17 00:00:00 2001 From: Hans Tirtaputra Date: Thu, 16 Jun 2022 00:51:42 +0800 Subject: [PATCH 21/23] refactor: form feedback tests to merge into one describe route --- .../public-forms.feedback.routes.spec.ts | 21 ++++--------------- 1 file changed, 4 insertions(+), 17 deletions(-) diff --git a/src/app/routes/api/v3/forms/__tests__/public-forms.feedback.routes.spec.ts b/src/app/routes/api/v3/forms/__tests__/public-forms.feedback.routes.spec.ts index 02e78cedc5..a2428fe492 100644 --- a/src/app/routes/api/v3/forms/__tests__/public-forms.feedback.routes.spec.ts +++ b/src/app/routes/api/v3/forms/__tests__/public-forms.feedback.routes.spec.ts @@ -37,6 +37,10 @@ describe('public-form.feedback.routes', () => { jest.restoreAllMocks() }) afterAll(async () => await dbHandler.closeDatabase()) + /** + * TODO #3964: Remove /forms/:formId/feedback tests, and keep '/forms/:formId/submissions/:submissionId/feedback' + * test once `/api/v3/forms/{formId}/feedback` route is cleaned up + */ describe('POST /forms/:formId/feedback', () => { it('should return 200 when feedback was successfully saved', async () => { // Arrange @@ -168,24 +172,7 @@ describe('public-form.feedback.routes', () => { expect(response.body).toEqual(expectedResponse) }) }) -}) -/** - * TODO #3964: Update the `describe` path from 'public-form.submissions.feedback.routes' - * to 'public-form.feedback.routes' once `/api/v3/forms/{formId}/feedback` route is cleaned up - */ -describe('public-form.submissions.feedback.routes', () => { - let request: Session - - beforeAll(async () => await dbHandler.connect()) - beforeEach(async () => { - request = supertest(app) - }) - afterEach(async () => { - await dbHandler.clearDatabase() - jest.restoreAllMocks() - }) - afterAll(async () => await dbHandler.closeDatabase()) describe('POST /forms/:formId/submissions/:submissionId/feedback', () => { it('should return 200 when feedback was successfully saved', async () => { const MOCK_FEEDBACK = { From c0d6450cf8d03563a3a862699ad8e024b3a62944 Mon Sep 17 00:00:00 2001 From: Hans Tirtaputra Date: Thu, 16 Jun 2022 00:52:28 +0800 Subject: [PATCH 22/23] refactor: move submissions.feedback route to feedback route --- .../v3/forms/public-forms.feedback.routes.ts | 21 ++++++++++++---- .../api/v3/forms/public-forms.routes.ts | 3 --- ...ublic-forms.submissions.feedback.routes.ts | 25 ------------------- 3 files changed, 16 insertions(+), 33 deletions(-) delete mode 100644 src/app/routes/api/v3/forms/public-forms.submissions.feedback.routes.ts diff --git a/src/app/routes/api/v3/forms/public-forms.feedback.routes.ts b/src/app/routes/api/v3/forms/public-forms.feedback.routes.ts index ee79212066..61004ccfd2 100644 --- a/src/app/routes/api/v3/forms/public-forms.feedback.routes.ts +++ b/src/app/routes/api/v3/forms/public-forms.feedback.routes.ts @@ -1,23 +1,34 @@ import { Router } from 'express' +import * as FeedbackController from '../../../../modules/feedback/feedback.controller' import * as PublicFormController from '../../../../modules/form/public-form/public-form.controller' export const PublicFormsFeedbackRouter = Router() +/** + * @deprecated + * TODO #3964: Remove this route once we fully migrate to /:formId/submissions/:submissionId/feedback endpoint + */ +PublicFormsFeedbackRouter.route('/:formId([a-fA-F0-9]{24})/feedback').post( + PublicFormController.handleSubmitFeedback, +) + /** * Send feedback for a public form - * @route POST /:formId/feedback + * @route POST /:formId/submissions/:submissionId/feedback * @group forms - endpoints to serve forms * @param {string} formId.path.required - the form id + * @param {string} submissionId.path.required - the form submission id * @param {Feedback.model} feedback.body.required - the user's feedback * @consumes application/json * @produces application/json * @returns 200 if feedback was successfully saved * @returns 400 if form feedback was malformed and hence cannot be saved - * @returns 404 if form with formId does not exist or is private + * @returns 404 if form with formId or submissionId does not exist, or form is private * @returns 410 if form has been archived + * @returns 422 if form feedback for the submissionId has already been submitted * @returns 500 if database error occurs */ -PublicFormsFeedbackRouter.route('/:formId([a-fA-F0-9]{24})/feedback').post( - PublicFormController.handleSubmitFeedback, -) +PublicFormsFeedbackRouter.route( + '/:formId([a-fA-F0-9]{24})/submissions/:submissionId([a-fA-F0-9]{24})/feedback', +).post(FeedbackController.handleSubmitFormFeedback) diff --git a/src/app/routes/api/v3/forms/public-forms.routes.ts b/src/app/routes/api/v3/forms/public-forms.routes.ts index 98459a3bb7..a5db22e138 100644 --- a/src/app/routes/api/v3/forms/public-forms.routes.ts +++ b/src/app/routes/api/v3/forms/public-forms.routes.ts @@ -3,7 +3,6 @@ import { Router } from 'express' import { PublicFormsAuthRouter } from './public-forms.auth.routes' import { PublicFormsFeedbackRouter } from './public-forms.feedback.routes' import { PublicFormsFormRouter } from './public-forms.form.routes' -import { PublicFormSubmissionsFeedbackRouter } from './public-forms.submissions.feedback.routes' import { PublicFormsSubmissionsRouter } from './public-forms.submissions.routes' import { PublicFormsVerificationRouter } from './public-forms.verification.routes' @@ -14,5 +13,3 @@ PublicFormsRouter.use(PublicFormsFeedbackRouter) PublicFormsRouter.use(PublicFormsFormRouter) PublicFormsRouter.use(PublicFormsAuthRouter) PublicFormsRouter.use(PublicFormsVerificationRouter) -// TODO #3964: Cleanup PublicFormsFeedbackRouter once it's fully migrated to PublicFormSubmissionsFeedbackRouter -PublicFormsRouter.use(PublicFormSubmissionsFeedbackRouter) diff --git a/src/app/routes/api/v3/forms/public-forms.submissions.feedback.routes.ts b/src/app/routes/api/v3/forms/public-forms.submissions.feedback.routes.ts deleted file mode 100644 index d5dda309d3..0000000000 --- a/src/app/routes/api/v3/forms/public-forms.submissions.feedback.routes.ts +++ /dev/null @@ -1,25 +0,0 @@ -import { Router } from 'express' - -import * as FeedbackController from '../../../../modules/feedback/feedback.controller' - -export const PublicFormSubmissionsFeedbackRouter = Router() - -/** - * Send feedback for a public form - * @route POST /:formId/submissions/:submissionId/feedback - * @group forms - endpoints to serve forms - * @param {string} formId.path.required - the form id - * @param {string} submissionId.path.required - the form submission id - * @param {Feedback.model} feedback.body.required - the user's feedback - * @consumes application/json - * @produces application/json - * @returns 200 if feedback was successfully saved - * @returns 400 if form feedback was malformed and hence cannot be saved - * @returns 404 if form with formId or submissionId does not exist, or form is private - * @returns 410 if form has been archived - * @returns 422 if form feedback for the submissionId has already been submitted - * @returns 500 if database error occurs - */ -PublicFormSubmissionsFeedbackRouter.route( - '/:formId([a-fA-F0-9]{24})/submissions/:submissionId([a-fA-F0-9]{24})/feedback', -).post(FeedbackController.handleSubmitFormFeedback) From a694bfdbf9ac71253230e8ab85d63e6196388ed4 Mon Sep 17 00:00:00 2001 From: Hans Tirtaputra Date: Mon, 4 Jul 2022 14:24:50 +0800 Subject: [PATCH 23/23] fix: update log message for form feedback submission --- src/app/modules/feedback/feedback.controller.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/modules/feedback/feedback.controller.ts b/src/app/modules/feedback/feedback.controller.ts index 3c1c99f63b..c5c16ad3b0 100644 --- a/src/app/modules/feedback/feedback.controller.ts +++ b/src/app/modules/feedback/feedback.controller.ts @@ -71,7 +71,7 @@ const submitFormFeedbackV2: ControllerHandler< .mapErr((error) => { const { errorMessage, statusCode } = mapRouteError(error) logger.error({ - message: errorMessage, + message: 'Error while submitting form feedback', meta: logMeta, error, })