From 05818dcf214e2a35072f7a9727d980cf7b725c59 Mon Sep 17 00:00:00 2001 From: FrankApiyo Date: Wed, 13 Nov 2024 12:24:58 +0300 Subject: [PATCH 1/3] Ensure geojson exports creation process is idempotent --- onadata/libs/tests/utils/test_export_tools.py | 76 +++++++++++++++++++ onadata/libs/utils/api_export_tools.py | 19 +++-- 2 files changed, 87 insertions(+), 8 deletions(-) diff --git a/onadata/libs/tests/utils/test_export_tools.py b/onadata/libs/tests/utils/test_export_tools.py index f621a5b557..744bcd698a 100644 --- a/onadata/libs/tests/utils/test_export_tools.py +++ b/onadata/libs/tests/utils/test_export_tools.py @@ -15,6 +15,7 @@ from django.core.files.storage import default_storage from django.core.files.temp import NamedTemporaryFile from django.test.utils import override_settings +from django.test import RequestFactory from django.utils import timezone from pyxform.builder import create_survey_from_xls @@ -23,8 +24,11 @@ from savReaderWriter import SavWriter from onadata.apps.api import tests as api_tests +from onadata.apps.main.models import MetaData +from django.contrib.contenttypes.models import ContentType from onadata.apps.api.tests.viewsets.test_abstract_viewset import TestAbstractViewSet from onadata.apps.api.viewsets.data_viewset import DataViewSet +from onadata.libs.utils.api_export_tools import custom_response_handler from onadata.apps.logger.models import Attachment, Instance, XForm, Entity, EntityList from onadata.apps.viewer.models.export import Export, GenericExport from onadata.apps.viewer.models.parsed_instance import query_fields_data @@ -273,6 +277,78 @@ def test_should_not_create_new_export_when_old_exists(self): self.assertFalse(will_create_new_export) + def test_should_not_create_new_export_when_old_exists(self): + export_type = "geojson" + self._publish_transportation_form_and_submit_instance() + + request = RequestFactory().get("/") + request.user = self.user + request.query_params = options = {} + metadata = MetaData.objects.create( + content_type=ContentType.objects.get_for_model(XForm), + data_type="media", + data_value=f"xform_geojson {self.xform.id} testgeojson", + extra_data={ + "data_title": "start", + "data_fields": "", + "data_geo_field": "qn09", + "data_simple_style": True, + }, + object_id=self.xform.id, + ) + _response = custom_response_handler( + request, + self.xform, + {}, + export_type, + filename="testgeojson", + dataview=False, + metadata=metadata, + ) + + self.assertEqual(1, Export.objects.filter(xform=self.xform).count()) + self.assertEqual( + { + "dataview_pk": False, + "include_hxl": True, + "include_images": True, + "include_labels": False, + "win_excel_utf8": False, + "group_delimiter": "/", + "include_reviews": False, + "remove_group_name": False, + "include_labels_only": False, + "split_select_multiples": True, + }, + Export.objects.get(xform=self.xform).options, + ) + _response = custom_response_handler( + request, + self.xform, + {}, + export_type, + filename="testgeojson", + dataview=False, + metadata=metadata, + ) + # we still have only one export, we didn't generate another + self.assertEqual(1, Export.objects.filter(xform=self.xform).count()) + self.assertEqual( + { + "dataview_pk": False, + "include_hxl": True, + "include_images": True, + "include_labels": False, + "win_excel_utf8": False, + "group_delimiter": "/", + "include_reviews": False, + "remove_group_name": False, + "include_labels_only": False, + "split_select_multiples": True, + }, + Export.objects.get(xform=self.xform).options, + ) + def test_should_create_new_export_when_filter_defined(self): export_type = "csv" options = { diff --git a/onadata/libs/utils/api_export_tools.py b/onadata/libs/utils/api_export_tools.py index f29cfbea6c..dd78ac34f1 100644 --- a/onadata/libs/utils/api_export_tools.py +++ b/onadata/libs/utils/api_export_tools.py @@ -203,6 +203,7 @@ def _new_export(): export_type, dataview_pk=dataview_pk, metadata=metadata, + options=options, ) if should_create_new_export(xform, export_type, options, request=request): @@ -245,18 +246,20 @@ def _new_export(): def _generate_new_export( # noqa: C0901 - request, xform, query, export_type, dataview_pk=False, metadata=None + request, xform, query, export_type, dataview_pk=False, metadata=None, options={} ): query = _set_start_end_params(request, query) extension = _get_extension_from_export_type(export_type) - options = { - "extension": extension, - "username": xform.user.username, - "id_string": xform.id_string, - "host": request.get_host(), - "sort": request.query_params.get("sort"), - } + options.update( + { + "extension": extension, + "username": xform.user.username, + "id_string": xform.id_string, + "host": request.get_host(), + "sort": request.query_params.get("sort"), + } + ) if query: options["query"] = query From e4549cbd98ac1b8a5eeefe716b8279fef42ca366 Mon Sep 17 00:00:00 2001 From: FrankApiyo Date: Thu, 14 Nov 2024 16:03:05 +0300 Subject: [PATCH 2/3] Use None instead of a mutable dictionary as default to fn --- onadata/libs/utils/api_export_tools.py | 28 ++++++++++++++++---------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/onadata/libs/utils/api_export_tools.py b/onadata/libs/utils/api_export_tools.py index dd78ac34f1..4a52b5320f 100644 --- a/onadata/libs/utils/api_export_tools.py +++ b/onadata/libs/utils/api_export_tools.py @@ -203,7 +203,7 @@ def _new_export(): export_type, dataview_pk=dataview_pk, metadata=metadata, - options=options, + export_options=options, ) if should_create_new_export(xform, export_type, options, request=request): @@ -246,20 +246,26 @@ def _new_export(): def _generate_new_export( # noqa: C0901 - request, xform, query, export_type, dataview_pk=False, metadata=None, options={} + request, + xform, + query, + export_type, + dataview_pk=False, + metadata=None, + export_options=None, ): query = _set_start_end_params(request, query) extension = _get_extension_from_export_type(export_type) - options.update( - { - "extension": extension, - "username": xform.user.username, - "id_string": xform.id_string, - "host": request.get_host(), - "sort": request.query_params.get("sort"), - } - ) + options = { + "extension": extension, + "username": xform.user.username, + "id_string": xform.id_string, + "host": request.get_host(), + "sort": request.query_params.get("sort"), + } + if export_options is not None: + options.update(export_options) if query: options["query"] = query From bcb4c4da5c208d567ba97e63a661832bd42cb22d Mon Sep 17 00:00:00 2001 From: FrankApiyo Date: Thu, 14 Nov 2024 16:41:35 +0300 Subject: [PATCH 3/3] Check if export options is a dictionary instead of checking None --- onadata/libs/utils/api_export_tools.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/onadata/libs/utils/api_export_tools.py b/onadata/libs/utils/api_export_tools.py index 4a52b5320f..a49a3cce43 100644 --- a/onadata/libs/utils/api_export_tools.py +++ b/onadata/libs/utils/api_export_tools.py @@ -264,7 +264,7 @@ def _generate_new_export( # noqa: C0901 "host": request.get_host(), "sort": request.query_params.get("sort"), } - if export_options is not None: + if isinstance(export_options, dict): options.update(export_options) if query: options["query"] = query