From 2e03d4025d18b9aecd954cfea63afa18b4d68004 Mon Sep 17 00:00:00 2001 From: lindsay stevens Date: Thu, 30 Nov 2023 21:19:38 +1100 Subject: [PATCH 1/2] fix: test failure due to column order randomisation (similar to #602) - found this issue because `error__contains` is the correct spelling, so the message was not being checked (the pyxformtestcase kwarg validation does not occur if the form has errors). - the error for the entities sheet should report the problematic columns in the order they appear. Until #602 is merged, the incoming dict may have random key order. This change avoids re-introducing the same sort of issue via set usage, instead doing the same with dict keys. - to avoid tests randomly failing, test `error__contains` looks for each column individually. --- pyxform/entities/entities_parsing.py | 6 ++---- tests/test_entities_create.py | 8 +++++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/pyxform/entities/entities_parsing.py b/pyxform/entities/entities_parsing.py index d62b7424..0bff765c 100644 --- a/pyxform/entities/entities_parsing.py +++ b/pyxform/entities/entities_parsing.py @@ -125,11 +125,9 @@ def validate_entity_saveto( def validate_entities_columns(row: Dict): - expected = set(EC.value_list()) - observed = set(row.keys()) - extra = observed.difference(expected) + extra = {k: None for k in row.keys() if k not in EC.value_list()} if 0 < len(extra): - fmt_extra = ", ".join((f"'{k}'" for k in extra)) + fmt_extra = ", ".join((f"'{k}'" for k in extra.keys())) msg = ( f"The entities sheet included the following unexpected column(s): {fmt_extra}. " f"These columns are not supported by this version of pyxform. Please either: " diff --git a/tests/test_entities_create.py b/tests/test_entities_create.py index e940272f..3f6b4d90 100644 --- a/tests/test_entities_create.py +++ b/tests/test_entities_create.py @@ -418,7 +418,7 @@ def test_entities_columns__one_unexpected(self): | | trees | a | ! | """, errored=True, - error_contains=[ + error__contains=[ "The entities sheet included the following unexpected column(s): 'what'" ], ) @@ -434,7 +434,9 @@ def test_entities_columns__multiple_unexpected(self): | | trees | a | ! | ? | """, errored=True, - error_contains=[ - "The entities sheet included the following unexpected column(s): 'what', 'why'" + error__contains=[ + "The entities sheet included the following unexpected column(s):", + "'what'", + "'why'", ], ) From e8ce57a5e8a845963a6864caaba858fe88c1243a Mon Sep 17 00:00:00 2001 From: lindsay stevens Date: Thu, 30 Nov 2023 21:22:25 +1100 Subject: [PATCH 2/2] add: validate list_name extension for select..from_file - previously, an error about a missing sheet name may be thrown, e.g. if the form didn't contain any regular choices. This change explicitly checks for file extension, similar to external selects. - added test case for all the combinations, and removed duplicative older tests - added new alias subgroup for select..from_file to reduce duplication --- pyxform/aliases.py | 11 ++-- ...rom_file_params.py => select_from_file.py} | 21 ++++++- pyxform/xls2json.py | 11 +++- tests/test_external_instances_for_selects.py | 61 ++++++++----------- 4 files changed, 60 insertions(+), 44 deletions(-) rename pyxform/validators/pyxform/{select_from_file_params.py => select_from_file.py} (65%) diff --git a/pyxform/aliases.py b/pyxform/aliases.py index de08bcd4..710c6835 100644 --- a/pyxform/aliases.py +++ b/pyxform/aliases.py @@ -18,6 +18,12 @@ "loop": constants.LOOP, "looped group": constants.REPEAT, } +select_from_file = { + "select_one_from_file": constants.SELECT_ONE, + "select_multiple_from_file": constants.SELECT_ALL_THAT_APPLY, + "select one from file": constants.SELECT_ONE, + "select multiple from file": constants.SELECT_ALL_THAT_APPLY, +} select = { "add select one prompt using": constants.SELECT_ONE, "add select multiple prompt using": constants.SELECT_ALL_THAT_APPLY, @@ -29,10 +35,7 @@ "select_multiple": constants.SELECT_ALL_THAT_APPLY, "select all that apply": constants.SELECT_ALL_THAT_APPLY, "select_one_external": constants.SELECT_ONE_EXTERNAL, - "select_one_from_file": constants.SELECT_ONE, - "select_multiple_from_file": constants.SELECT_ALL_THAT_APPLY, - "select one from file": constants.SELECT_ONE, - "select multiple from file": constants.SELECT_ALL_THAT_APPLY, + **select_from_file, "rank": constants.RANK, } cascading = { diff --git a/pyxform/validators/pyxform/select_from_file_params.py b/pyxform/validators/pyxform/select_from_file.py similarity index 65% rename from pyxform/validators/pyxform/select_from_file_params.py rename to pyxform/validators/pyxform/select_from_file.py index ceee8eff..161b76b5 100644 --- a/pyxform/validators/pyxform/select_from_file_params.py +++ b/pyxform/validators/pyxform/select_from_file.py @@ -1,6 +1,8 @@ import re +from pathlib import Path -from pyxform.constants import ROW_FORMAT_STRING +from pyxform import aliases +from pyxform.constants import EXTERNAL_INSTANCE_EXTENSIONS, ROW_FORMAT_STRING from pyxform.errors import PyXFormError VALUE_OR_LABEL_TEST_REGEX = re.compile(r"^[a-zA-Z_][a-zA-Z0-9\-_\.]*$") @@ -42,3 +44,20 @@ def value_or_label_check(name: str, value: str, row_number: int) -> None: msg = value_or_label_format_msg(name=name, row_number=row_number) raise PyXFormError(msg) return None + + +def validate_list_name_extension( + select_command: str, list_name: str, row_number: int +) -> None: + """For select_from_file types, the list_name should end with a supported extension.""" + list_path = Path(list_name) + if select_command in aliases.select_from_file and ( + 1 != len(list_path.suffixes) + or list_path.suffix not in EXTERNAL_INSTANCE_EXTENSIONS + ): + exts = ", ".join((f"'{e}'" for e in EXTERNAL_INSTANCE_EXTENSIONS)) + raise PyXFormError( + ROW_FORMAT_STRING % row_number + + f" File name for '{select_command} {list_name}' should end with one of " + + f"the supported file extensions: {exts}" + ) diff --git a/pyxform/xls2json.py b/pyxform/xls2json.py index d21f3644..030637e3 100644 --- a/pyxform/xls2json.py +++ b/pyxform/xls2json.py @@ -23,7 +23,7 @@ ) from pyxform.errors import PyXFormError from pyxform.utils import PYXFORM_REFERENCE_REGEX, default_is_dynamic -from pyxform.validators.pyxform import parameters_generic, select_from_file_params +from pyxform.validators.pyxform import parameters_generic, select_from_file from pyxform.validators.pyxform.android_package_name import validate_android_package_name from pyxform.validators.pyxform.translations_checks import SheetTranslations from pyxform.xls2json_backends import csv_to_dict, xls_to_dict, xlsx_to_dict @@ -1120,6 +1120,11 @@ def workbook_to_json( + "List name not in external choices sheet: " + list_name ) + select_from_file.validate_list_name_extension( + select_command=parse_dict["select_command"], + list_name=list_name, + row_number=row_number, + ) if ( list_name not in choices and select_type != constants.SELECT_ONE_EXTERNAL @@ -1209,13 +1214,13 @@ def workbook_to_json( ) if "value" in parameters.keys(): - select_from_file_params.value_or_label_check( + select_from_file.value_or_label_check( name="value", value=parameters["value"], row_number=row_number, ) if "label" in parameters.keys(): - select_from_file_params.value_or_label_check( + select_from_file.value_or_label_check( name="label", value=parameters["label"], row_number=row_number, diff --git a/tests/test_external_instances_for_selects.py b/tests/test_external_instances_for_selects.py index 9f967bcf..028ada44 100644 --- a/tests/test_external_instances_for_selects.py +++ b/tests/test_external_instances_for_selects.py @@ -7,6 +7,7 @@ import os from dataclasses import dataclass, field +from pyxform import aliases from pyxform.constants import EXTERNAL_INSTANCE_EXTENSIONS from pyxform.errors import PyXFormError from pyxform.xls2xform import get_xml_path, xls2xform_convert @@ -202,7 +203,7 @@ def test_param_value_and_label_validation(self): | | type | name | label | parameters | | | {q} cities{e} | city | City | {p} | """ - from pyxform.validators.pyxform import select_from_file_params + from pyxform.validators.pyxform import select_from_file q_types = ("select_one_from_file", "select_multiple_from_file") good_params = ( @@ -235,7 +236,7 @@ def test_param_value_and_label_validation(self): name = "value" if "label" in param: name = "label" - msg = select_from_file_params.value_or_label_format_msg( + msg = select_from_file.value_or_label_format_msg( name=name, row_number=2 ) self.assertPyxformXform( @@ -262,6 +263,28 @@ def test_param_value_case_preserved(self): ], ) + def test_expected_error_message(self): + """Should get helpful error when select_from_file is missing a file extension.""" + md = """ + | survey | | | | + | | type | name | label | + | | {select} cities{ext} | city | City | + """ + types = aliases.select_from_file.keys() + exts = ("", ".exe", ".sav", ".pdf") + error = ( + "File name for '{select} cities{ext}' should end with " + "one of the supported file extensions" + ) + for select in types: + for ext in exts: + with self.subTest(msg=(select, ext)): + self.assertPyxformXform( + md=md.format(select=select, ext=ext), + errored=True, + error__contains=[error.format(select=select, ext=ext)], + ) + class TestSelectOneExternal(PyxformTestCase): """ @@ -477,37 +500,3 @@ def test_external_choices_with_only_header__errors(self): self.assertContains( str(e), "should be an external_choices sheet in this xlsform" ) - - -class TestInvalidExternalFileInstances(PyxformTestCase): - def test_external_other_extension_instances(self): - # re: https://github.com/XLSForm/pyxform/issues/30 - self.assertPyxformXform( - name="epdf", - md=""" - | survey | | | | - | | type | name | label | - | | select_one_from_file cities.pdf | city | City | - | | select_multiple_from_file neighbourhoods.pdf | neighbourhoods | Neighbourhoods | - """, # noqa - errored=True, - error__contains=["should be a choices sheet in this xlsform"], - ) - - def test_external_choices_sheet_included_instances(self): - # re: https://github.com/XLSForm/pyxform/issues/30 - self.assertPyxformXform( - name="epdf", - md=""" - | survey | | | | - | | type | name | label | - | | select_one_from_file cities.pdf | city | City | - | | select_multiple_from_file neighbourhoods.pdf | neighbourhoods | Neighbourhoods | - - | choices | - | | list name | name | label | - | | fruits | apple | Apple | - """, # noqa - errored=True, - error__contains=["List name not in choices sheet: cities.pdf"], - )