fix: xform2json choice filters support, minor codestyle improvements

- builder.py: add type hint, remove unnecessary dict - xform2json.py: - _set_survey_name now handles multiple instances - _set_submission_info now sets itemset details from secondary instance, and attempts to parse choice filters - _set_translations now returns value to be set in __init__() - _get_text_from_translation now does media comparison as a list, since a dictkeysview is not a list - add _get_choices to include choices in internal xform representation - xls2json.py - skip adding choice lists that aren't used in the xform to avoid extra unnecessary data. Maybe worth a warning or maybe not. - add secondary instance for table-list appearance, but reject it if it was being added with a choice filter (not supported). - xform2json_test.py: - expand test_load_from_dump variables for easier debugging - add test cases for choice filters, field-list and table-list - these test cases can be refactored to PyxformTestCase at some stage - xls2xform_tests.py: use equivalent test method instead of keyword - update expected XML from inline choices to secondary itemset for the following (these can be refactored to XPath PyxformTestCase): - test_table_list.py - repeat_date_test.xml - xml_escaping.xml
XLSForm · May 5, 2023 · 1238bd6 · 1238bd6
1 parent f6df5b2
commit 1238bd6
Show file tree

Hide file tree

Showing 9 changed files with 127 additions and 57 deletions.
diff --git a/pyxform/builder.py b/pyxform/builder.py
@@ -344,7 +344,7 @@ def create_survey(
     id_string=None,
     title=None,
     default_language=None,
-):
+) -> Survey:
     """
     name_of_main_section -- a string key used to find the main section in the
                             sections dict if it is not supplied in the
@@ -384,7 +384,7 @@ def create_survey(
     return survey
 
 
-def create_survey_from_path(path, include_directory=False):
+def create_survey_from_path(path, include_directory=False) -> Survey:
     """
     include_directory -- Switch to indicate that all the survey forms in the
                          same directory as the specified file should be read
@@ -398,6 +398,4 @@ def create_survey_from_path(path, include_directory=False):
     else:
         main_section_name, section = file_utils.load_file_to_dict(path)
         sections = {main_section_name: section}
-    pkg = {"name_of_main_section": main_section_name, "sections": sections}
-
-    return create_survey(**pkg)
+    return create_survey(name_of_main_section=main_section_name, sections=sections)
diff --git a/pyxform/xform2json.py b/pyxform/xform2json.py
@@ -8,9 +8,12 @@
 import logging
 import re
 import xml.etree.ElementTree as ETree
+from collections import Mapping
 from operator import itemgetter
+from typing import Any, Dict, List
 
 from pyxform import builder
+from pyxform.errors import PyXFormError
 from pyxform.utils import NSMAP
 
 logger = logging.getLogger(__name__)
@@ -223,23 +226,27 @@ def __init__(self, xml_file):
         self.bindings = copy.deepcopy(self.model["bind"])
         self._bind_list = copy.deepcopy(self.model["bind"])
         self.title = doc_as_dict["html"]["head"]["title"]
+        secondary = []
+        if isinstance(self.model["instance"], list):
+            secondary = self.model["instance"][1:]
+        self.secondary_instances = secondary
+        self.translations = self._get_translations()
+        self.choices = self._get_choices()
         self.new_doc = {
             "type": "survey",
             "title": self.title,
             "children": [],
             "id_string": self.title,
             "sms_keyword": self.title,
             "default_language": "default",
+            "choices": self.choices,
         }
         self._set_submission_info()
         self._set_survey_name()
         self.children = []
         self.ordered_binding_refs = []
         self._set_binding_order()
 
-        # set self.translations
-        self._set_translations()
-
         for key, obj in iter(self.body.items()):
             if isinstance(obj, dict):
                 self.children.append(self._get_question_from_object(obj, type=key))
@@ -256,10 +263,16 @@ def _set_binding_order(self):
             self.ordered_binding_refs.append(bind["nodeset"])
 
     def _set_survey_name(self):
-        obj = self.bindings[0]
-        name = obj["nodeset"].split("/")[1]
+        name = self.bindings[0]["nodeset"].split("/")[1]
         self.new_doc["name"] = name
-        self.new_doc["id_string"] = self.model["instance"][name]["id"]
+        instances = self.model["instance"]
+        if isinstance(instances, Mapping):
+            id_string = instances[name]["id"]
+        elif isinstance(instances, list):
+            id_string = instances[0][name]["id"]
+        else:
+            raise PyXFormError(f"Unexpected type for model instances: {type(instances)}")
+        self.new_doc["id_string"] = id_string
 
     def _set_submission_info(self):
         if "submission" in self.model:
@@ -451,6 +464,19 @@ def _get_question_from_object(self, obj, type=None):
             del question["hint"]
         if "type" not in question and type:
             question["type"] = question_type
+        if "itemset" in obj:
+            # Secondary instances
+            nodeset = obj["itemset"]["nodeset"]
+            choices_name = re.findall(r"^instance\('(.*?)'\)", nodeset)[0]
+            question["itemset"] = choices_name
+            question["list_name"] = choices_name
+            question["choices"] = self.choices[choices_name]
+            # Choice filters - attempt parsing XPath like "/node/name[./something =cf]"
+            filter_ref = re.findall(rf"\[ /{self.new_doc['name']}/(.*?) ", nodeset)
+            filter_exp = re.findall(rf"{filter_ref} (.*?)]$", nodeset)
+            if 1 == len(filter_ref) and 1 == len(filter_exp):
+                question["choice_filter"] = f"${{{filter_ref[0]}}}{filter_exp[0]}"
+                question["query"] = choices_name
         return question
 
     def _get_children_questions(self, obj):
@@ -525,16 +551,16 @@ def _get_question_type(self, type):
             return self.QUESTION_TYPES[type]
         return type
 
-    def _set_translations(self):
+    def _get_translations(self) -> List[Dict]:
         if "itext" not in self.model:
-            self.translations = []
-            return
+            return []
         assert "translation" in self.model["itext"]
-        self.translations = self.model["itext"]["translation"]
-        if isinstance(self.translations, dict):
-            self.translations = [self.translations]
-        assert "text" in self.translations[0]
-        assert "lang" in self.translations[0]
+        translations = self.model["itext"]["translation"]
+        if isinstance(translations, dict):
+            translations = [translations]
+        assert "text" in translations[0]
+        assert "lang" in translations[0]
+        return translations
 
     def _get_label(self, label_obj, key="label"):
         if isinstance(label_obj, dict):
@@ -609,7 +635,7 @@ def _get_text_from_translation(self, ref, key="label"):
 
                     label[lang] = text
                     break
-        if key == "media" and label.keys() == ["default"]:
+        if key == "media" and list(label.keys()) == ["default"]:
             label = label["default"]
         return key, label
 
@@ -624,6 +650,24 @@ def _get_constraint_msg(self, constraint_msg):
                 k, constraint_msg = self._get_text_from_translation(ref)
         return constraint_msg
 
+    def _get_choices(self) -> Dict[str, Any]:
+        """
+        Get all form choices, using the model/instance and model/itext.
+        """
+        choices = {}
+        for instance in self.secondary_instances:
+            items = []
+            for choice in instance["root"]["item"]:
+                item = copy.deepcopy(choice)
+                if "itextId" in choice:
+                    key, label = self._get_text_from_translation(
+                        ref=item.pop("itextId"), key="label"
+                    )
+                    item[key] = label
+                items.append(item)
+            choices[instance["id"]] = items
+        return choices
+
     def _get_name_from_ref(self, ref):
         """given /xlsform_spec_test/launch,
         return the string after the last occurance of the character '/'

diff --git a/pyxform/xls2json.py b/pyxform/xls2json.py
@@ -1150,7 +1150,12 @@ def workbook_to_json(
 
                 # Always generate secondary instance for selects.
                 new_json_dict["itemset"] = list_name
-                json_dict["choices"] = choices
+                # Only add the choice if it's being used.
+                if list_name in choices:
+                    # Initialise choices output if none added already.
+                    if constants.CHOICES not in json_dict:
+                        json_dict[constants.CHOICES] = {}
+                    json_dict[constants.CHOICES][list_name] = choices[list_name]
 
                 if row.get("choice_filter"):
                     # External selects e.g. type = "select_one_external city".
@@ -1183,15 +1188,20 @@ def workbook_to_json(
                     # Then this row is the first select in a table list
                     if not isinstance(table_list, str):
                         table_list = list_name
+                        if row.get("choice_filter", None) is not None:
+                            msg = (
+                                ROW_FORMAT_STRING % row_number
+                                + " Choice filter not supported for table-list appearance."
+                            )
+                            raise PyXFormError(msg)
                         table_list_header = {
                             constants.TYPE: select_type,
                             constants.NAME: "reserved_name_for_field_list_labels_"
                             + str(row_number),
                             # Adding row number for uniqueness # noqa
                             constants.CONTROL: {"appearance": "label"},
                             constants.CHOICES: choices[list_name],
-                            # Do we care about filtered selects in table lists?
-                            # 'itemset' : list_name,
+                            "itemset": list_name,
                         }
                         parent_children_array.append(table_list_header)
 

diff --git a/tests/example_xls/field-list.xlsx b/tests/example_xls/field-list.xlsx
diff --git a/tests/test_expected_output/repeat_date_test.xml b/tests/test_expected_output/repeat_date_test.xml
@@ -22,6 +22,18 @@
           </meta>
         </repeat_date_test>
       </instance>
+      <instance id="yes_no">
+        <root>
+          <item>
+            <label>Yes</label>
+            <name>yes</name>
+          </item>
+          <item>
+            <label>No</label>
+            <name>no</name>
+          </item>
+        </root>
+      </instance>
       <bind nodeset="/repeat_date_test/generated_note_name_2" readonly="true()" type="string"/>
       <bind calculate="1" nodeset="/repeat_date_test/repeat_count" type="string"/>
       <bind calculate=" /repeat_date_test/repeat_count " nodeset="/repeat_date_test/repeat_test_count" readonly="true()" type="string"/>
@@ -40,25 +52,17 @@
       <repeat jr:count=" /repeat_date_test/repeat_test_count " nodeset="/repeat_date_test/repeat_test">
         <select1 ref="/repeat_date_test/repeat_test/table_list_3">
           <label>Q1</label>
-          <item>
-            <label>Yes</label>
-            <value>yes</value>
-          </item>
-          <item>
-            <label>No</label>
-            <value>no</value>
-          </item>
+          <itemset nodeset="instance('yes_no')/root/item">
+            <value ref="name"/>
+            <label ref="label"/>
+          </itemset>
         </select1>
         <select1 ref="/repeat_date_test/repeat_test/table_list_4">
           <label>Question 2</label>
-          <item>
-            <label>Yes</label>
-            <value>yes</value>
-          </item>
-          <item>
-            <label>No</label>
-            <value>no</value>
-          </item>
+          <itemset nodeset="instance('yes_no')/root/item">
+            <value ref="name"/>
+            <label ref="label"/>
+          </itemset>
         </select1>
       </repeat>
     </group>

diff --git a/tests/test_expected_output/xml_escaping.xml b/tests/test_expected_output/xml_escaping.xml
@@ -1,5 +1,5 @@
 <?xml version="1.0"?>
-<h:html xmlns="http://www.w3.org/2002/xforms" xmlns:ev="http://www.w3.org/2001/xml-events" xmlns:h="http://www.w3.org/1999/xhtml" xmlns:jr="http://openrosa.org/javarosa" xmlns:orx="http://openrosa.org/xforms" xmlns:xsd="http://www.w3.org/2001/XMLSchema" xmlns:odk="http://www.opendatakit.org/xforms">
+<h:html xmlns="http://www.w3.org/2002/xforms" xmlns:ev="http://www.w3.org/2001/xml-events" xmlns:h="http://www.w3.org/1999/xhtml" xmlns:jr="http://openrosa.org/javarosa" xmlns:odk="http://www.opendatakit.org/xforms" xmlns:orx="http://openrosa.org/xforms" xmlns:xsd="http://www.w3.org/2001/XMLSchema">
   <h:head>
     <h:title>xml_escaping</h:title>
     <model odk:xforms-version="1.0.0">
@@ -13,6 +13,18 @@
           </meta>
         </xml_escaping>
       </instance>
+      <instance id="yes_no">
+        <root>
+          <item>
+            <label>Yes</label>
+            <name>yes</name>
+          </item>
+          <item>
+            <label>No</label>
+            <name>no</name>
+          </item>
+        </root>
+      </instance>
       <bind calculate="2" nodeset="/xml_escaping/a" type="string"/>
       <bind calculate="1" nodeset="/xml_escaping/b" type="string"/>
       <bind nodeset="/xml_escaping/table_list_3" type="string"/>
@@ -22,14 +34,10 @@
   <h:body>
     <select1 ref="/xml_escaping/table_list_3">
       <label>
-        <output value=" /xml_escaping/a "/>&lt; <output value=" /xml_escaping/b "/></label><item>
-        <label>Yes</label>
-        <value>yes</value>
-      </item>
-      <item>
-        <label>No</label>
-        <value>no</value>
-      </item>
+        <output value=" /xml_escaping/a "/> &lt; <output value=" /xml_escaping/b "/> </label><itemset nodeset="instance('yes_no')/root/item">
+      <value ref="name"/>
+      <label ref="label"/>
+    </itemset>
     </select1>
   </h:body>
 </h:html>
diff --git a/tests/test_table_list.py b/tests/test_table_list.py
@@ -24,10 +24,10 @@
       </input>
       <select1 appearance="label" ref="/table-list-appearance-mod/tablelist1/reserved_name_for_field_list_labels_3">
         <label> </label>
-        <item>
-          <label>Yes</label>
-          <value>yes</value>
-        </item>
+        <itemset nodeset="instance('yes_no')/root/item">
+          <value ref="name"/>
+          <label ref="label"/>
+        </itemset>
       </select1>
       <select1 appearance="list-nolabel" ref="/table-list-appearance-mod/tablelist1/options1a">
         <label>Q1</label>

diff --git a/tests/xform2json_test.py b/tests/xform2json_test.py
@@ -21,6 +21,7 @@ def setUp(self):
         self.excel_files = [
             "gps.xls",
             # "include.xls",
+            "choice_filter_test.xlsx",
             "specify_other.xls",
             "loop.xls",
             "text_and_integer.xls",
@@ -31,6 +32,8 @@ def setUp(self):
             "simple_loop.xls",
             "yes_or_no_question.xls",
             "xlsform_spec_test.xlsx",
+            "field-list.xlsx",
+            "table-list.xls",
             "group.xls",
         ]
         self.surveys = {}
@@ -41,9 +44,12 @@ def setUp(self):
 
     def test_load_from_dump(self):
         for filename, survey in iter(self.surveys.items()):
-            survey.json_dump()
-            survey_from_dump = create_survey_element_from_xml(survey.to_xml())
-            self.assertXFormEqual(survey.to_xml(), survey_from_dump.to_xml())
+            with self.subTest(msg=filename):
+                survey.json_dump()
+                survey_from_dump = create_survey_element_from_xml(survey.to_xml())
+                expected = survey.to_xml()
+                observed = survey_from_dump.to_xml()
+                self.assertXFormEqual(expected, observed)
 
     def tearDown(self):
         for filename, survey in self.surveys.items():

diff --git a/tests/xls2xform_tests.py b/tests/xls2xform_tests.py
@@ -244,8 +244,8 @@ def test_get_xml_path_function(self):
         """Should return an xml path in the same directory as the xlsx file"""
         xlsx_path = "/home/user/Desktop/xlsform.xlsx"
         expected = "/home/user/Desktop/xlsform.xml"
-        assert expected == get_xml_path(xlsx_path)
+        self.assertEqual(expected, get_xml_path(xlsx_path))
         # check that it also handles spaced routes
         xlsx_path = "/home/user/Desktop/my xlsform.xlsx"
         expected = "/home/user/Desktop/my xlsform.xml"
-        assert expected == get_xml_path(xlsx_path)
+        self.assertEqual(expected, get_xml_path(xlsx_path))