Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: duplicate fields to next row #6285

Merged
merged 13 commits into from
May 21, 2023
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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
foochifa marked this conversation as resolved.
Show resolved Hide resolved
oldForm.form_fields.splice(insertionIndex, 0, newField)
return oldForm
})
// Switch to editing new field
Expand Down
49 changes: 46 additions & 3 deletions src/app/models/__tests__/form.server.model.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -2712,7 +2712,46 @@ 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',
'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 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',
Expand All @@ -2733,11 +2772,15 @@ 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 () => {
const updatedForm = await validForm.duplicateFormFieldById(
const updatedForm = await validForm.duplicateFormFieldByIdAndIndex(
new ObjectId().toHexString(),
0,
)

// Assert
Expand Down
7 changes: 5 additions & 2 deletions src/app/models/form.server.model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<IFieldSchema>).push(
;(this.form_fields as Types.DocumentArray<IFieldSchema>).splice(
insertionIndex,
0,
duplicatedField,
)
return this.save()
Expand Down
51 changes: 50 additions & 1 deletion src/app/modules/form/__tests__/form.utils.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 = '[email protected]'
const MOCK_EMAIL_2 = '[email protected]'
Expand Down Expand Up @@ -107,4 +111,49 @@ describe('form.utils', () => {
expect(result).toEqual(null)
})
})

describe('getFormFieldIndexById', () => {
it('should return index of the field on valid fieldId', 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)
})
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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' }),
),
Expand Down
17 changes: 14 additions & 3 deletions src/app/modules/form/admin-form/admin-form.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -531,15 +536,21 @@ 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
foochifa marked this conversation as resolved.
Show resolved Hide resolved
return ResultAsync.fromPromise(
form.duplicateFormFieldById(fieldId),
form.duplicateFormFieldByIdAndIndex(fieldId, insertionIndex),
(error) => {
logger.error({
message: 'Error encountered while duplicating form field',
meta: {
action: 'duplicateFormField',
formId: form._id,
fieldId,
fieldIndex,
insertionIndex,
},
error,
})
Expand All @@ -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())
Expand Down
13 changes: 13 additions & 0 deletions src/app/modules/form/form.utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,19 @@ 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
}

const index = formFields.findIndex((f) => fieldId === String(f._id))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the String() wrapper necessary here when it wasn't used in useDuplicateFormField.ts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thats because the f._id here is a Mongoose ObjectId whilst the one in useDuplicateFormField is a string hahaha

And the reason why I had to wrap it in string instead of using equals comparator is because for some reason typescript or our current mongoose type does not recognise ObjectId.equals which is very weird 😩

// if index is negative, fieldId does not exist in formFields
return index >= 0 ? index : null
}

/**
* Finds and returns form logic in given form by its id
* @param form_logics the logics to search from
Expand Down
9 changes: 7 additions & 2 deletions src/types/form.ts
Original file line number Diff line number Diff line change
Expand Up @@ -130,12 +130,17 @@ export interface IFormSchema extends IForm, Document, PublicView<PublicForm> {

/**
* 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<T>(this: T, fieldId: string): Promise<T | null>
duplicateFormFieldByIdAndIndex<T>(
this: T,
fieldId: string,
insertionIndex: number,
): Promise<T | null>

/**
* Reorders field corresponding to given fieldId to given newPosition
Expand Down