From 91d821fb2a16373eaa15a704641f37c7eb0a4344 Mon Sep 17 00:00:00 2001 From: WinnyTroy Date: Wed, 12 Feb 2020 11:53:11 +0300 Subject: [PATCH] Include deleted_by field to the attachments models Make changes as requested from revies Update failing tests --- .../tests/viewsets/test_attachment_viewset.py | 65 ------------------- .../migrations/0059_attachment_deleted_by.py | 21 ++++++ onadata/apps/logger/models/attachment.py | 17 +++-- onadata/apps/logger/models/instance.py | 2 +- onadata/libs/utils/logger_tools.py | 15 ++--- 5 files changed, 41 insertions(+), 79 deletions(-) create mode 100644 onadata/apps/logger/migrations/0059_attachment_deleted_by.py diff --git a/onadata/apps/api/tests/viewsets/test_attachment_viewset.py b/onadata/apps/api/tests/viewsets/test_attachment_viewset.py index 60fa2c8ffa..40a0f18097 100644 --- a/onadata/apps/api/tests/viewsets/test_attachment_viewset.py +++ b/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() diff --git a/onadata/apps/logger/migrations/0059_attachment_deleted_by.py b/onadata/apps/logger/migrations/0059_attachment_deleted_by.py new file mode 100644 index 0000000000..4a0cd2ac86 --- /dev/null +++ b/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), + ), + ] diff --git a/onadata/apps/logger/models/attachment.py b/onadata/apps/logger/models/attachment.py index 7e9fb3fda4..b024ec1413 100644 --- a/onadata/apps/logger/models/attachment.py +++ b/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) diff --git a/onadata/apps/logger/models/instance.py b/onadata/apps/logger/models/instance.py index c4a07e81b7..a4a7689b97 100644 --- a/onadata/apps/logger/models/instance.py +++ b/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 diff --git a/onadata/libs/utils/logger_tools.py b/onadata/libs/utils/logger_tools.py index 2c3c11a1ba..fb2ec35ecd 100644 --- a/onadata/libs/utils/logger_tools.py +++ b/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()