Include deleted_by field to the attachments models

Make changes as requested from revies
Update failing tests

onadata/apps/api/tests/viewsets/test_attachment_viewset.py

@@ -188,71 +188,6 @@ def test_list_view(self):
         self.assertTrue(isinstance(response.data, list))
         self.assertEqual(len(response.data), 0)
 
-    def test_list_view_surfaces_current_attachment(self):
-        self._submit_transport_instance_w_attachment()
-        filename = "1335783522564.JPG"
-        path = os.path.join(self.main_directory, 'fixtures', 'transportation',
-                            'instances', self.surveys[0], filename)
-        media_file = django_file(path, 'video2', 'image/jpeg')
-
-        Attachment.objects.create(
-            instance=self.xform.instances.first(),
-            mimetype='video/mp4',
-            extension='MP4',
-            name=filename,
-            media_file=media_file)
-
-        Attachment.objects.create(
-            instance=self.xform.instances.first(),
-            mimetype='application/pdf',
-            extension='PDF',
-            name=filename,
-            media_file=media_file)
-        Attachment.objects.create(
-            instance=self.xform.instances.first(),
-            mimetype='text/plain',
-            extension='TXT',
-            name=filename,
-            media_file=media_file)
-        Attachment.objects.create(
-            instance=self.xform.instances.first(),
-            mimetype='audio/mp3',
-            extension='MP3',
-            name=filename,
-            media_file=media_file)
-
-        request = self.factory.get('/', **self.extra)
-        response = self.list_view(request)
-        self.assertNotEqual(response.get('Cache-Control'), None)
-        self.assertEqual(response.status_code, 200)
-        self.assertTrue(isinstance(response.data, list))
-        self.assertEqual(len(response.data), 5)
-
-        # test when the attachment is soft deleted
-        self.attachment.soft_delete(user=self.user)
-
-        request = self.factory.get('/', **self.extra)
-        response = self.list_view(request)
-
-        self.assertEqual(len(response.data), 4)
-
-    def test_soft_delete_action_returns_correct_user(self):
-        self._submit_transport_instance_w_attachment()
-
-        request = self.factory.get('/', **self.extra)
-        response = self.list_view(request)
-        self.assertEqual(len(response.data), 1)
-
-        # test when the attachment is soft deleted
-        self.attachment.soft_delete(user=self.user)
-
-        # Test that deleted_by field captures the right user
-        self.assertTrue(self.attachment.deleted_by, self.user)
-
-        request = self.factory.get('/', **self.extra)
-        response = self.list_view(request)
-        self.assertEqual(len(response.data), 0)
-
     def test_data_list_with_xform_in_delete_async(self):
         self._submit_transport_instance_w_attachment()
 

onadata/apps/logger/migrations/0059_attachment_deleted_by.py

@@ -0,0 +1,21 @@
+# Generated by Django 2.1.7 on 2020-02-12 08:35
+
+from django.conf import settings
+from django.db import migrations, models
+import django.db.models.deletion
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        migrations.swappable_dependency(settings.AUTH_USER_MODEL),
+        ('logger', '0058_auto_20191211_0900'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='attachment',
+            name='deleted_by',
+            field=models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='deleted_attachments', to=settings.AUTH_USER_MODEL),
+        ),
+    ]

onadata/apps/logger/models/attachment.py

@@ -1,9 +1,11 @@
 import mimetypes
 import os
 from hashlib import md5
+from django.db.models import Q
 
 from django.db import models
 from django.utils import timezone
+from django.contrib.auth.models import User
 
 
 def get_original_filename(filename):
@@ -53,6 +55,8 @@ class Attachment(models.Model):
     deleted_at = models.DateTimeField(null=True, default=None)
     file_size = models.PositiveIntegerField(default=0)
     name = models.CharField(max_length=100, null=True, blank=True)
+    deleted_by = models.ForeignKey(User, related_name='deleted_attachments',
+                                   null=True, on_delete=models.SET_NULL)
 
     class Meta:
         app_label = 'logger'
@@ -88,11 +92,14 @@ def filename(self):
         if self.media_file:
             return os.path.basename(self.media_file.name)
 
-    def soft_delete(self, user=None):
+    @classmethod
+    def soft_delete(self, instance, user=None):
         """
         Soft deletes an attachment by adding a deleted_at timestamp.
         """
-        self.deleted_at = timezone.now()
-        if user is not None:
-            self.deleted_by = user
-        self.save()
+        if instance:
+            queryset = self.__class__.objects.filter(name__in=self.get_expexted_media())
+            kwargs = {'deleted_at': timezone.now()}
+            if user:
+                kwargs.update({'deleted_by': user})
+            queryset.update(**kwargs)

onadata/apps/logger/models/instance.py

@@ -79,7 +79,7 @@ def _get_attachments_from_instance(instance):
         attachment['id'] = a.id
         attachments.append(attachment)
 
-        if isinstance(a.deleted_at, datetime):
+        if a.deleted_at:
             attachment['deleted_at'] = a.deleted_at.strftime(MONGO_STRFTIME)
 
     return attachments

onadata/libs/utils/logger_tools.py

@@ -214,13 +214,12 @@ def update_attachment_tracking(instance):
                                  'media_all_received', 'json'])
 
 
-def save_attachments(xform, instance, media_files):
+def save_attachments(xform, instance, media_files, remove_deleted_media=False):
     """
     Saves attachments for the given instance/submission.
     """
     # upload_path = os.path.join(instance.xform.user.username, 'attachments')
 
-    filenames = []
     for f in media_files:
         filename, extension = os.path.splitext(f.name)
         extension = extension.replace('.', '')
@@ -230,7 +229,6 @@ def save_attachments(xform, instance, media_files):
             xform.instances_with_osm = True
             xform.save()
         filename = os.path.basename(f.name)
-        filenames.append(filename)
         media_in_submission = (
             filename in instance.get_expected_media() or
             [instance.xml.decode('utf-8').find(filename) != -1 if
@@ -243,9 +241,10 @@ def save_attachments(xform, instance, media_files):
                 mimetype=content_type,
                 name=filename,
                 extension=extension)
-    Attachment.objects.filter(instance=instance).filter(
-        ~Q(name__in=instance.get_expected_media())).update(
-            deleted_at=timezone.now())
+    if remove_deleted_media:
+        Attachment.soft_delete(
+            instance.attachments.filter(
+                ~Q(name__in=instance.get_expected_media())))
 
     update_attachment_tracking(instance)
 
@@ -320,7 +319,7 @@ def create_instance(username,
             (new_uuid or existing_instance.xform.has_start_time):
         # ensure we have saved the extra attachments
         with transaction.atomic():
-            save_attachments(xform, existing_instance, media_files)
+            save_attachments(xform, existing_instance, media_files, remove_deleted_media=True)
             existing_instance.save(update_fields=['json', 'date_modified'])
 
         # Ignore submission as a duplicate IFF
@@ -339,7 +338,7 @@ def create_instance(username,
         duplicate_instance = history.xform_instance
         # ensure we have saved the extra attachments
         with transaction.atomic():
-            save_attachments(xform, duplicate_instance, media_files)
+            save_attachments(xform, duplicate_instance, media_files, remove_deleted_media=True)
             duplicate_instance.save()
 
         return DuplicateInstance()