From 56e8be3217b92b19d496963390804253d2bbe23e Mon Sep 17 00:00:00 2001 From: WinnyTroy Date: Wed, 9 Oct 2019 11:47:42 +0300 Subject: [PATCH 1/4] Add deleted_at Field to Attachments. Create soft_delete action that will populate deleted_at field for attachments. Fixes #1696 Call soft_delete action when replacing attachments --- .../tests/viewsets/test_attachment_viewset.py | 65 +++++++++++++++++++ .../apps/api/viewsets/attachment_viewset.py | 3 +- onadata/apps/logger/models/attachment.py | 10 +++ onadata/apps/logger/models/instance.py | 5 +- onadata/libs/utils/logger_tools.py | 6 ++ 5 files changed, 87 insertions(+), 2 deletions(-) diff --git a/onadata/apps/api/tests/viewsets/test_attachment_viewset.py b/onadata/apps/api/tests/viewsets/test_attachment_viewset.py index 40a0f18097..60fa2c8ffa 100644 --- a/onadata/apps/api/tests/viewsets/test_attachment_viewset.py +++ b/onadata/apps/api/tests/viewsets/test_attachment_viewset.py @@ -188,6 +188,71 @@ 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/api/viewsets/attachment_viewset.py b/onadata/apps/api/viewsets/attachment_viewset.py index 0d63153ca6..af38671c40 100644 --- a/onadata/apps/api/viewsets/attachment_viewset.py +++ b/onadata/apps/api/viewsets/attachment_viewset.py @@ -47,7 +47,8 @@ class AttachmentViewSet(AuthenticateHeaderMixin, CacheControlMixin, ETagsMixin, content_negotiation_class = MediaFileContentNegotiation filter_backends = (filters.AttachmentFilter, filters.AttachmentTypeFilter) lookup_field = 'pk' - queryset = Attachment.objects.filter(instance__deleted_at__isnull=True) + queryset = Attachment.objects.filter( + instance__deleted_at__isnull=True, deleted_at__isnull=True) permission_classes = (AttachmentObjectPermissions,) serializer_class = AttachmentSerializer pagination_class = StandardPageNumberPagination diff --git a/onadata/apps/logger/models/attachment.py b/onadata/apps/logger/models/attachment.py index 72f6399305..7e9fb3fda4 100644 --- a/onadata/apps/logger/models/attachment.py +++ b/onadata/apps/logger/models/attachment.py @@ -3,6 +3,7 @@ from hashlib import md5 from django.db import models +from django.utils import timezone def get_original_filename(filename): @@ -86,3 +87,12 @@ def file_hash(self): def filename(self): if self.media_file: return os.path.basename(self.media_file.name) + + def soft_delete(self, 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() diff --git a/onadata/apps/logger/models/instance.py b/onadata/apps/logger/models/instance.py index eba834e846..c4a07e81b7 100644 --- a/onadata/apps/logger/models/instance.py +++ b/onadata/apps/logger/models/instance.py @@ -66,7 +66,7 @@ def get_attachment_url(attachment, suffix=None): def _get_attachments_from_instance(instance): attachments = [] - for a in instance.attachments.all(): + for a in instance.attachments.filter(deleted_at__isnull=True): attachment = dict() attachment['download_url'] = get_attachment_url(a) attachment['small_download_url'] = get_attachment_url(a, 'small') @@ -79,6 +79,9 @@ def _get_attachments_from_instance(instance): attachment['id'] = a.id attachments.append(attachment) + if isinstance(a.deleted_at, datetime): + 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 2c3b1dcc6d..2c3c11a1ba 100644 --- a/onadata/libs/utils/logger_tools.py +++ b/onadata/libs/utils/logger_tools.py @@ -220,6 +220,7 @@ def save_attachments(xform, instance, media_files): """ # 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('.', '') @@ -229,6 +230,7 @@ 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 @@ -241,6 +243,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()) + update_attachment_tracking(instance) From f98d2c75bec630274cbf8b49874fb19021e95d82 Mon Sep 17 00:00:00 2001 From: WinnyTroy Date: Wed, 12 Feb 2020 11:53:11 +0300 Subject: [PATCH 2/4] 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 | 16 +++-- onadata/apps/logger/models/instance.py | 2 +- onadata/libs/utils/logger_tools.py | 15 ++--- 5 files changed, 40 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..7eeeb97eec 100644 --- a/onadata/apps/logger/models/attachment.py +++ b/onadata/apps/logger/models/attachment.py @@ -4,6 +4,7 @@ from django.db import models from django.utils import timezone +from django.contrib.auth.models import User def get_original_filename(filename): @@ -53,6 +54,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 +91,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() From c3af830095e925d72f394479c0ed44e2d336bca9 Mon Sep 17 00:00:00 2001 From: WinnyTroy Date: Thu, 13 Feb 2020 12:17:48 +0300 Subject: [PATCH 3/4] Fix Flake8 errors --- onadata/apps/logger/models/attachment.py | 3 ++- onadata/libs/utils/logger_tools.py | 12 ++++++++++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/onadata/apps/logger/models/attachment.py b/onadata/apps/logger/models/attachment.py index 7eeeb97eec..26ee598278 100644 --- a/onadata/apps/logger/models/attachment.py +++ b/onadata/apps/logger/models/attachment.py @@ -97,7 +97,8 @@ def soft_delete(self, instance, user=None): Soft deletes an attachment by adding a deleted_at timestamp. """ if instance: - queryset = self.__class__.objects.filter(name__in=self.get_expexted_media()) + queryset = self.__class__.objects.filter( + name__in=self.get_expexted_media()) kwargs = {'deleted_at': timezone.now()} if user: kwargs.update({'deleted_by': user}) diff --git a/onadata/libs/utils/logger_tools.py b/onadata/libs/utils/logger_tools.py index fb2ec35ecd..5c42912ed8 100644 --- a/onadata/libs/utils/logger_tools.py +++ b/onadata/libs/utils/logger_tools.py @@ -319,7 +319,11 @@ 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, remove_deleted_media=True) + 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 @@ -338,7 +342,11 @@ 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, remove_deleted_media=True) + save_attachments( + xform, + duplicate_instance, + media_files, + remove_deleted_media=True) duplicate_instance.save() return DuplicateInstance() From 953e0cfa7e1c88db5aaf079da0396fcda7806f68 Mon Sep 17 00:00:00 2001 From: WinnyTroy Date: Mon, 17 Feb 2020 17:51:57 +0300 Subject: [PATCH 4/4] Included test showing replaced attachment functionality Moved soft delete action to the instance model Restructured how we call the soft_delete action when editing submissions --- onadata/apps/logger/models/attachment.py | 14 ---- onadata/apps/logger/models/instance.py | 15 +++- onadata/libs/tests/utils/test_logger_tools.py | 78 +++++++++++++++++++ onadata/libs/utils/logger_tools.py | 10 ++- 4 files changed, 96 insertions(+), 21 deletions(-) diff --git a/onadata/apps/logger/models/attachment.py b/onadata/apps/logger/models/attachment.py index 26ee598278..9ea5520bfb 100644 --- a/onadata/apps/logger/models/attachment.py +++ b/onadata/apps/logger/models/attachment.py @@ -3,7 +3,6 @@ from hashlib import md5 from django.db import models -from django.utils import timezone from django.contrib.auth.models import User @@ -90,16 +89,3 @@ def file_hash(self): def filename(self): if self.media_file: return os.path.basename(self.media_file.name) - - @classmethod - def soft_delete(self, instance, user=None): - """ - Soft deletes an attachment by adding a deleted_at timestamp. - """ - 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 a4a7689b97..0618e61f20 100644 --- a/onadata/apps/logger/models/instance.py +++ b/onadata/apps/logger/models/instance.py @@ -4,6 +4,7 @@ """ import math from datetime import datetime +from django.db.models import Q from future.utils import python_2_unicode_compatible @@ -79,9 +80,6 @@ def _get_attachments_from_instance(instance): attachment['id'] = a.id attachments.append(attachment) - if a.deleted_at: - attachment['deleted_at'] = a.deleted_at.strftime(MONGO_STRFTIME) - return attachments @@ -586,6 +584,17 @@ def set_deleted(self, deleted_at=timezone.now(), user=None): self.xform.submission_count(force_update=True) self.parsed_instance.save() + def soft_delete_attachments(self, user=None): + """ + Soft deletes an attachment by adding a deleted_at timestamp. + """ + queryset = self.attachments.filter( + ~Q(name__in=self.get_expected_media())) + kwargs = {'deleted_at': timezone.now()} + if user: + kwargs.update({'deleted_by': user}) + queryset.update(**kwargs) + def post_save_submission(sender, instance=None, created=False, **kwargs): if ASYNC_POST_SUBMISSION_PROCESSING_ENABLED: diff --git a/onadata/libs/tests/utils/test_logger_tools.py b/onadata/libs/tests/utils/test_logger_tools.py index 2418db2024..ad0f8be87b 100644 --- a/onadata/libs/tests/utils/test_logger_tools.py +++ b/onadata/libs/tests/utils/test_logger_tools.py @@ -2,6 +2,7 @@ from io import BytesIO from django.conf import settings +from mock import patch from pyxform.tests_v1.pyxform_test_case import PyxformTestCase @@ -236,6 +237,83 @@ def test_attachment_tracking_for_nested_repeats(self): self.assertEquals(instance2.json[MEDIA_ALL_RECEIVED], instance2.media_all_received) + def test_replaced_attachments_not_tracked(self): + """ + Test that when a submission with an attachments is made, + and later edited, whereby the attachment is replaced, + the replaced attachment is no longer tracked for that + submission + """ + md = """ + | survey | | | | + | | type | name | label | + | | image | image1 | Photo | + """ + self._create_user_and_login() + self.xform = self._publish_markdown(md, self.user) + + xml_string = """ + + + uuid:UJ5jz4EszdgH8uhy8nss1AsKaqBPO5VN7 + + 1300221157303.jpg + + """.format(self.xform.id_string) + file_path = "{}/apps/logger/tests/Health_2011_03_13."\ + "xml_2011-03-15_20-30-28/1300221157303"\ + ".jpg".format(settings.PROJECT_ROOT) + media_file = django_file( + path=file_path, field_name="image1", content_type="image/jpeg") + instance = create_instance( + self.user.username, + BytesIO(xml_string.strip().encode('utf-8')), + media_files=[media_file]) + self.assertTrue(instance.json[MEDIA_ALL_RECEIVED]) + self.assertEquals(instance.json[TOTAL_MEDIA], 1) + self.assertEquals(instance.json[MEDIA_COUNT], 1) + self.assertEquals(instance.json[TOTAL_MEDIA], instance.total_media) + self.assertEquals(instance.json[MEDIA_COUNT], instance.media_count) + self.assertEquals(instance.json[MEDIA_ALL_RECEIVED], + instance.media_all_received) + patch_value = 'onadata.apps.logger.models.Instance.get_expected_media' + with patch(patch_value) as get_expected_media: + get_expected_media.return_value = ['1300375832136.jpg'] + updated_xml_string = """ + + + uuid:UJ5jz4EszdgH8uhy8nss1AsKaqBPO5VN7 + + + 1300375832136.jpg + + + """.format(self.xform.id_string) + file2_path = "{}/apps/logger/tests/Water_2011_03_17_2011"\ + "-03-17_16-29-59/1300375832136.jpg".format( + settings.PROJECT_ROOT) + media2_file = django_file( + path=file2_path, + field_name="image1", + content_type="image/jpeg") + create_instance( + self.user.username, + BytesIO(updated_xml_string.strip().encode('utf-8')), + media_files=[media2_file]) + + instance2 = Instance.objects.get(pk=instance.pk) + self.assertTrue(instance2.json[MEDIA_ALL_RECEIVED]) + # Test that only one attachment is recognised for this submission + self.assertEquals(instance2.json[TOTAL_MEDIA], 1) + self.assertEquals(instance2.json[MEDIA_COUNT], 1) + self.assertEquals( + instance2.json[TOTAL_MEDIA], instance2.total_media) + self.assertEquals( + instance2.json[MEDIA_COUNT], instance2.media_count) + self.assertEquals( + instance2.json[MEDIA_ALL_RECEIVED], + instance2.media_all_received) + def test_attachment_tracking_duplicate(self): """ Test that duplicate attachments does not affect if all attachments were diff --git a/onadata/libs/utils/logger_tools.py b/onadata/libs/utils/logger_tools.py index 5c42912ed8..aa17ca0ff9 100644 --- a/onadata/libs/utils/logger_tools.py +++ b/onadata/libs/utils/logger_tools.py @@ -242,9 +242,7 @@ def save_attachments(xform, instance, media_files, remove_deleted_media=False): name=filename, extension=extension) if remove_deleted_media: - Attachment.soft_delete( - instance.attachments.filter( - ~Q(name__in=instance.get_expected_media()))) + instance.soft_delete_attachments() update_attachment_tracking(instance) @@ -256,7 +254,11 @@ def save_submission(xform, xml, media_files, new_uuid, submitted_by, status, instance = _get_instance(xml, new_uuid, submitted_by, status, xform, checksum) - save_attachments(xform, instance, media_files) + save_attachments( + xform, + instance, + media_files, + remove_deleted_media=True) # override date created if required if date_created_override: