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

✨ [#4344] Configurable objects API group in ZGW registration #4758

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

stevenbal
Copy link
Contributor

@stevenbal stevenbal commented Oct 15, 2024

previously this was not configurable and always used the ObjectsAPIGroup with the lowest pk to create the client when registering the submission

Closes #4344

Changes

  • Configurable objects API group in ZGW registration

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

@stevenbal stevenbal marked this pull request as draft October 15, 2024 13:13
Copy link

codecov bot commented Oct 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.56%. Comparing base (bfc679d) to head (0c7ae1c).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4758   +/-   ##
=======================================
  Coverage   96.56%   96.56%           
=======================================
  Files         746      746           
  Lines       25202    25205    +3     
  Branches     3318     3318           
=======================================
+ Hits        24336    24339    +3     
  Misses        602      602           
  Partials      264      264           

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

@stevenbal stevenbal force-pushed the feature/4344-zgw-registration-choose-objects-api-group branch from 59eab2f to 89df9de Compare October 15, 2024 13:25
previously this was not configurable and always used the ObjectsAPIGroup with the lowest pk to create the client when registering the submission
@stevenbal stevenbal force-pushed the feature/4344-zgw-registration-choose-objects-api-group branch from 89df9de to 0c7ae1c Compare October 15, 2024 13:52
@stevenbal stevenbal marked this pull request as ready for review October 15, 2024 13:53
Copy link
Member

@sergei-maertens sergei-maertens left a comment

Choose a reason for hiding this comment

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

You'll also need to update some stories since they now fail to render in chromatic :)

@@ -18,8 +18,12 @@ const ZGWOptionsForm = ({index, name, label, schema, formData, onChange}) => {
const [modalOpen, setModalOpen] = useState(false);
const validationErrors = useContext(ValidationErrorContext);

const {zgwApiGroup, zaakVertrouwelijkheidaanduiding} = schema.properties;
const {zgwApiGroup, zaakVertrouwelijkheidaanduiding, objectsApiGroup} = schema.properties;
Copy link
Member

Choose a reason for hiding this comment

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

please update the component prop types accordingly

Comment on lines +581 to +583
api_group = options.get(
"objects_api_group", ObjectsAPIGroupConfig.objects.order_by("pk").first()
)
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to remove this default/fallback behaviour entirely so that all configuration is explicit, and we can then also define this accordingly in the options type declarations.

api_group = options["objects_api_group"]
assert api_group is not None

@@ -37,6 +39,7 @@ class RegistrationOptions(TypedDict):
organisatie_rsin: NotRequired[str]
zaak_vertrouwelijkheidaanduiding: NotRequired[VertrouwelijkheidAanduiding]
medewerker_roltype: NotRequired[str]
objects_api_group: NotRequired[ObjectsAPIGroupConfig]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
objects_api_group: NotRequired[ObjectsAPIGroupConfig]
objects_api_group: ObjectsAPIGroupConfig | None

I think we can enforce this key being set at all times. If the value is None, then no objects API registration is enabled (and we expect objecttype and objecttype_version to not be set at all, but either way they're ignored)

Comment on lines +130 to +133
<ObjectsAPIGroup
apiGroupChoices={objectsApiGroupChoices}
onApiGroupChange={onApiGroupChange}
/>
Copy link
Member

Choose a reason for hiding this comment

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

I think this one should be isClearable to disable objects API integration again? Though clearing the objecttype field has the same effect

),
help_text=_("Which Objects API set to use."),
label=_("Objects API set"),
required=False,
Copy link
Member

Choose a reason for hiding this comment

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

I think we may need an allow_null=True to make it explicit that no objects API integration is done.

There's some more steps involved in this PR I think:

  • A data migration that updates existing registration backends
    • if no objecttype is specified, ensure that objects_api_group is set to None
    • if an objecttype is specified, set the objects_api_group to the lowest PK (use that queryset that is now used at runtime)
    • this behaviour also needs to be applied during form import time when legacy forms are being imported (!), we can probably do something with the DEFINITION_CONVERTERS
  • Validation! Once we know which API group is used, we know the API root URl of the objecttypes API, and can check the URL provided. We can even look up the provided object type and check that it exists, but let's not do that yet because credentials to the object types API may not be configured yet. This should only come up when people edit the form again, and should not affect existing runtime behaviour of forms.

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.

Add option to select Objects API Group to use in the ZGW registration
2 participants