From 054435be7201e3c9459d83de39b4ae11e71dece1 Mon Sep 17 00:00:00 2001 From: lindsay stevens Date: Tue, 19 Apr 2022 20:03:47 +1000 Subject: [PATCH] fix: survey json->xml round trip not working for some question types - details of issue noted in new test under js2x_test_import_from_json.py - xls2json updated to make it easier to be confident that parameters will be a dict (vs. none or a string). - moved off params processing to parse/validate step validators package to be consistent with other utils of this nature. - updated error message to show all invalid params at once, instead of one at a time (which may have resulted in users to having convert a few times to get a valid params set). --- pyxform/survey_element.py | 14 ++-- .../validators/pyxform/parameters_generic.py | 43 ++++++++++ pyxform/xls2json.py | 84 +++++++------------ tests/js2x_test_import_from_json.py | 29 ++++++- tests/test_allow_mock_accuracy.py | 4 +- tests/test_external_instances_for_selects.py | 6 +- tests/test_max_pixels.py | 2 +- tests/test_randomize_itemsets.py | 3 +- 8 files changed, 117 insertions(+), 68 deletions(-) create mode 100644 pyxform/validators/pyxform/parameters_generic.py diff --git a/pyxform/survey_element.py b/pyxform/survey_element.py index a54a76d15..990914224 100644 --- a/pyxform/survey_element.py +++ b/pyxform/survey_element.py @@ -53,7 +53,7 @@ class SurveyElement(dict): "default": str, "type": str, "appearance": str, - "parameters": str, + "parameters": dict, "intent": str, "jr:count": str, "bind": dict, @@ -128,7 +128,7 @@ def add_children(self, children): else: self.add_child(children) - binding_conversions = { + BINDING_CONVERSIONS = { "yes": "true()", "Yes": "true()", "YES": "true()", @@ -143,16 +143,16 @@ def add_children(self, children): "FALSE": "false()", } - CONVERTIBLE_BIND_ATTRIBUTES = [ + CONVERTIBLE_BIND_ATTRIBUTES = ( "readonly", "required", "relevant", "constraint", "calculate", - ] + ) # Supported media types for attaching to questions - SUPPORTED_MEDIA = ["image", "audio", "video"] + SUPPORTED_MEDIA = ("image", "audio", "video") def validate(self): if not is_valid_xml_tag(self.name): @@ -457,10 +457,10 @@ def xml_binding(self): # the xls2json side. if ( hashable(v) - and v in self.binding_conversions + and v in self.BINDING_CONVERSIONS and k in self.CONVERTIBLE_BIND_ATTRIBUTES ): - v = self.binding_conversions[v] + v = self.BINDING_CONVERSIONS[v] if k == "jr:constraintMsg" and ( type(v) is dict or re.search(BRACKETED_TAG_REGEX, v) ): diff --git a/pyxform/validators/pyxform/parameters_generic.py b/pyxform/validators/pyxform/parameters_generic.py new file mode 100644 index 000000000..a10847878 --- /dev/null +++ b/pyxform/validators/pyxform/parameters_generic.py @@ -0,0 +1,43 @@ +from typing import Any, Dict, Sequence + +from pyxform.errors import PyXFormError + +PARAMETERS_TYPE = Dict[str, Any] + + +def parse(raw_parameters: str) -> PARAMETERS_TYPE: + parts = raw_parameters.split(";") + if len(parts) == 1: + parts = raw_parameters.split(",") + if len(parts) == 1: + parts = raw_parameters.split() + + params = {} + for param in parts: + if "=" not in param: + raise PyXFormError( + "Expecting parameters to be in the form of " + "'parameter1=value parameter2=value'." + ) + k, v = param.split("=")[:2] + key = k.lower().strip() + params[key] = v.lower().strip() + + return params + + +def validate( + parameters: PARAMETERS_TYPE, + allowed: Sequence[str], +) -> Dict[str, str]: + """ + Raise an error if 'parameters' includes any keys not named in 'allowed'. + """ + extras = set(parameters.keys()) - (set(allowed)) + if 0 < len(extras): + msg = ( + "Accepted parameters are '{a}'. " + "The following are invalid parameter(s): '{e}'." + ).format(a=", ".join(sorted(allowed)), e=", ".join(sorted(extras))) + raise PyXFormError(msg) + return parameters diff --git a/pyxform/xls2json.py b/pyxform/xls2json.py index 1f068ba6e..282eb15a9 100644 --- a/pyxform/xls2json.py +++ b/pyxform/xls2json.py @@ -8,21 +8,18 @@ import re import sys from collections import Counter -from typing import TYPE_CHECKING +from typing import Any, Dict, KeysView, List, Optional from pyxform import aliases, constants from pyxform.constants import EXTERNAL_INSTANCE_EXTENSIONS, ROW_FORMAT_STRING from pyxform.errors import PyXFormError from pyxform.utils import default_is_dynamic, is_valid_xml_tag, levenshtein_distance -from pyxform.validators.pyxform import select_from_file_params +from pyxform.validators.pyxform import parameters_generic, select_from_file_params from pyxform.validators.pyxform.missing_translations_check import ( missing_translations_check, ) from pyxform.xls2json_backends import csv_to_dict, xls_to_dict, xlsx_to_dict -if TYPE_CHECKING: - from typing import Any, Dict, KeysView, List, Optional - SMART_QUOTES = {"\u2018": "'", "\u2019": "'", "\u201c": '"', "\u201d": '"'} _MSG_SUPPRESS_SPELLING = ( " If you do not mean to include a sheet, to suppress this message, " @@ -275,14 +272,18 @@ def add_flat_annotations(prompt_list, parent_relevant="", name_prefix=""): # prompt['name'] = name_prefix + prompt['name'] -def process_range_question_type(row): - """Returns a new row that includes the Range parameters start, end and - step. +def process_range_question_type( + row: Dict[str, Any], parameters: parameters_generic.PARAMETERS_TYPE +) -> Dict[str, Any]: + """ + Returns a new row that includes the Range parameters start, end and step. Raises PyXFormError when invalid range parameters are used. """ new_dict = row.copy() - parameters = get_parameters(new_dict.get("parameters", ""), ["start", "end", "step"]) + parameters = parameters_generic.validate( + parameters=parameters, allowed=("start", "end", "step") + ) parameters_map = {"start": "start", "end": "end", "step": "step"} defaults = {"start": "1", "end": "10", "step": "1"} @@ -669,6 +670,8 @@ def workbook_to_json( ROW_FORMAT_STRING % row_number + " Question with no type.\n" + str(row) ) + parameters = parameters_generic.parse(raw_parameters=row.get("parameters", "")) + # Pull out questions that will go in meta block if question_type == "audit": # Force audit name to always be "audit" to follow XForms spec @@ -681,16 +684,16 @@ def workbook_to_json( row["name"] = "audit" new_dict = row.copy() - parameters = get_parameters( - new_dict.get("parameters", ""), - [ + parameters_generic.validate( + parameters=parameters, + allowed=( constants.LOCATION_PRIORITY, constants.LOCATION_MIN_INTERVAL, constants.LOCATION_MAX_AGE, constants.TRACK_CHANGES, constants.IDENTIFY_USER, constants.TRACK_CHANGES_REASONS, - ], + ), ) if constants.TRACK_CHANGES in parameters.keys(): @@ -1123,8 +1126,8 @@ def workbook_to_json( select_params_allowed += ["value", "label"] # Look at parameters column for select parameters - parameters = get_parameters( - row.get("parameters", ""), select_params_allowed + parameters_generic.validate( + parameters=parameters, allowed=select_params_allowed ) if "randomize" in parameters.keys(): @@ -1240,7 +1243,7 @@ def workbook_to_json( # range question_type if question_type == "range": - new_dict = process_range_question_type(row) + new_dict = process_range_question_type(row=row, parameters=parameters) parent_children_array.append(new_dict) continue @@ -1249,8 +1252,7 @@ def workbook_to_json( if row.get("default"): new_dict["default"] = process_image_default(row["default"]) - # Validate max-pixels - parameters = get_parameters(row.get("parameters", ""), ["max-pixels"]) + parameters_generic.validate(parameters=parameters, allowed=("max-pixels",)) if "max-pixels" in parameters.keys(): try: int(parameters["max-pixels"]) @@ -1269,7 +1271,7 @@ def workbook_to_json( if question_type == "audio": new_dict = row.copy() - parameters = get_parameters(row.get("parameters", ""), ["quality"]) + parameters_generic.validate(parameters=parameters, allowed=("quality",)) if "quality" in parameters.keys(): if parameters["quality"] not in [ @@ -1288,7 +1290,7 @@ def workbook_to_json( if question_type == "background-audio": new_dict = row.copy() - parameters = get_parameters(row.get("parameters", ""), ["quality"]) + parameters_generic.validate(parameters=parameters, allowed=("quality",)) if "quality" in parameters.keys(): if parameters["quality"] not in [ @@ -1308,13 +1310,17 @@ def workbook_to_json( new_dict = row.copy() if question_type == "geopoint": - parameters = get_parameters( - row.get("parameters", ""), - ["allow-mock-accuracy", "capture-accuracy", "warning-accuracy"], + parameters_generic.validate( + parameters=parameters, + allowed=( + "allow-mock-accuracy", + "capture-accuracy", + "warning-accuracy", + ), ) else: - parameters = get_parameters( - row.get("parameters", ""), ["allow-mock-accuracy"] + parameters_generic.validate( + parameters=parameters, allowed=("allow-mock-accuracy",) ) if "allow-mock-accuracy" in parameters.keys(): @@ -1480,34 +1486,6 @@ def organize_by_values(dict_list, key): return result -def get_parameters(raw_parameters, allowed_parameters): - parts = raw_parameters.split(";") - if len(parts) == 1: - parts = raw_parameters.split(",") - if len(parts) == 1: - parts = raw_parameters.split() - - params = {} - for param in parts: - if "=" not in param: - raise PyXFormError( - "Expecting parameters to be in the form of " - "'parameter1=value parameter2=value'." - ) - k, v = param.split("=")[:2] - key = k.lower().strip() - if key in allowed_parameters: - params[key] = v.lower().strip() - else: - raise PyXFormError( - "Accepted parameters are " - "'%s': '%s' is an invalid parameter." - % (", ".join(allowed_parameters), key) - ) - - return params - - class SpreadsheetReader: def __init__(self, path_or_file): path = path_or_file diff --git a/tests/js2x_test_import_from_json.py b/tests/js2x_test_import_from_json.py index 064ea282c..d3e0ba631 100644 --- a/tests/js2x_test_import_from_json.py +++ b/tests/js2x_test_import_from_json.py @@ -4,10 +4,13 @@ """ from unittest import TestCase -from pyxform.builder import create_survey_element_from_dict +from pyxform.builder import ( + create_survey_element_from_dict, + create_survey_element_from_json, +) -class Json2XformTestJsonImport(TestCase): +class TestJson2XformJsonImport(TestCase): def test_simple_questions_can_be_imported_from_json(self): json_text = { "type": "survey", @@ -23,3 +26,25 @@ def test_simple_questions_can_be_imported_from_json(self): s = create_survey_element_from_dict(json_text) self.assertEqual(s.children[0].type, "decimal") + + def test_question_type_that_accepts_parameters__without_parameters__to_xml(self): + """Should be able to round-trip survey using a un-parameterised question without error.""" + # Per https://github.com/XLSForm/pyxform/issues/605 + # Underlying issue was that the SurveyElement.FIELDS default for "parameters" was + # a string, but in MultipleChoiceQuestion.build_xml a dict is assumed, because + # xls2json.parse_parameters always returns a dict. + js = """ + { + "type": "survey", + "name": "ExchangeRate", + "children": [ + { + "itemset": "pain_locations.xml", + "label": "Location of worst pain this week.", + "name": "pweek", + "type": "select one" + } + ] + } + """ + create_survey_element_from_json(str_or_path=js).to_xml() diff --git a/tests/test_allow_mock_accuracy.py b/tests/test_allow_mock_accuracy.py index 5e256f7ca..e5952040c 100644 --- a/tests/test_allow_mock_accuracy.py +++ b/tests/test_allow_mock_accuracy.py @@ -191,7 +191,7 @@ def test_geoshape_with_accuracy_parameters_errors(self): | | geoshape | geoshape | Geoshape | warning-accuracy=5 | """, errored=True, - error__contains=["'warning-accuracy' is an invalid parameter"], + error__contains=["invalid parameter(s): 'warning-accuracy'"], ) def test_geotrace_with_accuracy_parameters_errors(self): @@ -203,5 +203,5 @@ def test_geotrace_with_accuracy_parameters_errors(self): | | geotrace | geotrace | Geotrace | warning-accuracy=5 | """, errored=True, - error__contains=["'warning-accuracy' is an invalid parameter"], + error__contains=["invalid parameter(s): 'warning-accuracy'"], ) diff --git a/tests/test_external_instances_for_selects.py b/tests/test_external_instances_for_selects.py index 383294c0e..31f7e5cd5 100644 --- a/tests/test_external_instances_for_selects.py +++ b/tests/test_external_instances_for_selects.py @@ -288,7 +288,8 @@ def test_with_params_no_filters(self): | | select_one_external suburb | suburb | Suburb | value=val, label=lbl | """ err = ( - "Accepted parameters are 'randomize, seed': 'value' is an invalid parameter." + "Accepted parameters are 'randomize, seed'. " + "The following are invalid parameter(s): 'label, value'." ) self.assertPyxformXform( name="test", md=md + self.all_choices, errored=True, error__contains=[err] @@ -355,7 +356,8 @@ def test_with_params_with_filters(self): | | select_one_external suburb | suburb | Suburb | state=${state} and city=${city} | value=val, label=lbl | """ err = ( - "Accepted parameters are 'randomize, seed': 'value' is an invalid parameter." + "Accepted parameters are 'randomize, seed'. " + "The following are invalid parameter(s): 'label, value'." ) self.assertPyxformXform( name="test", md=md + self.all_choices, errored=True, error__contains=[err] diff --git a/tests/test_max_pixels.py b/tests/test_max_pixels.py index 1e4238d1d..9a37067f2 100644 --- a/tests/test_max_pixels.py +++ b/tests/test_max_pixels.py @@ -46,7 +46,7 @@ def test_string_extra_params(self): | | image | my_image | Image | max-pixels=640 foo=bar | """, error__contains=[ - "Accepted parameters are 'max-pixels': 'foo' is an invalid parameter." + "Accepted parameters are 'max-pixels'. The following are invalid parameter(s): 'foo'." ], ) diff --git a/tests/test_randomize_itemsets.py b/tests/test_randomize_itemsets.py index a510d2fb1..11544bb5a 100644 --- a/tests/test_randomize_itemsets.py +++ b/tests/test_randomize_itemsets.py @@ -144,7 +144,8 @@ def test_randomized_select_one_bad_param(self): """, error__contains=[ - "Accepted parameters are 'randomize, seed': 'step' is an invalid parameter." + "Accepted parameters are 'randomize, seed'. " + "The following are invalid parameter(s): 'step'." ], )