From cbcdd0b6d3f2ca1baf26d7b1b6c29381793768ac Mon Sep 17 00:00:00 2001 From: kemar Date: Mon, 18 Nov 2024 17:16:00 +0100 Subject: [PATCH 1/3] Exclude soft deleted instances from change requests --- iaso/api/org_unit_change_requests/views.py | 10 +-- .../org_unit_change_requests/views_mobile.py | 6 +- .../test_org_unit_change_requests.py | 64 +++++++++++++++---- .../test_org_unit_change_requests_mobile.py | 34 ++++++++++ 4 files changed, 94 insertions(+), 20 deletions(-) diff --git a/iaso/api/org_unit_change_requests/views.py b/iaso/api/org_unit_change_requests/views.py index b3d2224f37..88c99b065c 100644 --- a/iaso/api/org_unit_change_requests/views.py +++ b/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 @@ -75,11 +76,12 @@ def get_queryset(self): "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", - "new_reference_instances__form", - "old_reference_instances__form", + 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", ) return org_units_change_requests.filter(org_unit__in=org_units) diff --git a/iaso/api/org_unit_change_requests/views_mobile.py b/iaso/api/org_unit_change_requests/views_mobile.py index 72ab3e80ad..8c95d565a0 100644 --- a/iaso/api/org_unit_change_requests/views_mobile.py +++ b/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 Prefetch + 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,6 @@ 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()), ) ) diff --git a/iaso/tests/api/org_unit_change_requests/test_org_unit_change_requests.py b/iaso/tests/api/org_unit_change_requests/test_org_unit_change_requests.py index 6db2c9ef6a..f02bf55aed 100644 --- a/iaso/tests/api/org_unit_change_requests/test_org_unit_change_requests.py +++ b/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,7 +74,7 @@ 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 @@ -81,14 +82,12 @@ def test_list_ok(self): # 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}/") diff --git a/iaso/tests/api/org_unit_change_requests/test_org_unit_change_requests_mobile.py b/iaso/tests/api/org_unit_change_requests/test_org_unit_change_requests_mobile.py index 06b5939243..1d5c878fd5 100644 --- a/iaso/tests/api/org_unit_change_requests/test_org_unit_change_requests_mobile.py +++ b/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,31 @@ 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 + ) + change_request.new_reference_instances.set([self.instance_1.pk]) + + self.client.force_authenticate(self.user) + + with self.assertNumQueries(8): + 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() + + with self.assertNumQueries(8): + 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"]), 0) + 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) From 70c0cad7e77d43ddec1d784f53fc8430060ccfdf Mon Sep 17 00:00:00 2001 From: kemar Date: Tue, 19 Nov 2024 16:36:49 +0100 Subject: [PATCH 2/3] Exclude change requests with empty `new_reference_instances` --- iaso/api/org_unit_change_requests/views.py | 40 ++++++++++--------- .../org_unit_change_requests/views_mobile.py | 8 +++- iaso/models/org_unit.py | 19 ++++++++- .../test_org_unit_change_requests_mobile.py | 21 +++++----- .../models/test_org_unit_change_request.py | 17 ++++++++ 5 files changed, 73 insertions(+), 32 deletions(-) diff --git a/iaso/api/org_unit_change_requests/views.py b/iaso/api/org_unit_change_requests/views.py index 88c99b065c..5c8bc08065 100644 --- a/iaso/api/org_unit_change_requests/views.py +++ b/iaso/api/org_unit_change_requests/views.py @@ -65,24 +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", - 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", + 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) diff --git a/iaso/api/org_unit_change_requests/views_mobile.py b/iaso/api/org_unit_change_requests/views_mobile.py index 8c95d565a0..a7aa291ce6 100644 --- a/iaso/api/org_unit_change_requests/views_mobile.py +++ b/iaso/api/org_unit_change_requests/views_mobile.py @@ -4,7 +4,7 @@ from rest_framework import viewsets from rest_framework.mixins import ListModelMixin -from django.db.models import Prefetch +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 @@ -34,4 +34,10 @@ def get_queryset(self): "new_groups", 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() ) diff --git a/iaso/models/org_unit.py b/iaso/models/org_unit.py index d5729b98a2..b884122494 100644 --- a/iaso/models/org_unit.py +++ b/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 = [ diff --git a/iaso/tests/api/org_unit_change_requests/test_org_unit_change_requests_mobile.py b/iaso/tests/api/org_unit_change_requests/test_org_unit_change_requests_mobile.py index 1d5c878fd5..121d6a413c 100644 --- a/iaso/tests/api/org_unit_change_requests/test_org_unit_change_requests_mobile.py +++ b/iaso/tests/api/org_unit_change_requests/test_org_unit_change_requests_mobile.py @@ -66,28 +66,25 @@ def test_list_ok(self): 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 + 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) - with self.assertNumQueries(8): - 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) + 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() - with self.assertNumQueries(8): - 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"]), 0) + 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}") diff --git a/iaso/tests/models/test_org_unit_change_request.py b/iaso/tests/models/test_org_unit_change_request.py index c3e8e13dbd..2c1642deae 100644 --- a/iaso/tests/models/test_org_unit_change_request.py +++ b/iaso/tests/models/test_org_unit_change_request.py @@ -304,3 +304,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) From 5f31ba1753aa4ce32cd2921dcec00679092ad6ca Mon Sep 17 00:00:00 2001 From: kemar Date: Tue, 19 Nov 2024 16:43:31 +0100 Subject: [PATCH 3/3] Remove unused imports --- iaso/tests/models/test_org_unit_change_request.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/iaso/tests/models/test_org_unit_change_request.py b/iaso/tests/models/test_org_unit_change_request.py index 2c1642deae..f41b9123ff 100644 --- a/iaso/tests/models/test_org_unit_change_request.py +++ b/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