From 24a51bf88ad4b1a458374f590437e115667534f4 Mon Sep 17 00:00:00 2001 From: rajpatel24 Date: Wed, 18 Sep 2024 23:03:52 +0530 Subject: [PATCH] Address PR feedback and improve submission flow --- .../openrosa/apps/logger/models/instance.py | 5 +- .../apps/logger/tests/test_form_submission.py | 3 +- .../viewer/tests/test_pandas_mongo_bridge.py | 12 +++-- .../tests/mixins/make_submission_mixin.py | 4 +- kobo/apps/openrosa/libs/utils/logger_tools.py | 28 +++++----- .../tests/test_submission_stream.py | 51 +++++++++++-------- kpi/tests/api/v2/test_api_submissions.py | 6 ++- 7 files changed, 61 insertions(+), 48 deletions(-) diff --git a/kobo/apps/openrosa/apps/logger/models/instance.py b/kobo/apps/openrosa/apps/logger/models/instance.py index 091e515dd4..7f2691c617 100644 --- a/kobo/apps/openrosa/apps/logger/models/instance.py +++ b/kobo/apps/openrosa/apps/logger/models/instance.py @@ -197,8 +197,9 @@ def _set_uuid(self): def _populate_root_uuid(self): if self.xml and not self.root_uuid: - assert (root_uuid := get_root_uuid_from_xml(self.xml)), \ - 'root_uuid should not be empty' + assert ( + root_uuid := get_root_uuid_from_xml(self.xml) + ), 'root_uuid should not be empty' self.root_uuid = root_uuid def _populate_xml_hash(self): diff --git a/kobo/apps/openrosa/apps/logger/tests/test_form_submission.py b/kobo/apps/openrosa/apps/logger/tests/test_form_submission.py index b9c0db12b4..6a35e6a806 100644 --- a/kobo/apps/openrosa/apps/logger/tests/test_form_submission.py +++ b/kobo/apps/openrosa/apps/logger/tests/test_form_submission.py @@ -244,8 +244,7 @@ def test_duplicate_submission_with_different_content(self): self.assertEqual(self.response.status_code, 201) self.assertEqual(Instance.objects.count(), pre_count + 1) inst = Instance.objects.order_by('pk').last() - with self.assertRaises(Exception): - self._make_submission(duplicate_xml_submission_file_path) + self._make_submission(duplicate_xml_submission_file_path, assert_success=False) self.assertEqual(self.response.status_code, 409) self.assertEqual(Instance.objects.count(), pre_count + 1) # this is exactly the same instance diff --git a/kobo/apps/openrosa/apps/viewer/tests/test_pandas_mongo_bridge.py b/kobo/apps/openrosa/apps/viewer/tests/test_pandas_mongo_bridge.py index c57d676828..0ec24e4635 100644 --- a/kobo/apps/openrosa/apps/viewer/tests/test_pandas_mongo_bridge.py +++ b/kobo/apps/openrosa/apps/viewer/tests/test_pandas_mongo_bridge.py @@ -62,12 +62,16 @@ def _submit_fixture_instance( if add_submission_uuid: xml_submission_file_path = ( self._add_submission_uuid_to_submission_xml( - xml_submission_file_path, self.xform + xml_submission_file_path ) ) - self._make_submission( - xml_submission_file_path, forced_submission_time=submission_time) - self.assertEqual(self.response.status_code, 201) + try: + self._make_submission( + xml_submission_file_path, forced_submission_time=submission_time) + self.assertEqual(self.response.status_code, 201) + finally: + if add_submission_uuid: + os.remove(xml_submission_file_path) def _publish_single_level_repeat_form(self): self._publish_xls_fixture_set_xform("new_repeats") diff --git a/kobo/apps/openrosa/libs/tests/mixins/make_submission_mixin.py b/kobo/apps/openrosa/libs/tests/mixins/make_submission_mixin.py index 3c24f9cd54..80e7f219ca 100644 --- a/kobo/apps/openrosa/libs/tests/mixins/make_submission_mixin.py +++ b/kobo/apps/openrosa/libs/tests/mixins/make_submission_mixin.py @@ -40,12 +40,12 @@ def _add_uuid_to_submission_xml(self, path, xform): return path - def _add_submission_uuid_to_submission_xml(self, path, xform): + def _add_submission_uuid_to_submission_xml(self, path): with open(path, 'rb') as _file: xml_content = _file.read().decode() # Find the closing tag of the root element (e.g., ) - closing_tag_index = xml_content.rfind('') + closing_tag_index = xml_content.rfind(f'') if closing_tag_index == -1: raise ValueError('Root element closing tag not found') diff --git a/kobo/apps/openrosa/libs/utils/logger_tools.py b/kobo/apps/openrosa/libs/utils/logger_tools.py index 9018ac3d2a..7d19b9b8d7 100644 --- a/kobo/apps/openrosa/libs/utils/logger_tools.py +++ b/kobo/apps/openrosa/libs/utils/logger_tools.py @@ -170,16 +170,13 @@ def create_instance( # get new and deprecated uuid's new_uuid = get_uuid_from_xml(xml) if not new_uuid: - raise InstanceIdMissingError() + raise InstanceIdMissingError with get_instance_lock(xml_hash, new_uuid, xform.id) as lock_acquired: if not lock_acquired: - raise DuplicateInstanceError() + raise DuplicateInstanceError - # XML matches are identified by identical content hash OR, when a - # content hash is not present, by string comparison of the full - # content, which is slow! Use the management command - # `populate_xml_hashes_for_instances` to hash existing submissions + # Check for an existing instance existing_instance = Instance.objects.filter( xml_hash=xml_hash, xform__user=xform.user, @@ -190,7 +187,7 @@ def create_instance( # ensure we have saved the extra attachments new_attachments, _ = save_attachments(existing_instance, media_files) if not new_attachments: - raise DuplicateInstanceError() + raise DuplicateInstanceError else: # Update Mongo via the related ParsedInstance existing_instance.parsed_instance.save(asynchronous=False) @@ -847,11 +844,8 @@ def _get_instance( """ # check if it is an edit submission old_uuid = get_deprecated_uuid_from_xml(xml) - instances = Instance.objects.filter(uuid=old_uuid) - - if instances: + if old_uuid and (instance := Instance.objects.filter(uuid=old_uuid).first()): # edits - instance = instances[0] check_edit_submission_permissions(request, xform) InstanceHistory.objects.create( xml=instance.xml, xform_instance=instance, uuid=old_uuid) @@ -860,8 +854,10 @@ def _get_instance( instance.uuid = new_uuid try: instance.save() - except IntegrityError: - raise ConflictingSubmissionUUIDError + except IntegrityError as e: + if 'root_uuid' in str(e): + raise ConflictingSubmissionUUIDError + raise else: submitted_by = ( get_database_user(request.user) @@ -882,8 +878,10 @@ def _get_instance( instance.defer_counting = True try: instance.save() - except IntegrityError: - raise ConflictingSubmissionUUIDError + except IntegrityError as e: + if 'root_uuid' in str(e): + raise ConflictingSubmissionUUIDError + raise return instance diff --git a/kobo/apps/subsequences/tests/test_submission_stream.py b/kobo/apps/subsequences/tests/test_submission_stream.py index 60268092e5..804fa54db0 100644 --- a/kobo/apps/subsequences/tests/test_submission_stream.py +++ b/kobo/apps/subsequences/tests/test_submission_stream.py @@ -1,5 +1,6 @@ import json from copy import deepcopy +from unittest.mock import patch from django.contrib.auth import get_user_model from django.test import TestCase @@ -258,31 +259,39 @@ def test_stream_with_extras_handles_duplicated_submission_uuids(self): '_uuid': '1c05898e-b43c-491d-814c-79595eb84e81', }, ] - with self.assertRaises(ConflictingSubmissionUUIDError): - self.asset.deployment.mock_submissions(submissions) - # Process submissions with extras - output = list( - stream_with_extras( - self.asset.deployment.get_submissions(user=self.asset.owner), - self.asset, + # Mock the get_submissions method to return the test submissions + with patch.object( + self.asset.deployment, + 'get_submissions', + return_value=submissions, + ): + # Expect a ConflictingSubmissionUUIDError due to duplicated UUIDs + with self.assertRaises(ConflictingSubmissionUUIDError): + self.asset.deployment.mock_submissions(submissions) + + # Process submissions with extras + output = list( + stream_with_extras( + self.asset.deployment.get_submissions(user=self.asset.owner), + self.asset, + ) ) - ) - # Make sure that uuid values for single or multiple choice qualitative - # analysis questions are kept as strings and not mutated - for submission in output: - supplemental_details = submission['_supplementalDetails'] - for qual_response in supplemental_details['Tell_me_a_story']['qual']: - if qual_response['type'] not in [ - 'qual_select_one', - 'qual_select_multiple', - ]: - # question is not a single or multiple choice one - continue + # Make sure that uuid values for single or multiple choice qualitative + # analysis questions are kept as strings and not mutated + for submission in output: + supplemental_details = submission['_supplementalDetails'] + for qual_response in supplemental_details['Tell_me_a_story']['qual']: + if qual_response['type'] not in [ + 'qual_select_one', + 'qual_select_multiple', + ]: + # question is not a single or multiple choice one + continue - for v in qual_response['val']: - assert isinstance(v['uuid'], str) + for v in qual_response['val']: + assert isinstance(v['uuid'], str) def test_stream_with_extras_ignores_empty_qual_responses(self): # Modify submission extras 'val' to be an empty string diff --git a/kpi/tests/api/v2/test_api_submissions.py b/kpi/tests/api/v2/test_api_submissions.py index fa61e61312..d2c91c9844 100644 --- a/kpi/tests/api/v2/test_api_submissions.py +++ b/kpi/tests/api/v2/test_api_submissions.py @@ -1595,8 +1595,10 @@ def test_edit_submission_with_xml_missing_uuids(self): submission_xml_root = fromstring_preserve_root_xmlns(submission_xml) assert submission_json['_id'] == submission['_id'] assert submission_xml_root.find('./find_this').text == 'hello!' - assert (submission_xml_root.find('./meta/instanceID').text == - 'uuid:9710c729-00a5-41f1-b740-8dd618bb4a49') + assert ( + submission_xml_root.find('./meta/instanceID').text + == 'uuid:9710c729-00a5-41f1-b740-8dd618bb4a49' + ) assert submission_xml_root.find('./formhub/uuid') is None # Get edit endpoint