From 65d87b3a93018f0729aed565000eb2a2ce1f2ce7 Mon Sep 17 00:00:00 2001 From: Sandrina Pereira Date: Mon, 3 Jul 2023 23:20:01 +0100 Subject: [PATCH] fix(fieldset): support root conditionals for fieldsets (#23) * create playground to test things * docs(select-validation) - extract options values into an array * docs(select-validation) - extract options values into an array * feat(select) - update yupSchema * fix(yupSchema) - tests * add whitespace to yupSchemas * docs(select) - add tests for select * remove comments * add missing assertions * Revert "create playground to test things" This reverts commit 3312f29b103d00c099bdd4a876349c004f712d0c. * Handle null and empty string. Set custom error message. Add more tests and examples * Sandrina's self-review * fix the bug with a lot of changes. clean next * Clean unnecessary changes - commit it for future story * write test * branch-of-branch - verify old option is no longer valid * Release 0.4.1-dev.20230702181843 * also allow null in required select/radio fields * Release 0.4.1-dev.20230703082439 * fix schema type string -> number * fix incorrect json schema - default value was invalid * Still allow "null" in conventional empty fields * Release 0.4.1-dev.20230703161935 * handle conditionals that include select fields * Release 0.4.1-dev.20230703195833 * refactor ''/null edge-case to use .transform() * Release 0.4.1-dev.20230703203242 * Revert back to 0.4.0-beta.0 --------- Co-authored-by: Gabriel --- src/calculateConditionalProperties.js | 38 ++- src/helpers.js | 2 + src/tests/createHeadlessForm.test.js | 458 +++++++++++++++++--------- src/tests/helpers.js | 162 +++++++-- 4 files changed, 461 insertions(+), 199 deletions(-) diff --git a/src/calculateConditionalProperties.js b/src/calculateConditionalProperties.js index 4ff6ad86..385ca348 100644 --- a/src/calculateConditionalProperties.js +++ b/src/calculateConditionalProperties.js @@ -12,16 +12,16 @@ import { buildYupSchema } from './yupSchema'; /** * Verifies if a field is required * @param {Object} node - JSON schema parent node - * @param {String} inputName - input name + * @param {String} field - input name * @return {Boolean} */ -function isFieldRequired(node, inputName) { - // For nested properties (case of fieldset) we need to check recursively - if (node?.required) { - return node.required.includes(inputName); - } - - return false; +function isFieldRequired(node, field) { + return ( + // Check base root required + field.scopedJsonSchema?.required?.includes(field.name) || + // Check conditional required + node?.required?.includes(field.name) + ); } /** @@ -32,25 +32,34 @@ function isFieldRequired(node, inputName) { * @param {Object} property - property that relates with the list of fields * @returns {Object} */ -function rebuildInnerFieldsRequiredProperty(fields, property) { +function rebuildFieldset(fields, property) { if (property?.properties) { return fields.map((field) => { + const propertyConditionals = property.properties[field.name]; + if (!propertyConditionals) { + return field; + } + + const newFieldParams = extractParametersFromNode(propertyConditionals); + if (field.fields) { return { ...field, - fields: rebuildInnerFieldsRequiredProperty(field.fields, property.properties[field.name]), + ...newFieldParams, + fields: rebuildFieldset(field.fields, propertyConditionals), }; } return { ...field, - required: isFieldRequired(property, field.name), + ...newFieldParams, + required: isFieldRequired(property, field), }; }); } return fields.map((field) => ({ ...field, - required: isFieldRequired(property, field.name), + required: isFieldRequired(property, field), })); } @@ -85,10 +94,7 @@ export function calculateConditionalProperties(fieldParams, customProperties) { let fieldSetFields; if (fieldParams.inputType === supportedTypes.FIELDSET) { - fieldSetFields = rebuildInnerFieldsRequiredProperty( - fieldParams.fields, - conditionalProperty - ); + fieldSetFields = rebuildFieldset(fieldParams.fields, conditionalProperty); newFieldParams.fields = fieldSetFields; } diff --git a/src/helpers.js b/src/helpers.js index 4de2e179..5353488c 100644 --- a/src/helpers.js +++ b/src/helpers.js @@ -323,6 +323,8 @@ function processNode(node, formValues, formFields, accRequired = new Set()) { if (node.if) { const matchesCondition = checkIfConditionMatches(node, formValues, formFields); + // BUG HERE (unreleated) - what if it matches but doesn't has a then, + // it should do nothing, but instead it jumps to node.else when it shouldn't. if (matchesCondition && node.then) { const { required: branchRequired } = processNode( node.then, diff --git a/src/tests/createHeadlessForm.test.js b/src/tests/createHeadlessForm.test.js index 0e62661a..8eb39df4 100644 --- a/src/tests/createHeadlessForm.test.js +++ b/src/tests/createHeadlessForm.test.js @@ -47,6 +47,7 @@ import { mockNestedFieldset, mockGroupArrayInput, schemaFieldsetScopedCondition, + schemaWithConditionalToFieldset, schemaWithConditionalPresentationProperties, schemaWithConditionalReadOnlyProperty, schemaWithWrongConditional, @@ -79,6 +80,17 @@ function friendlyError({ formErrors }) { return formErrors; } +// Get a field by name recursively +// eg getField(demo, "age") -> returns "age" field +// eg getField(demo, child, name) -> returns "child.name" subfield +const getField = (fields, name, ...subNames) => { + const field = fields.find((f) => f.name === name); + if (subNames.length > 0) { + return getField(field.fields, ...subNames); + } + return field; +}; + beforeEach(() => { jest.spyOn(console, 'error').mockImplementation(() => {}); }); @@ -1287,46 +1299,6 @@ describe('createHeadlessForm', () => { }); }); - it('supports "fieldset" field type', () => { - const result = createHeadlessForm( - JSONSchemaBuilder() - .addInput({ - fieldset: mockFieldset, - }) - .build() - ); - - expect(result).toMatchObject({ - fields: [ - { - description: 'Fieldset description', - label: 'Fieldset title', - name: 'fieldset', - type: 'fieldset', - required: false, - fields: [ - { - description: 'The number of your national identification (max 10 digits)', - label: 'ID number', - name: 'id_number', - type: 'text', - required: true, - }, - { - description: 'How many open tabs do you have?', - label: 'Tabs', - maximum: 10, - minimum: 1, - name: 'tabs', - type: 'number', - required: false, - }, - ], - }, - ], - }); - }); - it('supports "radio" field type with its "card" and "card-expandable" variants', () => { const result = createHeadlessForm( JSONSchemaBuilder() @@ -1392,130 +1364,298 @@ describe('createHeadlessForm', () => { }); }); - it('supports nested "fieldset" field type', () => { - const result = createHeadlessForm( - JSONSchemaBuilder() - .addInput({ - nestedFieldset: mockNestedFieldset, - }) - .build() - ); - - expect(result).toMatchObject({ - fields: [ - { - label: 'Nested fieldset title', - description: 'Nested fieldset description', - name: 'nestedFieldset', - type: 'fieldset', - required: false, - fields: [ - { - description: 'Fieldset description', - label: 'Fieldset title', - name: 'innerFieldset', - type: 'fieldset', - required: false, - fields: [ - { - description: 'The number of your national identification (max 10 digits)', - label: 'ID number', - name: 'id_number', - type: 'text', - required: true, - }, - { - description: 'How many open tabs do you have?', - label: 'Tabs', - maximum: 10, - minimum: 1, - name: 'tabs', - type: 'number', - required: false, - }, - ], - }, - ], + describe('supports "fieldset" field type', () => { + it('supports basic case', () => { + const result = createHeadlessForm({ + properties: { + fieldset: mockFieldset, }, - ], + }); + + expect(result).toMatchObject({ + fields: [ + { + description: 'Fieldset description', + label: 'Fieldset title', + name: 'fieldset', + type: 'fieldset', + required: false, + fields: [ + { + description: 'The number of your national identification (max 10 digits)', + label: 'ID number', + name: 'id_number', + type: 'text', + required: true, + }, + { + description: 'How many open tabs do you have?', + label: 'Tabs', + maximum: 10, + minimum: 1, + name: 'tabs', + type: 'number', + required: false, + }, + ], + }, + ], + }); }); - }); - it('supported "fieldset" with scoped conditionals', () => { - const { handleValidation } = createHeadlessForm(schemaFieldsetScopedCondition, {}); - const validateForm = (vals) => friendlyError(handleValidation(vals)); + it('supports nested fieldset (fieldset inside fieldset)', () => { + const result = createHeadlessForm( + JSONSchemaBuilder() + .addInput({ + nestedFieldset: mockNestedFieldset, + }) + .build() + ); - // The "child.has_child" is required - expect(validateForm({})).toEqual({ - child: { - has_child: 'Required field', - }, + expect(result).toMatchObject({ + fields: [ + { + label: 'Nested fieldset title', + description: 'Nested fieldset description', + name: 'nestedFieldset', + type: 'fieldset', + required: false, + fields: [ + { + description: 'Fieldset description', + label: 'Fieldset title', + name: 'innerFieldset', + type: 'fieldset', + required: false, + fields: [ + { + description: 'The number of your national identification (max 10 digits)', + label: 'ID number', + name: 'id_number', + type: 'text', + required: true, + }, + { + description: 'How many open tabs do you have?', + label: 'Tabs', + maximum: 10, + minimum: 1, + name: 'tabs', + type: 'number', + required: false, + }, + ], + }, + ], + }, + ], + }); }); - // The "child.no" is valid - expect( - validateForm({ + it('supported "fieldset" with scoped conditionals', () => { + const { handleValidation } = createHeadlessForm(schemaFieldsetScopedCondition, {}); + const validateForm = (vals) => friendlyError(handleValidation(vals)); + + // The "child.has_child" is required + expect(validateForm({})).toEqual({ child: { - has_child: 'no', + has_child: 'Required field', }, - }) - ).toBeUndefined(); + }); - // Invalid because it expect child.age too - expect( - validateForm({ + // The "child.no" is valid + expect( + validateForm({ + child: { + has_child: 'no', + }, + }) + ).toBeUndefined(); + + // Invalid because it expect child.age too + expect( + validateForm({ + child: { + has_child: 'yes', + }, + }) + ).toEqual({ child: { - has_child: 'yes', + age: 'Required field', }, - }) - ).toEqual({ - child: { - age: 'Required field', - }, + }); + + // Valid without optional child.passport_id + expect( + validateForm({ + child: { + has_child: 'yes', + age: 15, + }, + }) + ).toBeUndefined(); + + // Valid with optional child.passport_id + expect( + validateForm({ + child: { + has_child: 'yes', + age: 15, + passport_id: 'asdf', + }, + }) + ).toBeUndefined(); }); - // Valid without optional child.passport_id - expect( - validateForm({ - child: { - has_child: 'yes', - age: 15, - }, - }) - ).toBeUndefined(); + it('should set any nested "fieldset" form values to null when they are invisible', async () => { + const { handleValidation } = createHeadlessForm(schemaFieldsetScopedCondition, {}); + const validateForm = (vals) => friendlyError(handleValidation(vals)); - // Valid with optional child.passport_id - expect( - validateForm({ + const formValues = { child: { has_child: 'yes', age: 15, - passport_id: 'asdf', }, - }) - ).toBeUndefined(); - }); + }; - it('should set any nested "fieldset" form values to null when they are invisible', async () => { - const { handleValidation } = createHeadlessForm(schemaFieldsetScopedCondition, {}); - const validateForm = (vals) => friendlyError(handleValidation(vals)); + await expect(validateForm(formValues)).toBeUndefined(); + expect(formValues.child.age).toBe(15); - const formValues = { - child: { - has_child: 'yes', - age: 15, - }, - }; + formValues.child.has_child = 'no'; + // form value updates re-validate; see computeYupSchema() + await expect(validateForm(formValues)).toBeUndefined(); + + // when child.has_child is 'no' child.age is invisible + expect(formValues.child.age).toBe(null); + }); + + describe('supports conditionals to fieldsets', () => { + // To not mix the concepts: + // - Scoped conditionals: Conditionals written inside a fieldset + // - Conditionals to fieldsets: Root conditionals that affect a fieldset + + // This describe has sequential tests, covering the following: + // If the working_hours > 30, + // Then the fieldset perks.food changes (the "no" option gets removed) - await expect(validateForm(formValues)).toBeUndefined(); - expect(formValues.child.age).toBe(15); + // Setup (arrange) + const { fields, handleValidation } = createHeadlessForm(schemaWithConditionalToFieldset); - formValues.child.has_child = 'no'; - // form value updates re-validate; see computeYupSchema() - await expect(validateForm(formValues)).toBeUndefined(); + const validateForm = (vals) => friendlyError(handleValidation(vals)); + const originalFood = getField(fields, 'perks', 'food'); + + const perksForLowWorkHours = { + food: 'no', // this option will be removed when the condition happens. + retirement: 'basic', + }; + + it('by default, the Perks.food has 4 options', () => { + expect(originalFood.options).toHaveLength(4); + expect(originalFood.description).toBeUndefined(); + + // Ensure the perks are required + expect(validateForm({})).toEqual({ + work_hours_per_week: 'Required field', + perks: { + food: 'Required field', + retirement: 'Required field', + }, + }); + + // Given low work hours, the form is valid. + expect( + validateForm({ + work_hours_per_week: 5, + perks: perksForLowWorkHours, + }) + ).toBeUndefined(); + }); + + it('Given a lot work hours, the perks.food options change', () => { + expect( + validateForm({ + work_hours_per_week: 35, + }) + ).toEqual({ + pto: 'Required field', // Sanity-check - this field gets required too. + perks: { + food: 'Required field', + retirement: 'Required field', + }, + }); - // when child.has_child is 'no' child.age is invisible - expect(formValues.child.age).toBe(null); + // The fieldset changed! + const foodField = getField(fields, 'perks', 'food'); + // perks.food options changed ("No" was removed) + expect(foodField.options).toHaveLength(3); + + // Ensure the "no" option is no longer accepted: + // This is a very important test in case the UI fails for some reason. + expect( + validateForm({ + work_hours_per_week: 35, + pto: 20, + perks: perksForLowWorkHours, + }) + ).toEqual({ + perks: { + food: 'The option "no" is not valid.', + }, + }); + + // perks.food has a new description + expect(foodField.description).toBe("Above 30 hours, the 'no' option disappears."); + // pto has a new description + expect(getField(fields, 'pto').description).toBe( + 'Above 30 hours, the PTO needs to be at least 20 days.' + ); + + // Sanity-check: Now the PTO also has a minimum value + expect( + validateForm({ + work_hours_per_week: 35, + pto: 5, // too low + perks: { food: 'lunch', retirement: 'basic' }, + }) + ).toEqual({ + pto: 'Must be greater or equal to 20', + }); + }); + + it('When changing back to low work hours, the perks.food goes back to the original state', () => { + expect( + validateForm({ + work_hours_per_week: 10, + pto: 5, + }) + ).toEqual({ + perks: { + food: 'Required field', + retirement: 'Required field', + }, + // ...pto is minimum error is gone! (sanity-check) + }); + + const foodField = getField(fields, 'perks', 'food'); + // ...Number of perks.food options was back to the original (4) + expect(foodField.options).toHaveLength(4); + // ...Food description was back to the original + expect(foodField.description).toBeUndefined(); + + // @BUG RMT-58 PTO description should disappear, but it didn't. + expect(getField(fields, 'pto').description).toBe( + 'Above 30 hours, the PTO needs to be at least 20 days.' + ); + + // Given again "low perks", the form valid. + expect( + validateForm({ + work_hours_per_week: 10, + perks: perksForLowWorkHours, + }) + ).toBeUndefined(); + }); + }); }); it('support "email" field type', () => { @@ -1834,14 +1974,12 @@ describe('createHeadlessForm', () => { strictInputType: false, }); - const getByName = (fieldList, name) => fieldList.find((f) => f.name === name); - - const aFieldInRoot = getByName(fields, 'a_string'); + const aFieldInRoot = getField(fields, 'a_string'); // It's the entire json schema expect(aFieldInRoot.scopedJsonSchema).toEqual(schemaWithoutInputTypes); - const aFieldset = getByName(fields, 'a_object'); - const aFieldInTheFieldset = getByName(aFieldset.fields, 'foo'); + const aFieldset = getField(fields, 'a_object'); + const aFieldInTheFieldset = getField(aFieldset.fields, 'foo'); // It's only the json schema of that fieldset expect(aFieldInTheFieldset.scopedJsonSchema).toEqual( @@ -2301,8 +2439,6 @@ describe('createHeadlessForm', () => { }); describe('when a JSON Schema is provided', () => { - const getByName = (fields, name) => fields.find((f) => f.name === name); - describe('and all fields are optional', () => { let handleValidation; const validateForm = (vals) => friendlyError(handleValidation(vals)); @@ -2709,23 +2845,23 @@ describe('createHeadlessForm', () => { const { fields } = createHeadlessForm(schemaWithConditionalReadOnlyProperty, { field_a: null, }); - expect(getByName(fields, 'field_b').isVisible).toBe(false); + expect(getField(fields, 'field_b').isVisible).toBe(false); }); it('given a match, runs "then" (turns visible and editable)', () => { const { fields } = createHeadlessForm(schemaWithConditionalReadOnlyProperty, { initialValues: { field_a: 'yes' }, }); - expect(getByName(fields, 'field_b').isVisible).toBe(true); - expect(getByName(fields, 'field_b').readOnly).toBe(false); + expect(getField(fields, 'field_b').isVisible).toBe(true); + expect(getField(fields, 'field_b').readOnly).toBe(false); }); it('given a nested match, runs "else-then" (turns visible but readOnly)', () => { const { fields } = createHeadlessForm(schemaWithConditionalReadOnlyProperty, { initialValues: { field_a: 'no' }, }); - expect(getByName(fields, 'field_b').isVisible).toBe(true); - expect(getByName(fields, 'field_b').readOnly).toBe(true); + expect(getField(fields, 'field_b').isVisible).toBe(true); + expect(getField(fields, 'field_b').readOnly).toBe(true); }); }); @@ -2737,26 +2873,26 @@ describe('createHeadlessForm', () => { initialValues: { field_a: null, field_a_wrong: null }, }); // The dependent correct field gets hidden, but... - expect(getByName(fieldsEmpty, 'field_b').isVisible).toBe(false); + expect(getField(fieldsEmpty, 'field_b').isVisible).toBe(false); // ...the dependent wrong field stays visible because the // conditional is wrong (it's missing the if.required[]) - expect(getByName(fieldsEmpty, 'field_b_wrong').isVisible).toBe(true); + expect(getField(fieldsEmpty, 'field_b_wrong').isVisible).toBe(true); }); it('given a match ("yes"), both runs "then" (turn visible)', () => { const { fields: fieldsVisible } = createHeadlessForm(schemaWithWrongConditional, { initialValues: { field_a: 'yes', field_a_wrong: 'yes' }, }); - expect(getByName(fieldsVisible, 'field_b').isVisible).toBe(true); - expect(getByName(fieldsVisible, 'field_b_wrong').isVisible).toBe(true); + expect(getField(fieldsVisible, 'field_b').isVisible).toBe(true); + expect(getField(fieldsVisible, 'field_b_wrong').isVisible).toBe(true); }); it('not given a match ("no"), both run else (stay hidden)', () => { const { fields: fieldsHidden } = createHeadlessForm(schemaWithWrongConditional, { initialValues: { field_a: 'no', field_a_wrong: 'no' }, }); - expect(getByName(fieldsHidden, 'field_b').isVisible).toBe(false); - expect(getByName(fieldsHidden, 'field_b_wrong').isVisible).toBe(false); + expect(getField(fieldsHidden, 'field_b').isVisible).toBe(false); + expect(getField(fieldsHidden, 'field_b_wrong').isVisible).toBe(false); }); }); @@ -2766,7 +2902,7 @@ describe('createHeadlessForm', () => { field_a: 'no', }, }); - const dependentField = getByName(fields, 'field_b'); + const dependentField = getField(fields, 'field_b'); expect(dependentField.isVisible).toBe(false); expect(dependentField.value).toBe(undefined); }); @@ -2777,7 +2913,7 @@ describe('createHeadlessForm', () => { field_a: 'yes', }, }); - const dependentField = getByName(fields, 'field_b'); + const dependentField = getField(fields, 'field_b'); expect(dependentField.isVisible).toBe(true); expect(dependentField.value).toBe(undefined); }); diff --git a/src/tests/helpers.js b/src/tests/helpers.js index 70ef45f1..b7f3078d 100644 --- a/src/tests/helpers.js +++ b/src/tests/helpers.js @@ -1228,8 +1228,8 @@ export const schemaWithOrderKeyword = JSONSchemaBuilder() .setOrder(['username', 'age', 'street']) .build(); -export const schemaDynamicValidationConst = JSONSchemaBuilder() - .addInput({ +export const schemaDynamicValidationConst = { + properties: { a_fieldset: mockFieldset, a_group_array: simpleGroupArrayInput, validate_tabs: { @@ -1266,8 +1266,8 @@ export const schemaDynamicValidationConst = JSONSchemaBuilder() inputType: 'radio', }, }, - }) - .addAllOf([ + }, + allOf: [ { if: { properties: { @@ -1286,27 +1286,25 @@ export const schemaDynamicValidationConst = JSONSchemaBuilder() }, }, }, - ]) - .addCondition( - { - properties: { - validate_tabs: { - const: 'yes', - }, + ], + if: { + properties: { + validate_tabs: { + const: 'yes', }, - required: ['validate_tabs'], }, - { - properties: { - a_fieldset: { - required: ['id_number', 'tabs'], - }, + required: ['validate_tabs'], + }, + then: { + properties: { + a_fieldset: { + required: ['id_number', 'tabs'], }, - } - ) - .setRequiredFields(['a_fieldset', 'validate_tabs', 'mandatory_group_array']) - .setOrder(['validate_tabs', 'a_fieldset', 'mandatory_group_array', 'a_group_array']) - .build(); + }, + }, + required: ['a_fieldset', 'validate_tabs', 'mandatory_group_array'], + 'x-jsf-order': ['validate_tabs', 'a_fieldset', 'mandatory_group_array', 'a_group_array'], +}; export const schemaDynamicValidationMinimumMaximum = JSONSchemaBuilder() .addInput({ @@ -1692,6 +1690,126 @@ export const schemaFieldsetScopedCondition = { type: 'object', }; +export const schemaWithConditionalToFieldset = { + additionalProperties: false, + type: 'object', + properties: { + work_hours_per_week: { + title: 'Hours per week', + type: 'number', + description: 'Above 30 hours, the Perk>Food options change, and PTO is required.', + 'x-jsf-presentation': { + inputType: 'number', + }, + }, + pto: { + title: 'Time-off (days)', + type: 'number', + 'x-jsf-presentation': { + inputType: 'number', + }, + }, + perks: { + additionalProperties: false, + properties: { + food: { + oneOf: [ + { + const: 'lunch', + title: 'Lunch', + }, + { + const: 'dinner', + title: 'Dinner', + }, + { + const: 'all', + title: 'All', + description: 'Every meal', + }, + { + const: 'no', + title: 'No food', + }, + ], + title: 'Food', + type: 'string', + 'x-jsf-presentation': { + inputType: 'radio', + }, + }, + retirement: { + oneOf: [ + { + const: 'basic', + title: 'Basic', + }, + { + const: 'plus', + title: 'Plus', + }, + ], + title: 'Retirement', + type: 'string', + 'x-jsf-presentation': { + inputType: 'radio', + }, + }, + }, + required: ['food', 'retirement'], + title: 'Perks', + type: 'object', + 'x-jsf-presentation': { + inputType: 'fieldset', + }, + }, + }, + allOf: [ + { + if: { + properties: { + work_hours_per_week: { + minimum: 30, + }, + }, + required: ['work_hours_per_week'], + }, + then: { + properties: { + pto: { + $comment: '@BUG: This description does not disappear once activated.', + description: 'Above 30 hours, the PTO needs to be at least 20 days.', + minimum: 20, + }, + perks: { + properties: { + food: { + description: "Above 30 hours, the 'no' option disappears.", + oneOf: [ + { + const: 'lunch', + title: 'Lunch', + }, + { + const: 'dinner', + title: 'Dinner', + }, + { + const: 'all', + title: 'all', + }, + ], + }, + }, + }, + }, + required: ['pto'], + }, + }, + ], + required: ['perks', 'work_hours_per_week'], +}; + export const schemaWorkSchedule = { type: 'object', properties: {