From 3ceac3eaafe2bcfb13c5d12249f8f2c8c1dd386b Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Mon, 2 Dec 2024 15:03:37 +0100 Subject: [PATCH] :recycle: [#4398] Refactor the TargetPathSelect component The TargetPathSelect component is now decoupled from its possible FieldArray parent, and the option label display is incorporated in the component itself so that options no longer need to be pre-processed. On top of that, it's now refactored to be based on react-select for consistency in the UI, and separate stories have been added so we can do (visual) regression testing/isolated development. We use this component in a number of places now: * the path to the auth attribute (no variablesMapping parent context), used in registration and prefill plugins for the Objects API * the path for the generic Object Types V2 registration mapping, here a variablesMapping parent container/context is relevant and different form state management semantics apply * the path to specialized AddressNL subfield mappings - a parent is relevant, but not within a bigger variable mapping so we can use the standard field assignment semantics of Formik. --- src/openforms/js/compiled-lang/en.json | 6 + src/openforms/js/compiled-lang/nl.json | 6 + .../form_design/RegistrationFields.stories.js | 10 ++ ...icObjectsApiVariableConfigurationEditor.js | 79 +++++++++++-- .../objectsapi/LegacyConfigFields.js | 15 ++- .../ObjectsApiOptionsFormFields.stories.js | 10 ++ .../ObjectsApiVariableConfigurationEditor.js | 3 +- .../objectsapi/V2ConfigFields.js | 16 ++- .../variables/VariablesEditor.stories.js | 69 ++++++----- .../forms/objects_api/AuthAttributePath.js | 31 +++-- .../forms/objects_api/TargetPathDisplay.js | 23 ---- .../forms/objects_api/TargetPathSelect.js | 107 ++++++++++++------ .../objects_api/TargetPathSelect.stories.js | 43 +++++++ .../admin/forms/objects_api/index.js | 3 +- src/openforms/js/lang/en.json | 5 + src/openforms/js/lang/nl.json | 5 + 16 files changed, 303 insertions(+), 128 deletions(-) delete mode 100644 src/openforms/js/components/admin/forms/objects_api/TargetPathDisplay.js create mode 100644 src/openforms/js/components/admin/forms/objects_api/TargetPathSelect.stories.js diff --git a/src/openforms/js/compiled-lang/en.json b/src/openforms/js/compiled-lang/en.json index 35e9958410..790c4a6d3e 100644 --- a/src/openforms/js/compiled-lang/en.json +++ b/src/openforms/js/compiled-lang/en.json @@ -6427,6 +6427,12 @@ "value": "Variable" } ], + "xBb5YI": [ + { + "type": 0, + "value": "Select an object type and version before you can pick a source path." + } + ], "xI6md8": [ { "type": 0, diff --git a/src/openforms/js/compiled-lang/nl.json b/src/openforms/js/compiled-lang/nl.json index 2c2060d55b..e354b6c13e 100644 --- a/src/openforms/js/compiled-lang/nl.json +++ b/src/openforms/js/compiled-lang/nl.json @@ -6449,6 +6449,12 @@ "value": "Variabele" } ], + "xBb5YI": [ + { + "type": 0, + "value": "Select an object type and version before you can pick a source path." + } + ], "xI6md8": [ { "type": 0, diff --git a/src/openforms/js/components/admin/form_design/RegistrationFields.stories.js b/src/openforms/js/components/admin/form_design/RegistrationFields.stories.js index 0d28050491..726b9cc2fa 100644 --- a/src/openforms/js/components/admin/form_design/RegistrationFields.stories.js +++ b/src/openforms/js/components/admin/form_design/RegistrationFields.stories.js @@ -6,6 +6,7 @@ import { mockCataloguesGet as mockObjectsApiCataloguesGet, mockObjecttypeVersionsGet, mockObjecttypesGet, + mockTargetPathsPost, } from 'components/admin/form_design/registrations/objectsapi/mocks'; import { mockCaseTypesGet, @@ -512,6 +513,15 @@ export default { ]), mockObjectsApiCataloguesGet(), mockDocumentTypesGet(), + mockTargetPathsPost({ + string: [ + { + targetPath: ['path', 'to.the', 'target'], + isRequired: true, + jsonSchema: {type: 'string'}, + }, + ], + }), ], zgwMocks: [ mockZGWApisCataloguesGet(), diff --git a/src/openforms/js/components/admin/form_design/registrations/objectsapi/GenericObjectsApiVariableConfigurationEditor.js b/src/openforms/js/components/admin/form_design/registrations/objectsapi/GenericObjectsApiVariableConfigurationEditor.js index 9c8259dec2..2530efef9b 100644 --- a/src/openforms/js/components/admin/form_design/registrations/objectsapi/GenericObjectsApiVariableConfigurationEditor.js +++ b/src/openforms/js/components/admin/form_design/registrations/objectsapi/GenericObjectsApiVariableConfigurationEditor.js @@ -1,5 +1,6 @@ -import {useFormikContext} from 'formik'; +import {FieldArray, useFormikContext} from 'formik'; import isEqual from 'lodash/isEqual'; +import PropTypes from 'prop-types'; import React, {useContext} from 'react'; import {FormattedMessage} from 'react-intl'; import {useAsync, useToggle} from 'react-use'; @@ -9,14 +10,74 @@ import {REGISTRATION_OBJECTS_TARGET_PATHS} from 'components/admin/form_design/co import Field from 'components/admin/forms/Field'; import FormRow from 'components/admin/forms/FormRow'; import {Checkbox} from 'components/admin/forms/Inputs'; -import Select, {LOADING_OPTION} from 'components/admin/forms/Select'; import {TargetPathSelect} from 'components/admin/forms/objects_api'; -import {TargetPathDisplay} from 'components/admin/forms/objects_api'; import ErrorMessage from 'components/errors/ErrorMessage'; import {post} from 'utils/fetch'; import {asJsonSchema} from './utils'; +/** + * Hack-ish way to manage the variablesMapping state for one particular entry. + * + * We ensure that an item is added to `variablesMapping` by using the `FieldArray` + * helper component if it doesn't exist yet, otherwise we update it. + */ +const MappedVariableTargetPathSelect = ({ + name, + index, + mappedVariable, + isLoading = false, + targetPaths = [], + isDisabled = false, +}) => { + const { + values: {variablesMapping = []}, + setFieldValue, + } = useFormikContext(); + const isNew = variablesMapping.length === index; + return ( + ( + { + // Clearing the select means we need to remove the record from the mapping, + // otherwise it's not a valid item for the backend. + if (newValue === null) { + arrayHelpers.remove(index); + return; + } + + // otherwise, either add a new item, or update the existing + if (isNew) { + const newMapping = {...mappedVariable, targetPath: newValue.targetPath}; + arrayHelpers.push(newMapping); + } else { + setFieldValue(name, newValue.targetPath); + } + }} + /> + )} + /> + ); +}; + +MappedVariableTargetPathSelect.propTypes = { + name: PropTypes.string.isRequired, + index: PropTypes.number.isRequired, + mappedVariable: PropTypes.shape({ + variableKey: PropTypes.string.isRequired, + targetPath: PropTypes.arrayOf(PropTypes.string), + options: PropTypes.object, + }).isRequired, + isLoading: PropTypes.bool, + isDisabled: PropTypes.bool, +}; + export const GenericEditor = ({ variable, components, @@ -56,11 +117,6 @@ export const GenericEditor = ({ const getTargetPath = pathSegment => targetPaths.find(t => isEqual(t.targetPath, pathSegment)); - const choices = - loading || error - ? LOADING_OPTION - : targetPaths.map(t => [JSON.stringify(t.targetPath), ]); - if (error) return ( @@ -108,12 +164,13 @@ export const GenericEditor = ({ } disabled={isGeometry} > - diff --git a/src/openforms/js/components/admin/form_design/registrations/objectsapi/LegacyConfigFields.js b/src/openforms/js/components/admin/form_design/registrations/objectsapi/LegacyConfigFields.js index 9830c54aa1..6f37658e0e 100644 --- a/src/openforms/js/components/admin/form_design/registrations/objectsapi/LegacyConfigFields.js +++ b/src/openforms/js/components/admin/form_design/registrations/objectsapi/LegacyConfigFields.js @@ -29,14 +29,17 @@ const onApiGroupChange = prevValues => ({ ...prevValues, objecttype: '', objecttypeVersion: undefined, + authAttributePath: undefined, }); const LegacyConfigFields = ({apiGroupChoices}) => { - const [updateExistingObject] = useField('updateExistingObject'); - const authAttributePathDisabled = !updateExistingObject.value; - const { - values: {objecttype, objecttypeVersion, objectsApiGroup}, + values: { + objectsApiGroup = null, + objecttype = '', + objecttypeVersion = null, + updateExistingObject = false, + }, } = useFormikContext(); return ( @@ -117,11 +120,11 @@ const LegacyConfigFields = ({apiGroupChoices}) => { > diff --git a/src/openforms/js/components/admin/form_design/registrations/objectsapi/ObjectsApiOptionsFormFields.stories.js b/src/openforms/js/components/admin/form_design/registrations/objectsapi/ObjectsApiOptionsFormFields.stories.js index df9ddc3a3e..0298a8a78d 100644 --- a/src/openforms/js/components/admin/form_design/registrations/objectsapi/ObjectsApiOptionsFormFields.stories.js +++ b/src/openforms/js/components/admin/form_design/registrations/objectsapi/ObjectsApiOptionsFormFields.stories.js @@ -14,6 +14,7 @@ import { mockObjecttypeVersionsGet, mockObjecttypesError, mockObjecttypesGet, + mockTargetPathsPost, } from './mocks'; const NAME = 'form.registrationBackends.0.options'; @@ -62,6 +63,15 @@ export default { ]), mockCataloguesGet(), mockDocumentTypesGet(), + mockTargetPathsPost({ + string: [ + { + targetPath: ['path', 'to.the', 'target'], + isRequired: true, + jsonSchema: {type: 'string'}, + }, + ], + }), ], }, }, diff --git a/src/openforms/js/components/admin/form_design/registrations/objectsapi/ObjectsApiVariableConfigurationEditor.js b/src/openforms/js/components/admin/form_design/registrations/objectsapi/ObjectsApiVariableConfigurationEditor.js index 570cd89e7b..65fd013801 100644 --- a/src/openforms/js/components/admin/form_design/registrations/objectsapi/ObjectsApiVariableConfigurationEditor.js +++ b/src/openforms/js/components/admin/form_design/registrations/objectsapi/ObjectsApiVariableConfigurationEditor.js @@ -8,7 +8,6 @@ import Field from 'components/admin/forms/Field'; import Fieldset from 'components/admin/forms/Fieldset'; import FormRow from 'components/admin/forms/FormRow'; import {TextInput} from 'components/admin/forms/Inputs'; -import {TargetPathDisplay} from 'components/admin/forms/objects_api'; import {AddressNlEditor} from './AddressNlObjectsApiVariableConfigurationEditor'; import {GenericEditor} from './GenericObjectsApiVariableConfigurationEditor'; @@ -37,7 +36,7 @@ const VARIABLE_CONFIGURATION_OPTIONS = { * @returns {JSX.Element} - The configuration form for the Objects API */ const ObjectsApiVariableConfigurationEditor = ({variable}) => { - const {values: backendOptions, getFieldProps, setFieldValue} = useFormikContext(); + const {values: backendOptions, getFieldProps} = useFormikContext(); const {components} = useContext(FormContext); /** @type {ObjectsAPIRegistrationBackendOptions} */ diff --git a/src/openforms/js/components/admin/form_design/registrations/objectsapi/V2ConfigFields.js b/src/openforms/js/components/admin/form_design/registrations/objectsapi/V2ConfigFields.js index 97fdd8dcbe..a3f2e2dac1 100644 --- a/src/openforms/js/components/admin/form_design/registrations/objectsapi/V2ConfigFields.js +++ b/src/openforms/js/components/admin/form_design/registrations/objectsapi/V2ConfigFields.js @@ -1,4 +1,4 @@ -import {useField, useFormikContext} from 'formik'; +import {useFormikContext} from 'formik'; import PropTypes from 'prop-types'; import {FormattedMessage} from 'react-intl'; @@ -27,15 +27,19 @@ const onApiGroupChange = prevValues => ({ ...prevValues, objecttype: '', objecttypeVersion: undefined, + authAttributePath: undefined, variablesMapping: [], }); const V2ConfigFields = ({apiGroupChoices}) => { - const [updateExistingObject] = useField('updateExistingObject'); - const authAttributePathDisabled = !updateExistingObject.value; - const { - values: {objecttype, objecttypeVersion, objectsApiGroup, variablesMapping = []}, + values: { + objectsApiGroup = null, + objecttype = '', + objecttypeVersion = null, + variablesMapping = [], + updateExistingObject = false, + }, setFieldValue, } = useFormikContext(); @@ -143,7 +147,7 @@ const V2ConfigFields = ({apiGroupChoices}) => { objectsApiGroup={objectsApiGroup} objecttypeUuid={objecttype} objecttypeVersion={objecttypeVersion} - disabled={authAttributePathDisabled} + disabled={!updateExistingObject} /> diff --git a/src/openforms/js/components/admin/form_design/variables/VariablesEditor.stories.js b/src/openforms/js/components/admin/form_design/variables/VariablesEditor.stories.js index fd51f5588c..60225156c0 100644 --- a/src/openforms/js/components/admin/form_design/variables/VariablesEditor.stories.js +++ b/src/openforms/js/components/admin/form_design/variables/VariablesEditor.stories.js @@ -1,4 +1,5 @@ import {expect, fn, screen, userEvent, waitFor, within} from '@storybook/test'; +import selectEvent from 'react-select-event'; import { mockObjectsAPIPrefillPropertiesGet, @@ -7,9 +8,10 @@ import { import {BACKEND_OPTIONS_FORMS} from 'components/admin/form_design/registrations'; import {mockTargetPathsPost} from 'components/admin/form_design/registrations/objectsapi/mocks'; +import {findReactSelectMenu} from '../../../../utils/storybookTestHelpers'; import {serializeValue} from '../../forms/VariableMapping'; import {mockObjecttypeVersionsGet, mockObjecttypesGet} from '../registrations/objectsapi/mocks'; -import {FormDecorator, withReactSelectDecorator} from '../story-decorators'; +import {FormDecorator} from '../story-decorators'; import VariablesEditor from './VariablesEditor'; BACKEND_OPTIONS_FORMS.testPlugin = { @@ -385,9 +387,7 @@ export const FilesMappingAndObjectAPIRegistration = { pluginVerboseName: 'Objects API registration', }, ], - onFieldChange: data => { - console.log(data); - }, + onFieldChange: fn(), }, parameters: { msw: { @@ -426,38 +426,47 @@ export const FilesMappingAndObjectAPIRegistration = { ], }, }, - play: async ({canvasElement}) => { + play: async ({canvasElement, step}) => { const canvas = within(canvasElement); const editIcons = canvas.getAllByTitle('Registratie-instellingen bewerken'); + expect(editIcons).toHaveLength(3); + + await step('Single file component', async () => { + // The second icon is for the single file upload component variable + await userEvent.click(editIcons[1]); + + const targetSchemaDropdown = await canvas.findByRole('combobox', {name: 'Bestemmingspad'}); + await expect(targetSchemaDropdown).toBeVisible(); + selectEvent.openMenu(targetSchemaDropdown); + + // Only the targets of type string should appear + const targetSelectMenu = within(await findReactSelectMenu(canvas)); + expect( + await targetSelectMenu.findByRole('option', {name: 'path > to.the > target (verplicht)'}) + ).toBeVisible(); + await expect( + await targetSelectMenu.findByRole('option', {name: 'path > to > uri (verplicht)'}) + ).toBeVisible(); + + const saveButton = canvas.getByRole('button', {name: 'Opslaan'}); + userEvent.click(saveButton); + }); - // The second icon is for the single file upload component variable - userEvent.click(editIcons[1]); - - const targetSchemaDropdown = await screen.findByRole('combobox'); - - await expect(targetSchemaDropdown).toBeInTheDocument(); - - // Only the targets of type string should appear - await expect( - await screen.findByRole('option', {name: 'path > to.the > target (verplicht)'}) - ).toBeVisible(); - await expect( - await screen.findByRole('option', {name: 'path > to > uri (verplicht)'}) - ).toBeVisible(); - - const saveButton = screen.getByRole('button', {name: 'Opslaan'}); - userEvent.click(saveButton); - - // The third icon is for the multiple file upload component variable - userEvent.click(editIcons[2]); + await step('Multi file component', async () => { + // The third icon is for the multiple file upload component variable + await userEvent.click(editIcons[2]); - const dropdown = await screen.findByRole('combobox'); + const targetSchemaDropdown = await canvas.findByRole('combobox', {name: 'Bestemmingspad'}); + await expect(targetSchemaDropdown).toBeVisible(); + selectEvent.openMenu(targetSchemaDropdown); - await expect(dropdown).toBeInTheDocument(); - await expect( - await screen.findByRole('option', {name: 'path > to > array (verplicht)'}) - ).toBeVisible(); + // Only the targets of type array should appear + const targetSelectMenu = within(await findReactSelectMenu(canvas)); + expect( + await targetSelectMenu.findByRole('option', {name: 'path > to > array (verplicht)'}) + ).toBeVisible(); + }); }, }; diff --git a/src/openforms/js/components/admin/forms/objects_api/AuthAttributePath.js b/src/openforms/js/components/admin/forms/objects_api/AuthAttributePath.js index d4d0599c50..ee3e52c8a5 100644 --- a/src/openforms/js/components/admin/forms/objects_api/AuthAttributePath.js +++ b/src/openforms/js/components/admin/forms/objects_api/AuthAttributePath.js @@ -1,15 +1,13 @@ -import {useField, useFormikContext} from 'formik'; import PropTypes from 'prop-types'; import React, {useContext} from 'react'; -import {FormattedMessage} from 'react-intl'; +import {FormattedMessage, useIntl} from 'react-intl'; import {useAsync} from 'react-use'; import {APIContext} from 'components/admin/form_design/Context'; import {REGISTRATION_OBJECTS_TARGET_PATHS} from 'components/admin/form_design/constants'; import Field from 'components/admin/forms/Field'; import FormRow from 'components/admin/forms/FormRow'; -import {LOADING_OPTION} from 'components/admin/forms/Select'; -import {TargetPathDisplay, TargetPathSelect} from 'components/admin/forms/objects_api'; +import {TargetPathSelect} from 'components/admin/forms/objects_api'; import {post} from 'utils/fetch'; const AuthAttributePath = ({ @@ -20,11 +18,11 @@ const AuthAttributePath = ({ objecttypeVersion, disabled = false, }) => { - const [fieldProps] = useField({name: name, type: 'array'}); + const intl = useIntl(); const {csrftoken} = useContext(APIContext); const { loading, - value: targetPaths, + value: targetPaths = [], error, } = useAsync(async () => { if (!objectsApiGroup || !objecttypeUuid || !objecttypeVersion) return []; @@ -39,11 +37,16 @@ const AuthAttributePath = ({ } return response.data; }, [objectsApiGroup, objecttypeUuid, objecttypeVersion]); + if (error) throw error; // bubble up to nearest error boundary - const choices = - loading || error - ? LOADING_OPTION - : targetPaths.map(t => [JSON.stringify(t.targetPath), ]); + // An object type (and particular version) must be selected before you can select + // a targetpath, since it grabs the available properties from the objecttype json + // schema definition. + const noObjectTypeSelectedMessage = intl.formatMessage({ + description: + 'Object type target path selection for auth attribute message for missing options because no object type has been selected.', + defaultMessage: 'Select an object type and version before you can pick a source path.', + }); return ( @@ -64,7 +67,13 @@ const AuthAttributePath = ({ } disabled={disabled} > - + noObjectTypeSelectedMessage : undefined} + /> ); diff --git a/src/openforms/js/components/admin/forms/objects_api/TargetPathDisplay.js b/src/openforms/js/components/admin/forms/objects_api/TargetPathDisplay.js deleted file mode 100644 index bedc9d016f..0000000000 --- a/src/openforms/js/components/admin/forms/objects_api/TargetPathDisplay.js +++ /dev/null @@ -1,23 +0,0 @@ -import PropTypes from 'prop-types'; -import React from 'react'; -import {FormattedMessage} from 'react-intl'; - -const TargetPathDisplay = ({target}) => { - const path = target.targetPath.length ? target.targetPath.join(' > ') : '/ (root)'; - return ( - - ); -}; - -TargetPathDisplay.propTypes = { - target: PropTypes.shape({ - targetPath: PropTypes.arrayOf(PropTypes.string).isRequired, - isRequired: PropTypes.bool.isRequired, - }).isRequired, -}; - -export default TargetPathDisplay; diff --git a/src/openforms/js/components/admin/forms/objects_api/TargetPathSelect.js b/src/openforms/js/components/admin/forms/objects_api/TargetPathSelect.js index 17e66db1e6..74bf4d8de4 100644 --- a/src/openforms/js/components/admin/forms/objects_api/TargetPathSelect.js +++ b/src/openforms/js/components/admin/forms/objects_api/TargetPathSelect.js @@ -1,54 +1,87 @@ -import {FieldArray, useFormikContext} from 'formik'; +import {useFormikContext} from 'formik'; import PropTypes from 'prop-types'; import React from 'react'; +import {FormattedMessage} from 'react-intl'; +import {components} from 'react-select'; -import Select from 'components/admin/forms/Select'; +import ReactSelect from 'components/admin/forms/ReactSelect'; -const TargetPathSelect = ({name, index, choices, mappedVariable, disabled}) => { +export const TargetPathType = PropTypes.shape({ + targetPath: PropTypes.arrayOf(PropTypes.string).isRequired, + isRequired: PropTypes.bool.isRequired, +}); + +/** + * (String) representation of a property target, used as dropdown option labels. + */ +export const TargetPathDisplay = ({target}) => { + const path = target.targetPath.length ? target.targetPath.join(' > ') : '/ (root)'; + return ( + + ); +}; + +TargetPathDisplay.propTypes = { + target: TargetPathType.isRequired, +}; + +/** + * Dropdown that allows you to select a particular (nested) path to a JsonSchema property. + * + * The form state must use the key `variablesMapping` for the mapped variables. + */ +const TargetPathSelect = ({ + name, + isLoading = false, + targetPaths = [], + isDisabled = false, + ...props +}) => { // To avoid having an incomplete variable mapping added in the `variablesMapping` array, // It is added only when an actual target path is selected. This way, having the empty // option selected means the variable is unmapped (hence the `arrayHelpers.remove` call below). - const { - values: {variablesMapping}, - getFieldProps, - setFieldValue, - } = useFormikContext(); - const props = getFieldProps(name); - const isNew = !!variablesMapping ? variablesMapping.length === index : false; + const {getFieldProps, setFieldValue} = useFormikContext(); + const {value} = getFieldProps(name); + + const options = targetPaths.map(({targetPath, isRequired}) => ({ + value: JSON.stringify(targetPath), + label: targetPath.length ? targetPath.join(' > ') : '/ (root)', + targetPath: targetPath, + isRequired: isRequired, + })); + return ( - ( -