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

Improve ZGW registration optionx UX - add catalogue selection #4609

Merged

Conversation

sergei-maertens
Copy link
Member

@sergei-maertens sergei-maertens commented Aug 19, 2024

Partially closes #4606

Changes

  • Allow setting a (default) catalogue at the api group level, validating the configuration against the configured catalogi API
  • Allow specifying a catalogue on the serializer (overriding the api group, if it is set)
  • Validates the case type and document type against he catalogue, if it's provided (either on serializer or model level)
  • Add additional tests for catalogue validation
  • Add catalogue selection in UI

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
  • Commit hygiene

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

@sergei-maertens sergei-maertens force-pushed the feature/4606-improve-zgw-registration-options-ux branch from 6dbf337 to 401b543 Compare August 19, 2024 19:36
@sergei-maertens sergei-maertens changed the title Improve ZGW registration optionx UX Improve ZGW registration optionx UX - add catalogue selection Aug 19, 2024
Copy link

codecov bot commented Aug 19, 2024

Codecov Report

Attention: Patch coverage is 98.05195% with 3 lines in your changes missing coverage. Please review.

Project coverage is 96.66%. Comparing base (bc1ac4d) to head (de55088).
Report is 14 commits behind head on master.

Files Patch % Lines
src/openforms/contrib/zgw/validators.py 83.33% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4609      +/-   ##
==========================================
+ Coverage   96.65%   96.66%   +0.01%     
==========================================
  Files         727      731       +4     
  Lines       24426    24526     +100     
  Branches     3218     3221       +3     
==========================================
+ Hits        23609    23709     +100     
  Misses        564      564              
  Partials      253      253              

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

@sergei-maertens sergei-maertens force-pushed the feature/4606-improve-zgw-registration-options-ux branch from 81ea5e6 to 221759a Compare August 20, 2024 14:31
* Added tests for the database check constraints
* Added tests for the catalogue validation
Objects API and ZGW APIs registration backends both use the same
option structure for the catalogue, so we can re-use the serializer to
have a single source of truth.
Added serializer-level validation for the catalogue options, and if a
catalogue is specified anywhere, validate the case type and document
type more strictly against it.

If no catalogue is specified, the existing behaviour should still
apply, making this new feature (and increased strictness) opt-in.
…alogue

When a catalogue is specified, the case/document type URLs need to be
validated not only against the ztc_service configured on the API
group, but they also must be members of the specified catalogue.

This makes the transition to dynamic resolution smoother.
@sergei-maertens sergei-maertens force-pushed the feature/4606-improve-zgw-registration-options-ux branch from 57f84ca to 37a1e8c Compare August 21, 2024 08:00
Since Objects API and ZGW APIs registration options both make use of
this functionality, we can make it agnostic and move it into a
specialized form components directory for ZGW APIs interaction.
API endpoint has been normalized to be consistent with Objects API
endpoint
@sergei-maertens sergei-maertens marked this pull request as ready for review August 21, 2024 09:48
src/openforms/contrib/zgw/serializers.py Show resolved Hide resolved
informatieobjecttype_exists = any(
informatieobjecttype_url in catalogus["informatieobjecttypen"]
for catalogus in catalogi
def _validate_against_catalogi_api(attrs: RegistrationOptions) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we put all these validators in the validators.py?

Copy link
Member Author

Choose a reason for hiding this comment

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

in my mind, validators.py is more intended for re-usable validators, and this validation helper is tied very strongly to the particular serializer that it's pretty much impossible to re-use.

These functions basically only exist to make the code a bit more readable, this could as well just have been directly implemented in def valiate(self, attrs): entirely, but that became such a huge chunk of code that I found it not maintainable anymore.

I also dislike the indirection created by moving private helper functions into different files, which requires me to click around more to see what the shape of data is actually like.

Then finally, validators.py are aimed at the django model layer, while these helpers are intended for DRF serializers. Unfortunately, these validators are not interchangeable (I tried building a generic solution but it gets real messy real quick). So, validators.py is used in the admin, while the serializer is used in the API endpoints used by the form designer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Understood and yes after your explanation makes sense.


// Components

const CatalogueSelect = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Again confused..why do we have two separate components (one in objectsAPI and one in ZGW)?Perhaps it has to do with my question above concerning the serializers in the backend.

Copy link
Member Author

Choose a reason for hiding this comment

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

both use the same generic components/admin/forms/zgw/CatalogueSelect, so they're both specializations of that one.

They are different because the registration plugins are different - objects API should not be aware of ZGW or vice versa. Additionally, the context of each select is different for each plugin, which can result in different form field labels, help texts or even catalogue selection options. In particular here, the catalogue select is disabled if no API group has been selected yet, and for Objects API the form field is objectsApiGroup, while for ZGW APIs the form field for the API group is zgwApiGroup. Additionally, the available options are loaded from their plugin-specific API endpoints.

@sergei-maertens sergei-maertens merged commit 15292df into master Aug 22, 2024
11 of 12 checks passed
@sergei-maertens sergei-maertens deleted the feature/4606-improve-zgw-registration-options-ux branch August 22, 2024 09:24
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.

Make ZGW registration options consistent with Objects API
2 participants