From c7f90afeb4fb98ae2a1a4e247fe074f85c019ac9 Mon Sep 17 00:00:00 2001 From: "John N. Milner" Date: Mon, 14 Jun 2021 06:43:18 -0400 Subject: [PATCH 1/4] Stop setting deleted_at; follow the API's behavior MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit …and delete submissions outright --- .../commands/clean_duplicated_submissions.py | 70 ++++------------- .../commands/sync_deleted_instances_fix.py | 44 ----------- onadata/apps/logger/models/instance.py | 13 +--- .../apps/logger/tests/models/test_xform.py | 16 ---- ...est_command_syncd_deleted_instances_fix.py | 75 ------------------- onadata/apps/main/views.py | 3 +- .../commands/update_delete_from_mongo.py | 28 ------- 7 files changed, 17 insertions(+), 232 deletions(-) delete mode 100644 onadata/apps/logger/management/commands/sync_deleted_instances_fix.py delete mode 100644 onadata/apps/logger/tests/test_command_syncd_deleted_instances_fix.py delete mode 100644 onadata/apps/viewer/management/commands/update_delete_from_mongo.py diff --git a/onadata/apps/logger/management/commands/clean_duplicated_submissions.py b/onadata/apps/logger/management/commands/clean_duplicated_submissions.py index 5847db5a2..dc74c7200 100644 --- a/onadata/apps/logger/management/commands/clean_duplicated_submissions.py +++ b/onadata/apps/logger/management/commands/clean_duplicated_submissions.py @@ -41,19 +41,9 @@ def add_arguments(self, parser): help="Specify a XForm's `id_string` to clean up only this form", ) - parser.add_argument( - "--purge", - action='store_true', - default=False, - help="Erase duplicate `Instance`s from the database entirely instead " - "of marking them as deleted using the `deleted_at` attribute. " - "Default is False", - ) - def handle(self, *args, **options): username = options['user'] xform_id_string = options['xform'] - purge = options['purge'] # Retrieve all instances with the same `uuid`. query = Instance.objects @@ -63,13 +53,6 @@ def handle(self, *args, **options): if username: query = query.filter(xform__user__username=username) - # if we don't purge, we don't want to see instances - # that have been marked as deleted. However, if we do purge - # we do need these instances to be in the list in order - # to delete them permanently - if not purge: - query = query.filter(deleted_at=None) - query = query.values_list('uuid', flat=True)\ .annotate(count_uuid=Count('uuid'))\ .filter(count_uuid__gt=1)\ @@ -79,13 +62,6 @@ def handle(self, *args, **options): duplicated_query = Instance.objects.filter(uuid=uuid) - # if we don't purge, we don't want to see instances - # that have been marked as deleted. However, if we do purge - # we do need these instances to be in the list in order - # to delete them permanently - if not purge: - duplicated_query = duplicated_query.filter(deleted_at=None) - instances_with_same_uuid = duplicated_query.values_list('id', 'xml_hash')\ .order_by('xml_hash', 'date_created') @@ -99,8 +75,7 @@ def handle(self, *args, **options): if instance_xml_hash != xml_hash_ref: self.__clean_up(instance_id_ref, - duplicated_instance_ids, - purge) + duplicated_instance_ids) xml_hash_ref = instance_xml_hash instance_id_ref = instance_id duplicated_instance_ids = [] @@ -109,14 +84,10 @@ def handle(self, *args, **options): duplicated_instance_ids.append(instance_id) self.__clean_up(instance_id_ref, - duplicated_instance_ids, - purge) + duplicated_instance_ids) if not self.__vaccuum: - if purge: - self.stdout.write('No instances have been purged.') - else: - self.stdout.write('No instances have been marked as deleted.') + self.stdout.write('No instances have been purged.') else: # Update number of submissions for each user. for user_ in list(self.__users): @@ -130,7 +101,7 @@ def handle(self, *args, **options): self.stdout.write( '\t\tDone! New number: {}'.format(result['count'])) - def __clean_up(self, instance_id_ref, duplicated_instance_ids, purge): + def __clean_up(self, instance_id_ref, duplicated_instance_ids): if instance_id_ref is not None and len(duplicated_instance_ids) > 0: self.__vaccuum = True with transaction.atomic(): @@ -146,30 +117,15 @@ def __clean_up(self, instance_id_ref, duplicated_instance_ids, purge): .get(id=instance_id_ref) main_instance.parsed_instance.save() - if purge: - self.stdout.write('\tPurging instances: {}'.format( - duplicated_instance_ids)) - Instance.objects.select_for_update()\ - .filter(id__in=duplicated_instance_ids).delete() - ParsedInstance.objects.select_for_update()\ - .filter(instance_id__in=duplicated_instance_ids).delete() - settings.MONGO_DB.instances.remove( - {'_id': {'$in': duplicated_instance_ids}} - ) - else: - self.stdout.write('\tMarking instances as deleted: {}'.format( - duplicated_instance_ids)) - # We could loop through instances and use `Instance.set_deleted()` - # but it would be way slower. - Instance.objects.select_for_update()\ - .filter(id__in=duplicated_instance_ids)\ - .update(deleted_at=timezone.now()) - settings.MONGO_DB.instances.update_many( - {'_id': {'$in': duplicated_instance_ids}}, - {'$set': { - '_deleted_at': timezone.now().strftime(MONGO_STRFTIME) - }} - ) + self.stdout.write('\tPurging instances: {}'.format( + duplicated_instance_ids)) + Instance.objects.select_for_update()\ + .filter(id__in=duplicated_instance_ids).delete() + ParsedInstance.objects.select_for_update()\ + .filter(instance_id__in=duplicated_instance_ids).delete() + settings.MONGO_DB.instances.remove( + {'_id': {'$in': duplicated_instance_ids}} + ) # Update number of submissions xform = main_instance.xform self.stdout.write( diff --git a/onadata/apps/logger/management/commands/sync_deleted_instances_fix.py b/onadata/apps/logger/management/commands/sync_deleted_instances_fix.py deleted file mode 100644 index 6b630feef..000000000 --- a/onadata/apps/logger/management/commands/sync_deleted_instances_fix.py +++ /dev/null @@ -1,44 +0,0 @@ -#!/usr/bin/env python -# vim: ai ts=4 sts=4 et sw=4 fileencoding=utf-8 -# coding: utf-8 -from __future__ import unicode_literals, print_function, division, absolute_import - -import json - -from django.conf import settings -from django.core.management import BaseCommand -from django.utils import timezone -from django.utils.dateparse import parse_datetime -from django.utils.translation import ugettext_lazy - -from onadata.apps.logger.models import Instance - - -class Command(BaseCommand): - help = ugettext_lazy("Fixes deleted instances by syncing " - "deleted items from mongo.") - - def handle(self, *args, **kwargs): - # Reset all sql deletes to None - Instance.objects.exclude( - deleted_at=None, xform__downloadable=True).update(deleted_at=None) - - # Get all mongo deletes - query = '{"$and": [{"_deleted_at": {"$exists": true}}, ' \ - '{"_deleted_at": {"$ne": null}}]}' - query = json.loads(query) - xform_instances = settings.MONGO_DB.instances - cursor = xform_instances.find(query) - for record in cursor: - # update sql instance with deleted_at datetime from mongo - try: - i = Instance.objects.get( - uuid=record["_uuid"], xform__downloadable=True) - except Instance.DoesNotExist: - continue - else: - deleted_at = parse_datetime(record["_deleted_at"]) - if not timezone.is_aware(deleted_at): - deleted_at = timezone.make_aware( - deleted_at, timezone.utc) - i.set_deleted(deleted_at) diff --git a/onadata/apps/logger/models/instance.py b/onadata/apps/logger/models/instance.py index 61a308969..20bb0cad0 100644 --- a/onadata/apps/logger/models/instance.py +++ b/onadata/apps/logger/models/instance.py @@ -174,12 +174,7 @@ def asset(self): @classmethod def set_deleted_at(cls, instance_id, deleted_at=timezone.now()): - try: - instance = cls.objects.get(id=instance_id) - except cls.DoesNotExist: - pass - else: - instance.set_deleted(deleted_at) + raise Exception('This method MUST NOT be used.') def _check_active(self, force): """Check that form is active and raise exception if not. @@ -392,11 +387,7 @@ def save(self, *args, **kwargs): super(Instance, self).save(*args, **kwargs) def set_deleted(self, deleted_at=timezone.now()): - self.deleted_at = deleted_at - self.save() - # force submission count re-calculation - self.xform.submission_count(force_update=True) - self.parsed_instance.save() + raise Exception('This method MUST NOT be used.') def get_validation_status(self): """ diff --git a/onadata/apps/logger/tests/models/test_xform.py b/onadata/apps/logger/tests/models/test_xform.py index d838fcb54..c67a74146 100644 --- a/onadata/apps/logger/tests/models/test_xform.py +++ b/onadata/apps/logger/tests/models/test_xform.py @@ -9,22 +9,6 @@ class TestXForm(TestBase): - def test_submission_count_filters_deleted(self): - self._publish_transportation_form_and_submit_instance() - - # update the xform object the num_submissions seems to be cached in - # the in-memory xform object as zero - self.xform = XForm.objects.get(pk=self.xform.id) - self.assertEqual(self.xform.submission_count(), 1) - instance = Instance.objects.get(xform=self.xform) - instance.set_deleted() - self.assertIsNotNone(instance.deleted_at) - - # update the xform object, the num_submissions seems to be cached in - # the in-memory xform object as one - self.xform = XForm.objects.get(pk=self.xform.id) - self.assertEqual(self.xform.submission_count(), 0) - def test_set_title_in_xml_unicode_error(self): xls_file_path = os.path.join( os.path.dirname(os.path.abspath(__file__)), diff --git a/onadata/apps/logger/tests/test_command_syncd_deleted_instances_fix.py b/onadata/apps/logger/tests/test_command_syncd_deleted_instances_fix.py deleted file mode 100644 index 2dd13e504..000000000 --- a/onadata/apps/logger/tests/test_command_syncd_deleted_instances_fix.py +++ /dev/null @@ -1,75 +0,0 @@ -# coding: utf-8 -from __future__ import unicode_literals, print_function, division, absolute_import - -from datetime import datetime -from django.utils import timezone -from django.core.management import call_command - -from onadata.apps.main.tests.test_base import TestBase -from onadata.apps.logger.models import Instance - - -class CommandSyncDeletedTests(TestBase): - - def setUp(self): - TestBase.setUp(self) - self._create_user_and_login() - self._publish_transportation_form_and_submit_instance() - - def test_command(self): - count = Instance.objects.filter(deleted_at=None).count() - self.assertTrue(count > 0) - deleted_at = timezone.now() - deleted_at = datetime.strptime( - deleted_at.strftime("%Y-%m-%dT%H:%M:%S"), "%Y-%m-%dT%H:%M:%S") - deleted_at = timezone.make_aware(deleted_at, timezone.utc) - instance = Instance.objects.filter(deleted_at=None)[0] - instance.deleted_at = deleted_at - instance.save() - - # ensure mongo has deleted_at by calling remongo - call_command('remongo') - - # reset deleted_at to None - instance.deleted_at = None - instance.save() - - same_instance = Instance.objects.get(pk=instance.pk) - self.assertIsNone(same_instance.deleted_at) - - # reset the deleted_at time from datetime in mongo - call_command('sync_deleted_instances_fix') - same_instance = Instance.objects.get(pk=instance.pk) - - # deleted_at should now have a datetime value - self.assertIsNotNone(same_instance.deleted_at) - self.assertTrue(isinstance(same_instance.deleted_at, datetime)) - self.assertEqual(same_instance.deleted_at, deleted_at) - - def test_command_on_inactive_form(self): - count = Instance.objects.filter(deleted_at=None).count() - self.assertTrue(count > 0) - deleted_at = timezone.now() - instance = Instance.objects.filter(deleted_at=None)[0] - instance.deleted_at = deleted_at - instance.save() - - # ensure mongo has deleted_at by calling remongo - call_command('remongo') - - # reset deleted_at to None - instance.deleted_at = None - instance.save() - - # make xform inactive - self.xform.downloadable = False - self.xform.save() - same_instance = Instance.objects.get(pk=instance.pk) - self.assertIsNone(same_instance.deleted_at) - - # reset the deleted_at time from datetime in mongo - call_command('sync_deleted_instances_fix') - same_instance = Instance.objects.get(pk=instance.pk) - - # deleted_at should now have a datetime value - self.assertIsNone(same_instance.deleted_at) diff --git a/onadata/apps/main/views.py b/onadata/apps/main/views.py index 1e0f76568..09c8dbe0f 100644 --- a/onadata/apps/main/views.py +++ b/onadata/apps/main/views.py @@ -1225,7 +1225,8 @@ def delete_data(request, username=None, id_string=None): if not data_id: return HttpResponseBadRequest(_("id must be specified")) - Instance.set_deleted_at(data_id) + doomed_instance = get_object_or_404(Instance, pk=data_id) + doomed_instance.delete() audit = { 'xform': xform.id_string } diff --git a/onadata/apps/viewer/management/commands/update_delete_from_mongo.py b/onadata/apps/viewer/management/commands/update_delete_from_mongo.py deleted file mode 100644 index 094f3ee57..000000000 --- a/onadata/apps/viewer/management/commands/update_delete_from_mongo.py +++ /dev/null @@ -1,28 +0,0 @@ -# coding: utf-8 -from __future__ import unicode_literals, print_function, division, absolute_import - -from django.core.management.base import BaseCommand -from django.utils.translation import ugettext_lazy - -from onadata.apps.logger.models.instance import Instance -from onadata.apps.viewer.models.parsed_instance import xform_instances,\ - datetime_from_str -from onadata.libs.utils.common_tags import DELETEDAT, ID - - -class Command(BaseCommand): - help = ugettext_lazy("Update deleted records from mongo to sql instances") - - def handle(self, *args, **kwargs): - q = {"$and": [{"_deleted_at": {"$exists": True}}, - {"_deleted_at": {"$ne": None}}]} - cursor = xform_instances.find(q) - c = 0 - for record in cursor: - date_deleted = datetime_from_str(record[DELETEDAT]) - id = record[ID] - if Instance.set_deleted_at(id, deleted_at=date_deleted): - c += 1 - print("deleted on ", date_deleted) - print("-------------------------------") - print("Updated %d records." % c) From ad110657f1c9628dd0559293ca18333c24bdffbc Mon Sep 17 00:00:00 2001 From: "John N. Milner" Date: Mon, 14 Jun 2021 07:31:12 -0400 Subject: [PATCH 2/4] Stop querying deleted_at See #696 --- onadata/apps/logger/models/xform.py | 8 ++--- onadata/apps/logger/views.py | 4 +-- onadata/apps/survey_report/views.py | 2 +- .../rewrite_attachments_urls_in_mongo.py | 4 --- onadata/apps/viewer/models/parsed_instance.py | 29 +++++-------------- onadata/libs/data/query.py | 3 +- onadata/libs/utils/export_tools.py | 7 +---- 7 files changed, 16 insertions(+), 41 deletions(-) diff --git a/onadata/apps/logger/models/xform.py b/onadata/apps/logger/models/xform.py index 9c6a996c7..f206fef49 100644 --- a/onadata/apps/logger/models/xform.py +++ b/onadata/apps/logger/models/xform.py @@ -223,7 +223,7 @@ def __unicode__(self): def submission_count(self, force_update=False): if self.num_of_submissions == 0 or force_update: - count = self.instances.filter(deleted_at__isnull=True).count() + count = self.instances.count() self.num_of_submissions = count self.save(update_fields=['num_of_submissions']) return self.num_of_submissions @@ -231,14 +231,12 @@ def submission_count(self, force_update=False): def geocoded_submission_count(self): """Number of geocoded submissions.""" - return self.instances.filter(deleted_at__isnull=True, - geom__isnull=False).count() + return self.instances.filter(geom__isnull=False).count() def time_of_last_submission(self): if self.last_submission_time is None and self.num_of_submissions > 0: try: - last_submission = self.instances.\ - filter(deleted_at__isnull=True).latest("date_created") + last_submission = self.instances.latest("date_created") except ObjectDoesNotExist: pass else: diff --git a/onadata/apps/logger/views.py b/onadata/apps/logger/views.py index a5e7d7cb0..baae0d8ba 100644 --- a/onadata/apps/logger/views.py +++ b/onadata/apps/logger/views.py @@ -579,7 +579,7 @@ def view_submission_list(request, username): return HttpResponseForbidden('Not shared.') num_entries = request.GET.get('numEntries', None) cursor = request.GET.get('cursor', None) - instances = xform.instances.filter(deleted_at=None).order_by('pk') + instances = xform.instances.order_by('pk') cursor = _parse_int(cursor) if cursor: @@ -625,7 +625,7 @@ def view_download_submission(request, username): uuid = _extract_uuid(form_id_parts[1]) instance = get_object_or_404( Instance, xform__id_string__exact=id_string, uuid=uuid, - xform__user__username=username, deleted_at=None) + xform__user__username=username) xform = instance.xform if not has_permission(xform, form_user, request, xform.shared_data): return HttpResponseForbidden('Not shared.') diff --git a/onadata/apps/survey_report/views.py b/onadata/apps/survey_report/views.py index d955cd5d2..f37fdfbd3 100644 --- a/onadata/apps/survey_report/views.py +++ b/onadata/apps/survey_report/views.py @@ -34,7 +34,7 @@ def _wrapper(request, username, id_string, *args, **kwargs): def get_instances_for_user_and_form(user, form_id, submission=None): userform_id = '{}_{}'.format(user, form_id) - query = {'_userform_id': userform_id, '_deleted_at': {'$exists': False}} + query = {'_userform_id': userform_id} if submission: query['_id'] = submission return settings.MONGO_DB.instances.find( diff --git a/onadata/apps/viewer/management/commands/rewrite_attachments_urls_in_mongo.py b/onadata/apps/viewer/management/commands/rewrite_attachments_urls_in_mongo.py index b884848fd..ef2a4bf93 100644 --- a/onadata/apps/viewer/management/commands/rewrite_attachments_urls_in_mongo.py +++ b/onadata/apps/viewer/management/commands/rewrite_attachments_urls_in_mongo.py @@ -78,10 +78,6 @@ def handle(self, *args, **kwargs): def __get_data(self): query = {"$and": [ - {"$or": [ - {"_deleted_at": {"$exists": False}}, - {"_deleted_at": None} - ]}, {"_attachments": {"$ne": ""}}, {"_attachments": {"$ne": []}}, {"$or": [ diff --git a/onadata/apps/viewer/models/parsed_instance.py b/onadata/apps/viewer/models/parsed_instance.py index b33737130..3158e43a5 100644 --- a/onadata/apps/viewer/models/parsed_instance.py +++ b/onadata/apps/viewer/models/parsed_instance.py @@ -94,9 +94,9 @@ def get_base_query(cls, username, id_string): @classmethod @apply_form_field_names def query_mongo(cls, username, id_string, query, fields, sort, start=0, - limit=DEFAULT_LIMIT, count=False, hide_deleted=True): + limit=DEFAULT_LIMIT, count=False): - cursor = cls._get_mongo_cursor(query, fields, hide_deleted, username, id_string) + cursor = cls._get_mongo_cursor(query, fields, username, id_string) if count: return [{"count": cursor.count()}] @@ -112,7 +112,7 @@ def query_mongo(cls, username, id_string, query, fields, sort, start=0, @classmethod @apply_form_field_names - def mongo_aggregate(cls, query, pipeline, hide_deleted=True): + def mongo_aggregate(cls, query, pipeline): """Perform mongo aggregate queries query - is a dict which is to be passed to $match, a pipeline operator pipeline - list of dicts or dict of mongodb pipeline operators, @@ -126,13 +126,6 @@ def mongo_aggregate(cls, query, pipeline, hide_deleted=True): if not isinstance(query, dict): raise Exception(_("Invalid query! %s" % query)) query = MongoHelper.to_safe_dict(query) - if hide_deleted: - # display only active elements - deleted_at_query = { - "$or": [{"_deleted_at": {"$exists": False}}, - {"_deleted_at": None}]} - # join existing query with deleted_at_query on an $and - query = {"$and": [query, deleted_at_query]} k = [{'$match': query}] if isinstance(pipeline, list): k.extend(pipeline) @@ -145,9 +138,9 @@ def mongo_aggregate(cls, query, pipeline, hide_deleted=True): @apply_form_field_names def query_mongo_minimal( cls, query, fields, sort, start=0, limit=DEFAULT_LIMIT, - count=False, hide_deleted=True): + count=False): - cursor = cls._get_mongo_cursor(query, fields, hide_deleted) + cursor = cls._get_mongo_cursor(query, fields) if count: return [{"count": cursor.count()}] @@ -166,9 +159,9 @@ def query_mongo_minimal( @classmethod @apply_form_field_names - def query_mongo_no_paging(cls, query, fields, count=False, hide_deleted=True): + def query_mongo_no_paging(cls, query, fields, count=False): - cursor = cls._get_mongo_cursor(query, fields, hide_deleted) + cursor = cls._get_mongo_cursor(query, fields) if count: return [{"count": cursor.count()}] @@ -176,13 +169,12 @@ def query_mongo_no_paging(cls, query, fields, count=False, hide_deleted=True): return cursor @classmethod - def _get_mongo_cursor(cls, query, fields, hide_deleted, username=None, id_string=None): + def _get_mongo_cursor(cls, query, fields, username=None, id_string=None): """ Returns a Mongo cursor based on the query. :param query: JSON string :param fields: Array string - :param hide_deleted: boolean :param username: string :param id_string: string :return: pymongo Cursor @@ -198,11 +190,6 @@ def _get_mongo_cursor(cls, query, fields, hide_deleted, username=None, id_string if username and id_string: query.update(cls.get_base_query(username, id_string)) - if hide_deleted: - # display only active elements - # join existing query with deleted_at_query on an $and - query = {"$and": [query, {"_deleted_at": None}]} - # fields must be a string array i.e. '["name", "age"]' if isinstance(fields, basestring): fields = json.loads(fields, object_hook=json_util.object_hook) diff --git a/onadata/libs/data/query.py b/onadata/libs/data/query.py index 56dc26048..f4eb0372b 100644 --- a/onadata/libs/data/query.py +++ b/onadata/libs/data/query.py @@ -72,8 +72,7 @@ def _postgres_select_key(field, name, xform): string_args = _query_args(field, name, xform) return "SELECT %(json)s AS \"%(name)s\" FROM %(table)s WHERE "\ - "%(restrict_field)s=%(restrict_value)s "\ - "AND %(exclude_deleted)s" % string_args + "%(restrict_field)s=%(restrict_value)s" % string_args def _query_args(field, name, xform): diff --git a/onadata/libs/utils/export_tools.py b/onadata/libs/utils/export_tools.py index d8fa2cd27..5f1573663 100644 --- a/onadata/libs/utils/export_tools.py +++ b/onadata/libs/utils/export_tools.py @@ -758,15 +758,11 @@ def generate_export(export_type, extension, username, id_string, return export -def query_mongo(username, id_string, query=None, hide_deleted=True): +def query_mongo(username, id_string, query=None): query = json.loads(query, object_hook=json_util.object_hook)\ if query else {} query = MongoHelper.to_safe_dict(query) query[USERFORM_ID] = '{0}_{1}'.format(username, id_string) - if hide_deleted: - # display only active elements - # join existing query with deleted_at_query on an $and - query = {"$and": [query, {"_deleted_at": None}]} return xform_instances.find(query, max_time_ms=settings.MONGO_DB_MAX_TIME_MS) @@ -895,7 +891,6 @@ def kml_export_data(id_string, user): instances = Instance.objects.filter( xform__user=user, xform__id_string=id_string, - deleted_at=None, geom__isnull=False ).order_by('id') data_for_template = [] From 9e2a78a9bc6e8f2539d9237547f600529c67c9d6 Mon Sep 17 00:00:00 2001 From: "John N. Milner" Date: Mon, 14 Jun 2021 18:11:07 -0400 Subject: [PATCH 3/4] Complete removal of deleted_at. Closes #696 --- onadata/apps/api/viewsets/data_viewset.py | 3 -- onadata/apps/logger/models/instance.py | 39 ++++++++++++------ .../apps/main/tests/test_form_api_delete.py | 40 ++++++++----------- onadata/apps/viewer/models/parsed_instance.py | 4 -- onadata/apps/viewer/pandas_mongo_bridge.py | 11 ++++- .../viewer/tests/test_pandas_mongo_bridge.py | 1 + onadata/libs/data/query.py | 4 +- onadata/libs/utils/common_tags.py | 2 +- onadata/libs/utils/export_tools.py | 16 +++++--- 9 files changed, 67 insertions(+), 53 deletions(-) diff --git a/onadata/apps/api/viewsets/data_viewset.py b/onadata/apps/api/viewsets/data_viewset.py index 5dbdfd46d..e4ab03517 100644 --- a/onadata/apps/api/viewsets/data_viewset.py +++ b/onadata/apps/api/viewsets/data_viewset.py @@ -111,7 +111,6 @@ class DataViewSet(AnonymousUserPublicFormsMixin, ModelViewSet): > [ > { > "_id": 4503, -> "_deleted_at": null, > "expense_type": "service", > "_xform_id_string": "exp", > "_geolocation": [ @@ -159,7 +158,6 @@ class DataViewSet(AnonymousUserPublicFormsMixin, ModelViewSet): > > { > "_id": 4503, -> "_deleted_at": null, > "expense_type": "service", > "_xform_id_string": "exp", > "_geolocation": [ @@ -215,7 +213,6 @@ class DataViewSet(AnonymousUserPublicFormsMixin, ModelViewSet): > [ > { > "_id": 4503, -> "_deleted_at": null, > "expense_type": "service", > "_xform_id_string": "exp", > "_geolocation": [ diff --git a/onadata/apps/logger/models/instance.py b/onadata/apps/logger/models/instance.py index 20bb0cad0..ff16fbba6 100644 --- a/onadata/apps/logger/models/instance.py +++ b/onadata/apps/logger/models/instance.py @@ -24,7 +24,6 @@ clean_and_parse_xml, get_uuid_from_xml from onadata.libs.utils.common_tags import ( ATTACHMENTS, - DELETEDAT, GEOLOCATION, ID, MONGO_STRFTIME, @@ -91,6 +90,28 @@ def update_xform_submission_count(sender, instance, created, **kwargs): ) +def nullify_exports_time_of_last_submission(sender, instance, **kwargs): + """ + Formerly, "deleting" a submission would set a flag on the `Instance`, + causing the `date_modified` attribute to be set to the current timestamp. + `Export.exports_outdated()` relied on this to detect when a new `Export` + needed to be generated due to submission deletion, but now that we always + delete `Instance`s outright, this trick doesn't work. This kludge simply + makes every `Export` for a form appear stale by nulling out its + `time_of_last_submission` attribute. + """ + # Avoid circular import + try: + export_model = instance.xform.export_set.model + except XForm.DoesNotExist: + return + f = instance.xform.export_set.filter( + # Match the statuses considered by `Export.exports_outdated()` + internal_status__in=[export_model.SUCCESSFUL, export_model.PENDING], + ) + f.update(time_of_last_submission=None) + + def update_xform_submission_count_delete(sender, instance, **kwargs): try: xform = XForm.objects.select_for_update().get(pk=instance.xform.pk) @@ -133,7 +154,8 @@ class Instance(models.Model): # this will end up representing "date last parsed" date_modified = models.DateTimeField(auto_now=True) - # this will end up representing "date instance was deleted" + # this formerly represented "date instance was deleted". + # do not use it anymore. deleted_at = models.DateTimeField(null=True, default=None) # ODK keeps track of three statuses for an instance: @@ -172,10 +194,6 @@ def asset(self): """ return self.xform - @classmethod - def set_deleted_at(cls, instance_id, deleted_at=timezone.now()): - raise Exception('This method MUST NOT be used.') - def _check_active(self, force): """Check that form is active and raise exception if not. @@ -331,9 +349,6 @@ def get_full_dict(self): NOTES: self.get_notes() } - if isinstance(self.instance.deleted_at, datetime): - data[DELETEDAT] = self.deleted_at.strftime(MONGO_STRFTIME) - d.update(data) return d @@ -386,9 +401,6 @@ def save(self, *args, **kwargs): super(Instance, self).save(*args, **kwargs) - def set_deleted(self, deleted_at=timezone.now()): - raise Exception('This method MUST NOT be used.') - def get_validation_status(self): """ Returns instance validation status. @@ -405,6 +417,9 @@ def get_validation_status(self): post_save.connect(update_xform_submission_count, sender=Instance, dispatch_uid='update_xform_submission_count') +post_delete.connect(nullify_exports_time_of_last_submission, sender=Instance, + dispatch_uid='nullify_exports_time_of_last_submission') + post_delete.connect(update_xform_submission_count_delete, sender=Instance, dispatch_uid='update_xform_submission_count_delete') diff --git a/onadata/apps/main/tests/test_form_api_delete.py b/onadata/apps/main/tests/test_form_api_delete.py index a509d8e01..6866a4606 100644 --- a/onadata/apps/main/tests/test_form_api_delete.py +++ b/onadata/apps/main/tests/test_form_api_delete.py @@ -34,15 +34,15 @@ def _get_data(self): def test_get_request_does_not_delete(self): # not allowed 405 - count = Instance.objects.filter(deleted_at=None).count() + count = Instance.objects.count() response = self.anon.get(self.delete_url) self.assertEqual(response.status_code, 405) self.assertEqual( - Instance.objects.filter(deleted_at=None).count(), count) + Instance.objects.count(), count) def test_anon_user_cant_delete(self): # Only authenticated user are allowed to access the url - count = Instance.objects.filter(deleted_at=None).count() + count = Instance.objects.count() instance = Instance.objects.filter( xform=self.xform).latest('date_created') # delete @@ -51,14 +51,14 @@ def test_anon_user_cant_delete(self): self.assertEqual(response.status_code, 302) self.assertIn("accounts/login/?next=", response["Location"]) self.assertEqual( - Instance.objects.filter(deleted_at=None).count(), count) + Instance.objects.count(), count) def test_delete_shared(self): # Test if someone can delete data from a shared form self.xform.shared = True self.xform.save() self._create_user_and_login("jo") - count = Instance.objects.filter(deleted_at=None).count() + count = Instance.objects.count() instance = Instance.objects.filter( xform=self.xform).latest('date_created') # delete @@ -66,24 +66,21 @@ def test_delete_shared(self): response = self.client.post(self.delete_url, params) self.assertEqual(response.status_code, 403) self.assertEqual( - Instance.objects.filter(deleted_at=None).count(), count) + Instance.objects.count(), count) def test_owner_can_delete(self): # Test if Form owner can delete # check record exist before delete and after delete - count = Instance.objects.filter(deleted_at=None).count() + count = Instance.objects.count() instance = Instance.objects.filter( xform=self.xform).latest('date_created') - self.assertEqual(instance.deleted_at, None) # delete params = {'id': instance.id} response = self.client.post(self.delete_url, params) self.assertEqual(response.status_code, 200) self.assertEqual( - Instance.objects.filter(deleted_at=None).count(), count - 1) - instance = Instance.objects.get(id=instance.id) - self.assertTrue(isinstance(instance.deleted_at, datetime)) - self.assertNotEqual(instance.deleted_at, None) + Instance.objects.count(), count - 1) + self.assertFalse(Instance.objects.filter(id=instance.id).exists()) query = '{"_id": %s}' % instance.id self.mongo_args.update({"query": query}) # check that query_mongo will not return the deleted record @@ -91,8 +88,7 @@ def test_owner_can_delete(self): self.assertEqual(len(after), count - 1) def test_delete_updates_mongo(self): - count = Instance.objects.filter( - xform=self.xform, deleted_at=None).count() + count = Instance.objects.filter(xform=self.xform).count() instance = Instance.objects.filter( xform=self.xform).latest('date_created') # delete @@ -100,14 +96,12 @@ def test_delete_updates_mongo(self): response = self.client.post(self.delete_url, params) self.assertEqual(response.status_code, 200) self.assertEqual( - Instance.objects.filter( - xform=self.xform, deleted_at=None).count(), count - 1) - # check that instance's deleted_at is set - instance = Instance.objects.get(id=instance.id) - self.assertTrue(isinstance(instance.deleted_at, datetime)) - # check mongo record was marked as deleted + Instance.objects.filter(xform=self.xform).count(), + count - 1, + ) + # check that instance was removed from postgres + self.assertFalse(Instance.objects.filter(id=instance.id).exists()) + # check that mongo record was also removed cursor = settings.MONGO_DB.instances.find( {common_tags.ID: instance.id}) - self.assertEqual(cursor.count(), 1) - record = cursor.next() - self.assertIsNotNone(record[common_tags.DELETEDAT]) + self.assertEqual(cursor.count(), 0) diff --git a/onadata/apps/viewer/models/parsed_instance.py b/onadata/apps/viewer/models/parsed_instance.py index 3158e43a5..bb4c25362 100644 --- a/onadata/apps/viewer/models/parsed_instance.py +++ b/onadata/apps/viewer/models/parsed_instance.py @@ -22,7 +22,6 @@ GEOLOCATION, SUBMISSION_TIME, MONGO_STRFTIME, - DELETEDAT, TAGS, NOTES, SUBMITTED_BY, @@ -250,9 +249,6 @@ def to_dict_for_mongo(self): if self.instance.user else None } - if isinstance(self.instance.deleted_at, datetime.datetime): - data[DELETEDAT] = self.instance.deleted_at.strftime(MONGO_STRFTIME) - d.update(data) return MongoHelper.to_safe_dict(d) diff --git a/onadata/apps/viewer/pandas_mongo_bridge.py b/onadata/apps/viewer/pandas_mongo_bridge.py index 1feaaffdb..aecd45868 100644 --- a/onadata/apps/viewer/pandas_mongo_bridge.py +++ b/onadata/apps/viewer/pandas_mongo_bridge.py @@ -87,8 +87,15 @@ def get_prefix_from_xpath(xpath): class AbstractDataFrameBuilder(object): - IGNORED_COLUMNS = [XFORM_ID_STRING, STATUS, ID, ATTACHMENTS, GEOLOCATION, - DELETEDAT, SUBMITTED_BY] + IGNORED_COLUMNS = [ + XFORM_ID_STRING, + STATUS, + ID, + ATTACHMENTS, + GEOLOCATION, + DELETEDAT, # no longer used but may persist in old submissions + SUBMITTED_BY, + ] # fields NOT within the form def that we want to include ADDITIONAL_COLUMNS = [UUID, SUBMISSION_TIME, TAGS, NOTES, VALIDATION_STATUS] BINARY_SELECT_MULTIPLES = False diff --git a/onadata/apps/viewer/tests/test_pandas_mongo_bridge.py b/onadata/apps/viewer/tests/test_pandas_mongo_bridge.py index f29fb94fa..0c13d0869 100644 --- a/onadata/apps/viewer/tests/test_pandas_mongo_bridge.py +++ b/onadata/apps/viewer/tests/test_pandas_mongo_bridge.py @@ -253,6 +253,7 @@ def test_csv_columns_for_gps_within_groups(self): ] + AbstractDataFrameBuilder.ADDITIONAL_COLUMNS +\ AbstractDataFrameBuilder.IGNORED_COLUMNS try: + # `_deleted_at` is no longer used but may persist in old submissions expected_columns.remove('_deleted_at') except ValueError: pass diff --git a/onadata/libs/data/query.py b/onadata/libs/data/query.py index f4eb0372b..9417448f5 100644 --- a/onadata/libs/data/query.py +++ b/onadata/libs/data/query.py @@ -64,7 +64,6 @@ def _postgres_count_group(field, name, xform): return "SELECT %(json)s AS \"%(name)s\", COUNT(*) AS count FROM "\ "%(table)s WHERE %(restrict_field)s=%(restrict_value)s "\ - "AND %(exclude_deleted)s "\ "GROUP BY %(json)s" % string_args @@ -81,8 +80,7 @@ def _query_args(field, name, xform): 'json': _json_query(field), 'name': name, 'restrict_field': 'xform_id', - 'restrict_value': xform.pk, - 'exclude_deleted': 'deleted_at IS NULL'} + 'restrict_value': xform.pk} def _select_key(field, name, xform): diff --git a/onadata/libs/utils/common_tags.py b/onadata/libs/utils/common_tags.py index 01b0112cd..f0a007d17 100644 --- a/onadata/libs/utils/common_tags.py +++ b/onadata/libs/utils/common_tags.py @@ -34,7 +34,7 @@ DATE = "_date" GEOLOCATION = "_geolocation" SUBMISSION_TIME = '_submission_time' -DELETEDAT = "_deleted_at" # marker for delete surveys +DELETEDAT = "_deleted_at" # no longer used but may persist in old submissions SUBMITTED_BY = "_submitted_by" VALIDATION_STATUS = "_validation_status" diff --git a/onadata/libs/utils/export_tools.py b/onadata/libs/utils/export_tools.py index 5f1573663..f0e281488 100644 --- a/onadata/libs/utils/export_tools.py +++ b/onadata/libs/utils/export_tools.py @@ -178,8 +178,13 @@ def dict_to_joined_export(data, index, indices, name): class ExportBuilder(object): - IGNORED_COLUMNS = [XFORM_ID_STRING, STATUS, ATTACHMENTS, GEOLOCATION, - DELETEDAT] + IGNORED_COLUMNS = [ + XFORM_ID_STRING, + STATUS, + ATTACHMENTS, + GEOLOCATION, + DELETEDAT, # no longer used but may persist in old submissions + ] # fields we export but are not within the form's structure EXTRA_FIELDS = [ID, UUID, SUBMISSION_TIME, INDEX, PARENT_TABLE_NAME, PARENT_INDEX, TAGS, NOTES] @@ -767,9 +772,10 @@ def query_mongo(username, id_string, query=None): def should_create_new_export(xform, export_type): - if Export.objects.filter( - xform=xform, export_type=export_type).count() == 0\ - or Export.exports_outdated(xform, export_type=export_type): + if ( + not Export.objects.filter(xform=xform, export_type=export_type).exists() + or Export.exports_outdated(xform, export_type=export_type) + ): return True return False From f759623271a721086cc0a417ce9c18a588b729ce Mon Sep 17 00:00:00 2001 From: Olivier Leger Date: Tue, 15 Jun 2021 13:32:13 -0400 Subject: [PATCH 4/4] Fixed previous bad merge --- onadata/apps/viewer/models/parsed_instance.py | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/onadata/apps/viewer/models/parsed_instance.py b/onadata/apps/viewer/models/parsed_instance.py index 0b4475cb9..345be03cc 100644 --- a/onadata/apps/viewer/models/parsed_instance.py +++ b/onadata/apps/viewer/models/parsed_instance.py @@ -216,12 +216,11 @@ def _get_mongo_cursor(cls, query, fields): ) @classmethod - def _get_mongo_cursor_query(cls, query, hide_deleted, username=None, id_string=None): + def _get_mongo_cursor_query(cls, query, username=None, id_string=None): """ Returns the query to get a Mongo cursor. :param query: JSON string - :param hide_deleted: boolean :param username: string :param id_string: string :return: dict @@ -236,11 +235,6 @@ def _get_mongo_cursor_query(cls, query, hide_deleted, username=None, id_string=N if username and id_string: query.update(cls.get_base_query(username, id_string)) - if hide_deleted: - # display only active elements - # join existing query with deleted_at_query on an $and - query = {"$and": [query, {"_deleted_at": None}]} - return query @classmethod @@ -307,7 +301,9 @@ def update_mongo(self, asynchronous=True): if success != self.instance.is_synced_with_mongo: # Skip the labor-intensive stuff in Instance.save() to gain performance # Use .update() instead of .save() - Instance.objects.filter(pk=self.instance.id).update(is_synced_with_mongo=success) + Instance.objects.filter(pk=self.instance.id).update( + is_synced_with_mongo=success + ) return True