Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Output empty <label/> instead of omitting it. #543

Merged
merged 8 commits into from
Jul 28, 2021
Merged

Output empty <label/> instead of omitting it. #543

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
10ee66e
fix: always output label node, remove warning, fix affected tests
lindsay-stevens Jul 5, 2021
6bb547b
add: test case for ok label node output when no label column in xlsform
lindsay-stevens Jul 5, 2021
4123379
Merge branch 'master' into pyxform-439
lindsay-stevens Jul 27, 2021
8ed7e64
chg: error on guidance with no label/hint, test style, remove e2e tests
lindsay-stevens Jul 27, 2021
1a59e16
fix: restore previously removed label check, revert test changes
lindsay-stevens Jul 28, 2021
3e4a84e
chg: make label check only apply to control items instead of questions
lindsay-stevens Jul 28, 2021
e91a176
dev: black formatting
lindsay-stevens Jul 28, 2021
a0d3f7c
chg: clarify test names
lindsay-stevens Jul 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 17 additions & 3 deletions pyxform/survey_element.py
View file
Open in desktop
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]":
lindsay-stevens marked this conversation as resolved.
Show resolved Hide resolved
"""
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
Open in desktop
Binary file not shown.
13 changes: 13 additions & 0 deletions pyxform/tests/utils.py
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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."""
lindsay-stevens marked this conversation as resolved.
Show resolved Hide resolved
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
Open in desktop
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):
lindsay-stevens marked this conversation as resolved.
Show resolved Hide resolved
"""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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Copy link
Contributor

@lognaturel lognaturel Jul 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there tests for this case and the two below? (I'm pretty sure there are but would be good to confirm that you identified them)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was for warnings, not for errors. It is related to what @yanokwa was doing at #542 so we can leave it in here and then look at #542 separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Restored this but in a slightly different location. #542 can still add in the new check condition, and add the tests. Please see commit msg for 3e4a84e

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