diff --git a/pyxform/survey_element.py b/pyxform/survey_element.py
index 633bd9d14..0b2ab1af5 100644
--- a/pyxform/survey_element.py
+++ b/pyxform/survey_element.py
@@ -17,6 +17,7 @@
default_is_dynamic,
)
from pyxform.xls2json import print_pyobj_to_json
+from typing import TYPE_CHECKING
try:
from functools import lru_cache
@@ -24,6 +25,11 @@
from functools32 import lru_cache
+if TYPE_CHECKING:
+ from pyxform.utils import DetachableElement
+ from typing import List
+
+
def _overlay(over, under):
if type(under) == dict:
result = under.copy()
@@ -414,19 +420,27 @@ def xml_hint(self):
)
return node("hint", hint, toParseString=output_inserted)
- def xml_label_and_hint(self):
+ def xml_label_and_hint(self) -> "List[DetachableElement]":
"""
Return a list containing one node for the label and if there
is a hint one node for the hint.
"""
result = []
+ label_appended = False
if self.label or self.media:
result.append(self.xml_label())
+ label_appended = True
+
if self.hint or self.guidance_hint:
+ if not label_appended:
+ result.append(self.xml_label())
result.append(self.xml_hint())
- if len(result) == 0 or self.guidance_hint and len(result) == 1:
- msg = "The survey element named '%s' " "has no label or hint." % self.name
+ msg = "The survey element named '%s' " "has no label or hint." % self.name
+ if len(result) == 0:
+ raise PyXFormError(msg)
+ # Guidance hint alone is not OK since they may be hidden by default.
+ if not any((self.label, self.media, self.hint)) and self.guidance_hint:
raise PyXFormError(msg)
return result
diff --git a/pyxform/tests/example_xls/warnings.xls b/pyxform/tests/example_xls/warnings.xls
deleted file mode 100644
index 1ad213da9..000000000
Binary files a/pyxform/tests/example_xls/warnings.xls and /dev/null differ
diff --git a/pyxform/tests/utils.py b/pyxform/tests/utils.py
index 32eed2caf..cca4574c9 100644
--- a/pyxform/tests/utils.py
+++ b/pyxform/tests/utils.py
@@ -7,6 +7,8 @@
from formencode.doctest_xml_compare import xml_compare
from unittest2 import TestCase
+import textwrap
+from typing import TYPE_CHECKING
from pyxform import file_utils
from pyxform.builder import create_survey, create_survey_from_path
@@ -18,6 +20,11 @@
except ImportError:
import configparser
+
+if TYPE_CHECKING:
+ from typing import Tuple
+
+
DIR = os.path.dirname(__file__)
@@ -127,3 +134,9 @@ def prep_class_config(cls, test_dir="tests"):
strings = os.path.join(root, test_dir, "fixtures", "strings.ini")
cls.config.read(strings)
cls.cls_name = cls.__name__
+
+
+def prep_for_xml_contains(text: str) -> "Tuple[str]":
+ """Prep string for finding an exact match to formatted XML text."""
+ # noinspection PyRedundantParentheses
+ return (textwrap.indent(textwrap.dedent(text), " ",),)
diff --git a/pyxform/tests/xlsform_spec_test.py b/pyxform/tests/xlsform_spec_test.py
index 1833a10b0..f59ad9374 100644
--- a/pyxform/tests/xlsform_spec_test.py
+++ b/pyxform/tests/xlsform_spec_test.py
@@ -86,24 +86,6 @@ def runTest(self):
self.assertXFormEqual(expected_file.read(), actual_file.read())
-class WarningsTest(unittest.TestCase):
- """
- Just checks that the number of warnings thrown when reading warnings.xls
- doesn't change
- """
-
- def runTest(self):
- filename = "warnings.xls"
- path_to_excel_file = os.path.join(DIR, "example_xls", filename)
- warnings = []
- pyxform.xls2json.parse_file_to_json(
- path_to_excel_file, default_name="warnings", warnings=warnings
- )
- self.assertEquals(
- len(warnings), 22, "Found " + str(len(warnings)) + " warnings"
- )
-
-
class CalculateWithoutCalculationTest(unittest.TestCase):
"""
Just checks that calculate field without calculation raises a PyXFormError.
diff --git a/pyxform/tests_v1/pyxform_test_case.py b/pyxform/tests_v1/pyxform_test_case.py
index ce821ab97..7cea1490a 100644
--- a/pyxform/tests_v1/pyxform_test_case.py
+++ b/pyxform/tests_v1/pyxform_test_case.py
@@ -153,10 +153,12 @@ def assertPyxformXform(self, **kwargs):
* title: (str)
* run_odk_validate: (bool) when True, runs ODK Validate process
Default value = False because it slows down tests
+ * warnings: (list) a list to use for storing warnings for inspection.
"""
debug = kwargs.get("debug", False)
expecting_invalid_survey = kwargs.get("errored", False)
errors = []
+ warnings = kwargs.get("warnings", [])
xml_nodes = {}
run_odk_validate = kwargs.get("run_odk_validate", False)
@@ -165,7 +167,9 @@ def assertPyxformXform(self, **kwargs):
try:
if "md" in kwargs.keys():
kwargs = self._autoname_inputs(kwargs)
- survey = self.md_to_pyxform_survey(kwargs.get("md"), kwargs)
+ survey = self.md_to_pyxform_survey(
+ kwargs.get("md"), kwargs, warnings=warnings,
+ )
elif "ss_structure" in kwargs.keys():
kwargs = self._autoname_inputs(kwargs)
survey = self._ss_structure_to_pyxform_survey(
diff --git a/pyxform/tests_v1/test_guidance_hint.py b/pyxform/tests_v1/test_guidance_hint.py
index 0812e6515..763a8e6e6 100644
--- a/pyxform/tests_v1/test_guidance_hint.py
+++ b/pyxform/tests_v1/test_guidance_hint.py
@@ -75,12 +75,12 @@ def test_guidance_hint_only(self):
"""Test guidance_hint only."""
self.assertPyxformXform(
name="data",
- errored=True,
md="""
- | survey | | | |
- | | type | name | guidance_hint |
- | | string | name | as shown on birth certificate|
+ | survey | | | |
+ | | type | name | guidance_hint |
+ | | string | name | as shown on birth certificate |
""",
+ errored=True,
error__contains=["The survey element named 'name' has no label or hint."],
)
@@ -88,12 +88,12 @@ def test_multi_language_guidance_only(self): # pylint:disable=C0103
"""Test guidance_hint only in multiple languages."""
self.assertPyxformXform(
name="data",
- errored=True,
md="""
- | survey | | | | |
- | | type | name | guidance_hint | guidance_hint::French (fr) |
- | | string | name | as shown on birth certificate| comme sur le certificat de naissance|
+ | survey | | | | |
+ | | type | name | guidance_hint | guidance_hint::French (fr) |
+ | | string | name | as shown on birth certificate| comme sur le certificat de naissance |
""", # noqa
+ errored=True,
error__contains=["The survey element named 'name' has no label or hint."],
)
diff --git a/pyxform/tests_v1/test_sheet_columns.py b/pyxform/tests_v1/test_sheet_columns.py
index f3c67a830..cb801f362 100644
--- a/pyxform/tests_v1/test_sheet_columns.py
+++ b/pyxform/tests_v1/test_sheet_columns.py
@@ -3,6 +3,7 @@
Test XLSForm sheet names.
"""
from pyxform.tests_v1.pyxform_test_case import PyxformTestCase
+from pyxform.tests.utils import prep_for_xml_contains
class InvalidSurveyColumnsTests(PyxformTestCase):
@@ -38,6 +39,32 @@ def test_missing_label(self):
error__contains=["no label or hint"],
)
+ def test_label_node_added_when_hint_given_but_no_label_value(self):
+ """Should output a label node even if no label is specified."""
+ expected = """
+
+
+ h
+
+
+
+
+
+
+
+ """
+ self.assertPyxformXform(
+ name="data",
+ md="""
+ | survey | | | | | |
+ | | type | name | label | media::image | hint |
+ | | text | a | | | h |
+ | | text | b | l | | |
+ | | text | c | | m.png | |
+ """,
+ xml__contains=prep_for_xml_contains(expected),
+ )
+
def test_column_case(self):
"""
Ensure that column name is case insensitive
diff --git a/pyxform/tests_v1/test_typed_calculates.py b/pyxform/tests_v1/test_typed_calculates.py
index 63c934884..61fc1a58e 100644
--- a/pyxform/tests_v1/test_typed_calculates.py
+++ b/pyxform/tests_v1/test_typed_calculates.py
@@ -76,9 +76,8 @@ def test_non_calculate_type_with_calculation_no_warns(self):
self.assertTrue(len(warnings) == 0)
- def test_non_calculate_type_with_hint_and_no_calculation_warns(self):
+ def test_non_calculate_type_with_hint_and_no_calculation__no_warning(self):
warnings = []
-
self.md_to_pyxform_survey(
"""
| survey | | | | | |
@@ -89,9 +88,7 @@ def test_non_calculate_type_with_hint_and_no_calculation_warns(self):
""",
warnings=warnings,
)
-
- self.assertTrue(len(warnings) == 1)
- self.assertTrue("Question has no label" in warnings[0])
+ self.assertTrue(len(warnings) == 0)
def test_non_calculate_type_with_calculation_and_dynamic_default_warns(self):
warnings = []
diff --git a/pyxform/tests_v1/test_xlsform_spec.py b/pyxform/tests_v1/test_xlsform_spec.py
new file mode 100644
index 000000000..58e52e8e6
--- /dev/null
+++ b/pyxform/tests_v1/test_xlsform_spec.py
@@ -0,0 +1,98 @@
+from pyxform.tests_v1.pyxform_test_case import PyxformTestCase
+
+
+class TestWarnings(PyxformTestCase):
+ def test_warnings__count(self):
+ """Should raise an expected number of warnings for a diverse form."""
+ # Converted from warnings.xls file for tests/xlsform_spec_test/WarningsTest
+ md = """
+ | survey | | | | | | | | |
+ | | type | name | label | hint | appearance | image | audio | video |
+ | | text | some_text | | a hint | | | | |
+ | | note | number_label | | a note | | | | |
+ | | note | display_image_test | | | | img_test.jpg | | |
+ | | select_one yes_no | autocomplete_test | autocomplete_test | | autocomplete | | | |
+ | | select_one yes_no | autocomplete_chars_test | autocomplete_chars_test | | autocomplete_chars | | | |
+ | | integer | a_integer | | integer | | | | |
+ | | decimal | a_decimal | | decimal | | | | |
+ | | begin repeat | repeat_test | | | | | | |
+ | | begin group | group_test | | | field-list | | | |
+ | | text | required_text | required_text | | | | | |
+ | | select_multiple yes_no | select_multiple_test | select multiple test | | minimal | | | |
+ | | end group | adsaf | | | | | | |
+ | | begin group | labeled_select_group | labeled select group test | | field-list | | | |
+ | | end group | | | | | | | |
+ | | begin group | name | | | table-list | | | |
+ | | select_one yes_no | table_list_question | table list question | hint | | | | |
+ | | end group | | | | | | | |
+ | | select_one a_b | compact-test | | hint | compact | | | |
+ | | end repeat | | | | | | | |
+ | | acknowledge | acknowledge_test | | hint | | | | |
+ | | date | date_test | | hint | | | | |
+ | | time | time_test | | hint | | | | |
+ | | datetime | datetime_test | | hint | | | | |
+ | | geopoint | geopoint_test | | hint | | | | |
+ | | barcode | barcode_test | | hint | | | | |
+ | | image | image_test | | hint | | | | |
+ | | audio | audio_test | | hint | | | | |
+ | | video | video_test | | hint | | | | |
+ | | start | start | | | | | | |
+ | | end | end | | | | | | |
+ | | today | today | | | | | | |
+ | | deviceid | deviceid | | | | | | |
+ | | phonenumber | phonenumber | | | | | | |
+ | choices | | | | |
+ | | list_name | name | label | image |
+ | | yes_no | yes | yes | |
+ | | yes_no | no | no | |
+ | | a_b | a | | a.jpg |
+ | | a_b | b | | b.jpg |
+ | | animals | zebra | | |
+ | | animals | buffalo | | |
+ | settings | | | | | |
+ | | form_title | form_id | public_key | submission_url | default_language |
+ | | spec_test | spec_test | | | |
+ """ # noqa
+ warnings = []
+ self.assertPyxformXform(
+ name="spec_test", md=md, warnings=warnings,
+ )
+ self.maxDiff = 2000
+ expected = [
+ "On the choices sheet there is a option with no label. [list_name : a_b]",
+ "On the choices sheet there is a option with no label. [list_name : a_b]",
+ "On the choices sheet there is a option with no label. [list_name : animals]",
+ "On the choices sheet there is a option with no label. [list_name : animals]",
+ "[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 : 16] Group has no label: {'name': 'name', 'type': 'begin group'}",
+ "[row : 27] Use the max-pixels parameter to speed up submission "
+ + "sending and save storage space. Learn more: https://xlsform.org/#image",
+ ]
+ self.assertListEqual(expected, warnings)
+
+ def test_warnings__unknown_control_group__with_name(self):
+ """Should raise an error when an unknown control group is found."""
+ self.assertPyxformXform(
+ name="spec_test",
+ md="""
+ | survey | | |
+ | | type | name |
+ | | begin dancing | dancing |
+ """,
+ errored=True,
+ error__contains=["Unknown question type 'begin dancing'."],
+ )
+
+ def test_warnings__unknown_control_group__no_name(self):
+ """Should raise an error when an unknown control group is found."""
+ self.assertPyxformXform(
+ name="spec_test",
+ md="""
+ | survey | | |
+ | | type | name |
+ | | begin | empty |
+ """,
+ errored=True,
+ error__contains=["Unknown question type 'begin'."],
+ )
diff --git a/pyxform/xls2json.py b/pyxform/xls2json.py
index 602aec3dd..6b71dd527 100644
--- a/pyxform/xls2json.py
+++ b/pyxform/xls2json.py
@@ -878,24 +878,6 @@ def workbook_to_json(
)
raise PyXFormError(error_message)
- if (
- 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("default")
- and default_is_dynamic(row.get("default"), question_type)
- )
- ):
- # TODO: Should there be a default label?
- # Not sure if we should throw warnings for groups...
- # Warnings can be ignored so I'm not too concerned
- # about false positives.
- warnings.append(
- row_format_string % row_number + " Question has no label: " + str(row)
- )
-
# Try to parse question as begin control statement
# (i.e. begin loop/repeat/group):
begin_control_parse = begin_control_regex.search(question_type)
@@ -910,6 +892,30 @@ def workbook_to_json(
control_type = aliases.control[parse_dict["type"]]
control_name = question_name
+ # Check if the control item has a label, if applicable.
+ # This label check used to apply to all items, but no longer is
+ # relevant for questions since label nodes are added by default.
+ # 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
+ 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("default")
+ and default_is_dynamic(row.get("default"), question_type)
+ )
+ ):
+ # Row number, name, and type probably enough for user message.
+ # Also means the error message text is stable for tests.
+ msg_dict = {"name": row.get("name"), "type": row.get("type")}
+ warnings.append(
+ row_format_string % row_number
+ + " %s has no label: " % control_type.capitalize()
+ + str(msg_dict)
+ )
+
new_json_dict = row.copy()
new_json_dict[constants.TYPE] = control_type
child_list = list()