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

✨ [#4993] Implement fetching select(boxes) options from Referentielijsten #4996

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

stevenbal
Copy link
Contributor

@stevenbal stevenbal commented Jan 7, 2025

Closes #4993

TODO:

  • generic ExternalServiceUnavailable error which should result in a 503 from our API

Changes

  • Implement fetching select(boxes) options from Referentielijsten
  • Add docker-compose file and fixtures for Referentielijsten

Checklist

Check off the items that are completed or not relevant.

  • Impact on features

    • Checked copying a form
    • Checked import/export of a form
    • Config checks in the configuration overview admin page
    • Problem detection in the admin email digest is handled
  • Release management

    • I have labelled the PR as "needs-backport" accordingly
  • I have updated the translations assets (you do NOT need to provide translations)

    • Ran ./bin/makemessages_js.sh
    • Ran ./bin/compilemessages_js.sh
  • Dockerfile/scripts

    • Updated the Dockerfile with the necessary scripts from the ./bin folder
  • Commit hygiene

    • Commit messages refer to the relevant Github issue
    • Commit messages explain the "why" of change, not the how

@stevenbal stevenbal marked this pull request as draft January 7, 2025 14:33
Copy link

codecov bot commented Jan 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.63%. Comparing base (f16d700) to head (714d33b).
Report is 16 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4996      +/-   ##
==========================================
+ Coverage   96.62%   96.63%   +0.01%     
==========================================
  Files         761      763       +2     
  Lines       25961    26068     +107     
  Branches     3394     3402       +8     
==========================================
+ Hits        25084    25191     +107     
  Misses        611      611              
  Partials      266      266              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@stevenbal stevenbal force-pushed the feature/4993-referentielijsten-populate-component-choices branch from dbb75e3 to b82f2ab Compare January 7, 2025 15:28
@stevenbal stevenbal force-pushed the feature/4993-referentielijsten-populate-component-choices branch 6 times, most recently from adc42bb to 9881217 Compare January 13, 2025 12:59
@stevenbal stevenbal changed the title 🚧 [#4993] Implement fetching select(boxes) options from Referentielijsten ✨ [#4993] Implement fetching select(boxes) options from Referentielijsten Jan 13, 2025
@stevenbal stevenbal force-pushed the feature/4993-referentielijsten-populate-component-choices branch 4 times, most recently from 45ea58b to 99217fa Compare January 13, 2025 15:18
Comment on lines 278 to 281
except Exception as e:
if detail := getattr(e, "detail", None):
raise APIException(detail)
raise e
Copy link
Contributor Author

@stevenbal stevenbal Jan 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is in preparation to display a proper error message to the user in case the referentielijsten options can't be fetched. I'm not sure if this is the right approach to get a specific message though, it currently looks like this:

image

Preferably I think the "detail" message should be displayed in the error block, which would require changes in the SDK

@stevenbal stevenbal marked this pull request as ready for review January 13, 2025 15:35
@stevenbal stevenbal force-pushed the feature/4993-referentielijsten-populate-component-choices branch from 99217fa to 5b99dc0 Compare January 13, 2025 15:35
src/openforms/formio/dynamic_config/dynamic_options.py Outdated Show resolved Hide resolved
src/openforms/formio/dynamic_config/dynamic_options.py Outdated Show resolved Hide resolved
src/openforms/formio/dynamic_config/referentielijsten.py Outdated Show resolved Hide resolved
src/openforms/formio/dynamic_config/referentielijsten.py Outdated Show resolved Hide resolved
src/openforms/formio/dynamic_config/referentielijsten.py Outdated Show resolved Hide resolved
src/openforms/formio/dynamic_config/referentielijsten.py Outdated Show resolved Hide resolved
Comment on lines 270 to +281
# invoke the configured form logic to dynamically update the Formio.js configuration
new_configuration = evaluate_form_logic(
instance.submission,
instance,
instance.submission.data,
**self.context,
)
try:
new_configuration = evaluate_form_logic(
instance.submission,
instance,
instance.submission.data,
**self.context,
)
except Exception as e:
if detail := getattr(e, "detail", None):
raise APIException(detail)
raise e # pragma: no cover
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently the philosophy of OF is "continue at all costs" and this breaks. Let's discuss with Joeri what the expected behaviour is here.

I'm also out of the loop now what the service fetch behaviour is 🤔

Copy link
Contributor Author

@stevenbal stevenbal Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the issue this was mentioned:

If the API is unavailable or returns an error, Open Forms must display a user-friendly error message to the end user.

My assumption was to let the form crash, because the user won't be able to select a valid option (because they could not be fetched), which means that they cannot fill out the form. But it might be good to discuss this indeed 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also out of the loop now what the service fetch behaviour is 🤔

Double checked this, with service fetch set up to fetch options from referentielijsten it does indeed continue and show no available options (Geen opties om te kiezen)

@stevenbal stevenbal marked this pull request as draft January 14, 2025 14:34
…sten

this was previously possible with logic and service fetch, but this functionality provides a shortcut to more easily integrate with Re ferentielijsten API
to be used for unittests with VCR and local development
@stevenbal stevenbal force-pushed the feature/4993-referentielijsten-populate-component-choices branch from 5b99dc0 to 714d33b Compare January 14, 2025 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When loading the formstep perform a service fetch operation to populate the component choices
2 participants