Merge pull request #260 from maykinmedia/feature/229-continue-deleting-if-a-case-fails

[#229] Continue deleting if a case fails

Author: SilviaAmAm

backend/src/openarchiefbeheer/destruction/exceptions.py

@@ -0,0 +1,2 @@
+class DeletionProcessingError(Exception):
+    pass

backend/src/openarchiefbeheer/destruction/models.py

@@ -180,6 +180,14 @@ def has_short_review_process(self) -> bool:
             [zaaktype in config.zaaktypes_short_process for zaaktype in zaaktypes_urls]
         )
 
+    def has_failures(self) -> bool:
+        return any(
+            [
+                status == InternalStatus.failed
+                for status in self.items.values_list("processing_status", flat=True)
+            ]
+        )
+
 
 class DestructionListItem(models.Model):
     destruction_list = models.ForeignKey(
@@ -254,27 +262,31 @@ def set_processing_status(self, status: InternalStatus) -> None:
         self.processing_status = status
         self.save()
 
-    def process_deletion(self) -> None:
-        from .utils import mark_as_failed_on_error
+    def _delete_zaak(self):
+        try:
+            zaak = Zaak.objects.get(url=self.zaak)
+        except ObjectDoesNotExist as exc:
+            logger.error(
+                "Could not find zaak with URL %s. Aborting deletion.", self.zaak
+            )
+            raise exc
 
-        self.processing_status = InternalStatus.processing
-        self.save()
+        store = ResultStore(store=self)
+        store.clear_traceback()
 
-        with mark_as_failed_on_error(self):
-            try:
-                zaak = Zaak.objects.get(url=self.zaak)
-            except ObjectDoesNotExist as exc:
-                logger.error(
-                    "Could not find zaak with URL %s. Aborting deletion.", self.zaak
-                )
-                raise exc
+        delete_zaak_and_related_objects(zaak=zaak, result_store=store)
 
-            store = ResultStore(store=self)
-            store.clear_traceback()
+        zaak.delete()
 
-            delete_zaak_and_related_objects(zaak=zaak, result_store=store)
+    def process_deletion(self) -> None:
+        self.processing_status = InternalStatus.processing
+        self.save()
 
-            zaak.delete()
+        try:
+            self._delete_zaak()
+        except Exception:
+            self.set_processing_status(InternalStatus.failed)
+            return
 
         self.processing_status = InternalStatus.succeeded
         self.save()

backend/src/openarchiefbeheer/destruction/tasks.py

@@ -7,6 +7,7 @@
 from openarchiefbeheer.celery import app
 
 from .constants import InternalStatus, ListItemStatus, ListStatus
+from .exceptions import DeletionProcessingError
 from .models import DestructionList, DestructionListItem, ReviewResponse
 from .signals import deletion_failure
 from .utils import notify_assignees_successful_deletion
@@ -66,11 +67,11 @@ def delete_destruction_list(destruction_list: DestructionList) -> None:
     chunk_tasks = delete_destruction_list_item.chunks(
         items_pks, settings.ZAKEN_CHUNK_SIZE
     )
-    notify_task = complete_and_notify.si(destruction_list.pk)
+    complete_and_notify_task = complete_and_notify.si(destruction_list.pk)
 
     task_chain = chain(
         chunk_tasks.group(),
-        notify_task,
+        complete_and_notify_task,
         link_error=handle_processing_error.si(destruction_list.pk),
     )
     task_chain.delay()
@@ -99,6 +100,9 @@ def delete_destruction_list_item(pk: int) -> None:
 @app.task
 def complete_and_notify(pk: int) -> None:
     destruction_list = DestructionList.objects.get(pk=pk)
+    if destruction_list.has_failures():
+        raise DeletionProcessingError()
+
     destruction_list.processing_status = InternalStatus.succeeded
     destruction_list.save()
 

backend/src/openarchiefbeheer/destruction/tests/test_models.py

@@ -84,8 +84,7 @@ def test_process_deletion_zaak_not_found(self):
             zaak="http://zaken.nl/api/v1/zaken/111-111-111",
         )
 
-        with self.assertRaises(ObjectDoesNotExist):
-            item.process_deletion()
+        item.process_deletion()
 
         item.refresh_from_db()
 

backend/src/openarchiefbeheer/destruction/tests/test_signals.py

@@ -55,7 +55,6 @@ def test_failure_during_deletion_sends_signal(self):
                     body_error_during_deletion="ERROR AAAh!",
                 ),
             ),
-            self.assertRaises(Exception),
         ):
             delete_destruction_list(destruction_list)
 

backend/src/openarchiefbeheer/destruction/tests/test_tasks.py

@@ -241,53 +241,6 @@ def test_process_list(self):
             ).exists()
         )
 
-    @override_settings(CELERY_TASK_ALWAYS_EAGER=True)
-    def test_process_item_raises_error(self):
-        destruction_list = DestructionListFactory.create(
-            status=ListStatus.ready_to_delete
-        )
-        ZaakFactory.create(
-            url="http://zaken.nl/api/v1/zaken/111-111-111",
-        )
-        item1 = DestructionListItemFactory.create(
-            zaak="http://zaken.nl/api/v1/zaken/111-111-111",
-            destruction_list=destruction_list,
-        )
-        ZaakFactory.create(
-            url="http://zaken.nl/api/v1/zaken/222-222-222",
-        )
-        item2 = DestructionListItemFactory.create(
-            zaak="http://zaken.nl/api/v1/zaken/222-222-222",
-            destruction_list=destruction_list,
-        )
-
-        with (
-            patch(
-                "openarchiefbeheer.destruction.models.delete_zaak_and_related_objects",
-                side_effect=Exception,
-            ),
-            self.assertRaises(Exception),
-        ):
-            delete_destruction_list(destruction_list)
-
-        destruction_list.refresh_from_db()
-        item1.refresh_from_db()
-        item2.refresh_from_db()
-
-        self.assertEqual(destruction_list.processing_status, InternalStatus.failed)
-        self.assertEqual(destruction_list.status, ListStatus.ready_to_delete)
-
-        # We don't know which item is processed first
-        statuses = [item1.processing_status, item2.processing_status]
-        self.assertIn(InternalStatus.failed, statuses)
-        self.assertIn(InternalStatus.new, statuses)
-        self.assertTrue(
-            Zaak.objects.filter(url="http://zaken.nl/api/v1/zaken/111-111-111").exists()
-        )
-        self.assertTrue(
-            Zaak.objects.filter(url="http://zaken.nl/api/v1/zaken/222-222-222").exists()
-        )
-
     @log_capture(level=logging.INFO)
     def test_item_skipped_if_already_succeeded(self, logs):
         item = DestructionListItemFactory.create(
@@ -369,3 +322,46 @@ def test_complete_and_notify(self):
 
         self.assertEqual(list.status, ListStatus.deleted)
         self.assertEqual(list.processing_status, InternalStatus.succeeded)
+
+    @override_settings(CELERY_TASK_ALWAYS_EAGER=True)
+    def test_other_items_processed_if_one_fails(self):
+        destruction_list = DestructionListFactory.create(
+            status=ListStatus.ready_to_delete
+        )
+        ZaakFactory.create(
+            url="http://zaken.nl/api/v1/zaken/111-111-111",
+        )
+        item1 = DestructionListItemFactory.create(
+            zaak="http://zaken.nl/api/v1/zaken/111-111-111",
+            destruction_list=destruction_list,
+        )
+        ZaakFactory.create(
+            url="http://zaken.nl/api/v1/zaken/222-222-222",
+        )
+        item2 = DestructionListItemFactory.create(
+            zaak="http://zaken.nl/api/v1/zaken/222-222-222",
+            destruction_list=destruction_list,
+        )
+
+        def mock_exceptions(zaak, result_store):
+            if zaak.url == item1.zaak:
+                raise Exception("An errur occurred!")
+
+        with (
+            patch(
+                "openarchiefbeheer.destruction.models.delete_zaak_and_related_objects",
+                side_effect=mock_exceptions,
+            ),
+        ):
+            delete_destruction_list(destruction_list)
+
+        destruction_list.refresh_from_db()
+        item1.refresh_from_db()
+        item2.refresh_from_db()
+
+        self.assertEqual(destruction_list.processing_status, InternalStatus.failed)
+        self.assertEqual(destruction_list.status, ListStatus.ready_to_delete)
+        self.assertEqual(item1.processing_status, InternalStatus.failed)
+        self.assertEqual(item2.processing_status, InternalStatus.succeeded)
+        self.assertTrue(Zaak.objects.filter(url=item1.zaak).exists())
+        self.assertFalse(Zaak.objects.filter(url=item2.zaak).exists())

backend/src/openarchiefbeheer/destruction/utils.py

@@ -1,4 +1,3 @@
-from contextlib import contextmanager
 from typing import Protocol
 
 from django.conf import settings
@@ -118,15 +117,6 @@ class ObjectWithStatus(Protocol):
     def set_processing_status(self, status: InternalStatus) -> None: ...
 
 
-@contextmanager
-def mark_as_failed_on_error(object_with_status: ObjectWithStatus):
-    try:
-        yield
-    except Exception as exc:
-        object_with_status.set_processing_status(InternalStatus.failed)
-        raise exc
-
-
 def process_new_assignees(
     destruction_list: DestructionList,
     assignees: list[dict],