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

Add Product to the ZGW API registration #4803

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

robinmolen
Copy link
Contributor

@robinmolen robinmolen commented Oct 30, 2024

Closes #4796

Changes

Added product as a configuration option to the ZGW API registration. The product select gets populated from the selected case type. If there are multiple versions of the case type, then the case type version that is active at the moment of configuring is chosen.

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

@robinmolen robinmolen force-pushed the feature/4796-zgw-configuration-select-product branch 3 times, most recently from 9e24349 to 135ff90 Compare October 30, 2024 16:16
Copy link

codecov bot commented Oct 30, 2024

Codecov Report

Attention: Patch coverage is 96.22642% with 2 lines in your changes missing coverage. Please review.

Project coverage is 96.55%. Comparing base (c47e11b) to head (4e229c2).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/openforms/contrib/zgw/clients/catalogi.py 87.50% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4803      +/-   ##
==========================================
- Coverage   96.55%   96.55%   -0.01%     
==========================================
  Files         748      748              
  Lines       25423    25474      +51     
  Branches     3362     3370       +8     
==========================================
+ Hits        24548    24597      +49     
  Misses        610      610              
- Partials      265      267       +2     

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

@robinmolen robinmolen force-pushed the feature/4796-zgw-configuration-select-product branch 2 times, most recently from 63d8e50 to f3c7954 Compare October 31, 2024 08:57
@robinmolen robinmolen marked this pull request as ready for review October 31, 2024 09:19
@robinmolen robinmolen marked this pull request as draft November 8, 2024 13:27
@robinmolen robinmolen force-pushed the feature/4796-zgw-configuration-select-product branch 2 times, most recently from 1b0eee6 to 3574ee1 Compare November 12, 2024 15:30
@robinmolen robinmolen marked this pull request as ready for review November 12, 2024 15:32
@robinmolen robinmolen force-pushed the feature/4796-zgw-configuration-select-product branch from 3574ee1 to 03d0f12 Compare November 13, 2024 07:04
@robinmolen robinmolen force-pushed the feature/4796-zgw-configuration-select-product branch from 03d0f12 to 4e229c2 Compare November 14, 2024 09:41
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.

I'd suggest renaming product -> product_url everywhere so that it's clear we're dealing with a URL. Especially because we're moving literal URL-configuration options to their semantic 'description' and resolve URLs dynamically, I suspect we will go through this at some point too (where maybe a product slug/description will be stored instead of the exact URL).

Comment on lines +67 to +72
flask_app:
build: ./open-zaak
ports:
- "80:80"
volumes:
- ./open-zaak/:/app/
Copy link
Member

Choose a reason for hiding this comment

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

Let's put this in it's own docker-compose and directory and not lump it in with open-zaak. The products/services API specification doesn't exist yet so it definitely can't be part of Open Zaak.

A folder named rx_mission is okay - that makes it clear which service/API we're supporting with this. Instructions for tests will then require you to bring up both open zaak and rx_mission docker compose


data = {
"url": request.base_url,
"id": "XXXXXXXX",
Copy link
Member

Choose a reason for hiding this comment

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

Maybe you can take the JSON file that was given to us, obfuscate that a little bit, but ultimately use that as a "database"? We then at least have some representative, different test data rather than every product (with different URLs) showing the same description.

Comment on lines +13 to +14
# Log the request body
app.logger.info("Request body: %s", request.data.decode("utf-8"))
Copy link
Member

Choose a reason for hiding this comment

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

Not needed, they're simple GET calls without body

@@ -48,9 +49,15 @@ class CaseType(TypedDict):
beginGeldigheid: str # ISO 8601 date string
eindeGeldigheid: NotRequired[str | None] # ISO 8601 date string or empty
concept: NotRequired[bool]
productenOfDiensten: NotRequired[list[str]] # URL pointers to products
Copy link
Member

Choose a reason for hiding this comment

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

in the Catalogi API specification it's required:

image

Suggested change
productenOfDiensten: NotRequired[list[str]] # URL pointers to products
productenOfDiensten: list[str] # URL pointers to products

Comment on lines +56 to +58
class Product(TypedDict):
uri: str
description: NotRequired[str]
Copy link
Member

Choose a reason for hiding this comment

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

Would be good to add a comment that this is non-standard and not backed by any API specification yet


const ProductSelect = ({catalogueUrl = ''}) => {
const {
values: {zgwApiGroup = null, caseTypeIdentification = null},
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
values: {zgwApiGroup = null, caseTypeIdentification = null},
values: {zgwApiGroup = null, caseTypeIdentification = ''},

we're not using typescript yet, but still it's good practice to think about the data types :) this field is not nullable, ever.

Comment on lines +95 to +97
ProductSelect.propTypes = {
caseTypeIdentification: PropTypes.string,
};
Copy link
Member

Choose a reason for hiding this comment

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

proptypes don't match the actual props

case_type_identification = filter_serializer.validated_data[
"case_type_identification"
]
version_valid_on = datetime_in_amsterdam(timezone.now()).date()
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
version_valid_on = datetime_in_amsterdam(timezone.now()).date()
today = datetime_in_amsterdam(timezone.now()).date()

@@ -72,6 +72,14 @@ class ZaakOptionsSerializer(JsonSchemaSerializerMixin, serializers.Serializer):
),
default="",
)
product = serializers.CharField(
Copy link
Member

Choose a reason for hiding this comment

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

Should be URLField, no?

Suggested change
product = serializers.CharField(
product = serializers.URLField(

@@ -32,6 +32,7 @@ class RegistrationOptions(TypedDict):
zgw_api_group: ZGWApiGroupConfig
catalogue: NotRequired[CatalogueOption]
case_type_identification: str
product: NotRequired[str]
Copy link
Member

Choose a reason for hiding this comment

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

if you do default="" on the serializer, you can drop the NotRequired and that makes code less verbose

Suggested change
product: NotRequired[str]
product: str # URL reference to a product in the case type

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants