From 47e3204c3076584576a8283b1b6423318b2e6a2c Mon Sep 17 00:00:00 2001 From: foochifa Date: Mon, 8 May 2023 17:43:49 +0800 Subject: [PATCH 01/12] feat: change form model duplicate method to insert at index --- src/app/models/form.server.model.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/app/models/form.server.model.ts b/src/app/models/form.server.model.ts index 3a0701b9b5..9796124bc4 100644 --- a/src/app/models/form.server.model.ts +++ b/src/app/models/form.server.model.ts @@ -700,15 +700,18 @@ const compileFormModel = (db: Mongoose): IFormModel => { return this.save() } - FormDocumentSchema.methods.duplicateFormFieldById = function ( + FormDocumentSchema.methods.duplicateFormFieldByIdAndIndex = function ( fieldId: string, + insertionIndex: number, ) { const fieldToDuplicate = getFormFieldById(this.form_fields, fieldId) if (!fieldToDuplicate) return Promise.resolve(null) const duplicatedField = omit(fieldToDuplicate, ['_id', 'globalId']) // eslint-disable-next-line @typescript-eslint/no-extra-semi - ;(this.form_fields as Types.DocumentArray).push( + ;(this.form_fields as Types.DocumentArray).splice( + insertionIndex, + 0, duplicatedField, ) return this.save() From 796c3fd71377f93ebb04e86b13d2cda92b383109 Mon Sep 17 00:00:00 2001 From: foochifa Date: Mon, 8 May 2023 17:45:46 +0800 Subject: [PATCH 02/12] chore: update form.ts with new duplicate method signature --- src/types/form.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/types/form.ts b/src/types/form.ts index 066bcef95d..c8d3ea43b8 100644 --- a/src/types/form.ts +++ b/src/types/form.ts @@ -130,12 +130,17 @@ export interface IFormSchema extends IForm, Document, PublicView { /** * Duplicates a form field into the form - * @param newField the fieldId of the field to duplicate + * @param fieldId the fieldId of the field to duplicate + * @param insertionIndex the index to insert the duplicated field in * @returns updated form after the duplication if field duplication is successful * @throws FieldNotFound error if field to duplicate does not exist */ - duplicateFormFieldById(this: T, fieldId: string): Promise + duplicateFormFieldByIdAndIndex( + this: T, + fieldId: string, + insertionIndex: number, + ): Promise /** * Reorders field corresponding to given fieldId to given newPosition From 93b6cb7e4790f50840995efc0bac41281869091e Mon Sep 17 00:00:00 2001 From: foochifa Date: Mon, 8 May 2023 17:46:59 +0800 Subject: [PATCH 03/12] feat: change duplicateFormField to duplicate at the immediate index after If index error occurs, will insert the duplicate field at the end of the form fields --- .../form/admin-form/admin-form.service.ts | 17 ++++++++++++++--- src/app/modules/form/form.utils.ts | 11 +++++++++++ 2 files changed, 25 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 2e63b53d9d..b5bdd04c5f 100644 --- a/src/app/modules/form/admin-form/admin-form.service.ts +++ b/src/app/modules/form/admin-form/admin-form.service.ts @@ -79,7 +79,12 @@ import { TransferOwnershipError, } from '../form.errors' import { getFormModelByResponseMode } from '../form.service' -import { getFormFieldById, getLogicById, isFormOnboarded } from '../form.utils' +import { + getFormFieldById, + getFormFieldIndexById, + getLogicById, + isFormOnboarded, +} from '../form.utils' import { TwilioCredentials, @@ -531,8 +536,12 @@ export const duplicateFormField = ( FormFieldSchema, PossibleDatabaseError | FormNotFoundError | FieldNotFoundError > => { + const fieldIndex = getFormFieldIndexById(form.form_fields, fieldId) + // if fieldIndex does not exist, append to end of form fields + const insertionIndex = + fieldIndex === null ? form.form_fields.length + 1 : fieldIndex + 1 return ResultAsync.fromPromise( - form.duplicateFormFieldById(fieldId), + form.duplicateFormFieldByIdAndIndex(fieldId, insertionIndex), (error) => { logger.error({ message: 'Error encountered while duplicating form field', @@ -540,6 +549,8 @@ export const duplicateFormField = ( action: 'duplicateFormField', formId: form._id, fieldId, + fieldIndex, + insertionIndex, }, error, }) @@ -555,7 +566,7 @@ export const duplicateFormField = ( errAsync(new FormNotFoundError()), ) } - const updatedField = last(updatedForm.form_fields) + const updatedField = updatedForm.form_fields[insertionIndex] return updatedField ? okAsync(updatedField) : errAsync(new FieldNotFoundError()) diff --git a/src/app/modules/form/form.utils.ts b/src/app/modules/form/form.utils.ts index c13c6f36d1..f421e61ba5 100644 --- a/src/app/modules/form/form.utils.ts +++ b/src/app/modules/form/form.utils.ts @@ -105,6 +105,17 @@ export const getFormFieldById = ( return formFields.find((f) => fieldId === String(f._id)) ?? null } +export const getFormFieldIndexById = ( + formFields: IFormSchema['form_fields'], + fieldId: FormFieldSchema['_id'], +): number | null => { + if (!formFields) { + return null + } + + return formFields.findIndex((o) => o._id.equals(fieldId)) +} + /** * Finds and returns form logic in given form by its id * @param form_logics the logics to search from From 4efd841adda3b0463858e87cbd1b0bb9c3bd2b92 Mon Sep 17 00:00:00 2001 From: foochifa Date: Mon, 8 May 2023 17:47:54 +0800 Subject: [PATCH 04/12] feat: update use query client to duplicate at following index --- .../builder-and-design/mutations/useDuplicateFormField.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/frontend/src/features/admin-form/create/builder-and-design/mutations/useDuplicateFormField.ts b/frontend/src/features/admin-form/create/builder-and-design/mutations/useDuplicateFormField.ts index 5ad6c183ca..82f803c62a 100644 --- a/frontend/src/features/admin-form/create/builder-and-design/mutations/useDuplicateFormField.ts +++ b/frontend/src/features/admin-form/create/builder-and-design/mutations/useDuplicateFormField.ts @@ -60,7 +60,9 @@ export const useDuplicateFormField = () => { // Should not happen, should not be able to update field if there is no // existing data. if (!oldForm) throw new Error('Query should have been set') - oldForm.form_fields.push(newField) + const insertionIndex = + oldForm.form_fields.findIndex((o) => o._id === fieldId) + 1 + oldForm.form_fields.splice(insertionIndex, 0, newField) return oldForm }) // Switch to editing new field From 591d3c2bb2abc04f05ee76702b2d54f99221fef4 Mon Sep 17 00:00:00 2001 From: foochifa Date: Mon, 8 May 2023 18:19:19 +0800 Subject: [PATCH 05/12] fix: use String typecast instead of equals Not recommended, but issue will be solved with later mongoose versions >=6 which allows string comparison against ObjectId. --- 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 f421e61ba5..a8a668060e 100644 --- a/src/app/modules/form/form.utils.ts +++ b/src/app/modules/form/form.utils.ts @@ -113,7 +113,7 @@ export const getFormFieldIndexById = ( return null } - return formFields.findIndex((o) => o._id.equals(fieldId)) + return formFields.findIndex((f) => fieldId === String(f._id)) } /** From 468ef0e070c04d9cdd6069001b47d6ac8a164cdd Mon Sep 17 00:00:00 2001 From: foochifa Date: Mon, 8 May 2023 18:19:56 +0800 Subject: [PATCH 06/12] tests: fix tests cases for form model and admin-form service --- src/app/models/__tests__/form.server.model.spec.ts | 10 +++++++--- .../admin-form/__tests__/admin-form.service.spec.ts | 10 ++++++---- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/src/app/models/__tests__/form.server.model.spec.ts b/src/app/models/__tests__/form.server.model.spec.ts index 9376c25637..0e7262171b 100644 --- a/src/app/models/__tests__/form.server.model.spec.ts +++ b/src/app/models/__tests__/form.server.model.spec.ts @@ -2703,7 +2703,7 @@ describe('Form Model', () => { }) }) - describe('duplicateFormFieldById', () => { + describe('duplicateFormFieldByIdAndIndex', () => { it('should return updated document with duplicated form field', async () => { // Arrange const fieldToDuplicate = generateDefaultField(BasicField.Checkbox) @@ -2712,7 +2712,10 @@ describe('Form Model', () => { const fieldId = fieldToDuplicate._id // Act - const actual = await validForm.duplicateFormFieldById(fieldId) + const actual = await validForm.duplicateFormFieldByIdAndIndex( + fieldId, + 1, + ) // @ts-ignore const actualDuplicatedField = omit(actual?.form_fields.toObject()[1], [ '_id', @@ -2736,8 +2739,9 @@ describe('Form Model', () => { }) it('should return null if given fieldId is invalid', async () => { - const updatedForm = await validForm.duplicateFormFieldById( + const updatedForm = await validForm.duplicateFormFieldByIdAndIndex( new ObjectId().toHexString(), + 0, ) // Assert 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 49617427a8..6d58e82d8d 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 @@ -1552,14 +1552,16 @@ describe('admin-form.service', () => { const duplicatedField = generateDefaultField(BasicField.Mobile) const mockUpdatedForm = { title: 'some mock form', - // Append duplicated field to end of form_fields. + // Append duplicated field to after target field to duplicate. form_fields: [fieldToDuplicate, duplicatedField], } as IFormSchema const mockForm = { title: 'some mock form', form_fields: [fieldToDuplicate], _id: new ObjectId(), - duplicateFormFieldById: jest.fn().mockResolvedValue(mockUpdatedForm), + duplicateFormFieldByIdAndIndex: jest + .fn() + .mockResolvedValue(mockUpdatedForm), } as unknown as IPopulatedForm // Act @@ -1589,7 +1591,7 @@ describe('admin-form.service', () => { title: 'some mock form', form_fields: [fieldToDuplicate], _id: new ObjectId(), - duplicateFormFieldById: jest.fn().mockResolvedValue(null), + duplicateFormFieldByIdAndIndex: jest.fn().mockResolvedValue(null), } as unknown as IPopulatedForm // Act @@ -1608,7 +1610,7 @@ describe('admin-form.service', () => { const mockForm = { title: 'some mock form', form_fields: initialFields, - duplicateFormFieldById: jest.fn().mockRejectedValue( + duplicateFormFieldByIdAndIndex: jest.fn().mockRejectedValue( // @ts-ignore new mongoose.Error.ValidationError({ errors: 'does not matter' }), ), From 7486c9d662d91dc25da941fff906120353c093f0 Mon Sep 17 00:00:00 2001 From: foochifa Date: Tue, 9 May 2023 09:57:17 +0800 Subject: [PATCH 07/12] fix: safeguard for get field index --- src/app/modules/form/form.utils.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/app/modules/form/form.utils.ts b/src/app/modules/form/form.utils.ts index a8a668060e..e6b4dab9fc 100644 --- a/src/app/modules/form/form.utils.ts +++ b/src/app/modules/form/form.utils.ts @@ -113,7 +113,9 @@ export const getFormFieldIndexById = ( return null } - return formFields.findIndex((f) => fieldId === String(f._id)) + const index = formFields.findIndex((f) => fieldId === String(f._id)) + // if index is negative, fieldId does not exist in formFields + return index >= 0 ? index : null } /** From 7a38e9f74d35b806100a0ec5a84060699f436b7a Mon Sep 17 00:00:00 2001 From: foochifa Date: Tue, 9 May 2023 09:58:01 +0800 Subject: [PATCH 08/12] test: add tests for getFormFieldsIndexById --- .../modules/form/__tests__/form.utils.spec.ts | 51 ++++++++++++++++++- 1 file changed, 50 insertions(+), 1 deletion(-) diff --git a/src/app/modules/form/__tests__/form.utils.spec.ts b/src/app/modules/form/__tests__/form.utils.spec.ts index 9d6cd70a3b..c75e87e18a 100644 --- a/src/app/modules/form/__tests__/form.utils.spec.ts +++ b/src/app/modules/form/__tests__/form.utils.spec.ts @@ -6,7 +6,11 @@ import { FormFieldSchema } from 'src/types' import { generateDefaultField } from 'tests/unit/backend/helpers/generate-form-data' import { BasicField, FormPermission } from '../../../../../shared/types' -import { getCollabEmailsWithPermission, getFormFieldById } from '../form.utils' +import { + getCollabEmailsWithPermission, + getFormFieldById, + getFormFieldIndexById, +} from '../form.utils' const MOCK_EMAIL_1 = 'a@abc.com' const MOCK_EMAIL_2 = 'b@def.com' @@ -107,4 +111,49 @@ describe('form.utils', () => { expect(result).toEqual(null) }) }) + + describe('getFormFieldIndexById', () => { + it('should return index of the field when form fields given is a primitive array', async () => { + // Arrange + const fieldToFind = generateDefaultField(BasicField.HomeNo) + const formFields = [ + generateDefaultField(BasicField.Date), + fieldToFind, + generateDefaultField(BasicField.Date), + ] + const expectedIndex = 1 + + // Act + const result = getFormFieldIndexById(formFields, fieldToFind._id) + + // Assert + expect(result).toEqual(expectedIndex) + }) + + it('should return null when given form fields are undefined', async () => { + // Arrange + const someFieldId = new ObjectId() + + // Act + const result = getFormFieldIndexById(undefined, someFieldId) + + // Assert + expect(result).toEqual(null) + }) + + it('should return null when no fields correspond to given field id', async () => { + // Arrange + const invalidFieldId = new ObjectId() + const formFields = [ + generateDefaultField(BasicField.Date), + generateDefaultField(BasicField.Date), + ] + + // Act + const result = getFormFieldById(formFields, invalidFieldId) + + // Assert + expect(result).toEqual(null) + }) + }) }) From 0a8c9095ef44d716ec897c2b38235baa5d1cca12 Mon Sep 17 00:00:00 2001 From: foochifa Date: Tue, 9 May 2023 10:43:42 +0800 Subject: [PATCH 09/12] test: add test for duplicating at target index --- .../__tests__/form.server.model.spec.ts | 36 +++++++++++++++++++ .../modules/form/__tests__/form.utils.spec.ts | 2 +- 2 files changed, 37 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 0e7262171b..3db5a2d9e4 100644 --- a/src/app/models/__tests__/form.server.model.spec.ts +++ b/src/app/models/__tests__/form.server.model.spec.ts @@ -2738,6 +2738,42 @@ describe('Form Model', () => { expect(actualDuplicatedField).toEqual(expectedDuplicatedField) }) + it('should duplicate form field at the target index', async () => { + // Arrange + const fieldToDuplicate = generateDefaultField(BasicField.Checkbox) + const dummyField = generateDefaultField(BasicField.Mobile) + + validForm.form_fields = [fieldToDuplicate, dummyField] + const fieldId = fieldToDuplicate._id + + // Act + const actual = await validForm.duplicateFormFieldByIdAndIndex( + fieldId, + 1, + ) + // actual duplicated field should be at index 1 + // @ts-ignore + const actualDuplicatedField = omit(actual?.form_fields.toObject()[1], [ + '_id', + 'globalId', + ]) // do not compare _id and globalId + + // Assert + const expectedOriginalField = { + ...omit(fieldToDuplicate, ['getQuestion']), + _id: new ObjectId(fieldToDuplicate._id), + } + const expectedDuplicatedField = omit(fieldToDuplicate, [ + '_id', + 'globalId', + 'getQuestion', + ]) + + // @ts-ignore + expect(actual?.form_fields.toObject()[0]).toEqual(expectedOriginalField) + expect(actualDuplicatedField).toEqual(expectedDuplicatedField) + }) + it('should return null if given fieldId is invalid', async () => { const updatedForm = await validForm.duplicateFormFieldByIdAndIndex( new ObjectId().toHexString(), diff --git a/src/app/modules/form/__tests__/form.utils.spec.ts b/src/app/modules/form/__tests__/form.utils.spec.ts index c75e87e18a..b8020793cc 100644 --- a/src/app/modules/form/__tests__/form.utils.spec.ts +++ b/src/app/modules/form/__tests__/form.utils.spec.ts @@ -113,7 +113,7 @@ describe('form.utils', () => { }) describe('getFormFieldIndexById', () => { - it('should return index of the field when form fields given is a primitive array', async () => { + it('should return index of the field on valid fieldId', async () => { // Arrange const fieldToFind = generateDefaultField(BasicField.HomeNo) const formFields = [ From 7935feb83223fba24b2780ad7c0f97a0962cf953 Mon Sep 17 00:00:00 2001 From: foochifa Date: Tue, 9 May 2023 10:50:21 +0800 Subject: [PATCH 10/12] test: add check to ensure form field length size --- src/app/models/__tests__/form.server.model.spec.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/app/models/__tests__/form.server.model.spec.ts b/src/app/models/__tests__/form.server.model.spec.ts index 3db5a2d9e4..95d5af2bb7 100644 --- a/src/app/models/__tests__/form.server.model.spec.ts +++ b/src/app/models/__tests__/form.server.model.spec.ts @@ -2772,6 +2772,9 @@ describe('Form Model', () => { // @ts-ignore expect(actual?.form_fields.toObject()[0]).toEqual(expectedOriginalField) expect(actualDuplicatedField).toEqual(expectedDuplicatedField) + // ensure nothing has been deleted from splice + // @ts-ignore + expect(actual?.form_fields.length).toEqual(3) }) it('should return null if given fieldId is invalid', async () => { From abd2b649d95ace20c05f44e756172189c1881d0b Mon Sep 17 00:00:00 2001 From: foochifa Date: Mon, 15 May 2023 12:23:01 +0800 Subject: [PATCH 11/12] fix: ensure that newField is push to end if index does not exist --- .../builder-and-design/mutations/useDuplicateFormField.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/frontend/src/features/admin-form/create/builder-and-design/mutations/useDuplicateFormField.ts b/frontend/src/features/admin-form/create/builder-and-design/mutations/useDuplicateFormField.ts index 82f803c62a..f7381b9859 100644 --- a/frontend/src/features/admin-form/create/builder-and-design/mutations/useDuplicateFormField.ts +++ b/frontend/src/features/admin-form/create/builder-and-design/mutations/useDuplicateFormField.ts @@ -62,7 +62,12 @@ export const useDuplicateFormField = () => { if (!oldForm) throw new Error('Query should have been set') const insertionIndex = oldForm.form_fields.findIndex((o) => o._id === fieldId) + 1 - oldForm.form_fields.splice(insertionIndex, 0, newField) + if (insertionIndex > 0) { + oldForm.form_fields.splice(insertionIndex, 0, newField) + } else { + // if index does not exist, push new field to end + oldForm.form_fields.push(newField) + } return oldForm }) // Switch to editing new field From 855ef96e38bb3e316907a88e3e3afa68fe7c4fff Mon Sep 17 00:00:00 2001 From: foochifa Date: Mon, 15 May 2023 12:25:23 +0800 Subject: [PATCH 12/12] fix: unneeded addition --- 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 b5bdd04c5f..11549f0a40 100644 --- a/src/app/modules/form/admin-form/admin-form.service.ts +++ b/src/app/modules/form/admin-form/admin-form.service.ts @@ -539,7 +539,7 @@ export const duplicateFormField = ( const fieldIndex = getFormFieldIndexById(form.form_fields, fieldId) // if fieldIndex does not exist, append to end of form fields const insertionIndex = - fieldIndex === null ? form.form_fields.length + 1 : fieldIndex + 1 + fieldIndex === null ? form.form_fields.length : fieldIndex + 1 return ResultAsync.fromPromise( form.duplicateFormFieldByIdAndIndex(fieldId, insertionIndex), (error) => {