Skip to content

Commit

Permalink
Merge pull request #543 from lindsay-stevens/pyxform-439
Browse files Browse the repository at this point in the history 
Output empty `<label/>` instead of omitting it.
  • Loading branch information
@lognaturel
lognaturel authored Jul 28, 2021
2 parents cf7ba39 + a0d3f7c commit 2e631b1
Show file tree
Hide file tree
Showing 10 changed files with 194 additions and 53 deletions.
20 changes: 17 additions & 3 deletions pyxform/survey_element.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,19 @@
default_is_dynamic,
)
from pyxform.xls2json import print_pyobj_to_json
from typing import TYPE_CHECKING

try:
from functools import lru_cache
except ImportError:
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()
Expand Down Expand Up @@ -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
Expand Down
Binary file removed pyxform/tests/example_xls/warnings.xls
View file
Binary file not shown.
13 changes: 13 additions & 0 deletions pyxform/tests/utils.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -18,6 +20,11 @@
except ImportError:
import configparser


if TYPE_CHECKING:
from typing import Tuple


DIR = os.path.dirname(__file__)


Expand Down Expand Up @@ -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), " ",),)
18 changes: 0 additions & 18 deletions pyxform/tests/xlsform_spec_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
6 changes: 5 additions & 1 deletion pyxform/tests_v1/pyxform_test_case.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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(
Expand Down
16 changes: 8 additions & 8 deletions pyxform/tests_v1/test_guidance_hint.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,25 +75,25 @@ 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."],
)

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."],
)

Expand Down
27 changes: 27 additions & 0 deletions pyxform/tests_v1/test_sheet_columns.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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 = """
<input ref="/data/a">
<label/>
<hint>h</hint>
</input>
<input ref="/data/b">
<label>l</label>
</input>
<input ref="/data/c">
<label ref="jr:itext('/data/c:label')"/>
</input>
"""
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
Expand Down
7 changes: 2 additions & 5 deletions pyxform/tests_v1/test_typed_calculates.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 | | | | | |
Expand All @@ -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 = []
Expand Down
98 changes: 98 additions & 0 deletions pyxform/tests_v1/test_xlsform_spec.py
View file
Original file line number Diff line number Diff line change
@@ -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'."],
)
42 changes: 24 additions & 18 deletions pyxform/xls2json.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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()
Expand Down

0 comments on commit 2e631b1

Please sign in to comment.