Merge pull request #1807 from BLSQ/IA-3677-do-not-return-soft-deleted…

…-instances-in-change-requests

IA-3677 Exclude soft deleted instances from change requests

iaso/api/org_unit_change_requests/views.py

@@ -2,8 +2,9 @@
 from datetime import datetime
 
 import django_filters
+from django.db.models import Prefetch
 from django.http import HttpResponse
-from django.utils import timezone
+
 from rest_framework import filters, viewsets
 from rest_framework.decorators import action
 from rest_framework.exceptions import PermissionDenied, ValidationError
@@ -23,7 +24,7 @@
     OrgUnitChangeRequestWriteSerializer,
 )
 from iaso.api.serializers import AppIdSerializer
-from iaso.models import OrgUnit, OrgUnitChangeRequest
+from iaso.models import OrgUnit, OrgUnitChangeRequest, Instance
 from iaso.utils.models.common import get_creator_name
 
 
@@ -64,23 +65,28 @@ def get_serializer_class(self):
 
     def get_queryset(self):
         org_units = OrgUnit.objects.filter_for_user(self.request.user)
-        org_units_change_requests = OrgUnitChangeRequest.objects.select_related(
-            "created_by",
-            "updated_by",
-            "org_unit__parent",
-            "org_unit__org_unit_type",
-            "new_parent",
-            "old_parent",
-            "new_org_unit_type",
-            "old_org_unit_type",
-        ).prefetch_related(
-            "org_unit__groups",
-            "org_unit__reference_instances__form",
-            "new_groups",
-            "old_groups",
-            "new_reference_instances__form",
-            "old_reference_instances__form",
-            "org_unit__org_unit_type__projects",
+        org_units_change_requests = (
+            OrgUnitChangeRequest.objects.select_related(
+                "created_by",
+                "updated_by",
+                "org_unit__parent",
+                "org_unit__org_unit_type",
+                "new_parent",
+                "old_parent",
+                "new_org_unit_type",
+                "old_org_unit_type",
+            )
+            .prefetch_related(
+                "org_unit__groups",
+                Prefetch("org_unit__reference_instances", queryset=Instance.non_deleted_objects.select_related("form")),
+                "org_unit__reference_instances__form",
+                "new_groups",
+                "old_groups",
+                Prefetch("new_reference_instances", queryset=Instance.non_deleted_objects.select_related("form")),
+                Prefetch("old_reference_instances", queryset=Instance.non_deleted_objects.select_related("form")),
+                "org_unit__org_unit_type__projects",
+            )
+            .exclude_soft_deleted_new_reference_instances()
         )
         return org_units_change_requests.filter(org_unit__in=org_units)
 

iaso/api/org_unit_change_requests/views_mobile.py

@@ -4,12 +4,14 @@
 from rest_framework import viewsets
 from rest_framework.mixins import ListModelMixin
 
+from django.db.models import Count, Prefetch, Q
+
 from iaso.api.org_unit_change_requests.filters import MobileOrgUnitChangeRequestListFilter
 from iaso.api.org_unit_change_requests.pagination import OrgUnitChangeRequestPagination
 from iaso.api.org_unit_change_requests.permissions import HasOrgUnitsChangeRequestPermission
 from iaso.api.org_unit_change_requests.serializers import MobileOrgUnitChangeRequestListSerializer
 from iaso.api.serializers import AppIdSerializer
-from iaso.models import OrgUnit, OrgUnitChangeRequest
+from iaso.models import Instance, OrgUnit, OrgUnitChangeRequest
 
 
 class MobileOrgUnitChangeRequestViewSet(ListModelMixin, viewsets.GenericViewSet):
@@ -30,6 +32,12 @@ def get_queryset(self):
             .select_related("org_unit")
             .prefetch_related(
                 "new_groups",
-                "new_reference_instances",
+                Prefetch("new_reference_instances", queryset=Instance.non_deleted_objects.all()),
+            )
+            .annotate(
+                annotated_new_reference_instances_count=Count(
+                    "new_reference_instances", filter=Q(new_reference_instances__deleted=False)
+                )
             )
+            .exclude_soft_deleted_new_reference_instances()
         )

iaso/models/org_unit.py

@@ -11,7 +11,7 @@
 from django.contrib.postgres.indexes import GinIndex, GistIndex
 from django.core.exceptions import ValidationError
 from django.db import models, transaction
-from django.db.models import Q, QuerySet
+from django.db.models import Count, Q, QuerySet
 from django.db.models.expressions import RawSQL
 from django.utils.translation import gettext_lazy as _
 from django_ltree.fields import PathField  # type: ignore
@@ -619,6 +619,21 @@ class Meta:
         unique_together = ("org_unit", "form")
 
 
+class OrgUnitChangeRequestQuerySet(models.QuerySet):
+    def exclude_soft_deleted_new_reference_instances(self):
+        """
+        Exclude change requests when `new_reference_instances` is the only
+        requested change but instances have all been soft-deleted.
+        """
+        return self.annotate(
+            annotated_non_deleted_new_reference_instances_count=Count(
+                "new_reference_instances", filter=Q(new_reference_instances__deleted=False)
+            )
+        ).exclude(
+            Q(requested_fields=["new_reference_instances"]) & Q(annotated_non_deleted_new_reference_instances_count=0)
+        )
+
+
 class OrgUnitChangeRequest(models.Model):
     """
     A request to change an OrgUnit.
@@ -704,6 +719,8 @@ class Statuses(models.TextChoices):
         "PotentialPayment", on_delete=models.SET_NULL, null=True, blank=True, related_name="change_requests"
     )
 
+    objects = models.Manager.from_queryset(OrgUnitChangeRequestQuerySet)()
+
     class Meta:
         verbose_name = _("Org unit change request")
         indexes = [

iaso/tests/api/org_unit_change_requests/test_org_unit_change_requests.py

@@ -6,10 +6,7 @@
 from iaso.utils.models.common import get_creator_name
 import time_machine
 
-from django.contrib.auth.models import Group
-
 from iaso.test import APITestCase
-from django.contrib.gis.geos import Point
 from iaso import models as m
 
 
@@ -59,12 +56,16 @@ def setUpTestData(cls):
         org_unit_type.projects.set([project])
         user.iaso_profile.org_units.set([org_unit])
 
+        cls.form_3 = form_3
+        cls.instance_1 = instance_1
+        cls.instance_2 = instance_2
+        cls.instance_3 = instance_3
         cls.org_unit = org_unit
+        cls.org_unit_change_request_csv_columns = OrgUnitChangeRequestViewSet.org_unit_change_request_csv_columns()
         cls.org_unit_type = org_unit_type
         cls.project = project
         cls.user = user
         cls.user_with_review_perm = user_with_review_perm
-        cls.org_unit_change_request_csv_columns = OrgUnitChangeRequestViewSet.org_unit_change_request_csv_columns()
         cls.version = version
 
     def test_list_ok(self):
@@ -73,22 +74,20 @@ def test_list_ok(self):
 
         self.client.force_authenticate(self.user)
 
-        with self.assertNumQueries(12):
+        with self.assertNumQueries(10):
             # filter_for_user_and_app_id
             #   1. SELECT OrgUnit
             # get_queryset
             #   2. COUNT(*)
             #   3. SELECT OrgUnitChangeRequest
             # prefetch
             #   4. PREFETCH OrgUnit.groups
-            #   5. PREFETCH OrgUnit.reference_instances
-            #   6. PREFETCH OrgUnit.reference_instances__form
-            #   7. PREFETCH OrgUnitChangeRequest.new_groups
-            #   8. PREFETCH OrgUnitChangeRequest.old_groups
-            #   9. PREFETCH OrgUnitChangeRequest.new_reference_instances
-            #  10. PREFETCH OrgUnitChangeRequest.old_reference_instances
-            #  11. PREFETCH OrgUnitChangeRequest.{new/old}_reference_instances__form
-            #  12. PREFETCH OrgUnitChangeRequest.org_unit_type.projects
+            #   5. PREFETCH OrgUnit.reference_instances__form
+            #   6. PREFETCH OrgUnitChangeRequest.new_groups
+            #   7. PREFETCH OrgUnitChangeRequest.old_groups
+            #   8. PREFETCH OrgUnitChangeRequest.new_reference_instances__form
+            #   9. PREFETCH OrgUnitChangeRequest.old_reference_instances__form
+            #  10. PREFETCH OrgUnitChangeRequest.org_unit_type.projects
             response = self.client.get("/api/orgunits/changes/")
             self.assertJSONResponse(response, 200)
             self.assertEqual(2, len(response.data["results"]))
@@ -100,11 +99,48 @@ def test_list_without_auth(self):
     def test_retrieve_ok(self):
         change_request = m.OrgUnitChangeRequest.objects.create(org_unit=self.org_unit, new_name="Foo")
         self.client.force_authenticate(self.user)
-        with self.assertNumQueries(11):
+        with self.assertNumQueries(9):
             response = self.client.get(f"/api/orgunits/changes/{change_request.pk}/")
         self.assertJSONResponse(response, 200)
         self.assertEqual(response.data["id"], change_request.pk)
 
+    def test_retrieve_should_not_include_soft_deleted_intances(self):
+        change_request = m.OrgUnitChangeRequest.objects.create(org_unit=self.org_unit, new_name="Foo")
+        change_request.new_reference_instances.set([self.instance_1.pk])
+        change_request.old_reference_instances.set([self.instance_2.pk])
+
+        m.OrgUnitReferenceInstance.objects.filter(org_unit=self.org_unit).delete()
+        m.OrgUnitReferenceInstance.objects.create(org_unit=self.org_unit, form=self.form_3, instance=self.instance_3)
+
+        self.client.force_authenticate(self.user)
+
+        with self.assertNumQueries(9):
+            response = self.client.get(f"/api/orgunits/changes/{change_request.pk}/")
+            self.assertJSONResponse(response, 200)
+            self.assertEqual(response.data["id"], change_request.pk)
+            self.assertEqual(len(response.data["new_reference_instances"]), 1)
+            self.assertEqual(response.data["new_reference_instances"][0]["id"], self.instance_1.pk)
+            self.assertEqual(len(response.data["old_reference_instances"]), 1)
+            self.assertEqual(response.data["old_reference_instances"][0]["id"], self.instance_2.pk)
+            self.assertEqual(len(response.data["org_unit"]["reference_instances"]), 1)
+            self.assertEqual(response.data["org_unit"]["reference_instances"][0]["id"], self.instance_3.pk)
+
+        # Soft delete instances.
+        self.instance_1.deleted = True
+        self.instance_1.save()
+        self.instance_2.deleted = True
+        self.instance_2.save()
+        self.instance_3.deleted = True
+        self.instance_3.save()
+
+        with self.assertNumQueries(9):
+            response = self.client.get(f"/api/orgunits/changes/{change_request.pk}/")
+            self.assertJSONResponse(response, 200)
+            self.assertEqual(response.data["id"], change_request.pk)
+            self.assertEqual(len(response.data["new_reference_instances"]), 0)
+            self.assertEqual(len(response.data["old_reference_instances"]), 0)
+            self.assertEqual(len(response.data["org_unit"]["reference_instances"]), 0)
+
     def test_retrieve_without_auth(self):
         change_request = m.OrgUnitChangeRequest.objects.create(org_unit=self.org_unit, new_name="Foo")
         response = self.client.get(f"/api/orgunits/changes/{change_request.pk}/")

iaso/tests/api/org_unit_change_requests/test_org_unit_change_requests_mobile.py

@@ -17,6 +17,13 @@ def setUpTestData(cls):
         org_unit_type = m.OrgUnitType.objects.create(name="Org unit type")
         org_unit = m.OrgUnit.objects.create(org_unit_type=org_unit_type, version=version)
 
+        form_1 = m.Form.objects.create(name="Form 1")
+        form_2 = m.Form.objects.create(name="Form 2")
+        instance_1 = m.Instance.objects.create(form=form_1, org_unit=org_unit)
+        instance_2 = m.Instance.objects.create(form=form_2, org_unit=org_unit)
+        m.OrgUnitReferenceInstance.objects.create(org_unit=org_unit, form=form_1, instance=instance_1)
+        m.OrgUnitReferenceInstance.objects.create(org_unit=org_unit, form=form_2, instance=instance_2)
+
         user = cls.create_user_with_profile(
             username="user", account=account, permissions=["iaso_org_unit_change_request"]
         )
@@ -28,6 +35,8 @@ def setUpTestData(cls):
         org_unit_type.projects.set([project])
         user.iaso_profile.org_units.set([org_unit])
 
+        cls.instance_1 = instance_1
+        cls.instance_2 = instance_2
         cls.org_unit = org_unit
         cls.project = project
         cls.user = user
@@ -55,6 +64,28 @@ def test_list_ok(self):
             self.assertJSONResponse(response, 200)
             self.assertEqual(2, len(response.data["results"]))
 
+    def test_list_should_not_include_soft_deleted_intances(self):
+        change_request = m.OrgUnitChangeRequest.objects.create(
+            org_unit=self.org_unit, new_name="Foo", created_by=self.user, requested_fields=["new_reference_instances"]
+        )
+        change_request.new_reference_instances.set([self.instance_1.pk])
+
+        self.client.force_authenticate(self.user)
+
+        response = self.client.get(f"/api/mobile/orgunits/changes/?app_id={self.project.app_id}")
+        self.assertJSONResponse(response, 200)
+        self.assertEqual(1, len(response.data["results"]))
+        self.assertEqual(len(response.data["results"][0]["new_reference_instances"]), 1)
+        self.assertEqual(response.data["results"][0]["new_reference_instances"][0]["id"], self.instance_1.pk)
+
+        # Soft delete instance.
+        self.instance_1.deleted = True
+        self.instance_1.save()
+
+        response = self.client.get(f"/api/mobile/orgunits/changes/?app_id={self.project.app_id}")
+        self.assertJSONResponse(response, 200)
+        self.assertEqual(0, len(response.data["results"]))
+
     def test_list_without_auth(self):
         response = self.client.get(f"/api/mobile/orgunits/changes/?app_id={self.project.app_id}")
         self.assertJSONResponse(response, 401)

iaso/tests/models/test_org_unit_change_request.py

@@ -5,11 +5,9 @@
 
 from django.contrib.gis.geos import Point
 from django.core.exceptions import ValidationError
-from django.utils import timezone
 
 from hat.audit.models import Modification
 from iaso import models as m
-from iaso.models.deduplication import ValidationStatus
 from iaso.test import TestCase
 
 
@@ -304,3 +302,20 @@ def test_approve_with_erasing(self):
 
         self.assertEqual(diff["modified"]["closed_date"]["before"], "2055-01-01")
         self.assertIsNone(diff["modified"]["closed_date"]["after"])
+
+    def test_exclude_soft_deleted_new_reference_instances(self):
+        change_request = m.OrgUnitChangeRequest.objects.create(
+            org_unit=self.org_unit, requested_fields=["new_reference_instances"]
+        )
+        change_request.new_reference_instances.set([self.new_instance1, self.new_instance2])
+
+        change_requests = m.OrgUnitChangeRequest.objects.exclude_soft_deleted_new_reference_instances()
+        self.assertEqual(change_requests.count(), 1)
+
+        self.new_instance1.deleted = True
+        self.new_instance1.save()
+        self.new_instance2.deleted = True
+        self.new_instance2.save()
+
+        change_requests = m.OrgUnitChangeRequest.objects.exclude_soft_deleted_new_reference_instances()
+        self.assertEqual(change_requests.count(), 0)