Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AddressNL validation on city & postcode issues #4699

Open
LaurensBurger opened this issue Sep 24, 2024 · 7 comments · Fixed by open-formulieren/open-forms-sdk#785 or open-formulieren/open-forms-sdk#786
Assignees

Comments

@LaurensBurger
Copy link
Collaborator

LaurensBurger commented Sep 24, 2024

Product versie / Product version

2.8.0

Customer reference

No response

Omschrijf het probleem / Describe the bug

The custom error mesaage is not displayed
image
image
image

The position of the postcode error is "wrong"
image

Validation triggers on "change" so any interaction with the component
image

Validation of City / Postcode only occurs on submit (back-end), which seems odd compared to any other component in OF.

Validation of City / Postcode does not scroll/focus the browser to the component - but the "required" validation does.

@LaurensBurger LaurensBurger added bug triage Issue needs to be validated. Remove this label if the issue considered valid. labels Sep 24, 2024
@sergei-maertens
Copy link
Member

Note: the SDK hasn't been updated yet, only the backend aspect has been "finished" so far (there's a notice in the changelog for this).

@joeribekker
Copy link
Contributor

Refinement: Circling back next week to see if SDK solves things.

@joeribekker
Copy link
Contributor

@LaurensBurger an you retest this on our test environment again?

@LaurensBurger
Copy link
Collaborator Author

LaurensBurger commented Nov 4, 2024

2.8.1
Validation triggers after first interaction with any of the fields

With validation (regex) set the whole component will "trigger" after loading. This keeps happening even when removing the validation
Only completely removing the component and using a new one will solve this validation trigger.

The component does not show when it's made visible with simpel logic. It does hide with the same logic reversed.
Uncaught TypeError: Cannot read properties of undefined (reading 'render') Target container is not a DOM element.

cannot continue a form when addressNL is not filled in, even when not required:
image

"Stad" should be "Plaats" of "Woonplaats"(?)

@joeribekker
Copy link
Contributor

We know this component needs more work. We're not picking this up now explicitely but in a larger adressnl ticket #4431

@sergei-maertens sergei-maertens removed the triage Issue needs to be validated. Remove this label if the issue considered valid. label Jan 6, 2025
robinmolen added a commit to open-formulieren/open-forms-sdk that referenced this issue Jan 7, 2025
@robinmolen
Copy link
Contributor

robinmolen commented Jan 8, 2025

Ive summarized the issues into two lists:

Straight forward:

Bit more complicated:

  • Display error messages from backend validation on the individual address fields (instead of below the AddressNL component)
    This will probably require some error mapping/passing around in the SDK
  • Use the custom error messages for the backend validation
    In the addressNL admin configuration you can define custom error messages. These are only shown when the frontend validation is applied. The backend validation gives the normal/generic error messages.
    Normally the custom error messages are added/used in backend validation, but only for top-level components (i'm not entirely sure how this works for repeating groups). We need to add the sub-components of the addressNL component to this validation process.

Notice!
This list only contains issues i was able to reproduce. The regex validation causing consistent validation triggers i wasn't able to reproduce:

With validation (regex) set the whole component will "trigger" after loading. This keeps happening even when removing the validation
Only completely removing the component and using a new one will solve this validation trigger.

@sergei-maertens
Copy link
Member

@robinmolen can you group these tasks into things that should be straight forward, and tasks that may be complicated because of the formio.js renderer + React setup? You will have to explore some code for this :)

@robinmolen robinmolen self-assigned this Jan 23, 2025
@robinmolen robinmolen moved this from Todo to In Progress in Development Jan 23, 2025
robinmolen added a commit to open-formulieren/open-forms-sdk that referenced this issue Jan 23, 2025
robinmolen added a commit to open-formulieren/open-forms-sdk that referenced this issue Jan 23, 2025
…when not hidden

The AddressNL component should only be rendered when it's not hidden. Otherwise, the attach function will call `createRoot` with an argument that isn't a DOM element.

So the creating of the root and, therefor, the rendering of the React component cannot be done.
If the reactRoot doesn't exist, then it also doesn't (and cannot) be unmounted. So this is also something that shouldn't happen when the component is hidden.

This works fine when the component is hidden/shown using form logic, as the `hidden` property will be updated.
@robinmolen robinmolen linked a pull request Jan 27, 2025 that will close this issue
10 tasks
robinmolen added a commit to open-formulieren/open-forms-sdk that referenced this issue Jan 27, 2025
…when not hidden

The AddressNL component should only be rendered when it's not hidden. Otherwise, the attach function will call `createRoot` with an argument that isn't a DOM element.

So the creating of the root and, therefor, the rendering of the React component cannot be done.
If the reactRoot doesn't exist, then it also doesn't (and cannot) be unmounted. So this is also something that shouldn't happen when the component is hidden.

This works fine when the component is hidden/shown using form logic, as the `hidden` property will be updated.
robinmolen added a commit that referenced this issue Jan 27, 2025
When the addressNL component is optional, the postcode and houseNumber fields should also be considered optional. But, if either postcode or houseNumber is provided, both fields become required
@github-project-automation github-project-automation bot moved this from In Progress to Done in Development Jan 28, 2025
@github-project-automation github-project-automation bot moved this from Done to In Progress in Development Jan 28, 2025
robinmolen added a commit to open-formulieren/open-forms-sdk that referenced this issue Jan 29, 2025
robinmolen added a commit to open-formulieren/open-forms-sdk that referenced this issue Jan 29, 2025
robinmolen added a commit to open-formulieren/open-forms-sdk that referenced this issue Jan 30, 2025
…er dirty

To trigger the validation once the addressNL component has changed, we have to monitor formik changes. Using the `dirty` property from the `useFormikContext` we can simplify this monitoring. Unfortunately we can only set/alter the validation on the top level. With this we have three options:

- We can add `onBlur` properties to all addressNL fields. This means that we can handle formValidation after blur, and only when `dirty === true`. This would add a lot of custom properties and logic, for a small 'problem'. This would also move us futher from a "the component functions on its own" way of working.
- We can add `onBlur` eventListeners via `useEffect`. This would give us the same functionality as the previous option, without all the custom properties. It would also make the addressNL component even more complex and introduce unexpected behavior.
- Finally we can change the validation rule based on the `dirty` property. For this to work, we would have to pass the `dirty` prop to the parent component, to then change the validation rule. It is a strange way to work, but would mean the least amount of custom behavior and keep the validation process understandable.

For simplicity sake, I've chosen the last option. To use the `dirty` prop from the formik context to change the addressNL component validation
@robinmolen robinmolen moved this from In Progress to Todo in Development Jan 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment