Merge pull request #607 from lindsay-stevens/pyxform-605

fix: survey json->xml round trip not working for some question types

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)
                 ):

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

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

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()

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

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]

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

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