From b411b2f8703ce74c378a44c452f85cd5921949bf Mon Sep 17 00:00:00 2001 From: SilviaAmAm Date: Tue, 28 Nov 2023 16:17:41 +0100 Subject: [PATCH] :ok_hand: [#3005] PR Feedback part 3 --- .../tests/test_api_appointment_create.py | 4 +- src/openforms/payments/tasks.py | 17 +-- .../stuf_zds/tests/test_failure_modes.py | 27 ++-- .../contrib/zgw_apis/tests/test_backend.py | 30 ++--- src/openforms/registrations/error_handler.py | 115 ++++++++++++++++++ src/openforms/registrations/exceptions.py | 8 ++ src/openforms/registrations/tasks.py | 104 +++------------- .../tests/test_pre_registration.py | 6 +- .../tests/test_registration_hook.py | 63 ++++------ ...124_1625.py => 0004_auto_20231128_1536.py} | 14 ++- .../models/post_completion_metadata.py | 12 +- .../submissions/models/submission.py | 9 +- src/openforms/submissions/tasks/__init__.py | 12 +- src/openforms/submissions/tests/factories.py | 2 +- .../tests/renderer/test_renderer.py | 6 + src/openforms/submissions/tests/test_admin.py | 20 +-- .../tests/test_on_completion_retry_chain.py | 7 +- .../tests/test_submission_status.py | 14 +-- 18 files changed, 276 insertions(+), 194 deletions(-) create mode 100644 src/openforms/registrations/error_handler.py rename src/openforms/submissions/migrations/{0004_auto_20231124_1625.py => 0004_auto_20231128_1536.py} (89%) diff --git a/src/openforms/appointments/tests/test_api_appointment_create.py b/src/openforms/appointments/tests/test_api_appointment_create.py index b719980d2a..a58b95a87a 100644 --- a/src/openforms/appointments/tests/test_api_appointment_create.py +++ b/src/openforms/appointments/tests/test_api_appointment_create.py @@ -116,7 +116,7 @@ def test_appointment_data_is_recorded(self): self.assertTrue(self.submission.privacy_policy_accepted) @patch("openforms.submissions.api.mixins.on_post_submission_event") - def test_submission_is_completed(self, mock_on_completion): + def test_submission_is_completed(self, mock_on_post_submission_event): appointment_datetime = timezone.make_aware( datetime.combine(TODAY, time(15, 15)) ) @@ -148,7 +148,7 @@ def test_submission_is_completed(self, mock_on_completion): self.assertIn("statusUrl", response.json()) # assert that the async celery task execution is scheduled - mock_on_completion.assert_called_once_with( + mock_on_post_submission_event.assert_called_once_with( self.submission.id, PostSubmissionEvents.on_completion ) diff --git a/src/openforms/payments/tasks.py b/src/openforms/payments/tasks.py index 0c31d686b0..fcbbb3df77 100644 --- a/src/openforms/payments/tasks.py +++ b/src/openforms/payments/tasks.py @@ -1,9 +1,8 @@ import logging -from celery_once import QueueOnce - from openforms.celery import app from openforms.logging import logevent +from openforms.registrations.error_handler import RegistrationInteractionTask from openforms.submissions.constants import RegistrationStatuses from openforms.submissions.models import Submission @@ -16,7 +15,7 @@ @app.task( - base=QueueOnce, + base=RegistrationInteractionTask, once={"graceful": True}, # do not spam error monitoring ) def update_submission_payment_status(submission_id: int): @@ -37,14 +36,4 @@ def update_submission_payment_status(submission_id: int): logger.info("Skippping payment update for submission %d.", submission_id) return - is_retrying = submission.needs_on_completion_retry - try: - update_submission_payment_registration(submission) - except Exception as exc: - logger.info("Updating submission payment registration failed", exc_info=exc) - submission.needs_on_completion_retry = True - submission.save(update_fields=["needs_on_completion_retry"]) - # re-raise if we're in the retry workflow to break the entire chain - if is_retrying: - raise - return + update_submission_payment_registration(submission) diff --git a/src/openforms/registrations/contrib/stuf_zds/tests/test_failure_modes.py b/src/openforms/registrations/contrib/stuf_zds/tests/test_failure_modes.py index 48fa190cfc..dddf95fc4a 100644 --- a/src/openforms/registrations/contrib/stuf_zds/tests/test_failure_modes.py +++ b/src/openforms/registrations/contrib/stuf_zds/tests/test_failure_modes.py @@ -1,6 +1,6 @@ from unittest.mock import patch -from django.test import override_settings, tag +from django.test import tag import requests_mock from freezegun import freeze_time @@ -8,8 +8,8 @@ from openforms.config.models import GlobalConfiguration from openforms.forms.tests.factories import FormRegistrationBackendFactory -from openforms.submissions.constants import PostSubmissionEvents, RegistrationStatuses -from openforms.submissions.tasks import on_post_submission_event +from openforms.submissions.constants import RegistrationStatuses +from openforms.submissions.tasks import pre_registration from openforms.submissions.tests.factories import SubmissionFactory from stuf.stuf_zds.models import StufZDSConfig from stuf.stuf_zds.tests import StUFZDSTestBase @@ -96,7 +96,6 @@ def setUp(self): self.addCleanup(self.requests_mock.stop) self.addCleanup(self.submission.refresh_from_db) - @override_settings(CELERY_TASK_ALWAYS_EAGER=True) def test_zaak_creation_fails_number_is_reserved(self): self.requests_mock.post( self.service.soap_service.url, @@ -115,7 +114,8 @@ def test_zaak_creation_fails_number_is_reserved(self): additional_matcher=match_text("zakLk01"), ) - on_post_submission_event(self.submission.id, PostSubmissionEvents.on_completion) + pre_registration(self.submission.id) + register_submission(self.submission.id) with self.subTest("Initial registration fails"): self.submission.refresh_from_db() self.assertEqual( @@ -128,6 +128,9 @@ def test_zaak_creation_fails_number_is_reserved(self): ) self.assertIn("traceback", self.submission.registration_result) + # This flag would be set by the finalise_completion task + self.submission.needs_on_completion_retry = True + self.submission.save() with self.subTest("Retry does not create new zaaknummer"): with self.assertRaises(RegistrationFailed): register_submission(self.submission.id) @@ -168,7 +171,6 @@ def test_zaak_creation_fails_number_is_reserved(self): }, ) - @override_settings(CELERY_TASK_ALWAYS_EAGER=True) def test_doc_id_creation_fails_zaak_registration_succeeds(self): self.requests_mock.post( self.service.soap_service.url, @@ -192,7 +194,8 @@ def test_doc_id_creation_fails_zaak_registration_succeeds(self): additional_matcher=match_text("genereerDocumentIdentificatie_Di02"), ) - on_post_submission_event(self.submission.id, PostSubmissionEvents.on_completion) + pre_registration(self.submission.id) + register_submission(self.submission.id) with self.subTest("Document id generation fails"): self.submission.refresh_from_db() self.assertEqual( @@ -208,6 +211,9 @@ def test_doc_id_creation_fails_zaak_registration_succeeds(self): ) self.assertIn("traceback", self.submission.registration_result) + # This flag would be set by the finalise_completion task + self.submission.needs_on_completion_retry = True + self.submission.save() with self.subTest("Retry does not create new zaak"): with self.assertRaises(RegistrationFailed): register_submission(self.submission.id) @@ -256,7 +262,6 @@ def test_doc_id_creation_fails_zaak_registration_succeeds(self): {"//zkn:stuurgegevens/stuf:functie": "genereerDocumentidentificatie"}, ) - @override_settings(CELERY_TASK_ALWAYS_EAGER=True) def test_doc_creation_failure_does_not_create_more_doc_ids(self): self.requests_mock.post( self.service.soap_service.url, @@ -288,7 +293,8 @@ def test_doc_creation_failure_does_not_create_more_doc_ids(self): additional_matcher=match_text("edcLk01"), ) - on_post_submission_event(self.submission.id, PostSubmissionEvents.on_completion) + pre_registration(self.submission.id) + register_submission(self.submission.id) with self.subTest("Document id generation fails"): self.submission.refresh_from_db() self.assertEqual( @@ -307,6 +313,9 @@ def test_doc_creation_failure_does_not_create_more_doc_ids(self): ) self.assertIn("traceback", self.submission.registration_result) + # This flag would be set by the finalise_completion task + self.submission.needs_on_completion_retry = True + self.submission.save() with self.subTest("Retry does not create new zaak"): with self.assertRaises(RegistrationFailed): register_submission(self.submission.id) diff --git a/src/openforms/registrations/contrib/zgw_apis/tests/test_backend.py b/src/openforms/registrations/contrib/zgw_apis/tests/test_backend.py index 043dd313a3..f26bbd0775 100644 --- a/src/openforms/registrations/contrib/zgw_apis/tests/test_backend.py +++ b/src/openforms/registrations/contrib/zgw_apis/tests/test_backend.py @@ -10,9 +10,9 @@ from zgw_consumers.test import generate_oas_component from openforms.authentication.tests.factories import RegistratorInfoFactory -from openforms.submissions.constants import PostSubmissionEvents, RegistrationStatuses +from openforms.submissions.constants import RegistrationStatuses from openforms.submissions.models import SubmissionStep -from openforms.submissions.tasks import on_post_submission_event, pre_registration +from openforms.submissions.tasks import pre_registration from openforms.submissions.tests.factories import ( SubmissionFactory, SubmissionFileAttachmentFactory, @@ -21,6 +21,7 @@ from ....constants import RegistrationAttribute from ....exceptions import RegistrationFailed from ....service import extract_submission_reference +from ....tasks import register_submission from ..plugin import ZGWRegistration from .factories import ZGWApiGroupConfigFactory @@ -1394,7 +1395,6 @@ def test_zgw_backend_has_reference_after_pre_submission(self, m): @tag("gh-1183") @temp_private_root() -@override_settings(CELERY_TASK_ALWAYS_EAGER=True) class PartialRegistrationFailureTests(TestCase): """ Test that partial results are stored and don't cause excessive registration calls. @@ -1469,9 +1469,8 @@ def test_failure_after_zaak_creation(self): ) with self.subTest("Initial document creation fails"): - on_post_submission_event( - self.submission.id, PostSubmissionEvents.on_completion - ) + pre_registration(self.submission.id) + register_submission(self.submission.id) self.submission.refresh_from_db() assert self.submission.registration_result @@ -1484,11 +1483,12 @@ def test_failure_after_zaak_creation(self): ) self.assertIn("traceback", self.submission.registration_result) + # This flag would be set by the finalise_completion task + self.submission.needs_on_completion_retry = True + self.submission.save() with self.subTest("New document creation attempt does not create zaak again"): with self.assertRaises(RegistrationFailed): - on_post_submission_event( - self.submission.id, PostSubmissionEvents.on_completion - ) + register_submission(self.submission.id) self.submission.refresh_from_db() self.assertEqual( @@ -1636,9 +1636,8 @@ def test_attachment_document_create_fails(self): ) with self.subTest("First try, attachment relation fails"): - on_post_submission_event( - self.submission.id, PostSubmissionEvents.on_completion - ) + pre_registration(self.submission.id) + register_submission(self.submission.id) self.submission.refresh_from_db() assert self.submission.registration_result @@ -1674,11 +1673,12 @@ def test_attachment_document_create_fails(self): self.assertIn("traceback", self.submission.registration_result) + # This flag would be set by the finalise_completion task + self.submission.needs_on_completion_retry = True + self.submission.save() with self.subTest("New attempt - ensure existing data is not created again"): with self.assertRaises(RegistrationFailed): - on_post_submission_event( - self.submission.id, PostSubmissionEvents.on_completion - ) + register_submission(self.submission.id) self.submission.refresh_from_db() self.assertEqual( diff --git a/src/openforms/registrations/error_handler.py b/src/openforms/registrations/error_handler.py new file mode 100644 index 0000000000..485c76cd3b --- /dev/null +++ b/src/openforms/registrations/error_handler.py @@ -0,0 +1,115 @@ +import logging +import traceback + +from celery_once import QueueOnce +from rest_framework.exceptions import ValidationError + +from openforms.logging import logevent +from openforms.registrations.base import BasePlugin +from openforms.registrations.exceptions import ( + RegistrationFailed, + RegistrationRelatedActionFailed, + TooManyRegistrationAttempts, +) +from openforms.submissions.constants import RegistrationStatuses +from openforms.submissions.models import Submission + +logger = logging.getLogger(__name__) + + +class RegistrationInteractionTask(QueueOnce): + def __call__(self, *args, **kwargs): + """Run the celery task but handle error raising""" + try: + super().__call__(*args, **kwargs) + except Exception as exc: + submission = Submission.objects.get(id=args[0]) + plugin = self._get_registration_plugin(submission) + + should_raise = self._handle_exception(submission, plugin, exc) + if should_raise: + raise exc + + def _handle_exception( + self, submission: "Submission", plugin: BasePlugin | None, exc: Exception + ) -> bool: + """Handle the exception based on the task + + Cases: + + 1. Pre-registration: + It should never interrupt the chain. In case the pre-registration fails, it will continue to the registration + task. This then check if pre-registration was completed and if not, it will raise RegistrationRelatedActionFailed. + + 2. Registration: + It interrupts the chain if the submission was not completed or not pre-registered, and if an exception occurs + when retrying. + + 3. Update status: + It interrupts the chain if it is retrying. + """ + should_raise = True + if self.name == "openforms.registrations.tasks.pre_registration": + should_raise = False + + logger.exception("ZGW pre-registration raised %s", exc) + submission.save_registration_status( + RegistrationStatuses.failed, + {"traceback": traceback.format_exc()}, + ) + elif ( + self.name == "openforms.registrations.tasks.register_submission" + and isinstance(exc, TooManyRegistrationAttempts) + ): + should_raise = False + + logevent.registration_attempts_limited(submission) + + submission.registration_status = RegistrationStatuses.failed + submission.needs_on_completion_retry = False + submission.registration_result = None + submission.save() + elif self.name == "openforms.registrations.tasks.register_submission": + should_raise = submission.needs_on_completion_retry or isinstance( + exc, (RegistrationRelatedActionFailed, ValidationError) + ) + + logevent.registration_failure(submission, exc, plugin) + if isinstance(exc, RegistrationFailed): + logger.warning( + "Registration using plugin '%r' for submission '%s' failed", + plugin, + submission, + ) + else: + logger.error( + "Registration using plugin '%r' for submission '%s' unexpectedly errored", + plugin, + submission, + exc_info=True, + ) + + submission.save_registration_status( + RegistrationStatuses.failed, + {"traceback": traceback.format_exc()}, + set_retry_flag=should_raise, + ) + + elif self.name == "openforms.payments.tasks.update_submission_payment_status": + should_raise = submission.needs_on_completion_retry + + logger.info("Updating submission payment registration failed", exc_info=exc) + + submission.needs_on_completion_retry = True + submission.save(update_fields=["needs_on_completion_retry"]) + + return should_raise + + def _get_registration_plugin(self, submission: "Submission") -> BasePlugin | None: + plugin = None + if registration_backend := submission.registration_backend: + registry = registration_backend._meta.get_field("backend").registry + backend = registration_backend.backend + plugin = registry[backend] + + return plugin diff --git a/src/openforms/registrations/exceptions.py b/src/openforms/registrations/exceptions.py index a2f60eb28a..035b92619d 100644 --- a/src/openforms/registrations/exceptions.py +++ b/src/openforms/registrations/exceptions.py @@ -4,5 +4,13 @@ def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) +class RegistrationRelatedActionFailed(Exception): + pass + + +class TooManyRegistrationAttempts(Exception): + pass + + class NoSubmissionReference(Exception): pass diff --git a/src/openforms/registrations/tasks.py b/src/openforms/registrations/tasks.py index f72831e4c6..b911f50dc1 100644 --- a/src/openforms/registrations/tasks.py +++ b/src/openforms/registrations/tasks.py @@ -1,26 +1,25 @@ import logging -import traceback from django.db import transaction from django.utils import timezone -from celery_once import QueueOnce from glom import assign from openforms.celery import app +from openforms.config.models import GlobalConfiguration from openforms.logging import logevent from openforms.submissions.constants import RegistrationStatuses from openforms.submissions.models import Submission from openforms.submissions.public_references import set_submission_reference -from ..config.models import GlobalConfiguration -from .exceptions import RegistrationFailed +from .error_handler import RegistrationInteractionTask +from .exceptions import RegistrationRelatedActionFailed, TooManyRegistrationAttempts from .service import get_registration_plugin logger = logging.getLogger(__name__) -@app.task() +@app.task(base=RegistrationInteractionTask) def pre_registration(submission_id: int) -> None: submission = Submission.objects.get(id=submission_id) if submission.pre_registration_completed: @@ -60,19 +59,15 @@ def pre_registration(submission_id: int) -> None: submission, options_serializer.validated_data ) except Exception as exc: - logger.exception("ZGW pre-registration raised %s", exc) - submission.save_registration_status( - RegistrationStatuses.failed, {"traceback": str(exc)} - ) set_submission_reference(submission) - return + raise exc submission.pre_registration_completed = True submission.save() @app.task( - base=QueueOnce, + base=RegistrationInteractionTask, ignore_result=False, once={"graceful": True}, # do not spam error monitoring ) @@ -97,7 +92,6 @@ def register_submission(submission_id: int) -> None: submission = Submission.objects.select_related("auth_info", "form").get( id=submission_id ) - is_retrying = submission.needs_on_completion_retry logger.debug("Register submission '%s'", submission) @@ -116,11 +110,7 @@ def register_submission(submission_id: int) -> None: config = GlobalConfiguration.get_solo() if submission.registration_attempts >= config.registration_attempt_limit: # if it fails after this many attempts we give up - submission.registration_status = RegistrationStatuses.failed - submission.registration_result = None - submission.save() - logevent.registration_attempts_limited(submission) - return + raise TooManyRegistrationAttempts() else: submission.registration_attempts += 1 submission.save(update_fields=["registration_attempts"]) @@ -128,16 +118,12 @@ def register_submission(submission_id: int) -> None: logevent.registration_start(submission) if not submission.completed_on: - e = RegistrationFailed("Submission should be completed first") - logevent.registration_failure(submission, e) - raise e + raise RegistrationRelatedActionFailed("Submission should be completed first") if not submission.pre_registration_completed: - e = RegistrationFailed( + raise RegistrationRelatedActionFailed( "Pre-registration of the submission should be completed first" ) - logevent.registration_failure(submission, e) - raise e submission.last_register_date = timezone.now() submission.registration_status = RegistrationStatuses.in_progress @@ -161,69 +147,19 @@ def register_submission(submission_id: int) -> None: if not plugin.is_enabled: logger.debug("Plugin '%s' is not enabled", backend) - e = RegistrationFailed("Registration plugin is not enabled") - submission.save_registration_status( - RegistrationStatuses.failed, {"traceback": str(e)}, set_retry_flag=True - ) - logevent.registration_failure(submission, e) - raise e + raise RegistrationRelatedActionFailed("Registration plugin is not enabled") logger.debug("De-serializing the plugin configuration options") options_serializer = plugin.configuration_options(data=backend_config.options) - try: - options_serializer.is_valid(raise_exception=True) - except Exception as e: - submission.save_registration_status( - RegistrationStatuses.failed, - {"traceback": traceback.format_exc()}, - set_retry_flag=True, - ) - logevent.registration_failure(submission, e, plugin) - raise + options_serializer.is_valid(raise_exception=True) logger.debug("Invoking the '%r' plugin callback", plugin) - try: - result = plugin.register_submission( - submission, options_serializer.validated_data - ) - # downstream tasks can still execute, so we return rather than failing. - except RegistrationFailed as e: - logger.warning( - "Registration using plugin '%r' for submission '%s' failed", - plugin, - submission, - ) - submission.save_registration_status( - RegistrationStatuses.failed, {"traceback": traceback.format_exc()} - ) - logevent.registration_failure(submission, e, plugin) - # if we're inside the retry workflow, continued failures should abort the entire - # chain so downstream tasks don't run with incorrect/outdated/missing data - if is_retrying: - raise - return - # unexpected exceptions should fail the entire chain and show up in error monitoring - except Exception as e: - logger.error( - "Registration using plugin '%r' for submission '%s' unexpectedly errored", - plugin, - submission, - exc_info=True, - ) - submission.save_registration_status( - RegistrationStatuses.failed, {"traceback": traceback.format_exc()} - ) - logevent.registration_failure(submission, e, plugin) - # if we're inside the retry workflow, continued failures should abort the entire - # chain so downstream tasks don't run with incorrect/outdated/missing data - if is_retrying: - raise - return - else: - logger.info( - "Registration using plugin '%r' for submission '%s' succeeded", - plugin, - submission, - ) - submission.save_registration_status(RegistrationStatuses.success, result) - logevent.registration_success(submission, plugin) + result = plugin.register_submission(submission, options_serializer.validated_data) + logger.info( + "Registration using plugin '%r' for submission '%s' succeeded", + plugin, + submission, + ) + + submission.save_registration_status(RegistrationStatuses.success, result) + logevent.registration_success(submission, plugin) diff --git a/src/openforms/registrations/tests/test_pre_registration.py b/src/openforms/registrations/tests/test_pre_registration.py index a9babc91f3..45a92e6fb3 100644 --- a/src/openforms/registrations/tests/test_pre_registration.py +++ b/src/openforms/registrations/tests/test_pre_registration.py @@ -5,7 +5,7 @@ from openforms.registrations.contrib.zgw_apis.tests.factories import ( ZGWApiGroupConfigFactory, ) -from openforms.registrations.exceptions import RegistrationFailed +from openforms.registrations.exceptions import RegistrationRelatedActionFailed from openforms.registrations.tasks import register_submission from openforms.submissions.tasks.registration import pre_registration from openforms.submissions.tests.factories import SubmissionFactory @@ -96,7 +96,7 @@ def test_pre_registration_task_errors_but_does_not_raise_error(self): submission.public_registration_reference, "OF-test-registration-failure" ) self.assertFalse(submission.pre_registration_completed) - self.assertEqual(submission.registration_result["traceback"], "I FAILED :(") + self.assertIn("I FAILED :(", submission.registration_result["traceback"]) def test_plugins_generating_reference_before_during_pre_registration(self): plugin_data = [ @@ -150,7 +150,7 @@ def test_if_pre_registration_fails_registration_task_raises_error(self): pre_registration(submission.id) # Fails because of invalid options - with self.assertRaises(RegistrationFailed): + with self.assertRaises(RegistrationRelatedActionFailed): register_submission(submission.id) def test_retry_doesnt_overwrite_internal_reference(self): diff --git a/src/openforms/registrations/tests/test_registration_hook.py b/src/openforms/registrations/tests/test_registration_hook.py index f597c5eaf8..20dd7761ec 100644 --- a/src/openforms/registrations/tests/test_registration_hook.py +++ b/src/openforms/registrations/tests/test_registration_hook.py @@ -4,7 +4,7 @@ from datetime import timedelta from unittest.mock import patch -from django.test import TestCase, override_settings +from django.test import TestCase from django.utils import timezone from freezegun import freeze_time @@ -15,12 +15,11 @@ from openforms.config.models import GlobalConfiguration from openforms.forms.models import FormRegistrationBackend from openforms.logging.models import TimelineLogProxy -from openforms.submissions.constants import PostSubmissionEvents, RegistrationStatuses -from openforms.submissions.tasks import on_post_submission_event +from openforms.submissions.constants import RegistrationStatuses from openforms.submissions.tests.factories import SubmissionFactory from ..base import BasePlugin -from ..exceptions import RegistrationFailed +from ..exceptions import RegistrationFailed, RegistrationRelatedActionFailed from ..registry import Registry from ..tasks import register_submission from .utils import patch_registry @@ -35,7 +34,6 @@ class ResultSerializer(serializers.Serializer): external_id = serializers.UUIDField() -@override_settings(CELERY_TASK_ALWAYS_EAGER=True) class RegistrationHookTests(TestCase): @classmethod def setUpTestData(cls): @@ -116,19 +114,15 @@ def get_reference_from_result(self, result: dict) -> None: with patch_registry(model_field, register), self.assertLogs( level="WARNING" ) as log: - on_post_submission_event( - self.submission.id, PostSubmissionEvents.on_completion - ) + register_submission(self.submission.id) - # check we log a WARNING without stack - error_log = log.records[-2] - self.assertEqual(error_log.funcName, "register_submission") + # check we log an WARNING without stack + error_log = log.records[-1] self.assertEqual(error_log.levelname, "WARNING") self.assertIn("failed", error_log.message) self.assertIsNone(error_log.exc_info) self.submission.refresh_from_db() - self.assertTrue(self.submission.needs_on_completion_retry) self.assertEqual( self.submission.registration_status, RegistrationStatuses.failed ) @@ -158,9 +152,7 @@ def get_reference_from_result(self, result: dict) -> None: with patch_registry(model_field, register), self.assertLogs( level="ERROR" ) as log: - on_post_submission_event( - self.submission.id, PostSubmissionEvents.on_completion - ) + register_submission(self.submission.id) # check we log an ERROR with stack error_log = log.records[-1] @@ -169,7 +161,6 @@ def get_reference_from_result(self, result: dict) -> None: self.assertIsNotNone(error_log.exc_info) self.submission.refresh_from_db() - self.assertTrue(self.submission.needs_on_completion_retry) self.assertEqual( self.submission.registration_status, RegistrationStatuses.failed ) @@ -244,7 +235,7 @@ def test_submission_marked_complete_when_form_has_no_registration_backend(self): def test_submission_is_not_completed_yet(self): submission = SubmissionFactory.create(completed=False) - with self.assertRaises(RegistrationFailed): + with self.assertRaises(RegistrationRelatedActionFailed): register_submission(submission.id) @patch("openforms.plugins.registry.GlobalConfiguration.get_solo") @@ -271,13 +262,10 @@ def get_reference_from_result(self, result: dict) -> None: # # call the hook for the submission, while patching the model field registry model_field = FormRegistrationBackend._meta.get_field("backend") with patch_registry(model_field, register): - with self.assertRaises(RegistrationFailed): - on_post_submission_event( - self.submission.id, PostSubmissionEvents.on_completion - ) + with self.assertRaises(RegistrationRelatedActionFailed): + register_submission(self.submission.id) self.submission.refresh_from_db() - self.assertTrue(self.submission.needs_on_completion_retry) self.assertEqual( self.submission.registration_status, RegistrationStatuses.failed ) @@ -286,7 +274,6 @@ def get_reference_from_result(self, result: dict) -> None: self.assertEqual(self.submission.last_register_date, timezone.now()) -@override_settings(CELERY_TASK_ALWAYS_EAGER=True) class NumRegistrationsTest(TestCase): @patch("openforms.plugins.registry.GlobalConfiguration.get_solo") def test_limit_registration_attempts(self, mock_get_solo): @@ -319,36 +306,40 @@ def get_reference_from_result(self, result: dict) -> None: model_field = FormRegistrationBackend._meta.get_field("backend") with patch_registry(model_field, register): # first registration won't re-raise RegistrationFailed - on_post_submission_event(submission.id, PostSubmissionEvents.on_completion) + register_submission(submission.id) submission.refresh_from_db() self.assertEqual( submission.registration_status, RegistrationStatuses.failed ) self.assertEqual(submission.registration_attempts, 1) - # marked for retry - self.assertTrue(submission.needs_on_completion_retry) + + # This flag would be set by the finalise_completion task + submission.needs_on_completion_retry = True + submission.save() # second and next attempts will re-raise RegistrationFailed for Celery retry for i in range(2, TEST_NUM_ATTEMPTS + 1): + # marked for retry + self.assertTrue(submission.needs_on_completion_retry) + with self.assertRaises(RegistrationFailed): - on_post_submission_event( - submission.id, PostSubmissionEvents.on_retry - ) + register_submission(submission.id) submission.refresh_from_db() self.assertEqual( submission.registration_status, RegistrationStatuses.failed ) self.assertEqual(submission.registration_attempts, i) - # marked for retry - self.assertTrue(submission.needs_on_completion_retry) + + # The last failure marked it as "not to retry" because the max number of attempts has been reached. + self.assertFalse(submission.needs_on_completion_retry) # check self.assertEqual(submission.registration_attempts, TEST_NUM_ATTEMPTS) - # now make more attempts then limit - on_post_submission_event(submission.id, PostSubmissionEvents.on_retry) + # now make more attempts than the limit + register_submission(submission.id) submission.refresh_from_db() self.assertEqual( @@ -359,7 +350,5 @@ def get_reference_from_result(self, result: dict) -> None: self.assertFalse(submission.needs_on_completion_retry) # added log - log = TimelineLogProxy.objects.filter( - template="logging/events/registration_attempts_limited.txt" - ) - self.assertTrue(log.exists()) + log = TimelineLogProxy.objects.last() + self.assertEqual(log.event, "registration_attempts_limited") diff --git a/src/openforms/submissions/migrations/0004_auto_20231124_1625.py b/src/openforms/submissions/migrations/0004_auto_20231128_1536.py similarity index 89% rename from src/openforms/submissions/migrations/0004_auto_20231124_1625.py rename to src/openforms/submissions/migrations/0004_auto_20231128_1536.py index f39e6ee646..8efa7a054f 100644 --- a/src/openforms/submissions/migrations/0004_auto_20231124_1625.py +++ b/src/openforms/submissions/migrations/0004_auto_20231128_1536.py @@ -1,4 +1,4 @@ -# Generated by Django 3.2.23 on 2023-11-24 15:25 +# Generated by Django 3.2.23 on 2023-11-28 14:36 from django.db import migrations, models import django.db.models.deletion @@ -69,7 +69,7 @@ class Migration(migrations.Migration): models.DateTimeField(auto_now_add=True, verbose_name="created on"), ), ( - "created_by", + "trigger_event", models.CharField( choices=[ ("on_completion", "On completion"), @@ -77,7 +77,7 @@ class Migration(migrations.Migration): ("on_cosign_complete", "On cosign complete"), ("on_retry", "On retry"), ], - help_text="Which event scheduled these tasks", + help_text="Which event scheduled these tasks.", max_length=100, verbose_name="created by", ), @@ -97,4 +97,12 @@ class Migration(migrations.Migration): "verbose_name_plural": "post completion metadata", }, ), + migrations.AddConstraint( + model_name="postcompletionmetadata", + constraint=models.UniqueConstraint( + condition=models.Q(("trigger_event", "on_completion")), + fields=("submission",), + name="unique_on_completion_event", + ), + ), ] diff --git a/src/openforms/submissions/models/post_completion_metadata.py b/src/openforms/submissions/models/post_completion_metadata.py index e79e340401..886c4d86b8 100644 --- a/src/openforms/submissions/models/post_completion_metadata.py +++ b/src/openforms/submissions/models/post_completion_metadata.py @@ -1,4 +1,5 @@ from django.db import models +from django.db.models import Q, UniqueConstraint from django.utils.translation import gettext_lazy as _ from django_better_admin_arrayfield.models.fields import ArrayField @@ -27,9 +28,9 @@ class PostCompletionMetadata(models.Model): ), ) created_on = models.DateTimeField(_("created on"), auto_now_add=True) - created_by = models.CharField( + trigger_event = models.CharField( _("created by"), - help_text=_("Which event scheduled these tasks"), + help_text=_("Which event scheduled these tasks."), choices=PostSubmissionEvents.choices, max_length=100, ) @@ -37,6 +38,13 @@ class PostCompletionMetadata(models.Model): class Meta: verbose_name = _("post completion metadata") verbose_name_plural = _("post completion metadata") + constraints = [ + UniqueConstraint( + fields=["submission"], + condition=Q(trigger_event=PostSubmissionEvents.on_completion), + name="unique_on_completion_event", + ) + ] def __str__(self): return f'{self.submission.public_registration_reference}: {", ".join(self.tasks_ids)}' diff --git a/src/openforms/submissions/models/submission.py b/src/openforms/submissions/models/submission.py index efccdd73b3..84b0bcf437 100644 --- a/src/openforms/submissions/models/submission.py +++ b/src/openforms/submissions/models/submission.py @@ -335,7 +335,7 @@ def refresh_from_db(self, *args, **kwargs): del self._execution_state def save_registration_status( - self, status: str, result: dict, set_retry_flag: bool = False + self, status: RegistrationStatuses, result: dict, set_retry_flag: bool = False ) -> None: # combine the new result with existing data, where the new result overwrites # on key collisions. This allows storing intermediate results in the plugin @@ -351,8 +351,11 @@ def save_registration_status( self.registration_result = full_result update_fields = ["registration_status", "registration_result"] + config = GlobalConfiguration.get_solo() if status == RegistrationStatuses.failed and set_retry_flag: - self.needs_on_completion_retry = True + self.needs_on_completion_retry = ( + self.registration_attempts < config.registration_attempt_limit + ) update_fields += ["needs_on_completion_retry"] self.save(update_fields=update_fields) @@ -846,6 +849,6 @@ def post_completion_task_ids(self) -> list[str]: But to give feedback to the user we only need the first set of triggered tasks. """ result = self.postcompletionmetadata_set.filter( - created_by=PostSubmissionEvents.on_completion + trigger_event=PostSubmissionEvents.on_completion ).first() return result.tasks_ids if result else [] diff --git a/src/openforms/submissions/tasks/__init__.py b/src/openforms/submissions/tasks/__init__.py index 4a3f65dc67..62122e825b 100644 --- a/src/openforms/submissions/tasks/__init__.py +++ b/src/openforms/submissions/tasks/__init__.py @@ -66,12 +66,20 @@ def on_post_submission_event(submission_id: int, event: str) -> None: # obtain all the task IDs so we can check the state later task_ids = async_result.as_list() + # then when they try to submit again we have an integrity error because of the unique constraint! # NOTE - this is "risky" since we're running outside of the transaction (this code # should run in transaction.on_commit)! + if event == PostSubmissionEvents.on_completion: + # Case in which an exception was raised that aborts the chain and the user has to try to resubmit the form. + PostCompletionMetadata.objects.filter( + submission_id=submission_id, + trigger_event=PostSubmissionEvents.on_completion, + ).delete() + PostCompletionMetadata.objects.create( tasks_ids=task_ids, - submission=Submission.objects.get(id=submission_id), - created_by=event, + submission_id=submission_id, + trigger_event=event, ) diff --git a/src/openforms/submissions/tests/factories.py b/src/openforms/submissions/tests/factories.py index 9a842c6907..a18d3e6796 100644 --- a/src/openforms/submissions/tests/factories.py +++ b/src/openforms/submissions/tests/factories.py @@ -62,7 +62,7 @@ class Params: "openforms.submissions.tests.factories.PostCompletionMetadataFactory", factory_related_name="submission", tasks_ids=["some-id"], - created_by=PostSubmissionEvents.on_completion, + trigger_event=PostSubmissionEvents.on_completion, ), ) completed_not_preregistered = factory.Trait( diff --git a/src/openforms/submissions/tests/renderer/test_renderer.py b/src/openforms/submissions/tests/renderer/test_renderer.py index d7494e9a13..f312d1916e 100644 --- a/src/openforms/submissions/tests/renderer/test_renderer.py +++ b/src/openforms/submissions/tests/renderer/test_renderer.py @@ -125,6 +125,12 @@ def test_performance_num_queries(self): """ Assert that the number of queries stays low while rendering a submission. """ + import logging + + logger = logging.getLogger("django.db.backends") + logger.setLevel(logging.DEBUG) + logger.addHandler(logging.StreamHandler()) + self.submission.is_authenticated # Load the auth info (otherwise an extra query is needed) renderer = Renderer( submission=self.submission, mode=RenderModes.pdf, as_html=True diff --git a/src/openforms/submissions/tests/test_admin.py b/src/openforms/submissions/tests/test_admin.py index 58617f91c8..8babfd13fb 100644 --- a/src/openforms/submissions/tests/test_admin.py +++ b/src/openforms/submissions/tests/test_admin.py @@ -113,7 +113,7 @@ def test_viewing_submission_details_in_admin_creates_log(self): @patch("openforms.submissions.admin.on_post_submission_event") def test_retry_processing_submissions_only_resends_failed_submissions( - self, on_completion_retry_mock + self, mock_on_post_submission_event ): failed = SubmissionFactory.create( needs_on_completion_retry=True, @@ -136,13 +136,13 @@ def test_retry_processing_submissions_only_resends_failed_submissions( form.submit() - on_completion_retry_mock.assert_called_once_with( + mock_on_post_submission_event.assert_called_once_with( failed.id, PostSubmissionEvents.on_retry ) @patch("openforms.submissions.admin.on_post_submission_event") def test_retry_processing_submissions_resends_all_failed_submissions( - self, on_completion_retry_mock + self, mock_on_post_submission_event ): failed = SubmissionFactory.create( needs_on_completion_retry=False, @@ -180,18 +180,18 @@ def test_retry_processing_submissions_resends_all_failed_submissions( form.submit() - on_completion_retry_mock.assert_any_call( + mock_on_post_submission_event.assert_any_call( failed.id, PostSubmissionEvents.on_retry ) - on_completion_retry_mock.assert_any_call( + mock_on_post_submission_event.assert_any_call( failed_needs_retry.id, PostSubmissionEvents.on_retry ) - self.assertEqual(on_completion_retry_mock.call_count, 2) + self.assertEqual(mock_on_post_submission_event.call_count, 2) @patch("openforms.submissions.admin.on_post_submission_event") @patch("openforms.registrations.tasks.GlobalConfiguration.get_solo") def test_retry_processing_submissions_resets_submission_registration_attempts( - self, mock_get_solo, on_completion_retry_mock + self, mock_get_solo, mock_on_post_submission_event ): mock_get_solo.return_value = GlobalConfiguration(registration_attempt_limit=5) @@ -219,13 +219,13 @@ def test_retry_processing_submissions_resets_submission_registration_attempts( form.submit() - on_completion_retry_mock.assert_any_call( + mock_on_post_submission_event.assert_any_call( failed_above_limit.id, PostSubmissionEvents.on_retry ) - on_completion_retry_mock.assert_any_call( + mock_on_post_submission_event.assert_any_call( failed_below_limit.id, PostSubmissionEvents.on_retry ) - self.assertEqual(on_completion_retry_mock.call_count, 2) + self.assertEqual(mock_on_post_submission_event.call_count, 2) failed_above_limit.refresh_from_db() failed_below_limit.refresh_from_db() diff --git a/src/openforms/submissions/tests/test_on_completion_retry_chain.py b/src/openforms/submissions/tests/test_on_completion_retry_chain.py index f66fce74f5..90c2bb43d3 100644 --- a/src/openforms/submissions/tests/test_on_completion_retry_chain.py +++ b/src/openforms/submissions/tests/test_on_completion_retry_chain.py @@ -11,7 +11,10 @@ from openforms.registrations.contrib.zgw_apis.tests.factories import ( ZGWApiGroupConfigFactory, ) -from openforms.registrations.exceptions import RegistrationFailed +from openforms.registrations.exceptions import ( + RegistrationFailed, + RegistrationRelatedActionFailed, +) from ..constants import PostSubmissionEvents, RegistrationStatuses from ..tasks import on_post_submission_event, retry_processing_submissions @@ -200,7 +203,7 @@ def test_backend_preregistration_still_fails(self, mock_update_payment): with ( preregistration_patcher as mock_preregister, registration_patcher as mock_register, - self.assertRaises(RegistrationFailed), + self.assertRaises(RegistrationRelatedActionFailed), ): on_post_submission_event(submission.id, PostSubmissionEvents.on_retry) diff --git a/src/openforms/submissions/tests/test_submission_status.py b/src/openforms/submissions/tests/test_submission_status.py index 3a9c584521..b1716e266d 100644 --- a/src/openforms/submissions/tests/test_submission_status.py +++ b/src/openforms/submissions/tests/test_submission_status.py @@ -39,7 +39,7 @@ def test_valid_token(self): submission = SubmissionFactory.create( completed=True, metadata__tasks_ids=[], - metadata__created_by=PostSubmissionEvents.on_completion, + metadata__trigger_event=PostSubmissionEvents.on_completion, ) token = submission_status_token_generator.make_token(submission) check_status_url = reverse( @@ -105,7 +105,7 @@ def test_no_task_id_registered(self): submission = SubmissionFactory.create( completed=True, metadata__tasks_ids=[], - metadata__created_by=PostSubmissionEvents.on_completion, + metadata__trigger_event=PostSubmissionEvents.on_completion, ) token = submission_status_token_generator.make_token(submission) check_status_url = reverse( @@ -237,7 +237,7 @@ def test_succesful_processing(self): form__submission_confirmation_template="You get a cookie!", public_registration_reference="OF-ABCDE", metadata__tasks_ids=["some-id"], - metadata__created_by=PostSubmissionEvents.on_completion, + metadata__trigger_event=PostSubmissionEvents.on_completion, ) SubmissionReportFactory.create(submission=submission) token = submission_status_token_generator.make_token(submission) @@ -300,7 +300,7 @@ def test_displaying_main_website_link_returns_main_link_through_api( completed=True, form__display_main_website_link=True, metadata__tasks_ids=["some-id"], - metadata__created_by=PostSubmissionEvents.on_completion, + metadata__trigger_event=PostSubmissionEvents.on_completion, ) token = submission_status_token_generator.make_token(submission) @@ -486,17 +486,17 @@ def test_cleanup_for_submission_with_multiple_post_completion_events( PostCompletionMetadataFactory.create( submission=submission, tasks_ids=["some-id-1", "some-id-2"], - created_by=PostSubmissionEvents.on_completion, + trigger_event=PostSubmissionEvents.on_completion, ) PostCompletionMetadataFactory.create( submission=submission, tasks_ids=["some-id-3"], - created_by=PostSubmissionEvents.on_completion, + trigger_event=PostSubmissionEvents.on_cosign_complete, ) PostCompletionMetadataFactory.create( submission=submission, tasks_ids=[], - created_by=PostSubmissionEvents.on_completion, + trigger_event=PostSubmissionEvents.on_payment_complete, ) cleanup_on_completion_results()