From 90d2f8de8106f7d88bfa7f08010c02e6a0895b25 Mon Sep 17 00:00:00 2001 From: lindsay stevens Date: Fri, 19 Dec 2025 19:40:20 +1100 Subject: [PATCH] chg: remove warning for group (or loop) without label - xls2json.py: - replace "is" (identity) operator with "==" as they're not equivalent - since the label check now only applies to "repeats", specify that. - move type check to the first in the list to avoid checking other test expressions unnecessarily for group/loop - remove irrelevant membership check of aliases.label_optional_types since this only applies to questions - remove aliases.label_optional_types which is now unused - remove irrelevant check of "calculation" and "default" (via the _dynamic_default) since these columns are not applicable to repeats - add positive/negative tests for groups/loops with and without labels, translations, and appearance. The appearance check is to avoid a regression since appearance used to be a factor in the label check. - among the loop tests there's no cases for unlabelled choice-based groups since currently choice labels are required by the loop generation code in builder.py --- pyxform/aliases.py | 11 -- pyxform/xls2json.py | 11 +- tests/test_group.py | 196 +++++++++++++++++++++--------- tests/test_loop.py | 227 +++++++++++++++++++++++++++++++++-- tests/test_xlsform_spec.py | 2 - tests/xpath_helpers/group.py | 90 ++++++++++++++ 6 files changed, 451 insertions(+), 86 deletions(-) create mode 100644 tests/xpath_helpers/group.py diff --git a/pyxform/aliases.py b/pyxform/aliases.py index 017a5e14..ff440b2a 100644 --- a/pyxform/aliases.py +++ b/pyxform/aliases.py @@ -148,17 +148,6 @@ "FALSE": False, "false()": False, } -label_optional_types = [ - "calculate", - "deviceid", - "end", - "phonenumber", - "simserial", - "start", - "start-geopoint", - "today", - "username", -] osm = {"osm": constants.OSM_TYPE} BINDING_CONVERSIONS = { "yes": "true()", diff --git a/pyxform/xls2json.py b/pyxform/xls2json.py index 2bac009f..043ba36b 100644 --- a/pyxform/xls2json.py +++ b/pyxform/xls2json.py @@ -836,16 +836,9 @@ def workbook_to_json( # There isn't an easy and neat place to put this besides here. # Could potentially be simplified for control item cases. if ( - constants.LABEL not in row + control_type == constants.REPEAT + and constants.LABEL not in row and row.get(constants.MEDIA) is None - and question_type not in aliases.label_optional_types - and not row.get("bind", {}).get("calculate") - and not row.get("_dynamic_default", False) - and not ( - control_type is constants.GROUP - and row.get("control", {}).get("appearance") - == constants.FIELD_LIST - ) ): # Row number, name, and type probably enough for user message. # Also means the error message text is stable for tests. diff --git a/tests/test_group.py b/tests/test_group.py index afec55f2..af54d4b2 100644 --- a/tests/test_group.py +++ b/tests/test_group.py @@ -10,6 +10,7 @@ from pyxform.xls2xform import convert from tests.pyxform_test_case import PyxformTestCase +from tests.xpath_helpers.group import xpg class TestGroupOutput(PyxformTestCase): @@ -113,6 +114,144 @@ def test_table_list_appearance(self): xml__contains=[xml_contains], ) + def test_group__label__ok(self): + """Should find a group control with a child `label` element and no warnings.""" + md = """ + | survey | + | | type | name | label | + | | begin_group | g1 | G1 | + | | text | q1 | Q1 | + | | end_group | | | + """ + self.assertPyxformXform( + md=md, + warnings_count=0, + xml__xpath_match=[ + xpg.group_label_no_translation("/test_name/g1", "G1"), + ], + ) + + def test_group__no_label__ok(self): + """Should find a group control with no child `label` element and no warnings.""" + md = """ + | survey | + | | type | name | label | + | | begin_group | g1 | | + | | text | q1 | Q1 | + | | end_group | | | + """ + self.assertPyxformXform( + md=md, + warnings_count=0, + xml__xpath_match=[ + xpg.group_no_label("/test_name/g1"), + ], + ) + + def test_group__label__translated__ok(self): + """Should find a group control with a child `label` element and no warnings.""" + md = """ + | survey | + | | type | name | label::English (en) | + | | begin_group | g1 | G1 | + | | text | q1 | Q1 | + | | end_group | | | + """ + self.assertPyxformXform( + md=md, + warnings_count=0, + xml__xpath_match=[ + xpg.group_label_translation("/test_name/g1"), + ], + ) + + def test_group__no_label__translated__ok(self): + """Should find a group control with no child `label` element and no warnings.""" + md = """ + | survey | + | | type | name | label::English (en) | + | | begin_group | g1 | | + | | text | q1 | Q1 | + | | end_group | | | + """ + self.assertPyxformXform( + md=md, + warnings_count=0, + xml__xpath_match=[ + xpg.group_no_label("/test_name/g1"), + ], + ) + + def test_group__label__appearance__ok(self): + """Should find a group control with a child `label` element and no warnings.""" + md = """ + | survey | + | | type | name | label | appearance | + | | begin_group | g1 | G1 | field-list | + | | text | q1 | Q1 | | + | | end_group | | | | + """ + self.assertPyxformXform( + md=md, + warnings_count=0, + xml__xpath_match=[ + xpg.group_label_no_translation_appearance( + "/test_name/g1", "G1", "field-list" + ), + ], + ) + + def test_group__no_label__appearance__ok(self): + """Should find a group control with no child `label` element and no warnings.""" + md = """ + | survey | + | | type | name | label | appearance | + | | begin_group | g1 | | field-list | + | | text | q1 | Q1 | | + | | end_group | | | | + """ + self.assertPyxformXform( + md=md, + warnings_count=0, + xml__xpath_match=[ + xpg.group_no_label_appearance("/test_name/g1", "field-list"), + ], + ) + + def test_group__label__translated__appearance__ok(self): + """Should find a group control with a child `label` element and no warnings.""" + md = """ + | survey | + | | type | name | label::English (en) | appearance | + | | begin_group | g1 | G1 | field-list | + | | text | q1 | Q1 | | + | | end_group | | | | + """ + self.assertPyxformXform( + md=md, + warnings_count=0, + xml__xpath_match=[ + xpg.group_label_translation_appearance("/test_name/g1", "field-list"), + ], + ) + + def test_group__no_label__translated__appearance__ok(self): + """Should find a group control with no child `label` element and no warnings.""" + md = """ + | survey | + | | type | name | label::English (en) | appearance | + | | begin_group | g1 | | field-list | + | | text | q1 | Q1 | | + | | end_group | | | | + """ + self.assertPyxformXform( + md=md, + warnings_count=0, + xml__xpath_match=[ + xpg.group_no_label_appearance("/test_name/g1", "field-list"), + ], + ) + class TestGroupParsing(PyxformTestCase): def test_names__group_basic_case__ok(self): @@ -643,63 +782,6 @@ def test_empty_group__no_question_control__error(self): ], ) - def test_unlabeled_group(self): - self.assertPyxformXform( - md=""" - | survey | - | | type | name | label | - | | begin_group | my-group | | - | | text | my-text | my-text | - | | end_group | | | - """, - warnings_count=1, - warnings__contains=["[row : 2] Group has no label"], - ) - - def test_unlabeled_group_alternate_syntax(self): - self.assertPyxformXform( - md=""" - | survey | - | | type | name | label::English (en) | - | | begin group | my-group | | - | | text | my-text | my-text | - | | end group | | | - """, - warnings_count=1, - warnings__contains=["[row : 2] Group has no label"], - ) - - def test_unlabeled_group_fieldlist(self): - self.assertPyxformXform( - md=""" - | survey | - | | type | name | label | appearance | - | | begin_group | my-group | | field-list | - | | text | my-text | my-text | | - | | end_group | | | | - """, - warnings_count=0, - xml__xpath_match=[ - """ - /h:html/h:body/x:group[ - @ref = '/test_name/my-group' and @appearance='field-list' - ] - """ - ], - ) - - def test_unlabeled_group_fieldlist_alternate_syntax(self): - self.assertPyxformXform( - md=""" - | survey | - | | type | name | label::English (en) | appearance | - | | begin group | my-group | | field-list | - | | text | my-text | my-text | | - | | end group | | | | - """, - warnings_count=0, - ) - class TestGroupInternalRepresentations(TestCase): maxDiff = None diff --git a/tests/test_loop.py b/tests/test_loop.py index 9dd0e8ba..75c15567 100644 --- a/tests/test_loop.py +++ b/tests/test_loop.py @@ -8,10 +8,11 @@ from pyxform.xls2xform import convert from tests.pyxform_test_case import PyxformTestCase +from tests.xpath_helpers.group import xpg from tests.xpath_helpers.questions import xpq -class TestLoop(PyxformTestCase): +class TestLoopOutput(PyxformTestCase): """ A 'loop' is a type of group that, for each choice in the referenced choice list, generates grouped set of questions using the questions inside the loop definition. @@ -24,10 +25,10 @@ def test_loop(self): md = """ | survey | | | type | name | label | - | | begin loop over c1 | l1 | | + | | begin_loop over c1 | l1 | | | | integer | q1 | Age | | | select_one c2 | q2 | Size of %(label)s | - | | end loop | | | + | | end_loop | | | | choices | | | list_name | name | label | @@ -96,12 +97,12 @@ def test_loop__groups_error(self): md = """ | survey | | | type | name | label | - | | begin loop over c1 | l1 | | - | | begin group | g1 | | + | | begin_loop over c1 | l1 | | + | | begin_group | g1 | | | | integer | q1 | Age | | | select_one c2 | q2 | Size of %(label)s | - | | end group | | | - | | end loop | | | + | | end_group | | | + | | end_loop | | | | choices | | | list_name | name | label | @@ -170,6 +171,218 @@ def test_loop__groups_error(self): ], ) + def test_loop__label__ok(self): + """Should find a group control with a child `label` element and no warnings.""" + md = """ + | survey | + | | type | name | label | + | | begin_loop over c1 | g1 | G1 | + | | text | q1 | Q1 | + | | end_loop | | | + + | choices | + | | list_name | name | label | + | | c1 | n1 | N1 | + """ + self.assertPyxformXform( + md=md, + warnings_count=0, + xml__xpath_match=[ + # Primary loop group. + xpg.group_label_no_translation("/test_name/g1", "G1"), + # Choice loop group. + xpg.group_label_no_translation( + "/test_name/g1/n1", "N1", "/x:group[@ref='/test_name/g1']" + ), + ], + ) + + def test_loop__no_label__ok(self): + """Should find a group control with no child `label` element and no warnings.""" + md = """ + | survey | + | | type | name | label | + | | begin_loop over c1 | g1 | | + | | text | q1 | Q1 | + | | end_loop | | | + + | choices | + | | list_name | name | label | + | | c1 | n1 | N1 | + """ + self.assertPyxformXform( + md=md, + warnings_count=0, + xml__xpath_match=[ + # Primary loop group. + xpg.group_no_label("/test_name/g1"), + # Choice loop group. + xpg.group_label_no_translation( + "/test_name/g1/n1", "N1", "/x:group[@ref='/test_name/g1']" + ), + ], + ) + + def test_loop__label__translated__ok(self): + """Should find a group control with a child `label` element and no warnings.""" + md = """ + | survey | + | | type | name | label::English (en) | + | | begin_loop over c1 | g1 | G1 | + | | text | q1 | Q1 | + | | end_loop | | | + + | choices | + | | list_name | name | label::English (en) | + | | c1 | n1 | N1 | + """ + self.assertPyxformXform( + md=md, + warnings_count=0, + xml__xpath_match=[ + # Primary loop group. + xpg.group_label_translation("/test_name/g1"), + # Choice loop group. + xpg.group_label_translation( + "/test_name/g1/n1", "/x:group[@ref='/test_name/g1']" + ), + ], + ) + + def test_loop__no_label__translated__ok(self): + """Should find a group control with no child `label` element and no warnings.""" + md = """ + | survey | + | | type | name | label::English (en) | + | | begin_loop over c1 | g1 | | + | | text | q1 | Q1 | + | | end_loop | | | + + | choices | + | | list_name | name | label::English (en) | + | | c1 | n1 | N1 | + """ + self.assertPyxformXform( + md=md, + warnings_count=0, + xml__xpath_match=[ + # Primary loop group. + xpg.group_no_label("/test_name/g1"), + # Choice loop group. + xpg.group_label_translation( + "/test_name/g1/n1", "/x:group[@ref='/test_name/g1']" + ), + ], + ) + + def test_loop__label__appearance__ok(self): + """Should find a group control with a child `label` element and no warnings.""" + md = """ + | survey | + | | type | name | label | appearance | + | | begin_loop over c1 | g1 | G1 | field-list | + | | text | q1 | Q1 | | + | | end_loop | | | | + + | choices | + | | list_name | name | label | + | | c1 | n1 | N1 | + """ + self.assertPyxformXform( + md=md, + warnings_count=0, + xml__xpath_match=[ + # Primary loop group. + xpg.group_label_no_translation_appearance( + "/test_name/g1", "G1", "field-list" + ), + # Choice loop group. + xpg.group_label_no_translation( + "/test_name/g1/n1", "N1", "/x:group[@ref='/test_name/g1']" + ), + ], + ) + + def test_loop__no_label__appearance__ok(self): + """Should find a group control with no child `label` element and no warnings.""" + md = """ + | survey | + | | type | name | label | appearance | + | | begin_loop over c1 | g1 | | field-list | + | | text | q1 | Q1 | | + | | end_loop | | | | + + | choices | + | | list_name | name | label | + | | c1 | n1 | N1 | + """ + self.assertPyxformXform( + md=md, + warnings_count=0, + xml__xpath_match=[ + # Primary loop group. + xpg.group_no_label_appearance("/test_name/g1", "field-list"), + # Choice loop group. + xpg.group_label_no_translation( + "/test_name/g1/n1", "N1", "/x:group[@ref='/test_name/g1']" + ), + ], + ) + + def test_loop__label__translated__appearance__ok(self): + """Should find a group control with a child `label` element and no warnings.""" + md = """ + | survey | + | | type | name | label::English (en) | appearance | + | | begin_loop over c1 | g1 | G1 | field-list | + | | text | q1 | Q1 | | + | | end_loop | | | | + + | choices | + | | list_name | name | label::English (en) | + | | c1 | n1 | N1 | + """ + self.assertPyxformXform( + md=md, + warnings_count=0, + xml__xpath_match=[ + # Primary loop group. + xpg.group_label_translation_appearance("/test_name/g1", "field-list"), + # Choice loop group. + xpg.group_label_translation( + "/test_name/g1/n1", "/x:group[@ref='/test_name/g1']" + ), + ], + ) + + def test_loop__no_label__translated__appearance__ok(self): + """Should find a group control with no child `label` element and no warnings.""" + md = """ + | survey | + | | type | name | label::English (en) | appearance | + | | begin_loop over c1 | g1 | | field-list | + | | text | q1 | Q1 | | + | | end_loop | | | | + + | choices | + | | list_name | name | label::English (en) | + | | c1 | n1 | N1 | + """ + self.assertPyxformXform( + md=md, + warnings_count=0, + xml__xpath_match=[ + # Primary loop group. + xpg.group_no_label_appearance("/test_name/g1", "field-list"), + # Choice loop group. + xpg.group_label_translation( + "/test_name/g1/n1", "/x:group[@ref='/test_name/g1']" + ), + ], + ) + + +class TestLoopParsing(PyxformTestCase): def test_loop__repeats_error(self): """Should find that using a repeat in a loop results in an error.""" md = """ diff --git a/tests/test_xlsform_spec.py b/tests/test_xlsform_spec.py index 891c591b..cbebed6a 100644 --- a/tests/test_xlsform_spec.py +++ b/tests/test_xlsform_spec.py @@ -68,8 +68,6 @@ def test_warnings__count(self): vc.INVALID_LABEL.format(row=6), vc.INVALID_LABEL.format(row=7), "[row : 9] Repeat has no label: {'name': 'repeat_test', 'type': 'begin repeat'}", - "[row : 10] Group has no label: {'name': 'group_test', 'type': 'begin group'}", - "[row : 17] Group has no label: {'name': 'name', 'type': 'begin group'}", "[row : 28] Use the max-pixels parameter to speed up submission " + "sending and save storage space. Learn more: https://xlsform.org/#image", ] diff --git a/tests/xpath_helpers/group.py b/tests/xpath_helpers/group.py new file mode 100644 index 00000000..a7292952 --- /dev/null +++ b/tests/xpath_helpers/group.py @@ -0,0 +1,90 @@ +class XPathHelper: + """ + XPath expressions for settings assertions. + """ + + @staticmethod + def group_no_label(ref: str) -> str: + """There is a @ref, and no inline label.""" + return f""" + /h:html/h:body/x:group[ + @ref='{ref}' + and count(@*) = 1 + and not(./x:label) + ] + """ + + @staticmethod + def group_no_label_appearance(ref: str, appearance: str) -> str: + """There is a @ref and @appearance, and no inline label.""" + return f""" + /h:html/h:body/x:group[ + @ref='{ref}' + and @appearance='{appearance}' + and count(@*) = 2 + and not(./x:label) + ] + """ + + @staticmethod + def group_label_no_translation(ref: str, label: str, path: str = "") -> str: + """There is a @ref, and an inline label.""" + return f""" + /h:html/h:body{path}/x:group[ + @ref='{ref}' + and count(@*) = 1 + and ./x:label[ + not(@ref) + and text()='{label}' + ] + ] + """ + + @staticmethod + def group_label_translation(ref: str, path: str = "") -> str: + """There is a @ref, and a translated label.""" + return f""" + /h:html/h:body{path}/x:group[ + @ref='{ref}' + and count(@*) = 1 + and ./x:label[ + @ref="jr:itext('{ref}:label')" + and not(text()) + ] + ] + """ + + @staticmethod + def group_label_no_translation_appearance( + ref: str, label: str, appearance: str + ) -> str: + """There is a @ref and @appearance, and an inline label.""" + return f""" + /h:html/h:body/x:group[ + @ref='{ref}' + and @appearance='{appearance}' + and count(@*) = 2 + and ./x:label[ + not(@ref) + and text()='{label}' + ] + ] + """ + + @staticmethod + def group_label_translation_appearance(ref: str, appearance: str) -> str: + """There is a @ref and @appearance, and a translated label.""" + return f""" + /h:html/h:body/x:group[ + @ref='{ref}' + and @appearance='{appearance}' + and count(@*) = 2 + and ./x:label[ + @ref="jr:itext('{ref}:label')" + and not(text()) + ] + ] + """ + + +xpg = XPathHelper()