Exclude deleted Entities from form's manifest data (#2648)

kelvin-muchiri · web-flow · commit 98064d140ee0 · 2024-07-25T14:31:23.000+03:00
* exclude deleted EntityList in endpoint /api/v1/projects

* delete form metadata on deleting EntityList

* ignore deleted Entity for EntityList dataset export

* index deleted_at field for model EntityList, Entity

* update dataset info when hard deleting Entity

* delete form metadata when EntityList is hard deleted

* add test

* resolve cyclic dep

* resolve cyclic dep

* delete EntityList in an atomic transaction

* fix failing test

* remove unnecessary .all()
diff --git a/onadata/apps/api/tests/viewsets/test_entity_list_viewset.py b/onadata/apps/api/tests/viewsets/test_entity_list_viewset.py
@@ -1119,12 +1119,15 @@ def test_delete(self, mock_now):
         request = self.factory.delete("/", **self.extra)
         response = self.view(request, pk=self.entity_list.pk, entity_pk=self.entity.pk)
         self.entity.refresh_from_db()
+        self.entity_list.refresh_from_db()
+
         self.assertEqual(response.status_code, 204)
         self.assertEqual(self.entity.deleted_at, date)
         self.assertEqual(self.entity.deleted_by, self.user)
-        self.entity_list.refresh_from_db()
         self.assertEqual(self.entity_list.num_entities, 0)
-        self.assertEqual(self.entity_list.last_entity_update_time, date)
+        self.assertEqual(
+            self.entity_list.last_entity_update_time, self.entity.date_modified
+        )
 
     def test_invalid_entity(self):
         """Invalid Entity is handled"""
diff --git a/onadata/apps/api/tests/viewsets/test_project_viewset.py b/onadata/apps/api/tests/viewsets/test_project_viewset.py
@@ -2796,6 +2796,7 @@ def test_get_project_w_registration_form(self):
         request = self.factory.get("/", **self.extra)
         response = view(request, pk=self.project.pk)
         entity_list = EntityList.objects.first()
+
         self.assertEqual(response.status_code, 200)
         self.assertEqual(
             response.data["forms"][0]["contributes_entities_to"],
@@ -2805,6 +2806,13 @@ def test_get_project_w_registration_form(self):
                 "is_active": True,
             },
         )
+        # Soft delete dataset
+        entity_list.soft_delete()
+        request = self.factory.get("/", **self.extra)
+        response = view(request, pk=self.project.pk)
+
+        self.assertEqual(response.status_code, 200)
+        self.assertIsNone(response.data["forms"][0]["contributes_entities_to"])
 
     def test_get_project_w_follow_up_form(self):
         """Retrieve project with Entity follow up form"""
@@ -2825,6 +2833,13 @@ def test_get_project_w_follow_up_form(self):
                 }
             ],
         )
+        # Soft delete dataset
+        entity_list.soft_delete()
+        request = self.factory.get("/", **self.extra)
+        response = view(request, pk=self.project.pk)
+
+        self.assertEqual(response.status_code, 200)
+        self.assertEqual(response.data["forms"][0]["consumes_entities_from"], [])
 
 
 class GetProjectInvitationListTestCase(TestAbstractViewSet):
diff --git a/onadata/apps/logger/migrations/0022_entity_logger_enti_deleted_66eee5_idx_and_more.py b/onadata/apps/logger/migrations/0022_entity_logger_enti_deleted_66eee5_idx_and_more.py
@@ -0,0 +1,45 @@
+# Generated by Django 4.2.13 on 2024-07-23 07:38
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+    atomic = False
+    dependencies = [
+        ("logger", "0021_alter_attachment_options_alter_xform_options"),
+    ]
+
+    operations = [
+        migrations.SeparateDatabaseAndState(
+            database_operations=[
+                migrations.RunSQL(
+                    sql=(
+                        'CREATE INDEX CONCURRENTLY "logger_enti_deleted_66eee5_idx" '
+                        'ON "logger_entity" ("deleted_at");'
+                    ),
+                    reverse_sql='DROP INDEX CONCURRENTLY "logger_enti_deleted_66eee5_idx";',
+                ),
+                migrations.RunSQL(
+                    sql=(
+                        'CREATE INDEX CONCURRENTLY "logger_enti_deleted_476ed6_idx" '
+                        'ON "logger_entitylist" ("deleted_at");'
+                    ),
+                    reverse_sql='DROP INDEX CONCURRENTLY "logger_enti_deleted_476ed6_idx";',
+                ),
+            ],
+            state_operations=[
+                migrations.AddIndex(
+                    model_name="entity",
+                    index=models.Index(
+                        fields=["deleted_at"], name="logger_enti_deleted_66eee5_idx"
+                    ),
+                ),
+                migrations.AddIndex(
+                    model_name="entitylist",
+                    index=models.Index(
+                        fields=["deleted_at"], name="logger_enti_deleted_476ed6_idx"
+                    ),
+                ),
+            ],
+        )
+    ]
diff --git a/onadata/apps/logger/models/entity.py b/onadata/apps/logger/models/entity.py
@@ -39,11 +39,11 @@ def soft_delete(self, deleted_by=None):
             self.deleted_by = deleted_by
             self.save(update_fields=["deleted_at", "deleted_by"])
             self.entity_list.num_entities = models.F("num_entities") - 1
-            self.entity_list.last_entity_update_time = deletion_time
             self.entity_list.save()
 
     class Meta(BaseModel.Meta):
         app_label = "logger"
+        indexes = [models.Index(fields=["deleted_at"])]
 
 
 class EntityHistory(BaseModel):
diff --git a/onadata/apps/logger/models/entity_list.py b/onadata/apps/logger/models/entity_list.py
@@ -5,15 +5,19 @@
 from django.contrib.auth import get_user_model
 from django.contrib.auth.models import Group, Permission
 from django.contrib.contenttypes.fields import GenericRelation
-from django.db import models
+from django.db import models, transaction
 from django.utils.translation import gettext_lazy as _
 from django.utils import timezone
 
+
 from guardian.models import UserObjectPermissionBase, GroupObjectPermissionBase
 from guardian.compat import user_model_label
 
 from onadata.apps.logger.models.project import Project
+from onadata.apps.logger.models.xform import clear_project_cache
+from onadata.apps.main.models.meta_data import MetaData
 from onadata.libs.models import BaseModel
+from onadata.libs.utils.model_tools import queryset_iterator
 
 User = get_user_model()
 
@@ -39,13 +43,6 @@ class EntityList(BaseModel):
     deleted_at = models.DateTimeField(null=True, blank=True)
     deleted_by = models.ForeignKey(User, null=True, on_delete=models.SET_NULL)
 
-    class Meta(BaseModel.Meta):
-        app_label = "logger"
-        unique_together = (
-            "name",
-            "project",
-        )
-
     def __str__(self):
         return f"{self.name}|{self.project}"
 
@@ -69,16 +66,35 @@ def properties(self) -> list[str]:
 
         return list(dataset_properties)
 
+    @transaction.atomic()
     def soft_delete(self, deleted_by=None):
         """Soft delete EntityList"""
         if self.deleted_at is None:
             deletion_time = timezone.now()
             deletion_suffix = deletion_time.strftime("-deleted-at-%s")
             self.deleted_at = deletion_time
             self.deleted_by = deleted_by
+            original_name = self.name
             self.name += deletion_suffix
             self.name = self.name[:255]  # Only first 255 characters
             self.save()
+            clear_project_cache(self.project.pk)
+            # Soft deleted follow up forms MetaData
+            metadata_qs = MetaData.objects.filter(
+                data_type="media",
+                data_value=f"entity_list {self.pk} {original_name}",
+            )
+
+            for datum in queryset_iterator(metadata_qs):
+                datum.soft_delete()
+
+    class Meta(BaseModel.Meta):
+        app_label = "logger"
+        unique_together = (
+            "name",
+            "project",
+        )
+        indexes = [models.Index(fields=["deleted_at"])]
 
 
 class EntityListUserObjectPermission(UserObjectPermissionBase):
diff --git a/onadata/apps/logger/signals.py b/onadata/apps/logger/signals.py
@@ -4,12 +4,15 @@
 """
 from django.db import transaction
 from django.db.models import F
-from django.db.models.signals import post_save
+from django.db.models.signals import post_save, post_delete
 from django.dispatch import receiver
+from django.utils import timezone
 
 from onadata.apps.logger.models import Entity, EntityList, Instance, RegistrationForm
+from onadata.apps.logger.models.xform import clear_project_cache
 from onadata.apps.logger.xform_instance_parser import get_meta_from_xml
 from onadata.apps.logger.tasks import set_entity_list_perms_async
+from onadata.apps.main.models.meta_data import MetaData
 from onadata.libs.utils.logger_tools import (
     create_entity_from_instance,
     update_entity_from_instance,
@@ -47,23 +50,67 @@ def create_or_update_entity(sender, instance, created=False, **kwargs):
                 create_entity_from_instance(instance, registration_form)
 
 
-@receiver(post_save, sender=Entity, dispatch_uid="update_entity_dataset")
-def update_entity_dataset(sender, instance, created=False, **kwargs):
-    """Update EntityList when Entity is created or updated"""
-    if not instance:
-        return
-
+@receiver(post_save, sender=Entity, dispatch_uid="update_enti_el_inc_num_entities")
+def increment_entity_list_num_entities(sender, instance, created=False, **kwargs):
+    """Increment EntityList `num_entities`"""
     entity_list = instance.entity_list
 
     if created:
-        entity_list.num_entities = F("num_entities") + 1
+        # Using Queryset.update ensures we do not call the model's save method and
+        # signals
+        EntityList.objects.filter(pk=entity_list.pk).update(
+            num_entities=F("num_entities") + 1
+        )
+
+
+@receiver(post_delete, sender=Entity, dispatch_uid="update_enti_el_dec_num_entities")
+def decrement_entity_list_num_entities(sender, instance, **kwargs):
+    """Decrement EntityList `num_entities`"""
+    entity_list = instance.entity_list
+    # Using Queryset.update ensures we do not call the model's save method and
+    # signals
+    EntityList.objects.filter(pk=entity_list.pk).update(
+        num_entities=F("num_entities") - 1
+    )
+
 
-    entity_list.last_entity_update_time = instance.date_modified
-    entity_list.save()
+@receiver(post_delete, sender=Entity, dispatch_uid="delete_enti_el_last_update_time")
+def update_last_entity_update_time_now(sender, instance, **kwargs):
+    """Update EntityList `last_entity_update_time`"""
+    entity_list = instance.entity_list
+    # Using Queryset.update ensures we do not call the model's save method and
+    # signals
+    EntityList.objects.filter(pk=entity_list.pk).update(
+        last_entity_update_time=timezone.now()
+    )
+
+
+@receiver(post_save, sender=Entity, dispatch_uid="update_enti_el_last_update_time")
+def update_last_entity_update_time(sender, instance, **kwargs):
+    """Update EntityList `last_entity_update_time`"""
+    entity_list = instance.entity_list
+    # Using Queryset.update ensures we do not call the model's save method and
+    # signals
+    EntityList.objects.filter(pk=entity_list.pk).update(
+        last_entity_update_time=instance.date_modified
+    )
 
 
 @receiver(post_save, sender=EntityList, dispatch_uid="set_entity_list_perms")
 def set_entity_list_perms(sender, instance, created=False, **kwargs):
     """Set project permissions to EntityList"""
     if created:
         transaction.on_commit(lambda: set_entity_list_perms_async.delay(instance.pk))
+
+
+@receiver(post_delete, sender=EntityList, dispatch_uid="delete_entity_list_metadata")
+def delete_entity_list_metadata(sender, instance, **kwargs):
+    """Delete EntityList related data on delete"""
+    clear_project_cache(instance.project.pk)
+    # We get original name incase name has been modified in the case where
+    # EntityList was first soft deleted
+    entity_list_name = instance.name.split("-")[0]
+    MetaData.objects.filter(
+        data_type="media",
+        data_value=f"entity_list {instance.pk} {entity_list_name}",
+    ).delete()
diff --git a/onadata/apps/logger/tests/models/test_entity.py b/onadata/apps/logger/tests/models/test_entity.py
@@ -93,6 +93,21 @@ def test_soft_delete(self, mock_now):
         self.assertEqual(entity3.deleted_at, self.mocked_now)
         self.assertIsNone(entity3.deleted_by)
 
+    def test_hard_delete(self):
+        """Hard deleting updates dataset info"""
+        entity = Entity.objects.create(entity_list=self.entity_list)
+        self.entity_list.refresh_from_db()
+        old_last_entity_update_time = self.entity_list.last_entity_update_time
+
+        self.assertEqual(self.entity_list.num_entities, 1)
+
+        entity.delete()
+        self.entity_list.refresh_from_db()
+        new_last_entity_update_time = self.entity_list.last_entity_update_time
+
+        self.assertEqual(self.entity_list.num_entities, 0)
+        self.assertTrue(old_last_entity_update_time < new_last_entity_update_time)
+
 
 class EntityHistoryTestCase(TestBase):
     """Tests for model EntityHistory"""
diff --git a/onadata/apps/logger/tests/models/test_entity_list.py b/onadata/apps/logger/tests/models/test_entity_list.py
@@ -132,13 +132,19 @@ def test_soft_delete(self):
         with patch("django.utils.timezone.now") as mock_now:
             mock_now.return_value = self.mocked_now
             entity_list = EntityList.objects.create(name="trees", project=self.project)
+            follow_up_form = self._publish_follow_up_form(self.user)
             entity_list.soft_delete(self.user)
             entity_list.refresh_from_db()
+            follow_up_form_meta_datum = follow_up_form.metadata_set.get(
+                data_value=f"entity_list {entity_list.pk} trees"
+            )
+
             self.assertEqual(entity_list.deleted_at, self.mocked_now)
             self.assertEqual(entity_list.deleted_by, self.user)
             self.assertEqual(
                 entity_list.name, f'trees{self.mocked_now.strftime("-deleted-at-%s")}'
             )
+            self.assertIsNotNone(follow_up_form_meta_datum.deleted_at)
 
         # Try soft deleting soft deleted dataset
         entity_list.soft_delete(self.user)
@@ -155,3 +161,33 @@ def test_soft_delete(self):
         entity_list.soft_delete()
         entity_list.refresh_from_db()
         self.assertEqual(entity_list.name, dataset_name)
+
+    def test_hard_delete(self):
+        """Hard delete removes consumers' metadata"""
+        entity_list = EntityList.objects.create(name="trees", project=self.project)
+        follow_up_form = self._publish_follow_up_form(self.user)
+        data_value = f"entity_list {entity_list.pk} trees"
+        self.assertTrue(
+            follow_up_form.metadata_set.filter(data_value=data_value).exists()
+        )
+
+        entity_list.delete()
+
+        self.assertFalse(
+            follow_up_form.metadata_set.filter(data_value=data_value).exists()
+        )
+        # Hard deleted previously soft deleted dataset works
+        follow_up_form.delete()
+        entity_list = EntityList.objects.create(name="trees", project=self.project)
+        follow_up_form = self._publish_follow_up_form(self.user)
+        data_value = f"entity_list {entity_list.pk} trees"
+
+        self.assertTrue(
+            follow_up_form.metadata_set.filter(data_value=data_value).exists()
+        )
+        entity_list.soft_delete()
+        entity_list.delete()
+
+        self.assertFalse(
+            follow_up_form.metadata_set.filter(data_value=data_value).exists()
+        )
diff --git a/onadata/libs/serializers/project_serializer.py b/onadata/libs/serializers/project_serializer.py
@@ -223,7 +223,9 @@ class BaseProjectXFormSerializer(serializers.HyperlinkedModelSerializer):
 
     def get_contributes_entities_to(self, obj: XForm):
         """Return the EntityList that the form contributes Entities to"""
-        registration_form = obj.registration_forms.first()
+        registration_form = obj.registration_forms.filter(
+            entity_list__deleted_at__isnull=True
+        ).first()
 
         if registration_form is None:
             return None
@@ -236,7 +238,7 @@ def get_contributes_entities_to(self, obj: XForm):
 
     def get_consumes_entities_from(self, obj: XForm):
         """Return the EntityLIst that the form consumes Entities"""
-        queryset = obj.follow_up_forms.all()
+        queryset = obj.follow_up_forms.filter(entity_list__deleted_at__isnull=True)
 
         if not queryset:
             return []
diff --git a/onadata/libs/tests/utils/test_export_tools.py b/onadata/libs/tests/utils/test_export_tools.py
diff --git a/onadata/libs/utils/export_tools.py b/onadata/libs/utils/export_tools.py
