From 1d6652a17f6eded4374fc453da047ff916d4782f Mon Sep 17 00:00:00 2001 From: Leonard Yam Date: Fri, 11 Aug 2023 18:40:48 +0800 Subject: [PATCH] refactor: apply suggestions from code review 1. For selectedValidation, create new enum in the frontend and simplify shared enum values. 2. Rename rangeMinimum and rangeMaximum to customMin and customMax respectively. 3. Rewrite typeof checks to truthiness checks in backend validators. 4. Remove unnecessary '' from selectedLengthValidation --- .../edit-fieldtype/EditNumber/EditNumber.tsx | 104 +++++++++++------- .../builder-and-design/utils/fieldCreation.ts | 4 +- frontend/src/utils/fieldValidation.ts | 16 +-- shared/types/field/numberField.ts | 10 +- src/app/models/field/numberField.ts | 40 +++---- .../validators/numberValidator.ts | 12 +- 6 files changed, 103 insertions(+), 83 deletions(-) diff --git a/frontend/src/features/admin-form/create/builder-and-design/BuilderAndDesignDrawer/EditFieldDrawer/edit-fieldtype/EditNumber/EditNumber.tsx b/frontend/src/features/admin-form/create/builder-and-design/BuilderAndDesignDrawer/EditFieldDrawer/edit-fieldtype/EditNumber/EditNumber.tsx index 7bf3831b9e..453d923cbe 100644 --- a/frontend/src/features/admin-form/create/builder-and-design/BuilderAndDesignDrawer/EditFieldDrawer/edit-fieldtype/EditNumber/EditNumber.tsx +++ b/frontend/src/features/admin-form/create/builder-and-design/BuilderAndDesignDrawer/EditFieldDrawer/edit-fieldtype/EditNumber/EditNumber.tsx @@ -29,19 +29,26 @@ type EditNumberProps = EditFieldProps const EDIT_NUMBER_FIELD_KEYS = ['title', 'description', 'required'] as const +// As we want to keep the values in the shared type simple, +// we create a separate enum for frontend options and transform them as needed +enum NumberSelectedValidationInputs { + Length = 'Character Length', + Range = 'Number Range', +} + type EditNumberInputs = Pick< NumberFieldBase, typeof EDIT_NUMBER_FIELD_KEYS[number] > & { ValidationOptions: { - selectedValidation: NumberSelectedValidation | '' + selectedValidation: NumberSelectedValidationInputs | '' LengthValidationOptions: { customVal: number | '' selectedLengthValidation: NumberSelectedLengthValidation | '' } RangeValidationOptions: { - rangeMinimum: number | '' - rangeMaximum: number | '' + customMin: number | '' + customMax: number | '' } } } @@ -49,27 +56,31 @@ type EditNumberInputs = Pick< const transformNumberFieldToEditForm = ( field: NumberFieldBase, ): EditNumberInputs => { + const { + selectedValidation, + LengthValidationOptions, + RangeValidationOptions, + } = field.ValidationOptions + const nextSelectedValidation = - field.ValidationOptions.selectedValidation || ('' as const) + selectedValidation === NumberSelectedValidation.Length + ? NumberSelectedValidationInputs.Length + : selectedValidation === NumberSelectedValidation.Range + ? NumberSelectedValidationInputs.Range + : ('' as const) const nextLengthValidationOptions = { selectedLengthValidation: - field.ValidationOptions.LengthValidationOptions - .selectedLengthValidation || ('' as const), + LengthValidationOptions.selectedLengthValidation || ('' as const), customVal: - (!!field.ValidationOptions.LengthValidationOptions - .selectedLengthValidation && - field.ValidationOptions.LengthValidationOptions.customVal) || + (!!LengthValidationOptions.selectedLengthValidation && + LengthValidationOptions.customVal) || ('' as const), } const nextRangeValidationOptions = { - rangeMinimum: - field.ValidationOptions.RangeValidationOptions.rangeMinimum || - ('' as const), - rangeMaximum: - field.ValidationOptions.RangeValidationOptions.rangeMaximum || - ('' as const), + customMin: RangeValidationOptions.customMin || ('' as const), + customMax: RangeValidationOptions.customMax || ('' as const), } return { @@ -86,30 +97,40 @@ const transformNumberEditFormToField = ( inputs: EditNumberInputs, originalField: NumberFieldBase, ): NumberFieldBase => { + const { + selectedValidation, + LengthValidationOptions, + RangeValidationOptions, + } = inputs.ValidationOptions + const hasSelectedLengthValidationOption = - inputs.ValidationOptions.selectedValidation === - NumberSelectedValidation.Length && - inputs.ValidationOptions.LengthValidationOptions - .selectedLengthValidation !== '' + selectedValidation === NumberSelectedValidationInputs.Length && + LengthValidationOptions.selectedLengthValidation !== '' + + const nextSelectedValidation = + selectedValidation === NumberSelectedValidationInputs.Length + ? NumberSelectedValidation.Length + : selectedValidation === NumberSelectedValidationInputs.Range + ? NumberSelectedValidation.Range + : null const nextLengthValidationOptions = hasSelectedLengthValidationOption - ? inputs.ValidationOptions.LengthValidationOptions + ? LengthValidationOptions : { selectedLengthValidation: null, customVal: null, } const nextRangeValidationOptions = - inputs.ValidationOptions.selectedValidation === - NumberSelectedValidation.Range - ? inputs.ValidationOptions.RangeValidationOptions + selectedValidation === NumberSelectedValidationInputs.Range + ? RangeValidationOptions : { - rangeMinimum: null, - rangeMaximum: null, + customMin: null, + customMax: null, } const nextValidationOptions = { - selectedValidation: inputs.ValidationOptions.selectedValidation || null, + selectedValidation: nextSelectedValidation, LengthValidationOptions: nextLengthValidationOptions, RangeValidationOptions: nextRangeValidationOptions, } @@ -182,23 +203,23 @@ export const EditNumber = ({ field }: EditNumberProps): JSX.Element => { [getValues], ) - // We use the rangeMinimum field to perform cross-field validation for + // We use the customMin field to perform cross-field validation for // the number range const RangeMinimumValidationOptions: RegisterOptions< EditNumberInputs, - 'ValidationOptions.RangeValidationOptions.rangeMinimum' + 'ValidationOptions.RangeValidationOptions.customMin' > = useMemo( () => ({ validate: { hasValidRange: (val) => { const numVal = Number(val) - const rangeMaximum = getValues( - 'ValidationOptions.RangeValidationOptions.rangeMaximum', + const customMax = getValues( + 'ValidationOptions.RangeValidationOptions.customMax', ) - const numRangeMaximum = Number(rangeMaximum) + return ( - isNaN(numRangeMaximum) || - numVal <= numRangeMaximum || + customMax === '' || + numVal <= Number(customMax) || `Please enter a valid range!` ) }, @@ -212,8 +233,8 @@ export const EditNumber = ({ field }: EditNumberProps): JSX.Element => { if (!watchedSelectedValidation) { clearErrors('ValidationOptions') setValue('ValidationOptions.LengthValidationOptions.customVal', '') - setValue('ValidationOptions.RangeValidationOptions.rangeMinimum', '') - setValue('ValidationOptions.RangeValidationOptions.rangeMaximum', '') + setValue('ValidationOptions.RangeValidationOptions.customMin', '') + setValue('ValidationOptions.RangeValidationOptions.customMax', '') } }, [clearErrors, setValue, watchedSelectedValidation]) @@ -248,12 +269,12 @@ export const EditNumber = ({ field }: EditNumberProps): JSX.Element => { control={control} render={({ field }) => ( )} /> - {watchedSelectedValidation === NumberSelectedValidation.Range && ( + {watchedSelectedValidation === NumberSelectedValidationInputs.Range && ( <> Minimum and/or maximum value @@ -264,7 +285,7 @@ export const EditNumber = ({ field }: EditNumberProps): JSX.Element => { spacing="0.5rem" > ( @@ -278,7 +299,7 @@ export const EditNumber = ({ field }: EditNumberProps): JSX.Element => { )} /> ( { { - errors?.ValidationOptions?.RangeValidationOptions?.rangeMinimum + errors?.ValidationOptions?.RangeValidationOptions?.customMin ?.message } )} - {watchedSelectedValidation === NumberSelectedValidation.Length && ( + {watchedSelectedValidation === + NumberSelectedValidationInputs.Length && ( <> Number of characters allowed diff --git a/frontend/src/features/admin-form/create/builder-and-design/utils/fieldCreation.ts b/frontend/src/features/admin-form/create/builder-and-design/utils/fieldCreation.ts index 06e7821931..660423918b 100644 --- a/frontend/src/features/admin-form/create/builder-and-design/utils/fieldCreation.ts +++ b/frontend/src/features/admin-form/create/builder-and-design/utils/fieldCreation.ts @@ -112,8 +112,8 @@ export const getFieldCreationMeta = (fieldType: BasicField): FieldCreateDto => { customVal: null, }, RangeValidationOptions: { - rangeMinimum: null, - rangeMaximum: null, + customMin: null, + customMax: null, }, }, } diff --git a/frontend/src/utils/fieldValidation.ts b/frontend/src/utils/fieldValidation.ts index 3c779345e7..ca7b539318 100644 --- a/frontend/src/utils/fieldValidation.ts +++ b/frontend/src/utils/fieldValidation.ts @@ -210,7 +210,7 @@ export const createNumberValidationRules: ValidationRuleFn = ( const { selectedValidation } = schema.ValidationOptions const { selectedLengthValidation, customVal } = schema.ValidationOptions.LengthValidationOptions - const { rangeMinimum, rangeMaximum } = + const { customMin, customMax } = schema.ValidationOptions.RangeValidationOptions return { @@ -249,19 +249,19 @@ export const createNumberValidationRules: ValidationRuleFn = ( return true const numVal = Number(val) - const hasMinimum = typeof rangeMinimum === 'number' - const hasMaximum = typeof rangeMaximum === 'number' + const hasMinimum = !!customMin + const hasMaximum = !!customMax const isOutsideRange = - (hasMinimum && rangeMinimum > numVal) || - (hasMaximum && rangeMaximum < numVal) + (hasMinimum && customMin > numVal) || + (hasMaximum && customMax < numVal) if (!isOutsideRange) return true else if (hasMinimum && hasMaximum) { - return `Please enter a number within the range ${rangeMinimum} to ${rangeMaximum}` + return `Please enter a number within the range ${customMin} to ${customMax}` } else if (hasMinimum) { - return `Please enter a number that has a minimum value of ${rangeMinimum}` + return `Please enter a number that has a minimum value of ${customMin}` } else if (hasMaximum) { - return `Please enter a number that has a maximum value of ${rangeMaximum}` + return `Please enter a number that has a maximum value of ${customMax}` } }, }, diff --git a/shared/types/field/numberField.ts b/shared/types/field/numberField.ts index 9c8d64a184..7af801b193 100644 --- a/shared/types/field/numberField.ts +++ b/shared/types/field/numberField.ts @@ -1,8 +1,8 @@ import { BasicField, MyInfoableFieldBase } from './base' export enum NumberSelectedValidation { - Length = 'Character Length', - Range = 'Number Range', + Length = 'Length', + Range = 'Range', } export enum NumberSelectedLengthValidation { @@ -13,12 +13,12 @@ export enum NumberSelectedLengthValidation { export type NumberLengthValidationOptions = { customVal: number | null - selectedLengthValidation: NumberSelectedLengthValidation | '' | null + selectedLengthValidation: NumberSelectedLengthValidation | null } export type NumberRangeValidationOptions = { - rangeMinimum: number | null - rangeMaximum: number | null + customMin: number | null + customMax: number | null } export type NumberValidationOptions = { diff --git a/src/app/models/field/numberField.ts b/src/app/models/field/numberField.ts index c0537b29dd..f43f0bf92e 100644 --- a/src/app/models/field/numberField.ts +++ b/src/app/models/field/numberField.ts @@ -11,28 +11,28 @@ import { MyInfoSchema } from './baseField' const createNumberFieldSchema = () => { const ValidationOptionsSchema = new Schema({ - selectedValidation: { + selectedValidation: { + type: String, + enum: [...Object.values(NumberSelectedValidation), null], + }, + LengthValidationOptions: { + customVal: { + type: Number, + }, + selectedLengthValidation: { type: String, - enum: [...Object.values(NumberSelectedValidation), null], + enum: [...Object.values(NumberSelectedLengthValidation), null], }, - LengthValidationOptions: { - customVal: { - type: Number, - }, - selectedLengthValidation: { - type: String, - enum: [...Object.values(NumberSelectedLengthValidation), null], - }, + }, + RangeValidationOptions: { + customMin: { + type: Number, }, - RangeValidationOptions: { - rangeMinimum: { - type: Number, - }, - rangeMaximum: { - type: Number, - }, + customMax: { + type: Number, }, - }) + }, + }) const NumberFieldSchema = new Schema({ myInfo: MyInfoSchema, @@ -46,8 +46,8 @@ const createNumberFieldSchema = () => { selectedLengthValidation: null, }, RangeValidationOptions: { - rangeMinimum: null, - rangeMaximum: null, + customMin: null, + customMax: null, }, }, }, diff --git a/src/app/utils/field-validation/validators/numberValidator.ts b/src/app/utils/field-validation/validators/numberValidator.ts index b7697581b1..3968e1dd63 100644 --- a/src/app/utils/field-validation/validators/numberValidator.ts +++ b/src/app/utils/field-validation/validators/numberValidator.ts @@ -92,15 +92,13 @@ const getNumberLengthValidator: NumberValidatorConstructor = (numberField) => { * Returns a validation function to check if number is * within the number range specified. */ -const numberRangeValidator: NumberValidatorConstructor = +const getNumberRangeValidator: NumberValidatorConstructor = (numberField) => (response) => { const val = Number(response.answer) - const { rangeMinimum, rangeMaximum } = + const { customMin, customMax } = numberField.ValidationOptions.RangeValidationOptions - const isWithinMinimum = - typeof rangeMinimum === 'number' && rangeMinimum <= val - const isWithinMaximum = - typeof rangeMaximum === 'number' && val <= rangeMaximum + const isWithinMinimum = !!customMin && customMin <= val + const isWithinMaximum = !!customMax && val <= customMax return isWithinMinimum && isWithinMaximum ? right(response) @@ -116,7 +114,7 @@ const getNumberValidator: NumberValidatorConstructor = (numberField) => { case NumberSelectedValidation.Length: return getNumberLengthValidator(numberField) case NumberSelectedValidation.Range: - return numberRangeValidator(numberField) + return getNumberRangeValidator(numberField) default: return right }