From 909913ba2a6166a723c48e12d6e601d9030089a2 Mon Sep 17 00:00:00 2001 From: Kipchirchir Sigei Date: Wed, 16 Nov 2022 08:17:23 +0300 Subject: [PATCH 1/8] Update geojson enpoint not to return features without geometries Signed-off-by: Kipchirchir Sigei --- onadata/apps/api/viewsets/data_viewset.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/onadata/apps/api/viewsets/data_viewset.py b/onadata/apps/api/viewsets/data_viewset.py index 18cadc0a49..e01cf87605 100644 --- a/onadata/apps/api/viewsets/data_viewset.py +++ b/onadata/apps/api/viewsets/data_viewset.py @@ -603,8 +603,12 @@ def list(self, request, *args, **kwargs): return super().list(request, *args, **kwargs) if export_type == "geojson": + # filter by instances with geometries + instances_with_geoms = self.object_list.filter( + xform__instances_with_geopoints=True) + # add pagination when fetching geojson features - page = self.paginate_queryset(self.object_list) + page = self.paginate_queryset(instances_with_geoms) serializer = self.get_serializer(page, many=True) return Response(serializer.data) From 0c45b8516f4658273e2ecaa24cb9825dd2d39aeb Mon Sep 17 00:00:00 2001 From: Kipchirchir Sigei Date: Wed, 16 Nov 2022 08:25:50 +0300 Subject: [PATCH 2/8] Add tests Signed-off-by: Kipchirchir Sigei --- .../api/tests/viewsets/test_data_viewset.py | 35 +++++++++++++++++++ .../GeoLocationForm_empty_geoms.csv | 3 ++ onadata/apps/main/tests/test_base.py | 6 ++-- 3 files changed, 42 insertions(+), 2 deletions(-) create mode 100644 onadata/apps/main/tests/fixtures/geolocation/GeoLocationForm_empty_geoms.csv diff --git a/onadata/apps/api/tests/viewsets/test_data_viewset.py b/onadata/apps/api/tests/viewsets/test_data_viewset.py index 4e63fe0ceb..ff21d5caf6 100644 --- a/onadata/apps/api/tests/viewsets/test_data_viewset.py +++ b/onadata/apps/api/tests/viewsets/test_data_viewset.py @@ -2110,6 +2110,41 @@ def test_geojson_format(self): } self.assertEqual(response.status_code, 200) self.assertEqual(response.data, data) + + def test_instances_with_geopoints(self): + # publist sample geo submissions + self._publish_submit_geojson() + + view = DataViewSet.as_view({"get": "list"}) + request = self.factory.get("/", **self.extra) + response = view(request, pk=self.xform.pk, format="geojson") + + self.assertEqual(response.status_code, 200) + self.assertEqual(self.xform.instances.count(), 4) + self.assertEqual(len(response.data["features"]), 4) + + # check if instances_with_geopoints is True for the form + self.xform.refresh_from_db() + self.assertTrue(self.xform.instances_with_geopoints) + + def test_instances_with_empty_geopoints(self): + # publist sample geo submissions + self._publish_submit_geojson(has_empty_geoms=True) + + view = DataViewSet.as_view({"get": "list"}) + request = self.factory.get("/", **self.extra) + response = view(request, pk=self.xform.pk, format="geojson") + + self.assertEqual(response.status_code, 200) + self.assertEqual(self.xform.instances.count(), 2) + + data = {"type": "FeatureCollection", "features": []} + self.assertEqual(response.data, data) + self.assertEqual(len(response.data["features"]), 0) + + # check if instances_with_geopoints is True for the form + self.xform.refresh_from_db() + self.assertFalse(self.xform.instances_with_geopoints) @patch("onadata.apps.api.viewsets.data_viewset" ".DataViewSet.paginate_queryset") def test_retry_on_operational_error(self, mock_paginate_queryset): diff --git a/onadata/apps/main/tests/fixtures/geolocation/GeoLocationForm_empty_geoms.csv b/onadata/apps/main/tests/fixtures/geolocation/GeoLocationForm_empty_geoms.csv new file mode 100644 index 0000000000..6aca44904f --- /dev/null +++ b/onadata/apps/main/tests/fixtures/geolocation/GeoLocationForm_empty_geoms.csv @@ -0,0 +1,3 @@ +today,location,_location_latitude,_location_longitude,_location_altitude,_location_precision,path,shape,meta/instanceID,_uuid,_submission_time,_tags,_notes,_version,_duration,_submitted_by +2015-01-15,n/a,n/a,n/a,n/a,n/a,-1.294234 36.78722 0 30;-1.292286 36.787152 0 0;-1.295182 36.800439 0 0;-1.295289 36.802215 0 0;-1.286708 36.817235 0 0,-1.2942 36.78723 0 22;-1.292415 36.787356 0 0;-1.292586 36.779084 0 0;-1.29988 36.779546 0 0;-1.299666 36.788096 0 0;-1.2942 36.78723 0 22,uuid:7c0d8b2d-7ad0-4100-923d-f2767d532258,7c0d8b2d-7ad0-4100-923d-f2767d532258,2015-01-15T06:19:23,,,201501150616,, +2015-01-15,n/a,n/a,n/a,n/a,n/a,-1.294234 36.78722 0 30;-1.292286 36.787152 0 0;-1.295182 36.800439 0 0;-1.295289 36.802215 0 0;-1.286708 36.817235 0 0,-1.2942 36.78723 0 22;-1.292415 36.787356 0 0;-1.292586 36.779084 0 0;-1.29988 36.779546 0 0;-1.299666 36.788096 0 0;-1.2942 36.78723 0 22,uuid:7c0d8b2d-7ad0-4100-923d-f2767d532259,7c0d8b2d-7ad0-4100-923d-f2767d532259,2015-01-15T06:19:23,,,201501150616,, diff --git a/onadata/apps/main/tests/test_base.py b/onadata/apps/main/tests/test_base.py index 15e5766381..a638b61202 100644 --- a/onadata/apps/main/tests/test_base.py +++ b/onadata/apps/main/tests/test_base.py @@ -393,7 +393,7 @@ def _get_digest_client(self): client.set_authorization("bob", "bob", "Digest") return client - def _publish_submit_geojson(self): + def _publish_submit_geojson(self, has_empty_geoms=False): path = os.path.join( settings.PROJECT_ROOT, "apps", @@ -406,6 +406,8 @@ def _publish_submit_geojson(self): self._publish_xls_file_and_set_xform(path) + csv_sub = "empty_geoms" if has_empty_geoms else "2015_01_15_01_28_45" + view = XFormViewSet.as_view({"post": "csv_import"}) with open( os.path.join( @@ -415,7 +417,7 @@ def _publish_submit_geojson(self): "tests", "fixtures", "geolocation", - "GeoLocationForm_2015_01_15_01_28_45.csv", + f"GeoLocationForm_{csv_sub}.csv", ), encoding="utf-8", ) as csv_import: From e033ef1e67fb9854fe7768dffdd2aa728ac2d9dd Mon Sep 17 00:00:00 2001 From: Kipchirchir Sigei Date: Wed, 16 Nov 2022 11:19:59 +0300 Subject: [PATCH 3/8] Return 404 if all instances dont have geometries Signed-off-by: Kipchirchir Sigei --- onadata/apps/api/tests/viewsets/test_data_viewset.py | 9 +++------ onadata/apps/api/viewsets/data_viewset.py | 8 ++++---- 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/onadata/apps/api/tests/viewsets/test_data_viewset.py b/onadata/apps/api/tests/viewsets/test_data_viewset.py index ff21d5caf6..3ba23ce74f 100644 --- a/onadata/apps/api/tests/viewsets/test_data_viewset.py +++ b/onadata/apps/api/tests/viewsets/test_data_viewset.py @@ -2135,14 +2135,11 @@ def test_instances_with_empty_geopoints(self): request = self.factory.get("/", **self.extra) response = view(request, pk=self.xform.pk, format="geojson") - self.assertEqual(response.status_code, 200) + # return 404 if all instances dont have geoms + self.assertEqual(response.status_code, 404) self.assertEqual(self.xform.instances.count(), 2) - data = {"type": "FeatureCollection", "features": []} - self.assertEqual(response.data, data) - self.assertEqual(len(response.data["features"]), 0) - - # check if instances_with_geopoints is True for the form + # check if instances_with_geopoints is False for the form self.xform.refresh_from_db() self.assertFalse(self.xform.instances_with_geopoints) diff --git a/onadata/apps/api/viewsets/data_viewset.py b/onadata/apps/api/viewsets/data_viewset.py index e01cf87605..ff7f05ff0c 100644 --- a/onadata/apps/api/viewsets/data_viewset.py +++ b/onadata/apps/api/viewsets/data_viewset.py @@ -603,12 +603,12 @@ def list(self, request, *args, **kwargs): return super().list(request, *args, **kwargs) if export_type == "geojson": - # filter by instances with geometries - instances_with_geoms = self.object_list.filter( - xform__instances_with_geopoints=True) + # raise 404 if all instances dont have geoms + if not xform.instances_with_geopoints: + raise Http404(_("Not Found")) # add pagination when fetching geojson features - page = self.paginate_queryset(instances_with_geoms) + page = self.paginate_queryset(self.object_list) serializer = self.get_serializer(page, many=True) return Response(serializer.data) From 9d36be566124f5655d854bf2f50a89a1074be2bc Mon Sep 17 00:00:00 2001 From: Kipchirchir Sigei Date: Wed, 16 Nov 2022 16:29:08 +0300 Subject: [PATCH 4/8] Exclude deleted submissions when updating instances with geoms Signed-off-by: Kipchirchir Sigei --- .../api/tests/viewsets/test_data_viewset.py | 25 ++++++++++++++++--- onadata/apps/api/viewsets/data_viewset.py | 11 ++++++++ .../GeoLocationForm_empty_geoms.csv | 2 +- 3 files changed, 34 insertions(+), 4 deletions(-) diff --git a/onadata/apps/api/tests/viewsets/test_data_viewset.py b/onadata/apps/api/tests/viewsets/test_data_viewset.py index 3ba23ce74f..871ca645d1 100644 --- a/onadata/apps/api/tests/viewsets/test_data_viewset.py +++ b/onadata/apps/api/tests/viewsets/test_data_viewset.py @@ -2112,7 +2112,7 @@ def test_geojson_format(self): self.assertEqual(response.data, data) def test_instances_with_geopoints(self): - # publist sample geo submissions + # publish sample geo submissions self._publish_submit_geojson() view = DataViewSet.as_view({"get": "list"}) @@ -2128,14 +2128,33 @@ def test_instances_with_geopoints(self): self.assertTrue(self.xform.instances_with_geopoints) def test_instances_with_empty_geopoints(self): - # publist sample geo submissions + # publish sample geo submissions self._publish_submit_geojson(has_empty_geoms=True) - view = DataViewSet.as_view({"get": "list"}) + view = DataViewSet.as_view({"delete": "destroy", "get": "list"}) request = self.factory.get("/", **self.extra) response = view(request, pk=self.xform.pk, format="geojson") + # should return 200 if it has atleast one valid geom + self.assertEqual(response.status_code, 200) + self.assertEqual(self.xform.instances.count(), 2) + + # check if instances_with_geopoints is False for the form + self.xform.refresh_from_db() + self.assertTrue(self.xform.instances_with_geopoints) + + # soft delete instance with geoms + dataid = self.xform.instances.all().order_by("id")[0].pk + request = self.factory.delete("/", **self.extra) + response = view(request, pk=self.xform.pk, dataid=dataid) + + # get the soft deleted instance + first_xform_instance = self.xform.instances.get(pk=dataid) + self.assertEqual(first_xform_instance.deleted_by, request.user) + # return 404 if all instances dont have geoms + request = self.factory.get("/", **self.extra) + response = view(request, pk=self.xform.pk, format="geojson") self.assertEqual(response.status_code, 404) self.assertEqual(self.xform.instances.count(), 2) diff --git a/onadata/apps/api/viewsets/data_viewset.py b/onadata/apps/api/viewsets/data_viewset.py index ff7f05ff0c..788bc7829a 100644 --- a/onadata/apps/api/viewsets/data_viewset.py +++ b/onadata/apps/api/viewsets/data_viewset.py @@ -393,6 +393,17 @@ def destroy(self, request, *args, **kwargs): if request.user.has_perm(CAN_DELETE_SUBMISSION, self.object.xform): instance_id = self.object.pk delete_instance(self.object, request.user) + + # update xform if no instance has geoms + if ( + self.object.xform.instances.filter( + deleted_at__isnull=True, geom=None + ).count() + < 1 + ): + self.object.xform.instances_with_geopoints = False + self.object.xform.save() + # send message send_message( instance_id=instance_id, diff --git a/onadata/apps/main/tests/fixtures/geolocation/GeoLocationForm_empty_geoms.csv b/onadata/apps/main/tests/fixtures/geolocation/GeoLocationForm_empty_geoms.csv index 6aca44904f..60d8402ae4 100644 --- a/onadata/apps/main/tests/fixtures/geolocation/GeoLocationForm_empty_geoms.csv +++ b/onadata/apps/main/tests/fixtures/geolocation/GeoLocationForm_empty_geoms.csv @@ -1,3 +1,3 @@ today,location,_location_latitude,_location_longitude,_location_altitude,_location_precision,path,shape,meta/instanceID,_uuid,_submission_time,_tags,_notes,_version,_duration,_submitted_by -2015-01-15,n/a,n/a,n/a,n/a,n/a,-1.294234 36.78722 0 30;-1.292286 36.787152 0 0;-1.295182 36.800439 0 0;-1.295289 36.802215 0 0;-1.286708 36.817235 0 0,-1.2942 36.78723 0 22;-1.292415 36.787356 0 0;-1.292586 36.779084 0 0;-1.29988 36.779546 0 0;-1.299666 36.788096 0 0;-1.2942 36.78723 0 22,uuid:7c0d8b2d-7ad0-4100-923d-f2767d532258,7c0d8b2d-7ad0-4100-923d-f2767d532258,2015-01-15T06:19:23,,,201501150616,, +2015-01-15,-1.294197 36.787219 0 34,-1.294197,36.787219,0,34,-1.294234 36.78722 0 30;-1.292286 36.787152 0 0;-1.295182 36.800439 0 0;-1.295289 36.802215 0 0;-1.286708 36.817235 0 0,-1.2942 36.78723 0 22;-1.292415 36.787356 0 0;-1.292586 36.779084 0 0;-1.29988 36.779546 0 0;-1.299666 36.788096 0 0;-1.2942 36.78723 0 22,uuid:7c0d8b2d-7ad0-4100-923d-f2767d532258,7c0d8b2d-7ad0-4100-923d-f2767d532258,2015-01-15T06:19:23,,,201501150616,, 2015-01-15,n/a,n/a,n/a,n/a,n/a,-1.294234 36.78722 0 30;-1.292286 36.787152 0 0;-1.295182 36.800439 0 0;-1.295289 36.802215 0 0;-1.286708 36.817235 0 0,-1.2942 36.78723 0 22;-1.292415 36.787356 0 0;-1.292586 36.779084 0 0;-1.29988 36.779546 0 0;-1.299666 36.788096 0 0;-1.2942 36.78723 0 22,uuid:7c0d8b2d-7ad0-4100-923d-f2767d532259,7c0d8b2d-7ad0-4100-923d-f2767d532259,2015-01-15T06:19:23,,,201501150616,, From d6019903139274c93a22ab53797393d2ddc8e9b8 Mon Sep 17 00:00:00 2001 From: Kipchirchir Sigei Date: Mon, 21 Nov 2022 14:34:02 +0300 Subject: [PATCH 5/8] Update post_save signal to update XForm instances_with_geopoints Signed-off-by: Kipchirchir Sigei --- onadata/apps/api/tests/viewsets/test_data_viewset.py | 6 +++++- onadata/apps/api/viewsets/data_viewset.py | 10 ---------- onadata/apps/logger/models/instance.py | 12 +++++++++++- 3 files changed, 16 insertions(+), 12 deletions(-) diff --git a/onadata/apps/api/tests/viewsets/test_data_viewset.py b/onadata/apps/api/tests/viewsets/test_data_viewset.py index 871ca645d1..64c1445417 100644 --- a/onadata/apps/api/tests/viewsets/test_data_viewset.py +++ b/onadata/apps/api/tests/viewsets/test_data_viewset.py @@ -2127,7 +2127,8 @@ def test_instances_with_geopoints(self): self.xform.refresh_from_db() self.assertTrue(self.xform.instances_with_geopoints) - def test_instances_with_empty_geopoints(self): + @patch("onadata.apps.viewer.signals._post_process_submissions") + def test_instances_with_empty_geopoints(self, mock_signal): # publish sample geo submissions self._publish_submit_geojson(has_empty_geoms=True) @@ -2148,6 +2149,9 @@ def test_instances_with_empty_geopoints(self): request = self.factory.delete("/", **self.extra) response = view(request, pk=self.xform.pk, dataid=dataid) + # test that signal to update instances_with_geopoints is sent + self.assertTrue(mock_signal.called) + # get the soft deleted instance first_xform_instance = self.xform.instances.get(pk=dataid) self.assertEqual(first_xform_instance.deleted_by, request.user) diff --git a/onadata/apps/api/viewsets/data_viewset.py b/onadata/apps/api/viewsets/data_viewset.py index 788bc7829a..e2bb65b139 100644 --- a/onadata/apps/api/viewsets/data_viewset.py +++ b/onadata/apps/api/viewsets/data_viewset.py @@ -394,16 +394,6 @@ def destroy(self, request, *args, **kwargs): instance_id = self.object.pk delete_instance(self.object, request.user) - # update xform if no instance has geoms - if ( - self.object.xform.instances.filter( - deleted_at__isnull=True, geom=None - ).count() - < 1 - ): - self.object.xform.instances_with_geopoints = False - self.object.xform.save() - # send message send_message( instance_id=instance_id, diff --git a/onadata/apps/logger/models/instance.py b/onadata/apps/logger/models/instance.py index c228ac87ca..f2a889831b 100644 --- a/onadata/apps/logger/models/instance.py +++ b/onadata/apps/logger/models/instance.py @@ -794,11 +794,21 @@ def soft_delete_attachments(self, user=None): def post_save_submission(sender, instance=None, created=False, **kwargs): """Update XForm, Project, JSON field - - XForm submission coun + - XForm submission count & instances_with_geopoints field - Project date modified - Update the submission JSON field data """ if instance.deleted_at is not None: + # update xform if no instance has geoms + if ( + instance.xform.instances.filter( + deleted_at__isnull=True, geom=None + ).count() + < 1 + ): + instance.xform.instances_with_geopoints = False + instance.xform.save() + _update_submission_count_for_today( instance.xform_id, incr=False, date_created=instance.date_created ) From 4008f694cf0ba4f70dc8f57712a3272aeff65379 Mon Sep 17 00:00:00 2001 From: Kipchirchir Sigei Date: Tue, 22 Nov 2022 11:02:40 +0300 Subject: [PATCH 6/8] Cleanup Signed-off-by: Kipchirchir Sigei --- onadata/apps/logger/models/instance.py | 37 +++++++++++++------------- 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/onadata/apps/logger/models/instance.py b/onadata/apps/logger/models/instance.py index f2a889831b..e3827cc192 100644 --- a/onadata/apps/logger/models/instance.py +++ b/onadata/apps/logger/models/instance.py @@ -281,8 +281,8 @@ def update_xform_submission_count(instance_id, created): clear_project_cache(instance.xform.project_id) -# pylint: disable=unused-argument,invalid-name -def update_xform_submission_count_delete(sender, instance, **kwargs): +@transaction.atomic() +def _update_xform_submission_count_delete(instance): """Updates the XForm submissions count on deletion of a submission.""" try: xform = XForm.objects.select_for_update().get(pk=instance.xform.pk) @@ -316,9 +316,22 @@ def update_xform_submission_count_delete(sender, instance, **kwargs): safe_delete(f"{DATAVIEW_COUNT}{xform.pk}") safe_delete(f"{XFORM_COUNT}{xform.pk}") - if xform.instances.exclude(geom=None).count() < 1: - xform.instances_with_geopoints = False - xform.save() + # update xform if no instance has geoms + if ( + instance.xform.instances.filter( + deleted_at__isnull=True, geom=None + ).count() + < 1 + ): + instance.xform.instances_with_geopoints = False + instance.xform.save() + + +# pylint: disable=unused-argument,invalid-name +def update_xform_submission_count_delete(sender, instance, **kwargs): + """Updates the XForm submissions count on deletion of a submission.""" + if instance: + _update_xform_submission_count_delete(instance) @app.task(bind=True, max_retries=3) @@ -799,19 +812,7 @@ def post_save_submission(sender, instance=None, created=False, **kwargs): - Update the submission JSON field data """ if instance.deleted_at is not None: - # update xform if no instance has geoms - if ( - instance.xform.instances.filter( - deleted_at__isnull=True, geom=None - ).count() - < 1 - ): - instance.xform.instances_with_geopoints = False - instance.xform.save() - - _update_submission_count_for_today( - instance.xform_id, incr=False, date_created=instance.date_created - ) + _update_xform_submission_count_delete(instance) if ASYNC_POST_SUBMISSION_PROCESSING_ENABLED: update_xform_submission_count_async.apply_async(args=[instance.pk, created]) From 6f010e9fa3cdad821e61d2211840a01c2e21a9da Mon Sep 17 00:00:00 2001 From: Kipchirchir Sigei Date: Tue, 22 Nov 2022 17:17:17 +0300 Subject: [PATCH 7/8] Set geom field to None if instance doesnt have coords Signed-off-by: Kipchirchir Sigei --- onadata/apps/logger/models/instance.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/onadata/apps/logger/models/instance.py b/onadata/apps/logger/models/instance.py index e3827cc192..0d0a532380 100644 --- a/onadata/apps/logger/models/instance.py +++ b/onadata/apps/logger/models/instance.py @@ -319,8 +319,8 @@ def _update_xform_submission_count_delete(instance): # update xform if no instance has geoms if ( instance.xform.instances.filter( - deleted_at__isnull=True, geom=None - ).count() + deleted_at__isnull=True + ).exclude(geom=None).count() < 1 ): instance.xform.instances_with_geopoints = False @@ -463,7 +463,10 @@ def _set_geom(self): xform.save() # pylint: disable=attribute-defined-outside-init - self.geom = GeometryCollection(points) + if points: + self.geom = GeometryCollection(points) + else: + self.geom = None def _set_json(self): # pylint: disable=attribute-defined-outside-init From d987602b35ccdbe49e7c62f9ff302a29dcb3067e Mon Sep 17 00:00:00 2001 From: Kipchirchir Sigei Date: Tue, 22 Nov 2022 17:17:52 +0300 Subject: [PATCH 8/8] Add migration to update existing instances with correct geom values Signed-off-by: Kipchirchir Sigei --- .../migrations/0004_update_instance_geoms.py | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 onadata/apps/logger/migrations/0004_update_instance_geoms.py diff --git a/onadata/apps/logger/migrations/0004_update_instance_geoms.py b/onadata/apps/logger/migrations/0004_update_instance_geoms.py new file mode 100644 index 0000000000..a313561509 --- /dev/null +++ b/onadata/apps/logger/migrations/0004_update_instance_geoms.py @@ -0,0 +1,27 @@ +# Generated by Django 3.2.16 on 2022-11-22 14:06 + +from django.db import migrations +from onadata.apps.logger.models.instance import Instance + + +def update_instance_geoms(apps, schema_editor): + """ + Update instance geom field with valid geom values + """ + for inst in Instance.objects.filter( + deleted_at__isnull=True, + xform__downloadable=True, + xform__deleted_at__isnull=True, + ): + if inst.geom.empty: + inst.geom = None + inst.save() + + +class Migration(migrations.Migration): + + dependencies = [ + ('logger', '0003_alter_instance_media_all_received'), + ] + + operations = [migrations.RunPython(update_instance_geoms)]