From 8bf6a08e08b1e6fdd126fc91ad6252d4192b20d7 Mon Sep 17 00:00:00 2001 From: Foo Yong Jie Date: Wed, 14 Jul 2021 13:11:37 +0800 Subject: [PATCH] feat(form-logic): introduce form logic model validation (#2302) --- .../__tests__/form.server.model.spec.ts | 50 +++++++++ src/app/models/form.server.model.ts | 102 ++++++++++++++---- .../edit-logic-modal.client.controller.js | 2 +- .../form-logic.client.service.spec.ts | 78 -------------- .../form-logic/form-logic.client.service.ts | 55 ---------- src/shared/util/__tests__/logic.spec.ts | 76 +++++++++++++ src/shared/util/logic.ts | 54 ++++++++++ 7 files changed, 262 insertions(+), 155 deletions(-) delete mode 100644 src/public/modules/forms/services/form-logic/__tests__/form-logic.client.service.spec.ts delete mode 100644 src/public/modules/forms/services/form-logic/form-logic.client.service.ts diff --git a/src/app/models/__tests__/form.server.model.spec.ts b/src/app/models/__tests__/form.server.model.spec.ts index 5eb7a8bae1..7146175ac3 100644 --- a/src/app/models/__tests__/form.server.model.spec.ts +++ b/src/app/models/__tests__/form.server.model.spec.ts @@ -147,6 +147,56 @@ describe('Form Model', () => { expect(Object.keys(saved)).not.toContain('extra') }) + it('should create and save successfully with form_logics that reference nonexistent form_fields', async () => { + // Arrange + const FORM_LOGICS = { + form_logics: [ + { + conditions: [ + { + _id: '', + field: new ObjectId(), + state: 'is equals to', + value: '', + ifValueType: 'number', + }, + ], + logicType: 'preventSubmit', + preventSubmitMessage: '', + }, + ], + } + const formParamsWithLogic = merge({}, MOCK_FORM_PARAMS, FORM_LOGICS) + + // Act + const validForm = new Form(formParamsWithLogic) + const saved = await validForm.save() + + // Assert + // All fields should exist + // Object Id should be defined when successfully saved to MongoDB. + expect(saved._id).toBeDefined() + expect(saved.created).toBeInstanceOf(Date) + expect(saved.lastModified).toBeInstanceOf(Date) + // Retrieve object and compare to params, remove indeterministic keys + const actualSavedObject = omit(saved.toObject(), [ + '_id', + 'created', + 'lastModified', + '__v', + ]) + actualSavedObject.form_logics = actualSavedObject.form_logics?.map( + (logic) => omit(logic, '_id'), + ) + const expectedObject = merge( + {}, + FORM_DEFAULTS, + MOCK_FORM_PARAMS, + FORM_LOGICS, + ) + expect(actualSavedObject).toEqual(expectedObject) + }) + it('should create and save successfully with valid permissionList emails', async () => { // Arrange // permissionList has email with valid domain diff --git a/src/app/models/form.server.model.ts b/src/app/models/form.server.model.ts index c301a753c3..9305274bf4 100644 --- a/src/app/models/form.server.model.ts +++ b/src/app/models/form.server.model.ts @@ -1,4 +1,4 @@ -import BSON from 'bson-ext' +import BSON, { ObjectId } from 'bson-ext' import { compact, omit, pick, uniq } from 'lodash' import mongoose, { Mongoose, @@ -11,6 +11,7 @@ import validator from 'validator' import { MB } from '../../shared/constants' import { reorder } from '../../shared/util/immutable-array-fns' +import { getApplicableIfStates } from '../../shared/util/logic' import { AuthType, BasicField, @@ -30,7 +31,9 @@ import { IFormDocument, IFormModel, IFormSchema, + ILogicSchema, IPopulatedForm, + LogicConditionState, LogicDto, LogicType, Permission, @@ -208,7 +211,67 @@ const compileFormModel = (db: Mongoose): IFormModel => { 'Check that your form is MyInfo-authenticated, is an email mode form and has 30 or fewer MyInfo fields.', }, }, - form_logics: [LogicSchema], + form_logics: { + type: [LogicSchema], + validate: { + validator(this: IFormSchema, v: ILogicSchema[]) { + /** + * A validatable condition is incomplete if there is a possibility + * that its fieldType is null, which is a sign that a condition's + * field property references a non-existent form_field. + */ + type IncompleteValidatableCondition = { + state: LogicConditionState + fieldType?: BasicField + } + + /** + * A condition object is said to be validatable if it contains the two + * necessary for validation: fieldType and state + */ + type ValidatableCondition = IncompleteValidatableCondition & { + fieldType: BasicField + } + + const isConditionReferencesExistingField = ( + condition: IncompleteValidatableCondition, + ): condition is ValidatableCondition => !!condition.fieldType + + const conditions = v.flatMap((logic) => { + return logic.conditions.map( + (condition) => { + const { + field, + state, + }: { field: ObjectId | string; state: LogicConditionState } = + condition + return { + state, + fieldType: this.form_fields?.find( + (f: IFieldSchema) => String(f._id) === String(field), + )?.fieldType, + } + }, + ) + }) + + return conditions.every((condition) => { + /** + * Form fields can get deleted by form admins, which causes logic + * conditions to reference invalid fields. Here we bypass validation + * and allow these conditions to be saved, so we don't make life + * difficult for form admins. + */ + if (!isConditionReferencesExistingField(condition)) return true + + const { fieldType, state } = condition + const applicableIfStates = getApplicableIfStates(fieldType) + return applicableIfStates.includes(state) + }) + }, + message: 'Form logic condition validation failed.', + }, + }, admin: { type: Schema.Types.ObjectId, @@ -690,14 +753,13 @@ const compileFormModel = (db: Mongoose): IFormModel => { formId: string, createLogicBody: LogicDto, ): Promise { - return this.findByIdAndUpdate( - formId, - { $push: { form_logics: createLogicBody } }, - { - new: true, - runValidators: true, - }, - ).exec() + const form = await this.findById(formId).exec() + if (!form?.form_logics) return null + const newLogic = ( + form.form_logics as Types.DocumentArray + ).create(createLogicBody) + form.form_logics.push(newLogic) + return form.save() } // Deletes specified form field by id. @@ -718,17 +780,15 @@ const compileFormModel = (db: Mongoose): IFormModel => { logicId: string, updatedLogic: LogicDto, ): Promise { - return this.findByIdAndUpdate( - formId, - { - $set: { 'form_logics.$[object]': updatedLogic }, - }, - { - arrayFilters: [{ 'object._id': logicId }], - new: true, - runValidators: true, - }, - ).exec() + let form = await this.findById(formId).exec() + if (!form?.form_logics) return null + const index = form.form_logics.findIndex( + (logic) => String(logic._id) === logicId, + ) + form = form.set(`form_logics.${index}`, updatedLogic, { + new: true, + }) + return form.save() } FormSchema.statics.updateEndPageById = async function ( 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 c6d2d5c164..36f72c7d0f 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 @@ -2,7 +2,7 @@ const { range } = require('lodash') const { LogicType } = require('../../../../../types') -const FormLogic = require('../../services/form-logic/form-logic.client.service') +const FormLogic = require('../../../../../shared/util/logic') const UpdateFormService = require('../../../../services/UpdateFormService') angular diff --git a/src/public/modules/forms/services/form-logic/__tests__/form-logic.client.service.spec.ts b/src/public/modules/forms/services/form-logic/__tests__/form-logic.client.service.spec.ts deleted file mode 100644 index 3fe16c424f..0000000000 --- a/src/public/modules/forms/services/form-logic/__tests__/form-logic.client.service.spec.ts +++ /dev/null @@ -1,78 +0,0 @@ -import { - BasicField, - IField, - LogicConditionState, -} from '../../../../../../types' -import * as FormLogic from '../form-logic.client.service' - -const VALID_IF_CONDITION_FIELDS = [ - BasicField.Dropdown, - BasicField.Number, - BasicField.Decimal, - BasicField.Rating, - BasicField.YesNo, - BasicField.Radio, -] - -const INVALID_IF_CONDITION_FIELDS = Object.values(BasicField).filter( - (fieldType) => !VALID_IF_CONDITION_FIELDS.includes(fieldType), -) -describe('form-logic.client.service', () => { - describe('getApplicableIfFields', () => { - it('should not filter fields suitable as an if-conditional', () => { - const validIfFields: IField[] = VALID_IF_CONDITION_FIELDS.map( - (fieldType) => ({ fieldType } as unknown as IField), - ) - const fields = FormLogic.getApplicableIfFields(validIfFields) - validIfFields.forEach((v, i) => expect(v).toStrictEqual(fields[i])) - }) - it('should filter fields not suitable as an if-conditional', () => { - const invalidIfFields: IField[] = INVALID_IF_CONDITION_FIELDS.map( - (x) => x as unknown as IField, - ) - const fields = FormLogic.getApplicableIfFields(invalidIfFields) - expect(fields).toStrictEqual([]) - }) - }) - - describe('getApplicableIfStates', () => { - it('should return valid logic states for categorical field types', () => { - const categoricalFields = [BasicField.Dropdown, BasicField.Radio] - categoricalFields.forEach((fieldType) => { - const states = FormLogic.getApplicableIfStates(fieldType) - expect(states).toIncludeSameMembers([ - LogicConditionState.Equal, - LogicConditionState.Either, - ]) - expect(states).toBeArrayOfSize(2) - }) - }) - it('should return valid logic states for binary field types', () => { - const states = FormLogic.getApplicableIfStates(BasicField.YesNo) - expect(states).toIncludeSameMembers([LogicConditionState.Equal]) - expect(states).toBeArrayOfSize(1) - }) - it('should return valid logic states for numerical field types', () => { - const numericalFields = [ - BasicField.Number, - BasicField.Decimal, - BasicField.Rating, - ] - numericalFields.forEach((fieldType) => { - const states = FormLogic.getApplicableIfStates(fieldType) - expect(states).toIncludeSameMembers([ - LogicConditionState.Equal, - LogicConditionState.Lte, - LogicConditionState.Gte, - ]) - expect(states).toBeArrayOfSize(3) - }) - }) - it('should return empty array for invalid conditional fields', () => { - INVALID_IF_CONDITION_FIELDS.forEach((fieldType) => { - const states = FormLogic.getApplicableIfStates(fieldType) - expect(states).toStrictEqual([]) - }) - }) - }) -}) diff --git a/src/public/modules/forms/services/form-logic/form-logic.client.service.ts b/src/public/modules/forms/services/form-logic/form-logic.client.service.ts deleted file mode 100644 index 544af894c9..0000000000 --- a/src/public/modules/forms/services/form-logic/form-logic.client.service.ts +++ /dev/null @@ -1,55 +0,0 @@ -import { - BasicField, - IField, - LogicCondition, - LogicConditionState, -} from '../../../../../types' - -const LOGIC_CONDITIONS: LogicCondition[] = [ - [ - BasicField.Dropdown, - [LogicConditionState.Equal, LogicConditionState.Either], - ], - [ - BasicField.Number, - [ - LogicConditionState.Equal, - LogicConditionState.Lte, - LogicConditionState.Gte, - ], - ], - [ - BasicField.Decimal, - [ - LogicConditionState.Equal, - LogicConditionState.Lte, - LogicConditionState.Gte, - ], - ], - [ - BasicField.Rating, - [ - LogicConditionState.Equal, - LogicConditionState.Lte, - LogicConditionState.Gte, - ], - ], - [BasicField.YesNo, [LogicConditionState.Equal]], - [BasicField.Radio, [LogicConditionState.Equal, LogicConditionState.Either]], -] - -const LOGIC_MAP = new Map(LOGIC_CONDITIONS) - -/** - * Given a list of form fields, returns only the fields that are - * allowed to be present in the if-condition dropdown in the Logic tab. - */ -export const getApplicableIfFields = (formFields: IField[]): IField[] => - formFields.filter((field) => !!LOGIC_MAP.get(field.fieldType)) - -/** - * Given a single form field type, returns the applicable logic states for that field type. - */ -export const getApplicableIfStates = ( - fieldType: BasicField, -): LogicConditionState[] => LOGIC_MAP.get(fieldType) ?? [] diff --git a/src/shared/util/__tests__/logic.spec.ts b/src/shared/util/__tests__/logic.spec.ts index 057d7477b6..1edc508102 100644 --- a/src/shared/util/__tests__/logic.spec.ts +++ b/src/shared/util/__tests__/logic.spec.ts @@ -1,12 +1,15 @@ import { ObjectId } from 'bson-ext' import { + getApplicableIfFields, + getApplicableIfStates, getLogicUnitPreventingSubmit, getVisibleFieldIds, } from 'src/shared/util/logic' import { BasicField, FieldResponse, + IField, IFieldSchema, IFormDocument, IPreventSubmitLogicSchema, @@ -1002,3 +1005,76 @@ describe('Logic validation', () => { }) }) }) + +describe('Logic util', () => { + const VALID_IF_CONDITION_FIELDS = [ + BasicField.Dropdown, + BasicField.Number, + BasicField.Decimal, + BasicField.Rating, + BasicField.YesNo, + BasicField.Radio, + ] + + const INVALID_IF_CONDITION_FIELDS = Object.values(BasicField).filter( + (fieldType) => !VALID_IF_CONDITION_FIELDS.includes(fieldType), + ) + + describe('getApplicableIfFields', () => { + it('should not filter fields suitable as an if-conditional', () => { + const validIfFields: IField[] = VALID_IF_CONDITION_FIELDS.map( + (fieldType) => ({ fieldType } as unknown as IField), + ) + const fields = getApplicableIfFields(validIfFields) + validIfFields.forEach((v, i) => expect(v).toStrictEqual(fields[i])) + }) + it('should filter fields not suitable as an if-conditional', () => { + const invalidIfFields: IField[] = INVALID_IF_CONDITION_FIELDS.map( + (x) => x as unknown as IField, + ) + const fields = getApplicableIfFields(invalidIfFields) + expect(fields).toStrictEqual([]) + }) + }) + + describe('getApplicableIfStates', () => { + it('should return valid logic states for categorical field types', () => { + const categoricalFields = [BasicField.Dropdown, BasicField.Radio] + categoricalFields.forEach((fieldType) => { + const states = getApplicableIfStates(fieldType) + expect(states).toIncludeSameMembers([ + LogicConditionState.Equal, + LogicConditionState.Either, + ]) + expect(states).toBeArrayOfSize(2) + }) + }) + it('should return valid logic states for binary field types', () => { + const states = getApplicableIfStates(BasicField.YesNo) + expect(states).toIncludeSameMembers([LogicConditionState.Equal]) + expect(states).toBeArrayOfSize(1) + }) + it('should return valid logic states for numerical field types', () => { + const numericalFields = [ + BasicField.Number, + BasicField.Decimal, + BasicField.Rating, + ] + numericalFields.forEach((fieldType) => { + const states = getApplicableIfStates(fieldType) + expect(states).toIncludeSameMembers([ + LogicConditionState.Equal, + LogicConditionState.Lte, + LogicConditionState.Gte, + ]) + expect(states).toBeArrayOfSize(3) + }) + }) + it('should return empty array for invalid conditional fields', () => { + INVALID_IF_CONDITION_FIELDS.forEach((fieldType) => { + const states = getApplicableIfStates(fieldType) + expect(states).toStrictEqual([]) + }) + }) + }) +}) diff --git a/src/shared/util/logic.ts b/src/shared/util/logic.ts index d582cab64d..2415d5ce12 100644 --- a/src/shared/util/logic.ts +++ b/src/shared/util/logic.ts @@ -1,15 +1,69 @@ import { + BasicField, FieldResponse, IClientFieldSchema, IConditionSchema, + IField, IFormDocument, ILogicSchema, IPreventSubmitLogicSchema, IShowFieldsLogicSchema, + LogicCondition, LogicConditionState, LogicType, } from '../../types' +const LOGIC_CONDITIONS: LogicCondition[] = [ + [ + BasicField.Dropdown, + [LogicConditionState.Equal, LogicConditionState.Either], + ], + [ + BasicField.Number, + [ + LogicConditionState.Equal, + LogicConditionState.Lte, + LogicConditionState.Gte, + ], + ], + [ + BasicField.Decimal, + [ + LogicConditionState.Equal, + LogicConditionState.Lte, + LogicConditionState.Gte, + ], + ], + [ + BasicField.Rating, + [ + LogicConditionState.Equal, + LogicConditionState.Lte, + LogicConditionState.Gte, + ], + ], + [BasicField.YesNo, [LogicConditionState.Equal]], + [BasicField.Radio, [LogicConditionState.Equal, LogicConditionState.Either]], +] + +export const LOGIC_MAP = new Map( + LOGIC_CONDITIONS, +) + +/** + * Given a list of form fields, returns only the fields that are + * allowed to be present in the if-condition dropdown in the Logic tab. + */ +export const getApplicableIfFields = (formFields: IField[]): IField[] => + formFields.filter((field) => !!LOGIC_MAP.get(field.fieldType)) + +/** + * Given a single form field type, returns the applicable logic states for that field type. + */ +export const getApplicableIfStates = ( + fieldType: BasicField, +): LogicConditionState[] => LOGIC_MAP.get(fieldType) ?? [] + type GroupedLogic = Record export type FieldIdSet = Set // This module handles logic on both the client side (IFieldSchema[])