From 8d8335c2bf6c67dff18ec23d52d4e3ed6849f16b Mon Sep 17 00:00:00 2001 From: vasileios Date: Fri, 12 Apr 2024 10:37:23 +0200 Subject: [PATCH 1/3] [#3725] Added detection of BRP broken configuration to the email digest --- src/openforms/config/service.py | 39 +++++ src/openforms/emails/tasks.py | 3 + .../emails/templates/emails/admin_digest.html | 14 ++ src/openforms/emails/tests/test_tasks.py | 137 +++++++++++++++++- src/openforms/forms/models/form.py | 5 + 5 files changed, 197 insertions(+), 1 deletion(-) create mode 100644 src/openforms/config/service.py diff --git a/src/openforms/config/service.py b/src/openforms/config/service.py new file mode 100644 index 0000000000..2e1b1d17e0 --- /dev/null +++ b/src/openforms/config/service.py @@ -0,0 +1,39 @@ +from dataclasses import dataclass + +from openforms.contrib.brk.checks import BRKValidatorCheck +from openforms.forms.models.form import Form +from openforms.plugins.exceptions import InvalidPluginConfiguration + + +@dataclass +class BrokenConfiguration: + config_name: str + exception_message: str | None + + +def check_BRK_config_for_addressNl() -> str | None: + live_forms = Form.objects.live() + + for form in live_forms: + if form.form_uses_component("addressNL"): + try: + BRKValidatorCheck.check_config() + except InvalidPluginConfiguration as e: + return e.args[0] + break + + return + + +def collect_broken_configurations() -> list[BrokenConfiguration]: + check_BRK_configuration = check_BRK_config_for_addressNl() + + broken_configurations = [] + if check_BRK_configuration: + broken_configurations.append( + BrokenConfiguration( + config_name="BRK Client", exception_message=check_BRK_configuration + ) + ) + + return broken_configurations diff --git a/src/openforms/emails/tasks.py b/src/openforms/emails/tasks.py index dd9c48cc70..21875528e1 100644 --- a/src/openforms/emails/tasks.py +++ b/src/openforms/emails/tasks.py @@ -8,6 +8,7 @@ from openforms.celery import app from openforms.config.models import GlobalConfiguration +from openforms.config.service import collect_broken_configurations from openforms.logging.service import ( collect_failed_emails, collect_failed_prefill_plugins, @@ -25,11 +26,13 @@ def get_context_data(self) -> dict[str, Any]: failed_emails = collect_failed_emails(self.since) failed_registrations = collect_failed_registrations(self.since) failed_prefill_plugins = collect_failed_prefill_plugins(self.since) + broken_configurations = collect_broken_configurations() return { "failed_emails": failed_emails, "failed_registrations": failed_registrations, "failed_prefill_plugins": failed_prefill_plugins, + "broken_configurations": broken_configurations, } def render(self) -> str: diff --git a/src/openforms/emails/templates/emails/admin_digest.html b/src/openforms/emails/templates/emails/admin_digest.html index d27d33f5fe..84f641d277 100644 --- a/src/openforms/emails/templates/emails/admin_digest.html +++ b/src/openforms/emails/templates/emails/admin_digest.html @@ -63,3 +63,17 @@
{% trans "Prefill plugins" %}
{% endfor %} {% endif %} + +{% if broken_configurations %} +
{% trans "Broken configuration" %}
+ +{% endif %} diff --git a/src/openforms/emails/tests/test_tasks.py b/src/openforms/emails/tests/test_tasks.py index 425d419b0f..e849ebc93c 100644 --- a/src/openforms/emails/tests/test_tasks.py +++ b/src/openforms/emails/tests/test_tasks.py @@ -2,12 +2,20 @@ from django.core import mail from django.test import TestCase, override_settings +from django.utils.translation import gettext as _ +import requests_mock from django_yubin.models import Message from freezegun import freeze_time from openforms.config.models import GlobalConfiguration -from openforms.forms.tests.factories import FormFactory +from openforms.contrib.brk.models import BRKConfig +from openforms.contrib.brk.tests.base import BRK_SERVICE +from openforms.forms.tests.factories import ( + FormDefinitionFactory, + FormFactory, + FormStepFactory, +) from openforms.logging import logevent from openforms.prefill.registry import register from openforms.registrations.exceptions import RegistrationFailed @@ -260,3 +268,130 @@ def test_prefill_plugin_failures_are_sent(self): self.assertIn(str(submission_1.id), sent_email.body) self.assertIn(str(submission_2.id), sent_email.body) self.assertIn(str(submission.id), sent_email.body) + + @requests_mock.Mocker() + def test_no_email_sent_when_brk_congiguration_is_valid_for_addressNL(self, m): + form = FormFactory.create() + form_definition = FormDefinitionFactory.create( + configuration={ + "display": "form", + "components": [ + { + "key": "addressNl", + "type": "addressNL", + "label": "AddressNL", + "defaultValue": { + "postcode": "", + "houseLetter": "", + "houseNumber": "", + "houseNumberAddition": "", + }, + } + ], + } + ) + FormStepFactory.create(form=form, form_definition=form_definition) + + with ( + freeze_time("2023-01-03T01:00:00+01:00"), + patch( + "openforms.emails.tasks.GlobalConfiguration.get_solo", + return_value=GlobalConfiguration( + recipients_email_digest=["user@example.com"] + ), + ), + patch( + "openforms.contrib.brk.client.BRKConfig.get_solo", + return_value=BRKConfig(service=BRK_SERVICE), + ), + ): + + m.get( + "https://api.brk.kadaster.nl/esd-eto-apikey/bevragen/v2/kadastraalonroerendezaken?postcode=1234AB&huisnummer=1", + json={"_embedded": {}}, + ) + + send_email_digest() + + self.assertEqual(0, len(mail.outbox)) + + def test_broken_brk_configuration_for_addressNL_is_sent(self): + form = FormFactory.create() + form_definition = FormDefinitionFactory.create( + configuration={ + "display": "form", + "components": [ + { + "key": "addressNl", + "type": "addressNL", + "label": "AddressNL", + "defaultValue": { + "postcode": "", + "houseLetter": "", + "houseNumber": "", + "houseNumberAddition": "", + }, + } + ], + } + ) + FormStepFactory.create(form=form, form_definition=form_definition) + + with ( + freeze_time("2023-01-03T01:00:00+01:00"), + patch( + "openforms.emails.tasks.GlobalConfiguration.get_solo", + return_value=GlobalConfiguration( + recipients_email_digest=["user@example.com"] + ), + ), + patch( + "openforms.contrib.brk.client.BRKConfig.get_solo", + return_value=BRKConfig(service=None), + ), + ): + + send_email_digest() + + sent_email = mail.outbox[0] + + self.assertEqual( + sent_email.subject, "[Open Forms] Daily summary of detected problems" + ) + self.assertEqual(sent_email.recipients(), ["user@example.com"]) + self.assertIn( + _( + "Configuration for 'BRK Client' is broken (KVK endpoint is not configured.)" + ), + sent_email.body, + ) + + def test_no_email_sent_when_brk_congiguration_is_valid_for_other_component(self): + form = FormFactory.create() + form_definition = FormDefinitionFactory.create( + configuration={ + "display": "form", + "components": [ + { + "key": "textField", + "type": "textfield", + "label": "Text Field", + } + ], + } + ) + FormStepFactory.create(form=form, form_definition=form_definition) + + with ( + freeze_time("2023-01-03T01:00:00+01:00"), + patch( + "openforms.emails.tasks.GlobalConfiguration.get_solo", + return_value=GlobalConfiguration( + recipients_email_digest=["user@example.com"] + ), + ), + ): + + send_email_digest() + + self.assertEqual(0, len(mail.outbox)) diff --git a/src/openforms/forms/models/form.py b/src/openforms/forms/models/form.py index 7c8e8abb33..0b6dbb7c25 100644 --- a/src/openforms/forms/models/form.py +++ b/src/openforms/forms/models/form.py @@ -603,6 +603,11 @@ def deactivate(self): logger.debug("Deactivating form %s", self.admin_name) self.save(update_fields=["active", "deactivate_on"]) + def form_uses_component(self, component_type: str) -> bool: + return any( + component["type"] == component_type for component in self.iter_components() + ) + class FormsExportQuerySet(DeleteFilesQuerySetMixin, models.QuerySet): pass From 7727b356c367e4117f1a6e339f1a959d6af7cf3f Mon Sep 17 00:00:00 2001 From: vasileios Date: Mon, 15 Apr 2024 12:44:33 +0200 Subject: [PATCH 2/3] [#3725] PR feedback --- src/openforms/config/service.py | 25 ++++++++++--------- src/openforms/contrib/brk/checks.py | 2 +- .../emails/templates/emails/admin_digest.html | 2 +- src/openforms/emails/tests/test_tasks.py | 2 +- src/openforms/forms/models/form.py | 2 +- 5 files changed, 17 insertions(+), 16 deletions(-) diff --git a/src/openforms/config/service.py b/src/openforms/config/service.py index 2e1b1d17e0..baacd7281a 100644 --- a/src/openforms/config/service.py +++ b/src/openforms/config/service.py @@ -1,38 +1,39 @@ from dataclasses import dataclass +from django.utils.translation import gettext_lazy as _ + from openforms.contrib.brk.checks import BRKValidatorCheck from openforms.forms.models.form import Form from openforms.plugins.exceptions import InvalidPluginConfiguration +from openforms.typing import StrOrPromise @dataclass class BrokenConfiguration: - config_name: str + config_name: StrOrPromise exception_message: str | None -def check_BRK_config_for_addressNl() -> str | None: +def check_brk_config_for_addressNL() -> str | None: live_forms = Form.objects.live() - for form in live_forms: - if form.form_uses_component("addressNL"): - try: - BRKValidatorCheck.check_config() - except InvalidPluginConfiguration as e: - return e.args[0] - break + if any(form.has_component("addressNL") for form in live_forms): + try: + BRKValidatorCheck.check_config() + except InvalidPluginConfiguration as e: + return e.args[0] return def collect_broken_configurations() -> list[BrokenConfiguration]: - check_BRK_configuration = check_BRK_config_for_addressNl() + check_brk_configuration = check_brk_config_for_addressNL() broken_configurations = [] - if check_BRK_configuration: + if check_brk_configuration: broken_configurations.append( BrokenConfiguration( - config_name="BRK Client", exception_message=check_BRK_configuration + config_name=_("BRK Client"), exception_message=check_brk_configuration ) ) diff --git a/src/openforms/contrib/brk/checks.py b/src/openforms/contrib/brk/checks.py index 255eba53e6..7490d07fb2 100644 --- a/src/openforms/contrib/brk/checks.py +++ b/src/openforms/contrib/brk/checks.py @@ -21,7 +21,7 @@ def check_config(): {"postcode": "1234AB", "huisnummer": "1"} ) except NoServiceConfigured as exc: - msg = _("{api_name} endpoint is not configured.").format(api_name="KVK") + msg = _("{api_name} endpoint is not configured").format(api_name="KVK") raise InvalidPluginConfiguration(msg) from exc except requests.RequestException as exc: raise InvalidPluginConfiguration( diff --git a/src/openforms/emails/templates/emails/admin_digest.html b/src/openforms/emails/templates/emails/admin_digest.html index 84f641d277..061cc609d1 100644 --- a/src/openforms/emails/templates/emails/admin_digest.html +++ b/src/openforms/emails/templates/emails/admin_digest.html @@ -70,7 +70,7 @@
{% trans "Broken configuration" %}
{% for config in broken_configurations %}
  • {% blocktranslate with name=config.config_name message=config.exception_message trimmed %} - Configuration for '{{ name }}' is broken ({{ message }})
    + Configuration for '{{ name }}' is broken ({{ message }}).
    Components or plugins which make use of this will not work properly. {% endblocktranslate %}
    • diff --git a/src/openforms/emails/tests/test_tasks.py b/src/openforms/emails/tests/test_tasks.py index e849ebc93c..be776d1a20 100644 --- a/src/openforms/emails/tests/test_tasks.py +++ b/src/openforms/emails/tests/test_tasks.py @@ -361,7 +361,7 @@ def test_broken_brk_configuration_for_addressNL_is_sent(self): self.assertEqual(sent_email.recipients(), ["user@example.com"]) self.assertIn( _( - "Configuration for 'BRK Client' is broken (KVK endpoint is not configured.)" + "Configuration for 'BRK Client' is broken (KVK endpoint is not configured)." ), sent_email.body, ) diff --git a/src/openforms/forms/models/form.py b/src/openforms/forms/models/form.py index 0b6dbb7c25..3304a1666e 100644 --- a/src/openforms/forms/models/form.py +++ b/src/openforms/forms/models/form.py @@ -603,7 +603,7 @@ def deactivate(self): logger.debug("Deactivating form %s", self.admin_name) self.save(update_fields=["active", "deactivate_on"]) - def form_uses_component(self, component_type: str) -> bool: + def has_component(self, component_type: str) -> bool: return any( component["type"] == component_type for component in self.iter_components() ) From c22d0d90d3638af99961d6ab791991876ab37c14 Mon Sep 17 00:00:00 2001 From: vasileios Date: Wed, 17 Apr 2024 09:49:23 +0200 Subject: [PATCH 3/3] [#3725] PR feedback-refactor --- src/openforms/config/service.py | 40 ------------------- src/openforms/contrib/brk/service.py | 16 ++++++++ .../{logging/service.py => emails/digest.py} | 23 +++++++++++ src/openforms/emails/tasks.py | 6 +-- .../emails/templates/emails/admin_digest.html | 4 +- src/openforms/emails/tests/test_tasks.py | 8 +++- 6 files changed, 50 insertions(+), 47 deletions(-) delete mode 100644 src/openforms/config/service.py create mode 100644 src/openforms/contrib/brk/service.py rename src/openforms/{logging/service.py => emails/digest.py} (87%) diff --git a/src/openforms/config/service.py b/src/openforms/config/service.py deleted file mode 100644 index baacd7281a..0000000000 --- a/src/openforms/config/service.py +++ /dev/null @@ -1,40 +0,0 @@ -from dataclasses import dataclass - -from django.utils.translation import gettext_lazy as _ - -from openforms.contrib.brk.checks import BRKValidatorCheck -from openforms.forms.models.form import Form -from openforms.plugins.exceptions import InvalidPluginConfiguration -from openforms.typing import StrOrPromise - - -@dataclass -class BrokenConfiguration: - config_name: StrOrPromise - exception_message: str | None - - -def check_brk_config_for_addressNL() -> str | None: - 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] - - return - - -def collect_broken_configurations() -> list[BrokenConfiguration]: - check_brk_configuration = check_brk_config_for_addressNL() - - broken_configurations = [] - if check_brk_configuration: - broken_configurations.append( - BrokenConfiguration( - config_name=_("BRK Client"), exception_message=check_brk_configuration - ) - ) - - return broken_configurations diff --git a/src/openforms/contrib/brk/service.py b/src/openforms/contrib/brk/service.py new file mode 100644 index 0000000000..2d1d49056f --- /dev/null +++ b/src/openforms/contrib/brk/service.py @@ -0,0 +1,16 @@ +from openforms.forms.models.form import Form +from openforms.plugins.exceptions import InvalidPluginConfiguration + +from .checks import BRKValidatorCheck + + +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] + + return "" diff --git a/src/openforms/logging/service.py b/src/openforms/emails/digest.py similarity index 87% rename from src/openforms/logging/service.py rename to src/openforms/emails/digest.py index d950abfa79..f67af0549d 100644 --- a/src/openforms/logging/service.py +++ b/src/openforms/emails/digest.py @@ -6,13 +6,16 @@ from django.contrib.contenttypes.models import ContentType from django.urls import reverse +from django.utils.translation import gettext_lazy as _ from django_yubin.models import Message from furl import furl +from openforms.contrib.brk.service import check_brk_config_for_addressNL from openforms.logging.models import TimelineLogProxy from openforms.submissions.models.submission import Submission from openforms.submissions.utils import get_filtered_submission_admin_url +from openforms.typing import StrOrPromise @dataclass @@ -60,6 +63,12 @@ def failed_submissions_counter(self) -> int: return len(self.submission_ids) +@dataclass +class BrokenConfiguration: + config_name: StrOrPromise + exception_message: str + + def collect_failed_emails(since: datetime) -> Iterable[FailedEmail]: logs = TimelineLogProxy.objects.filter( timestamp__gt=since, @@ -156,3 +165,17 @@ def collect_failed_prefill_plugins(since: datetime) -> list[FailedPrefill]: ) return failed_prefill_plugins + + +def collect_broken_configurations() -> list[BrokenConfiguration]: + check_brk_configuration = check_brk_config_for_addressNL() + + broken_configurations = [] + if check_brk_configuration: + broken_configurations.append( + BrokenConfiguration( + config_name=_("BRK Client"), exception_message=check_brk_configuration + ) + ) + + return broken_configurations diff --git a/src/openforms/emails/tasks.py b/src/openforms/emails/tasks.py index 21875528e1..2963fad61c 100644 --- a/src/openforms/emails/tasks.py +++ b/src/openforms/emails/tasks.py @@ -8,13 +8,13 @@ from openforms.celery import app from openforms.config.models import GlobalConfiguration -from openforms.config.service import collect_broken_configurations -from openforms.logging.service import ( + +from .digest import ( + collect_broken_configurations, collect_failed_emails, collect_failed_prefill_plugins, collect_failed_registrations, ) - from .utils import send_mail_html diff --git a/src/openforms/emails/templates/emails/admin_digest.html b/src/openforms/emails/templates/emails/admin_digest.html index 061cc609d1..474eee6f89 100644 --- a/src/openforms/emails/templates/emails/admin_digest.html +++ b/src/openforms/emails/templates/emails/admin_digest.html @@ -65,12 +65,12 @@
    {% trans "Prefill plugins" %}
    {% endif %} {% if broken_configurations %} -
    {% trans "Broken configuration" %}
    +
    {% trans "Configuration problems" %}
      {% for config in broken_configurations %}
    • {% blocktranslate with name=config.config_name message=config.exception_message trimmed %} - Configuration for '{{ name }}' is broken ({{ message }}).
      + The configuration for '{{ name }}' is invalid ({{ message }}).
      Components or plugins which make use of this will not work properly. {% endblocktranslate %}
      • diff --git a/src/openforms/emails/tests/test_tasks.py b/src/openforms/emails/tests/test_tasks.py index be776d1a20..fb10c93023 100644 --- a/src/openforms/emails/tests/test_tasks.py +++ b/src/openforms/emails/tests/test_tasks.py @@ -361,12 +361,12 @@ def test_broken_brk_configuration_for_addressNL_is_sent(self): self.assertEqual(sent_email.recipients(), ["user@example.com"]) self.assertIn( _( - "Configuration for 'BRK Client' is broken (KVK endpoint is not configured)." + "The configuration for 'BRK Client' is invalid (KVK endpoint is not configured)." ), sent_email.body, ) - def test_no_email_sent_when_brk_congiguration_is_valid_for_other_component(self): + def test_no_email_sent_when_brk_congiguration_is_invalid_but_unused(self): form = FormFactory.create() form_definition = FormDefinitionFactory.create( configuration={ @@ -390,6 +390,10 @@ def test_no_email_sent_when_brk_congiguration_is_valid_for_other_component(self) recipients_email_digest=["user@example.com"] ), ), + patch( + "openforms.contrib.brk.client.BRKConfig.get_solo", + return_value=BRKConfig(service=None), + ), ): send_email_digest()