✨ [#500] Implement Referentielijsten integration for Klantcontact #530
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #530 +/- ##
==========================================
- Coverage 97.66% 97.55% -0.11%
==========================================
Files 214 221 +7
Lines 12438 12655 +217
==========================================
+ Hits 12147 12346 +199
- Misses 291 309 +18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
OlhaZahoruiko
force-pushed
the
feature/500-Using-the-Referentielijsten-API-to-validate-the-kanalen
branch
from
afe0c2a to
4cedbda
Compare
OlhaZahoruiko
force-pushed
the
feature/500-Using-the-Referentielijsten-API-to-validate-the-kanalen
branch
2 times, most recently
from
363609e to
a132559
Compare
stevenbal
left a comment
stevenbal
left a comment
There was a problem hiding this comment.
@OlhaZahoruiko a general remark: make sure to also test this locally with runserver and the Referentielijsten docker-compose instance. Currently it seems like it's not possible to save the config admin, so I cannot test the implementation locally either
|
|
||
| @my_vcr.use_cassette("get_items_by_tabel_code.yaml") | ||
| def test_get_items_by_tabel_code(self): | ||
| items = self._get_items_from_fixture("KANAAL") |
There was a problem hiding this comment.
These tests don't actually seem to make use of the ReferentielijstenClient?
I think you should build a client for the configured service and then test the methods that client provides
| @@ -0,0 +1,148 @@ | |||
| import json | |||
There was a problem hiding this comment.
Let's move this file to src/openklant/components/klantinteracties/api/test_klantcontact_referentielijsten_kanalen.py
There was a problem hiding this comment.
to tests/ or just api/ src/openklant/components/klantinteracties/api/tests?
OlhaZahoruiko
force-pushed
the
feature/500-Using-the-Referentielijsten-API-to-validate-the-kanalen
branch
from
f53da53 to
6232f86
Compare
stevenbal
left a comment
stevenbal
left a comment
There was a problem hiding this comment.
@OlhaZahoruiko there are two remaining checkbox items in the original issue (about documentation and showing the response code in the config model admin), but since this PR is quite big already let's put those in a separate PR when this one is merged
| @@ -0,0 +1,21 @@ | |||
| # Generated by Django 5.2.7 on 2025-10-30 10:32 | |||
There was a problem hiding this comment.
Can you make sure these migrations are collapsed into one (only 0001?). You can do this locally by running src/manage.py migrate config zero, deleting all three files and running makemigrations again.
That way we keep the number of migrations small
| notification_delivery_retry_backoff: 2 | ||
| notification_delivery_retry_backoff_max: 3 | ||
|
|
||
| referentielijsten_config_enable: true |
There was a problem hiding this comment.
You'll have to add ReferentielijstenConfigurationStep to the SETUP_CONFIGURATION_STEPS in base.py to make sure it is run when calling setup_configuration
docker/setup_configuration/data.yaml
Outdated
| referentielijsten_config: | ||
| enabled: true | ||
| service_identifier: referentielijsten-api | ||
| tabel_code: KANAAL |
There was a problem hiding this comment.
| tabel_code: KANAAL | |
| kanalen_tabel_code: KANAAL |
|
|
||
| networks: | ||
| open-klant-dev: | ||
| name: referentielijsten-dev |
There was a problem hiding this comment.
The name of the network has to be the same as the network in docker-compose.yml to make sure the containers can reach eachother over HTTP
| name: referentielijsten-dev | |
| name: open-klant-dev |
docker/setup_configuration/data.yaml
Outdated
| api_type: brc | ||
| auth_type: zgw | ||
| client_id: open-klant-referentielijsten | ||
| user_id: open-klant-referentielijsten | ||
| secret: open-klant-referentielijsten-secret |
There was a problem hiding this comment.
Referentielijsten doesn't have any API authentication/authorization, so this can be left empty. The API type is technically "orc" ("overige"), but it doesn't really matter in practice afaik
| api_type: brc | |
| auth_type: zgw | |
| client_id: open-klant-referentielijsten | |
| user_id: open-klant-referentielijsten | |
| secret: open-klant-referentielijsten-secret | |
| api_type: orc | |
| auth_type: no_auth |
| self.assertIn("email", codes) | ||
| self.assertIn("phone", codes) |
There was a problem hiding this comment.
| self.assertIn("email", codes) | |
| self.assertIn("phone", codes) | |
| self.assertEqual(codes, ["email", "phone"]) |
| def _get_items_from_fixture(self, tabel_code: str): | ||
| return self.client.get_cached_items_by_tabel_code(tabel_code) |
There was a problem hiding this comment.
Instead of defining this as a custom method, let's just call self.client.get_cached_items_by_tabel_code directly where this is used instead
| def _get_items_from_fixture(self, tabel_code: str): | |
| return self.client.get_cached_items_by_tabel_code(tabel_code) |
| first = self._get_items_from_fixture("KANAAL") | ||
| second = self._get_items_from_fixture("KANAAL") |
There was a problem hiding this comment.
It would be nice if this test also verifies that only one network call happens (because it's cached by the underlying method). I don't fully remember how to check this, but I think maybe with something like self.vcr._current_cassette.requests you could check this
| @@ -0,0 +1,59 @@ | |||
| from pathlib import Path | |||
There was a problem hiding this comment.
Could you move this file to src/openklant/components/klantinteracties/api/tests/test_kanaal_referentielijsten.py?
| def test_validation_succeeds_for_valid_kanaal(self): | ||
| validator = KanaalValidator() | ||
| try: | ||
| validator("email") | ||
| except ValidationError: | ||
| self.fail("KanaalValidator raised ValidationError unexpectedly for 'email'") | ||
|
|
||
| def test_validation_fails_for_invalid_kanaal(self): | ||
| validator = KanaalValidator() | ||
|
|
||
| with self.assertRaises(ValidationError) as cm: | ||
| validator("fax") | ||
|
|
||
| self.assertIn("'fax' is not a valid kanaal", str(cm.exception)) | ||
| self.assertIn("email", str(cm.exception)) | ||
| self.assertIn("phone", str(cm.exception)) |
There was a problem hiding this comment.
Instead of testing this by calling the validator directly, could you test this by making API calls (POST/PUT/PATCH) to /klantcontacten?
I think it would also be good to have a test for the behavior of the API if:
- referentielijsten returns a 400 or 500 error
- a requestexception or timeout occurs (you could use https://maykin-django-common.readthedocs.io/en/latest/reference/vcr.html#maykin_common.vcr.VCRMixin.vcr_raises for this)
OlhaZahoruiko
force-pushed
the
feature/500-Using-the-Referentielijsten-API-to-validate-the-kanalen
branch
2 times, most recently
from
744fe77 to
848fa94
Compare
OlhaZahoruiko
force-pushed
the
feature/500-Using-the-Referentielijsten-API-to-validate-the-kanalen
branch
from
b13979a to
9d7226c
Compare
Fixes #500
Changes
Add ReferentielijstenConfig singletonmodel
enabledfield (default false)servicefieldtabel_codefield (CharField to indicate which tabel contains the Kanalen)tabel__code(similar tozgw_consumers.Servicestatus check)Klantcontact.kanaalin the database are present in the linked tabelCreate a ReferentielijstenClient in
src/referentielijsten/client.pythat subclasseszgw_consumers.NLXClientget_items_by_tabel_codethat does a GET on/items?tabel__code={code}Add a validator to Klantcontact
ReferentielijstenConfig.enabled == FalseReferentielijstenClientbased onReferentielijstenConfig.serviceget_items_by_tabel_codeand raise aValidationErrorif the suppliedKanaalis not found (validation error should list all valid kanalen)Add a configuration step to load this config via
setup_configurationAdd a page in the docs under Manual explaining this optional integration