From 28702dd951de1ab57522c8367f9db1b3b3679323 Mon Sep 17 00:00:00 2001 From: vasileios Date: Wed, 18 Dec 2024 14:31:43 +0100 Subject: [PATCH] [#4821] Fixed email digest for BRK and addressNL component We want the email digest to report problems (broken configuration) for the addressNL component only when a valid BRK service and the validator are defined/configured. --- src/openforms/contrib/brk/service.py | 22 ++- src/openforms/contrib/brk/tests/base.py | 7 +- src/openforms/contrib/brk/validators.py | 2 + .../emails/tests/test_digest_functions.py | 160 +++++++++++++++--- .../emails/tests/test_tasks_integration.py | 17 +- src/openforms/forms/models/form.py | 5 - 6 files changed, 179 insertions(+), 34 deletions(-) diff --git a/src/openforms/contrib/brk/service.py b/src/openforms/contrib/brk/service.py index 2d1d49056f..5639e56573 100644 --- a/src/openforms/contrib/brk/service.py +++ b/src/openforms/contrib/brk/service.py @@ -1,16 +1,28 @@ +from glom import glom + from openforms.forms.models.form import Form from openforms.plugins.exceptions import InvalidPluginConfiguration from .checks import BRKValidatorCheck +from .models import BRKConfig +from .validators import BRK_ZAKELIJK_GERECHTIGD_VALIDATOR_ID def check_brk_config_for_addressNL() -> str: live_forms = Form.objects.live() - if any(form.has_component("addressNL") for form in live_forms): - try: - BRKValidatorCheck.check_config() - except InvalidPluginConfiguration as e: - return e.args[0] + for form in live_forms: + components = form.iter_components() + for component in components: + if ( + component["type"] == "addressNL" + and (plugins := glom(component, "validate.plugins", default=[])) + and BRK_ZAKELIJK_GERECHTIGD_VALIDATOR_ID in plugins + and BRKConfig.get_solo().service + ): + try: + BRKValidatorCheck.check_config() + except InvalidPluginConfiguration as e: + return e.args[0] return "" diff --git a/src/openforms/contrib/brk/tests/base.py b/src/openforms/contrib/brk/tests/base.py index 51a37b25b3..08f5987c15 100644 --- a/src/openforms/contrib/brk/tests/base.py +++ b/src/openforms/contrib/brk/tests/base.py @@ -12,13 +12,18 @@ BRK_SERVICE = ServiceFactory.build( api_root="https://api.brk.kadaster.nl/esd-eto-apikey/bevragen/v2/", - oas="https://api.brk.kadaster.nl/esd-eto-apikey/bevragen/v2/", # ignored/unused api_type=APITypes.orc, auth_type=AuthTypes.api_key, header_key="X-Api-Key", header_value=BRK_API_KEY, ) +INVALID_BRK_SERVICE = ServiceFactory.build( + api_root="https://api.brk.kadaster.nl/invalid", + api_type=APITypes.orc, + auth_type=AuthTypes.api_key, +) + class BRKTestMixin: api_root = BRK_SERVICE.api_root diff --git a/src/openforms/contrib/brk/validators.py b/src/openforms/contrib/brk/validators.py index 00746e822a..ff0a46159c 100644 --- a/src/openforms/contrib/brk/validators.py +++ b/src/openforms/contrib/brk/validators.py @@ -21,6 +21,8 @@ logger = logging.getLogger(__name__) +BRK_ZAKELIJK_GERECHTIGD_VALIDATOR_ID = "brk-zakelijk-gerechtigd" + @contextmanager def suppress_api_errors(error_message: str) -> Iterator[None]: diff --git a/src/openforms/emails/tests/test_digest_functions.py b/src/openforms/emails/tests/test_digest_functions.py index 7d98b077f6..58074c669e 100644 --- a/src/openforms/emails/tests/test_digest_functions.py +++ b/src/openforms/emails/tests/test_digest_functions.py @@ -14,7 +14,7 @@ from zgw_consumers.test.factories import ServiceFactory from openforms.contrib.brk.models import BRKConfig -from openforms.contrib.brk.tests.base import BRK_SERVICE +from openforms.contrib.brk.tests.base import BRK_SERVICE, INVALID_BRK_SERVICE from openforms.contrib.kadaster.models import KadasterApiConfig from openforms.forms.tests.factories import ( FormFactory, @@ -244,12 +244,119 @@ def test_timestamp_constraint_returns_no_results(self): @override_settings(LANGUAGE_CODE="en") class BrokenConfigurationTests(TestCase): + def test_no_addressNL_component_not_collected(self): + FormFactory.create( + generate_minimal_setup=True, + formstep__form_definition__configuration={ + "components": [ + { + "key": "textField", + "type": "textfield", + "label": "Text Field", + } + ], + }, + ) + + broken_configuration = collect_broken_configurations() + + self.assertEqual(broken_configuration, []) + + @patch( + "openforms.contrib.brk.client.BRKConfig.get_solo", + return_value=BRKConfig(service=None), + ) + def test_no_brk_conf_for_addressnl_and_missing_validator_not_collected(self, m): + FormFactory.create( + generate_minimal_setup=True, + formstep__form_definition__configuration={ + "components": [ + { + "key": "addressNl", + "type": "addressNL", + "label": "AddressNL", + "defaultValue": { + "postcode": "", + "houseLetter": "", + "houseNumber": "", + "houseNumberAddition": "", + }, + } + ], + }, + ) + + broken_configuration = collect_broken_configurations() + + self.assertEqual(broken_configuration, []) + + @patch( + "openforms.contrib.brk.client.BRKConfig.get_solo", + return_value=BRKConfig(service=None), + ) + def test_no_brk_conf_for_addressnl_and_existing_validator_not_collected(self, m): + FormFactory.create( + generate_minimal_setup=True, + formstep__form_definition__configuration={ + "components": [ + { + "key": "addressNl", + "type": "addressNL", + "label": "AddressNL", + "defaultValue": { + "postcode": "", + "houseLetter": "", + "houseNumber": "", + "houseNumberAddition": "", + }, + "validate": {"plugins": ["brk-zakelijk-gerechtigd"]}, + } + ], + }, + ) + + broken_configuration = collect_broken_configurations() + + self.assertEqual(broken_configuration, []) + + @patch( + "openforms.contrib.brk.client.BRKConfig.get_solo", + return_value=BRKConfig(service=BRK_SERVICE), + ) + def test_valid_brk_conf_for_addressnl_and_missing_validator_not_collected( + self, brk_config + ): + FormFactory.create( + generate_minimal_setup=True, + formstep__form_definition__configuration={ + "components": [ + { + "key": "addressNl", + "type": "addressNL", + "label": "AddressNL", + "defaultValue": { + "postcode": "", + "houseLetter": "", + "houseNumber": "", + "houseNumberAddition": "", + }, + } + ], + }, + ) + + broken_configuration = collect_broken_configurations() + + self.assertEqual(broken_configuration, []) + @patch( "openforms.contrib.brk.client.BRKConfig.get_solo", return_value=BRKConfig(service=BRK_SERVICE), ) @requests_mock.Mocker() - def test_valid_brk_congiguration_for_addressNL_not_collected(self, brk_config, m): + def test_valid_brk_conf_for_addressnl_and_existing_validator_not_collected( + self, brk_config, m + ): FormFactory.create( generate_minimal_setup=True, formstep__form_definition__configuration={ @@ -264,6 +371,7 @@ def test_valid_brk_congiguration_for_addressNL_not_collected(self, brk_config, m "houseNumber": "", "houseNumberAddition": "", }, + "validate": {"plugins": ["brk-zakelijk-gerechtigd"]}, } ], }, @@ -280,9 +388,11 @@ def test_valid_brk_congiguration_for_addressNL_not_collected(self, brk_config, m @patch( "openforms.contrib.brk.client.BRKConfig.get_solo", - return_value=BRKConfig(service=None), + return_value=BRKConfig(service=INVALID_BRK_SERVICE), ) - def test_invalid_brk_configuration_for_addressNL_is_collected(self, brk_config): + def test_invalid_brk_conf_for_addressnl_and_missing_validator_not_collected( + self, brk_config + ): FormFactory.create( generate_minimal_setup=True, formstep__form_definition__configuration={ @@ -304,30 +414,45 @@ def test_invalid_brk_configuration_for_addressNL_is_collected(self, brk_config): broken_configuration = collect_broken_configurations() - self.assertEqual(len(broken_configuration), 1) - self.assertEqual(broken_configuration[0].config_name, "BRK Client") + self.assertEqual(broken_configuration, []) @patch( "openforms.contrib.brk.client.BRKConfig.get_solo", - return_value=BRKConfig(service=None), + return_value=BRKConfig(service=INVALID_BRK_SERVICE), ) - def test_invalid_brk_congiguration_but_unused_not_collected(self, brk_config): + @requests_mock.Mocker() + def test_invalid_brk_conf_for_addressnl_and_existing_validator_collected( + self, brk_config, m + ): FormFactory.create( generate_minimal_setup=True, formstep__form_definition__configuration={ "components": [ { - "key": "textField", - "type": "textfield", - "label": "Text Field", + "key": "addressNl", + "type": "addressNL", + "label": "AddressNL", + "defaultValue": { + "postcode": "", + "houseLetter": "", + "houseNumber": "", + "houseNumberAddition": "", + }, + "validate": {"plugins": ["brk-zakelijk-gerechtigd"]}, } ], }, ) + m.get( + "https://api.brk.kadaster.nl/invalid/kadastraalonroerendezaken?postcode=1234AB&huisnummer=1", + status_code=400, + ) + broken_configuration = collect_broken_configurations() - self.assertEqual(broken_configuration, []) + self.assertEqual(len(broken_configuration), 1) + self.assertEqual(broken_configuration[0].config_name, "BRK Client") @patch( "openforms.contrib.kadaster.models.KadasterApiConfig.get_solo", @@ -464,10 +589,8 @@ def test_invalid_bag_configuration_for_address_nl_component_is_collected( broken_configuration = collect_broken_configurations() - self.assertEqual(len(broken_configuration), 2) - self.assertIn( - "BAG Client", [config.config_name for config in broken_configuration] - ) + self.assertEqual(len(broken_configuration), 1) + self.assertEqual(broken_configuration[0].config_name, "BAG Client") @patch("openforms.contrib.kadaster.models.KadasterApiConfig.get_solo") def test_invalid_bag_configuration_for_address_nl_component_not_collected_when_disabled( @@ -491,10 +614,7 @@ def test_invalid_bag_configuration_for_address_nl_component_not_collected_when_d broken_configuration = collect_broken_configurations() - self.assertEqual(len(broken_configuration), 1) - self.assertNotIn( - "BAG Client", [config.config_name for config in broken_configuration] - ) + self.assertEqual(len(broken_configuration), 0) @override_settings(LANGUAGE_CODE="en") diff --git a/src/openforms/emails/tests/test_tasks_integration.py b/src/openforms/emails/tests/test_tasks_integration.py index 31f8d08440..479fafe970 100644 --- a/src/openforms/emails/tests/test_tasks_integration.py +++ b/src/openforms/emails/tests/test_tasks_integration.py @@ -7,6 +7,7 @@ from django.test import TestCase, override_settings from django.urls import reverse +import requests_mock from django_yubin.models import Message from freezegun import freeze_time from furl import furl @@ -15,6 +16,7 @@ from openforms.config.models import GlobalConfiguration from openforms.contrib.brk.models import BRKConfig +from openforms.contrib.brk.tests.base import INVALID_BRK_SERVICE from openforms.forms.tests.factories import ( FormFactory, FormLogicFactory, @@ -118,11 +120,14 @@ def test_no_email_sent_if_no_recipients(self, mock_global_config): @patch( "openforms.contrib.brk.client.BRKConfig.get_solo", - return_value=BRKConfig(service=None), + return_value=BRKConfig(service=INVALID_BRK_SERVICE), ) @freeze_time("2023-01-03T01:00:00+01:00") @override_settings(BASE_URL="http://testserver") - def test_email_sent_when_there_are_failures(self, mock_global_config, brk_config): + @requests_mock.Mocker() + def test_email_sent_when_there_are_failures( + self, mock_global_config, brk_config, m + ): """Integration test for all the possible failures - failed emails @@ -147,6 +152,7 @@ def test_email_sent_when_there_are_failures(self, mock_global_config, brk_config "houseNumber": "", "houseNumberAddition": "", }, + "validate": {"plugins": ["brk-zakelijk-gerechtigd"]}, } ], }, @@ -200,6 +206,11 @@ def test_email_sent_when_there_are_failures(self, mock_global_config, brk_config ) ServiceFactory.create(client_certificate=certificate) + m.get( + "https://api.brk.kadaster.nl/invalid/kadastraalonroerendezaken?postcode=1234AB&huisnummer=1", + status_code=400, + ) + # send the email digest send_email_digest() sent_email = mail.outbox[-1] @@ -250,7 +261,7 @@ def test_email_sent_when_there_are_failures(self, mock_global_config, brk_config with self.subTest("broken configuration"): self.assertIn( - "The configuration for 'BRK Client' is invalid (KVK endpoint is not configured).", + "The configuration for 'BRK Client' is invalid", sent_email.body, ) diff --git a/src/openforms/forms/models/form.py b/src/openforms/forms/models/form.py index 5647013db0..2b1a489c84 100644 --- a/src/openforms/forms/models/form.py +++ b/src/openforms/forms/models/form.py @@ -675,11 +675,6 @@ def deactivate(self): logger.debug("Deactivating form %s", self.admin_name) self.save(update_fields=["active", "deactivate_on"]) - def has_component(self, component_type: str) -> bool: - return any( - component["type"] == component_type for component in self.iter_components() - ) - class FormsExportQuerySet(DeleteFilesQuerySetMixin, models.QuerySet): pass