From 9e2a78a9bc6e8f2539d9237547f600529c67c9d6 Mon Sep 17 00:00:00 2001 From: "John N. Milner" Date: Mon, 14 Jun 2021 18:11:07 -0400 Subject: [PATCH] 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