From de731dcd23c0bd5b351eaec081f34984a6cb94e3 Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Tue, 26 Sep 2023 10:24:38 +0200 Subject: [PATCH 1/8] :art: [#3489] Replace absolute with relative imports --- src/openforms/contrib/microsoft/client.py | 6 +++--- src/openforms/contrib/microsoft/tests/factories.py | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/openforms/contrib/microsoft/client.py b/src/openforms/contrib/microsoft/client.py index 53ff539ade..7c1a9b25b5 100644 --- a/src/openforms/contrib/microsoft/client.py +++ b/src/openforms/contrib/microsoft/client.py @@ -6,9 +6,9 @@ from O365 import Account -from openforms.contrib.microsoft.constants import ConflictHandling -from openforms.contrib.microsoft.exceptions import MSAuthenticationError -from openforms.contrib.microsoft.models import MSGraphService +from .constants import ConflictHandling +from .exceptions import MSAuthenticationError +from .models import MSGraphService class MSGraphClient: diff --git a/src/openforms/contrib/microsoft/tests/factories.py b/src/openforms/contrib/microsoft/tests/factories.py index 664e616815..d808c7c237 100644 --- a/src/openforms/contrib/microsoft/tests/factories.py +++ b/src/openforms/contrib/microsoft/tests/factories.py @@ -1,6 +1,6 @@ import factory -from openforms.contrib.microsoft.models import MSGraphService +from ..models import MSGraphService class MSGraphServiceFactory(factory.django.DjangoModelFactory): From 5a1e4a708d2f7f62fb0e42fe7e0b3c9e4ab7db5a Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Tue, 26 Sep 2023 10:36:44 +0200 Subject: [PATCH 2/8] :bulb: [#3489] Document why the microsoft graph client is different The library client is already very specialized and it's recommended to use those interfaces rather than rolling our own HTTP communication implementation. mTLS is not relevant here, and as it uses oauth2, configuring tenant ID, client ID and secret is sufficient - it is a cloud service. --- src/openforms/contrib/microsoft/client.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/openforms/contrib/microsoft/client.py b/src/openforms/contrib/microsoft/client.py index 7c1a9b25b5..ece87f4441 100644 --- a/src/openforms/contrib/microsoft/client.py +++ b/src/openforms/contrib/microsoft/client.py @@ -1,3 +1,12 @@ +""" +Microsoft (graph) API client. + +This client implementation wraps around the API client/integration implemented in the +O365 package. While it uses requests-oauth2client under the hood, we opt to *not* +use our own :module:`api_client` implementation here, as the typical Dutch API/service +requirements such as mTLS are not relevant. The service model definition also does not +allow configuring any of those aspects. +""" import json import os from io import BytesIO From e51c211f998d536f3ea39572be4ea8f42c863b5a Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Tue, 26 Sep 2023 10:56:54 +0200 Subject: [PATCH 3/8] :truck: [#3489] Merge openforms.locations into openforms.contrib.kadaster It was only interfacing with the BAG API anyway, and now the geo endpoints code is co-located. The existing endpoint still works, and it was already deprecated earlier. In Open Forms 3.0 we can make the breaking change. --- src/openapi.yaml | 44 ++++++++++- src/openforms/api/urls.py | 9 ++- .../contrib/kadaster/api/serializers.py | 20 ++++- src/openforms/contrib/kadaster/api/urls.py | 7 +- src/openforms/contrib/kadaster/api/views.py | 79 +++++++++++++++++-- .../tests/test_api_address_autocomplete.py} | 13 +-- src/openforms/locations/__init__.py | 0 src/openforms/locations/api/__init__.py | 0 src/openforms/locations/api/serializers.py | 19 ----- src/openforms/locations/api/tests/__init__.py | 0 src/openforms/locations/api/urls.py | 11 --- src/openforms/locations/api/views.py | 79 ------------------- 12 files changed, 155 insertions(+), 126 deletions(-) rename src/openforms/{locations/api/tests/test_api.py => contrib/kadaster/tests/test_api_address_autocomplete.py} (94%) delete mode 100644 src/openforms/locations/__init__.py delete mode 100644 src/openforms/locations/api/__init__.py delete mode 100644 src/openforms/locations/api/serializers.py delete mode 100644 src/openforms/locations/api/tests/__init__.py delete mode 100644 src/openforms/locations/api/urls.py delete mode 100644 src/openforms/locations/api/views.py diff --git a/src/openapi.yaml b/src/openapi.yaml index 623905d067..ec8e536fe0 100644 --- a/src/openapi.yaml +++ b/src/openapi.yaml @@ -2902,6 +2902,46 @@ paths: $ref: '#/components/headers/X-Is-Form-Designer' Content-Language: $ref: '#/components/headers/Content-Language' + /api/v2/geo/address-autocomplete: + get: + operationId: geo_address_autocomplete_retrieve + description: |- + Get the street name and city for a given postal code and house number. + + **NOTE** the `/api/v2/location/get-street-name-and-city/` endpoint will be removed in v3. Use `/api/v2/geo/address-autocomplete/` instead. + summary: Get a street name and city + parameters: + - in: query + name: house_number + schema: + type: number + description: House number of the address + required: true + - in: query + name: postcode + schema: + type: string + description: Postal code of the address + required: true + tags: + - geo + deprecated: true + responses: + '200': + content: + application/json: + schema: + $ref: '#/components/schemas/GetStreetNameAndCityViewResult' + description: '' + headers: + X-Session-Expires-In: + $ref: '#/components/headers/X-Session-Expires-In' + X-CSRFToken: + $ref: '#/components/headers/X-CSRFToken' + X-Is-Form-Designer: + $ref: '#/components/headers/X-Is-Form-Designer' + Content-Language: + $ref: '#/components/headers/Content-Language' /api/v2/geo/address-search: get: operationId: geo_address_search_list @@ -3208,11 +3248,11 @@ paths: $ref: '#/components/headers/Content-Language' /api/v2/location/get-street-name-and-city: get: - operationId: get_street_name_and_city_list + operationId: location_get_street_name_and_city_retrieve description: |- Get the street name and city for a given postal code and house number. - **NOTE** this endpoint will redirect to `/api/v3/geo/address-autocomplete/` in v3. + **NOTE** the `/api/v2/location/get-street-name-and-city/` endpoint will be removed in v3. Use `/api/v2/geo/address-autocomplete/` instead. summary: Get a street name and city parameters: - in: query diff --git a/src/openforms/api/urls.py b/src/openforms/api/urls.py index 5e5ce0b73b..56b03a9186 100644 --- a/src/openforms/api/urls.py +++ b/src/openforms/api/urls.py @@ -10,6 +10,7 @@ from rest_framework import routers from rest_framework_nested.routers import NestedSimpleRouter +from openforms.contrib.kadaster.api.views import AddressAutocompleteView from openforms.forms.api.viewsets import ( CategoryViewSet, FormDefinitionViewSet, @@ -97,7 +98,13 @@ path("forms-import", FormsImportAPIView.as_view(), name="forms-import"), path("prefill/", include("openforms.prefill.api.urls")), path("validation/", include("openforms.validations.api.urls")), - path("location/", include("openforms.locations.api.urls")), + # TODO: in Open Forms v3, this must become a simple RedirectView to the + # endpoint for openforms.contrib.kadaster.api.views.AddressAutocomplete + path( + "location/get-street-name-and-city", + AddressAutocompleteView.as_view(), + name="get-street-name-and-city-list", + ), path( "logic/description", GenerateLogicDescriptionView.as_view(), diff --git a/src/openforms/contrib/kadaster/api/serializers.py b/src/openforms/contrib/kadaster/api/serializers.py index c00bb5934b..bba3100fdc 100644 --- a/src/openforms/contrib/kadaster/api/serializers.py +++ b/src/openforms/contrib/kadaster/api/serializers.py @@ -4,6 +4,22 @@ from rest_framework import serializers +class GetStreetNameAndCityViewInputSerializer(serializers.Serializer): + postcode = serializers.CharField( + label=_("postal code"), help_text=_("Postal code to use in search") + ) + house_number = serializers.CharField( + label=_("house number"), help_text=_("House number to use in search") + ) + + +class GetStreetNameAndCityViewResultSerializer(serializers.Serializer): + street_name = serializers.CharField( + label=_("street name"), help_text=_("Found street name") + ) + city = serializers.CharField(label=_("city"), help_text=_("Found city")) + + class LatitudeLongitudeSerializer(serializers.Serializer): lat = serializers.FloatField(label=_("Latitude")) lng = serializers.FloatField(label=_("Longitude")) @@ -15,7 +31,7 @@ class RijksDriehoekSerializer(serializers.Serializer): class AddressSearchResultSerializer(serializers.Serializer): - label = serializers.CharField(label=_("Location name")) + label = serializers.CharField(label=_("Location name")) # type: ignore lat_lng = LatitudeLongitudeSerializer( label=_("Latitude/longitude"), help_text=_("Latitude and longitude in the WGS 84 coordinate system."), @@ -47,4 +63,4 @@ class LatLngSearchInputSerializer(serializers.Serializer): class LatLngSearchResultSerializer(serializers.Serializer): - label = serializers.CharField(label=_("Closest address")) + label = serializers.CharField(label=_("Closest address")) # type: ignore diff --git a/src/openforms/contrib/kadaster/api/urls.py b/src/openforms/contrib/kadaster/api/urls.py index c35463df0f..b3ccf466e2 100644 --- a/src/openforms/contrib/kadaster/api/urls.py +++ b/src/openforms/contrib/kadaster/api/urls.py @@ -1,10 +1,15 @@ from django.urls import path -from .views import AddressSearchView, LatLngSearchView +from .views import AddressAutocompleteView, AddressSearchView, LatLngSearchView app_name = "geo" urlpatterns = [ + path( + "address-autocomplete", + AddressAutocompleteView.as_view(), + name="address-autocomplete", + ), path( "address-search", AddressSearchView.as_view(), diff --git a/src/openforms/contrib/kadaster/api/views.py b/src/openforms/contrib/kadaster/api/views.py index 16aa5f0cd5..01f72dc7c2 100644 --- a/src/openforms/contrib/kadaster/api/views.py +++ b/src/openforms/contrib/kadaster/api/views.py @@ -1,5 +1,7 @@ import logging +from functools import partial +from django.core.cache import caches from django.utils.translation import gettext_lazy as _ from drf_spectacular.types import OpenApiTypes @@ -13,9 +15,11 @@ from openforms.api.views.mixins import ListMixin from openforms.submissions.api.permissions import AnyActiveSubmissionPermission -from ..clients import get_locatieserver_client +from ..clients import get_bag_client, get_locatieserver_client from .serializers import ( AddressSearchResultSerializer, + GetStreetNameAndCityViewInputSerializer, + GetStreetNameAndCityViewResultSerializer, LatLngSearchInputSerializer, LatLngSearchResultSerializer, ) @@ -23,11 +27,76 @@ logger = logging.getLogger(__name__) +ADDRESS_AUTOCOMPLETE_CACHE_TIMEOUT = ( + 60 * 60 * 24 +) # 24 hours - address data does NOT update frequently + + +def lookup_address(postcode: str, number: str): + with get_bag_client() as client: + return client.get_address(postcode, number) + + +class AddressAutocompleteView(APIView): + """ + Get the street name and city when given a postcode and house number. + """ + + authentication_classes = () + permission_classes = [AnyActiveSubmissionPermission] + + @extend_schema( + summary=_("Get a street name and city"), # type: ignore + description=_( + "Get the street name and city for a given postal code and house number.\n\n" + "**NOTE** the `/api/v2/location/get-street-name-and-city/` endpoint will " + "be removed in v3. Use `/api/v2/geo/address-autocomplete/` instead." + ), # type: ignore + responses=GetStreetNameAndCityViewResultSerializer, + parameters=[ + OpenApiParameter( + "postcode", + OpenApiTypes.STR, + OpenApiParameter.QUERY, + description=_("Postal code of the address"), # type: ignore + required=True, + ), + OpenApiParameter( + "house_number", + OpenApiTypes.NUMBER, + OpenApiParameter.QUERY, + description=_("House number of the address"), # type: ignore + required=True, + ), + ], + deprecated=True, + ) + def get(self, request, *args, **kwargs): + serializer = GetStreetNameAndCityViewInputSerializer(data=request.query_params) + serializer.is_valid(raise_exception=True) + data = serializer.validated_data + postcode, number = data["postcode"], data["house_number"] + + # check the cache so we avoid hitting the remote API too often (and risk + # of being throttled, see #1832) + address_data = caches["default"].get_or_set( + key=f"BAG|get_address|{postcode}|{number}", + default=partial(lookup_address, postcode, number), + timeout=ADDRESS_AUTOCOMPLETE_CACHE_TIMEOUT, + ) + + if not address_data: + # If address is not found just return an empty response + return Response({}) + + return Response(GetStreetNameAndCityViewResultSerializer(address_data).data) + + @extend_schema( - summary=_("Get an adress based on coordinates"), + summary=_("Get an adress based on coordinates"), # type: ignore description=_( "Get the closest address name based on the given longitude and latitude." - ), + ), # type: ignore parameters=[ OpenApiParameter( "lat", @@ -77,13 +146,13 @@ def get(self, request: Request): @extend_schema( - summary=_("List address suggestions with coordinates."), + summary=_("List address suggestions with coordinates."), # type: ignore description=_( "Get a list of addresses, ordered by relevance/match score of the input " "query. Note that only results having latitude/longitude data are returned.\n\n" "The results are retrieved from the configured geo search service, defaulting " "to the Kadaster location server." - ), + ), # type: ignore parameters=[ OpenApiParameter( "q", diff --git a/src/openforms/locations/api/tests/test_api.py b/src/openforms/contrib/kadaster/tests/test_api_address_autocomplete.py similarity index 94% rename from src/openforms/locations/api/tests/test_api.py rename to src/openforms/contrib/kadaster/tests/test_api_address_autocomplete.py index e68acab470..e78544c98b 100644 --- a/src/openforms/locations/api/tests/test_api.py +++ b/src/openforms/contrib/kadaster/tests/test_api_address_autocomplete.py @@ -8,12 +8,13 @@ import requests_mock -from openforms.contrib.kadaster.clients.bag import AddressResult -from openforms.contrib.kadaster.models import KadasterApiConfig from openforms.submissions.tests.factories import SubmissionFactory from openforms.submissions.tests.mixins import SubmissionsMixin from zgw_consumers_ext.tests.factories import ServiceFactory +from ..clients.bag import AddressResult +from ..models import KadasterApiConfig + CACHES = settings.CACHES.copy() CACHES["default"] = {"BACKEND": "django.core.cache.backends.locmem.LocMemCache"} @@ -31,7 +32,7 @@ def setUp(self): self.addCleanup(caches["default"].clear) self._add_submission_to_session(self.submission) - @patch("openforms.locations.api.views.lookup_address") + @patch("openforms.contrib.kadaster.api.views.lookup_address") def test_getting_street_name_and_city(self, m_lookup_address): m_lookup_address.return_value = AddressResult( street_name="Keizersgracht", city="Amsterdam" @@ -111,7 +112,7 @@ def test_getting_street_name_and_city_without_house_number_returns_error( }, ) - @patch("openforms.locations.api.views.lookup_address") + @patch("openforms.contrib.kadaster.api.views.lookup_address") def test_getting_street_name_and_city_with_extra_query_params_ignores_extra_param( self, m_lookup_address ): @@ -129,7 +130,7 @@ def test_getting_street_name_and_city_with_extra_query_params_ignores_extra_para self.assertEqual(response.json()["streetName"], "Keizersgracht") self.assertEqual(response.json()["city"], "Amsterdam") - @patch("openforms.locations.api.views.lookup_address") + @patch("openforms.contrib.kadaster.api.views.lookup_address") def test_address_not_found_returns_empty_200_response(self, m_lookup_address): m_lookup_address.return_value = None @@ -142,7 +143,7 @@ def test_address_not_found_returns_empty_200_response(self, m_lookup_address): self.assertEqual(response.json(), {}) @tag("gh-1832") - @patch("openforms.locations.api.views.lookup_address") + @patch("openforms.contrib.kadaster.api.views.lookup_address") def test_endpoint_uses_caching(self, m_lookup_address): m_lookup_address.return_value = AddressResult( street_name="Keizersgracht", city="Amsterdam" diff --git a/src/openforms/locations/__init__.py b/src/openforms/locations/__init__.py deleted file mode 100644 index e69de29bb2..0000000000 diff --git a/src/openforms/locations/api/__init__.py b/src/openforms/locations/api/__init__.py deleted file mode 100644 index e69de29bb2..0000000000 diff --git a/src/openforms/locations/api/serializers.py b/src/openforms/locations/api/serializers.py deleted file mode 100644 index 7a345580d3..0000000000 --- a/src/openforms/locations/api/serializers.py +++ /dev/null @@ -1,19 +0,0 @@ -from django.utils.translation import gettext_lazy as _ - -from rest_framework import serializers - - -class GetStreetNameAndCityViewInputSerializer(serializers.Serializer): - postcode = serializers.CharField( - label=_("postal code"), help_text=_("Postal code to use in search") - ) - house_number = serializers.CharField( - label=_("house number"), help_text=_("House number to use in search") - ) - - -class GetStreetNameAndCityViewResultSerializer(serializers.Serializer): - street_name = serializers.CharField( - label=_("street name"), help_text=_("Found street name") - ) - city = serializers.CharField(label=_("city"), help_text=_("Found city")) diff --git a/src/openforms/locations/api/tests/__init__.py b/src/openforms/locations/api/tests/__init__.py deleted file mode 100644 index e69de29bb2..0000000000 diff --git a/src/openforms/locations/api/urls.py b/src/openforms/locations/api/urls.py deleted file mode 100644 index 62d5c778f2..0000000000 --- a/src/openforms/locations/api/urls.py +++ /dev/null @@ -1,11 +0,0 @@ -from django.urls import path - -from .views import GetStreetNameAndCityView - -urlpatterns = [ - path( - "get-street-name-and-city", - GetStreetNameAndCityView.as_view(), - name="get-street-name-and-city-list", - ), -] diff --git a/src/openforms/locations/api/views.py b/src/openforms/locations/api/views.py deleted file mode 100644 index 932eb26151..0000000000 --- a/src/openforms/locations/api/views.py +++ /dev/null @@ -1,79 +0,0 @@ -from functools import partial - -from django.core.cache import caches -from django.utils.translation import ugettext_lazy as _ - -from drf_spectacular.types import OpenApiTypes -from drf_spectacular.utils import OpenApiParameter, extend_schema -from rest_framework.response import Response -from rest_framework.views import APIView - -from openforms.contrib.kadaster.clients import get_bag_client -from openforms.locations.api.serializers import ( - GetStreetNameAndCityViewInputSerializer, - GetStreetNameAndCityViewResultSerializer, -) -from openforms.submissions.api.permissions import AnyActiveSubmissionPermission - -CACHE_TIMEOUT = 60 * 60 * 24 # 24 hours - address data does NOT update frequently - - -def lookup_address(postcode: str, number: str): - with get_bag_client() as client: - return client.get_address(postcode, number) - - -class GetStreetNameAndCityView(APIView): - """ - Get the street name and city when given a postcode and house number - """ - - authentication_classes = () - permission_classes = [AnyActiveSubmissionPermission] - - @extend_schema( - operation_id="get_street_name_and_city_list", - summary=_("Get a street name and city"), - description=_( - "Get the street name and city for a given postal code and house number.\n\n" - "**NOTE** this endpoint will redirect to `/api/v3/geo/address-autocomplete/`" - " in v3." - ), - responses=GetStreetNameAndCityViewResultSerializer, - parameters=[ - OpenApiParameter( - "postcode", - OpenApiTypes.STR, - OpenApiParameter.QUERY, - description=_("Postal code of the address"), - required=True, - ), - OpenApiParameter( - "house_number", - OpenApiTypes.NUMBER, - OpenApiParameter.QUERY, - description=_("House number of the address"), - required=True, - ), - ], - deprecated=True, - ) - def get(self, request, *args, **kwargs): - serializer = GetStreetNameAndCityViewInputSerializer(data=request.query_params) - serializer.is_valid(raise_exception=True) - data = serializer.validated_data - postcode, number = data["postcode"], data["house_number"] - - # check the cache so we avoid hitting the remote API too often (and risk - # of being throttled, see #1832) - address_data = caches["default"].get_or_set( - key=f"BAG|get_address|{postcode}|{number}", - default=partial(lookup_address, postcode, number), - timeout=CACHE_TIMEOUT, - ) - - if not address_data: - # If address is not found just return an empty response - return Response({}) - - return Response(GetStreetNameAndCityViewResultSerializer(address_data).data) From 84f37ecb538abe1b5589f7f89bef55965cfb71f0 Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Tue, 26 Sep 2023 11:10:43 +0200 Subject: [PATCH 4/8] :art: [#3489] Tidy up pre_requests code --- src/openforms/pre_requests/base.py | 2 +- src/openforms/pre_requests/clients.py | 15 +++++++++------ 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/src/openforms/pre_requests/base.py b/src/openforms/pre_requests/base.py index 5888b72f51..47de457712 100644 --- a/src/openforms/pre_requests/base.py +++ b/src/openforms/pre_requests/base.py @@ -19,7 +19,7 @@ def __call__( self, method: str, url: str, - kwargs: dict, + kwargs: dict | None, context: PreRequestClientContext | None = None, ) -> None: raise NotImplementedError() # pragma: nocover diff --git a/src/openforms/pre_requests/clients.py b/src/openforms/pre_requests/clients.py index 27bf04e9ee..e966fb7cfa 100644 --- a/src/openforms/pre_requests/clients.py +++ b/src/openforms/pre_requests/clients.py @@ -1,4 +1,6 @@ -from typing import TYPE_CHECKING, Any, Optional, TypedDict +from __future__ import annotations + +from typing import TYPE_CHECKING, Any, TypedDict from requests.models import PreparedRequest, Request from zgw_consumers.client import ZGWClient @@ -10,7 +12,7 @@ class PreRequestClientContext(TypedDict): - submission: Optional["Submission"] + submission: Submission | None class PreRequestZGWClient(ZGWClient): @@ -18,12 +20,13 @@ class PreRequestZGWClient(ZGWClient): A :class:`zgw_consumers.client.ZGWClient` with pre-requests support. .. note:: this client is being deprecated in favour of a pure requests approach, - using :class:`api_client.APIClient` + using :class:`api_client.APIClient` and + :class:`pre_requests.clients.PreRequestMixin`. """ _context = None - def pre_request(self, method: str, url: str, kwargs: Optional[dict] = None) -> Any: + def pre_request(self, method: str, url: str, kwargs: dict | None = None) -> Any: result = super().pre_request(method, url, kwargs) for pre_request in registry: @@ -32,7 +35,7 @@ def pre_request(self, method: str, url: str, kwargs: Optional[dict] = None) -> A return result @property - def context(self) -> Optional[PreRequestClientContext]: + def context(self) -> PreRequestClientContext | None: return self._context @context.setter @@ -80,4 +83,4 @@ def prepare_request(self, request: Request) -> PreparedRequest: for key, value in kwargs.items(): setattr(request, key, value) - return super().prepare_request(request) + return super().prepare_request(request) # type: ignore From d133c37c4b6240c7d13021cd87fa4f375ea29d5b Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Tue, 26 Sep 2023 11:25:29 +0200 Subject: [PATCH 5/8] :recycle: [#3489] Refactor stuf_bg prefill to use similar interface... ... as the other client definitions/usages in other parts of the project. --- .../components/np_family_members/stuf_bg.py | 13 +-- .../tests/test_family_members.py | 4 +- .../prefill/contrib/stufbg/plugin.py | 83 +++++++++---------- src/openforms/utils/xml.py | 2 +- src/stuf/stuf_bg/client.py | 13 +++ src/stuf/stuf_bg/models.py | 8 -- src/stuf/stuf_bg/tests/test_client.py | 28 ++++--- 7 files changed, 73 insertions(+), 78 deletions(-) diff --git a/src/openforms/formio/components/np_family_members/stuf_bg.py b/src/openforms/formio/components/np_family_members/stuf_bg.py index 97b45a79ec..4d14b969d6 100644 --- a/src/openforms/formio/components/np_family_members/stuf_bg.py +++ b/src/openforms/formio/components/np_family_members/stuf_bg.py @@ -1,18 +1,11 @@ from typing import List, Tuple -from stuf.stuf_bg.models import StufBGConfig +from stuf.stuf_bg.client import get_client def get_np_children_stuf_bg(bsn: str) -> List[Tuple[str, str]]: - config = StufBGConfig.get_solo() - assert isinstance(config, StufBGConfig) - client = config.get_client() - - attributes = [ - "inp.heeftAlsKinderen", - ] - - data = client.get_values(bsn, attributes) + with get_client() as client: + data = client.get_values(bsn, ["inp.heeftAlsKinderen"]) # Kids child_choices = [] diff --git a/src/openforms/formio/components/np_family_members/tests/test_family_members.py b/src/openforms/formio/components/np_family_members/tests/test_family_members.py index 2827140358..34c84196b4 100644 --- a/src/openforms/formio/components/np_family_members/tests/test_family_members.py +++ b/src/openforms/formio/components/np_family_members/tests/test_family_members.py @@ -107,9 +107,7 @@ def test_get_children_haal_centraal(self, mock_brp_config_get_solo): self.assertEqual(("456789123", "Bolly van Doe"), kids_choices[0]) self.assertEqual(("789123456", "Billy van Doe"), kids_choices[1]) - @patch( - "openforms.formio.components.np_family_members.stuf_bg.StufBGConfig.get_solo" - ) + @patch("stuf.stuf_bg.client.StufBGConfig.get_solo") def test_get_children_stuf_bg(self, mock_stufbg_config_get_solo): stuf_bg_service = StufServiceFactory.build() mock_stufbg_config_get_solo.return_value = StufBGConfig(service=stuf_bg_service) diff --git a/src/openforms/prefill/contrib/stufbg/plugin.py b/src/openforms/prefill/contrib/stufbg/plugin.py index 9e16b96b74..705755e65c 100644 --- a/src/openforms/prefill/contrib/stufbg/plugin.py +++ b/src/openforms/prefill/contrib/stufbg/plugin.py @@ -1,17 +1,18 @@ import logging -from typing import Any, Dict, Iterable, List, Tuple +from typing import Any from django.urls import reverse from django.utils.translation import gettext_lazy as _ +import requests from glom import T as Target, glom from lxml import etree -from requests import HTTPError, RequestException from openforms.authentication.constants import AuthAttribute from openforms.plugins.exceptions import InvalidPluginConfiguration from openforms.submissions.models import Submission from openforms.utils.xml import fromstring +from stuf.stuf_bg.client import NoServiceConfigured, get_client from stuf.stuf_bg.constants import FieldChoices from stuf.stuf_bg.models import StufBGConfig @@ -95,15 +96,14 @@ class StufBgPrefill(BasePlugin): verbose_name = _("StUF-BG") requires_auth = AuthAttribute.bsn - def get_available_attributes(self) -> Iterable[Tuple[str, str]]: + def get_available_attributes(self) -> list[tuple[str, str]]: return FieldChoices.choices def _get_values_for_bsn( - self, bsn: str, attributes: Iterable[str] - ) -> Dict[str, Any]: - config = StufBGConfig.get_solo() - with config.get_client() as client: - data = client.get_values(bsn, attributes) + self, bsn: str, attributes: list[FieldChoices] + ) -> dict[str, Any]: + with get_client() as client: + data = client.get_values(bsn, [str(attr) for attr in attributes]) response_dict = {} for attribute in attributes: @@ -146,9 +146,9 @@ def get_identifier_value( def get_prefill_values( self, submission: Submission, - attributes: List[str], + attributes: list[FieldChoices], identifier_role: str = IdentifierRoles.main, - ) -> Dict[str, Any]: + ) -> dict[str, Any]: if not (bsn_value := self.get_identifier_value(submission, identifier_role)): # If there is no bsn we can't prefill any values so just return logger.info("No BSN associated with submission, cannot prefill.") @@ -156,7 +156,7 @@ def get_prefill_values( return self._get_values_for_bsn(bsn_value, attributes) - def get_co_sign_values(self, identifier: str) -> Tuple[Dict[str, Any], str]: + def get_co_sign_values(self, identifier: str) -> tuple[dict[str, Any], str]: """ Given an identifier, fetch the co-sign specific values. @@ -170,11 +170,11 @@ def get_co_sign_values(self, identifier: str) -> Tuple[Dict[str, Any], str]: """ values = self._get_values_for_bsn( identifier, - ( + [ FieldChoices.voornamen, FieldChoices.voorvoegselGeslachtsnaam, FieldChoices.geslachtsnaam, - ), + ], ) representation_bits = [ " ".join( @@ -190,41 +190,36 @@ def get_co_sign_values(self, identifier: str) -> Tuple[Dict[str, Any], str]: def check_config(self): check_bsn = "111222333" + try: + with get_client() as client: + response = client.templated_request( + "npsLv01", + template="stuf_bg/StufBgRequest.xml", + context={"bsn": check_bsn}, + ) + response.raise_for_status() + except NoServiceConfigured as exc: + raise InvalidPluginConfiguration(_("Service not selected")) from exc + except requests.RequestException as exc: + raise InvalidPluginConfiguration( + _("Client error: {exception}").format(exception=exc) + ) from exc - config = StufBGConfig.get_solo() - if not config.service: - raise InvalidPluginConfiguration(_("Service not selected")) - - client = config.get_client() try: - response = client.templated_request( - "npsLv01", - template="stuf_bg/StufBgRequest.xml", - context={"bsn": check_bsn}, - ) - response.raise_for_status() - except (RequestException, HTTPError) as e: + xml = fromstring(response.content) + except etree.XMLSyntaxError as exc: raise InvalidPluginConfiguration( - _("Client error: {exception}").format(exception=e) - ) - else: - try: - xml = fromstring(response.content) - except etree.XMLSyntaxError as e: - raise InvalidPluginConfiguration( - _("SyntaxError in response: {exception}").format(exception=e) + _("SyntaxError in response: {exception}").format(exception=exc) + ) from exc + + # we expect a valid 'object not found' response, + # but also accept an empty response (for 3rd party backend implementation reasons) + if not is_object_not_found_response(xml) and not is_empty_wrapped_response(xml): + raise InvalidPluginConfiguration( + _("Unexpected response: expected '{message}' SOAP response").format( + message="Object niet gevonden" ) - else: - # we expect a valid 'object not found' response, - # but also accept an empty response (for 3rd party backend implementation reasons) - if not is_object_not_found_response( - xml - ) and not is_empty_wrapped_response(xml): - raise InvalidPluginConfiguration( - _( - "Unexpected response: expected '{message}' SOAP response" - ).format(message="Object niet gevonden") - ) + ) def get_config_actions(self): return [ diff --git a/src/openforms/utils/xml.py b/src/openforms/utils/xml.py index fc9b944492..184d0b49c3 100644 --- a/src/openforms/utils/xml.py +++ b/src/openforms/utils/xml.py @@ -7,7 +7,7 @@ from lxml.etree import XMLParser, fromstring as _fromstring -def fromstring(content: str): +def fromstring(content: str | bytes): """ Create an LXML etree from the string content without resolving entities. diff --git a/src/stuf/stuf_bg/client.py b/src/stuf/stuf_bg/client.py index 6859c32aaa..23de2eeadd 100644 --- a/src/stuf/stuf_bg/client.py +++ b/src/stuf/stuf_bg/client.py @@ -12,10 +12,23 @@ from ..models import StufService from ..service_client_factory import ServiceClientFactory, get_client_init_kwargs from .constants import NAMESPACE_REPLACEMENTS, STUF_BG_EXPIRY_MINUTES +from .models import StufBGConfig logger = logging.getLogger(__name__) +class NoServiceConfigured(RuntimeError): + pass + + +def get_client() -> "Client": + config = StufBGConfig.get_solo() + assert isinstance(config, StufBGConfig) + if not (service := config.service): + raise NoServiceConfigured("You must configure a service!") + return StufBGClient(service) + + def StufBGClient(service: StufService) -> "Client": """ Client instance factory, given a service configured in the database. diff --git a/src/stuf/stuf_bg/models.py b/src/stuf/stuf_bg/models.py index 1faf1fd379..a2aaf785f4 100644 --- a/src/stuf/stuf_bg/models.py +++ b/src/stuf/stuf_bg/models.py @@ -20,13 +20,5 @@ class StufBGConfig(SingletonModel): objects = ConfigManager() - def get_client(self): - from .client import StufBGClient - - if not self.service: - raise RuntimeError("You must configure a service!") - - return StufBGClient(self.service) - class Meta: verbose_name = _("StUF-BG configuration") diff --git a/src/stuf/stuf_bg/tests/test_client.py b/src/stuf/stuf_bg/tests/test_client.py index 89b8af7f18..592b7389a6 100644 --- a/src/stuf/stuf_bg/tests/test_client.py +++ b/src/stuf/stuf_bg/tests/test_client.py @@ -1,10 +1,11 @@ from functools import lru_cache from pathlib import Path from random import shuffle +from unittest.mock import patch from django.conf import settings from django.template import loader -from django.test import TestCase, tag +from django.test import SimpleTestCase, TestCase, tag from django.utils import timezone import requests_mock @@ -16,13 +17,14 @@ from openforms.prefill.contrib.stufbg.plugin import ATTRIBUTES_TO_STUF_BG_MAPPING from soap.constants import SOAP_VERSION_CONTENT_TYPES, SOAPVersion from stuf.models import StufService -from stuf.stuf_bg.client import StufBGClient -from stuf.stuf_bg.constants import NAMESPACE_REPLACEMENTS, FieldChoices -from stuf.stuf_bg.models import StufBGConfig from stuf.stuf_zds.client import nsmap from stuf.tests.factories import StufServiceFactory from stuf.xml import fromstring +from ..client import NoServiceConfigured, StufBGClient, get_client +from ..constants import NAMESPACE_REPLACEMENTS, FieldChoices +from ..models import StufBGConfig + PATH_XSDS = (Path(settings.BASE_DIR) / "src" / "stuf" / "stuf_bg" / "xsd").resolve() STUF_BG_XSD = PATH_XSDS / "bg0310" / "vraagAntwoord" / "bg0310_namespace.xsd" @@ -70,21 +72,23 @@ def _stuf_bg_response(service: StufService): return response_body -class StufBGConfigTests(TestCase): - def test_client_requires_a_service(self): - config = StufBGConfig(service=None) - - with self.assertRaises(RuntimeError): - config.get_client() +class StufBGConfigTests(SimpleTestCase): + @patch( + "stuf.stuf_bg.client.StufBGConfig.get_solo", + return_value=StufBGConfig(service=None), + ) + def test_client_requires_a_service(self, m_get_solo): + with self.assertRaises(NoServiceConfigured): + get_client() def test_all_prefill_attributes_are_mapped(self): - available_attributes = FieldChoices.values - for attribute in available_attributes: + for attribute in FieldChoices: with self.subTest(attribute=attribute): glom_target = ATTRIBUTES_TO_STUF_BG_MAPPING.get(attribute) self.assert_(glom_target, f"unmapped attribute: {attribute}") +# not SimpleTestCase because of openforms.logging.logevent usage class StufBGClientTests(TestCase): def setUp(self): super().setUp() From 07ad16f4c278143372d93e191ad8a2ed570ff84d Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Tue, 26 Sep 2023 11:44:22 +0200 Subject: [PATCH 6/8] :recycle: [#3489] Refactor stuf_zds registration to use similar interface... ... as the other client definitions/usages in other parts of the project. --- .../registrations/contrib/stuf_zds/plugin.py | 41 +++++++++---------- src/stuf/stuf_zds/client.py | 14 +++++++ src/stuf/stuf_zds/models.py | 5 --- 3 files changed, 33 insertions(+), 27 deletions(-) diff --git a/src/openforms/registrations/contrib/stuf_zds/plugin.py b/src/openforms/registrations/contrib/stuf_zds/plugin.py index bb6fca89e4..b7a731c9f5 100644 --- a/src/openforms/registrations/contrib/stuf_zds/plugin.py +++ b/src/openforms/registrations/contrib/stuf_zds/plugin.py @@ -23,6 +23,7 @@ ) from openforms.submissions.models import Submission, SubmissionReport from openforms.utils.mixins import JsonSchemaSerializerMixin +from stuf.stuf_zds.client import NoServiceConfigured, ZaakOptions, get_client from stuf.stuf_zds.constants import VertrouwelijkheidsAanduidingen from stuf.stuf_zds.models import StufZDSConfig @@ -195,11 +196,10 @@ class StufZDSRegistration(BasePlugin): ), } - def pre_register_submission(self, submission: "Submission", options: dict) -> None: - config = StufZDSConfig.get_solo() - config.apply_defaults_to(options) - - with config.get_client(options) as client: + def pre_register_submission( + self, submission: "Submission", options: ZaakOptions + ) -> None: + with get_client(options=options) as client: # obtain a zaaknummer & save it - first, check if we have an intermediate result # from earlier attempts. if we do, do not generate a new number zaak_id = execute_unless_result_exists( @@ -213,8 +213,8 @@ def pre_register_submission(self, submission: "Submission", options: dict) -> No submission.save() def register_submission( - self, submission: Submission, options: dict - ) -> Optional[dict]: + self, submission: Submission, options: ZaakOptions + ) -> dict | None: """ Register the submission by creating a ZAAK. @@ -225,12 +225,8 @@ def register_submission( prevents Open Forms from reserving case numbers over and over again (for example). See #1183 for a reported issue about this. """ - config = StufZDSConfig.get_solo() - config.apply_defaults_to(options) - options["omschrijving"] = submission.form.admin_name - - with config.get_client(options) as client: + with get_client(options=options) as client: # Zaak ID reserved during the pre-registration phase zaak_id = submission.public_registration_reference @@ -339,10 +335,8 @@ def get_reference_from_result(self, result: Dict[str, str]) -> str: """ return result["zaak"] - def update_payment_status(self, submission: "Submission", options: dict): - config = StufZDSConfig.get_solo() - config.apply_defaults_to(options) - with config.get_client(options) as client: + def update_payment_status(self, submission: "Submission", options: ZaakOptions): + with get_client(options) as client: client.set_zaak_payment( submission.registration_result["zaak"], ) @@ -350,23 +344,26 @@ def update_payment_status(self, submission: "Submission", options: dict): def check_config(self): config = StufZDSConfig.get_solo() assert isinstance(config, StufZDSConfig) - if not config.service_id: - raise InvalidPluginConfiguration(_("StufService not selected")) + if not config.gemeentecode: raise InvalidPluginConfiguration( _("StufService missing setting '{name}'").format(name="gemeentecode") ) - options = { + options: ZaakOptions = { "omschrijving": "MyForm", "zds_zaaktype_code": "test", "zds_zaaktype_omschrijving": "test", "zds_zaaktype_status_code": "test", "zds_zaaktype_status_omschrijving": "test", "zds_documenttype_omschrijving_inzending": "test", - } - config.apply_defaults_to(options) - with config.get_client(options) as client: + } # type: ignore + try: + client = get_client(options) + except NoServiceConfigured: + raise InvalidPluginConfiguration(_("StufService not selected")) + + with client: try: client.check_config() except Exception as e: diff --git a/src/stuf/stuf_zds/client.py b/src/stuf/stuf_zds/client.py index f7ef063ac8..0abb4f16bb 100644 --- a/src/stuf/stuf_zds/client.py +++ b/src/stuf/stuf_zds/client.py @@ -25,6 +25,7 @@ from ..service_client_factory import ServiceClientFactory, get_client_init_kwargs from ..xml import fromstring from .constants import STUF_ZDS_EXPIRY_MINUTES +from .models import StufZDSConfig logger = logging.getLogger(__name__) @@ -96,6 +97,19 @@ class ZaakOptions(TypedDict): referentienummer: str +class NoServiceConfigured(RuntimeError): + pass + + +def get_client(options: ZaakOptions) -> "Client": + config = StufZDSConfig.get_solo() + assert isinstance(config, StufZDSConfig) + config.apply_defaults_to(options) + if not (service := config.service): + raise NoServiceConfigured("You must configure a service!") + return StufZDSClient(service, options) + + def StufZDSClient(service: StufService, options: ZaakOptions) -> "Client": factory = ServiceClientFactory(service) init_kwargs = get_client_init_kwargs( diff --git a/src/stuf/stuf_zds/models.py b/src/stuf/stuf_zds/models.py index 86230b2efe..eb9f6c0825 100644 --- a/src/stuf/stuf_zds/models.py +++ b/src/stuf/stuf_zds/models.py @@ -36,10 +36,5 @@ class StufZDSConfig(SingletonModel): def apply_defaults_to(self, options): options.setdefault("gemeentecode", self.gemeentecode) - def get_client(self, options): - from .client import StufZDSClient - - return StufZDSClient(self.service, options) - class Meta: verbose_name = _("StUF-ZDS configuration") From 355b4a099515c601ede0ae1f9d9d4f2073b50c9d Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Wed, 27 Sep 2023 09:14:51 +0200 Subject: [PATCH 7/8] :white_check_mark: [#3489] Address missing coverage --- .../contrib/stufbg/tests/test_plugin.py | 28 ++++++++++- .../registrations/contrib/stuf_zds/plugin.py | 4 +- .../stuf_zds/tests/test_config_check.py | 50 +++++++++++++++++++ 3 files changed, 80 insertions(+), 2 deletions(-) create mode 100644 src/openforms/registrations/contrib/stuf_zds/tests/test_config_check.py diff --git a/src/openforms/prefill/contrib/stufbg/tests/test_plugin.py b/src/openforms/prefill/contrib/stufbg/tests/test_plugin.py index 889cbcaee3..4ccc1fdc26 100644 --- a/src/openforms/prefill/contrib/stufbg/tests/test_plugin.py +++ b/src/openforms/prefill/contrib/stufbg/tests/test_plugin.py @@ -53,6 +53,18 @@ def setUp(self) -> None: stufbg_config_patcher.start() self.addCleanup(stufbg_config_patcher.stop) + def test_getting_available_attributes(self): + attributes = self.plugin.get_available_attributes() + self.assertIsInstance(attributes, list) + + for entry in attributes: + with self.subTest(entry=entry): + self.assertIsInstance(entry, tuple) + self.assertEqual(len(entry), 2) + value, label = entry + self.assertEqual(value, str(value)) + self.assertEqual(label, str(label)) + def test_get_available_attributes_returns_correct_attributes(self): client_patcher = mock_stufbg_client("StufBgResponse.xml") self.addCleanup(client_patcher.stop) @@ -241,13 +253,20 @@ def setUp(self) -> None: self.plugin = StufBgPrefill("test-plugin") # mock out django-solo interface (we don't have to deal with caches then) + self.config = StufBGConfig(service=self.stuf_bg_service) stufbg_config_patcher = patch( "openforms.prefill.contrib.stufbg.plugin.StufBGConfig.get_solo", - return_value=StufBGConfig(service=self.stuf_bg_service), + return_value=self.config, ) stufbg_config_patcher.start() self.addCleanup(stufbg_config_patcher.stop) + def test_no_service_configured(self): + self.config.service = None + + with self.assertRaises(InvalidPluginConfiguration): + self.plugin.check_config() + def test_check_config_exception(self): with patch( "stuf.stuf_bg.client.Client.get_values_for_attributes", @@ -256,6 +275,13 @@ def test_check_config_exception(self): with self.assertRaises(InvalidPluginConfiguration): self.plugin.check_config() + @requests_mock.Mocker() + def test_check_config_invalid_xml_returned(self, m): + m.register_uri(requests_mock.ANY, requests_mock.ANY, json={"not": "xml"}) + + with self.assertRaises(InvalidPluginConfiguration): + self.plugin.check_config() + def test_check_config_ok_not_found(self): try: with mock_stufbg_make_request("StufBgNotFoundResponse.xml"): diff --git a/src/openforms/registrations/contrib/stuf_zds/plugin.py b/src/openforms/registrations/contrib/stuf_zds/plugin.py index b7a731c9f5..ef41bb9b1c 100644 --- a/src/openforms/registrations/contrib/stuf_zds/plugin.py +++ b/src/openforms/registrations/contrib/stuf_zds/plugin.py @@ -33,6 +33,8 @@ logger = logging.getLogger(__name__) +PLUGIN_IDENTIFIER = "stuf-zds-create-zaak" + class ZaakOptionsSerializer(JsonSchemaSerializerMixin, serializers.Serializer): gemeentecode = serializers.CharField( @@ -155,7 +157,7 @@ def _gender_choices(value): return value -@register("stuf-zds-create-zaak") +@register(PLUGIN_IDENTIFIER) class StufZDSRegistration(BasePlugin): verbose_name = _("StUF-ZDS") configuration_options = ZaakOptionsSerializer diff --git a/src/openforms/registrations/contrib/stuf_zds/tests/test_config_check.py b/src/openforms/registrations/contrib/stuf_zds/tests/test_config_check.py new file mode 100644 index 0000000000..777f170437 --- /dev/null +++ b/src/openforms/registrations/contrib/stuf_zds/tests/test_config_check.py @@ -0,0 +1,50 @@ +from unittest.mock import patch + +from django.test import SimpleTestCase + +import requests +import requests_mock + +from openforms.plugins.exceptions import InvalidPluginConfiguration +from stuf.stuf_zds.models import StufZDSConfig +from stuf.tests.factories import StufServiceFactory + +from ..plugin import PLUGIN_IDENTIFIER, StufZDSRegistration + + +class ConfigCheckTests(SimpleTestCase): + def setUp(self): + super().setUp() + + self.config = StufZDSConfig( + service=StufServiceFactory.build(), + gemeentecode="foo", + ) + patcher = patch( + "stuf.stuf_zds.client.StufZDSConfig.get_solo", + return_value=self.config, + ) + patcher.start() + self.addCleanup(patcher.stop) + + def test_no_service_configured(self): + self.config.service = None + plugin = StufZDSRegistration(PLUGIN_IDENTIFIER) + + with self.assertRaises(InvalidPluginConfiguration): + plugin.check_config() + + def test_missing_gemeentecode(self): + self.config.gemeentecode = "" + plugin = StufZDSRegistration(PLUGIN_IDENTIFIER) + + with self.assertRaises(InvalidPluginConfiguration): + plugin.check_config() + + @requests_mock.Mocker() + def test_failing_request(self, m): + m.post(requests_mock.ANY, exc=requests.ConnectionError("nope")) + plugin = StufZDSRegistration(PLUGIN_IDENTIFIER) + + with self.assertRaises(InvalidPluginConfiguration): + plugin.check_config() From 792b7de971a4844a4738365184c7de2aa870a6e6 Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Wed, 27 Sep 2023 16:59:21 +0200 Subject: [PATCH 8/8] :ok_hand: [#3489] PR feedback --- src/openforms/contrib/kadaster/api/views.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/openforms/contrib/kadaster/api/views.py b/src/openforms/contrib/kadaster/api/views.py index 01f72dc7c2..06c0e9a6f5 100644 --- a/src/openforms/contrib/kadaster/api/views.py +++ b/src/openforms/contrib/kadaster/api/views.py @@ -1,7 +1,7 @@ import logging from functools import partial -from django.core.cache import caches +from django.core.cache import cache from django.utils.translation import gettext_lazy as _ from drf_spectacular.types import OpenApiTypes @@ -13,6 +13,7 @@ from openforms.api.serializers import ExceptionSerializer, ValidationErrorSerializer from openforms.api.views.mixins import ListMixin +from openforms.contrib.kadaster.clients.bag import AddressResult from openforms.submissions.api.permissions import AnyActiveSubmissionPermission from ..clients import get_bag_client, get_locatieserver_client @@ -32,7 +33,7 @@ ) # 24 hours - address data does NOT update frequently -def lookup_address(postcode: str, number: str): +def lookup_address(postcode: str, number: str) -> AddressResult | None: with get_bag_client() as client: return client.get_address(postcode, number) @@ -79,7 +80,7 @@ def get(self, request, *args, **kwargs): # check the cache so we avoid hitting the remote API too often (and risk # of being throttled, see #1832) - address_data = caches["default"].get_or_set( + address_data = cache.get_or_set( key=f"BAG|get_address|{postcode}|{number}", default=partial(lookup_address, postcode, number), timeout=ADDRESS_AUTOCOMPLETE_CACHE_TIMEOUT,