Merge pull request #379 from maykinmedia/fix/378-crash-on-zaak-null

SilviaAmAm · web-flow · commit 2ea379017bb4 · 2024-09-26T11:51:42.000+02:00
[#378] Prevent crash if reviewItem has zaak = null
diff --git a/backend/src/openarchiefbeheer/destruction/api/serializers.py b/backend/src/openarchiefbeheer/destruction/api/serializers.py
@@ -458,8 +458,9 @@ def create(self, validated_data: dict) -> DestructionListReview:
 class DestructionListItemReviewSerializer(serializers.ModelSerializer):
     zaak = serializers.SerializerMethodField(
         help_text=_(
-            "In the case that the zaak has already been deleted, only the URL field will be returned."
-        )
+            "In the case that the zaak has already been deleted, this field will be null."
+        ),
+        allow_null=True,
     )
 
     class Meta:
diff --git a/backend/src/openarchiefbeheer/destruction/tests/e2e/features/test_feature_list_process_review.py b/backend/src/openarchiefbeheer/destruction/tests/e2e/features/test_feature_list_process_review.py
@@ -161,3 +161,38 @@ def create_data():
             await self.then.path_should_be(page, "/destruction-lists/00000000-0000-0000-0000-000000000000")
             
             await self.then.zaaktype_filters_are(page, ["ZAAKTYPE-01 (ZAAKTYPE-01)"])
+        
+    @tag("gh-378")
+    async def test_zaak_removed_outside_process(self):
+        @sync_to_async
+        def create_data():
+            record_manager = UserFactory.create(username="Record Manager", password="ANic3Password", role__can_start_destruction=True)
+
+            zaken = ZaakFactory.create_batch(2)
+            list = DestructionListFactory.create(
+                author=record_manager,
+                assignee=record_manager,
+                status=ListStatus.changes_requested,
+                uuid="00000000-0000-0000-0000-000000000000",
+                name="Destruction list to process",
+            )
+            item1 = DestructionListItemFactory.create(destruction_list=list, zaak=zaken[0])
+            item2 = DestructionListItemFactory.create(destruction_list=list, zaak=zaken[1])
+
+            review = DestructionListReviewFactory.create(destruction_list=list, decision=ReviewDecisionChoices.rejected)
+            DestructionListItemReviewFactory.create(destruction_list=list, destruction_list_item=item1, review=review)
+            DestructionListItemReviewFactory.create(destruction_list=list, destruction_list_item=item2, review=review)
+
+            # Simulate the zaak being deleted by *something else*
+            item1.zaak.delete()
+
+        async with browser_page() as page:
+            await self.given.data_exists(create_data)
+            await self.when.record_manager_logs_in(page)
+            await self.then.path_should_be(page, "/destruction-lists")
+
+            await self.when.user_clicks_button(page, "Destruction list to process")
+            
+            await self.then.path_should_be(page, "/destruction-lists/00000000-0000-0000-0000-000000000000")
+            await self.then.page_should_contain_text(page, "Opnieuw indienen")
+            await self.then.this_number_of_zaken_should_be_visible(page, 1)
diff --git a/backend/src/openarchiefbeheer/destruction/tests/e2e/features/test_feature_list_review.py b/backend/src/openarchiefbeheer/destruction/tests/e2e/features/test_feature_list_review.py
@@ -270,3 +270,39 @@ def create_data():
             await self.then.path_should_be(page, "/destruction-lists/00000000-0000-0000-0000-000000000000/review")
             
             await self.then.zaaktype_filters_are(page, ["ZAAKTYPE-01 (ZAAKTYPE-01)", "ZAAKTYPE-02 (ZAAKTYPE-02)"])
+
+    @tag("gh-378")
+    async def test_zaak_removed_outside_process(self):
+        @sync_to_async
+        def create_data():
+            record_manager = UserFactory.create(role__can_start_destruction=True)
+            reviewer = UserFactory.create(username="Beoordelaar", password="ANic3Password", role__can_review_destruction=True)
+
+            zaken = ZaakFactory.create_batch(2)
+            list = DestructionListFactory.create(
+                author=record_manager,
+                assignee=reviewer,
+                status=ListStatus.ready_to_review,
+                uuid="00000000-0000-0000-0000-000000000000",
+                name="Destruction list to review",
+            )
+            item1 = DestructionListItemFactory.create(destruction_list=list, zaak=zaken[0])
+            item2 = DestructionListItemFactory.create(destruction_list=list, zaak=zaken[1])
+
+            review = DestructionListReviewFactory.create(destruction_list=list, decision=ReviewDecisionChoices.rejected)
+            DestructionListItemReviewFactory.create(destruction_list=list, destruction_list_item=item1, review=review)
+            DestructionListItemReviewFactory.create(destruction_list=list, destruction_list_item=item2, review=review)
+
+            # Simulate the zaak being deleted by *something else*
+            item1.zaak.delete()
+
+        async with browser_page() as page:
+            await self.given.data_exists(create_data)
+            await self.when.reviewer_logs_in(page)
+            await self.then.path_should_be(page, "/destruction-lists")
+
+            await self.when.user_clicks_button(page, "Destruction list to review")
+
+            await self.then.path_should_be(page, "/destruction-lists/00000000-0000-0000-0000-000000000000/review")
+            await self.then.page_should_contain_text(page, "Accorderen")
+            await self.then.this_number_of_zaken_should_be_visible(page, 1)
diff --git a/backend/src/openarchiefbeheer/zaken/admin.py b/backend/src/openarchiefbeheer/zaken/admin.py
@@ -9,6 +9,7 @@
 
 @admin.register(Zaak)
 class ZaakAdmin(admin.ModelAdmin):
+    search_fields = ("identificatie", "uuid")
 
     def get_urls(self):
         urls = super().get_urls()
diff --git a/frontend/src/fixtures/reviewItem.ts b/frontend/src/fixtures/reviewItem.ts
@@ -1,14 +1,14 @@
-import { ReviewItem } from "../lib/api/review";
+import { ReviewItemWithZaak } from "../lib/api/review";
 import { createArrayFactory, createObjectFactory } from "./factory";
 import { zaakFactory, zakenFactory } from "./zaak";
 
-const FIXTURE_REVIEW_ITEM: ReviewItem = {
+const FIXTURE_REVIEW_ITEM: ReviewItemWithZaak = {
   pk: 1,
   zaak: zaakFactory(),
   feedback: "Deze niet",
 };
 
-const FIXTURE_REVIEW_ITEMS: ReviewItem[] = [
+const FIXTURE_REVIEW_ITEMS: ReviewItemWithZaak[] = [
   FIXTURE_REVIEW_ITEM,
   {
     pk: 2,
@@ -22,7 +22,9 @@ const FIXTURE_REVIEW_ITEMS: ReviewItem[] = [
   },
 ];
 
-const reviewItemFactory = createObjectFactory<ReviewItem>(FIXTURE_REVIEW_ITEM);
-const reviewItemsFactory = createArrayFactory<ReviewItem>(FIXTURE_REVIEW_ITEMS);
+const reviewItemFactory =
+  createObjectFactory<ReviewItemWithZaak>(FIXTURE_REVIEW_ITEM);
+const reviewItemsFactory =
+  createArrayFactory<ReviewItemWithZaak>(FIXTURE_REVIEW_ITEMS);
 
 export { reviewItemFactory, reviewItemsFactory };
diff --git a/frontend/src/lib/api/review.ts b/frontend/src/lib/api/review.ts
@@ -19,6 +19,12 @@ export type ZaakReview = {
 };
 
 export type ReviewItem = {
+  pk: number;
+  zaak: Zaak | null;
+  feedback: string;
+};
+
+export type ReviewItemWithZaak = {
   pk: number;
   zaak: Zaak;
   feedback: string;
diff --git a/frontend/src/pages/destructionlist/detail/DestructionListDetail.loader.ts b/frontend/src/pages/destructionlist/detail/DestructionListDetail.loader.ts
@@ -15,7 +15,7 @@ import {
 import { listSelectielijstKlasseChoices } from "../../../lib/api/private";
 import {
   Review,
-  ReviewItem,
+  ReviewItemWithZaak,
   getLatestReview,
   listReviewItems,
 } from "../../../lib/api/review";
@@ -46,7 +46,7 @@ export interface DestructionListDetailContext {
   user: User;
 
   review: Review | null;
-  reviewItems: ReviewItem[] | null;
+  reviewItems: ReviewItemWithZaak[] | null;
 
   selectieLijstKlasseChoicesMap: Record<string, Option[]> | null;
 }
@@ -83,15 +83,21 @@ export const destructionListDetailLoader = loginRequired(
           })
         : null;
 
+      // #378 - If for some unfortunate reason a zaak has been deleted outside of the process,
+      // item.zaak can be null
+      const reviewItemsWithZaak = reviewItems
+        ? (reviewItems.filter((item) => !!item.zaak) as ReviewItemWithZaak[])
+        : reviewItems;
+
       /**
        * Fetch selectable zaken: empty array if review collected OR all zaken not in another destruction list.
        * FIXME: Accept no/implement real pagination?
        */
       const getDestructionListItems =
         async (): Promise<PaginatedDestructionListItems> =>
-          reviewItems
+          reviewItemsWithZaak
             ? {
-                count: reviewItems.length,
+                count: reviewItemsWithZaak.length,
                 next: null,
                 previous: null,
                 results: [],
@@ -122,13 +128,13 @@ export const destructionListDetailLoader = loginRequired(
        * reviewItems ? await listSelectieLijstKlasseChoices({}) : null,
        */
       const getReviewItems = () =>
-        reviewItems
+        reviewItemsWithZaak
           ? cacheMemo(
               "selectieLijstKlasseChoicesMap",
               async () =>
                 Object.fromEntries(
                   await Promise.all(
-                    reviewItems.map(async (ri) => {
+                    reviewItemsWithZaak.map(async (ri) => {
                       const choices = await listSelectielijstKlasseChoices({
                         zaak: ri.zaak.url,
                       });
@@ -137,12 +143,12 @@ export const destructionListDetailLoader = loginRequired(
                   ),
                 ),
               // @ts-expect-error - Params not used in function but in case key only.
-              reviewItems.map((ri) => ri.pk),
+              reviewItemsWithZaak.map((ri) => ri.pk),
             )
           : null;
 
       const getSelectableZaken = () =>
-        reviewItems || destructionList.status === "ready_to_delete"
+        reviewItemsWithZaak || destructionList.status === "ready_to_delete"
           ? ({
               count: 0,
               next: null,
@@ -196,7 +202,7 @@ export const destructionListDetailLoader = loginRequired(
         user,
 
         review: review,
-        reviewItems: reviewItems,
+        reviewItems: reviewItemsWithZaak,
 
         selectieLijstKlasseChoicesMap,
       };
diff --git a/frontend/src/pages/destructionlist/review/DestructionListReview.loader.ts b/frontend/src/pages/destructionlist/review/DestructionListReview.loader.ts
@@ -8,7 +8,7 @@ import {
 } from "../../../lib/api/destructionLists";
 import {
   Review,
-  ReviewItem,
+  ReviewItemWithZaak,
   getLatestReview,
   listReviewItems,
 } from "../../../lib/api/review";
@@ -37,7 +37,7 @@ export type DestructionListReviewContext = {
 
   paginatedZaken: PaginatedZaken;
   review: Review;
-  reviewItems?: ReviewItem[];
+  reviewItems?: ReviewItemWithZaak[];
   reviewResponse?: ReviewResponse;
   reviewers: User[];
 
@@ -94,8 +94,15 @@ export const destructionListReviewLoader = loginRequired(
         storageKey,
       );
 
-      const zakenOnPage = reviewItems?.length
-        ? reviewItems.map((ri) => ri.zaak.url as string)
+      // #378 - If for some unfortunate reason a zaak has been deleted outside of the process,
+      // item.zaak can be null
+      // TODO refactor: This code is the same as for the DestructionListDetail loader.
+      const reviewItemsWithZaak = reviewItems
+        ? (reviewItems.filter((item) => !!item.zaak) as ReviewItemWithZaak[])
+        : reviewItems;
+
+      const zakenOnPage = reviewItemsWithZaak?.length
+        ? reviewItemsWithZaak.map((ri) => ri.zaak.url as string)
         : zaken.results.map((z) => z.url as string);
 
       const approvedZaakUrlsOnPagePromise = await Promise.all(
@@ -123,7 +130,7 @@ export const destructionListReviewLoader = loginRequired(
 
         paginatedZaken: zaken,
         review: latestReview,
-        reviewItems,
+        reviewItems: reviewItemsWithZaak,
         reviewResponse,
         reviewers,
 
