wip: initial impl changes and test fixes - 27 tests still broken

pyxform/survey.py

@@ -10,6 +10,7 @@
 from collections import defaultdict
 from datetime import datetime
 from functools import lru_cache
+from typing import List, Iterator, Optional
 
 from pyxform import constants
 from pyxform.constants import EXTERNAL_INSTANCE_EXTENSIONS
@@ -25,6 +26,7 @@
     LAST_SAVED_INSTANCE_NAME,
     LAST_SAVED_REGEX,
     NSMAP,
+    DetachableElement,
     PatchedText,
     get_languages_with_bad_tags,
     has_dynamic_label,
@@ -259,7 +261,7 @@ def get_setvalues_for_question_name(self, question_name):
         return self.setvalues_by_triggering_ref.get("${%s}" % question_name)
 
     @staticmethod
-    def _generate_static_instances(list_name, choice_list):
+    def _generate_static_instances(list_name, choice_list) -> InstanceInfo:
         """
         Generates <instance> elements for static data
         (e.g. choices for select type questions)
@@ -272,16 +274,12 @@ def _generate_static_instances(list_name, choice_list):
         instance_element_list = []
         multi_language = isinstance(choice_list[0].get("label"), dict)
         has_media = bool(choice_list[0].get("media"))
+        has_dyn_label = has_dynamic_label(choice_list, multi_language)
 
         for idx, choice in enumerate(choice_list):
             choice_element_list = []
-            # Add a unique id to the choice element in case there is itext
-            # it references
-            if (
-                multi_language
-                or has_media
-                or has_dynamic_label(choice_list, multi_language)
-            ):
+            # Add a unique id to the choice element in case there are itext references
+            if multi_language or has_media or has_dyn_label:
                 itext_id = "-".join([list_name, str(idx)])
                 choice_element_list.append(node("itextId", itext_id))
 
@@ -308,7 +306,7 @@ def _generate_static_instances(list_name, choice_list):
         )
 
     @staticmethod
-    def _generate_external_instances(element):
+    def _generate_external_instances(element) -> Optional[InstanceInfo]:
         if isinstance(element, ExternalInstance):
             name = element["name"]
             extension = element["type"].split("-")[0]
@@ -327,7 +325,7 @@ def _generate_external_instances(element):
         return None
 
     @staticmethod
-    def _validate_external_instances(instances):
+    def _validate_external_instances(instances) -> None:
         """
         Must have unique names.
 
@@ -356,7 +354,7 @@ def _validate_external_instances(instances):
             raise ValidationError("\n".join(errors))
 
     @staticmethod
-    def _generate_pulldata_instances(element):
+    def _generate_pulldata_instances(element) -> Optional[List[InstanceInfo]]:
         def get_pulldata_functions(element):
             """
             Returns a list of different pulldata(... function strings if
@@ -406,7 +404,7 @@ def get_instance_info(element, file_id):
         return None
 
     @staticmethod
-    def _generate_from_file_instances(element):
+    def _generate_from_file_instances(element) -> Optional[InstanceInfo]:
         itemset = element.get("itemset")
         file_id, ext = os.path.splitext(itemset)
         if itemset and ext in EXTERNAL_INSTANCE_EXTENSIONS:
@@ -426,22 +424,25 @@ def _generate_from_file_instances(element):
 
         return None
 
-    # True if a last-saved instance should be generated, false otherwise
     @staticmethod
-    def _generate_last_saved_instance(element):
+    def _generate_last_saved_instance(element) -> bool:
+        """
+        True if a last-saved instance should be generated, false otherwise.
+        """
         for expression_type in constants.EXTERNAL_INSTANCES:
             last_saved_expression = re.search(
                 LAST_SAVED_REGEX, str(element["bind"].get(expression_type))
             )
             if last_saved_expression:
                 return True
 
-        return re.search(LAST_SAVED_REGEX, str(element["choice_filter"])) or re.search(
-            LAST_SAVED_REGEX, str(element["default"])
+        return bool(
+            re.search(LAST_SAVED_REGEX, str(element["choice_filter"]))
+            or re.search(LAST_SAVED_REGEX, str(element["default"]))
         )
 
     @staticmethod
-    def _get_last_saved_instance():
+    def _get_last_saved_instance() -> InstanceInfo:
         name = "__last-saved"  # double underscore used to minimize risk of name conflicts
         uri = "jr://instance/last-saved"
 
@@ -453,7 +454,7 @@ def _get_last_saved_instance():
             instance=node("instance", id=name, src=uri),
         )
 
-    def _generate_instances(self):
+    def _generate_instances(self) -> Iterator[DetachableElement]:
         """
         Get instances from all the different ways that they may be generated.
 

pyxform/utils.py

@@ -74,7 +74,7 @@ def is_valid_xml_tag(tag):
     return re.search(r"^" + XFORM_TAG_REGEXP + r"$", tag)
 
 
-def node(*args, **kwargs):
+def node(*args, **kwargs) -> DetachableElement:
     """
     args[0] -- a XML tag
     args[1:] -- an array of children to append to the newly created node

pyxform/xls2json.py

@@ -1168,25 +1168,32 @@ def workbook_to_json(
 
                 new_json_dict["parameters"] = parameters
 
+                # Always generate secondary instance for selects.
+                new_json_dict["itemset"] = list_name
+                json_dict["choices"] = choices
+
                 if row.get("choice_filter"):
+                    # External selects e.g. type = "select_one_external city".
                     if select_type == "select one external":
                         new_json_dict["query"] = list_name
-                    else:
-                        new_json_dict["itemset"] = list_name
-                        json_dict["choices"] = choices
-                        if choices.get(list_name):
-                            new_json_dict["list_name"] = list_name
-                            new_json_dict[constants.CHOICES] = choices[list_name]
-                elif (
-                    "randomize" in parameters.keys() and parameters["randomize"] == "true"
-                ):
-                    new_json_dict["itemset"] = list_name
-                    json_dict["choices"] = choices
-                elif file_extension in EXTERNAL_INSTANCE_EXTENSIONS or re.match(
-                    r"\$\{(.*?)\}", list_name
+                    elif choices.get(list_name):
+                        # Reference to list name for data dictionary tools (ilri/odktools).
+                        new_json_dict["list_name"] = list_name
+                        # Copy choices for data export tools (onaio/onadata).
+                        # TODO: could onadata use the list_name to look up the list for
+                        #  export, instead of pyxform internally duplicating choices data?
+                        new_json_dict[constants.CHOICES] = choices[list_name]
+                elif not (
+                    # Select with randomized choices.
+                    (
+                        "randomize" in parameters.keys()
+                        and parameters["randomize"] == "true"
+                    )
+                    # Select from file e.g. type = "select_one_from_file cities.xml".
+                    or file_extension in EXTERNAL_INSTANCE_EXTENSIONS
+                    # Select from previous answers e.g. type = "select_one ${q1}".
+                    or bool(re.match(r"\$\{(.*?)\}", list_name))
                 ):
-                    new_json_dict["itemset"] = list_name
-                else:
                     new_json_dict["list_name"] = list_name
                     new_json_dict[constants.CHOICES] = choices[list_name]
 

tests/builder_tests.py

@@ -125,12 +125,19 @@ def test_specify_other(self):
             "default_language": "default",
             "id_string": "specify_other",
             "sms_keyword": "specify_other",
+            "choices": {
+                "sexes": [
+                    {"label": {"English": "Male"}, "name": "male"},
+                    {"label": {"English": "Female"}, "name": "female"},
+                ]
+            },
             "children": [
                 {
                     "name": "sex",
                     "label": {"English": "What sex are you?"},
                     "type": "select one",
                     "list_name": "sexes",
+                    "itemset": "sexes",
                     "children": [
                         # TODO Change to choices (there is stuff in the
                         # json2xform half that will need to change)
@@ -177,13 +184,15 @@ def test_select_one_question_with_identical_choice_name(self):
             "default_language": "default",
             "id_string": "choice_name_same_as_select_name",
             "type": "survey",
+            "choices": {"zone": [{"label": "Zone", "name": "zone"}]},
             "children": [
                 {
                     "children": [{"name": "zone", "label": "Zone"}],
                     "type": "select one",
                     "name": "zone",
                     "label": "Zone",
                     "list_name": "zone",
+                    "itemset": "zone",
                 },
                 {
                     "children": [
@@ -211,10 +220,24 @@ def test_loop(self):
             "title": "loop",
             "type": "survey",
             "default_language": "default",
+            "choices": {
+                "toilet_type": [
+                    {
+                        "label": {"english": "Pit latrine with slab"},
+                        "name": "pit_latrine_with_slab",
+                    },
+                    {
+                        "label": {"english": "Pit latrine without " "slab/open pit"},
+                        "name": "open_pit_latrine",
+                    },
+                    {"label": {"english": "Bucket system"}, "name": "bucket_system"},
+                ]
+            },
             "children": [
                 {
                     "name": "available_toilet_types",
                     "list_name": "toilet_type",
+                    "itemset": "toilet_type",
                     "label": {"english": "What type of toilets are on the premises?"},
                     "type": "select all that apply",
                     "children": [
@@ -332,6 +355,7 @@ def test_sms_columns(self):
                             ],
                             "label": "Do you have any children?",
                             "list_name": "yes_no",
+                            "itemset": "yes_no",
                             "name": "has_children",
                             "sms_field": "q2",
                             "type": "select one",
@@ -397,6 +421,7 @@ def test_sms_columns(self):
                             ],
                             "label": "What web browsers do you use?",
                             "list_name": "browsers",
+                            "itemset": "browsers",
                             "name": "web_browsers",
                             "sms_field": "q5",
                             "type": "select all that apply",
@@ -446,6 +471,18 @@ def test_sms_columns(self):
             "sms_separator": "+",
             "title": "SMS Example",
             "type": "survey",
+            "choices": {
+                "browsers": [
+                    {"label": "Mozilla Firefox", "name": "firefox", "sms_option": "ff"},
+                    {"label": "Google Chrome", "name": "chrome", "sms_option": "gc"},
+                    {"label": "Internet Explorer", "name": "ie", "sms_option": "ie"},
+                    {"label": "Safari", "name": "safari", "sms_option": "saf"},
+                ],
+                "yes_no": [
+                    {"label": "no", "name": "0", "sms_option": "n"},
+                    {"label": "yes", "name": "1", "sms_option": "y"},
+                ],
+            },
         }
         self.assertEqual(survey.to_json_dict(), expected_dict)
 

tests/pyxform_test_case.py

@@ -125,13 +125,12 @@ def _autoname_inputs(kwargs):
         include in test cases, so this will pull a default value
         from the stack trace.
         """
-        test_name_root = "pyxform"
         if "name" not in kwargs.keys():
-            kwargs["name"] = test_name_root + "_autotestname"
+            kwargs["name"] = "test_name"
         if "title" not in kwargs.keys():
-            kwargs["title"] = test_name_root + "_autotesttitle"
+            kwargs["title"] = "test_title"
         if "id_string" not in kwargs.keys():
-            kwargs["id_string"] = test_name_root + "_autotest_id_string"
+            kwargs["id_string"] = "test_id"
 
         return kwargs
 

tests/test_choices_sheet.py

@@ -1,5 +1,6 @@
 # -*- coding: utf-8 -*-
 from tests.pyxform_test_case import PyxformTestCase
+from tests.xpath_helpers.choices import xp
 
 
 class ChoicesSheetTest(PyxformTestCase):
@@ -17,7 +18,10 @@ def test_numeric_choice_names__for_static_selects__allowed(self):
             |          | choices            | 1    | One   |
             |          | choices            | 2    | Two   |
             """,
-            xml__contains=["<value>1</value>"],
+            xml__xpath_match=[
+                xp.model_instance_choices("choices", (("1", "One"), ("2", "Two"))),
+                xp.body_select1_itemset("a"),
+            ],
         )
 
     def test_numeric_choice_names__for_dynamic_selects__allowed(self):
@@ -34,7 +38,10 @@ def test_numeric_choice_names__for_dynamic_selects__allowed(self):
             |          | choices            | 1    | One   |
             |          | choices            | 2    | Two   |
             """,
-            xml__contains=['<instance id="choices">', "<item>", "<name>1</name>"],
+            xml__xpath_match=[
+                xp.model_instance_choices("choices", (("1", "One"), ("2", "Two"))),
+                xp.body_select1_itemset("a"),
+            ],
         )
 
     def test_choices_without_labels__for_static_selects__allowed(self):
@@ -51,7 +58,10 @@ def test_choices_without_labels__for_static_selects__allowed(self):
             |          | choices            | 1    |       |
             |          | choices            | 2    |       |
             """,
-            xml__contains=["<value>1</value>"],
+            xml__xpath_match=[
+                xp.model_instance_choices_nl("choices", (("1", ""), ("2", ""))),
+                xp.body_select1_itemset("a"),
+            ],
         )
 
     def test_choices_without_labels__for_dynamic_selects__allowed_by_pyxform(self):
@@ -69,5 +79,8 @@ def test_choices_without_labels__for_dynamic_selects__allowed_by_pyxform(self):
             |          | choices            | 2    |       |
             """,
             run_odk_validate=False,
-            xml__contains=['<instance id="choices">', "<item>", "<name>1</name>"],
+            xml__xpath_match=[
+                xp.model_instance_choices_nl("choices", (("1", ""), ("2", ""))),
+                xp.body_select1_itemset("a"),
+            ],
         )