From e4b95ae65519c57672cf8cc8310f0359dd16936b Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Wed, 1 Nov 2023 17:44:28 +0100 Subject: [PATCH 1/2] :test_tube: [open-formulieren/open-forms#3536] Add regression test for appointment form crash --- .../CreateAppointment.spec.js | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/src/components/appointments/CreateAppointment/CreateAppointment.spec.js b/src/components/appointments/CreateAppointment/CreateAppointment.spec.js index c8c3f02a3..4205918ed 100644 --- a/src/components/appointments/CreateAppointment/CreateAppointment.spec.js +++ b/src/components/appointments/CreateAppointment/CreateAppointment.spec.js @@ -233,3 +233,25 @@ describe('Preselecting a product via querystring', () => { expect(await screen.findByText('Paspoort aanvraag')).toBeVisible(); }); }); + +describe('Changing the product amounts', () => { + // regression test for https://github.com/open-formulieren/open-forms/issues/3536 + it('does not crash when clearing the amount field to enter a value', async () => { + mswServer.use(mockSubmissionPost(buildSubmission({steps: []})), mockAppointmentProductsGet); + const user = userEvent.setup({delay: null}); + + renderApp(); + + const productDropdown = await screen.findByRole('combobox'); + expect(productDropdown).toBeVisible(); + + const amountInput = screen.getByLabelText('Amount'); + expect(amountInput).toBeVisible(); + expect(amountInput).toHaveDisplayValue('1'); + + // clear the field value to enter a different value + await user.clear(amountInput); + await user.type(amountInput, '3'); + expect(amountInput).toHaveDisplayValue('3'); + }); +}); From ba7f83f031a83cd9263f2fabbf5230966149f35e Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Wed, 1 Nov 2023 18:06:39 +0100 Subject: [PATCH 2/2] :adhesive_bandage: [open-formulieren/open-forms#3536] Fix crash because of requiring unvalidated data to be valid The selected product IDs are required as query string inputs, but the submitted data/user input at this point is not validated yet. Using Zod to normalize the data causes crashes when the user leaves the amount field empty or enters an invalid value (like 0 or a negative value). Relaxing the validation schema is not an option, because that causes problems downstream. Instead, we discard the amount information as it wasn't used up until now yet anyway and only pass the list of product IDs that are selected, filtering out empty-ish values which accounts for no-choice-made-yet in the product dropdown. --- src/components/appointments/fields/Product.js | 7 +++---- .../appointments/fields/Product.stories.js | 8 ++++---- .../appointments/fields/ProductSelect.js | 18 ++++++++---------- .../fields/ProductSelect.stories.js | 2 +- .../appointments/steps/ChooseProductStep.js | 8 ++++++-- .../steps/ChooseProductStep.spec.js | 2 +- 6 files changed, 23 insertions(+), 22 deletions(-) diff --git a/src/components/appointments/fields/Product.js b/src/components/appointments/fields/Product.js index 0d662dbcb..3cf1e02fe 100644 --- a/src/components/appointments/fields/Product.js +++ b/src/components/appointments/fields/Product.js @@ -5,7 +5,6 @@ import {defineMessage, useIntl} from 'react-intl'; import {NumberField} from 'components/forms'; import {AppointmentConfigContext} from '../Context'; -import {ProductsType} from '../types'; import ProductSelect from './ProductSelect'; export const amountLabel = defineMessage({ @@ -13,14 +12,14 @@ export const amountLabel = defineMessage({ defaultMessage: 'Amount', }); -const Product = ({namePrefix, index, selectedProducts}) => { +const Product = ({namePrefix, index, selectedProductIds}) => { const intl = useIntl(); const {supportsMultipleProducts} = useContext(AppointmentConfigContext); return (
{ Product.propTypes = { namePrefix: PropTypes.string.isRequired, index: PropTypes.number.isRequired, - selectedProducts: ProductsType.isRequired, + selectedProductIds: PropTypes.arrayOf(PropTypes.string).isRequired, }; export default Product; diff --git a/src/components/appointments/fields/Product.stories.js b/src/components/appointments/fields/Product.stories.js index 12947c81b..fe9735fac 100644 --- a/src/components/appointments/fields/Product.stories.js +++ b/src/components/appointments/fields/Product.stories.js @@ -41,17 +41,17 @@ export default { index: {table: {disable: true}}, }, args: { - selectedProducts: [], + selectedProductIds: [], supportsMultipleProducts: true, }, }; -const render = ({productId, selectedProducts, supportsMultipleProducts = true}) => { +const render = ({productId, selectedProductIds, supportsMultipleProducts = true}) => { const data_entry = PRODUCTS_DATA.find(prod => prod.productId === productId); const index = supportsMultipleProducts ? PRODUCTS_DATA.indexOf(data_entry) : 0; return ( - + ); }; @@ -97,7 +97,7 @@ export const NoMultipleProductsSupport = { }, argTypes: { productId: {table: {disable: true}}, - selectedProducts: {table: {disable: true}}, + selectedProductIds: {table: {disable: true}}, }, play: async ({canvasElement}) => { const canvas = within(canvasElement); diff --git a/src/components/appointments/fields/ProductSelect.js b/src/components/appointments/fields/ProductSelect.js index e53d051e1..a1674ea01 100644 --- a/src/components/appointments/fields/ProductSelect.js +++ b/src/components/appointments/fields/ProductSelect.js @@ -8,8 +8,6 @@ import {get} from 'api'; import {getCached, setCached} from 'cache'; import {AsyncSelectField} from 'components/forms'; -import {ProductsType} from '../types'; - const CACHED_PRODUCTS_KEY = 'appointment|all-products'; const CACHED_PRODUCTS_MAX_AGE_MS = 15 * 60 * 1000; // 15 minutes @@ -27,10 +25,10 @@ export const getAllProducts = async baseUrl => { return products; }; -const getProducts = async (baseUrl, selectedProducts, currentProductId) => { - const otherProductIds = selectedProducts - .map(p => p.productId) - .filter(productId => productId && productId !== currentProductId); +const getProducts = async (baseUrl, selectedProductIds, currentProductId) => { + const otherProductIds = selectedProductIds.filter( + productId => productId && productId !== currentProductId + ); if (!otherProductIds.length) { return await getAllProducts(baseUrl); } @@ -49,15 +47,15 @@ const getProducts = async (baseUrl, selectedProducts, currentProductId) => { return products; }; -const ProductSelect = ({name, selectedProducts}) => { +const ProductSelect = ({name, selectedProductIds}) => { const {getFieldProps} = useFormikContext(); const intl = useIntl(); const {baseUrl} = useContext(ConfigContext); const {value} = getFieldProps(name); const getOptions = useCallback( - async () => await getProducts(baseUrl, selectedProducts, value), + async () => await getProducts(baseUrl, selectedProductIds, value), // eslint-disable-next-line react-hooks/exhaustive-deps - [baseUrl, JSON.stringify(selectedProducts), value] + [baseUrl, JSON.stringify(selectedProductIds), value] ); return ( { }; ProductSelect.propTypes = { name: PropTypes.string.isRequired, - selectedProducts: ProductsType.isRequired, + selectedProductIds: PropTypes.arrayOf(PropTypes.string).isRequired, }; export default ProductSelect; diff --git a/src/components/appointments/fields/ProductSelect.stories.js b/src/components/appointments/fields/ProductSelect.stories.js index 8bd1560bb..7e84a26c9 100644 --- a/src/components/appointments/fields/ProductSelect.stories.js +++ b/src/components/appointments/fields/ProductSelect.stories.js @@ -23,7 +23,7 @@ export default { }, args: { name: 'productId', - selectedProducts: [], + selectedProductIds: [], }, argTypes: { name: {table: {disable: true}}, diff --git a/src/components/appointments/steps/ChooseProductStep.js b/src/components/appointments/steps/ChooseProductStep.js index 50691eb86..12a90a9c3 100644 --- a/src/components/appointments/steps/ChooseProductStep.js +++ b/src/components/appointments/steps/ChooseProductStep.js @@ -35,7 +35,7 @@ const chooseMultiProductSchema = z.object({products: productSchema}); const ChooseProductStepFields = ({values: {products = []}, validateForm}) => { const intl = useIntl(); const {supportsMultipleProducts} = useContext(AppointmentConfigContext); - products = productSchema.parse(products); + const selectedProductIds = products.map(p => p.productId).filter(Boolean); /** * Decorate an arrayHelper callback to invoke the validate function. @@ -84,7 +84,11 @@ const ChooseProductStepFields = ({values: {products = []}, validateForm}) => { numProducts={numProducts} onRemove={withValidate(() => arrayHelpers.remove(index))} > - + ))} diff --git a/src/components/appointments/steps/ChooseProductStep.spec.js b/src/components/appointments/steps/ChooseProductStep.spec.js index ae00c0018..537576e2a 100644 --- a/src/components/appointments/steps/ChooseProductStep.spec.js +++ b/src/components/appointments/steps/ChooseProductStep.spec.js @@ -89,6 +89,6 @@ describe('The product selection step', () => { await user.click(secondDropdown); await user.keyboard('[ArrowDown]'); expect(await screen.findByText('Paspoort aanvraag')).toBeVisible(); - expect(await screen.queryByText('Not available with drivers license')).not.toBeInTheDocument(); + expect(screen.queryByText('Not available with drivers license')).not.toBeInTheDocument(); }); });