Address PR feedback and improve submission flow

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):

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

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")

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., </new_repeats>)
-        closing_tag_index = xml_content.rfind('</new_repeats>')
+        closing_tag_index = xml_content.rfind(f'</{self.xform.id_string}>')
 
         if closing_tag_index == -1:
             raise ValueError('Root element closing tag not found')

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
 
 

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

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