diff --git a/src/openforms/emails/digest.py b/src/openforms/emails/digest.py index 5045aaf7fe..864f2eaab4 100644 --- a/src/openforms/emails/digest.py +++ b/src/openforms/emails/digest.py @@ -10,17 +10,20 @@ from django_yubin.models import Message from furl import furl +from rest_framework import serializers from openforms.contrib.brk.service import check_brk_config_for_addressNL +from openforms.forms.models import Form from openforms.forms.models.form_registration_backend import FormRegistrationBackend from openforms.forms.models.logic import FormLogic -from openforms.forms.utils import is_form_config_valid from openforms.logging.models import TimelineLogProxy -from openforms.plugins.checks import check_general_config +from openforms.plugins.exceptions import InvalidPluginConfiguration +from openforms.registrations.base import BasePlugin from openforms.registrations.registry import register from openforms.submissions.models.submission import Submission from openforms.submissions.utils import get_filtered_submission_admin_url from openforms.typing import StrOrPromise +from openforms.variables.service import get_static_variables @dataclass @@ -75,7 +78,7 @@ class BrokenConfiguration: @dataclass -class FailedRegistrationBackend: +class InvalidRegistrationBackend: config_name: StrOrPromise exception_message: str form_name: str @@ -91,17 +94,57 @@ def admin_link(self) -> str: @dataclass -class FailedLogicRule: +class InvalidLogicRule: variable: str form_name: str form_id: str @property def admin_link(self) -> str: - form_admin_link = reverse( - "admin:forms_form_change", kwargs={"object_id": self.form_id} - ) - return form_admin_link + return reverse("admin:forms_form_change", kwargs={"object_id": self.form_id}) + + +def is_form_config_valid(plugin: BasePlugin, options: dict[str, str]) -> bool: + serializer = plugin.configuration_options(data=options) + + try: + serializer.is_valid(raise_exception=True) + except serializers.ValidationError: + return False + + return True + + +def check_general_config(plugin: BasePlugin) -> str: + try: + plugin.check_config() + + # we always raise an InvalidPluginConfiguration exception even when we have + # errors like HTTPError + except InvalidPluginConfiguration as e: + return e.args[0] + + return "" + + +def get_variables_from_logic(form_logic: FormLogic) -> list[str]: + data = form_logic.json_logic_trigger + var_values = [] + + def _recursive_find(data): + if isinstance(data, dict): + if "var" in data: + var_values.append(data["var"]) + else: + for value in data.values(): + _recursive_find(value) + elif isinstance(data, list): + for item in data: + _recursive_find(item) + + _recursive_find(data) + + return var_values def collect_failed_emails(since: datetime) -> Iterable[FailedEmail]: @@ -216,71 +259,78 @@ def collect_broken_configurations() -> list[BrokenConfiguration]: return broken_configurations -def collect_failed_registration_backends() -> list[FailedRegistrationBackend]: - registration_backends = FormRegistrationBackend.objects.all() - form_backend_mappings = [ - {registration_backend.form.admin_name: registration_backend} - for registration_backend in registration_backends - if registration_backend.form.is_available - ] - - failed_registration_backends = [] - for mapping in form_backend_mappings: - for form_name, registration_backend in mapping.items(): - form = registration_backend.form - backend_options = registration_backend.options - backend_type = registration_backend.backend - plugin = register[backend_type] - - # errors in general configuration are more straightforward for the user, - # so we show the exact ones - if broken_general_config := check_general_config(plugin): - failed_registration_backends.append( - FailedRegistrationBackend( - config_name=plugin.verbose_name, - exception_message=broken_general_config, - form_name=form_name, - ) +def collect_invalid_registration_backends() -> list[InvalidRegistrationBackend]: + registration_backends = ( + backend + for backend in FormRegistrationBackend.objects.select_related("form").iterator() + if backend.form.is_available + ) + + already_detected = [] + invalid_registration_backends = [] + for registration_backend in registration_backends: + form_name = registration_backend.form.admin_name + form = registration_backend.form + backend_options = registration_backend.options + backend_type = registration_backend.backend + plugin = register[backend_type] + + # errors in general configuration are more straightforward for the user, + # so we show the exact ones + if plugin.verbose_name not in already_detected and ( + broken_general_config := check_general_config(plugin) + ): + already_detected.append(plugin.verbose_name) + invalid_registration_backends.append( + InvalidRegistrationBackend( + config_name=plugin.verbose_name, + exception_message=broken_general_config, + form_name=form_name, ) + ) - # errors in the form level can be more detailed so here we want to show - # a more general error and the link to the broken form to investigate - if not is_form_config_valid(plugin, backend_options): - failed_registration_backends.append( - FailedRegistrationBackend( - config_name=plugin.verbose_name, - exception_message=_( - "Invalid registration backend configuration has been detected in the form level" - ), - form_name=form_name, - form_id=str(form.id), - form_level=True, - ) + # errors in the form level can be more detailed so here we want to show + # a more general error and the link to the broken form to investigate + if not is_form_config_valid(plugin, backend_options): + invalid_registration_backends.append( + InvalidRegistrationBackend( + config_name=plugin.verbose_name, + exception_message=_( + "Invalid registration backend configuration has been detected at the form level" + ), + form_name=form_name, + form_id=str(form.id), + form_level=True, ) + ) - return failed_registration_backends + return invalid_registration_backends -def collect_failed_logic_rules() -> list[FailedLogicRule]: - logic_for_active_forms = FormLogic.objects.filter(form__active=True) - variables_forms_mapping = [ - {form_variable: logic.form} - for logic in logic_for_active_forms - for form_variable in logic.get_variables_from_logic() - ] +def collect_invalid_logic_rules() -> list[InvalidLogicRule]: + forms = Form.objects.live() + static_variables = [var.key for var in get_static_variables()] - failed_logic_rules = [] - for mapping in variables_forms_mapping: - for var, form in mapping.items(): - form_variables_keys = [ - form_variable.key for form_variable in form.formvariable_set.all() - ] - - if var not in form_variables_keys: - failed_logic_rules.append( - FailedLogicRule( - variable=var, form_name=form.admin_name, form_id=str(form.id) + invalid_logic_rules = [] + for form in forms: + form_variables_keys = [ + form_variable.key for form_variable in form.formvariable_set.all() + ] + + all_keys = static_variables + form_variables_keys + form_logics = FormLogic.objects.filter(form=form) + form_logics_vars = [ + var for logic in form_logics for var in get_variables_from_logic(logic) + ] + + for var in form_logics_vars: + if var not in all_keys: + invalid_logic_rules.append( + InvalidLogicRule( + variable=var, + form_name=form.admin_name, + form_id=str(form.id), ) ) - return failed_logic_rules + return invalid_logic_rules diff --git a/src/openforms/emails/tasks.py b/src/openforms/emails/tasks.py index a121338b42..ae44e5bb3b 100644 --- a/src/openforms/emails/tasks.py +++ b/src/openforms/emails/tasks.py @@ -12,10 +12,10 @@ from .digest import ( collect_broken_configurations, collect_failed_emails, - collect_failed_logic_rules, collect_failed_prefill_plugins, - collect_failed_registration_backends, collect_failed_registrations, + collect_invalid_logic_rules, + collect_invalid_registration_backends, ) from .utils import send_mail_html @@ -29,16 +29,16 @@ def get_context_data(self) -> dict[str, Any]: failed_registrations = collect_failed_registrations(self.since) failed_prefill_plugins = collect_failed_prefill_plugins(self.since) broken_configurations = collect_broken_configurations() - failed_registration_backends = collect_failed_registration_backends() - failed_logic_rules = collect_failed_logic_rules() + invalid_registration_backends = collect_invalid_registration_backends() + invalid_logic_rules = collect_invalid_logic_rules() return { "failed_emails": failed_emails, "failed_registrations": failed_registrations, "failed_prefill_plugins": failed_prefill_plugins, "broken_configurations": broken_configurations, - "failed_registration_backends": failed_registration_backends, - "failed_logic_rules": failed_logic_rules, + "invalid_registration_backends": invalid_registration_backends, + "invalid_logic_rules": invalid_logic_rules, } 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 a7e223bdc6..3cc53f4a93 100644 --- a/src/openforms/emails/templates/emails/admin_digest.html +++ b/src/openforms/emails/templates/emails/admin_digest.html @@ -78,11 +78,11 @@
{% blocktranslate with name=failed_backend.config_name message=failed_backend.exception_message trimmed %} @@ -103,11 +103,11 @@
{% blocktranslate with var=logic_rule.variable form_name=logic_rule.form_name trimmed %} diff --git a/src/openforms/emails/tests/test_tasks.py b/src/openforms/emails/tests/test_tasks.py index f4bf1cf177..7004773219 100644 --- a/src/openforms/emails/tests/test_tasks.py +++ b/src/openforms/emails/tests/test_tasks.py @@ -5,10 +5,10 @@ from django.urls import reverse from django.utils.translation import gettext as _ -import requests import requests_mock from django_yubin.models import Message from freezegun import freeze_time +from rest_framework import serializers from openforms.config.models import GlobalConfiguration from openforms.contrib.brk.models import BRKConfig @@ -22,11 +22,11 @@ FormVariableFactory, ) from openforms.logging import logevent -from openforms.prefill.registry import register -from openforms.registrations.contrib.zgw_apis.tests.factories import ( - ZGWApiGroupConfigFactory, -) +from openforms.plugins.exceptions import InvalidPluginConfiguration +from openforms.prefill.registry import register as prefill_register +from openforms.registrations.base import BasePlugin from openforms.registrations.exceptions import RegistrationFailed +from openforms.registrations.registry import register from openforms.submissions.constants import RegistrationStatuses from openforms.submissions.tests.factories import SubmissionFactory from stuf.stuf_bg.client import NoServiceConfigured @@ -34,6 +34,34 @@ from ..tasks import send_email_digest +class OptionsSerializer(serializers.Serializer): + def validate(self, attrs): + raise serializers.ValidationError("Invalid configuration") + + +@register("test-invalid-backend") +class InvalidBackend(BasePlugin): + verbose_name = "Invalid backend" + + def register_submission(self, submission, options): + pass + + def check_config(self): + raise InvalidPluginConfiguration("Invalid") + + +@register("test-invalid-form-conf") +class InvalidFormConfiguration(BasePlugin): + verbose_name = "Invalid form configuration" + configuration_options = OptionsSerializer + + def register_submission(self, submission, options): + pass + + def check_config(self): + pass + + @override_settings(LANGUAGE_CODE="en") class EmailDigestTaskTests(TestCase): def test_create_digest_email(self): @@ -224,8 +252,8 @@ def test_email_sent_if_failed_submissions_exist(self): self.assertIn(form_2.name, sent_email.body) def test_prefill_plugin_failures_are_sent(self): - hc_plugin = register["haalcentraal"] - stufbg_plugin = register["stufbg"] + hc_plugin = prefill_register["haalcentraal"] + stufbg_plugin = prefill_register["stufbg"] # 1st form with 2 failures in the past 24 hours form_1 = FormFactory.create() @@ -408,47 +436,12 @@ def test_no_email_sent_when_brk_congiguration_is_invalid_but_unused(self): self.assertEqual(0, len(mail.outbox)) - @requests_mock.Mocker() - def test_failed_general_registration_backend_is_sent(self, m): + def test_invalid_general_registration_backend_is_sent(self): form = FormFactory.create() - zgw_group = ZGWApiGroupConfigFactory.create( - zrc_service__api_root="http://localhost:8003/zaken/api/v1/", - drc_service__api_root="http://localhost:8003/documenten/api/v1/", - ztc_service__api_root="http://localhost:8003/catalogi/api/v1/", - ) FormRegistrationBackendFactory.create( form=form, - key=f"test-zgw{form.id}", - backend="zgw-create-zaak", - options={ - "informatieobjecttype": "http://localhost:8003/catalogi/api/v1/informatieobjecttypen/123-123-123", - "zaaktype": "http://localhost:8003/catalogi/api/v1/zaaktypen/1", - "zgw_api_group": zgw_group.pk, - }, - ) - - m.get( - "http://localhost:8003/zaken/api/v1/zaken", - exc=requests.RequestException("Custom exception"), - ) - m.get( - "http://localhost:8003/catalogi/api/v1/catalogussen", - status_code=200, - json={ - "count": 1, - "next": None, - "previous": None, - "results": [ - { - "zaaktypen": [ - "http://localhost:8003/catalogi/api/v1/zaaktypen/1" - ], - "informatieobjecttypen": [ - "http://localhost:8003/catalogi/api/v1/informatieobjecttypen/123-123-123" - ], - } - ], - }, + key="plugin1", + backend="test-invalid-backend", ) with patch( @@ -466,7 +459,8 @@ def test_failed_general_registration_backend_is_sent(self, m): ) self.assertEqual(sent_email.recipients(), ["user@example.com"]) self.assertIn( - _("The configuration for plugin 'ZGW API's' is invalid."), sent_email.body + _("The configuration for plugin 'Invalid backend' is invalid."), + sent_email.body, ) self.assertNotIn( _("This affects form '{form_admin_name}'").format( @@ -476,7 +470,7 @@ def test_failed_general_registration_backend_is_sent(self, m): ) self.assertNotIn( _( - "Invalid registration backend configuration has been detected in the form level" + "Invalid registration backend configuration has been detected at the form level" ), sent_email.body, ) @@ -485,33 +479,12 @@ def test_failed_general_registration_backend_is_sent(self, m): sent_email.body, ) - @requests_mock.Mocker() - def test_failed_registration_backend_in_form_level_is_sent(self, m): + def test_invalid_registration_backend_at_form_level_is_sent(self): form = FormFactory.create() - zgw_group = ZGWApiGroupConfigFactory.create( - zrc_service__api_root="http://localhost:8003/zaken/api/v1/", - drc_service__api_root="http://localhost:8003/documenten/api/v1/", - ztc_service__api_root="http://localhost:8003/catalogi/api/v1/", - ) FormRegistrationBackendFactory.create( form=form, - key=f"test-zgw{form.id}", - backend="zgw-create-zaak", - options={ - "informatieobjecttype": "", - "zaaktype": "", - "zgw_api_group": zgw_group.pk, - }, - ) - - m.get("http://localhost:8003/zaken/api/v1/zaken", status_code=200, json=[]) - m.get( - "http://localhost:8003/documenten/api/v1/enkelvoudiginformatieobjecten", - status_code=200, - json=[], - ) - m.get( - "http://localhost:8003/catalogi/api/v1/zaaktypen", status_code=200, json=[] + key="plugin1", + backend="test-invalid-form-conf", ) with patch( @@ -529,7 +502,8 @@ def test_failed_registration_backend_in_form_level_is_sent(self, m): ) self.assertEqual(sent_email.recipients(), ["user@example.com"]) self.assertIn( - _("The configuration for plugin 'ZGW API's' is invalid."), sent_email.body + _("The configuration for plugin 'Invalid form configuration' is invalid."), + sent_email.body, ) self.assertIn( _("This affects form '{form_admin_name}'").format( @@ -539,7 +513,7 @@ def test_failed_registration_backend_in_form_level_is_sent(self, m): ) self.assertIn( _( - "Invalid registration backend configuration has been detected in the form level" + "Invalid registration backend configuration has been detected at the form level" ), sent_email.body, ) @@ -548,51 +522,17 @@ def test_failed_registration_backend_in_form_level_is_sent(self, m): sent_email.body, ) - @requests_mock.Mocker() - def test_failed_registration_backend_in_both_general_and_form_level_is_sent( - self, m - ): + def test_invalid_registration_backend_at_both_general_and_form_level_is_sent(self): form = FormFactory.create() - form1 = FormFactory.create() - zgw_group = ZGWApiGroupConfigFactory.create( - zrc_service__api_root="http://localhost:8003/zaken/api/v1/", - drc_service__api_root="http://localhost:8003/documenten/api/v1/", - ztc_service__api_root="http://localhost:8003/catalogi/api/v1/", - ) FormRegistrationBackendFactory.create( form=form, - key=f"test-zgw{form.id}", - backend="zgw-create-zaak", - options={ - "informatieobjecttype": "", - "zaaktype": "", - "zgw_api_group": zgw_group.pk, - }, - ) - FormRegistrationBackendFactory.create( - form=form1, - key=f"test-zgw{form.id}", - backend="zgw-create-zaak", - options={ - "informatieobjecttype": "", - "zaaktype": "", - "zgw_api_group": zgw_group.pk, - }, + key="plugin1", + backend="test-invalid-backend", ) FormRegistrationBackendFactory.create( form=form, - key=f"test-zgw{form.id}2", - backend="zgw-create-zaak", - options={ - "informatieobjecttype": "", - "zaaktype": "", - "zgw_api_group": zgw_group.pk, - }, - ) - - m.get( - "http://localhost:8003/zaken/api/v1/zaken", - exc=requests.RequestException("Custom exception"), + key="plugin2", + backend="test-invalid-form-conf", ) with patch( @@ -610,7 +550,8 @@ def test_failed_registration_backend_in_both_general_and_form_level_is_sent( ) self.assertEqual(sent_email.recipients(), ["user@example.com"]) self.assertIn( - _("The configuration for plugin 'ZGW API's' is invalid."), sent_email.body + _("The configuration for plugin 'Invalid backend' is invalid."), + sent_email.body, ) self.assertIn( _("This affects form '{form_admin_name}'").format( @@ -620,7 +561,7 @@ def test_failed_registration_backend_in_both_general_and_form_level_is_sent( ) self.assertIn( _( - "Invalid registration backend configuration has been detected in the form level" + "Invalid registration backend configuration has been detected at the form level" ), sent_email.body, ) @@ -629,28 +570,12 @@ def test_failed_registration_backend_in_both_general_and_form_level_is_sent( sent_email.body, ) - @requests_mock.Mocker() - def test_failed_registration_backend_is_not_sent_when_form_unavailable(self, m): + def test_invalid_registration_backend_is_not_sent_when_form_unavailable(self): form = FormFactory.create(active=False) - zgw_group = ZGWApiGroupConfigFactory.create( - zrc_service__api_root="http://localhost:8003/zaken/api/v1/", - drc_service__api_root="http://localhost:8003/documenten/api/v1/", - ztc_service__api_root="http://localhost:8003/catalogi/api/v1/", - ) FormRegistrationBackendFactory.create( form=form, - key=f"test-zgw{form.id}", - backend="zgw-create-zaak", - options={ - "informatieobjecttype": "http://oz.nl/catalogi/api/v1/informatieobjecttypen/123-123-123", - "zaaktype": "http://catalogi.nl/api/v1/zaaktypen/1", - "zgw_api_group": zgw_group.pk, - }, - ) - - m.get( - "http://localhost:8003/zaken/api/v1/zaken", - exc=requests.RequestException("Custom exception"), + key="plugin1", + backend="test-invalid-backend", ) with patch( @@ -663,9 +588,9 @@ def test_failed_registration_backend_is_not_sent_when_form_unavailable(self, m): self.assertEqual(len(mail.outbox), 0) - def test_failed_logic_rules_are_sent(self): + def test_invalid_logic_rules_are_sent(self): form = FormFactory.create() - logic = FormLogicFactory(form=form) + FormLogicFactory(form=form, json_logic_trigger={"==": [{"var": "test-key"}, 1]}) with patch( "openforms.emails.tasks.GlobalConfiguration.get_solo", @@ -683,10 +608,8 @@ def test_failed_logic_rules_are_sent(self): self.assertEqual(sent_email.recipients(), ["user@example.com"]) self.assertIn( _( - "Logic rule for variable '{var}' is invalid in form '{form_name}'." - ).format( - var=logic.get_variables_from_logic()[0], form_name=form.admin_name - ), + "Logic rule for variable 'test-key' is invalid in form '{form_name}'." + ).format(form_name=form.admin_name), sent_email.body, ) self.assertIn( diff --git a/src/openforms/forms/models/logic.py b/src/openforms/forms/models/logic.py index e60f2ee1f8..c8a17449a6 100644 --- a/src/openforms/forms/models/logic.py +++ b/src/openforms/forms/models/logic.py @@ -76,22 +76,3 @@ def action_operations(self): for action in map(compile_action_operation, self.actions): action.rule = self yield action - - def get_variables_from_logic(self) -> list[str]: - data = self.json_logic_trigger - var_values = [] - - def _recursive_find(data): - if isinstance(data, dict): - if "var" in data: - var_values.append(data["var"]) - else: - for value in data.values(): - _recursive_find(value) - elif isinstance(data, list): - for item in data: - _recursive_find(item) - - _recursive_find(data) - - return var_values diff --git a/src/openforms/forms/tests/test_models.py b/src/openforms/forms/tests/test_models.py index 3d6789ac7f..4eab36ba7c 100644 --- a/src/openforms/forms/tests/test_models.py +++ b/src/openforms/forms/tests/test_models.py @@ -501,16 +501,6 @@ def test_block_form_logic_trigger_step_other_form(self): except ValidationError: self.fail("Should be allowed") - def test_get_variables_from_logic_returns_expected_list(self): - form = FormFactory.create() - logic = FormLogicFactory( - form=form, json_logic_trigger={"==": [{"var": "test-key"}, 1]} - ) - variables_list = logic.get_variables_from_logic() - expected_list = ["test-key"] - - self.assertEqual(variables_list, expected_list) - class FormRegistrationBackendTests(TestCase): def test_string_contains_both_parts_of_the_relation(self): diff --git a/src/openforms/forms/utils.py b/src/openforms/forms/utils.py index fbbd319eca..b3491be734 100644 --- a/src/openforms/forms/utils.py +++ b/src/openforms/forms/utils.py @@ -12,13 +12,11 @@ from django.utils import timezone from django.utils.translation import override -from rest_framework import serializers from rest_framework.exceptions import ValidationError from rest_framework.test import APIRequestFactory from openforms.formio.migration_converters import CONVERTERS from openforms.formio.utils import iter_components -from openforms.registrations.base import BasePlugin from openforms.variables.constants import FormVariableSources from .api.datastructures import FormVariableWrapper @@ -404,14 +402,3 @@ def clear_old_service_fetch_config(rule: dict) -> None: # We can't reliably relate the service fetch configured to an existing configuration. # So we don't add any existing service fetch config to the variables action["action"]["value"] = "" - - -def is_form_config_valid(plugin: BasePlugin, options: dict[str, str]) -> bool: - serializer = plugin.configuration_options(data=options) - - try: - serializer.is_valid(raise_exception=True) - except serializers.ValidationError: - return False - - return True diff --git a/src/openforms/plugins/checks.py b/src/openforms/plugins/checks.py deleted file mode 100644 index 4d03663885..0000000000 --- a/src/openforms/plugins/checks.py +++ /dev/null @@ -1,14 +0,0 @@ -from openforms.plugins.exceptions import InvalidPluginConfiguration -from openforms.registrations.base import BasePlugin - - -def check_general_config(plugin: BasePlugin) -> str: - try: - plugin.check_config() - - # we always raise an InvalidPluginConfiguration exception even when we have - # errors like HTTPError - except InvalidPluginConfiguration as e: - return e.args[0] - - return ""