Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for choice from previous repeat answers #472

Merged
merged 16 commits into from
Nov 6, 2020
Merged

Add support for choice from previous repeat answers #472

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
01412df
Add support for choice from previous repeat answers
ukanga Oct 2, 2019
4ca43f3
Test choices from previous repeat answers with choice filter
DavisRayM Sep 22, 2020
ba03680
Add support for choices from previous repeat answers with choice filter
DavisRayM Sep 24, 2020
41c36ad
Update default predicate for choices from previous repeat answers
DavisRayM Sep 24, 2020
206f6ab
Test choices from previous repeat answers in nested repeat
DavisRayM Sep 24, 2020
a635a5a
Set default value for 'multi_language' variable
DavisRayM Sep 28, 2020
aa7d9b5
Test that a relative nodeset is generated for select from previous
DavisRayM Oct 17, 2020
9cd3825
Test that choice from previous answers in child repeat work
DavisRayM Oct 17, 2020
3390a3b
Test that current is used when select from previous question is a sib…
DavisRayM Oct 17, 2020
aff5ba7
Check if xpath_parent and context_parent share a common repeat ancestor
DavisRayM Oct 23, 2020
4d719a0
Replace nodeset if within choice_filter
DavisRayM Oct 23, 2020
2d020ba
Update choice from previous repeat answers with choice_filter test case
DavisRayM Oct 24, 2020
8eb48d0
Populate itemset instead of list_name value
DavisRayM Oct 28, 2020
37d0312
Disable "run_odk_validate" functionality for select from previous tests
DavisRayM Oct 28, 2020
4aebb2c
Reference shared parent if context is a child of the target xpath parent
DavisRayM Oct 28, 2020
32b17c7
Remove Validate run
DavisRayM Oct 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 26 additions & 3 deletions pyxform/question.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
XForm Survey element classes for different question types.
"""
import os.path
import re

from pyxform.errors import PyXFormError
from pyxform.question_type_dictionary import QUESTION_TYPE_DICT
Expand Down Expand Up @@ -186,6 +187,7 @@ def build_xml(self):
result.appendChild(element)

choices = survey.get("choices")
multi_language = False
if choices is not None and len(choices) > 0:
first_choices = next(iter(choices.values()))
multi_language = isinstance(first_choices[0].get("label"), dict)
Expand All @@ -196,6 +198,7 @@ def build_xml(self):
choice_filter = self.get("choice_filter")
itemset, file_extension = os.path.splitext(self["itemset"])
has_media = False
is_previous_question = bool(re.match(r"^\${.*}$", self.get("itemset")))

if choices.get(itemset):
has_media = bool(choices[itemset][0].get("media"))
Expand All @@ -210,9 +213,17 @@ def build_xml(self):
else:
itemset = self["itemset"]
itemset_label_ref = "jr:itext(itextId)"
nodeset = "instance('" + itemset + "')/root/item"

choice_filter = survey.insert_xpaths(choice_filter, self, True)
if is_previous_question:
path = survey.insert_xpaths(self["itemset"], self).strip().split("/")
nodeset = "/".join(path[:-1])
itemset_label_ref = path[-1]
else:
nodeset = "instance('" + itemset + "')/root/item"

if choice_filter:
choice_filter = choice_filter.replace(nodeset, ".")
nodeset += "[" + choice_filter + "]"

if self["parameters"]:
Expand All @@ -238,9 +249,21 @@ def build_xml(self):
node("label", ref=itemset_label_ref),
]
result.appendChild(node("itemset", *itemset_children, nodeset=nodeset))
elif not self["children"] and self["list_name"]:
Copy link
Contributor

@lognaturel lognaturel Oct 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially thought it would be clearer to have this whole "select from repeat" case separated from the existing cases but I see now that with having itemset vs. list_name attributes it gets messy. So I think the way you have it is fine but it'd be helpful to have a comment indicating which case this is -- a select from a previous repeat with no choice filter specified.

Or see another possible option at #472 (comment)

My goal would be to unify the "select from repeat" case so it's easier to understand and detangle.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the option described in #472 (comment) is a bit more cleaner as you've stated. I've implemented the option in the latest commits.

list_name = survey.insert_xpaths(self["list_name"], self).strip()
path = list_name.split("/")
depth = len(path)
if depth > 2:
name = path[-1]
# Choices must have a value. Filter out repeat instances without
# an answer for the linked question
nodeset = "/".join(path[:-2] + [path[-2] + f"[./{name} != '']"])
itemset_children = [node("value", ref=name), node("label", ref=name)]
result.appendChild(node("itemset", *itemset_children, nodeset=nodeset))
else:
for n in [o.xml() for o in self.children]:
result.appendChild(n)
for child in self.children:
result.appendChild(child.xml())

return result


Expand Down
81 changes: 81 additions & 0 deletions pyxform/tests_v1/test_repeat.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,3 +330,84 @@ def test_hints_are_present_within_groups(self):
</group>""" # noqa

self.assertPyxformXform(md=md, xml__contains=[expected], run_odk_validate=True)

def test_choice_from_previous_repeat_answers(self):
"""Select one choices from previous repeat answers."""
xlsform_md = """
| survey | | | |
| | type | name | label |
| | begin repeat | rep | Repeat |
| | text | name | Enter name |
| | end repeat | | |
| | select one fruits | fruit | Choose a fruit |
| | select one ${name} | choice | Choose name |
| choices | | | |
| | list name | name | label |
| | fruits | banana | Banana |
| | fruits | mango | Mango |
"""
self.assertPyxformXform(
md=xlsform_md,
xml__contains=[
"<itemset nodeset=\"/pyxform_autotestname/rep[./name != '']\">"
],
run_odk_validate=True,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great that you tried all with Validate. We typically turn this flag off for the test suite because it takes time.

)

def test_choice_from_previous_repeat_answers_with_choice_filter(self):
"""Select one choices from previous repeat answers with choice filter"""
xlsform_md = """
| survey | | | | |
| | type | name | label | choice_filter |
| | begin repeat | rep | Repeat | |
| | text | name | Enter name | |
| | begin group | demographics | Demographics | |
| | integer | age | Enter age | |
| | end group | demographics | | |
| | end repeat | | | |
| | select one fruits | fruit | Choose a fruit | |
| | select one ${name} | choice | Choose name | starts-with(./name, "b") |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make this more realistic, use ${name}. Then you'll just need to add a space before ./name in the expected output.

| | select one ${name} | choice_18_over | Choose name | ${age} > 18 |
| choices | | | | |
| | list name | name | label | |
| | fruits | banana | Banana | |
| | fruits | mango | Mango | |
"""
self.assertPyxformXform(
name="data",
id_string="some-id",
md=xlsform_md,
xml__contains=[
'<itemset nodeset="/data/rep[starts-with(./name, &quot;b&quot;)]">',
'<itemset nodeset="/data/rep[ ./demographics/age &gt; 18]">',
],
run_odk_validate=True,
)

def test_choice_from_previous_repeat_answers_in_nested_repeat(self):
"""Select one choices from previous repeat answers within a nested repeat"""
xlsform_md = """
| survey | | | | |
| | type | name | label | choice_filter |
| | text | enumerators_name | Enter enumerators name | |
| | begin repeat | household_rep | Household Repeat | |
| | integer | household_id | Enter household ID | |
| | begin repeat | household_mem_rep | Household member repeat | |
| | text | name | Enter name of a household member | |
| | integer | age | Enter age of the household member | |
| | end repeat | household_mem_rep | | |
| | begin repeat | selected | Select a representative | |
| | integer | target_min_age | Minimum age requirement | |
| | select one ${name} | selected_name | Choose a name | ${age} > ${target_min_age} |
| | end repeat | selected | | |
| | end repeat | household_rep | | |
"""
self.assertPyxformXform(
name="data",
id_string="some-id",
md=xlsform_md,
xml__contains=[
'<itemset nodeset="/data/household_rep/household_mem_rep[ ./age &gt; current()/../target_min_age ]">',
MartijnR marked this conversation as resolved.
Show resolved Hide resolved
],
run_odk_validate=True,
lognaturel marked this conversation as resolved.
Show resolved Hide resolved
)
3 changes: 3 additions & 0 deletions pyxform/xls2json.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1077,6 +1077,7 @@ def replace_prefix(d, prefix):
list_name not in choices
and select_type != "select one external"
and file_extension not in [".csv", ".xml"]
and not re.match(r"\$\{(.*?)\}", list_name)
):
if not choices:
raise PyXFormError(
Expand Down Expand Up @@ -1187,6 +1188,8 @@ def replace_prefix(d, prefix):
json_dict["choices"] = choices
elif file_extension in [".csv", ".xml"]:
new_json_dict["itemset"] = list_name
elif re.match(r"\$\{(.*?)\}", list_name):
new_json_dict["list_name"] = list_name
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if you populate itemset here, instead? Could you then collapse the cases in question.py and have a case in the is_previous_question branch for if the choice_filter is not set? That would feel more "correct" to me in that when you define a select from a repeat it's inherently an itemset, not a simple choice list.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is possible. I've made these changes in the latest commits

else:
new_json_dict["list_name"] = list_name
new_json_dict[constants.CHOICES] = choices[list_name]
Expand Down