From 0c28dae142744c42cdb0a882120df32d3ce055ef Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Thu, 8 Apr 2021 09:38:08 +0800 Subject: [PATCH 01/30] refactor: v3 delete logic api --- .../form/admin-form/admin-form.controller.ts | 48 ++++++++++++++ .../form/admin-form/admin-form.service.ts | 64 ++++++++++++++++++- .../form/admin-form/admin-form.utils.ts | 6 ++ src/app/modules/form/form.errors.ts | 9 +++ .../api/v3/admin/forms/admin-forms.routes.ts | 17 +++++ 5 files changed, 142 insertions(+), 2 deletions(-) diff --git a/src/app/modules/form/admin-form/admin-form.controller.ts b/src/app/modules/form/admin-form/admin-form.controller.ts index 5b6238dbf7..2a99e2a18e 100644 --- a/src/app/modules/form/admin-form/admin-form.controller.ts +++ b/src/app/modules/form/admin-form/admin-form.controller.ts @@ -1565,6 +1565,54 @@ export const _handleCreateFormField: RequestHandler< }) ) } +/* + * Handler for DELETE /forms/:formId/logic/:logicId + * @security session + * + * @returns 200 with success message when successfully deleted + * @returns 403 when user does not have permissions to delete logic + * @returns 404 when form cannot be found + * @returns 422 when user in session cannot be retrieved from the database + * @returns 500 when database error occurs + */ +export const handleDeleteLogic: RequestHandler = (req, res) => { + const { formId, logicId } = req.params + const sessionUserId = (req.session as Express.AuthedSession).user._id + + // Step 1: Retrieve currently logged in user. + return ( + UserService.getPopulatedUserById(sessionUserId) + .andThen((user) => + // Step 2: Retrieve form with write permission check. + AuthService.getFormAfterPermissionChecks({ + user, + formId, + level: PermissionLevel.Write, + }), + ) + + // Step 3: Delete form logic + .andThen((retrievedForm) => + AdminFormService.deleteFormLogic(retrievedForm, logicId), + ) + .map(() => res.json({ message: 'Logic deleted successfully' })) + .mapErr((error) => { + logger.error({ + message: 'Error occurred when deleting form logic', + meta: { + action: 'handleDeleteLogic', + ...createReqMeta(req), + userId: sessionUserId, + formId, + logicId, + }, + error, + }) + const { errorMessage, statusCode } = mapRouteError(error) + return res.status(statusCode).json({ message: errorMessage }) + }) + ) +} /** * Handler for POST /forms/:formId/fields diff --git a/src/app/modules/form/admin-form/admin-form.service.ts b/src/app/modules/form/admin-form/admin-form.service.ts index 5b20c17249..a40722b32f 100644 --- a/src/app/modules/form/admin-form/admin-form.service.ts +++ b/src/app/modules/form/admin-form/admin-form.service.ts @@ -1,7 +1,7 @@ import { PresignedPost } from 'aws-sdk/clients/s3' import { assignIn, last, omit } from 'lodash' import mongoose from 'mongoose' -import { errAsync, okAsync, ResultAsync } from 'neverthrow' +import { err, errAsync, okAsync, Result, ResultAsync } from 'neverthrow' import { Except, Merge } from 'type-fest' import { @@ -41,7 +41,11 @@ import { } from '../../core/core.errors' import { MissingUserError } from '../../user/user.errors' import * as UserService from '../../user/user.service' -import { FormNotFoundError, TransferOwnershipError } from '../form.errors' +import { + FormNotFoundError, + LogicNotFoundError, + TransferOwnershipError, +} from '../form.errors' import { getFormModelByResponseMode } from '../form.service' import { getFormFieldById } from '../form.utils' @@ -623,3 +627,59 @@ export const updateFormSettings = ( return okAsync(updatedForm.getSettings()) }) } + +/** + * Deletes form logic. + * @param form The original form to delete logic in + * @param logicId the logicId to delete + * @returns ok(true) on success + * @returns err(database errors) if db error is thrown during logic delete + * @returns err(LogicNotFoundError) if logicId does not exist on form + */ +export const deleteFormLogic = ( + form: IPopulatedForm, + logicId: string, +): Result | ResultAsync => { + // First check if specified logic exists + if (!form.form_logics.some((logic) => logic.id === logicId)) { + logger.error({ + message: 'Error occurred when deleting form logic', + meta: { + action: 'deleteFormLogic', + formId: form._id, + logicId, + }, + }) + return err(new LogicNotFoundError('logicId does not exist on form')) + } + + // Remove specified logic and then update form logic + const updated_form_logic = form.form_logics.filter( + (logic) => logic.id !== logicId, + ) + const ModelToUse = getFormModelByResponseMode(form.responseMode) + + return ResultAsync.fromPromise( + ModelToUse.findByIdAndUpdate( + form._id, + { form_logics: updated_form_logic }, + { + new: true, + runValidators: true, + }, + ).exec(), + (error) => { + logger.error({ + message: 'Error occurred when deleting form logic', + meta: { + action: 'deleteFormLogic', + formId: form._id, + logicId, + }, + error, + }) + return new DatabaseError(getMongoErrorMessage(error)) + }, + // On success, return true + ).map(() => true) +} diff --git a/src/app/modules/form/admin-form/admin-form.utils.ts b/src/app/modules/form/admin-form/admin-form.utils.ts index c800b1eed2..06593dcf67 100644 --- a/src/app/modules/form/admin-form/admin-form.utils.ts +++ b/src/app/modules/form/admin-form/admin-form.utils.ts @@ -24,6 +24,7 @@ import { ForbiddenFormError, FormDeletedError, FormNotFoundError, + LogicNotFoundError, PrivateFormError, TransferOwnershipError, } from '../form.errors' @@ -68,6 +69,11 @@ export const mapRouteError = ( statusCode: StatusCodes.NOT_FOUND, errorMessage: error.message, } + case LogicNotFoundError: + return { + statusCode: StatusCodes.NOT_FOUND, + errorMessage: error.message, + } case FormDeletedError: return { statusCode: StatusCodes.GONE, diff --git a/src/app/modules/form/form.errors.ts b/src/app/modules/form/form.errors.ts index 65ea70cfeb..428d1d6cac 100644 --- a/src/app/modules/form/form.errors.ts +++ b/src/app/modules/form/form.errors.ts @@ -51,3 +51,12 @@ export class TransferOwnershipError extends ApplicationError { super(message) } } + +/** + * Error to be returned when form logic cannot be found + */ +export class LogicNotFoundError extends ApplicationError { + constructor(message: string) { + super(message) + } +} diff --git a/src/app/routes/api/v3/admin/forms/admin-forms.routes.ts b/src/app/routes/api/v3/admin/forms/admin-forms.routes.ts index fb4287704d..3dafa15a58 100644 --- a/src/app/routes/api/v3/admin/forms/admin-forms.routes.ts +++ b/src/app/routes/api/v3/admin/forms/admin-forms.routes.ts @@ -1,6 +1,7 @@ import { Router } from 'express' import { withUserAuthentication } from '../../../../../modules/auth/auth.middlewares' +import { handleDeleteLogic } from '../../../../../modules/form/admin-form/admin-form.controller' import { AdminFormsFeedbackRouter } from './admin-forms.feedback.routes' import { AdminFormsFormRouter } from './admin-forms.form.routes' @@ -18,3 +19,19 @@ AdminFormsRouter.use(AdminFormsFeedbackRouter) AdminFormsRouter.use(AdminFormsFormRouter) AdminFormsRouter.use(AdminFormsSubmissionsRouter) AdminFormsRouter.use(AdminFormsPreviewRouter) + +/** + * Deletes a logic. + * @route DELETE /admin/forms/:formId/logic/:logicId + * @group admin + * @produces application/json + * @consumes application/json + * @returns 200 with success message when successfully deleted + * @returns 403 when user does not have permissions to delete logic + * @returns 404 when form cannot be found + * @returns 422 when user in session cannot be retrieved from the database + * @returns 500 when database error occurs + */ +AdminFormsRouter.route( + '/:formId([a-fA-F0-9]{24})/logic/:logicId([a-fA-F0-9]{24})', +).delete(handleDeleteLogic) From d20051242deaf14696547c138d4e7dc4fc114c2c Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Thu, 8 Apr 2021 10:59:45 +0800 Subject: [PATCH 02/30] test: integration tests --- .../admin-forms.settings.routes.spec.ts | 177 +++++++++++++++++- 1 file changed, 176 insertions(+), 1 deletion(-) diff --git a/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.settings.routes.spec.ts b/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.settings.routes.spec.ts index c6efd1968c..480fb6b065 100644 --- a/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.settings.routes.spec.ts +++ b/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.settings.routes.spec.ts @@ -3,7 +3,7 @@ import mongoose from 'mongoose' import supertest, { Session } from 'supertest-session' import getUserModel from 'src/app/models/user.server.model' -import { Status } from 'src/types' +import { ILogicSchema, Status } from 'src/types' import { SettingsUpdateDto } from 'src/types/api' import { createAuthedSession } from 'tests/integration/helpers/express-auth' @@ -265,4 +265,179 @@ describe('admin-form.settings.routes', () => { }) }) }) + + describe('DELETE /forms/:formId/logic/:logicId', () => { + it('should return 200 with success message on successful form logic delete for email mode form', async () => { + // Arrange + const formLogicId = '606d408f37af650047dd1a6a' + const { form: formToUpdate, user } = await dbHandler.insertEmailForm({ + formOptions: { + form_logics: [ + { + _id: formLogicId, + } as ILogicSchema, + ], + }, + }) + const session = await createAuthedSession(user.email, request) + + // Act + const response = await session + .delete(`/admin/forms/${formToUpdate._id}/logic/${formLogicId}`) + .send() + + // Assert + const expectedResponse = { + message: 'Logic deleted successfully', + } + expect(response.status).toEqual(200) + expect(response.body).toEqual(expectedResponse) + }) + + it('should return 200 with success message on successful form logic delete for encrypt mode form', async () => { + // Arrange + const formLogicId = '606d408f37af650047dd1a6a' + const { form: formToUpdate, user } = await dbHandler.insertEncryptForm({ + formOptions: { + form_logics: [ + { + _id: formLogicId, + } as ILogicSchema, + ], + }, + }) + const session = await createAuthedSession(user.email, request) + + // Act + const response = await session + .delete(`/admin/forms/${formToUpdate._id}/logic/${formLogicId}`) + .send() + + // Assert + const expectedResponse = { + message: 'Logic deleted successfully', + } + expect(response.status).toEqual(200) + expect(response.body).toEqual(expectedResponse) + }) + + it('should return 403 when current user does not have permissions to delete form logic', async () => { + // Arrange + const formLogicId = '606d408f37af650047dd1a6a' + const { form: formToUpdate, agency } = await dbHandler.insertEncryptForm({ + formOptions: { + form_logics: [ + { + _id: formLogicId, + } as ILogicSchema, + ], + }, + }) + const diffUser = await dbHandler.insertUser({ + mailName: 'newUser', + agencyId: agency._id, + }) + // Log in as different user. + const session = await createAuthedSession(diffUser.email, request) + + // Act + const response = await session + .delete(`/admin/forms/${formToUpdate._id}/logic/${formLogicId}`) + .send() + + // Assert + expect(response.status).toEqual(403) + expect(response.body).toEqual({ + message: expect.stringContaining( + 'not authorized to perform write operation', + ), + }) + }) + + it('should return 404 with error message if logicId does not exist', async () => { + // Arrange + const formLogicId = '606d408f37af650047dd1a6a' + const wrongLogicId = '606d408f37af650047dd0000' + const { form: formToUpdate, user } = await dbHandler.insertEmailForm({ + formOptions: { + form_logics: [ + { + _id: formLogicId, + } as ILogicSchema, + ], + }, + }) + const session = await createAuthedSession(user.email, request) + + // Act + const response = await session + .delete(`/admin/forms/${formToUpdate._id}/logic/${wrongLogicId}`) + .send() + + // Assert + const expectedResponse = { + message: 'logicId does not exist on form', + } + expect(response.status).toEqual(404) + expect(response.body).toEqual(expectedResponse) + }) + + it('should return 404 with error message if form does not exist', async () => { + // Arrange + const formLogicId = '606d408f37af650047dd1a6a' + const { user } = await dbHandler.insertEmailForm({ + formOptions: { + form_logics: [ + { + _id: formLogicId, + } as ILogicSchema, + ], + }, + }) + const session = await createAuthedSession(user.email, request) + + // Act + const wrongFormId = '12345abcde67890aaaaa1234' + const response = await session + .delete(`/admin/forms/${wrongFormId}/logic/${formLogicId}`) + .send() + + // Assert + const expectedResponse = { + message: 'Form not found', + } + expect(response.status).toEqual(404) + expect(response.body).toEqual(expectedResponse) + }) + + it('should return 422 when userId cannot be found in the database', async () => { + // Arrange + const formLogicId = '606d408f37af650047dd1a6a' + const { form: formToUpdate, user } = await dbHandler.insertEmailForm({ + formOptions: { + form_logics: [ + { + _id: formLogicId, + } as ILogicSchema, + ], + }, + }) + const session = await createAuthedSession(user.email, request) + + // Delete user after login. + await dbHandler.clearCollection(UserModel.collection.name) + + // Act + const response = await session + .delete(`/admin/forms/${formToUpdate._id}/logic/${formLogicId}`) + .send() + + // Assert + const expectedResponse = { + message: 'User not found', + } + expect(response.status).toEqual(422) + expect(response.body).toEqual(expectedResponse) + }) + }) }) From 0b9af1a632d4fdc1a3d97d10a717147bd8a8feeb Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Mon, 12 Apr 2021 11:12:10 +0800 Subject: [PATCH 03/30] chore: use general form model --- src/app/modules/form/admin-form/admin-form.service.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/modules/form/admin-form/admin-form.service.ts b/src/app/modules/form/admin-form/admin-form.service.ts index a40722b32f..38d0208c68 100644 --- a/src/app/modules/form/admin-form/admin-form.service.ts +++ b/src/app/modules/form/admin-form/admin-form.service.ts @@ -657,7 +657,7 @@ export const deleteFormLogic = ( const updated_form_logic = form.form_logics.filter( (logic) => logic.id !== logicId, ) - const ModelToUse = getFormModelByResponseMode(form.responseMode) + const ModelToUse = getFormModel(mongoose) return ResultAsync.fromPromise( ModelToUse.findByIdAndUpdate( From ab36dbb9f2ad6d9498615906eae3722eaf579800 Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Tue, 13 Apr 2021 10:26:49 +0800 Subject: [PATCH 04/30] refactor: use mongoose $pull operator instead of manual filtering --- src/app/modules/form/admin-form/admin-form.service.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/app/modules/form/admin-form/admin-form.service.ts b/src/app/modules/form/admin-form/admin-form.service.ts index 38d0208c68..d29ddaa54f 100644 --- a/src/app/modules/form/admin-form/admin-form.service.ts +++ b/src/app/modules/form/admin-form/admin-form.service.ts @@ -654,15 +654,14 @@ export const deleteFormLogic = ( } // Remove specified logic and then update form logic - const updated_form_logic = form.form_logics.filter( - (logic) => logic.id !== logicId, - ) const ModelToUse = getFormModel(mongoose) return ResultAsync.fromPromise( ModelToUse.findByIdAndUpdate( form._id, - { form_logics: updated_form_logic }, + { + $pull: { form_logics: { _id: logicId } }, + }, { new: true, runValidators: true, From 4b878c8ad32fe1ccb5350c7f80b598279e3f4928 Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Tue, 13 Apr 2021 10:27:26 +0800 Subject: [PATCH 05/30] chore: return transformMongoError --- src/app/modules/form/admin-form/admin-form.service.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/modules/form/admin-form/admin-form.service.ts b/src/app/modules/form/admin-form/admin-form.service.ts index d29ddaa54f..ee65230c75 100644 --- a/src/app/modules/form/admin-form/admin-form.service.ts +++ b/src/app/modules/form/admin-form/admin-form.service.ts @@ -677,7 +677,7 @@ export const deleteFormLogic = ( }, error, }) - return new DatabaseError(getMongoErrorMessage(error)) + return transformMongoError(error) }, // On success, return true ).map(() => true) From b514f6f1fde547eee564f1c249605cbebdd52956 Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Tue, 13 Apr 2021 10:28:47 +0800 Subject: [PATCH 06/30] refactor: use ObjectId() --- .../admin-forms.settings.routes.spec.ts | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.settings.routes.spec.ts b/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.settings.routes.spec.ts index 480fb6b065..7e2dd490b8 100644 --- a/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.settings.routes.spec.ts +++ b/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.settings.routes.spec.ts @@ -269,7 +269,7 @@ describe('admin-form.settings.routes', () => { describe('DELETE /forms/:formId/logic/:logicId', () => { it('should return 200 with success message on successful form logic delete for email mode form', async () => { // Arrange - const formLogicId = '606d408f37af650047dd1a6a' + const formLogicId = new ObjectId() const { form: formToUpdate, user } = await dbHandler.insertEmailForm({ formOptions: { form_logics: [ @@ -296,7 +296,7 @@ describe('admin-form.settings.routes', () => { it('should return 200 with success message on successful form logic delete for encrypt mode form', async () => { // Arrange - const formLogicId = '606d408f37af650047dd1a6a' + const formLogicId = new ObjectId() const { form: formToUpdate, user } = await dbHandler.insertEncryptForm({ formOptions: { form_logics: [ @@ -323,7 +323,7 @@ describe('admin-form.settings.routes', () => { it('should return 403 when current user does not have permissions to delete form logic', async () => { // Arrange - const formLogicId = '606d408f37af650047dd1a6a' + const formLogicId = new ObjectId() const { form: formToUpdate, agency } = await dbHandler.insertEncryptForm({ formOptions: { form_logics: [ @@ -356,8 +356,8 @@ describe('admin-form.settings.routes', () => { it('should return 404 with error message if logicId does not exist', async () => { // Arrange - const formLogicId = '606d408f37af650047dd1a6a' - const wrongLogicId = '606d408f37af650047dd0000' + const formLogicId = new ObjectId() + const wrongLogicId = new ObjectId() const { form: formToUpdate, user } = await dbHandler.insertEmailForm({ formOptions: { form_logics: [ @@ -384,7 +384,7 @@ describe('admin-form.settings.routes', () => { it('should return 404 with error message if form does not exist', async () => { // Arrange - const formLogicId = '606d408f37af650047dd1a6a' + const formLogicId = new ObjectId() const { user } = await dbHandler.insertEmailForm({ formOptions: { form_logics: [ @@ -397,7 +397,7 @@ describe('admin-form.settings.routes', () => { const session = await createAuthedSession(user.email, request) // Act - const wrongFormId = '12345abcde67890aaaaa1234' + const wrongFormId = new ObjectId() const response = await session .delete(`/admin/forms/${wrongFormId}/logic/${formLogicId}`) .send() @@ -412,7 +412,7 @@ describe('admin-form.settings.routes', () => { it('should return 422 when userId cannot be found in the database', async () => { // Arrange - const formLogicId = '606d408f37af650047dd1a6a' + const formLogicId = new ObjectId() const { form: formToUpdate, user } = await dbHandler.insertEmailForm({ formOptions: { form_logics: [ From 5183a65c77c03361d9482751690149f9e1429ae3 Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Wed, 14 Apr 2021 13:38:29 +0800 Subject: [PATCH 07/30] chore: add unit tests --- .../__tests__/admin-form.controller.spec.ts | 173 +++++++++++++++++- .../__tests__/admin-form.service.spec.ts | 84 ++++++++- 2 files changed, 255 insertions(+), 2 deletions(-) diff --git a/src/app/modules/form/admin-form/__tests__/admin-form.controller.spec.ts b/src/app/modules/form/admin-form/__tests__/admin-form.controller.spec.ts index ec8fdc05a2..20b2a0835d 100644 --- a/src/app/modules/form/admin-form/__tests__/admin-form.controller.spec.ts +++ b/src/app/modules/form/admin-form/__tests__/admin-form.controller.spec.ts @@ -49,6 +49,7 @@ import { IFieldSchema, IForm, IFormSchema, + ILogicSchema, IPopulatedEmailForm, IPopulatedEncryptedForm, IPopulatedForm, @@ -6777,6 +6778,7 @@ describe('admin-form.controller', () => { MockAuthService.getFormAfterPermissionChecks.mockReturnValue( okAsync(MOCK_FORM), ) + MockAdminFormService.updateFormField.mockReturnValue( okAsync(MOCK_UPDATED_FIELD as IFieldSchema), ) @@ -7017,6 +7019,7 @@ describe('admin-form.controller', () => { _id: MOCK_USER_ID, email: 'somerandom@example.com', } as IPopulatedUser + const MOCK_FORM = { admin: MOCK_USER, _id: MOCK_FORM_ID, @@ -7039,7 +7042,6 @@ describe('admin-form.controller', () => { }, body: MOCK_CREATE_FIELD_BODY, }) - beforeEach(() => { MockUserService.getPopulatedUserById.mockReturnValue(okAsync(MOCK_USER)) MockAuthService.getFormAfterPermissionChecks.mockReturnValue( @@ -7296,4 +7298,173 @@ describe('admin-form.controller', () => { ) }) }) + + describe('handleDeleteLogic', () => { + const MOCK_USER_ID = new ObjectId().toHexString() + const MOCK_FORM_ID = new ObjectId().toHexString() + const MOCK_USER = { + _id: MOCK_USER_ID, + email: 'somerandom@example.com', + } as IPopulatedUser + + const logicId = new ObjectId().toHexString() + const mockFormLogic = { + form_logics: [ + { + _id: logicId, + id: logicId, + } as ILogicSchema, + ], + } + + const MOCK_FORM = { + admin: MOCK_USER, + _id: MOCK_FORM_ID, + title: 'mock title', + ...mockFormLogic, + } as IPopulatedForm + + const mockReq = expressHandler.mockRequest({ + params: { + formId: MOCK_FORM_ID, + logicId: logicId, + }, + session: { + user: { + _id: MOCK_USER_ID, + }, + }, + }) + const mockRes = expressHandler.mockResponse() + beforeEach(() => { + MockUserService.getPopulatedUserById.mockReturnValue(okAsync(MOCK_USER)) + MockAuthService.getFormAfterPermissionChecks.mockReturnValue( + okAsync(MOCK_FORM), + ) + MockAdminFormService.deleteFormLogic.mockReturnValue(okAsync(true)) + }) + + it('should call all services correctly when request is valid', async () => { + await AdminFormController.handleDeleteLogic(mockReq, mockRes, jest.fn()) + + expect(MockUserService.getPopulatedUserById).toHaveBeenCalledWith( + MOCK_USER_ID, + ) + expect(MockAuthService.getFormAfterPermissionChecks).toHaveBeenCalledWith( + { + user: MOCK_USER, + formId: MOCK_FORM_ID, + level: PermissionLevel.Write, + }, + ) + expect(MockAdminFormService.deleteFormLogic).toHaveBeenCalledWith( + MOCK_FORM, + logicId, + ) + + expect(mockRes.json).toHaveBeenCalledWith({ + message: 'Logic deleted successfully', + }) + }) + + it('should return 403 when user does not have permissions to delete logic', async () => { + MockAuthService.getFormAfterPermissionChecks.mockReturnValue( + errAsync( + new ForbiddenFormError('not authorized to perform write operation'), + ), + ) + + await AdminFormController.handleDeleteLogic(mockReq, mockRes, jest.fn()) + + expect(MockUserService.getPopulatedUserById).toHaveBeenCalledWith( + MOCK_USER_ID, + ) + expect(MockAuthService.getFormAfterPermissionChecks).toHaveBeenCalledWith( + { + user: MOCK_USER, + formId: MOCK_FORM_ID, + level: PermissionLevel.Write, + }, + ) + expect(MockAdminFormService.deleteFormLogic).not.toHaveBeenCalled() + + expect(mockRes.status).toHaveBeenCalledWith(403) + + expect(mockRes.json).toHaveBeenCalledWith({ + message: 'not authorized to perform write operation', + }) + }) + + it('should return 404 when form cannot be found', async () => { + MockAuthService.getFormAfterPermissionChecks.mockReturnValue( + errAsync(new FormNotFoundError()), + ) + + await AdminFormController.handleDeleteLogic(mockReq, mockRes, jest.fn()) + + expect(MockUserService.getPopulatedUserById).toHaveBeenCalledWith( + MOCK_USER_ID, + ) + expect(MockAuthService.getFormAfterPermissionChecks).toHaveBeenCalledWith( + { + user: MOCK_USER, + formId: MOCK_FORM_ID, + level: PermissionLevel.Write, + }, + ) + expect(MockAdminFormService.deleteFormLogic).not.toHaveBeenCalled() + + expect(mockRes.status).toHaveBeenCalledWith(404) + + expect(mockRes.json).toHaveBeenCalledWith({ + message: 'Form not found', + }) + }) + + it('should return 422 when user in session cannot be retrieved from the database', async () => { + MockUserService.getPopulatedUserById.mockReturnValue( + errAsync(new MissingUserError()), + ) + + await AdminFormController.handleDeleteLogic(mockReq, mockRes, jest.fn()) + + expect(MockUserService.getPopulatedUserById).toHaveBeenCalledWith( + MOCK_USER_ID, + ) + expect( + MockAuthService.getFormAfterPermissionChecks, + ).not.toHaveBeenCalled() + + expect(MockAdminFormService.deleteFormLogic).not.toHaveBeenCalled() + + expect(mockRes.status).toHaveBeenCalledWith(422) + + expect(mockRes.json).toHaveBeenCalledWith({ + message: 'User not found', + }) + }) + + it('should return 500 when database error occurs', async () => { + MockUserService.getPopulatedUserById.mockReturnValue( + errAsync(new DatabaseError()), + ) + + await AdminFormController.handleDeleteLogic(mockReq, mockRes, jest.fn()) + + expect(MockUserService.getPopulatedUserById).toHaveBeenCalledWith( + MOCK_USER_ID, + ) + expect( + MockAuthService.getFormAfterPermissionChecks, + ).not.toHaveBeenCalled() + + expect(MockAdminFormService.deleteFormLogic).not.toHaveBeenCalled() + + expect(mockRes.status).toHaveBeenCalledWith(500) + + expect(mockRes.json).toHaveBeenCalledWith({ + message: 'Something went wrong. Please try again.', + }) + }) + }) }) diff --git a/src/app/modules/form/admin-form/__tests__/admin-form.service.spec.ts b/src/app/modules/form/admin-form/__tests__/admin-form.service.spec.ts index 2c2cd1bc4d..fbbac5dbfd 100644 --- a/src/app/modules/form/admin-form/__tests__/admin-form.service.spec.ts +++ b/src/app/modules/form/admin-form/__tests__/admin-form.service.spec.ts @@ -31,6 +31,7 @@ import { IEmailFormSchema, IFormDocument, IFormSchema, + ILogicSchema, IPopulatedForm, IPopulatedUser, IUserSchema, @@ -46,7 +47,7 @@ import { import { generateDefaultField } from 'tests/unit/backend/helpers/generate-form-data' -import { TransferOwnershipError } from '../../form.errors' +import { LogicNotFoundError, TransferOwnershipError } from '../../form.errors' import { CreatePresignedUrlError, EditFieldError, @@ -59,6 +60,7 @@ import { createFormField, createPresignedPostUrlForImages, createPresignedPostUrlForLogos, + deleteFormLogic, duplicateForm, editFormFields, getDashboardForms, @@ -1344,4 +1346,84 @@ describe('admin-form.service', () => { expect(actual._unsafeUnwrapErr()).toBeInstanceOf(DatabaseValidationError) }) }) + describe('deleteFormLogic', () => { + const logicId = new ObjectId().toHexString() + const mockFormLogic = { + form_logics: [ + { + _id: logicId, + id: logicId, + } as ILogicSchema, + ], + } + const MOCK_EMAIL_FORM = ({ + _id: new ObjectId(), + status: Status.Public, + responseMode: ResponseMode.Email, + ...mockFormLogic, + } as unknown) as IPopulatedForm + const MOCK_ENCRYPT_FORM = ({ + _id: new ObjectId(), + status: Status.Public, + responseMode: ResponseMode.Encrypt, + ...mockFormLogic, + } as unknown) as IPopulatedForm + const UPDATE_SPY = jest + .spyOn(FormModel, 'findByIdAndUpdate') + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + .mockReturnValue({ + exec: jest.fn().mockResolvedValue(true), + }) + + it('should return ok(true) on successful form logic delete for email mode form', async () => { + // Act + const actualResult = await deleteFormLogic(MOCK_EMAIL_FORM, logicId) + + // Assert + expect(actualResult.isOk()).toEqual(true) + expect(actualResult._unsafeUnwrap()).toEqual(true) + expect(UPDATE_SPY).toHaveBeenCalledWith( + MOCK_EMAIL_FORM._id, + { + $pull: { form_logics: { _id: logicId } }, + }, + { + new: true, + runValidators: true, + }, + ) + }) + + it('should return ok on successful form logic delete for encrypt mode form', async () => { + // Act + const actualResult = await deleteFormLogic(MOCK_ENCRYPT_FORM, logicId) + + // Assert + expect(actualResult.isOk()).toEqual(true) + expect(actualResult._unsafeUnwrap()).toEqual(true) + expect(UPDATE_SPY).toHaveBeenCalledWith( + MOCK_ENCRYPT_FORM._id, + { + $pull: { form_logics: { _id: logicId } }, + }, + { + new: true, + runValidators: true, + }, + ) + }) + + it('should return LogicNotFoundError if logic does not exist on form', async () => { + // Act + const wrongLogicId = new ObjectId().toHexString() + const actualResult = await deleteFormLogic(MOCK_EMAIL_FORM, wrongLogicId) + + // Assert + expect(actualResult.isErr()).toEqual(true) + expect(actualResult._unsafeUnwrapErr()).toEqual( + new LogicNotFoundError('logicId does not exist on form'), + ) + }) + }) }) From b099515b2facfbbc0963d57cf72e0ab82abed29b Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Wed, 14 Apr 2021 16:31:30 +0800 Subject: [PATCH 08/30] refactor: default message for LogicNotFoundError --- .../form/admin-form/__tests__/admin-form.service.spec.ts | 4 +--- src/app/modules/form/admin-form/admin-form.service.ts | 2 +- src/app/modules/form/form.errors.ts | 2 +- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/app/modules/form/admin-form/__tests__/admin-form.service.spec.ts b/src/app/modules/form/admin-form/__tests__/admin-form.service.spec.ts index fbbac5dbfd..5fe579802f 100644 --- a/src/app/modules/form/admin-form/__tests__/admin-form.service.spec.ts +++ b/src/app/modules/form/admin-form/__tests__/admin-form.service.spec.ts @@ -1421,9 +1421,7 @@ describe('admin-form.service', () => { // Assert expect(actualResult.isErr()).toEqual(true) - expect(actualResult._unsafeUnwrapErr()).toEqual( - new LogicNotFoundError('logicId does not exist on form'), - ) + expect(actualResult._unsafeUnwrapErr()).toEqual(new LogicNotFoundError()) }) }) }) diff --git a/src/app/modules/form/admin-form/admin-form.service.ts b/src/app/modules/form/admin-form/admin-form.service.ts index ee65230c75..b555d24ccc 100644 --- a/src/app/modules/form/admin-form/admin-form.service.ts +++ b/src/app/modules/form/admin-form/admin-form.service.ts @@ -650,7 +650,7 @@ export const deleteFormLogic = ( logicId, }, }) - return err(new LogicNotFoundError('logicId does not exist on form')) + return err(new LogicNotFoundError()) } // Remove specified logic and then update form logic diff --git a/src/app/modules/form/form.errors.ts b/src/app/modules/form/form.errors.ts index 428d1d6cac..50dacb5589 100644 --- a/src/app/modules/form/form.errors.ts +++ b/src/app/modules/form/form.errors.ts @@ -56,7 +56,7 @@ export class TransferOwnershipError extends ApplicationError { * Error to be returned when form logic cannot be found */ export class LogicNotFoundError extends ApplicationError { - constructor(message: string) { + constructor(message = 'logicId does not exist on form') { super(message) } } From d47a9cb77bb01611de8d6d3879d6054b9d6d42f5 Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Wed, 14 Apr 2021 16:35:23 +0800 Subject: [PATCH 09/30] refactor: shift logic to separate router --- .../admin/forms/admin-forms.logic.routes.ts | 21 +++++++++++++++++++ .../api/v3/admin/forms/admin-forms.routes.ts | 2 ++ 2 files changed, 23 insertions(+) create mode 100644 src/app/routes/api/v3/admin/forms/admin-forms.logic.routes.ts diff --git a/src/app/routes/api/v3/admin/forms/admin-forms.logic.routes.ts b/src/app/routes/api/v3/admin/forms/admin-forms.logic.routes.ts new file mode 100644 index 0000000000..595c89671f --- /dev/null +++ b/src/app/routes/api/v3/admin/forms/admin-forms.logic.routes.ts @@ -0,0 +1,21 @@ +import { Router } from 'express' + +import * as AdminFormController from '../../../../../modules/form/admin-form/admin-form.controller' + +export const AdminFormsLogicRouter = Router() + +/** + * Deletes a logic. + * @route DELETE /admin/forms/:formId/logic/:logicId + * @group admin + * @produces application/json + * @consumes application/json + * @returns 200 with success message when successfully deleted + * @returns 403 when user does not have permissions to delete logic + * @returns 404 when form cannot be found + * @returns 422 when user in session cannot be retrieved from the database + * @returns 500 when database error occurs + */ +AdminFormsLogicRouter.route( + '/:formId([a-fA-F0-9]{24})/logic/:logicId([a-fA-F0-9]{24})', +).delete(AdminFormController.handleDeleteLogic) diff --git a/src/app/routes/api/v3/admin/forms/admin-forms.routes.ts b/src/app/routes/api/v3/admin/forms/admin-forms.routes.ts index 3dafa15a58..0d7f4a6884 100644 --- a/src/app/routes/api/v3/admin/forms/admin-forms.routes.ts +++ b/src/app/routes/api/v3/admin/forms/admin-forms.routes.ts @@ -5,6 +5,7 @@ import { handleDeleteLogic } from '../../../../../modules/form/admin-form/admin- import { AdminFormsFeedbackRouter } from './admin-forms.feedback.routes' import { AdminFormsFormRouter } from './admin-forms.form.routes' +import { AdminFormsLogicRouter } from './admin-forms.logic.routes' import { AdminFormsPreviewRouter } from './admin-forms.preview.routes' import { AdminFormsSettingsRouter } from './admin-forms.settings.routes' import { AdminFormsSubmissionsRouter } from './admin-forms.submissions.routes' @@ -19,6 +20,7 @@ AdminFormsRouter.use(AdminFormsFeedbackRouter) AdminFormsRouter.use(AdminFormsFormRouter) AdminFormsRouter.use(AdminFormsSubmissionsRouter) AdminFormsRouter.use(AdminFormsPreviewRouter) +AdminFormsRouter.use(AdminFormsLogicRouter) /** * Deletes a logic. From 2cf4291ab81ecf8edcc42fc5accb894bfc4e3bcb Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Thu, 15 Apr 2021 10:43:11 +0800 Subject: [PATCH 10/30] chore: add test case for logicId cannot be found --- .../__tests__/admin-form.controller.spec.ts | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/src/app/modules/form/admin-form/__tests__/admin-form.controller.spec.ts b/src/app/modules/form/admin-form/__tests__/admin-form.controller.spec.ts index 20b2a0835d..2f1be7420c 100644 --- a/src/app/modules/form/admin-form/__tests__/admin-form.controller.spec.ts +++ b/src/app/modules/form/admin-form/__tests__/admin-form.controller.spec.ts @@ -77,6 +77,7 @@ import { ForbiddenFormError, FormDeletedError, FormNotFoundError, + LogicNotFoundError, PrivateFormError, TransferOwnershipError, } from '../../form.errors' @@ -7395,6 +7396,53 @@ describe('admin-form.controller', () => { }) }) + it('should return 404 when logicId cannot be found', async () => { + const wrongLogicId = new ObjectId().toHexString() + const mockReqWrongLogic = expressHandler.mockRequest({ + params: { + formId: MOCK_FORM_ID, + logicId: wrongLogicId, + }, + session: { + user: { + _id: MOCK_USER_ID, + }, + }, + }) + + MockAdminFormService.deleteFormLogic.mockReturnValue( + err(new LogicNotFoundError()), + ) + + await AdminFormController.handleDeleteLogic( + mockReqWrongLogic, + mockRes, + jest.fn(), + ) + + expect(MockUserService.getPopulatedUserById).toHaveBeenCalledWith( + MOCK_USER_ID, + ) + expect(MockAuthService.getFormAfterPermissionChecks).toHaveBeenCalledWith( + { + user: MOCK_USER, + formId: MOCK_FORM_ID, + level: PermissionLevel.Write, + }, + ) + + expect(MockAdminFormService.deleteFormLogic).toHaveBeenCalledWith( + MOCK_FORM, + wrongLogicId, + ) + + expect(mockRes.status).toHaveBeenCalledWith(404) + + expect(mockRes.json).toHaveBeenCalledWith({ + message: 'logicId does not exist on form', + }) + }) + it('should return 404 when form cannot be found', async () => { MockAuthService.getFormAfterPermissionChecks.mockReturnValue( errAsync(new FormNotFoundError()), From 380bd1f627df1a46e15e13e63aa371ec97d2a4c0 Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Thu, 15 Apr 2021 10:47:50 +0800 Subject: [PATCH 11/30] chore: reset mock forms in beforeEach --- .../__tests__/admin-form.service.spec.ts | 40 +++++++++++-------- 1 file changed, 23 insertions(+), 17 deletions(-) diff --git a/src/app/modules/form/admin-form/__tests__/admin-form.service.spec.ts b/src/app/modules/form/admin-form/__tests__/admin-form.service.spec.ts index 5fe579802f..8b5a2fec89 100644 --- a/src/app/modules/form/admin-form/__tests__/admin-form.service.spec.ts +++ b/src/app/modules/form/admin-form/__tests__/admin-form.service.spec.ts @@ -1356,18 +1356,7 @@ describe('admin-form.service', () => { } as ILogicSchema, ], } - const MOCK_EMAIL_FORM = ({ - _id: new ObjectId(), - status: Status.Public, - responseMode: ResponseMode.Email, - ...mockFormLogic, - } as unknown) as IPopulatedForm - const MOCK_ENCRYPT_FORM = ({ - _id: new ObjectId(), - status: Status.Public, - responseMode: ResponseMode.Encrypt, - ...mockFormLogic, - } as unknown) as IPopulatedForm + const UPDATE_SPY = jest .spyOn(FormModel, 'findByIdAndUpdate') // eslint-disable-next-line @typescript-eslint/ban-ts-comment @@ -1376,15 +1365,32 @@ describe('admin-form.service', () => { exec: jest.fn().mockResolvedValue(true), }) + let mockEmailForm: IPopulatedForm, mockEncryptForm: IPopulatedForm + + beforeEach(() => { + mockEmailForm = ({ + _id: new ObjectId(), + status: Status.Public, + responseMode: ResponseMode.Email, + ...mockFormLogic, + } as unknown) as IPopulatedForm + mockEncryptForm = ({ + _id: new ObjectId(), + status: Status.Public, + responseMode: ResponseMode.Encrypt, + ...mockFormLogic, + } as unknown) as IPopulatedForm + }) + it('should return ok(true) on successful form logic delete for email mode form', async () => { // Act - const actualResult = await deleteFormLogic(MOCK_EMAIL_FORM, logicId) + const actualResult = await deleteFormLogic(mockEmailForm, logicId) // Assert expect(actualResult.isOk()).toEqual(true) expect(actualResult._unsafeUnwrap()).toEqual(true) expect(UPDATE_SPY).toHaveBeenCalledWith( - MOCK_EMAIL_FORM._id, + mockEmailForm._id, { $pull: { form_logics: { _id: logicId } }, }, @@ -1397,13 +1403,13 @@ describe('admin-form.service', () => { it('should return ok on successful form logic delete for encrypt mode form', async () => { // Act - const actualResult = await deleteFormLogic(MOCK_ENCRYPT_FORM, logicId) + const actualResult = await deleteFormLogic(mockEncryptForm, logicId) // Assert expect(actualResult.isOk()).toEqual(true) expect(actualResult._unsafeUnwrap()).toEqual(true) expect(UPDATE_SPY).toHaveBeenCalledWith( - MOCK_ENCRYPT_FORM._id, + mockEncryptForm._id, { $pull: { form_logics: { _id: logicId } }, }, @@ -1417,7 +1423,7 @@ describe('admin-form.service', () => { it('should return LogicNotFoundError if logic does not exist on form', async () => { // Act const wrongLogicId = new ObjectId().toHexString() - const actualResult = await deleteFormLogic(MOCK_EMAIL_FORM, wrongLogicId) + const actualResult = await deleteFormLogic(mockEmailForm, wrongLogicId) // Assert expect(actualResult.isErr()).toEqual(true) From fdffeb2654bd8546c30b03d64e3652f080ed58c2 Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Thu, 15 Apr 2021 10:48:38 +0800 Subject: [PATCH 12/30] refactor: FormModel --- src/app/modules/form/admin-form/admin-form.service.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/app/modules/form/admin-form/admin-form.service.ts b/src/app/modules/form/admin-form/admin-form.service.ts index b555d24ccc..77d7258441 100644 --- a/src/app/modules/form/admin-form/admin-form.service.ts +++ b/src/app/modules/form/admin-form/admin-form.service.ts @@ -654,10 +654,8 @@ export const deleteFormLogic = ( } // Remove specified logic and then update form logic - const ModelToUse = getFormModel(mongoose) - return ResultAsync.fromPromise( - ModelToUse.findByIdAndUpdate( + FormModel.findByIdAndUpdate( form._id, { $pull: { form_logics: { _id: logicId } }, From 5b57f98241be4fbece1ffcb0b2b5484216a0f5ba Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Thu, 15 Apr 2021 10:53:02 +0800 Subject: [PATCH 13/30] refactor: shift tests to admin-forms.logic.routes.spec --- .../admin-forms.logic.routes.spec.ts | 207 ++++++++++++++++++ .../admin-forms.settings.routes.spec.ts | 177 +-------------- 2 files changed, 208 insertions(+), 176 deletions(-) create mode 100644 src/app/routes/api/v3/admin/forms/__tests__/admin-forms.logic.routes.spec.ts diff --git a/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.logic.routes.spec.ts b/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.logic.routes.spec.ts new file mode 100644 index 0000000000..216625bb80 --- /dev/null +++ b/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.logic.routes.spec.ts @@ -0,0 +1,207 @@ +import { ObjectId } from 'bson-ext' +import mongoose from 'mongoose' +import supertest, { Session } from 'supertest-session' + +import getUserModel from 'src/app/models/user.server.model' +import { ILogicSchema } from 'src/types' + +import { createAuthedSession } from 'tests/integration/helpers/express-auth' +import { setupApp } from 'tests/integration/helpers/express-setup' +import dbHandler from 'tests/unit/backend/helpers/jest-db' + +import { AdminFormsRouter } from '../admin-forms.routes' + +const UserModel = getUserModel(mongoose) + +const app = setupApp('/admin/forms', AdminFormsRouter, { + setupWithAuth: true, +}) + +describe('admin-form.logic.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('DELETE /forms/:formId/logic/:logicId', () => { + it('should return 200 with success message on successful form logic delete for email mode form', async () => { + // Arrange + const formLogicId = new ObjectId() + const { form: formToUpdate, user } = await dbHandler.insertEmailForm({ + formOptions: { + form_logics: [ + { + _id: formLogicId, + } as ILogicSchema, + ], + }, + }) + const session = await createAuthedSession(user.email, request) + + // Act + const response = await session + .delete(`/admin/forms/${formToUpdate._id}/logic/${formLogicId}`) + .send() + + // Assert + const expectedResponse = { + message: 'Logic deleted successfully', + } + expect(response.status).toEqual(200) + expect(response.body).toEqual(expectedResponse) + }) + + it('should return 200 with success message on successful form logic delete for encrypt mode form', async () => { + // Arrange + const formLogicId = new ObjectId() + const { form: formToUpdate, user } = await dbHandler.insertEncryptForm({ + formOptions: { + form_logics: [ + { + _id: formLogicId, + } as ILogicSchema, + ], + }, + }) + const session = await createAuthedSession(user.email, request) + + // Act + const response = await session + .delete(`/admin/forms/${formToUpdate._id}/logic/${formLogicId}`) + .send() + + // Assert + const expectedResponse = { + message: 'Logic deleted successfully', + } + expect(response.status).toEqual(200) + expect(response.body).toEqual(expectedResponse) + }) + + it('should return 403 when current user does not have permissions to delete form logic', async () => { + // Arrange + const formLogicId = new ObjectId() + const { form: formToUpdate, agency } = await dbHandler.insertEncryptForm({ + formOptions: { + form_logics: [ + { + _id: formLogicId, + } as ILogicSchema, + ], + }, + }) + const diffUser = await dbHandler.insertUser({ + mailName: 'newUser', + agencyId: agency._id, + }) + // Log in as different user. + const session = await createAuthedSession(diffUser.email, request) + + // Act + const response = await session + .delete(`/admin/forms/${formToUpdate._id}/logic/${formLogicId}`) + .send() + + // Assert + expect(response.status).toEqual(403) + expect(response.body).toEqual({ + message: expect.stringContaining( + 'not authorized to perform write operation', + ), + }) + }) + + it('should return 404 with error message if logicId does not exist', async () => { + // Arrange + const formLogicId = new ObjectId() + const wrongLogicId = new ObjectId() + const { form: formToUpdate, user } = await dbHandler.insertEmailForm({ + formOptions: { + form_logics: [ + { + _id: formLogicId, + } as ILogicSchema, + ], + }, + }) + const session = await createAuthedSession(user.email, request) + + // Act + const response = await session + .delete(`/admin/forms/${formToUpdate._id}/logic/${wrongLogicId}`) + .send() + + // Assert + const expectedResponse = { + message: 'logicId does not exist on form', + } + expect(response.status).toEqual(404) + expect(response.body).toEqual(expectedResponse) + }) + + it('should return 404 with error message if form does not exist', async () => { + // Arrange + const formLogicId = new ObjectId() + const { user } = await dbHandler.insertEmailForm({ + formOptions: { + form_logics: [ + { + _id: formLogicId, + } as ILogicSchema, + ], + }, + }) + const session = await createAuthedSession(user.email, request) + + // Act + const wrongFormId = new ObjectId() + const response = await session + .delete(`/admin/forms/${wrongFormId}/logic/${formLogicId}`) + .send() + + // Assert + const expectedResponse = { + message: 'Form not found', + } + expect(response.status).toEqual(404) + expect(response.body).toEqual(expectedResponse) + }) + + it('should return 422 when userId cannot be found in the database', async () => { + // Arrange + const formLogicId = new ObjectId() + const { form: formToUpdate, user } = await dbHandler.insertEmailForm({ + formOptions: { + form_logics: [ + { + _id: formLogicId, + } as ILogicSchema, + ], + }, + }) + const session = await createAuthedSession(user.email, request) + + // Delete user after login. + await dbHandler.clearCollection(UserModel.collection.name) + + // Act + const response = await session + .delete(`/admin/forms/${formToUpdate._id}/logic/${formLogicId}`) + .send() + + // Assert + const expectedResponse = { + message: 'User not found', + } + expect(response.status).toEqual(422) + expect(response.body).toEqual(expectedResponse) + }) + }) +}) diff --git a/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.settings.routes.spec.ts b/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.settings.routes.spec.ts index 7e2dd490b8..c6efd1968c 100644 --- a/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.settings.routes.spec.ts +++ b/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.settings.routes.spec.ts @@ -3,7 +3,7 @@ import mongoose from 'mongoose' import supertest, { Session } from 'supertest-session' import getUserModel from 'src/app/models/user.server.model' -import { ILogicSchema, Status } from 'src/types' +import { Status } from 'src/types' import { SettingsUpdateDto } from 'src/types/api' import { createAuthedSession } from 'tests/integration/helpers/express-auth' @@ -265,179 +265,4 @@ describe('admin-form.settings.routes', () => { }) }) }) - - describe('DELETE /forms/:formId/logic/:logicId', () => { - it('should return 200 with success message on successful form logic delete for email mode form', async () => { - // Arrange - const formLogicId = new ObjectId() - const { form: formToUpdate, user } = await dbHandler.insertEmailForm({ - formOptions: { - form_logics: [ - { - _id: formLogicId, - } as ILogicSchema, - ], - }, - }) - const session = await createAuthedSession(user.email, request) - - // Act - const response = await session - .delete(`/admin/forms/${formToUpdate._id}/logic/${formLogicId}`) - .send() - - // Assert - const expectedResponse = { - message: 'Logic deleted successfully', - } - expect(response.status).toEqual(200) - expect(response.body).toEqual(expectedResponse) - }) - - it('should return 200 with success message on successful form logic delete for encrypt mode form', async () => { - // Arrange - const formLogicId = new ObjectId() - const { form: formToUpdate, user } = await dbHandler.insertEncryptForm({ - formOptions: { - form_logics: [ - { - _id: formLogicId, - } as ILogicSchema, - ], - }, - }) - const session = await createAuthedSession(user.email, request) - - // Act - const response = await session - .delete(`/admin/forms/${formToUpdate._id}/logic/${formLogicId}`) - .send() - - // Assert - const expectedResponse = { - message: 'Logic deleted successfully', - } - expect(response.status).toEqual(200) - expect(response.body).toEqual(expectedResponse) - }) - - it('should return 403 when current user does not have permissions to delete form logic', async () => { - // Arrange - const formLogicId = new ObjectId() - const { form: formToUpdate, agency } = await dbHandler.insertEncryptForm({ - formOptions: { - form_logics: [ - { - _id: formLogicId, - } as ILogicSchema, - ], - }, - }) - const diffUser = await dbHandler.insertUser({ - mailName: 'newUser', - agencyId: agency._id, - }) - // Log in as different user. - const session = await createAuthedSession(diffUser.email, request) - - // Act - const response = await session - .delete(`/admin/forms/${formToUpdate._id}/logic/${formLogicId}`) - .send() - - // Assert - expect(response.status).toEqual(403) - expect(response.body).toEqual({ - message: expect.stringContaining( - 'not authorized to perform write operation', - ), - }) - }) - - it('should return 404 with error message if logicId does not exist', async () => { - // Arrange - const formLogicId = new ObjectId() - const wrongLogicId = new ObjectId() - const { form: formToUpdate, user } = await dbHandler.insertEmailForm({ - formOptions: { - form_logics: [ - { - _id: formLogicId, - } as ILogicSchema, - ], - }, - }) - const session = await createAuthedSession(user.email, request) - - // Act - const response = await session - .delete(`/admin/forms/${formToUpdate._id}/logic/${wrongLogicId}`) - .send() - - // Assert - const expectedResponse = { - message: 'logicId does not exist on form', - } - expect(response.status).toEqual(404) - expect(response.body).toEqual(expectedResponse) - }) - - it('should return 404 with error message if form does not exist', async () => { - // Arrange - const formLogicId = new ObjectId() - const { user } = await dbHandler.insertEmailForm({ - formOptions: { - form_logics: [ - { - _id: formLogicId, - } as ILogicSchema, - ], - }, - }) - const session = await createAuthedSession(user.email, request) - - // Act - const wrongFormId = new ObjectId() - const response = await session - .delete(`/admin/forms/${wrongFormId}/logic/${formLogicId}`) - .send() - - // Assert - const expectedResponse = { - message: 'Form not found', - } - expect(response.status).toEqual(404) - expect(response.body).toEqual(expectedResponse) - }) - - it('should return 422 when userId cannot be found in the database', async () => { - // Arrange - const formLogicId = new ObjectId() - const { form: formToUpdate, user } = await dbHandler.insertEmailForm({ - formOptions: { - form_logics: [ - { - _id: formLogicId, - } as ILogicSchema, - ], - }, - }) - const session = await createAuthedSession(user.email, request) - - // Delete user after login. - await dbHandler.clearCollection(UserModel.collection.name) - - // Act - const response = await session - .delete(`/admin/forms/${formToUpdate._id}/logic/${formLogicId}`) - .send() - - // Assert - const expectedResponse = { - message: 'User not found', - } - expect(response.status).toEqual(422) - expect(response.body).toEqual(expectedResponse) - }) - }) }) From 7717d4357d3d4c34fa6ab6d83f7d5621a2d15183 Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Thu, 15 Apr 2021 11:09:22 +0800 Subject: [PATCH 14/30] refactor: simplify return of deleteFormLogic --- src/app/modules/form/admin-form/admin-form.service.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/app/modules/form/admin-form/admin-form.service.ts b/src/app/modules/form/admin-form/admin-form.service.ts index 77d7258441..3805f7ca5d 100644 --- a/src/app/modules/form/admin-form/admin-form.service.ts +++ b/src/app/modules/form/admin-form/admin-form.service.ts @@ -1,7 +1,7 @@ import { PresignedPost } from 'aws-sdk/clients/s3' import { assignIn, last, omit } from 'lodash' import mongoose from 'mongoose' -import { err, errAsync, okAsync, Result, ResultAsync } from 'neverthrow' +import { errAsync, okAsync, ResultAsync } from 'neverthrow' import { Except, Merge } from 'type-fest' import { @@ -639,7 +639,7 @@ export const updateFormSettings = ( export const deleteFormLogic = ( form: IPopulatedForm, logicId: string, -): Result | ResultAsync => { +): ResultAsync => { // First check if specified logic exists if (!form.form_logics.some((logic) => logic.id === logicId)) { logger.error({ @@ -650,7 +650,7 @@ export const deleteFormLogic = ( logicId, }, }) - return err(new LogicNotFoundError()) + return errAsync(new LogicNotFoundError()) } // Remove specified logic and then update form logic From 6079d497562028c05a682b08b48a0c1a5034b6e2 Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Mon, 19 Apr 2021 13:57:17 +0800 Subject: [PATCH 15/30] chore: frontend changes for delete logic api --- .../admin/components/edit-logic.client.component.js | 4 +++- src/public/services/AdminFormService.ts | 9 +++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/public/modules/forms/admin/components/edit-logic.client.component.js b/src/public/modules/forms/admin/components/edit-logic.client.component.js index 79b752fb91..92d76d88f2 100644 --- a/src/public/modules/forms/admin/components/edit-logic.client.component.js +++ b/src/public/modules/forms/admin/components/edit-logic.client.component.js @@ -1,6 +1,7 @@ 'use strict' const { LogicType } = require('../../../../../types') +const AdminFormService = require('../../../../services/AdminFormService') angular.module('forms').component('editLogicComponent', { templateUrl: 'modules/forms/admin/componentViews/edit-logic.client.view.html', @@ -74,8 +75,9 @@ function editLogicComponentController($uibModal, FormFields) { } vm.deleteLogic = function (logicIndex) { + const logicIdToDelete = vm.myform.form_logics[logicIndex]._id + AdminFormService.deleteFormLogic(vm.myform._id, logicIdToDelete) vm.myform.form_logics.splice(logicIndex, 1) - updateLogic({ form_logics: vm.myform.form_logics }) } vm.openEditLogicModal = function (currLogic, isNew, logicIndex = -1) { diff --git a/src/public/services/AdminFormService.ts b/src/public/services/AdminFormService.ts index 2beed7cb06..dc35472c61 100644 --- a/src/public/services/AdminFormService.ts +++ b/src/public/services/AdminFormService.ts @@ -46,3 +46,12 @@ export const createSingleFormField = async ( ) .then(({ data }) => data) } + +export const deleteFormLogic = async ( + formId: string, + logicId: string, +): Promise => { + return axios + .delete(`${ADMIN_FORM_ENDPOINT}/${formId}/logic/${logicId}`) + .then(({ data }) => data) +} From bf73b36779489e7f782ec26f4259186c65aeaa52 Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Mon, 19 Apr 2021 13:59:58 +0800 Subject: [PATCH 16/30] chore: simplify --- .../__tests__/admin-form.controller.spec.ts | 25 +++---------------- 1 file changed, 4 insertions(+), 21 deletions(-) diff --git a/src/app/modules/form/admin-form/__tests__/admin-form.controller.spec.ts b/src/app/modules/form/admin-form/__tests__/admin-form.controller.spec.ts index 2f1be7420c..100c266a61 100644 --- a/src/app/modules/form/admin-form/__tests__/admin-form.controller.spec.ts +++ b/src/app/modules/form/admin-form/__tests__/admin-form.controller.spec.ts @@ -7328,7 +7328,7 @@ describe('admin-form.controller', () => { const mockReq = expressHandler.mockRequest({ params: { formId: MOCK_FORM_ID, - logicId: logicId, + logicId, }, session: { user: { @@ -7397,28 +7397,11 @@ describe('admin-form.controller', () => { }) it('should return 404 when logicId cannot be found', async () => { - const wrongLogicId = new ObjectId().toHexString() - const mockReqWrongLogic = expressHandler.mockRequest({ - params: { - formId: MOCK_FORM_ID, - logicId: wrongLogicId, - }, - session: { - user: { - _id: MOCK_USER_ID, - }, - }, - }) - MockAdminFormService.deleteFormLogic.mockReturnValue( - err(new LogicNotFoundError()), + errAsync(new LogicNotFoundError()), ) - await AdminFormController.handleDeleteLogic( - mockReqWrongLogic, - mockRes, - jest.fn(), - ) + await AdminFormController.handleDeleteLogic(mockReq, mockRes, jest.fn()) expect(MockUserService.getPopulatedUserById).toHaveBeenCalledWith( MOCK_USER_ID, @@ -7433,7 +7416,7 @@ describe('admin-form.controller', () => { expect(MockAdminFormService.deleteFormLogic).toHaveBeenCalledWith( MOCK_FORM, - wrongLogicId, + logicId, ) expect(mockRes.status).toHaveBeenCalledWith(404) From cbf974f02b69843f674e1e550eee1e9276ae9d78 Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Mon, 19 Apr 2021 15:04:49 +0800 Subject: [PATCH 17/30] refactor: use model static method --- src/app/models/form.server.model.ts | 18 ++++++++++++++++++ .../form/admin-form/admin-form.service.ts | 11 +---------- src/types/form.ts | 4 ++++ 3 files changed, 23 insertions(+), 10 deletions(-) diff --git a/src/app/models/form.server.model.ts b/src/app/models/form.server.model.ts index 1f90eb7d06..89f418895e 100644 --- a/src/app/models/form.server.model.ts +++ b/src/app/models/form.server.model.ts @@ -613,6 +613,24 @@ const compileFormModel = (db: Mongoose): IFormModel => { ) } + // Deletes specified form logic. + FormSchema.statics.deleteFormLogic = async function ( + this: IFormModel, + form: IPopulatedForm, + logicId: string, + ): Promise { + return this.findByIdAndUpdate( + form._id, + { + $pull: { form_logics: { _id: logicId } }, + }, + { + new: true, + runValidators: true, + }, + ).exec() + } + // Hooks FormSchema.pre('validate', function (next) { // Reject save if form document is too large diff --git a/src/app/modules/form/admin-form/admin-form.service.ts b/src/app/modules/form/admin-form/admin-form.service.ts index 3805f7ca5d..4ced61e277 100644 --- a/src/app/modules/form/admin-form/admin-form.service.ts +++ b/src/app/modules/form/admin-form/admin-form.service.ts @@ -655,16 +655,7 @@ export const deleteFormLogic = ( // Remove specified logic and then update form logic return ResultAsync.fromPromise( - FormModel.findByIdAndUpdate( - form._id, - { - $pull: { form_logics: { _id: logicId } }, - }, - { - new: true, - runValidators: true, - }, - ).exec(), + FormModel.deleteFormLogic(form, logicId), (error) => { logger.error({ message: 'Error occurred when deleting form logic', diff --git a/src/types/form.ts b/src/types/form.ts index 06549398ce..52e9214a3e 100644 --- a/src/types/form.ts +++ b/src/types/form.ts @@ -282,6 +282,10 @@ export interface IFormModel extends Model { formId: string, fields?: (keyof IPopulatedForm)[], ): Promise + deleteFormLogic( + formId: IPopulatedForm, + logicId: string, + ): Promise deactivateById(formId: string): Promise getMetaByUserIdOrEmail( userId: IUserSchema['_id'], From 867e846003b2af8e90df126c43065c75a0ae8f35 Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Mon, 19 Apr 2021 15:32:39 +0800 Subject: [PATCH 18/30] chore: use $q to splice only upon success --- .../components/edit-logic.client.component.js | 24 +++++++++++++++---- src/public/services/AdminFormService.ts | 4 +--- 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/src/public/modules/forms/admin/components/edit-logic.client.component.js b/src/public/modules/forms/admin/components/edit-logic.client.component.js index 92d76d88f2..5ca6532a69 100644 --- a/src/public/modules/forms/admin/components/edit-logic.client.component.js +++ b/src/public/modules/forms/admin/components/edit-logic.client.component.js @@ -10,11 +10,17 @@ angular.module('forms').component('editLogicComponent', { isLogicError: '<', updateForm: '&', }, - controller: ['$uibModal', 'FormFields', editLogicComponentController], + controller: [ + '$uibModal', + 'FormFields', + 'Toastr', + '$q', + editLogicComponentController, + ], controllerAs: 'vm', }) -function editLogicComponentController($uibModal, FormFields) { +function editLogicComponentController($uibModal, FormFields, Toastr, $q) { const vm = this vm.LogicType = LogicType const getNewCondition = function () { @@ -76,8 +82,18 @@ function editLogicComponentController($uibModal, FormFields) { vm.deleteLogic = function (logicIndex) { const logicIdToDelete = vm.myform.form_logics[logicIndex]._id - AdminFormService.deleteFormLogic(vm.myform._id, logicIdToDelete) - vm.myform.form_logics.splice(logicIndex, 1) + $q.when(AdminFormService.deleteFormLogic(vm.myform._id, logicIdToDelete)) + .then((result) => { + if (result.request.status == 200) { + vm.myform.form_logics.splice(logicIndex, 1) + } else { + Toastr.error('Failed to delete logic, please refresh and try again!') + } + }) + .catch((logicDeleteError) => { + console.error(logicDeleteError) + Toastr.error('Failed to delete logic, please refresh and try again!') + }) } vm.openEditLogicModal = function (currLogic, isNew, logicIndex = -1) { diff --git a/src/public/services/AdminFormService.ts b/src/public/services/AdminFormService.ts index dc35472c61..1d5b3879fc 100644 --- a/src/public/services/AdminFormService.ts +++ b/src/public/services/AdminFormService.ts @@ -51,7 +51,5 @@ export const deleteFormLogic = async ( formId: string, logicId: string, ): Promise => { - return axios - .delete(`${ADMIN_FORM_ENDPOINT}/${formId}/logic/${logicId}`) - .then(({ data }) => data) + return axios.delete(`${ADMIN_FORM_ENDPOINT}/${formId}/logic/${logicId}`) } From bc3767dca20bbf5108c81f12b5aa8d3b5bb10f45 Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Tue, 20 Apr 2021 10:35:15 +0800 Subject: [PATCH 19/30] refactor: pass in formId string instead of form object --- src/app/models/form.server.model.ts | 4 ++-- src/app/modules/form/admin-form/admin-form.service.ts | 2 +- src/types/form.ts | 5 +---- 3 files changed, 4 insertions(+), 7 deletions(-) diff --git a/src/app/models/form.server.model.ts b/src/app/models/form.server.model.ts index 89f418895e..a88bb0d1c5 100644 --- a/src/app/models/form.server.model.ts +++ b/src/app/models/form.server.model.ts @@ -616,11 +616,11 @@ const compileFormModel = (db: Mongoose): IFormModel => { // Deletes specified form logic. FormSchema.statics.deleteFormLogic = async function ( this: IFormModel, - form: IPopulatedForm, + formId: string, logicId: string, ): Promise { return this.findByIdAndUpdate( - form._id, + mongoose.Types.ObjectId(formId), { $pull: { form_logics: { _id: logicId } }, }, diff --git a/src/app/modules/form/admin-form/admin-form.service.ts b/src/app/modules/form/admin-form/admin-form.service.ts index 4ced61e277..0b798b4131 100644 --- a/src/app/modules/form/admin-form/admin-form.service.ts +++ b/src/app/modules/form/admin-form/admin-form.service.ts @@ -655,7 +655,7 @@ export const deleteFormLogic = ( // Remove specified logic and then update form logic return ResultAsync.fromPromise( - FormModel.deleteFormLogic(form, logicId), + FormModel.deleteFormLogic(form._id, logicId), (error) => { logger.error({ message: 'Error occurred when deleting form logic', diff --git a/src/types/form.ts b/src/types/form.ts index 52e9214a3e..3e85ea88fc 100644 --- a/src/types/form.ts +++ b/src/types/form.ts @@ -282,10 +282,7 @@ export interface IFormModel extends Model { formId: string, fields?: (keyof IPopulatedForm)[], ): Promise - deleteFormLogic( - formId: IPopulatedForm, - logicId: string, - ): Promise + deleteFormLogic(formId: string, logicId: string): Promise deactivateById(formId: string): Promise getMetaByUserIdOrEmail( userId: IUserSchema['_id'], From a713fdae6a0449c40eeca6f88e54ff70baf4368d Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Tue, 20 Apr 2021 11:27:58 +0800 Subject: [PATCH 20/30] test: add unit tests for static method --- .../__tests__/form.server.model.spec.ts | 55 ++++++++++++++++++- 1 file changed, 54 insertions(+), 1 deletion(-) diff --git a/src/app/models/__tests__/form.server.model.spec.ts b/src/app/models/__tests__/form.server.model.spec.ts index 36665d40a2..775099a222 100644 --- a/src/app/models/__tests__/form.server.model.spec.ts +++ b/src/app/models/__tests__/form.server.model.spec.ts @@ -14,6 +14,7 @@ import { IEncryptedForm, IFieldSchema, IFormSchema, + ILogicSchema, IPopulatedUser, Permission, ResponseMode, @@ -384,7 +385,7 @@ describe('Form Model', () => { expect(actualSavedObject).toEqual(expectedObject) // Remove indeterministic id from actual permission list - const actualPermissionList = (saved.toObject() as IEncryptedForm).permissionList?.map( + const actualPermissionList = ((saved.toObject() as unknown) as IEncryptedForm).permissionList?.map( (permission) => omit(permission, '_id'), ) expect(actualPermissionList).toEqual(permissionList) @@ -1053,6 +1054,58 @@ describe('Form Model', () => { expect(actual).toEqual(expected) }) }) + + describe('deleteFormLogic', () => { + const logicId = new ObjectId().toHexString() + const mockFormLogic = { + form_logics: [ + { + _id: logicId, + id: logicId, + } as ILogicSchema, + ], + } + + it('should return form upon for successful delete', async () => { + // arrange + const formParams = merge({}, MOCK_EMAIL_FORM_PARAMS, { + admin: populatedAdmin, + status: Status.Public, + responseMode: ResponseMode.Email, + ...mockFormLogic, + }) + const form = await Form.create(formParams) + + // act + const modifiedForm = await Form.deleteFormLogic(form._id, logicId) + + // assert + // Form should be returned + expect(modifiedForm).not.toBeNull() + + // Form should have correct status, responsemode + expect(modifiedForm?.responseMode).not.toBeNull() + expect(modifiedForm?.responseMode).toEqual(ResponseMode.Email) + expect(modifiedForm?.status).not.toBeNull() + expect(modifiedForm?.status).toEqual(Status.Public) + + // Check that form logic has been deleted + expect(modifiedForm?.form_logics).toBeEmpty() + }) + + it('should return null if formId is invalid', async () => { + // arrange + + const invalidFormId = new ObjectId().toHexString() + + // act + const modifiedForm = await Form.deleteFormLogic(invalidFormId, logicId) + + // assert + // should return null + expect(modifiedForm).toBeNull() + }) + }) }) describe('Methods', () => { From 7b19b6100fd55861b26172c2f7eca2b9141790c1 Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Tue, 20 Apr 2021 11:56:49 +0800 Subject: [PATCH 21/30] chore: return form instead of true --- .../__tests__/admin-form.service.spec.ts | 41 ++++++++++++++----- .../form/admin-form/admin-form.service.ts | 9 +++- 2 files changed, 37 insertions(+), 13 deletions(-) diff --git a/src/app/modules/form/admin-form/__tests__/admin-form.service.spec.ts b/src/app/modules/form/admin-form/__tests__/admin-form.service.spec.ts index 8b5a2fec89..cf7cc2a0c9 100644 --- a/src/app/modules/form/admin-form/__tests__/admin-form.service.spec.ts +++ b/src/app/modules/form/admin-form/__tests__/admin-form.service.spec.ts @@ -1357,13 +1357,7 @@ describe('admin-form.service', () => { ], } - const UPDATE_SPY = jest - .spyOn(FormModel, 'findByIdAndUpdate') - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-ignore - .mockReturnValue({ - exec: jest.fn().mockResolvedValue(true), - }) + const DELETE_SPY = jest.spyOn(FormModel, 'deleteFormLogic') let mockEmailForm: IPopulatedForm, mockEncryptForm: IPopulatedForm @@ -1382,13 +1376,23 @@ describe('admin-form.service', () => { } as unknown) as IPopulatedForm }) - it('should return ok(true) on successful form logic delete for email mode form', async () => { + it('should return ok(form) on successful form logic delete for email mode form', async () => { + // Arrange + const UPDATE_SPY = jest + .spyOn(FormModel, 'findByIdAndUpdate') + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + .mockReturnValue({ + exec: jest.fn().mockResolvedValue(mockEmailForm), + }) + // Act const actualResult = await deleteFormLogic(mockEmailForm, logicId) // Assert expect(actualResult.isOk()).toEqual(true) - expect(actualResult._unsafeUnwrap()).toEqual(true) + expect(actualResult._unsafeUnwrap()).toEqual(mockEmailForm) + expect(UPDATE_SPY).toHaveBeenCalledWith( mockEmailForm._id, { @@ -1399,15 +1403,27 @@ describe('admin-form.service', () => { runValidators: true, }, ) + + expect(DELETE_SPY).toHaveBeenCalledWith(mockEmailForm._id, logicId) }) - it('should return ok on successful form logic delete for encrypt mode form', async () => { + it('should return ok(form) on successful form logic delete for encrypt mode form', async () => { + // Arrange + const UPDATE_SPY = jest + .spyOn(FormModel, 'findByIdAndUpdate') + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + .mockReturnValue({ + exec: jest.fn().mockResolvedValue(mockEncryptForm), + }) + // Act const actualResult = await deleteFormLogic(mockEncryptForm, logicId) // Assert expect(actualResult.isOk()).toEqual(true) - expect(actualResult._unsafeUnwrap()).toEqual(true) + expect(actualResult._unsafeUnwrap()).toEqual(mockEncryptForm) + expect(UPDATE_SPY).toHaveBeenCalledWith( mockEncryptForm._id, { @@ -1418,6 +1434,8 @@ describe('admin-form.service', () => { runValidators: true, }, ) + + expect(DELETE_SPY).toHaveBeenCalledWith(mockEncryptForm._id, logicId) }) it('should return LogicNotFoundError if logic does not exist on form', async () => { @@ -1428,6 +1446,7 @@ describe('admin-form.service', () => { // Assert expect(actualResult.isErr()).toEqual(true) expect(actualResult._unsafeUnwrapErr()).toEqual(new LogicNotFoundError()) + expect(DELETE_SPY).not.toHaveBeenCalled() }) }) }) diff --git a/src/app/modules/form/admin-form/admin-form.service.ts b/src/app/modules/form/admin-form/admin-form.service.ts index 0b798b4131..a5836fd45b 100644 --- a/src/app/modules/form/admin-form/admin-form.service.ts +++ b/src/app/modules/form/admin-form/admin-form.service.ts @@ -639,7 +639,7 @@ export const updateFormSettings = ( export const deleteFormLogic = ( form: IPopulatedForm, logicId: string, -): ResultAsync => { +): ResultAsync => { // First check if specified logic exists if (!form.form_logics.some((logic) => logic.id === logicId)) { logger.error({ @@ -669,5 +669,10 @@ export const deleteFormLogic = ( return transformMongoError(error) }, // On success, return true - ).map(() => true) + ).andThen((updatedForm) => { + if (!updatedForm) { + return errAsync(new FormNotFoundError()) + } + return okAsync(updatedForm) + }) } From 54b7861add16107bc47d69179171240aaada839d Mon Sep 17 00:00:00 2001 From: tshuli <63710093+tshuli@users.noreply.github.com> Date: Wed, 21 Apr 2021 09:19:32 +0800 Subject: [PATCH 22/30] nit: typo Co-authored-by: Antariksh Mahajan --- src/app/models/__tests__/form.server.model.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/models/__tests__/form.server.model.spec.ts b/src/app/models/__tests__/form.server.model.spec.ts index 775099a222..38adeec8c2 100644 --- a/src/app/models/__tests__/form.server.model.spec.ts +++ b/src/app/models/__tests__/form.server.model.spec.ts @@ -1066,7 +1066,7 @@ describe('Form Model', () => { ], } - it('should return form upon for successful delete', async () => { + it('should return form upon successful delete', async () => { // arrange const formParams = merge({}, MOCK_EMAIL_FORM_PARAMS, { admin: populatedAdmin, From f67c6526485223182171cefe979ade93eced94a1 Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Wed, 21 Apr 2021 09:21:10 +0800 Subject: [PATCH 23/30] chore: add details to error msg --- src/app/modules/form/admin-form/admin-form.service.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/modules/form/admin-form/admin-form.service.ts b/src/app/modules/form/admin-form/admin-form.service.ts index a5836fd45b..5f7019079f 100644 --- a/src/app/modules/form/admin-form/admin-form.service.ts +++ b/src/app/modules/form/admin-form/admin-form.service.ts @@ -643,7 +643,7 @@ export const deleteFormLogic = ( // First check if specified logic exists if (!form.form_logics.some((logic) => logic.id === logicId)) { logger.error({ - message: 'Error occurred when deleting form logic', + message: 'Error occurred - logicId to be deleted does not exist', meta: { action: 'deleteFormLogic', formId: form._id, From 6e02df5c59cf290df735297c416035d7694d9357 Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Wed, 21 Apr 2021 09:48:13 +0800 Subject: [PATCH 24/30] chore: add test for multiple logic case --- .../__tests__/form.server.model.spec.ts | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/src/app/models/__tests__/form.server.model.spec.ts b/src/app/models/__tests__/form.server.model.spec.ts index 38adeec8c2..67d7650ea3 100644 --- a/src/app/models/__tests__/form.server.model.spec.ts +++ b/src/app/models/__tests__/form.server.model.spec.ts @@ -1093,6 +1093,51 @@ describe('Form Model', () => { expect(modifiedForm?.form_logics).toBeEmpty() }) + it('should return form with remaining logic upon successful delete of one logic', async () => { + // arrange + + const logicId2 = new ObjectId().toHexString() + const mockFormLogicMultiple = { + form_logics: [ + { + _id: logicId, + id: logicId, + } as ILogicSchema, + { + _id: logicId2, + id: logicId2, + } as ILogicSchema, + ], + } + + const formParams = merge({}, MOCK_EMAIL_FORM_PARAMS, { + admin: populatedAdmin, + status: Status.Public, + responseMode: ResponseMode.Email, + ...mockFormLogicMultiple, + }) + const form = await Form.create(formParams) + + // act + const modifiedForm = await Form.deleteFormLogic(form._id, logicId) + + // assert + // Form should be returned + expect(modifiedForm).not.toBeNull() + + // Form should have correct status, responsemode + expect(modifiedForm?.responseMode).not.toBeNull() + expect(modifiedForm?.responseMode).toEqual(ResponseMode.Email) + expect(modifiedForm?.status).not.toBeNull() + expect(modifiedForm?.status).toEqual(Status.Public) + + // Check that correct form logic has been deleted + expect(modifiedForm?.form_logics).toBeDefined() + expect(modifiedForm?.form_logics).toHaveLength(1) + const logic = modifiedForm?.form_logics || ['some logic'] + expect((logic[0] as any)['_id'].toString()).toEqual(logicId2) + }) + it('should return null if formId is invalid', async () => { // arrange From cc2d7bc21c5c65e30129ebe9de7b4e274ff41ee3 Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Wed, 21 Apr 2021 10:31:24 +0800 Subject: [PATCH 25/30] build: merge conflict --- src/types/form.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/types/form.ts b/src/types/form.ts index 3e85ea88fc..09fbbb6e14 100644 --- a/src/types/form.ts +++ b/src/types/form.ts @@ -282,7 +282,7 @@ export interface IFormModel extends Model { formId: string, fields?: (keyof IPopulatedForm)[], ): Promise - deleteFormLogic(formId: string, logicId: string): Promise + deleteFormLogic(formId: string, logicId: string): Promise deactivateById(formId: string): Promise getMetaByUserIdOrEmail( userId: IUserSchema['_id'], From 1f681e7737c43f33e860f3e37dabe750e1cbc35b Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Wed, 21 Apr 2021 11:05:11 +0800 Subject: [PATCH 26/30] chore: not send message on logic delete --- .../__tests__/admin-form.controller.spec.ts | 4 +--- .../modules/form/admin-form/admin-form.controller.ts | 2 +- .../forms/__tests__/admin-forms.logic.routes.spec.ts | 12 ++---------- 3 files changed, 4 insertions(+), 14 deletions(-) diff --git a/src/app/modules/form/admin-form/__tests__/admin-form.controller.spec.ts b/src/app/modules/form/admin-form/__tests__/admin-form.controller.spec.ts index 100c266a61..634674ff12 100644 --- a/src/app/modules/form/admin-form/__tests__/admin-form.controller.spec.ts +++ b/src/app/modules/form/admin-form/__tests__/admin-form.controller.spec.ts @@ -7363,9 +7363,7 @@ describe('admin-form.controller', () => { logicId, ) - expect(mockRes.json).toHaveBeenCalledWith({ - message: 'Logic deleted successfully', - }) + expect(mockRes.sendStatus).toHaveBeenCalledWith(200) }) it('should return 403 when user does not have permissions to delete logic', async () => { diff --git a/src/app/modules/form/admin-form/admin-form.controller.ts b/src/app/modules/form/admin-form/admin-form.controller.ts index 2a99e2a18e..c8d41b87c0 100644 --- a/src/app/modules/form/admin-form/admin-form.controller.ts +++ b/src/app/modules/form/admin-form/admin-form.controller.ts @@ -1595,7 +1595,7 @@ export const handleDeleteLogic: RequestHandler = (req, res) => { .andThen((retrievedForm) => AdminFormService.deleteFormLogic(retrievedForm, logicId), ) - .map(() => res.json({ message: 'Logic deleted successfully' })) + .map(() => res.sendStatus(StatusCodes.OK)) .mapErr((error) => { logger.error({ message: 'Error occurred when deleting form logic', diff --git a/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.logic.routes.spec.ts b/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.logic.routes.spec.ts index 216625bb80..6c5cdeadaa 100644 --- a/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.logic.routes.spec.ts +++ b/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.logic.routes.spec.ts @@ -31,7 +31,7 @@ describe('admin-form.logic.routes', () => { afterAll(async () => await dbHandler.closeDatabase()) describe('DELETE /forms/:formId/logic/:logicId', () => { - it('should return 200 with success message on successful form logic delete for email mode form', async () => { + it('should return 200 on successful form logic delete for email mode form', async () => { // Arrange const formLogicId = new ObjectId() const { form: formToUpdate, user } = await dbHandler.insertEmailForm({ @@ -51,14 +51,10 @@ describe('admin-form.logic.routes', () => { .send() // Assert - const expectedResponse = { - message: 'Logic deleted successfully', - } expect(response.status).toEqual(200) - expect(response.body).toEqual(expectedResponse) }) - it('should return 200 with success message on successful form logic delete for encrypt mode form', async () => { + it('should return 200 on successful form logic delete for encrypt mode form', async () => { // Arrange const formLogicId = new ObjectId() const { form: formToUpdate, user } = await dbHandler.insertEncryptForm({ @@ -78,11 +74,7 @@ describe('admin-form.logic.routes', () => { .send() // Assert - const expectedResponse = { - message: 'Logic deleted successfully', - } expect(response.status).toEqual(200) - expect(response.body).toEqual(expectedResponse) }) it('should return 403 when current user does not have permissions to delete form logic', async () => { From 35a78f8ac21bf01a33a40b1ed4f116f6ecea0dc4 Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Wed, 21 Apr 2021 11:08:46 +0800 Subject: [PATCH 27/30] chore: add return true for axios --- src/public/services/AdminFormService.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/public/services/AdminFormService.ts b/src/public/services/AdminFormService.ts index 1d5b3879fc..ef6b115954 100644 --- a/src/public/services/AdminFormService.ts +++ b/src/public/services/AdminFormService.ts @@ -51,5 +51,7 @@ export const deleteFormLogic = async ( formId: string, logicId: string, ): Promise => { - return axios.delete(`${ADMIN_FORM_ENDPOINT}/${formId}/logic/${logicId}`) + return axios + .delete(`${ADMIN_FORM_ENDPOINT}/${formId}/logic/${logicId}`) + .then(() => true) } From 122edf1808c9bcf4e00d36894ce06e0f9744e9df Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Wed, 21 Apr 2021 11:17:26 +0800 Subject: [PATCH 28/30] chore: splice upon promise success --- .../forms/admin/components/edit-logic.client.component.js | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/public/modules/forms/admin/components/edit-logic.client.component.js b/src/public/modules/forms/admin/components/edit-logic.client.component.js index 5ca6532a69..abef915000 100644 --- a/src/public/modules/forms/admin/components/edit-logic.client.component.js +++ b/src/public/modules/forms/admin/components/edit-logic.client.component.js @@ -83,12 +83,8 @@ function editLogicComponentController($uibModal, FormFields, Toastr, $q) { vm.deleteLogic = function (logicIndex) { const logicIdToDelete = vm.myform.form_logics[logicIndex]._id $q.when(AdminFormService.deleteFormLogic(vm.myform._id, logicIdToDelete)) - .then((result) => { - if (result.request.status == 200) { - vm.myform.form_logics.splice(logicIndex, 1) - } else { - Toastr.error('Failed to delete logic, please refresh and try again!') - } + .then(() => { + vm.myform.form_logics.splice(logicIndex, 1) }) .catch((logicDeleteError) => { console.error(logicDeleteError) From 9aad91c488a63a7e4833a20773250ce490239bb1 Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Wed, 21 Apr 2021 14:40:03 +0800 Subject: [PATCH 29/30] chore: cast form._id to string --- src/app/modules/form/admin-form/admin-form.service.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/modules/form/admin-form/admin-form.service.ts b/src/app/modules/form/admin-form/admin-form.service.ts index 5f7019079f..0fdb0e33f5 100644 --- a/src/app/modules/form/admin-form/admin-form.service.ts +++ b/src/app/modules/form/admin-form/admin-form.service.ts @@ -655,7 +655,7 @@ export const deleteFormLogic = ( // Remove specified logic and then update form logic return ResultAsync.fromPromise( - FormModel.deleteFormLogic(form._id, logicId), + FormModel.deleteFormLogic(String(form._id), logicId), (error) => { logger.error({ message: 'Error occurred when deleting form logic', From c108ff8359cb6251bccc27e81514865acdebb5f1 Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Wed, 21 Apr 2021 15:16:37 +0800 Subject: [PATCH 30/30] chore: update tests --- .../admin-form/__tests__/admin-form.service.spec.ts | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/app/modules/form/admin-form/__tests__/admin-form.service.spec.ts b/src/app/modules/form/admin-form/__tests__/admin-form.service.spec.ts index cf7cc2a0c9..f78b8aeb8e 100644 --- a/src/app/modules/form/admin-form/__tests__/admin-form.service.spec.ts +++ b/src/app/modules/form/admin-form/__tests__/admin-form.service.spec.ts @@ -1404,7 +1404,10 @@ describe('admin-form.service', () => { }, ) - expect(DELETE_SPY).toHaveBeenCalledWith(mockEmailForm._id, logicId) + expect(DELETE_SPY).toHaveBeenCalledWith( + String(mockEmailForm._id), + logicId, + ) }) it('should return ok(form) on successful form logic delete for encrypt mode form', async () => { @@ -1435,7 +1438,10 @@ describe('admin-form.service', () => { }, ) - expect(DELETE_SPY).toHaveBeenCalledWith(mockEncryptForm._id, logicId) + expect(DELETE_SPY).toHaveBeenCalledWith( + String(mockEncryptForm._id), + logicId, + ) }) it('should return LogicNotFoundError if logic does not exist on form', async () => {