Skip to content

Commit

Permalink
fix: select or_other label is "-" in multilanguage form not "Other" (#…
Browse files Browse the repository at this point in the history 
…687)

- prior to using secondary instances for all selects, the default label
  for the or_other shortcut was "Other" in all languages. Despite the
  various warnings, users wanted the old behaviour to be retained.
- if a user has defined an "other" choice for the or_other choices, then
  any blank translations ("-") will not be replaced with "Other".
  • Loading branch information
@lindsay-stevens
lindsay-stevens authored Jan 18, 2024
1 parent 9ba1367 commit 637f5e4
Show file tree
Hide file tree
Showing 3 changed files with 216 additions and 53 deletions.
31 changes: 27 additions & 4 deletions pyxform/builder.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -205,8 +205,28 @@ def _add_other_option_to_multiple_choice_question(d: Dict[str, Any]) -> None:
choice_list = d.get(const.CHOICES, d.get(const.CHILDREN, []))
if len(choice_list) <= 0:
raise PyXFormError("There should be choices for this question.")
if OR_OTHER_CHOICE not in choice_list:
choice_list.append(OR_OTHER_CHOICE)
if not any(c[const.NAME] == OR_OTHER_CHOICE[const.NAME] for c in choice_list):
choice_list.append(SurveyElementBuilder._get_or_other_choice(choice_list))

@staticmethod
def _get_or_other_choice(
choice_list: List[Dict[str, Any]]
) -> Dict[str, Union[str, Dict]]:
"""
If the choices have any translations, return an OR_OTHER choice for each lang.
"""
if any(isinstance(c.get(const.LABEL), dict) for c in choice_list):
langs = set(
lang
for c in choice_list
for lang in c[const.LABEL]
if isinstance(c.get(const.LABEL), dict)
)
return {
const.NAME: OR_OTHER_CHOICE[const.NAME],
const.LABEL: {lang: OR_OTHER_CHOICE[const.LABEL] for lang in langs},
}
return OR_OTHER_CHOICE

@staticmethod
def _add_none_option_to_select_all_that_apply(d_copy):
Expand Down Expand Up @@ -268,9 +288,12 @@ def _create_section_from_dict(self, d):
if (
itemset_choices is not None
and isinstance(itemset_choices, list)
and OR_OTHER_CHOICE not in itemset_choices
and not any(
c[const.NAME] == OR_OTHER_CHOICE[const.NAME]
for c in itemset_choices
)
):
itemset_choices.append(OR_OTHER_CHOICE)
itemset_choices.append(self._get_or_other_choice(itemset_choices))
# This is required for builder_tests.BuilderTests.test_loop to pass.
self._add_other_option_to_multiple_choice_question(d=child)
if survey_element:
Expand Down
10 changes: 5 additions & 5 deletions tests/builder_tests.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ def test_specify_other(self):
"sexes": [
{"label": {"English": "Male"}, "name": "male"},
{"label": {"English": "Female"}, "name": "female"},
{"label": "Other", "name": "other"},
{"label": {"English": "Other"}, "name": "other"},
]
},
"children": [
Expand All @@ -145,7 +145,7 @@ def test_specify_other(self):
# json2xform half that will need to change)
{"name": "male", "label": {"English": "Male"}},
{"name": "female", "label": {"English": "Female"}},
{"name": "other", "label": "Other"},
{"name": "other", "label": {"English": "Other"}},
],
},
{
Expand Down Expand Up @@ -233,7 +233,7 @@ def test_loop(self):
"name": "open_pit_latrine",
},
{"label": {"english": "Bucket system"}, "name": "bucket_system"},
{"label": "Other", "name": "other"},
{"label": {"english": "Other"}, "name": "other"},
]
},
"children": [
Expand Down Expand Up @@ -262,7 +262,7 @@ def test_loop(self):
# u'name': u'none',
# u'label': u'None',
# },
{"name": "other", "label": "Other"},
{"name": "other", "label": {"english": "Other"}},
],
},
{
Expand Down Expand Up @@ -330,7 +330,7 @@ def test_loop(self):
"type": "integer",
}
],
"label": "Other",
"label": {"english": "Other"},
"name": "other",
"type": "group",
},
Expand Down
228 changes: 184 additions & 44 deletions tests/test_translations.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1520,13 +1520,175 @@ def test_missing_translation__two_lang__warn__default(self):
warnings__not_contains=[OR_OTHER_WARNING],
)

def test_choice_name_containing_dash_output_itext(self):
"""Should output itext when list_name contains a dash (itextId separator)."""
md = """
| survey | | | |
| | type | name | label:en | label:fr |
| | select_one with_us | q0 | Q1 EN | Q1 FR |
| | select_one with-dash | q1 | Q2 EN | Q2 FR |
| choices | | | |
| | list name | name | label:en | label:fr |
| | with_us | na | l1a-en | l1a-fr |
| | with_us | nb | l1b-en | l1b-fr |
| | with-dash | na | l2a-en | l2a-fr |
| | with-dash | nb | l2b-en | l2b-fr |
"""
self.assertPyxformXform(
md=md,
xml__xpath_match=[
xpc.model_itext_choice_text_label_by_pos(
"en", "with_us", ("l1a-en", "l1b-en")
),
xpc.model_itext_choice_text_label_by_pos(
"en", "with-dash", ("l2a-en", "l2b-en")
),
xpc.model_itext_choice_text_label_by_pos(
"fr", "with_us", ("l1a-fr", "l1b-fr")
),
xpc.model_itext_choice_text_label_by_pos(
"fr", "with-dash", ("l2a-fr", "l2b-fr")
),
],
)


class TestTranslationsOrOther(PyxformTestCase):
"""Translations behaviour with or_other."""

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 | media::image::eng |
| | c1 | na | la | la-e | la-f | a.jpg |
| | c1 | nb | lb | lb-e | | b.jpg |
"""
self.assertPyxformXform(
md=md,
xml__xpath_match=[
xpc.model_itext_choice_text_label_by_pos(
"eng", "c1", ("la-e", "lb-e", "Other")
),
xpc.model_itext_choice_text_label_by_pos(
"fr", "c1", ("la-f", "-", "Other")
),
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.']
""",
],
warnings__contains=[OR_OTHER_WARNING],
)

def test_specify_other__with_translations_only(self):
"""Should add an "other" choice to the itemset instance and an itext label."""
md = """
| survey | | | | |
| | type | name | label::en | label::fr |
| | select_one c1 or_other | q1 | Question 1 | Question A |
| choices | | | | |
| | list name | name | label::en | label::fr |
| | c1 | na | la-e | la-f |
| | c1 | nb | lb-e | |
"""
self.assertPyxformXform(
md=md,
xml__xpath_match=[
xpc.model_itext_choice_text_label_by_pos(
"en", "c1", ("la-e", "lb-e", "Other")
),
xpc.model_itext_choice_text_label_by_pos(
"fr", "c1", ("la-f", "-", "Other")
),
"""
/h:html/h:head/x:model/x:itext[
not(descendant::x:translation[@lang='default'])
]
""",
xpq.body_select1_itemset("q1"),
"""
/h:html/h:body/x:input[@ref='/test_name/q1_other']/
x:label[text() = 'Specify other.']
""",
],
warnings__contains=[OR_OTHER_WARNING],
)

def test_specify_other__with_media_only(self):
"""Should add an "other" choice to the itemset instance and an itext label."""
md = """
| survey | | | |
| | type | name | label |
| | select_one c1 or_other | q1 | Question 1 |
| choices | | | | |
| | list name | name | label | media::image |
| | c1 | na | la | a.jpg |
| | c1 | nb | lb | b.jpg |
"""
self.assertPyxformXform(
md=md,
xml__xpath_match=[
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__with_translations_only__existing_other_choice(self):
"""Should not add an extra "other" choice if already defined for some reason."""
# Blank translations for existing "other" choices are not replaced with "Other".
md = """
| survey | | | | |
| | type | name | label::en | label::fr |
| | select_one c1 or_other | q1 | Question 1 | Question A |
| choices | | | | |
| | list name | name | label::en | label::fr |
| | c1 | na | la-e | la-f |
| | c1 | nb | lb-e | |
| | c1 | other | Other | |
"""
self.assertPyxformXform(
md=md,
xml__xpath_match=[
xpc.model_itext_choice_text_label_by_pos(
"en", "c1", ("la-e", "lb-e", "Other")
),
xpc.model_itext_choice_text_label_by_pos("fr", "c1", ("la-f", "-", "-")),
"""
/h:html/h:head/x:model/x:itext[
not(descendant::x:translation[@lang='default'])
]
""",
xpq.body_select1_itemset("q1"),
"""
/h:html/h:body/x:input[@ref='/test_name/q1_other']/
x:label[text() = 'Specify other.']
""",
],
warnings__contains=[OR_OTHER_WARNING],
)

def test_specify_other__with_translations_only__missing_first_translation(self):
"""Should add an "other" choice to the itemset instance and an itext label."""
# xls2json validation would raise an error if a choice has no label at all.
md = """
| survey | | | | | |
| | type | name | label | label::eng | label:fr |
| | select_one c1 or_other | q1 | Question 1 | Question A | QA fr |
| choices | | | | | |
| | list name | name | label | label::eng | label::fr |
| | c1 | na | la | la-e | la-f |
| | c1 | nb | lb | lb-e | |
Expand All @@ -1535,9 +1697,11 @@ def test_specify_other__with_translations(self):
md=md,
xml__xpath_match=[
xpc.model_itext_choice_text_label_by_pos(
"eng", "c1", ("la-e", "lb-e", "-")
"eng", "c1", ("la-e", "lb-e", "Other")
),
xpc.model_itext_choice_text_label_by_pos(
"fr", "c1", ("la-f", "-", "Other")
),
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")
),
Expand All @@ -1560,16 +1724,18 @@ def test_specify_other__with_translations__with_group(self):
| | end group | g1 | | |
| choices | | | | | |
| | list name | name | label | label::eng | label::fr |
| | c1 | na | la | la-e | la-f |
| | c1 | nb | lb | lb-e | |
| | c1 | na | la | | |
| | c1 | nb | lb | lb-e | lb-f |
"""
self.assertPyxformXform(
md=md,
xml__xpath_match=[
xpc.model_itext_choice_text_label_by_pos(
"eng", "c1", ("la-e", "lb-e", "-")
"eng", "c1", ("-", "lb-e", "Other")
),
xpc.model_itext_choice_text_label_by_pos(
"fr", "c1", ("-", "lb-f", "Other")
),
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")
),
Expand Down Expand Up @@ -1600,9 +1766,11 @@ def test_specify_other__with_translations__with_repeat(self):
md=md,
xml__xpath_match=[
xpc.model_itext_choice_text_label_by_pos(
"eng", "c1", ("la-e", "lb-e", "-")
"eng", "c1", ("la-e", "lb-e", "Other")
),
xpc.model_itext_choice_text_label_by_pos(
"fr", "c1", ("la-f", "-", "Other")
),
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")
),
Expand Down Expand Up @@ -1636,9 +1804,11 @@ def test_specify_other__with_translations__with_nested_group(self):
md=md,
xml__xpath_match=[
xpc.model_itext_choice_text_label_by_pos(
"eng", "c1", ("la-e", "lb-e", "-")
"eng", "c1", ("la-e", "lb-e", "Other")
),
xpc.model_itext_choice_text_label_by_pos(
"fr", "c1", ("la-f", "-", "Other")
),
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")
),
Expand Down Expand Up @@ -1679,9 +1849,11 @@ def test_specify_other__with_translations__with_nested_repeat(self):
md=md,
xml__xpath_match=[
xpc.model_itext_choice_text_label_by_pos(
"eng", "c1", ("la-e", "lb-e", "-")
"eng", "c1", ("la-e", "lb-e", "Other")
),
xpc.model_itext_choice_text_label_by_pos(
"fr", "c1", ("la-f", "-", "Other")
),
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")
),
Expand Down Expand Up @@ -1752,35 +1924,3 @@ def test_specify_other__choice_filter(self):
errored=True,
error__contains=["[row : 3] Choice filter not supported with or_other."],
)

def test_choice_name_containing_dash_output_itext(self):
"""Should output itext when list_name contains a dash (itextId separator)."""
md = """
| survey | | | |
| | type | name | label:en | label:fr |
| | select_one with_us | q0 | Q1 EN | Q1 FR |
| | select_one with-dash | q1 | Q2 EN | Q2 FR |
| choices | | | |
| | list name | name | label:en | label:fr |
| | with_us | na | l1a-en | l1a-fr |
| | with_us | nb | l1b-en | l1b-fr |
| | with-dash | na | l2a-en | l2a-fr |
| | with-dash | nb | l2b-en | l2b-fr |
"""
self.assertPyxformXform(
md=md,
xml__xpath_match=[
xpc.model_itext_choice_text_label_by_pos(
"en", "with_us", ("l1a-en", "l1b-en")
),
xpc.model_itext_choice_text_label_by_pos(
"en", "with-dash", ("l2a-en", "l2b-en")
),
xpc.model_itext_choice_text_label_by_pos(
"fr", "with_us", ("l1a-fr", "l1b-fr")
),
xpc.model_itext_choice_text_label_by_pos(
"fr", "with-dash", ("l2a-fr", "l2b-fr")
),
],
)

0 comments on commit 637f5e4

Please sign in to comment.