fix: support of or_other with selects in itemsets

- builder.py: - add or_other choice to survey-level choice dict - add type annotations - constants.py: add new constant for "or specify other" internal suffix - xls2json.py: add error for choice_filter + or_other. Main reason I suppose is that it's unknown how to filter the "other" choice so the user might never see it. - xlsform_spec_test.py: - fix or_other expected XML to include "other" and match sorted element names. - move or_other test into this testcase since it is the same as others there already. - builder_tests.py: update expected output to match or_other output.
XLSForm · May 12, 2023 · 751e4ea · 751e4ea
1 parent c2dd549
commit 751e4ea
Show file tree

Hide file tree

Showing 8 changed files with 178 additions and 97 deletions.
diff --git a/pyxform/builder.py b/pyxform/builder.py
@@ -5,9 +5,9 @@
 import copy
 import os
 import re
-from typing import Any, Dict, Optional
+from typing import TYPE_CHECKING, Any, Dict, List, Optional, Union
 
-from pyxform import file_utils, utils
+from pyxform import constants, file_utils, utils
 from pyxform.entities.entity_declaration import EntityDeclaration
 from pyxform.errors import PyXFormError
 from pyxform.external_instance import ExternalInstance
@@ -25,6 +25,14 @@
 from pyxform.survey import Survey
 from pyxform.xls2json import SurveyReader
 
+if TYPE_CHECKING:
+    from pyxform.survey_element import SurveyElement
+
+OR_OTHER_CHOICE = {
+    "name": "other",
+    "label": "Other",
+}
+
 
 def copy_json_dict(json_dict):
     """
@@ -88,7 +96,9 @@ def set_sections(self, sections):
         assert type(sections) == dict
         self._sections = sections
 
-    def create_survey_element_from_dict(self, d):
+    def create_survey_element_from_dict(
+        self, d: Dict[str, Any]
+    ) -> Union["SurveyElement", List["SurveyElement"]]:
         """
         Convert from a nested python dictionary/array structure (a json dict I
         call it because it corresponds directly with a json object)
@@ -143,7 +153,11 @@ def _save_trigger_as_setvalue_and_remove_calculate(self, d):
                 self.setvalues_by_triggering_ref[triggering_ref] = [(d["name"], value)]
 
     @staticmethod
-    def _create_question_from_dict(d, question_type_dictionary, add_none_option=False):
+    def _create_question_from_dict(
+        d: Dict[str, Any],
+        question_type_dictionary: Dict[str, Any],
+        add_none_option: bool = False,
+    ) -> Union[Question, List[Question]]:
         question_type_str = d["type"]
         d_copy = d.copy()
 
@@ -152,11 +166,9 @@ def _create_question_from_dict(d, question_type_dictionary, add_none_option=Fals
             SurveyElementBuilder._add_none_option_to_select_all_that_apply(d_copy)
 
         # Handle or_other on select type questions
-        or_other_str = " or specify other"
-        if question_type_str.endswith(or_other_str):
-            question_type_str = question_type_str[
-                : len(question_type_str) - len(or_other_str)
-            ]
+        or_other_len = len(constants.SELECT_OR_OTHER_SUFFIX)
+        if question_type_str.endswith(constants.SELECT_OR_OTHER_SUFFIX):
+            question_type_str = question_type_str[: len(question_type_str) - or_other_len]
             d_copy["type"] = question_type_str
             SurveyElementBuilder._add_other_option_to_multiple_choice_question(d_copy)
             return [
@@ -173,20 +185,18 @@ def _create_question_from_dict(d, question_type_dictionary, add_none_option=Fals
         # todo: clean up this spaghetti code
         d_copy["question_type_dictionary"] = question_type_dictionary
         if question_class:
-
             return question_class(**d_copy)
 
         return []
 
     @staticmethod
-    def _add_other_option_to_multiple_choice_question(d):
+    def _add_other_option_to_multiple_choice_question(d: Dict[str, Any]) -> None:
         # ideally, we'd just be pulling from children
         choice_list = d.get("choices", d.get("children", []))
         if len(choice_list) <= 0:
             raise PyXFormError("There should be choices for this question.")
-        other_choice = {"name": "other", "label": "Other"}
-        if other_choice not in choice_list:
-            choice_list.append(other_choice)
+        if OR_OTHER_CHOICE not in choice_list:
+            choice_list.append(OR_OTHER_CHOICE)
 
     @staticmethod
     def _add_none_option_to_select_all_that_apply(d_copy):
@@ -220,7 +230,7 @@ def _get_question_class(question_type_str, question_type_dictionary):
         return SurveyElementBuilder.QUESTION_CLASSES[control_tag]
 
     @staticmethod
-    def _create_specify_other_question_from_dict(d):
+    def _create_specify_other_question_from_dict(d: Dict[str, Any]) -> InputQuestion:
         kwargs = {
             "type": "text",
             "name": "%s_other" % d["name"],
@@ -242,6 +252,11 @@ def _create_section_from_dict(self, d):
             # And I hope it doesn't break something else.
             # I think the good solution would be to rewrite this class.
             survey_element = self.create_survey_element_from_dict(copy.deepcopy(child))
+            if child["type"].endswith(" or specify other"):
+                select_question = survey_element[0]
+                itemset_choices = result["choices"][select_question["itemset"]]
+                if OR_OTHER_CHOICE not in itemset_choices:
+                    itemset_choices.append(OR_OTHER_CHOICE)
             if survey_element:
                 result.add_children(survey_element)
 

diff --git a/pyxform/constants.py b/pyxform/constants.py
@@ -62,6 +62,7 @@
 SELECT_ONE = "select one"
 SELECT_ONE_EXTERNAL = "select one external"
 SELECT_ALL_THAT_APPLY = "select all that apply"
+SELECT_OR_OTHER_SUFFIX = " or specify other"
 RANK = "rank"
 CHOICES = "choices"
 

diff --git a/pyxform/xls2json.py b/pyxform/xls2json.py
@@ -1158,7 +1158,13 @@ def workbook_to_json(
 
                 specify_other_question = None
                 if parse_dict.get("specify_other") is not None:
-                    select_type += " or specify other"
+                    select_type += constants.SELECT_OR_OTHER_SUFFIX
+                    if row.get(constants.CHOICE_FILTER):
+                        msg = (
+                            ROW_FORMAT_STRING % row_number
+                            + " Choice filter not supported with or_other."
+                        )
+                        raise PyXFormError(msg)
 
                 new_json_dict = row.copy()
                 new_json_dict[constants.TYPE] = select_type

diff --git a/tests/builder_tests.py b/tests/builder_tests.py
@@ -129,6 +129,7 @@ def test_specify_other(self):
                 "sexes": [
                     {"label": {"English": "Male"}, "name": "male"},
                     {"label": {"English": "Female"}, "name": "female"},
+                    {"label": "Other", "name": "other"},
                 ]
             },
             "children": [
@@ -231,6 +232,7 @@ def test_loop(self):
                         "name": "open_pit_latrine",
                     },
                     {"label": {"english": "Bucket system"}, "name": "bucket_system"},
+                    {"label": "Other", "name": "other"},
                 ]
             },
             "children": [
@@ -317,6 +319,20 @@ def test_loop(self):
                                 }
                             ],
                         },
+                        {
+                            "children": [
+                                {
+                                    "label": {
+                                        "english": "How many Other are on the premises?"
+                                    },
+                                    "name": "number",
+                                    "type": "integer",
+                                }
+                            ],
+                            "label": "Other",
+                            "name": "other",
+                            "type": "group",
+                        },
                     ],
                 },
                 {

diff --git a/tests/test_expected_output/or_other.xml b/tests/test_expected_output/or_other.xml
@@ -16,29 +16,33 @@
       <instance id="colors">
         <root>
           <item>
-            <pastel>no</pastel>
-            <name>red</name>
             <label>red</label>
+            <name>red</name>
+            <pastel>no</pastel>
           </item>
           <item>
-            <pastel>no</pastel>
-            <name>green</name>
             <label>green</label>
+            <name>green</name>
+            <pastel>no</pastel>
           </item>
           <item>
-            <pastel>no</pastel>
-            <name>blue</name>
             <label>blue</label>
+            <name>blue</name>
+            <pastel>no</pastel>
           </item>
           <item>
-            <pastel>yes</pastel>
-            <name>mauve</name>
             <label>mauve</label>
+            <name>mauve</name>
+            <pastel>yes</pastel>
           </item>
           <item>
-            <pastel>yes</pastel>
-            <name>apricot</name>
             <label>apricot</label>
+            <name>apricot</name>
+            <pastel>yes</pastel>
+          </item>
+          <item>
+            <label>Other</label>
+            <name>other</name>
           </item>
         </root>
       </instance>

diff --git a/tests/test_translations.py b/tests/test_translations.py
@@ -960,16 +960,17 @@ def test_select1__non_dynamic_choices__no_lang__all_columns(self):
         |         | c1        | na   | la    | a.mp3        | a.jpg        | a.jpg            | a.mkv        |
         |         | c1        | nb   | lb    | b.mp3        | b.jpg        | b.jpg            | b.mkv        |
         """
-        xpath_match = [
-            self.xp.question_label_in_body("Question 1"),
-            xpq.body_select1_itemset("q1"),
-            xpc.model_instance_choices_itext("c1", ("na", "nb")),
-            xpc.model_itext_choice_text_label_by_pos(DEFAULT_LANG, "c1", ("la", "lb")),
-            xpc.model_itext_choice_media_by_pos(DEFAULT_LANG, "c1", self.forms__ab),
-        ]
         self.assertPyxformXform(
             md=md,
-            xml__xpath_match=xpath_match,
+            xml__xpath_match=[
+                self.xp.question_label_in_body("Question 1"),
+                xpq.body_select1_itemset("q1"),
+                xpc.model_instance_choices_itext("c1", ("na", "nb")),
+                xpc.model_itext_choice_text_label_by_pos(
+                    DEFAULT_LANG, "c1", ("la", "lb")
+                ),
+                xpc.model_itext_choice_media_by_pos(DEFAULT_LANG, "c1", self.forms__ab),
+            ],
         )
 
     def test_select1__non_dynamic_choices__one_lang__all_columns(self):
@@ -983,16 +984,15 @@ def test_select1__non_dynamic_choices__one_lang__all_columns(self):
         |         | c1        | na   | la              | a.mp3                  | a.jpg                  | a.jpg                      | a.mkv                  |
         |         | c1        | nb   | lb              | b.mp3                  | b.jpg                  | b.jpg                      | b.mkv                  |
         """
-        xpath_match = [
-            self.xp.question_label_in_body("Question 1"),
-            xpq.body_select1_itemset("q1"),
-            xpc.model_instance_choices_itext("c1", ("na", "nb")),
-            xpc.model_itext_choice_text_label_by_pos("Eng (en)", "c1", ("la", "lb")),
-            xpc.model_itext_choice_media_by_pos("Eng (en)", "c1", self.forms__ab),
-        ]
         self.assertPyxformXform(
             md=md,
-            xml__xpath_match=xpath_match,
+            xml__xpath_match=[
+                self.xp.question_label_in_body("Question 1"),
+                xpq.body_select1_itemset("q1"),
+                xpc.model_instance_choices_itext("c1", ("na", "nb")),
+                xpc.model_itext_choice_text_label_by_pos("Eng (en)", "c1", ("la", "lb")),
+                xpc.model_itext_choice_media_by_pos("Eng (en)", "c1", self.forms__ab),
+            ],
         )
 
     def test_select1__dynamic_choices__no_lang__all_columns(self):
@@ -1006,16 +1006,17 @@ def test_select1__dynamic_choices__no_lang__all_columns(self):
         |         | c1        | na   | la    | a.mp3        | a.jpg        | a.jpg            | a.mkv        |
         |         | c1        | nb   | lb    | b.mp3        | b.jpg        | b.jpg            | b.mkv        |
         """
-        xpath_match = [
-            self.xp.question_label_in_body("Question 1"),
-            xpc.body_itemset_references_itext("select1", "q1", "c1"),
-            xpc.model_instance_choices_itext("c1", ("na", "nb")),
-            xpc.model_itext_choice_text_label_by_pos(DEFAULT_LANG, "c1", ("la", "lb")),
-            xpc.model_itext_choice_media_by_pos(DEFAULT_LANG, "c1", self.forms__ab),
-        ]
         self.assertPyxformXform(
             md=md,
-            xml__xpath_match=xpath_match,
+            xml__xpath_match=[
+                self.xp.question_label_in_body("Question 1"),
+                xpc.body_itemset_references_itext("select1", "q1", "c1"),
+                xpc.model_instance_choices_itext("c1", ("na", "nb")),
+                xpc.model_itext_choice_text_label_by_pos(
+                    DEFAULT_LANG, "c1", ("la", "lb")
+                ),
+                xpc.model_itext_choice_media_by_pos(DEFAULT_LANG, "c1", self.forms__ab),
+            ],
         )
 
     def test_select1__dynamic_choices__one_lang__all_columns(self):
@@ -1029,16 +1030,15 @@ def test_select1__dynamic_choices__one_lang__all_columns(self):
         |         | c1        | na   | la              | a.mp3                  | a.jpg                  | a.jpg                      | a.mkv                  |
         |         | c1        | nb   | lb              | b.mp3                  | b.jpg                  | b.jpg                      | b.mkv                  |
         """
-        xpath_match = [
-            self.xp.question_label_in_body("Question 1"),
-            xpc.body_itemset_references_itext("select1", "q1", "c1"),
-            xpc.model_instance_choices_itext("c1", ("na", "nb")),
-            xpc.model_itext_choice_text_label_by_pos("Eng (en)", "c1", ("la", "lb")),
-            xpc.model_itext_choice_media_by_pos("Eng (en)", "c1", self.forms__ab),
-        ]
         self.assertPyxformXform(
             md=md,
-            xml__xpath_match=xpath_match,
+            xml__xpath_match=[
+                self.xp.question_label_in_body("Question 1"),
+                xpc.body_itemset_references_itext("select1", "q1", "c1"),
+                xpc.model_instance_choices_itext("c1", ("na", "nb")),
+                xpc.model_itext_choice_text_label_by_pos("Eng (en)", "c1", ("la", "lb")),
+                xpc.model_itext_choice_media_by_pos("Eng (en)", "c1", self.forms__ab),
+            ],
         )
 
     def test_missing_translation__one_lang_simple__warn__no_default(self):
@@ -1374,3 +1374,78 @@ def test_missing_translation__two_lang__warn__default(self):
                 self.xp.language_no_itext(DEFAULT_LANG),
             ],
         )
+
+    def test_specify_other__with_translations(self):
+        """Should add an "other" choice to the itemset instance and an itext label."""
+        md = """
+        | survey  |                        |       |            |            |
+        |         | type                   | name  | label      | label::eng |
+        |         | select_one c1 or_other | q1    | Question 1 | Question A |
+        | choices |           |      |       |            |           |
+        |         | list name | name | label | label::eng | label::fr |
+        |         | c1        | na   | la    | la-e       | la-f      |
+        |         | c1        | nb   | lb    | lb-e       |           |
+        """
+        self.assertPyxformXform(
+            md=md,
+            xml__xpath_match=[
+                xpc.model_itext_choice_text_label_by_pos(
+                    "eng", "c1", ("la-e", "lb-e", "-")
+                ),
+                xpc.model_itext_choice_text_label_by_pos("fr", "c1", ("la-f", "-", "-")),
+                xpc.model_itext_choice_text_label_by_pos(
+                    DEFAULT_LANG, "c1", ("la", "lb", "Other")
+                ),
+                xpq.body_select1_itemset("q1"),
+                """
+                /h:html/h:body/x:input[@ref='/test_name/q1_other']/
+                  x:label[text() = 'Specify other.']
+                """,
+            ],
+        )
+
+    def test_specify_other__no_translations(self):
+        """Should add an "other" choice to the itemset instance, but not use itext."""
+        md = """
+        | survey  |                        |       |            |
+        |         | type                   | name  | label      |
+        |         | select_one c1 or_other | q1    | Question 1 |
+        | choices |           |      |       |
+        |         | list name | name | label |
+        |         | c1        | na   | la    |
+        |         | c1        | nb   | lb    |
+        """
+        self.assertPyxformXform(
+            md=md,
+            xml__xpath_match=[
+                """
+                /h:html/h:head/x:model[not(descendant::x:itext)]
+                """,
+                xpc.model_instance_choices_label(
+                    "c1", (("na", "la"), ("nb", "lb"), ("other", "Other"))
+                ),
+                xpq.body_select1_itemset("q1"),
+                """
+                /h:html/h:body/x:input[@ref='/test_name/q1_other']/
+                  x:label[text() = 'Specify other.']
+                """,
+            ],
+        )
+
+    def test_specify_other__choice_filter(self):
+        """Should raise an error since these featuers are unsupported together."""
+        md = """
+        | survey  |                        |       |            |
+        |         | type                   | name  | label      | choice_filter |
+        |         | input                  | q0    | Question 0 |               |
+        |         | select_one c1 or_other | q1    | Question 1 | ${q0} = cf    |
+        | choices |           |      |       |
+        |         | list name | name | label | cf |
+        |         | c1        | na   | la    | 1  |
+        |         | c1        | nb   | lb    | 2  |
+        """
+        self.assertPyxformXform(
+            md=md,
+            errored=True,
+            error__contains=["[row : 3] Choice filter not supported with or_other."],
+        )