Skip to content

Commit

Permalink
🩹 [open-formulieren/open-forms#3536] Fix crash because of requiring u…
Browse files Browse the repository at this point in the history
…nvalidated 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.
  • Loading branch information
sergei-maertens committed Nov 1, 2023
1 parent e4b95ae commit ba7f83f
Show file tree
Hide file tree
Showing 6 changed files with 23 additions and 22 deletions.
7 changes: 3 additions & 4 deletions src/components/appointments/fields/Product.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,22 +5,21 @@ 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({
description: 'Appointments: product amount field label',
defaultMessage: 'Amount',
});

const Product = ({namePrefix, index, selectedProducts}) => {
const Product = ({namePrefix, index, selectedProductIds}) => {
const intl = useIntl();
const {supportsMultipleProducts} = useContext(AppointmentConfigContext);
return (
<div className="openforms-form-field-container">
<ProductSelect
name={`${namePrefix}[${index}].productId`}
selectedProducts={selectedProducts}
selectedProductIds={selectedProductIds}
/>
<NumberField
name={`${namePrefix}[${index}].amount`}
Expand All @@ -38,7 +37,7 @@ const Product = ({namePrefix, index, selectedProducts}) => {
Product.propTypes = {
namePrefix: PropTypes.string.isRequired,
index: PropTypes.number.isRequired,
selectedProducts: ProductsType.isRequired,
selectedProductIds: PropTypes.arrayOf(PropTypes.string).isRequired,
};

export default Product;
8 changes: 4 additions & 4 deletions src/components/appointments/fields/Product.stories.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
<AppointmentConfigContext.Provider value={{supportsMultipleProducts}}>
<Product namePrefix="products" index={index} selectedProducts={selectedProducts} />
<Product namePrefix="products" index={index} selectedProductIds={selectedProductIds} />
</AppointmentConfigContext.Provider>
);
};
Expand Down Expand Up @@ -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);
Expand Down
18 changes: 8 additions & 10 deletions src/components/appointments/fields/ProductSelect.js
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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);
}
Expand All @@ -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 (
<AsyncSelectField
Expand All @@ -72,6 +70,6 @@ const ProductSelect = ({name, selectedProducts}) => {
};
ProductSelect.propTypes = {
name: PropTypes.string.isRequired,
selectedProducts: ProductsType.isRequired,
selectedProductIds: PropTypes.arrayOf(PropTypes.string).isRequired,
};
export default ProductSelect;
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ export default {
},
args: {
name: 'productId',
selectedProducts: [],
selectedProductIds: [],
},
argTypes: {
name: {table: {disable: true}},
Expand Down
8 changes: 6 additions & 2 deletions src/components/appointments/steps/ChooseProductStep.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -84,7 +84,11 @@ const ChooseProductStepFields = ({values: {products = []}, validateForm}) => {
numProducts={numProducts}
onRemove={withValidate(() => arrayHelpers.remove(index))}
>
<Product namePrefix="products" index={index} selectedProducts={products} />
<Product
namePrefix="products"
index={index}
selectedProductIds={selectedProductIds}
/>
</ProductWrapper>
))}
</EditGrid>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
});

0 comments on commit ba7f83f

Please sign in to comment.