From 375f6f0a32ecff30227577a2759a124a19a71d0f Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Mon, 17 May 2021 09:50:38 +0800 Subject: [PATCH 01/18] chore: align joi validation types to schema --- src/app/modules/form/admin-form/admin-form.controller.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 3846a07ee8..a44122a5e7 100644 --- a/src/app/modules/form/admin-form/admin-form.controller.ts +++ b/src/app/modules/form/admin-form/admin-form.controller.ts @@ -1880,7 +1880,7 @@ export const handleUpdateLogic = [ }), preventSubmitMessage: Joi.alternatives().conditional('logicType', { is: LogicType.PreventSubmit, - then: Joi.string().required(), + then: Joi.string(), }), // Allow other field related key-values to be provided and let the model // layer handle the validation. From e1c5831e4ec3246f8cbab95246fa057f1d863674 Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Mon, 17 May 2021 09:50:38 +0800 Subject: [PATCH 02/18] chore: adjust joi validation types --- src/app/modules/form/admin-form/admin-form.controller.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 a44122a5e7..3846a07ee8 100644 --- a/src/app/modules/form/admin-form/admin-form.controller.ts +++ b/src/app/modules/form/admin-form/admin-form.controller.ts @@ -1880,7 +1880,7 @@ export const handleUpdateLogic = [ }), preventSubmitMessage: Joi.alternatives().conditional('logicType', { is: LogicType.PreventSubmit, - then: Joi.string(), + then: Joi.string().required(), }), // Allow other field related key-values to be provided and let the model // layer handle the validation. From f70df83ef448734b90affcb1ce939df05b442ee3 Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Mon, 17 May 2021 09:50:38 +0800 Subject: [PATCH 03/18] feat: set up controller and route for create logic --- .../form/admin-form/admin-form.controller.ts | 93 ++++++++++++++++++- .../admin/forms/admin-forms.logic.routes.ts | 18 +++- 2 files changed, 109 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 3846a07ee8..b989a547db 100644 --- a/src/app/modules/form/admin-form/admin-form.controller.ts +++ b/src/app/modules/form/admin-form/admin-form.controller.ts @@ -1642,6 +1642,97 @@ export const _handleCreateFormField: RequestHandler< ) } +/** + * NOTE: Exported for testing. + * Private handler for POST /forms/:formId/logic + * @precondition Must be preceded by request validation + * @security session + * + * @returns 200 with created logic object when successfully created + * @returns 403 when user does not have permissions to create 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 _handleCreateLogic: RequestHandler< + { formId: string }, + LogicDto | ErrorDto, + LogicDto +> = (req, res) => { + const { formId } = req.params + const createdLogic = req.body + 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: Create form logic + .andThen((retrievedForm) => + AdminFormService.createFormLogic(retrievedForm, createdLogic), + ) + .map((createdLogic) => res.status(StatusCodes.OK).json(createdLogic)) + .mapErr((error) => { + logger.error({ + message: 'Error occurred when creating form logic', + meta: { + action: 'handleCreateLogic', + ...createReqMeta(req), + userId: sessionUserId, + formId, + createdLogic, + }, + error, + }) + const { errorMessage, statusCode } = mapRouteError(error) + return res.status(statusCode).json({ message: errorMessage }) + }) + ) +} + +/** + * Handler for POST /forms/:formId/logic + */ +export const handleCreateLogic = [ + celebrate({ + [Segments.BODY]: Joi.object({ + logicType: Joi.string() + .valid(...Object.values(LogicType)) + .required(), + conditions: Joi.array() + .items( + Joi.object({ + field: Joi.string().required(), + state: Joi.string() + .valid(...Object.values(LogicConditionState)) + .required(), + value: Joi.any().required(), + ifValueType: Joi.valid(...Object.values(LogicIfValue)), + }).unknown(true), + ) + .required(), + show: Joi.alternatives().conditional('logicType', { + is: LogicType.ShowFields, + then: Joi.array().items(Joi.string()).required(), + }), + preventSubmitMessage: Joi.alternatives().conditional('logicType', { + is: LogicType.PreventSubmit, + then: Joi.string(), + }), + // Allow other field related key-values to be provided and let the model + // layer handle the validation. + }).unknown(true), + }), + _handleCreateLogic, +] as RequestHandler[] + /** * Handler for DELETE /forms/:formId/logic/:logicId * @security session @@ -1791,7 +1882,7 @@ export const handleReorderFormField = [ * @precondition Must be preceded by request validation * @security session * - * @returns 200 with success message and updated logic object when successfully updated + * @returns 200 with updated logic object when successfully updated * @returns 403 when user does not have permissions to update logic * @returns 404 when form cannot be found * @returns 422 when user in session cannot be retrieved from the database 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 index b914d01e06..a92079634b 100644 --- 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 @@ -4,6 +4,22 @@ import * as AdminFormController from '../../../../../modules/form/admin-form/adm export const AdminFormsLogicRouter = Router() +/** + * Creates a logic. + * @route POST /admin/forms/:formId/logic + * @group admin + * @produces application/json + * @consumes application/json + * @returns 200 with created logic when successfully created + * @returns 403 when user does not have permissions to create 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').post( + AdminFormController.handleCreateLogic, +) + /** * Deletes a logic. * @route DELETE /admin/forms/:formId/logic/:logicId @@ -26,7 +42,7 @@ AdminFormsLogicRouter.route( * @group admin * @produces application/json * @consumes application/json - * @returns 200 with success message when successfully updated + * @returns 200 with updated logic when successfully updated * @returns 403 when user does not have permissions to update logic * @returns 404 when form cannot be found * @returns 422 when user in session cannot be retrieved from the database From 8075a028b4a2b08e8f35971e8bad35096a4ad147 Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Mon, 17 May 2021 09:50:38 +0800 Subject: [PATCH 04/18] feat: set up service for create logic --- src/app/models/form.server.model.ts | 17 +++++++++ .../form/admin-form/admin-form.service.ts | 38 ++++++++++++++++++- src/app/modules/form/form.utils.ts | 15 ++++++++ src/types/form.ts | 4 ++ 4 files changed, 73 insertions(+), 1 deletion(-) diff --git a/src/app/models/form.server.model.ts b/src/app/models/form.server.model.ts index aada0cd8fc..bbe914185c 100644 --- a/src/app/models/form.server.model.ts +++ b/src/app/models/form.server.model.ts @@ -681,6 +681,22 @@ const compileFormModel = (db: Mongoose): IFormModel => { ).exec() } + // Creates specified form logic. + FormSchema.statics.createFormLogic = async function ( + this: IFormModel, + formId: string, + createdLogic: LogicDto, + ): Promise { + return this.findByIdAndUpdate( + formId, + { $push: { form_logics: createdLogic } }, + { + new: true, + runValidators: true, + }, + ).exec() + } + // Deletes specified form field by id. FormSchema.statics.deleteFormFieldById = async function ( this: IFormModel, @@ -693,6 +709,7 @@ const compileFormModel = (db: Mongoose): IFormModel => { { new: true, runValidators: true }, ).exec() } + // Updates specified form logic. FormSchema.statics.updateFormLogic = async function ( this: IFormModel, 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 8ac0b05401..b99e4175eb 100644 --- a/src/app/modules/form/admin-form/admin-form.service.ts +++ b/src/app/modules/form/admin-form/admin-form.service.ts @@ -52,7 +52,7 @@ import { TransferOwnershipError, } from '../form.errors' import { getFormModelByResponseMode } from '../form.service' -import { getFormFieldById, getLogicById } from '../form.utils' +import { getFormFieldById, getLastLogic, getLogicById } from '../form.utils' import { PRESIGNED_POST_EXPIRY_SECS } from './admin-form.constants' import { @@ -717,6 +717,42 @@ export const updateFormSettings = ( }) } +/** + * Creates form logic. + * @param form The original form to create logic in + * @param createdLogic Object containing the created logic + * @returns ok(created logic dto) on success + * @returns err(database errors) if db error is thrown during logic update + */ +export const createFormLogic = ( + form: IPopulatedForm, + createdLogic: LogicDto, +): ResultAsync => { + // Create new form logic + return ResultAsync.fromPromise( + FormModel.createFormLogic(form._id.toHexString(), createdLogic), + (error) => { + logger.error({ + message: 'Error occurred when creating form logic', + meta: { + action: 'createFormLogic', + formId: form._id, + createdLogic, + }, + error, + }) + return transformMongoError(error) + }, + // On success, return created form logic + ).andThen((updatedForm) => { + if (!updatedForm) { + return errAsync(new FormNotFoundError()) + } + const createdLogic = getLastLogic(updatedForm.form_logics) + return createdLogic ? okAsync(createdLogic) : errAsync(new DatabaseError()) + }) +} + /** * Deletes form logic. * @param form The original form to delete logic in diff --git a/src/app/modules/form/form.utils.ts b/src/app/modules/form/form.utils.ts index e80a70022c..488da3f559 100644 --- a/src/app/modules/form/form.utils.ts +++ b/src/app/modules/form/form.utils.ts @@ -121,3 +121,18 @@ export const getLogicById = ( return form_logics.find((logic) => logicId === String(logic._id)) ?? null } + +/** + * Returns the last form logic in form_logics array + * @param form_logics the logics + * @returns the last logic if exists, `null` otherwise + */ +export const getLastLogic = ( + form_logics: IFormSchema['form_logics'], +): ILogicSchema | null => { + if (!form_logics) { + return null + } + + return form_logics[form_logics.length - 1] ?? null +} diff --git a/src/types/form.ts b/src/types/form.ts index 50bfb13a05..59655a4c71 100644 --- a/src/types/form.ts +++ b/src/types/form.ts @@ -301,6 +301,10 @@ export interface IFormModel extends Model { formId: string, fields?: (keyof IPopulatedForm)[], ): Promise + createFormLogic( + formId: string, + createdLogic: LogicDto, + ): Promise deleteFormLogic(formId: string, logicId: string): Promise /** * Deletes specified form field by its id from the form corresponding to given form id. From 8ab73e1e6946f69edc78ecdd1d4e26a8a91dc9e8 Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Mon, 17 May 2021 09:50:38 +0800 Subject: [PATCH 05/18] feat: update frontend for create logic --- .../components/edit-logic.client.component.js | 5 ---- .../edit-logic-modal.client.controller.js | 25 ++++++++++++------- src/public/services/AdminFormService.ts | 9 +++++++ 3 files changed, 25 insertions(+), 14 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 45e0138505..e7b3bf9312 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 @@ -112,12 +112,7 @@ function editLogicComponentController($uibModal, FormFields, Toastr, $q) { getNewCondition, myform: vm.myform, }), - updateLogic: () => updateLogic, }, }) } - - const updateLogic = (update) => { - return vm.updateForm({ update }) - } } diff --git a/src/public/modules/forms/admin/controllers/edit-logic-modal.client.controller.js b/src/public/modules/forms/admin/controllers/edit-logic-modal.client.controller.js index 3be8cff745..34fe39ce35 100644 --- a/src/public/modules/forms/admin/controllers/edit-logic-modal.client.controller.js +++ b/src/public/modules/forms/admin/controllers/edit-logic-modal.client.controller.js @@ -10,7 +10,6 @@ angular .controller('EditLogicModalController', [ '$uibModalInstance', 'externalScope', - 'updateLogic', 'FormFields', '$q', 'Toastr', @@ -20,7 +19,6 @@ angular function EditLogicModalController( $uibModalInstance, externalScope, - updateLogic, FormFields, $q, Toastr, @@ -267,18 +265,27 @@ function EditLogicModalController( const { isNew, logicIndex } = externalScope if (isNew) { - vm.formLogics.push(vm.logic) - updateLogic({ form_logics: vm.formLogics }).then((error) => { - if (!error) { - $uibModalInstance.close() - } - }) - // Not new, and logic index is provided + vm.createNewLogic(vm.logic) } else if (logicIndex !== -1) { vm.updateExistingLogic(logicIndex, vm.logic) } } + vm.createNewLogic = function (newLogic) { + $q.when(AdminFormService.createFormLogic(vm.myform._id, newLogic)) + .then((createdLogic) => { + const updatedFormLogics = [...vm.formLogics] + updatedFormLogics.push(createdLogic) + vm.formLogics = updatedFormLogics + externalScope.myform.form_logics = updatedFormLogics // update global myform + $uibModalInstance.close() + }) + .catch((logicCreateError) => { + console.error(logicCreateError) + Toastr.error('Failed to create logic, please refresh and try again!') + }) + } + vm.cancel = function () { $uibModalInstance.close() } diff --git a/src/public/services/AdminFormService.ts b/src/public/services/AdminFormService.ts index 23c655ab80..d8582ee15d 100644 --- a/src/public/services/AdminFormService.ts +++ b/src/public/services/AdminFormService.ts @@ -122,6 +122,15 @@ export const updateFormEndPage = async ( .then(({ data }) => data) } +export const createFormLogic = async ( + formId: string, + createdLogic: LogicDto, +): Promise => { + return axios + .post(`${ADMIN_FORM_ENDPOINT}/${formId}/logic`, createdLogic) + .then(({ data }) => data) +} + export const deleteFormLogic = async ( formId: string, logicId: string, From c356d9a75b8c755c8938bd23205079d7251d0dab Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Mon, 17 May 2021 09:50:38 +0800 Subject: [PATCH 06/18] chore: add tests --- .../__tests__/form.server.model.spec.ts | 143 ++++++++++++++ .../__tests__/admin-form.controller.spec.ts | 176 ++++++++++++++++++ .../__tests__/admin-form.service.spec.ts | 123 +++++++++++- 3 files changed, 437 insertions(+), 5 deletions(-) diff --git a/src/app/models/__tests__/form.server.model.spec.ts b/src/app/models/__tests__/form.server.model.spec.ts index 152a660b06..74d8588073 100644 --- a/src/app/models/__tests__/form.server.model.spec.ts +++ b/src/app/models/__tests__/form.server.model.spec.ts @@ -1057,6 +1057,149 @@ describe('Form Model', () => { }) }) + describe('createFormLogic', () => { + const logicId = new ObjectId().toHexString() + + const mockExistingFormLogic = { + form_logics: [ + { + _id: logicId, + logicType: LogicType.ShowFields, + } as ILogicSchema, + ], + } + + const mockNewFormLogic = ({ + logicType: LogicType.PreventSubmit, + } as unknown) as ILogicSchema + + it('should return form upon successful create logic if form_logic is currently empty', async () => { + // arrange + const formParams = merge({}, MOCK_EMAIL_FORM_PARAMS, { + admin: populatedAdmin, + status: Status.Public, + responseMode: ResponseMode.Email, + form_logics: [], + }) + const form = await Form.create(formParams) + + // act + const modifiedForm = await Form.createFormLogic( + form._id, + mockNewFormLogic, + ) + + // 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 added + expect(modifiedForm?.form_logics).toBeDefined() + expect(modifiedForm?.form_logics).toHaveLength(1) + expect(modifiedForm!.form_logics![0].logicType).toEqual( + LogicType.PreventSubmit, + ) + }) + + it('should allow the same logic to be added more than once and then return form if createLogic is called more than once', async () => { + // arrange + const formParams = merge({}, MOCK_EMAIL_FORM_PARAMS, { + admin: populatedAdmin, + status: Status.Public, + responseMode: ResponseMode.Email, + form_logics: [], + }) + const form = await Form.create(formParams) + + // act + await Form.createFormLogic(form._id, mockNewFormLogic) + + const modifiedFormRepeat = await Form.createFormLogic( + form._id, + mockNewFormLogic, + ) + + // assert + // Form should be returned + expect(modifiedFormRepeat).not.toBeNull() + + // Form should have correct status, responsemode + expect(modifiedFormRepeat?.responseMode).not.toBeNull() + expect(modifiedFormRepeat?.responseMode).toEqual(ResponseMode.Email) + expect(modifiedFormRepeat?.status).not.toBeNull() + expect(modifiedFormRepeat?.status).toEqual(Status.Public) + + // Check that form logic has been added + expect(modifiedFormRepeat?.form_logics).toBeDefined() + expect(modifiedFormRepeat?.form_logics).toHaveLength(2) + expect(modifiedFormRepeat!.form_logics![0].logicType).toEqual( + LogicType.PreventSubmit, + ) + expect(modifiedFormRepeat!.form_logics![1].logicType).toEqual( + LogicType.PreventSubmit, + ) + }) + + it('should return form upon successful create logic if form_logic has existing elements', async () => { + // arrange + const formParams = merge({}, MOCK_EMAIL_FORM_PARAMS, { + admin: populatedAdmin, + status: Status.Public, + responseMode: ResponseMode.Email, + ...mockExistingFormLogic, + }) + const form = await Form.create(formParams) + + // act + const modifiedForm = await Form.createFormLogic( + form._id, + mockNewFormLogic, + ) + + // 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 added + expect(modifiedForm?.form_logics).toBeDefined() + expect(modifiedForm?.form_logics).toHaveLength(2) + expect(modifiedForm!.form_logics![0].logicType).toEqual( + LogicType.ShowFields, + ) + expect(modifiedForm!.form_logics![1].logicType).toEqual( + LogicType.PreventSubmit, + ) + }) + + it('should return null if formId is invalid', async () => { + // arrange + + const invalidFormId = new ObjectId().toHexString() + + // act + const modifiedForm = await Form.createFormLogic( + invalidFormId, + mockNewFormLogic, + ) + + // assert + // should return null + expect(modifiedForm).toBeNull() + }) + }) + describe('deleteFormLogic', () => { const logicId = new ObjectId().toHexString() const mockFormLogic = { 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 e83d66b790..7831f2098d 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 @@ -7824,6 +7824,182 @@ describe('admin-form.controller', () => { }) }) + describe('_handleCreateLogic', () => { + 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 mockCreatedLogic = { + _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, + }, + session: { + user: { + _id: MOCK_USER_ID, + }, + }, + body: mockCreatedLogic, + }) + const mockRes = expressHandler.mockResponse() + + beforeEach(() => { + MockUserService.getPopulatedUserById.mockReturnValue(okAsync(MOCK_USER)) + MockAuthService.getFormAfterPermissionChecks.mockReturnValue( + okAsync(MOCK_FORM), + ) + MockAdminFormService.createFormLogic.mockReturnValue( + okAsync(mockCreatedLogic), + ) + }) + + it('should call all services correctly when request is valid', async () => { + await AdminFormController._handleCreateLogic(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.createFormLogic).toHaveBeenCalledWith( + MOCK_FORM, + mockCreatedLogic, + ) + + expect(mockRes.status).toHaveBeenCalledWith(200) + expect(mockRes.json).toHaveBeenCalledWith(mockCreatedLogic) + }) + + it('should return 403 when user does not have permissions to update logic', async () => { + MockAuthService.getFormAfterPermissionChecks.mockReturnValue( + errAsync( + new ForbiddenFormError('not authorized to perform write operation'), + ), + ) + + await AdminFormController._handleCreateLogic(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.createFormLogic).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._handleCreateLogic(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.createFormLogic).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._handleCreateLogic(mockReq, mockRes, jest.fn()) + + expect(MockUserService.getPopulatedUserById).toHaveBeenCalledWith( + MOCK_USER_ID, + ) + expect( + MockAuthService.getFormAfterPermissionChecks, + ).not.toHaveBeenCalled() + + expect(MockAdminFormService.createFormLogic).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._handleCreateLogic(mockReq, mockRes, jest.fn()) + + expect(MockUserService.getPopulatedUserById).toHaveBeenCalledWith( + MOCK_USER_ID, + ) + expect( + MockAuthService.getFormAfterPermissionChecks, + ).not.toHaveBeenCalled() + + expect(MockAdminFormService.createFormLogic).not.toHaveBeenCalled() + + expect(mockRes.status).toHaveBeenCalledWith(500) + + expect(mockRes.json).toHaveBeenCalledWith({ + message: 'Something went wrong. Please try again.', + }) + }) + }) + describe('handleDeleteFormField', () => { const MOCK_USER_ID = new ObjectId().toHexString() const MOCK_FORM_ID = new ObjectId().toHexString() 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 8d5a50847e..b40f7a4d38 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 @@ -37,6 +37,7 @@ import { IPopulatedForm, IPopulatedUser, IUserSchema, + LogicType, PickDuplicateForm, ResponseMode, Status, @@ -64,6 +65,7 @@ import { archiveForm, createForm, createFormField, + createFormLogic, createPresignedPostUrlForImages, createPresignedPostUrlForLogos, deleteFormField, @@ -1606,6 +1608,117 @@ describe('admin-form.service', () => { expect(actual._unsafeUnwrapErr()).toBeInstanceOf(ApplicationError) }) }) + + describe('createFormLogic', () => { + const logicId1 = new ObjectId() + const logicId2 = new ObjectId() + const logicId3 = new ObjectId() + const mockEmailFormId = new ObjectId() + const mockEncryptFormId = new ObjectId() + + const mockFormLogicOld = { + form_logics: [ + { + _id: logicId1, + logicType: LogicType.ShowFields, + } as ILogicSchema, + { + _id: logicId2, + logicType: LogicType.ShowFields, + } as ILogicSchema, + ], + } + + const createdLogic = { + logicType: LogicType.PreventSubmit, + } as ILogicSchema + + const mockFormLogicUpdated = { + form_logics: [ + { + _id: logicId1, + logicType: LogicType.ShowFields, + } as ILogicSchema, + { + _id: logicId2, + logicType: LogicType.ShowFields, + } as ILogicSchema, + { + _id: logicId3, + logicType: LogicType.PreventSubmit, + } as ILogicSchema, + ], + } + + const CREATE_SPY = jest.spyOn(FormModel, 'createFormLogic') + + let mockEmailForm: IPopulatedForm, + mockEncryptForm: IPopulatedForm, + mockEmailFormUpdated: IPopulatedForm, + mockEncryptFormUpdated: IPopulatedForm + + beforeEach(() => { + mockEmailForm = ({ + _id: mockEmailFormId, + status: Status.Public, + responseMode: ResponseMode.Email, + ...mockFormLogicOld, + } as unknown) as IPopulatedForm + mockEncryptForm = ({ + _id: mockEncryptFormId, + status: Status.Public, + responseMode: ResponseMode.Encrypt, + ...mockFormLogicOld, + } as unknown) as IPopulatedForm + mockEmailFormUpdated = ({ + ...mockEmailForm, + ...mockFormLogicUpdated, + } as unknown) as IPopulatedForm + mockEncryptFormUpdated = ({ + ...mockEncryptForm, + ...mockFormLogicUpdated, + } as unknown) as IPopulatedForm + }) + + it('should return ok(created logic) on successful form logic create for email mode form', async () => { + // Arrange + CREATE_SPY.mockResolvedValue(mockEmailFormUpdated as IFormSchema) + + // Act + const actualResult = await createFormLogic(mockEmailForm, createdLogic) + + // Assert + expect(actualResult.isOk()).toEqual(true) + expect(actualResult._unsafeUnwrap()).toEqual( + expect.objectContaining(createdLogic), + ) + + expect(CREATE_SPY).toHaveBeenCalledWith( + mockEmailForm._id.toHexString(), + createdLogic, + ) + }) + + it('should return ok(created logic) on successful form logic create for encrypt mode form', async () => { + // Arrange + CREATE_SPY.mockResolvedValue(mockEncryptFormUpdated as IFormSchema) + + // Act + const actualResult = await createFormLogic(mockEncryptForm, createdLogic) + + // Assert + expect(actualResult.isOk()).toEqual(true) + expect(actualResult._unsafeUnwrap()).toEqual( + expect.objectContaining(createdLogic), + ) + + expect(CREATE_SPY).toHaveBeenCalledWith( + mockEncryptFormId.toHexString(), + createdLogic, + ) + }) + }) + describe('deleteFormField', () => { let deleteSpy: jest.SpyInstance @@ -1746,29 +1859,29 @@ describe('admin-form.service', () => { form_logics: [ { _id: logicId1, - logicType: 'showFields', + logicType: LogicType.ShowFields, } as ILogicSchema, { _id: logicId2, - logicType: 'showFields', + logicType: LogicType.ShowFields, } as ILogicSchema, ], } const updatedLogic = { _id: logicId1, - logicType: 'preventSubmit', + logicType: LogicType.PreventSubmit, } as ILogicSchema const mockFormLogicUpdated = { form_logics: [ { _id: logicId1, - logicType: 'preventSubmit', + logicType: LogicType.PreventSubmit, } as ILogicSchema, { _id: logicId2, - logicType: 'showFields', + logicType: LogicType.ShowFields, } as ILogicSchema, ], } From eb8e1baaec3b8f34e102128ff9d28436caa93e1a Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Mon, 17 May 2021 09:50:38 +0800 Subject: [PATCH 07/18] refactor: use $watchCollection instead of manually triggering $watch by recreating form_logics array --- .../admin/components/edit-logic.client.component.js | 4 +--- .../edit-logic-modal.client.controller.js | 12 ++++-------- .../admin/directives/edit-form.client.directive.js | 11 ++++------- 3 files changed, 9 insertions(+), 18 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 e7b3bf9312..78f5a88e7f 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 @@ -84,9 +84,7 @@ function editLogicComponentController($uibModal, FormFields, Toastr, $q) { const logicIdToDelete = vm.myform.form_logics[logicIndex]._id $q.when(AdminFormService.deleteFormLogic(vm.myform._id, logicIdToDelete)) .then(() => { - const updatedLogic = [...vm.myform.form_logics] - updatedLogic.splice(logicIndex, 1) - vm.myform.form_logics = updatedLogic + vm.myform.form_logics.splice(logicIndex, 1) }) .catch((logicDeleteError) => { console.error(logicDeleteError) diff --git a/src/public/modules/forms/admin/controllers/edit-logic-modal.client.controller.js b/src/public/modules/forms/admin/controllers/edit-logic-modal.client.controller.js index 34fe39ce35..58926860d4 100644 --- a/src/public/modules/forms/admin/controllers/edit-logic-modal.client.controller.js +++ b/src/public/modules/forms/admin/controllers/edit-logic-modal.client.controller.js @@ -274,10 +274,8 @@ function EditLogicModalController( vm.createNewLogic = function (newLogic) { $q.when(AdminFormService.createFormLogic(vm.myform._id, newLogic)) .then((createdLogic) => { - const updatedFormLogics = [...vm.formLogics] - updatedFormLogics.push(createdLogic) - vm.formLogics = updatedFormLogics - externalScope.myform.form_logics = updatedFormLogics // update global myform + vm.formLogics.push(createdLogic) + externalScope.myform.form_logics.push(createdLogic) // update global myform $uibModalInstance.close() }) .catch((logicCreateError) => { @@ -300,10 +298,8 @@ function EditLogicModalController( ), ) .then((updatedLogic) => { - const updatedFormLogics = [...vm.formLogics] - updatedFormLogics[logicIndex] = updatedLogic - vm.formLogics = updatedFormLogics - externalScope.myform.form_logics = updatedFormLogics // update global myform + vm.formLogics[logicIndex] = updatedLogic + externalScope.myform.form_logics[logicIndex] = updatedLogic // update global myform $uibModalInstance.close() }) .catch((logicUpdateError) => { diff --git a/src/public/modules/forms/admin/directives/edit-form.client.directive.js b/src/public/modules/forms/admin/directives/edit-form.client.directive.js index 81caa1619f..673d63f370 100644 --- a/src/public/modules/forms/admin/directives/edit-form.client.directive.js +++ b/src/public/modules/forms/admin/directives/edit-form.client.directive.js @@ -98,13 +98,10 @@ function editFormController( let hiddenFieldSet = new Set() // To monitor which fields should be hidden because of form logic let conditionFieldSet = new Set() // To monitor which fields are conditions in the form logic // On change of form logic (scope.myform.form_logics), update our logic monitors - $scope.$watch( - (scope) => scope.myform.form_logics, - function (_newVal, _oldVal) { - updateHiddenFieldSet() - updateConditionFieldSet() - }, - ) + $scope.$watchCollection('myform.form_logics', function (_newVal, _oldVal) { + updateHiddenFieldSet() + updateConditionFieldSet() + }) // Loop through form_logics to identify which fields should be hidden const updateHiddenFieldSet = function () { From 8575c6ab9db820b5b9d94009360cd38afa64119f Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Mon, 17 May 2021 09:50:38 +0800 Subject: [PATCH 08/18] chore: update joi validation types --- .../modules/form/admin-form/admin-form.controller.ts | 11 +++++++++-- 1 file changed, 9 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 b989a547db..6e9f111514 100644 --- a/src/app/modules/form/admin-form/admin-form.controller.ts +++ b/src/app/modules/form/admin-form/admin-form.controller.ts @@ -1713,7 +1713,14 @@ export const handleCreateLogic = [ state: Joi.string() .valid(...Object.values(LogicConditionState)) .required(), - value: Joi.any().required(), + value: Joi.alternatives() + .try( + Joi.number(), + Joi.string(), + Joi.array().items(Joi.string()), + Joi.array().items(Joi.number()), + ) + .required(), ifValueType: Joi.valid(...Object.values(LogicIfValue)), }).unknown(true), ) @@ -1724,7 +1731,7 @@ export const handleCreateLogic = [ }), preventSubmitMessage: Joi.alternatives().conditional('logicType', { is: LogicType.PreventSubmit, - then: Joi.string(), + then: Joi.string().required(), }), // Allow other field related key-values to be provided and let the model // layer handle the validation. From 7d3ab5b8b30c2ba8d432719eefe6546f6fb1a714 Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Mon, 17 May 2021 09:50:38 +0800 Subject: [PATCH 09/18] docs: getLastLogic --- src/app/modules/form/form.utils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/modules/form/form.utils.ts b/src/app/modules/form/form.utils.ts index 488da3f559..72f6b43cc8 100644 --- a/src/app/modules/form/form.utils.ts +++ b/src/app/modules/form/form.utils.ts @@ -125,7 +125,7 @@ export const getLogicById = ( /** * Returns the last form logic in form_logics array * @param form_logics the logics - * @returns the last logic if exists, `null` otherwise + * @returns the last logic if it exists in form_logics, `null` otherwise */ export const getLastLogic = ( form_logics: IFormSchema['form_logics'], From ef7e9c65c03fb740e9ded5eaefec0015c76c0f8e Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Mon, 17 May 2021 09:50:38 +0800 Subject: [PATCH 10/18] chore: not cast _id to string --- .../admin-form/__tests__/admin-form.service.spec.ts | 10 ++-------- src/app/modules/form/admin-form/admin-form.service.ts | 2 +- 2 files changed, 3 insertions(+), 9 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 b40f7a4d38..aae0c14793 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 @@ -1693,10 +1693,7 @@ describe('admin-form.service', () => { expect.objectContaining(createdLogic), ) - expect(CREATE_SPY).toHaveBeenCalledWith( - mockEmailForm._id.toHexString(), - createdLogic, - ) + expect(CREATE_SPY).toHaveBeenCalledWith(mockEmailForm._id, createdLogic) }) it('should return ok(created logic) on successful form logic create for encrypt mode form', async () => { @@ -1712,10 +1709,7 @@ describe('admin-form.service', () => { expect.objectContaining(createdLogic), ) - expect(CREATE_SPY).toHaveBeenCalledWith( - mockEncryptFormId.toHexString(), - createdLogic, - ) + expect(CREATE_SPY).toHaveBeenCalledWith(mockEncryptFormId, createdLogic) }) }) 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 b99e4175eb..25e52086b3 100644 --- a/src/app/modules/form/admin-form/admin-form.service.ts +++ b/src/app/modules/form/admin-form/admin-form.service.ts @@ -730,7 +730,7 @@ export const createFormLogic = ( ): ResultAsync => { // Create new form logic return ResultAsync.fromPromise( - FormModel.createFormLogic(form._id.toHexString(), createdLogic), + FormModel.createFormLogic(form._id, createdLogic), (error) => { logger.error({ message: 'Error occurred when creating form logic', From f856e60bb9a0d3804d42b144b2566562cf08a5c1 Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Mon, 17 May 2021 09:50:38 +0800 Subject: [PATCH 11/18] chore: fix joi validation typings --- src/app/modules/form/admin-form/admin-form.controller.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) 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 6e9f111514..81e1f2a1d2 100644 --- a/src/app/modules/form/admin-form/admin-form.controller.ts +++ b/src/app/modules/form/admin-form/admin-form.controller.ts @@ -1721,7 +1721,9 @@ export const handleCreateLogic = [ Joi.array().items(Joi.number()), ) .required(), - ifValueType: Joi.valid(...Object.values(LogicIfValue)), + ifValueType: Joi.string() + .valid(...Object.values(LogicIfValue)) + .required(), }).unknown(true), ) .required(), From 3084cd7d6c3abcb6ce60970f0eacc7c98f92d8db Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Mon, 17 May 2021 09:50:38 +0800 Subject: [PATCH 12/18] refactor: use lodash last --- .../modules/form/admin-form/admin-form.service.ts | 6 +++--- src/app/modules/form/form.utils.ts | 15 --------------- 2 files changed, 3 insertions(+), 18 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 25e52086b3..1695961e7b 100644 --- a/src/app/modules/form/admin-form/admin-form.service.ts +++ b/src/app/modules/form/admin-form/admin-form.service.ts @@ -1,5 +1,5 @@ import { PresignedPost } from 'aws-sdk/clients/s3' -import { assignIn, last, omit } from 'lodash' +import _, { assignIn, last, omit } from 'lodash' import mongoose from 'mongoose' import { err, errAsync, ok, okAsync, Result, ResultAsync } from 'neverthrow' import { Except, Merge } from 'type-fest' @@ -52,7 +52,7 @@ import { TransferOwnershipError, } from '../form.errors' import { getFormModelByResponseMode } from '../form.service' -import { getFormFieldById, getLastLogic, getLogicById } from '../form.utils' +import { getFormFieldById, getLogicById } from '../form.utils' import { PRESIGNED_POST_EXPIRY_SECS } from './admin-form.constants' import { @@ -748,7 +748,7 @@ export const createFormLogic = ( if (!updatedForm) { return errAsync(new FormNotFoundError()) } - const createdLogic = getLastLogic(updatedForm.form_logics) + const createdLogic = _.last(updatedForm.form_logics) return createdLogic ? okAsync(createdLogic) : errAsync(new DatabaseError()) }) } diff --git a/src/app/modules/form/form.utils.ts b/src/app/modules/form/form.utils.ts index 72f6b43cc8..e80a70022c 100644 --- a/src/app/modules/form/form.utils.ts +++ b/src/app/modules/form/form.utils.ts @@ -121,18 +121,3 @@ export const getLogicById = ( return form_logics.find((logic) => logicId === String(logic._id)) ?? null } - -/** - * Returns the last form logic in form_logics array - * @param form_logics the logics - * @returns the last logic if it exists in form_logics, `null` otherwise - */ -export const getLastLogic = ( - form_logics: IFormSchema['form_logics'], -): ILogicSchema | null => { - if (!form_logics) { - return null - } - - return form_logics[form_logics.length - 1] ?? null -} From 1170fcfc329226b225dff82bd3a8ef0a85bf0711 Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Mon, 17 May 2021 09:50:38 +0800 Subject: [PATCH 13/18] refactor: extract joiLogicBody --- .../form/admin-form/admin-form.controller.ts | 104 +++++++----------- 1 file changed, 40 insertions(+), 64 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 81e1f2a1d2..132d35a451 100644 --- a/src/app/modules/form/admin-form/admin-form.controller.ts +++ b/src/app/modules/form/admin-form/admin-form.controller.ts @@ -1697,44 +1697,51 @@ export const _handleCreateLogic: RequestHandler< ) } +/** + * Shape of request body used for joi validation for create and update logic + */ +const joiLogicBody = { + logicType: Joi.string() + .valid(...Object.values(LogicType)) + .required(), + conditions: Joi.array() + .items( + Joi.object({ + field: Joi.string().required(), + state: Joi.string() + .valid(...Object.values(LogicConditionState)) + .required(), + value: Joi.alternatives() + .try( + Joi.number(), + Joi.string(), + Joi.array().items(Joi.string()), + Joi.array().items(Joi.number()), + ) + .required(), + ifValueType: Joi.string() + .valid(...Object.values(LogicIfValue)) + .required(), + }).unknown(true), + ) + .required(), + show: Joi.alternatives().conditional('logicType', { + is: LogicType.ShowFields, + then: Joi.array().items(Joi.string()).required(), + }), + preventSubmitMessage: Joi.alternatives().conditional('logicType', { + is: LogicType.PreventSubmit, + then: Joi.string().required(), + }), +} + /** * Handler for POST /forms/:formId/logic */ export const handleCreateLogic = [ celebrate({ [Segments.BODY]: Joi.object({ - logicType: Joi.string() - .valid(...Object.values(LogicType)) - .required(), - conditions: Joi.array() - .items( - Joi.object({ - field: Joi.string().required(), - state: Joi.string() - .valid(...Object.values(LogicConditionState)) - .required(), - value: Joi.alternatives() - .try( - Joi.number(), - Joi.string(), - Joi.array().items(Joi.string()), - Joi.array().items(Joi.number()), - ) - .required(), - ifValueType: Joi.string() - .valid(...Object.values(LogicIfValue)) - .required(), - }).unknown(true), - ) - .required(), - show: Joi.alternatives().conditional('logicType', { - is: LogicType.ShowFields, - then: Joi.array().items(Joi.string()).required(), - }), - preventSubmitMessage: Joi.alternatives().conditional('logicType', { - is: LogicType.PreventSubmit, - then: Joi.string().required(), - }), + ...joiLogicBody, // Allow other field related key-values to be provided and let the model // layer handle the validation. }).unknown(true), @@ -1950,38 +1957,7 @@ export const handleUpdateLogic = [ [Segments.BODY]: Joi.object({ // Ensures given logic is same as accessed logic _id: Joi.string().valid(Joi.ref('$params.logicId')).required(), - logicType: Joi.string() - .valid(...Object.values(LogicType)) - .required(), - conditions: Joi.array() - .items( - Joi.object({ - field: Joi.string().required(), - state: Joi.string() - .valid(...Object.values(LogicConditionState)) - .required(), - value: Joi.alternatives() - .try( - Joi.number(), - Joi.string(), - Joi.array().items(Joi.string()), - Joi.array().items(Joi.number()), - ) - .required(), - ifValueType: Joi.string() - .valid(...Object.values(LogicIfValue)) - .required(), - }).unknown(true), - ) - .required(), - show: Joi.alternatives().conditional('logicType', { - is: LogicType.ShowFields, - then: Joi.array().items(Joi.string()).required(), - }), - preventSubmitMessage: Joi.alternatives().conditional('logicType', { - is: LogicType.PreventSubmit, - then: Joi.string().required(), - }), + ...joiLogicBody, // Allow other field related key-values to be provided and let the model // layer handle the validation. }).unknown(true), From 809844e271cf685a4c74c04f8bd7b08025f5f324 Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Mon, 17 May 2021 09:50:38 +0800 Subject: [PATCH 14/18] chore: add error test cases --- .../__tests__/admin-form.service.spec.ts | 52 ++++++++++++++++++- 1 file changed, 51 insertions(+), 1 deletion(-) 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 aae0c14793..636c8b3f13 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 @@ -1,7 +1,7 @@ /* eslint-disable @typescript-eslint/ban-ts-comment */ import { PresignedPost } from 'aws-sdk/clients/s3' import { ObjectId } from 'bson-ext' -import { assignIn, cloneDeep, merge, omit, pick } from 'lodash' +import _, { assignIn, cloneDeep, merge, omit, pick } from 'lodash' import mongoose from 'mongoose' import { err, errAsync, ok, okAsync } from 'neverthrow' import { mocked } from 'ts-jest/utils' @@ -1711,6 +1711,56 @@ describe('admin-form.service', () => { expect(CREATE_SPY).toHaveBeenCalledWith(mockEncryptFormId, createdLogic) }) + + it('should return err(FormNotFoundError) if db does not return form object', async () => { + // Arrange + CREATE_SPY.mockResolvedValue((undefined as unknown) as IFormSchema) + + // Act + const actualResult = await createFormLogic(mockEncryptForm, createdLogic) + + // Assert + expect(actualResult.isErr()).toEqual(true) + expect(actualResult._unsafeUnwrapErr()).toEqual(new FormNotFoundError()) + + expect(CREATE_SPY).toHaveBeenCalledWith(mockEncryptFormId, createdLogic) + }) + + it('should return err(DatabaseError) if db returns form object that does not have form_logics array', async () => { + // Arrange + const updatedFormWithoutLogic = _.omit( + mockEncryptFormUpdated, + 'form_logics', + ) + CREATE_SPY.mockResolvedValue(updatedFormWithoutLogic as IFormSchema) + + // Act + const actualResult = await createFormLogic(mockEncryptForm, createdLogic) + + // Assert + expect(actualResult.isErr()).toEqual(true) + expect(actualResult._unsafeUnwrapErr()).toEqual(new DatabaseError()) + + expect(CREATE_SPY).toHaveBeenCalledWith(mockEncryptFormId, createdLogic) + }) + + it('should return err(DatabaseError) if db returns form object that has empty form_logics array', async () => { + // Arrange + const updatedFormWithEmptyLogic = { + ...mockEncryptFormUpdated, + form_logics: [], + } + CREATE_SPY.mockResolvedValue(updatedFormWithEmptyLogic as IFormSchema) + + // Act + const actualResult = await createFormLogic(mockEncryptForm, createdLogic) + + // Assert + expect(actualResult.isErr()).toEqual(true) + expect(actualResult._unsafeUnwrapErr()).toEqual(new DatabaseError()) + + expect(CREATE_SPY).toHaveBeenCalledWith(mockEncryptFormId, createdLogic) + }) }) describe('deleteFormField', () => { From cb7ea735f7548316e37492eea0f1fd4fd057f0ec Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Mon, 17 May 2021 09:50:38 +0800 Subject: [PATCH 15/18] chore: remove console.error --- .../admin/controllers/edit-logic-modal.client.controller.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/public/modules/forms/admin/controllers/edit-logic-modal.client.controller.js b/src/public/modules/forms/admin/controllers/edit-logic-modal.client.controller.js index 58926860d4..19f88c5561 100644 --- a/src/public/modules/forms/admin/controllers/edit-logic-modal.client.controller.js +++ b/src/public/modules/forms/admin/controllers/edit-logic-modal.client.controller.js @@ -278,8 +278,7 @@ function EditLogicModalController( externalScope.myform.form_logics.push(createdLogic) // update global myform $uibModalInstance.close() }) - .catch((logicCreateError) => { - console.error(logicCreateError) + .catch(() => { Toastr.error('Failed to create logic, please refresh and try again!') }) } @@ -302,8 +301,7 @@ function EditLogicModalController( externalScope.myform.form_logics[logicIndex] = updatedLogic // update global myform $uibModalInstance.close() }) - .catch((logicUpdateError) => { - console.error(logicUpdateError) + .catch(() => { Toastr.error('Failed to update logic, please refresh and try again!') }) } From 9940b963ea92a369681c10f2a1455c7c9fbffb19 Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Mon, 17 May 2021 09:50:38 +0800 Subject: [PATCH 16/18] refactor: import destructured function --- .../form/admin-form/__tests__/admin-form.service.spec.ts | 4 ++-- src/app/modules/form/admin-form/admin-form.service.ts | 4 ++-- 2 files changed, 4 insertions(+), 4 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 636c8b3f13..ecc28e0bcb 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 @@ -1,7 +1,7 @@ /* eslint-disable @typescript-eslint/ban-ts-comment */ import { PresignedPost } from 'aws-sdk/clients/s3' import { ObjectId } from 'bson-ext' -import _, { assignIn, cloneDeep, merge, omit, pick } from 'lodash' +import { assignIn, cloneDeep, merge, omit, pick } from 'lodash' import mongoose from 'mongoose' import { err, errAsync, ok, okAsync } from 'neverthrow' import { mocked } from 'ts-jest/utils' @@ -1728,7 +1728,7 @@ describe('admin-form.service', () => { it('should return err(DatabaseError) if db returns form object that does not have form_logics array', async () => { // Arrange - const updatedFormWithoutLogic = _.omit( + const updatedFormWithoutLogic = omit( mockEncryptFormUpdated, 'form_logics', ) 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 1695961e7b..456d12f7eb 100644 --- a/src/app/modules/form/admin-form/admin-form.service.ts +++ b/src/app/modules/form/admin-form/admin-form.service.ts @@ -1,5 +1,5 @@ import { PresignedPost } from 'aws-sdk/clients/s3' -import _, { assignIn, last, omit } from 'lodash' +import { assignIn, last, omit } from 'lodash' import mongoose from 'mongoose' import { err, errAsync, ok, okAsync, Result, ResultAsync } from 'neverthrow' import { Except, Merge } from 'type-fest' @@ -748,7 +748,7 @@ export const createFormLogic = ( if (!updatedForm) { return errAsync(new FormNotFoundError()) } - const createdLogic = _.last(updatedForm.form_logics) + const createdLogic = last(updatedForm.form_logics) return createdLogic ? okAsync(createdLogic) : errAsync(new DatabaseError()) }) } From c8202223fa639d4945384e0b22034bfd20cd355a Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Mon, 17 May 2021 09:50:38 +0800 Subject: [PATCH 17/18] chore: tighten validation --- .../modules/form/admin-form/admin-form.controller.ts | 10 ++-------- 1 file changed, 2 insertions(+), 8 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 132d35a451..2263f23268 100644 --- a/src/app/modules/form/admin-form/admin-form.controller.ts +++ b/src/app/modules/form/admin-form/admin-form.controller.ts @@ -1740,11 +1740,7 @@ const joiLogicBody = { */ export const handleCreateLogic = [ celebrate({ - [Segments.BODY]: Joi.object({ - ...joiLogicBody, - // Allow other field related key-values to be provided and let the model - // layer handle the validation. - }).unknown(true), + [Segments.BODY]: joiLogicBody, }), _handleCreateLogic, ] as RequestHandler[] @@ -1958,9 +1954,7 @@ export const handleUpdateLogic = [ // Ensures given logic is same as accessed logic _id: Joi.string().valid(Joi.ref('$params.logicId')).required(), ...joiLogicBody, - // Allow other field related key-values to be provided and let the model - // layer handle the validation. - }).unknown(true), + }), }, undefined, // Required so req.body can be validated against values in req.params. From 34d09caa24df4993e11ff3961830d37dde3ddfb2 Mon Sep 17 00:00:00 2001 From: shuli-ogp Date: Mon, 17 May 2021 09:50:38 +0800 Subject: [PATCH 18/18] chore: rename to createLogicBody --- src/app/models/form.server.model.ts | 4 +- .../__tests__/admin-form.controller.spec.ts | 10 ++-- .../__tests__/admin-form.service.spec.ts | 53 ++++++++++++++----- .../form/admin-form/admin-form.controller.ts | 6 +-- .../form/admin-form/admin-form.service.ts | 8 +-- src/public/services/AdminFormService.ts | 4 +- src/types/form.ts | 2 +- 7 files changed, 57 insertions(+), 30 deletions(-) diff --git a/src/app/models/form.server.model.ts b/src/app/models/form.server.model.ts index bbe914185c..187e7c15a5 100644 --- a/src/app/models/form.server.model.ts +++ b/src/app/models/form.server.model.ts @@ -685,11 +685,11 @@ const compileFormModel = (db: Mongoose): IFormModel => { FormSchema.statics.createFormLogic = async function ( this: IFormModel, formId: string, - createdLogic: LogicDto, + createLogicBody: LogicDto, ): Promise { return this.findByIdAndUpdate( formId, - { $push: { form_logics: createdLogic } }, + { $push: { form_logics: createLogicBody } }, { new: true, runValidators: true, 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 7831f2098d..74ba19b88a 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 @@ -7842,7 +7842,7 @@ describe('admin-form.controller', () => { ], } - const mockCreatedLogic = { + const mockCreateLogicBody = { _id: logicId, } as ILogicSchema @@ -7863,7 +7863,7 @@ describe('admin-form.controller', () => { _id: MOCK_USER_ID, }, }, - body: mockCreatedLogic, + body: mockCreateLogicBody, }) const mockRes = expressHandler.mockResponse() @@ -7873,7 +7873,7 @@ describe('admin-form.controller', () => { okAsync(MOCK_FORM), ) MockAdminFormService.createFormLogic.mockReturnValue( - okAsync(mockCreatedLogic), + okAsync(mockCreateLogicBody), ) }) @@ -7892,11 +7892,11 @@ describe('admin-form.controller', () => { ) expect(MockAdminFormService.createFormLogic).toHaveBeenCalledWith( MOCK_FORM, - mockCreatedLogic, + mockCreateLogicBody, ) expect(mockRes.status).toHaveBeenCalledWith(200) - expect(mockRes.json).toHaveBeenCalledWith(mockCreatedLogic) + expect(mockRes.json).toHaveBeenCalledWith(mockCreateLogicBody) }) it('should return 403 when user does not have permissions to update logic', async () => { 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 ecc28e0bcb..d873e71e08 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 @@ -1629,7 +1629,7 @@ describe('admin-form.service', () => { ], } - const createdLogic = { + const createLogicBody = { logicType: LogicType.PreventSubmit, } as ILogicSchema @@ -1685,15 +1685,18 @@ describe('admin-form.service', () => { CREATE_SPY.mockResolvedValue(mockEmailFormUpdated as IFormSchema) // Act - const actualResult = await createFormLogic(mockEmailForm, createdLogic) + const actualResult = await createFormLogic(mockEmailForm, createLogicBody) // Assert expect(actualResult.isOk()).toEqual(true) expect(actualResult._unsafeUnwrap()).toEqual( - expect.objectContaining(createdLogic), + expect.objectContaining(createLogicBody), ) - expect(CREATE_SPY).toHaveBeenCalledWith(mockEmailForm._id, createdLogic) + expect(CREATE_SPY).toHaveBeenCalledWith( + mockEmailForm._id, + createLogicBody, + ) }) it('should return ok(created logic) on successful form logic create for encrypt mode form', async () => { @@ -1701,15 +1704,21 @@ describe('admin-form.service', () => { CREATE_SPY.mockResolvedValue(mockEncryptFormUpdated as IFormSchema) // Act - const actualResult = await createFormLogic(mockEncryptForm, createdLogic) + const actualResult = await createFormLogic( + mockEncryptForm, + createLogicBody, + ) // Assert expect(actualResult.isOk()).toEqual(true) expect(actualResult._unsafeUnwrap()).toEqual( - expect.objectContaining(createdLogic), + expect.objectContaining(createLogicBody), ) - expect(CREATE_SPY).toHaveBeenCalledWith(mockEncryptFormId, createdLogic) + expect(CREATE_SPY).toHaveBeenCalledWith( + mockEncryptFormId, + createLogicBody, + ) }) it('should return err(FormNotFoundError) if db does not return form object', async () => { @@ -1717,13 +1726,19 @@ describe('admin-form.service', () => { CREATE_SPY.mockResolvedValue((undefined as unknown) as IFormSchema) // Act - const actualResult = await createFormLogic(mockEncryptForm, createdLogic) + const actualResult = await createFormLogic( + mockEncryptForm, + createLogicBody, + ) // Assert expect(actualResult.isErr()).toEqual(true) expect(actualResult._unsafeUnwrapErr()).toEqual(new FormNotFoundError()) - expect(CREATE_SPY).toHaveBeenCalledWith(mockEncryptFormId, createdLogic) + expect(CREATE_SPY).toHaveBeenCalledWith( + mockEncryptFormId, + createLogicBody, + ) }) it('should return err(DatabaseError) if db returns form object that does not have form_logics array', async () => { @@ -1735,13 +1750,19 @@ describe('admin-form.service', () => { CREATE_SPY.mockResolvedValue(updatedFormWithoutLogic as IFormSchema) // Act - const actualResult = await createFormLogic(mockEncryptForm, createdLogic) + const actualResult = await createFormLogic( + mockEncryptForm, + createLogicBody, + ) // Assert expect(actualResult.isErr()).toEqual(true) expect(actualResult._unsafeUnwrapErr()).toEqual(new DatabaseError()) - expect(CREATE_SPY).toHaveBeenCalledWith(mockEncryptFormId, createdLogic) + expect(CREATE_SPY).toHaveBeenCalledWith( + mockEncryptFormId, + createLogicBody, + ) }) it('should return err(DatabaseError) if db returns form object that has empty form_logics array', async () => { @@ -1753,13 +1774,19 @@ describe('admin-form.service', () => { CREATE_SPY.mockResolvedValue(updatedFormWithEmptyLogic as IFormSchema) // Act - const actualResult = await createFormLogic(mockEncryptForm, createdLogic) + const actualResult = await createFormLogic( + mockEncryptForm, + createLogicBody, + ) // Assert expect(actualResult.isErr()).toEqual(true) expect(actualResult._unsafeUnwrapErr()).toEqual(new DatabaseError()) - expect(CREATE_SPY).toHaveBeenCalledWith(mockEncryptFormId, createdLogic) + expect(CREATE_SPY).toHaveBeenCalledWith( + mockEncryptFormId, + createLogicBody, + ) }) }) 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 2263f23268..61b0427b18 100644 --- a/src/app/modules/form/admin-form/admin-form.controller.ts +++ b/src/app/modules/form/admin-form/admin-form.controller.ts @@ -1660,7 +1660,7 @@ export const _handleCreateLogic: RequestHandler< LogicDto > = (req, res) => { const { formId } = req.params - const createdLogic = req.body + const createLogicBody = req.body const sessionUserId = (req.session as Express.AuthedSession).user._id // Step 1: Retrieve currently logged in user. @@ -1676,7 +1676,7 @@ export const _handleCreateLogic: RequestHandler< ) // Step 3: Create form logic .andThen((retrievedForm) => - AdminFormService.createFormLogic(retrievedForm, createdLogic), + AdminFormService.createFormLogic(retrievedForm, createLogicBody), ) .map((createdLogic) => res.status(StatusCodes.OK).json(createdLogic)) .mapErr((error) => { @@ -1687,7 +1687,7 @@ export const _handleCreateLogic: RequestHandler< ...createReqMeta(req), userId: sessionUserId, formId, - createdLogic, + createLogicBody, }, error, }) 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 456d12f7eb..5242d5f91f 100644 --- a/src/app/modules/form/admin-form/admin-form.service.ts +++ b/src/app/modules/form/admin-form/admin-form.service.ts @@ -720,24 +720,24 @@ export const updateFormSettings = ( /** * Creates form logic. * @param form The original form to create logic in - * @param createdLogic Object containing the created logic + * @param createLogicBody Object containing the created logic * @returns ok(created logic dto) on success * @returns err(database errors) if db error is thrown during logic update */ export const createFormLogic = ( form: IPopulatedForm, - createdLogic: LogicDto, + createLogicBody: LogicDto, ): ResultAsync => { // Create new form logic return ResultAsync.fromPromise( - FormModel.createFormLogic(form._id, createdLogic), + FormModel.createFormLogic(form._id, createLogicBody), (error) => { logger.error({ message: 'Error occurred when creating form logic', meta: { action: 'createFormLogic', formId: form._id, - createdLogic, + createLogicBody, }, error, }) diff --git a/src/public/services/AdminFormService.ts b/src/public/services/AdminFormService.ts index d8582ee15d..57b71a3022 100644 --- a/src/public/services/AdminFormService.ts +++ b/src/public/services/AdminFormService.ts @@ -124,10 +124,10 @@ export const updateFormEndPage = async ( export const createFormLogic = async ( formId: string, - createdLogic: LogicDto, + createLogicBody: LogicDto, ): Promise => { return axios - .post(`${ADMIN_FORM_ENDPOINT}/${formId}/logic`, createdLogic) + .post(`${ADMIN_FORM_ENDPOINT}/${formId}/logic`, createLogicBody) .then(({ data }) => data) } diff --git a/src/types/form.ts b/src/types/form.ts index 59655a4c71..3dadbc1a56 100644 --- a/src/types/form.ts +++ b/src/types/form.ts @@ -303,7 +303,7 @@ export interface IFormModel extends Model { ): Promise createFormLogic( formId: string, - createdLogic: LogicDto, + createLogicBody: LogicDto, ): Promise deleteFormLogic(formId: string, logicId: string): Promise /**