From 8692e30304daa27bbc04b3d870396e6963ba0b7c Mon Sep 17 00:00:00 2001 From: rajpatel24 Date: Wed, 2 Oct 2024 01:55:41 +0530 Subject: [PATCH 1/4] Enable support for name attribute in XLSForms to facilitate custom confirmation messages --- kobo/apps/openrosa/apps/viewer/models/data_dictionary.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/kobo/apps/openrosa/apps/viewer/models/data_dictionary.py b/kobo/apps/openrosa/apps/viewer/models/data_dictionary.py index 4048e82208..3d048c32b1 100644 --- a/kobo/apps/openrosa/apps/viewer/models/data_dictionary.py +++ b/kobo/apps/openrosa/apps/viewer/models/data_dictionary.py @@ -157,9 +157,8 @@ def add_instances(self): def save(self, *args, **kwargs): if self.xls: survey = create_survey_from_xls(self.xls) - survey.update({ - 'name': survey.id_string, - }) + if not survey.name or survey.name == 'None': + survey.name = survey.id_string self.json = survey.to_json() self.xml = survey.to_xml() self._mark_start_time_boolean() From 001425c2e57f7fb95dd80e91d2fc0dad8bcc7ef2 Mon Sep 17 00:00:00 2001 From: rajpatel24 Date: Tue, 15 Oct 2024 00:47:56 +0530 Subject: [PATCH 2/4] Add test for handling name setting in the asset content --- kpi/tests/test_assets.py | 84 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+) diff --git a/kpi/tests/test_assets.py b/kpi/tests/test_assets.py index 3506a04078..fe1311d08d 100644 --- a/kpi/tests/test_assets.py +++ b/kpi/tests/test_assets.py @@ -12,6 +12,8 @@ from rest_framework import serializers from kobo.apps.kobo_auth.shortcuts import User +from kobo.apps.openrosa.apps.logger.models import XForm +from kobo.apps.openrosa.apps.logger.xform_instance_parser import XFormInstanceParser from kpi.constants import ( ASSET_TYPE_SURVEY, ASSET_TYPE_COLLECTION, @@ -779,3 +781,85 @@ def test_anonymous_as_baseline_for_authenticated(self): for user_obj in AnonymousUser(), self.anotheruser: self.assertTrue(user_obj.has_perm( PERM_VIEW_ASSET, self.asset)) + + +class TestAssetNameSettingHandling(AssetsTestCase): + """ + Tests for the 'name' setting in the asset content + """ + def test_asset_name_matches_instance_root(self): + """ + Test if 'name' setting is provided, it should match with the root node. + """ + content = { + 'survey': [ + { + 'type': 'select_one', + 'label': 'q1', + 'select_from_list_name': 'iu0sl99', + }, + ], + 'choices': [ + {'name': 'a11', 'label': ['a11'], 'list_name': 'iu0sl99'}, + {'name': 'a3', 'label': ['a3'], 'list_name': 'iu0sl99'}, + ], + 'settings': {'name': 'custom_root_node_name'} + } + + # Create and deploy the asset + asset = Asset.objects.create( + owner=User.objects.get(username=self.user), + content=content, + asset_type=ASSET_TYPE_SURVEY + ) + asset.deploy(backend='mock', active=True) + + # Get the deployed XForm and parse it + xform = XForm.objects.get(id_string=asset.uid) + parser = XFormInstanceParser(xform.xml, xform.data_dictionary()) + + # Access the first child element of the node + instance_node = parser.get_root_node().getElementsByTagName('instance')[0] + root_element = instance_node.firstChild + + # Assert that the name setting matches the root node name + self.assertEqual(asset.content['settings']['name'], root_element.nodeName) + + def test_asset_without_name_setting(self): + """ + Test if 'name' setting is not provided, the root node should fall back + to xform.id_string + """ + content = { + 'survey': [ + { + 'type': 'select_one', + 'label': 'q1', + 'select_from_list_name': 'iu0sl99', + }, + ], + 'choices': [ + {'name': 'a11', 'label': ['a11'], 'list_name': 'iu0sl99'}, + {'name': 'a3', 'label': ['a3'], 'list_name': 'iu0sl99'}, + ], + # No 'name' setting provided in this case + } + + # Create and deploy the asset + asset = Asset.objects.create( + owner=User.objects.get(username=self.user), + content=content, + asset_type=ASSET_TYPE_SURVEY + ) + asset.deploy(backend='mock', active=True) + + # Get the deployed XForm and parse it + xform = XForm.objects.get(id_string=asset.uid) + parser = XFormInstanceParser(xform.xml, xform.data_dictionary()) + + # Access the first child element of the node + instance_node = parser.get_root_node().getElementsByTagName('instance')[0] + root_element = instance_node.firstChild + + # Assert that the root node name is the xform.id_sting + self.assertEqual(xform.id_string, root_element.nodeName) From 4c81d76be1d6d58f7b99b92c5d8935f0e0530469 Mon Sep 17 00:00:00 2001 From: rajpatel24 Date: Mon, 4 Nov 2024 15:00:32 +0530 Subject: [PATCH 3/4] Use unique placeholder for missing survey name and update tests --- .../apps/viewer/models/data_dictionary.py | 7 ++++-- kpi/constants.py | 2 ++ kpi/tests/test_assets.py | 24 ++++++------------- 3 files changed, 14 insertions(+), 19 deletions(-) diff --git a/kobo/apps/openrosa/apps/viewer/models/data_dictionary.py b/kobo/apps/openrosa/apps/viewer/models/data_dictionary.py index 3d048c32b1..b16fce0b95 100644 --- a/kobo/apps/openrosa/apps/viewer/models/data_dictionary.py +++ b/kobo/apps/openrosa/apps/viewer/models/data_dictionary.py @@ -20,6 +20,7 @@ DictOrganizer, ) from kobo.apps.openrosa.libs.utils.model_tools import queryset_iterator, set_uuid +from kpi.constants import DEFAULT_SURVEY_NAME class ColumnRename(models.Model): @@ -156,8 +157,10 @@ def add_instances(self): def save(self, *args, **kwargs): if self.xls: - survey = create_survey_from_xls(self.xls) - if not survey.name or survey.name == 'None': + survey = create_survey_from_xls( + self.xls, default_name=DEFAULT_SURVEY_NAME + ) + if survey.name == DEFAULT_SURVEY_NAME: survey.name = survey.id_string self.json = survey.to_json() self.xml = survey.to_xml() diff --git a/kpi/constants.py b/kpi/constants.py index 663f9c6ab8..3eb7d844c5 100644 --- a/kpi/constants.py +++ b/kpi/constants.py @@ -61,6 +61,8 @@ ASSET_TYPE_TEMPLATE: [ASSET_TYPE_SURVEY, ASSET_TYPE_TEMPLATE] } +DEFAULT_SURVEY_NAME = '__kobo_default_survey_name_value__' + ASSET_TYPE_ARG_NAME = 'asset_type' # List of app labels that need to read/write data from KoBoCAT database diff --git a/kpi/tests/test_assets.py b/kpi/tests/test_assets.py index fe1311d08d..3e38e2d74b 100644 --- a/kpi/tests/test_assets.py +++ b/kpi/tests/test_assets.py @@ -794,15 +794,10 @@ def test_asset_name_matches_instance_root(self): content = { 'survey': [ { - 'type': 'select_one', - 'label': 'q1', - 'select_from_list_name': 'iu0sl99', + 'text': 'select_one', + 'label': 'q1' }, ], - 'choices': [ - {'name': 'a11', 'label': ['a11'], 'list_name': 'iu0sl99'}, - {'name': 'a3', 'label': ['a3'], 'list_name': 'iu0sl99'}, - ], 'settings': {'name': 'custom_root_node_name'} } @@ -823,25 +818,20 @@ def test_asset_name_matches_instance_root(self): root_element = instance_node.firstChild # Assert that the name setting matches the root node name - self.assertEqual(asset.content['settings']['name'], root_element.nodeName) + assert root_element.nodeName == 'custom_root_node_name' def test_asset_without_name_setting(self): """ Test if 'name' setting is not provided, the root node should fall back - to xform.id_string + to asset UID """ content = { 'survey': [ { - 'type': 'select_one', + 'text': 'select_one', 'label': 'q1', - 'select_from_list_name': 'iu0sl99', }, ], - 'choices': [ - {'name': 'a11', 'label': ['a11'], 'list_name': 'iu0sl99'}, - {'name': 'a3', 'label': ['a3'], 'list_name': 'iu0sl99'}, - ], # No 'name' setting provided in this case } @@ -861,5 +851,5 @@ def test_asset_without_name_setting(self): instance_node = parser.get_root_node().getElementsByTagName('instance')[0] root_element = instance_node.firstChild - # Assert that the root node name is the xform.id_sting - self.assertEqual(xform.id_string, root_element.nodeName) + # Assert that the root node name is the asset.uid + assert root_element.nodeName == asset.uid From 66cff30119a20861b58a6e39a7bcaf3b6fd1bee2 Mon Sep 17 00:00:00 2001 From: "John N. Milner" Date: Mon, 4 Nov 2024 13:21:08 -0500 Subject: [PATCH 4/4] Correct question structure in unit test --- kpi/tests/test_assets.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/kpi/tests/test_assets.py b/kpi/tests/test_assets.py index 3e38e2d74b..23721c9cc0 100644 --- a/kpi/tests/test_assets.py +++ b/kpi/tests/test_assets.py @@ -794,8 +794,9 @@ def test_asset_name_matches_instance_root(self): content = { 'survey': [ { - 'text': 'select_one', - 'label': 'q1' + 'type': 'text', + 'name': 'some_text', + 'label': 'Enter some text', }, ], 'settings': {'name': 'custom_root_node_name'} @@ -828,8 +829,9 @@ def test_asset_without_name_setting(self): content = { 'survey': [ { - 'text': 'select_one', - 'label': 'q1', + 'type': 'text', + 'name': 'some_text', + 'label': 'Enter some text', }, ], # No 'name' setting provided in this case