Skip to content

Commit

Permalink
👌 [#3005] PR Feedback part 3
Browse files Browse the repository at this point in the history
  • Loading branch information
SilviaAmAm committed Nov 30, 2023
1 parent 56ee8d9 commit b411b2f
Show file tree
Hide file tree
Showing 18 changed files with 276 additions and 194 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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))
)
Expand Down Expand Up @@ -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
)

Expand Down
17 changes: 3 additions & 14 deletions src/openforms/payments/tasks.py
Original file line number Diff line number Diff line change
@@ -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

Expand All @@ -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):
Expand All @@ -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)
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
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
from privates.test import temp_private_root

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
Expand Down Expand Up @@ -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,
Expand All @@ -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(
Expand All @@ -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)
Expand Down Expand Up @@ -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,
Expand All @@ -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(
Expand All @@ -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)
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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(
Expand All @@ -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)
Expand Down
30 changes: 15 additions & 15 deletions src/openforms/registrations/contrib/zgw_apis/tests/test_backend.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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

Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand All @@ -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(
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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(
Expand Down
115 changes: 115 additions & 0 deletions src/openforms/registrations/error_handler.py
Original file line number Diff line number Diff line change
@@ -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
8 changes: 8 additions & 0 deletions src/openforms/registrations/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Loading

0 comments on commit b411b2f

Please sign in to comment.