From e126bd34d901692cc216097010dca6b4d22867d5 Mon Sep 17 00:00:00 2001 From: Hans Tirtaputra Date: Tue, 28 Jun 2022 13:33:18 +0800 Subject: [PATCH 1/2] chore: cleanup deprecated feedback api --- shared/types/form/form_feedback.ts | 3 +- src/app/models/form_feedback.server.model.ts | 3 +- .../modules/feedback/feedback.controller.ts | 8 +- .../public-form/public-form.controller.ts | 102 ------------------ .../form/public-form/public-form.routes.ts | 26 ----- .../form/public-form/public-form.service.ts | 3 +- .../v3/forms/public-forms.feedback.routes.ts | 9 -- 7 files changed, 7 insertions(+), 147 deletions(-) diff --git a/shared/types/form/form_feedback.ts b/shared/types/form/form_feedback.ts index 0fcc504740..e37dd449df 100644 --- a/shared/types/form/form_feedback.ts +++ b/shared/types/form/form_feedback.ts @@ -18,8 +18,7 @@ export type FormFeedbackBase = { formId: FormDto['_id'] created?: Date lastModified?: Date - // TODO #3964: Update to not optional once we fully migrate to /submissions/{submissionId}/feedback endpoint - submissionId?: SubmissionResponseDto['submissionId'] + 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 51651f7dcc..42329ccfcb 100644 --- a/src/app/models/form_feedback.server.model.ts +++ b/src/app/models/form_feedback.server.model.ts @@ -18,8 +18,7 @@ const FormFeedbackSchema = new Schema( submissionId: { type: Schema.Types.ObjectId, ref: SUBMISSION_SCHEMA_ID, - // TODO #3964: Update to true once we fully migrate to /submissions/{submissionId}/feedback endpoint - required: false, + required: true, }, rating: { type: Number, diff --git a/src/app/modules/feedback/feedback.controller.ts b/src/app/modules/feedback/feedback.controller.ts index c5c16ad3b0..49c167a18f 100644 --- a/src/app/modules/feedback/feedback.controller.ts +++ b/src/app/modules/feedback/feedback.controller.ts @@ -35,10 +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< +const submitFormFeedback: ControllerHandler< { formId: string; submissionId: string }, { message: string } | ErrorDto | PrivateFormErrorDto, { rating: number; comment: string } @@ -46,7 +44,7 @@ const submitFormFeedbackV2: ControllerHandler< const { formId, submissionId } = req.params const { rating, comment } = req.body const logMeta = { - action: 'submitFormFeedbackV2', + action: 'submitFormFeedback', ...createReqMeta(req), formId, submissionId, @@ -91,5 +89,5 @@ const submitFormFeedbackV2: ControllerHandler< export const handleSubmitFormFeedback = [ validateSubmitFormFeedbackParams, - submitFormFeedbackV2, + submitFormFeedback, ] as ControllerHandler[] 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 e28454f831..6e9cf5c8db 100644 --- a/src/app/modules/form/public-form/public-form.controller.ts +++ b/src/app/modules/form/public-form/public-form.controller.ts @@ -52,108 +52,6 @@ import { mapFormAuthError, mapRouteError } from './public-form.utils' 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), -}) - -/** - * @deprecated use submitFormFeedbackV2 instead - * - * TODO #3964: Cleanup we fully migrate feedback endpoint to /submissions/{submissionId}/feedback - */ -export const submitFormFeedback: ControllerHandler< - { formId: string }, - { message: string } | ErrorDto | PrivateFormErrorDto, - { rating: number; comment: string } -> = async (req, res) => { - const { formId } = req.params - const { rating, comment } = req.body - - const formResult = await FormService.retrieveFullFormById(formId) - - if (formResult.isErr()) { - const { error } = formResult - logger.error({ - message: 'Failed to retrieve form', - meta: { - action: 'handleSubmitFeedback', - ...createReqMeta(req), - formId, - }, - error, - }) - const { errorMessage, statusCode } = mapRouteError(error) - return res.status(statusCode).json({ message: errorMessage }) - } - - const form = formResult.value - - // Handle form status states. - const isPublicResult = FormService.isFormPublic(form) - if (isPublicResult.isErr()) { - const { error } = isPublicResult - logger.warn({ - message: 'Form is not public', - meta: { - action: 'handleSubmitFeedback', - ...createReqMeta(req), - formId, - }, - 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 }) - } - - // Form is valid, proceed to next step. - return PublicFormService.insertFormFeedback({ - formId: form._id, - rating, - comment, - }) - .map(() => - res - .status(StatusCodes.OK) - .json({ message: 'Successfully submitted feedback' }), - ) - .mapErr((error) => { - logger.error({ - message: 'Error creating form feedback', - meta: { - action: 'handleSubmitFeedback', - ...createReqMeta(req), - formId, - }, - error, - }) - const { errorMessage, statusCode } = mapRouteError(error) - return res.status(statusCode).json({ message: errorMessage }) - }) -} - -export const handleSubmitFeedback = [ - validateSubmitFormFeedbackParams, - submitFormFeedback, -] as ControllerHandler[] - /** * Handler for various endpoints to redirect to their hashbanged versions. * This allows form links to be free of hashbangs and can thus be shared diff --git a/src/app/modules/form/public-form/public-form.routes.ts b/src/app/modules/form/public-form/public-form.routes.ts index bc3897d789..e9a4e1f99a 100644 --- a/src/app/modules/form/public-form/public-form.routes.ts +++ b/src/app/modules/form/public-form/public-form.routes.ts @@ -1,4 +1,3 @@ -import { celebrate, Joi, Segments } from 'celebrate' import { Router } from 'express' import * as PublicFormController from './public-form.controller' @@ -61,28 +60,3 @@ PublicFormRouter.get( '/forms/:agency/:Id([a-fA-F0-9]{24})/embed', PublicFormController.handleRedirect, ) - -/** - * Send feedback for a public form - * @deprecate in favour of POST api/v3/forms/:formId/feedback - * @route POST /:formId/feedback - * @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 410 if form has been archived - * @returns 500 if database error occurs - */ -PublicFormRouter.post( - '/:formId([a-fA-F0-9]{24})/feedback', - 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), - }), - PublicFormController.handleSubmitFeedback, -) 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 fd41a82cd9..c40491eacc 100644 --- a/src/app/modules/form/public-form/public-form.service.ts +++ b/src/app/modules/form/public-form/public-form.service.ts @@ -21,6 +21,7 @@ const logger = createLoggerWithLabel(module) /** * Inserts given feedback to the database. * @param formId the formId to insert the feedback for + * @param submissionId the submissionId of the form submission that the feedback is for * @param rating the feedback rating to insert * @param comment the comment accompanying the feedback * @returns ok(true) if successfully inserted @@ -33,7 +34,7 @@ export const insertFormFeedback = ({ comment, }: { formId: string - submissionId?: string + submissionId: string rating: number comment?: string }): ResultAsync => { 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 61004ccfd2..432cc64a76 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,18 +1,9 @@ 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/submissions/:submissionId/feedback From 24a809f88645bda4dd384f5bb2721ea9157d1d3d Mon Sep 17 00:00:00 2001 From: Hans Tirtaputra Date: Tue, 28 Jun 2022 13:34:04 +0800 Subject: [PATCH 2/2] fix: broken feedback test cases --- .../form_feedback.server.model.spec.ts | 15 ++ .../__tests__/helpers/prepareTestData.ts | 14 ++ .../__tests__/feedback.service.spec.ts | 8 + .../__tests__/admin-form.routes.spec.ts | 43 +++- .../__tests__/public-form.controller.spec.ts | 201 ------------------ .../admin-forms.feedback.routes.spec.ts | 43 +++- .../public-forms.feedback.routes.spec.ts | 135 ------------ src/types/form_feedback.ts | 3 +- 8 files changed, 113 insertions(+), 349 deletions(-) diff --git a/src/app/models/__tests__/form_feedback.server.model.spec.ts b/src/app/models/__tests__/form_feedback.server.model.spec.ts index 444a73dfc3..0794e4644b 100644 --- a/src/app/models/__tests__/form_feedback.server.model.spec.ts +++ b/src/app/models/__tests__/form_feedback.server.model.spec.ts @@ -17,6 +17,7 @@ describe('form_feedback.server.model', () => { describe('Schema', () => { const DEFAULT_PARAMS: IFormFeedback = { formId: new ObjectId(), + submissionId: new ObjectId(), rating: 5, comment: 'feedback comment', } @@ -74,6 +75,18 @@ describe('form_feedback.server.model', () => { mongoose.Error.ValidationError, ) }) + + it('should throw validation error when submissionId param is missing', async () => { + // Act + const actualPromise = new FeedbackModel( + omit(DEFAULT_PARAMS, 'submissionId'), + ).save() + + // Assert + await expect(actualPromise).rejects.toThrowError( + mongoose.Error.ValidationError, + ) + }) }) describe('Statics', () => { @@ -82,8 +95,10 @@ describe('form_feedback.server.model', () => { // Arrange // Create document const mockFormId = new ObjectId() + const mockSubmissionId = new ObjectId() const mockFeedbackDoc = await FeedbackModel.create({ formId: mockFormId, + submissionId: mockSubmissionId, rating: 5, comment: 'feedback comment', }) diff --git a/src/app/modules/examples/__tests__/helpers/prepareTestData.ts b/src/app/modules/examples/__tests__/helpers/prepareTestData.ts index 32b221fda0..4e7c08fc44 100644 --- a/src/app/modules/examples/__tests__/helpers/prepareTestData.ts +++ b/src/app/modules/examples/__tests__/helpers/prepareTestData.ts @@ -9,6 +9,7 @@ import { IAgencySchema, IFormDocument, IFormFeedbackSchema, + ISubmissionSchema, IUserSchema, } from 'src/types' @@ -43,6 +44,8 @@ export type TestData = { expectedFormInfo: FormInfo[] // Feedbacks for each of the forms. feedbacks: IFormFeedbackSchema[] + // Submissions for each of the forms. + submissions: ISubmissionSchema[] } } @@ -64,6 +67,7 @@ const prepareTestData = async ( forms: [], expectedFormInfo: [], feedbacks: [], + submissions: [], }, second: { searchTerm: SearchTerm.Second, @@ -72,6 +76,7 @@ const prepareTestData = async ( forms: [], expectedFormInfo: [], feedbacks: [], + submissions: [], }, total: { searchTerm: '', @@ -80,6 +85,7 @@ const prepareTestData = async ( forms: [], expectedFormInfo: [], feedbacks: [], + submissions: [], }, } @@ -126,6 +132,8 @@ const prepareTestData = async ( ), ), ) + testData.first.submissions = await Promise.all(firstSubmissionPromises) + const secondSubmissionPromises = flatten( testData.second.forms.map((form) => times(testData.second.submissionCount, () => @@ -138,10 +146,15 @@ const prepareTestData = async ( ), ), ) + testData.second.submissions = await Promise.all(secondSubmissionPromises) // Assign all forms in test data. testData.total.forms = testData.first.forms.concat(testData.second.forms) + testData.total.submissions = testData.first.submissions.concat( + testData.second.submissions, + ) + // Add form statistics for "submissions" for both form prefixes. const formStatsPromises = testData.first.forms .map((form) => @@ -176,6 +189,7 @@ const prepareTestData = async ( return FeedbackModel.create({ rating: score, formId: testData.total.forms[i]._id, + submissionId: testData.total.submissions[i]._id, comment: 'very good test', }) }) diff --git a/src/app/modules/feedback/__tests__/feedback.service.spec.ts b/src/app/modules/feedback/__tests__/feedback.service.spec.ts index d38cc983f3..3ef68c5263 100644 --- a/src/app/modules/feedback/__tests__/feedback.service.spec.ts +++ b/src/app/modules/feedback/__tests__/feedback.service.spec.ts @@ -30,11 +30,13 @@ describe('feedback.service', () => { // Arrange // Insert 3 form feedbacks. const validFormId = new ObjectId() + const validSubmissionId = new ObjectId().toHexString() const expectedFeedbackCount = 3 const feedbackPromises = times(expectedFeedbackCount, (count) => FormFeedback.create({ comment: `test feedback ${count}`, formId: validFormId, + submissionId: validSubmissionId, rating: 5, }), ) @@ -109,6 +111,8 @@ describe('feedback.service', () => { }) describe('getFormFeedbacks', () => { + const MOCK_SUBMISSION_ID = new ObjectId().toHexString() + it('should return correct feedback responses', async () => { // Arrange const expectedCount = 3 @@ -116,6 +120,7 @@ describe('feedback.service', () => { const expectedFbPromises = times(expectedCount, (count) => FormFeedback.create({ formId: mockFormId, + submissionId: MOCK_SUBMISSION_ID, comment: `cool form ${count}`, rating: 5 - count, }), @@ -123,6 +128,7 @@ describe('feedback.service', () => { // Add another feedback with a different form id. await FormFeedback.create({ formId: new ObjectId(), + submissionId: MOCK_SUBMISSION_ID, comment: 'boo this form sux', rating: 1, }) @@ -194,6 +200,7 @@ describe('feedback.service', () => { const mockFormId = new ObjectId().toHexString() const createdFb = await FormFeedback.create({ formId: mockFormId, + submissionId: MOCK_SUBMISSION_ID, // Missing comment key value. rating: 3, }) @@ -281,6 +288,7 @@ describe('feedback.service', () => { await FormFeedback.create({ comment: `test feedback`, formId: MOCK_FORM_ID, + submissionId: MOCK_SUBMISSION_ID, rating: 5, }) diff --git a/src/app/modules/form/admin-form/__tests__/admin-form.routes.spec.ts b/src/app/modules/form/admin-form/__tests__/admin-form.routes.spec.ts index c3389f41be..e9450aa803 100644 --- a/src/app/modules/form/admin-form/__tests__/admin-form.routes.spec.ts +++ b/src/app/modules/form/admin-form/__tests__/admin-form.routes.spec.ts @@ -2,6 +2,7 @@ import { ObjectId } from 'bson-ext' import { format, subDays } from 'date-fns' import { cloneDeep, omit, times } from 'lodash' +import { ObjectID } from 'mongodb' import mongoose from 'mongoose' import { errAsync, okAsync } from 'neverthrow' import SparkMD5 from 'spark-md5' @@ -3128,8 +3129,18 @@ describe('admin-form.routes', () => { it('should return 200 with form feedback meta when feedback exists', async () => { // Arrange const formFeedbacks = [ - { formId: formForFeedback._id, rating: 5, comment: 'nice' }, - { formId: formForFeedback._id, rating: 2, comment: 'not nice' }, + { + formId: formForFeedback._id, + rating: 5, + comment: 'nice', + submissionId: new ObjectID().toHexString(), + }, + { + formId: formForFeedback._id, + rating: 2, + comment: 'not nice', + submissionId: new ObjectID().toHexString(), + }, ] await insertFormFeedback(formFeedbacks[0]) await insertFormFeedback(formFeedbacks[1]) @@ -3308,8 +3319,18 @@ describe('admin-form.routes', () => { it('should return 200 with feedback count when feedback exists', async () => { // Arrange const formFeedbacks = [ - { formId: formForFeedback._id, rating: 5, comment: 'nice' }, - { formId: formForFeedback._id, rating: 2, comment: 'not nice' }, + { + formId: formForFeedback._id, + rating: 5, + comment: 'nice', + submissionId: new ObjectID().toHexString(), + }, + { + formId: formForFeedback._id, + rating: 2, + comment: 'not nice', + submissionId: new ObjectID().toHexString(), + }, ] await insertFormFeedback(formFeedbacks[0]) await insertFormFeedback(formFeedbacks[1]) @@ -3452,8 +3473,18 @@ describe('admin-form.routes', () => { it('should return 200 with feedback stream when feedbacks exist', async () => { // Arrange const formFeedbacks = [ - { formId: formForFeedback._id, rating: 5, comment: 'nice' }, - { formId: formForFeedback._id, rating: 2, comment: 'not nice' }, + { + formId: formForFeedback._id, + rating: 5, + comment: 'nice', + submissionId: new ObjectID().toHexString(), + }, + { + formId: formForFeedback._id, + rating: 2, + comment: 'not nice', + submissionId: new ObjectID().toHexString(), + }, ] await insertFormFeedback(formFeedbacks[0]) await insertFormFeedback(formFeedbacks[1]) diff --git a/src/app/modules/form/public-form/__tests__/public-form.controller.spec.ts b/src/app/modules/form/public-form/__tests__/public-form.controller.spec.ts index e191bfe6ba..06ec8bdf03 100644 --- a/src/app/modules/form/public-form/__tests__/public-form.controller.spec.ts +++ b/src/app/modules/form/public-form/__tests__/public-form.controller.spec.ts @@ -2,12 +2,10 @@ import { IPersonResponse } from '@opengovsg/myinfo-gov-client' import { ObjectId } from 'bson-ext' import { Request } from 'express' import { merge } from 'lodash' -import mongoose from 'mongoose' import { err, errAsync, ok, okAsync } from 'neverthrow' import querystring from 'querystring' import { mocked } from 'ts-jest/utils' -import getFormFeedbackModel from 'src/app/models/form_feedback.server.model' import { DatabaseError } from 'src/app/modules/core/core.errors' import { MyInfoData } from 'src/app/modules/myinfo/myinfo.adapter' import { @@ -48,7 +46,6 @@ import { JwtName } from '../../../spcp/spcp.types' import { AuthTypeMismatchError, FormAuthNoEsrvcIdError, - FormDeletedError, FormNotFoundError, PrivateFormError, } from '../../form.errors' @@ -71,207 +68,9 @@ const MockSpcpService = mocked(SpcpService, true) const MockSpOidcService = mocked(SpOidcService, true) const MockMyInfoService = mocked(MyInfoService, true) -const FormFeedbackModel = getFormFeedbackModel(mongoose) - describe('public-form.controller', () => { afterEach(() => jest.clearAllMocks()) - describe('submitFormFeedback', () => { - const MOCK_FORM_ID = new ObjectId().toHexString() - const MOCK_FORM = { - _id: MOCK_FORM_ID, - title: 'mock form title', - inactiveMessage: 'This mock form is mock closed.', - } as IPopulatedForm - const MOCK_REQ = expressHandler.mockRequest({ - body: { - rating: 4, - comment: 'good', - }, - params: { - formId: MOCK_FORM_ID, - }, - }) - - it('should return 200 when feedback is saved successfully', async () => { - // Arrange - const mockRes = expressHandler.mockResponse() - - // Mock services to return success. - const mockFormFeedback = new FormFeedbackModel({ - formId: new ObjectId().toHexString(), - rating: 5, - comment: 'Great test', - }) - MockFormService.retrieveFullFormById.mockReturnValueOnce( - okAsync(MOCK_FORM as IPopulatedForm), - ) - MockFormService.isFormPublic.mockReturnValueOnce(ok(true)) - MockPublicFormService.insertFormFeedback.mockReturnValueOnce( - okAsync(mockFormFeedback), - ) - - // Act - await PublicFormController.submitFormFeedback( - MOCK_REQ, - mockRes, - jest.fn(), - ) - - // Assert - // Check args of mocked services. - expect(MockFormService.retrieveFullFormById).toHaveBeenCalledWith( - MOCK_FORM_ID, - ) - expect(MockFormService.isFormPublic).toHaveBeenCalledWith(MOCK_FORM) - expect(MockPublicFormService.insertFormFeedback).toHaveBeenCalledWith({ - formId: MOCK_REQ.params.formId, - rating: MOCK_REQ.body.rating, - comment: MOCK_REQ.body.comment, - }) - expect(mockRes.status).toHaveBeenCalledWith(200) - expect(mockRes.json).toHaveBeenCalledWith({ - message: 'Successfully submitted feedback', - }) - }) - - it('should return 404 when retrieving form results in FormNotFoundError', async () => { - // Arrange - const mockRes = expressHandler.mockResponse() - - // Mock retrieval of form to return failure. - MockFormService.retrieveFullFormById.mockReturnValueOnce( - errAsync(new FormNotFoundError()), - ) - - // Act - await PublicFormController.submitFormFeedback( - MOCK_REQ, - mockRes, - jest.fn(), - ) - - // Assert - // Check args of mocked services. - expect(MockFormService.retrieveFullFormById).toHaveBeenCalledWith( - MOCK_FORM_ID, - ) - expect(MockFormService.isFormPublic).not.toHaveBeenCalled() - expect(MockPublicFormService.insertFormFeedback).not.toHaveBeenCalled() - expect(mockRes.status).toHaveBeenCalledWith(404) - expect(mockRes.json).toHaveBeenCalledWith({ - message: 'Form not found', - }) - }) - - it('should return 404 when checking form status results in PrivateFormError', async () => { - // Arrange - const mockRes = expressHandler.mockResponse() - - // Mock services to return correct mock states. - MockFormService.retrieveFullFormById.mockReturnValueOnce( - okAsync(MOCK_FORM as IPopulatedForm), - ) - // Mock return error. - MockFormService.isFormPublic.mockReturnValueOnce( - err(new PrivateFormError(MOCK_FORM.inactiveMessage, MOCK_FORM.title)), - ) - - // Act - await PublicFormController.submitFormFeedback( - MOCK_REQ, - mockRes, - jest.fn(), - ) - - // Assert - // Check args of mocked services. - expect(MockFormService.retrieveFullFormById).toHaveBeenCalledWith( - MOCK_FORM_ID, - ) - expect(MockFormService.isFormPublic).toHaveBeenCalledWith(MOCK_FORM) - expect(MockPublicFormService.insertFormFeedback).not.toHaveBeenCalled() - expect(mockRes.status).toHaveBeenCalledWith(404) - expect(mockRes.json).toHaveBeenCalledWith({ - message: MOCK_FORM.inactiveMessage, - isPageFound: true, - formTitle: MOCK_FORM.title, - }) - }) - - it('should return 410 when checking form status results in FormDeletedError', async () => { - // Arrange - const mockRes = expressHandler.mockResponse() - - // Mock services to return correct mock states. - MockFormService.retrieveFullFormById.mockReturnValueOnce( - okAsync(MOCK_FORM as IPopulatedForm), - ) - // Mock return error. - MockFormService.isFormPublic.mockReturnValueOnce( - err(new FormDeletedError()), - ) - - // Act - await PublicFormController.submitFormFeedback( - MOCK_REQ, - mockRes, - jest.fn(), - ) - - // Assert - // Check args of mocked services. - expect(MockFormService.retrieveFullFormById).toHaveBeenCalledWith( - MOCK_FORM_ID, - ) - expect(MockFormService.isFormPublic).toHaveBeenCalledWith(MOCK_FORM) - expect(MockPublicFormService.insertFormFeedback).not.toHaveBeenCalled() - expect(mockRes.status).toHaveBeenCalledWith(410) - expect(mockRes.json).toHaveBeenCalledWith({ - message: 'This form is no longer active', - }) - }) - - it('should return 500 when database errors occur', async () => { - // Arrange - const mockRes = expressHandler.mockResponse() - const mockErrorString = 'Form feedback could not be created' - - // Mock services to return success or failure states. - MockFormService.retrieveFullFormById.mockReturnValueOnce( - okAsync(MOCK_FORM as IPopulatedForm), - ) - MockFormService.isFormPublic.mockReturnValueOnce(ok(true)) - // Mock database error. - MockPublicFormService.insertFormFeedback.mockReturnValueOnce( - errAsync(new DatabaseError(mockErrorString)), - ) - - // Act - await PublicFormController.submitFormFeedback( - MOCK_REQ, - mockRes, - jest.fn(), - ) - - // Assert - // Check args of mocked services. - expect(MockFormService.retrieveFullFormById).toHaveBeenCalledWith( - MOCK_FORM_ID, - ) - expect(MockFormService.isFormPublic).toHaveBeenCalledWith(MOCK_FORM) - expect(MockPublicFormService.insertFormFeedback).toHaveBeenCalledWith({ - formId: MOCK_REQ.params.formId, - rating: MOCK_REQ.body.rating, - comment: MOCK_REQ.body.comment, - }) - expect(mockRes.status).toHaveBeenCalledWith(500) - expect(mockRes.json).toHaveBeenCalledWith({ - message: mockErrorString, - }) - }) - }) - describe('handleRedirect', () => { const MOCK_FORM_ID = new ObjectId().toHexString() const MOCK_REQ = expressHandler.mockRequest({ diff --git a/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.feedback.routes.spec.ts b/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.feedback.routes.spec.ts index d0f7addbca..1beb9b386b 100644 --- a/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.feedback.routes.spec.ts +++ b/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.feedback.routes.spec.ts @@ -1,5 +1,6 @@ /* eslint-disable @typescript-eslint/ban-ts-comment */ import { ObjectId } from 'bson-ext' +import { ObjectID } from 'mongodb' import mongoose from 'mongoose' import supertest, { Session } from 'supertest-session' @@ -82,8 +83,18 @@ describe('admin-form.feedback.routes', () => { it('should return 200 with form feedback meta when feedback exists', async () => { // Arrange const formFeedbacks = [ - { formId: formForFeedback._id, rating: 5, comment: 'nice' }, - { formId: formForFeedback._id, rating: 2, comment: 'not nice' }, + { + formId: formForFeedback._id, + rating: 5, + comment: 'nice', + submissionId: new ObjectID().toHexString(), + }, + { + formId: formForFeedback._id, + rating: 2, + comment: 'not nice', + submissionId: new ObjectID().toHexString(), + }, ] await insertFormFeedback(formFeedbacks[0]) await insertFormFeedback(formFeedbacks[1]) @@ -262,8 +273,18 @@ describe('admin-form.feedback.routes', () => { it('should return 200 with feedback count when feedback exists', async () => { // Arrange const formFeedbacks = [ - { formId: formForFeedback._id, rating: 5, comment: 'nice' }, - { formId: formForFeedback._id, rating: 2, comment: 'not nice' }, + { + formId: formForFeedback._id, + rating: 5, + comment: 'nice', + submissionId: new ObjectID().toHexString(), + }, + { + formId: formForFeedback._id, + rating: 2, + comment: 'not nice', + submissionId: new ObjectID().toHexString(), + }, ] await insertFormFeedback(formFeedbacks[0]) await insertFormFeedback(formFeedbacks[1]) @@ -406,8 +427,18 @@ describe('admin-form.feedback.routes', () => { it('should return 200 with feedback stream when feedbacks exist', async () => { // Arrange const formFeedbacks = [ - { formId: formForFeedback._id, rating: 5, comment: 'nice' }, - { formId: formForFeedback._id, rating: 2, comment: 'not nice' }, + { + formId: formForFeedback._id, + rating: 5, + comment: 'nice', + submissionId: new ObjectID().toHexString(), + }, + { + formId: formForFeedback._id, + rating: 2, + comment: 'not nice', + submissionId: new ObjectID().toHexString(), + }, ] await insertFormFeedback(formFeedbacks[0]) await insertFormFeedback(formFeedbacks[1]) 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 a2428fe492..b4f6a7bda1 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,141 +37,6 @@ 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 - const { form } = await dbHandler.insertEmailForm({ - formOptions: { - status: FormStatus.Public, - }, - }) - const MOCK_FEEDBACK = { - rating: 5, - comment: 'great mock', - } - const expectedResponse = JSON.parse( - JSON.stringify({ message: 'Successfully submitted feedback' }), - ) - - // Act - const response = await request - .post(`/forms/${form._id}/feedback`) - .send(MOCK_FEEDBACK) - - // Assert - expect(response.status).toEqual(200) - expect(response.body).toEqual(expectedResponse) - }) - - it('should return 400 when form feedback submitted is malformed', async () => { - // Arrange - const { form } = await dbHandler.insertEmailForm() - const MOCK_FEEDBACK = { rating: 6 } - - // Act - const response = await request - .post(`/forms/${form._id}/feedback`) - .send(MOCK_FEEDBACK) - - // Assert - expect(response.status).toEqual(400) - expect(response.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 () => { - // Arrange - const { form } = await dbHandler.insertEmailForm() - const MOCK_FEEDBACK = { - rating: 5, - comment: 'great mock', - } - const expectedResponse = JSON.parse( - JSON.stringify({ - message: form.inactiveMessage, - formTitle: form.title, - isPageFound: true, - }), - ) - - // Act - const response = await request - .post(`/forms/${form._id}/feedback`) - .send(MOCK_FEEDBACK) - - // Assert - expect(response.status).toEqual(404) - expect(response.body).toEqual(expectedResponse) - }) - - it('should return 410 when form is archived', async () => { - // Arrange - const { form } = await dbHandler.insertEmailForm({ - formOptions: { - status: FormStatus.Archived, - }, - }) - const MOCK_FEEDBACK = { - rating: 5, - comment: 'great mock', - } - const expectedResponse = JSON.parse( - JSON.stringify({ - message: 'This form is no longer active', - }), - ) - - // Act - const response = await request - .post(`/forms/${form._id}/feedback`) - .send(MOCK_FEEDBACK) - - // Assert - expect(response.status).toEqual(410) - expect(response.body).toEqual(expectedResponse) - }) - - it('should return 500 when form could not be retrieved due to a database error', async () => { - // Arrange - const { form } = await dbHandler.insertEmailForm({ - formOptions: { - status: FormStatus.Public, - }, - }) - const MOCK_ERROR_MESSAGE = 'mock me' - const MOCK_FEEDBACK = { - rating: 5, - comment: 'great mock', - } - const expectedResponse = JSON.parse( - JSON.stringify({ - message: MOCK_ERROR_MESSAGE, - }), - ) - jest - .spyOn(FormService, 'retrieveFullFormById') - .mockReturnValueOnce(errAsync(new DatabaseError(MOCK_ERROR_MESSAGE))) - - // Act - const response = await request - .post(`/forms/${form._id}/feedback`) - .send(MOCK_FEEDBACK) - - // Assert - expect(response.status).toEqual(500) - expect(response.body).toEqual(expectedResponse) - }) - }) describe('POST /forms/:formId/submissions/:submissionId/feedback', () => { it('should return 200 when feedback was successfully saved', async () => { diff --git a/src/types/form_feedback.ts b/src/types/form_feedback.ts index 74de8a3e13..e5e331949f 100644 --- a/src/types/form_feedback.ts +++ b/src/types/form_feedback.ts @@ -4,10 +4,11 @@ import { Merge } from 'type-fest' import { FormFeedbackBase } from '../../shared/types' import { IFormSchema } from './form' +import { ISubmissionSchema } from './submission' export type IFormFeedback = Merge< FormFeedbackBase, - { formId: IFormSchema['_id'] } + { formId: IFormSchema['_id']; submissionId: ISubmissionSchema['_id'] } > export interface IFormFeedbackSchema extends IFormFeedback, Document { created?: Date