♻️ - refactor: refactor DestructionListWriteSerializer items by add and adjust frontend

Author: svenvandescheur

backend/src/openarchiefbeheer/destruction/api/filtersets.py

@@ -1,7 +1,13 @@
 from django.db.models import Case, QuerySet, Value, When
 from django.utils.translation import gettext_lazy as _
 
-from django_filters import FilterSet, NumberFilter, OrderingFilter, UUIDFilter
+from django_filters import (
+    BooleanFilter,
+    FilterSet,
+    NumberFilter,
+    OrderingFilter,
+    UUIDFilter,
+)
 
 from ..constants import InternalStatus
 from ..models import (
@@ -23,13 +29,34 @@ class DestructionListItemFilterset(FilterSet):
         ),
     )
 
+    # FIXME: Due to the implementation of the dit page, the zake endpoint and the destruction list items endpoint
+    # MUST return values in EXACTLY THE SAME order, untill we fix this properly, we use a special param
+    # "item-order_match_zaken" to instruct the filter to order on the "zaak_pk" which is the (default) ordering of the
+    # zaken endpoint.
+    order_match_zaken = BooleanFilter(
+        field_name="ordering",
+        method="filter_order_match_zaken",
+        help_text=_(
+            "If edit mode is active, ordering should match ordering on the zaken endpoint, use the item-ordering param "
+            "to define the exact same ordering as the zaak endpoint"
+        ),
+    )
+
     class Meta:
         model = DestructionListItem
-        fields = ("destruction_list", "status", "processing_status")
+        fields = (
+            "destruction_list",
+            "status",
+            "processing_status",
+            "order_match_zaken",
+        )
 
     def filter_in_destruction_list(
         self, queryset: QuerySet[DestructionListItem], name: str, value: str
     ) -> QuerySet[DestructionListItem]:
+        """
+        Fixme: see filter field.
+        """
         return (
             queryset.filter(destruction_list__uuid=value)
             .annotate(
@@ -45,6 +72,11 @@ def filter_in_destruction_list(
             .order_by("processing_status_index")
         )
 
+    def filter_order_match_zaken(
+        self, queryset: QuerySet[DestructionListItem], name: str, value: str
+    ) -> QuerySet[DestructionListItem]:
+        return queryset.order_by("zaak__pk")
+
 
 class DestructionListFilterset(FilterSet):
     assignee = NumberFilter(

backend/src/openarchiefbeheer/destruction/api/serializers.py

@@ -151,7 +151,6 @@ class Meta:
 class DestructionListWriteSerializer(serializers.ModelSerializer):
     add = DestructionListItemWriteSerializer(many=True, required=False)
     remove = DestructionListItemWriteSerializer(many=True, required=False)
-    items = DestructionListItemWriteSerializer(many=True, required=False)
     reviewer = ReviewerAssigneeSerializer(required=False)
     author = UserSerializer(read_only=True)
     select_all = serializers.BooleanField(
@@ -172,7 +171,6 @@ class Meta:
         fields = (
             "add",
             "remove",
-            "items",
             "uuid",
             "name",
             "author",
@@ -204,9 +202,15 @@ def validate_zaak_filters(self, value: Any) -> dict:
         return value
 
     def validate(self, attrs: dict) -> dict:
-        if not self.instance and not attrs.get("items") and not attrs.get("select_all"):
+        if (attrs.get("add") or attrs.get("remove")) and attrs.get("select_all"):
             raise ValidationError(
-                "Neither the 'items' nor the 'select_all' field have been specified.",
+                "'add' or 'remove' cannot be combined with 'select_all'",
+                code="invalid",
+            )
+
+        if not self.instance and not attrs.get("add") and not attrs.get("select_all"):
+            raise ValidationError(
+                "Neither the 'add' nor the 'select_all' field have been specified.",
                 code="invalid",
             )
 
@@ -234,7 +238,7 @@ def _get_zaken(
 
     def create(self, validated_data: dict) -> DestructionList:
         reviewer_data = validated_data.pop("reviewer")
-        items = validated_data.pop("items", None)
+        add = validated_data.pop("add", [])
         bulk_select = validated_data.pop("select_all", False)
         zaak_filters = validated_data.pop("zaak_filters", {})
 
@@ -244,7 +248,7 @@ def create(self, validated_data: dict) -> DestructionList:
         validated_data["status"] = ListStatus.new
         destruction_list = DestructionList.objects.create(**validated_data)
 
-        zaken = self._get_zaken(zaak_filters, items, bulk_select)
+        zaken = self._get_zaken(zaak_filters, add, bulk_select)
         destruction_list.add_items(zaken)
 
         # Create an assignee also for the author
@@ -269,7 +273,6 @@ def update(
         validated_data.pop("reviewer", None)
         add_data = validated_data.pop("add", [])
         remove_data = validated_data.pop("remove", [])
-        items_data = validated_data.pop("items", [])
         instance.contains_sensitive_info = validated_data.pop(
             "contains_sensitive_info", instance.contains_sensitive_info
         )
@@ -278,19 +281,12 @@ def update(
 
         instance.name = validated_data.pop("name", instance.name)
 
-        if items_data or bulk_select:
-            instance.items.all().delete()
-
-            zaken = self._get_zaken(zaak_filters, items_data, bulk_select)
-
-            instance.add_items(zaken)
-
-        if add_data:
+        if add_data or bulk_select:
             zaken = self._get_zaken(zaak_filters, add_data, bulk_select)
-            self.instance.add_items(zaken)
+            self.instance.add_items(zaken, True)
 
         if remove_data:
-            zaken = self._get_zaken(zaak_filters, remove_data or [], bulk_select)
+            zaken = [item["zaak"] for item in remove_data]
             self.instance.remove_items(zaken)
 
         instance.save()

backend/src/openarchiefbeheer/destruction/api/viewsets.py

@@ -88,7 +88,7 @@
                             "user": 2,
                         },
                     ],
-                    "items": [
+                    "add": [
                         {
                             "zaak": "http://some-zaken-api.nl/zaken/api/v1/zaken/111-111-111",
                             "extraZaakData": {},
@@ -131,24 +131,6 @@
                     ],
                 },
             ),
-            OpenApiExample(
-                name="Replace items",
-                request_only=True,
-                value={
-                    "name": "An example updated list",
-                    "containsSensitiveInfo": False,
-                    "items": [
-                        {
-                            "zaak": "http://some-zaken-api.nl/zaken/api/v1/zaken/111-111-111",
-                            "extraZaakData": {},
-                        },
-                        {
-                            "zaak": "http://some-zaken-api.nl/zaken/api/v1/zaken/222-222-222",
-                            "extraZaakData": {},
-                        },
-                    ],
-                },
-            ),
         ],
     ),
     partial_update=extend_schema(

backend/src/openarchiefbeheer/destruction/models.py

@@ -147,14 +147,17 @@ def set_status(self, status: str) -> None:
         self.status_changed = timezone.now()
         self.save()
 
-    def add_items(self, zaken: Iterable["Zaak"]) -> list["DestructionListItem"]:
+    def add_items(
+        self, zaken: Iterable["Zaak"], ignore_conflicts: bool = False
+    ) -> list["DestructionListItem"]:
         return DestructionListItem.objects.bulk_create(
             [
                 DestructionListItem(
                     destruction_list=self, zaak=zaak, _zaak_url=zaak.url
                 )
                 for zaak in zaken
-            ]
+            ],
+            ignore_conflicts=ignore_conflicts,
         )
 
     def remove_items(self, zaken: Iterable["Zaak"]) -> tuple[int, dict[str, int]]:

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

@@ -79,7 +79,7 @@ def test_create_destruction_list(self):
                 "name": "A test list",
                 "contains_sensitive_info": True,
                 "reviewer": {"user": reviewer.pk},
-                "items": [
+                "add": [
                     {
                         "zaak": "http://localhost:8003/zaken/api/v1/zaken/111-111-111",
                         "extra_zaak_data": {},
@@ -161,7 +161,7 @@ def test_zaak_already_in_another_destruction_list(self):
                 "assignees": [
                     {"user": reviewer.pk},
                 ],
-                "items": [
+                "add": [
                     {
                         "zaak": "http://localhost:8003/zaken/api/v1/zaken/111-111-111",
                         "extra_zaak_data": {},
@@ -182,7 +182,7 @@ def test_zaak_already_in_another_destruction_list(self):
         self.assertEqual(
             data,
             {
-                "items": [
+                "add": [
                     {
                         "zaak": [
                             _(
@@ -228,7 +228,7 @@ def test_update_destruction_list(self):
         data = {
             "name": "An updated test list",
             "contains_sensitive_info": False,
-            "items": [
+            "add": [
                 {
                     "zaak": "http://localhost:8003/zaken/api/v1/zaken/111-111-111",
                     "extra_zaak_data": {"key": "value"},
@@ -251,7 +251,7 @@ def test_update_destruction_list(self):
         destruction_list.refresh_from_db()
 
         self.assertEqual(destruction_list.name, "An updated test list")
-        self.assertEqual(destruction_list.items.all().count(), 1)
+        self.assertEqual(destruction_list.items.all().count(), 3)
 
     def test_cannot_update_destruction_list_if_not_new(self):
         record_manager = UserFactory.create(post__can_start_destruction=True)

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

@@ -55,7 +55,7 @@ def test_create_destruction_list(self):
             "name": "A test list",
             "contains_sensitive_info": True,
             "reviewer": {"user": reviewer.pk},
-            "items": [
+            "add": [
                 {
                     "zaak": "http://localhost:8003/zaken/api/v1/zaken/111-111-111",
                     "extra_zaak_data": {},
@@ -139,7 +139,7 @@ def test_zaak_already_included_in_other_list(self):
             "assignees": [
                 {"user": reviewer.pk, "order": 0},
             ],
-            "items": [
+            "add": [
                 {
                     "zaak": "http://localhost:8003/zaken/api/v1/zaken/111-111-111",
                     "extra_zaak_data": {},
@@ -157,7 +157,7 @@ def test_zaak_already_included_in_other_list(self):
 
         self.assertFalse(serializer.is_valid())
         self.assertEqual(
-            serializer.errors["items"][0]["zaak"],
+            serializer.errors["add"][0]["zaak"],
             [
                 _(
                     "This case was already included in another destruction list and was not exempt during the review process."
@@ -189,7 +189,7 @@ def test_zaak_already_included_in_other_list_but_exempt(self):
             "name": "A test list",
             "contains_sensitive_info": True,
             "reviewer": {"user": reviewer.pk},
-            "items": [
+            "add": [
                 {
                     "zaak": "http://localhost:8003/zaken/api/v1/zaken/111-111-111",
                     "extra_zaak_data": {},
@@ -226,7 +226,7 @@ def test_full_list_update(self):
         data = {
             "name": "An updated test list",
             "contains_sensitive_info": False,
-            "items": [
+            "add": [
                 {
                     "zaak": "http://localhost:8003/zaken/api/v1/zaken/111-111-111",
                 },
@@ -249,7 +249,7 @@ def test_full_list_update(self):
 
         items = destruction_list.items.all()
 
-        self.assertEqual(items.count(), 1)
+        self.assertEqual(items.count(), 3)
 
         logs = TimelineLog.objects.filter(
             template="logging/destruction_list_updated.txt"
@@ -401,10 +401,12 @@ def test_partial_update_with_zaken(self):
             status=ListItemStatus.suggested,
             with_zaak=True,
         )
+        zaak = ZaakFactory.create()
 
         # We are removing 2 zaken from the destruction list
         data = {
-            "items": [{"zaak": items[0].zaak.url}, {"zaak": items[1].zaak.url}],
+            "add": [{"zaak": zaak.url}],
+            "remove": [{"zaak": items[0].zaak.url}, {"zaak": items[1].zaak.url}],
         }
 
         record_manager = UserFactory.create(post__can_start_destruction=True)
@@ -417,17 +419,22 @@ def test_partial_update_with_zaken(self):
             partial=True,
             context={"request": request},
         )
-
         self.assertTrue(serializer.is_valid())
 
         serializer.save()
 
-        items = DestructionListItem.objects.filter(destruction_list=destruction_list)
-        items_in_list = items.values_list("zaak__url", flat=True)
+        destruction_list_items = DestructionListItem.objects.filter(
+            destruction_list=destruction_list
+        )
+        items_in_list = destruction_list_items.values_list("zaak__url", flat=True)
 
-        self.assertEqual(items_in_list.count(), 2)
-        self.assertIn(data["items"][0]["zaak"], items_in_list)
-        self.assertIn(data["items"][1]["zaak"], items_in_list)
+        self.assertEqual(items_in_list.count(), 3)
+        self.assertNotIn(items, items_in_list)
+        self.assertIn(items[2].zaak.url, items_in_list)
+        self.assertIn(items[3].zaak.url, items_in_list)
+        self.assertIn(data["add"][0]["zaak"], items_in_list)
+        self.assertNotIn(data["remove"][0]["zaak"], items_in_list)
+        self.assertNotIn(data["remove"][1]["zaak"], items_in_list)
 
     @tag("gh-122")
     def test_assign_author_as_reviewer(self):
@@ -603,7 +610,7 @@ def test_no_bulk_select_and_no_items(self):
         self.assertFalse(is_valid)
         self.assertEqual(
             serializer.errors["non_field_errors"][0],
-            "Neither the 'items' nor the 'select_all' field have been specified.",
+            "Neither the 'add' nor the 'select_all' field have been specified.",
         )
 
     def test_zaak_filters_validation(self):

frontend/src/lib/api/destructionLists.ts

@@ -52,7 +52,6 @@ export type DestructionListUpdateData = {
   assignees?: DestructionListAssigneeUpdate[];
   add?: DestructionListItemUpdate[];
   remove?: DestructionListItemUpdate[];
-  items?: DestructionListItemUpdate[];
   comment?: string;
 };
 
@@ -74,7 +73,7 @@ export type DestructionListMarkAsFinalData = {
  * Create a new destruction list.
  * @param name
  * @param zaken
- * @param assignees
+ * @param assigneeId
  * @param zaakFilters
  * @param allZakenSelected
  */
@@ -90,7 +89,7 @@ export async function createDestructionList(
   const destructionList = {
     name,
     reviewer: { user: JSON.parse(assigneeId) },
-    items: urls.map((url) => ({ zaak: url })),
+    add: urls.map((url) => ({ zaak: url })),
     selectAll: allZakenSelected,
     zaakFilters: JSON.parse(zaakFilters),
   };

frontend/src/pages/destructionlist/abstract/BaseListView.tsx

@@ -119,7 +119,6 @@ export function BaseListView({
     getSelectionDetail,
   );
 
-  // Merge `selectedZakenOnPage`.
   const selectedZakenOnPage = useMemo(() => {
     const deselectedZaakUrls = deSelectedZakenOnPage.map(
       (z) => z.url as string,
@@ -200,7 +199,11 @@ export function BaseListView({
         page,
         sort: sortable && sort,
         selected: selectable
-          ? (selectedZakenOnPage as unknown as AttributeData[])
+          ? ([
+              ...new Map(
+                selectedZakenOnPage.map((zaak) => [zaak["uuid"], zaak]),
+              ).values(),
+            ] as unknown as AttributeData[])
           : [],
         selectionActions: getSelectionActions(),