Add support for choice from previous repeat answers #472

DavisRayM · 2020-09-22T14:42:08Z

Based on #381
Closes #38

Why is this the best possible solution? Were any other approaches considered?

Easiest solution with the least amount of changes.

What are the regression risks?

None

Does this change require updates to documentation? If so, please file an issue here and include the link below.

Yes, would need an additional entry that states that this is possible.

Before submitting this PR, please make sure you have:

included test cases for core behavior and edge cases in tests_v1
run nosetests and verified all tests pass
run black pyxform to format code
verified that any code or assets from external sources are properly credited in comments

pyxform/question.py

codecov-commenter · 2020-09-22T14:52:47Z

Codecov Report

Merging #472 into master will increase coverage by 0.05%.
The diff coverage is 95.45%.

@@            Coverage Diff             @@
##           master     #472      +/-   ##
==========================================
+ Coverage   83.53%   83.59%   +0.05%     
==========================================
  Files          25       25              
  Lines        3608     3627      +19     
  Branches      838      842       +4     
==========================================
+ Hits         3014     3032      +18     
  Misses        452      452              
- Partials      142      143       +1

Impacted Files	Coverage Δ
pyxform/question.py	`93.04% <95.00%> (+0.08%)`	⬆️
pyxform/xls2json.py	`77.96% <100.00%> (+0.06%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d4b4e28...a635a5a. Read the comment docs.

lognaturel

Thanks for picking this back up and for explaining the string-length predicate! I described a few more tests to add.

pyxform/question.py

lognaturel · 2020-09-22T20:23:41Z

pyxform/tests_v1/test_repeat.py

+        |         | text               | name       | Enter name     |                           |
+        |         | end repeat         |            |                |                           |
+        |         | select one fruits  | fruit      | Choose a fruit |                           |
+        |         | select one ${name} | choice     | Choose name    | starts-with(./name, "b")  |


I realize users wouldn't write the choice_filter that way, they'd write starts-with(${name}, "b"). Another even more useful predicate to write would be, if there were also an age question in the repeat, ${age} > 10. In both of those cases, the references should expand to ./name and ./age respectively because the predicate's context is the repeat. I'm not sure the best way to do that. Maybe expand references to an absolute reference and then substring the repeat path? So here you'd expand ${age} to /data/repeat/age and then remove /data/repeat. This should work even if there's a subgroup in the repeat (e.g. /data/repeat/demographics/age).

I think you should also have a test for one of these in a nested repeat. This seems likely to happen. For example, imagine an outer loop that collects information about households. Then a first repeat gets a roster of all household members. A second repeat after that asks questions about everyone over 18 (see the dynamic filter case above). I think the nodeset reference would have to be relative and look like ../repeat1. Any relative references in the predicate would also have to be relative and start with current()/.., I believe.

I agree @lognaturel. Do leave some tests without ${} syntax though. There are users that have started to leverage external data with complex queries and those will be using XPath for e.g. choice_filters (because the ${} syntax won't work). They may naturally use XPath for all choice_filters.

Actually, this may not be the case (or maybe you have another example in mind):

I think the nodeset reference would have to be relative and look like ../repeat1. Any relative references in the predicate would also have to be relative and start with current()/.., I believe.

We're not switching to another ~~(instance)~~ context here. So for:

<data> <household>  <member>  <name/> <age/> <adult>  <person/> <education/> </adult> </member> </household> </data>

If person is a select one {name} question, the nodeset would be ~~"../../member[./age > 18]"~~ "../../../member[./age > 18]"

the nodeset would be "../../member[./age > 18]"

For that case the context would be /data/household[X]/member[Y]/adult[Z]/person, right? Wouldn't the nodeset be ../../../member[./age > 18]?

I was thinking of:

<data> <household>  <person>  <name/> <age/> </person> <adult>  <adult_name/> </adult> </household> </data>

First you get a roster of all the people in the household. Then you have a separate repeat that applies only to adults. Why would you do this instead of having a section with a relevant in the person repeat? I'm not sure. But I've seen forms like this.

If adult_name has type select one ${name} with choice filter ${age} > 18 then the context is /data/household[X]/adult[Y]/adult_name and the nodeset would be ../../person[./age > 18]. You're right, in that scenario, no current() is needed.

I think a scenario where current() would be needed is when a sibling node of a select in a repeat is used in the choice filter:

<data> <household>  <eligible/> <person>  <name/> <age/> </person> <selected>  <target_min_age/> <selected_name/> </selected> </household> </data>

Let's say selected_name has type select one ${name} with choice filter ${age} > ${target_min_age}. The context is /data/household[X]/selected[Y]/selected_name. The nodeset would be ../../person[./age > current()/../target_min_age], I think. This doesn't seem like a particularly practical example and it might be ok not to support it with ${} syntax. I'd just like for us to be explicit about what we do and don't support.

Added a test case for this scenario:

<data> <household>  <eligible/> <person>  <name/> <age/> </person> <selected>  <target_min_age/> <selected_name/> </selected> </household> </data>

With the current changes present in this branch, this seems to be supported with the ${} syntax (Though we're currently using an absolute path to the node... Not entirely sure how & if that'll affect anything... Looking into generating relative paths at the moment).

Kindly have another look at the PR when possible @lognaturel @MartijnR

@lognaturel, your explanation seems flawless (and my example indeed should have stepped down once more).
This also means that my understanding of current() and our spec needs work: https://getodk.github.io/xforms-spec/#fn:current getodk/xforms-spec#276

@DavisRayM, sorry for forgetting. I'll try to look at the tests today.

This doesn't seem like a particularly practical example and it might be ok not to support it with ${} syntax. I'd just like for us to be explicit about what we do and don't support.

Would be great if we did. We'd likely save more work in analyzing difficult support requests in the future.

<data> <household>  <member>  <name/> <age/> <adult>  <person/> <education/> </adult> </member> </household> </data>

Hey @MartijnR @lognaturel, For the example above should the nodeset be ../.. [ ./age > 18] or ../../../member [ ./age > 18 ? Is there a difference between the two ?

I'd missed this and answered in Slack but for completeness: "That would not be the same because it would be going up to a specific instance of the member repeat. What we want is to get the member nodeset and then filter its nodes based on age."

pyxform/tests_v1/test_repeat.py

lognaturel

🎉 Great progress. One test case left to address. The rest all looks good and I've just given a couple ideas for you to consider. @MartijnR the test coverage looks sufficient to me.

lognaturel · 2020-10-23T17:42:00Z

pyxform/tests_v1/test_repeat.py

+        |         | end group          | demographics   |                |                           |
+        |         | end repeat         |                |                |                           |
+        |         | select one fruits  | fruit          | Choose a fruit |                           |
+        |         | select one ${name} | choice         | Choose name    | starts-with(./name, "b")  |


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

lognaturel · 2020-10-23T17:42:44Z

pyxform/tests_v1/test_repeat.py

+            xml__contains=[
+                "<itemset nodeset=\"/pyxform_autotestname/rep[./name != '']\">"
+            ],
+            run_odk_validate=True,


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

lognaturel · 2020-10-23T17:54:14Z

pyxform/question.py

@@ -238,9 +251,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"]:


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.

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.

lognaturel · 2020-10-23T18:07:14Z

pyxform/xls2json.py

@@ -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


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.

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

lognaturel · 2020-10-23T18:09:26Z

pyxform/tests_v1/test_repeat.py

+            name="data",
+            id_string="some-id",
+            md=xlsform_md,
+            xml__contains=['<itemset nodeset="../../../member[ ./age  &gt; 18]">',],


This is tricky indeed. The rule is that if you want to refer to an ancestor repeat, you need to parent above it and include its name. I'm not sure where the best place for that is and let's talk if you get stuck.

Are the changes at https://github.com/XLSForm/pyxform/pull/472/files#r511057165 to address this case? I think basically you'll need to address #453. Does that sound right, @MartijnR?

Actually, this was kicking around my brain and I believe https://github.com/XLSForm/pyxform/pull/472/files#discussion_r511920303 addresses the case in #453. The case that's now missing is referencing a parent repeat. That is, a reference with a relative terminal is being generated (../../) when the parent repeat name needs to be included (../../../member).

Made a few changes in the latest commits; Now checking if the context_parent is a child of the xpath_path and only referencing the parent repeat when this is the case

pyxform/survey.py

lognaturel

Thank you, @DavisRayM! I think using "itemset" for the case with and without a choice filter turned out really well. And great that you've addressed all tricky cases!!

Going to give @MartijnR a chance to comment if he wants but this is ready to merge as far as I'm concerned.

pyxform/tests_v1/test_repeat.py

DavisRayM · 2020-10-29T09:26:16Z

Documentation Update issue: XLSForm/xlsform.github.io#210

MartijnR · 2020-11-02T20:17:15Z

Sorry, I've not been much online in the past month (network issues) and not really able to do any work because of it. Relative path values for `nodeset` and `ref` attributes won't work in Enketo. I think this may have been first introduced with the setvalue feature in pyxform. I don't think we can get this to work in Enketo without severe performance degradation during the transformation step (and that's why it was actually removed about 7 years ago and because pyxform never generated such values).

…

On Thu, Oct 29, 2020 at 11:07 AM Hélène Martin ***@***.***> wrote: ***@***.**** approved this pull request. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#472 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEZFFUIV4G5M3FDHWXHUDTSNGOOZANCNFSM4RV3WC2Q> .

-- *Pushing data since 2012.* Enketo <https://enketo.org> | LinkedIn <http://www.linkedin.com/company/enketo-llc> | GitHub <https://github.com/enketo> | Twitter <https://twitter.com/enketo> | Blog <http://blog.enketo.org>

-- -- *Revolutionizing data collection since 2012.* Enketo <https://enketo.org/> | LinkedIn <http://www.linkedin.com/company/enketo-llc> | GitHub <https://github.com/enketo> | Twitter <https://twitter.com/enketo> | Blog <http://blog.enketo.org/>

lognaturel · 2020-11-04T20:06:30Z

network issues

Oh no! ⛰️

Relative path values for nodeset and ref attributes won't work in Enketo.

Sorry to hear that. I can see how performance could be a challenge for contextualization. What do you suggest as a way forward? Maybe we could add an Enketo-specific warning if such attribute values are generated? My (admittedly limited) experience has been that forms with nested repeats tend to be enumerator-mediated and lean more on Collect so it may not be a huge limitation. There are a lot of very useful things that can be done with this functionality for a single top-level repeat. I haven't seen as many nested repeats in Enketo but maybe you have more insights or see it differently, @MartijnR.

MartijnR · 2020-11-04T20:38:37Z

I think there would be two solutions.

Pyxform only generates absolute path references for ref and nodeset attribute values. I think that wouldn't affect anything, right? I'm planning to look into a similar setvalue issue and may propose to make that pyxform change.
Enketo re-adds support for relative path references as ref and nodeset attribute values.

lognaturel · 2020-11-04T21:04:00Z

Pyxform only generates absolute path references for ref and nodeset attribute values

In a nested repeat context, wouldn't that mean that the first instance of the outer repeat is always used?

<data>
   <household> <!-- repeat -->
       <eligible/>
       <person> <!-- repeat -->
            <name/>
            <age/>
       </person>
       <selected> <!-- repeat -->
           <target_min_age/>
           <selected_name/>
       </selected>
    </household>
</data>

If you want selected_name to be built from the person repeat in the current household, I think that the nodeset has to be a relative reference. An absolute path would still work in Collect because it still contextualizes absolute refs within repeats but I think that in Enketo it would just always use the person repeat from the first household.

I think the same would be the case for setvalue where the outer repeats need to be contextualized against the current context.

MartijnR · 2020-11-04T21:08:57Z

Ah, no, I think the difference is when relative paths are using in expressions (that should stay). We've always generated absolute paths for ref and nodeset attribute values for questions inside repeats (until about 2 months ago) and I think we still do for most questions. I think the relative path introduction was just an unnecessary slip (though syntax-wise not incorrect).

lognaturel · 2020-11-04T21:50:55Z

Interesting, I see! I think we had identified a while back that according to W3C XForms ref and nodeset values inside of groups/repeats are supposed to be relative but yes, they have always been absolute in ODK.

I think it might be a messy special case to introduce but should be possible. How about merging this if it seems complete to you and then addressing the nodeset/ref case separately since it doesn't only touch on this work?

MartijnR · 2020-11-04T22:05:31Z

OMG, I am so sorry. I saw the nodeset="../../" in my email thread yesterday when I still had a 0.5 Mbps internet connection and thought it was the same new syntax as was introduced with setvalue recently. It's a very different case though. In this case (for an itemset element) it is totally correct and should be relative. For some reason, I missed that this was not referring to a question/bind and neglected to look into it deeper. I think this should work fine in Enketo too.

(an absolute value would return all the nodes from all repeats combined, I think)

Sorry for wasting your time with this! Ready to merge I would think. Thanks to you both for this cool feature @DavisRayM and @lognaturel!

lognaturel · 2020-11-04T22:25:26Z

It's a very different case though.

Ok, now I'm confused. 😄 In this case it's some arbitrary XPath expression that gets contextualized against the current context and that may or may not be in the primary instance. That makes sense to me and it makes sense why it would be relative for the reasons I stated above.

The setvalue case is like the ref for the select1 below? I think that in W3C XForms that would be relative as well. But basically it gets associated with the abstract repeat concept at form parse time so it doesn't get dynamically contextualized? The implementation will vary by client but is that the rough justification for why it's ok for that to be absolute?

<select1 ref="/data/household/adult/adult_name">
  <label>Choose a name</label>
  <itemset nodeset="../../person[ ./age  &gt; 18]">
    <value ref="name"/>
    <label ref="name"/>
  </itemset>
</select1>

Does Enketo have trouble with any relative setvalue refs or only ones that end up in the model rather than being nested in the body (so ones triggered by e.g. repeat addition rather than e.g. value change)? What am I talking about, repeat addition is also nested in the body so I don't think there is any model-level setvalue applicable to repeats, only to groups. Ah, except for populating concrete repeats in the main instance at form load time. So that could still be the issue.

MartijnR · 2020-11-04T22:32:57Z

The setvalue case is like the ref for the select1 below?

No, I believe it's a ref for just a simple question that gets a relative path value. It's basically about finding the bind that belongs to a question (and dependencies between questions) that's not supported in Enketo. But I promise to first investigate properly and then explain it clearly in a new setvalue-only issue (if still applicable after investigating ;)), once I get to it.

In the case of an itemset Enketo just evaluates the nodeset value, so it doesn't relate to finding binds and question logic dependencies. It was really silly of me to flag that. I should have just kept sanding my picnic table instead.

lognaturel · 2020-11-05T17:54:52Z

Great, looking forward to the explanation if there is one. 😄

Merging is blocked on your change request, @MartijnR! If you're happy can you please approve and merge?

MartijnR

Sorry, forgot I had changes to review. Looks good!

lognaturel · 2020-11-06T17:11:38Z

Thanks all, and especially @DavisRayM! 🎉

Add support for choice from previous repeat answers

01412df

DavisRayM requested review from ukanga and lognaturel September 22, 2020 14:42

DavisRayM commented Sep 22, 2020

View reviewed changes

pyxform/question.py Outdated Show resolved Hide resolved

DavisRayM mentioned this pull request Sep 22, 2020

Add support for choice from previous repeat answers #381

Closed

lognaturel requested changes Sep 22, 2020

View reviewed changes

DavisRayM force-pushed the choice-from-previous-repeat-answers branch 4 times, most recently from e01adae to 67b1b4c Compare September 24, 2020 13:15

Test choices from previous repeat answers with choice filter

4ca43f3

DavisRayM force-pushed the choice-from-previous-repeat-answers branch 2 times, most recently from 7aeb1fb to 4ca43f3 Compare September 24, 2020 13:55

Add support for choices from previous repeat answers with choice filter

ba03680

DavisRayM force-pushed the choice-from-previous-repeat-answers branch from 73f0b2d to 7736a48 Compare September 24, 2020 14:00

Update default predicate for choices from previous repeat answers

41c36ad

DavisRayM force-pushed the choice-from-previous-repeat-answers branch from 7736a48 to 41c36ad Compare September 24, 2020 14:02

lognaturel mentioned this pull request Sep 24, 2020

Itemset from previous answers - select one from ${node} #38

Closed

DavisRayM force-pushed the choice-from-previous-repeat-answers branch from d3af776 to 5d96e21 Compare September 28, 2020 09:30

DavisRayM added 2 commits September 28, 2020 12:36

Test choices from previous repeat answers in nested repeat

206f6ab

Set default value for 'multi_language' variable

a635a5a

DavisRayM force-pushed the choice-from-previous-repeat-answers branch from 49a6bf6 to a635a5a Compare September 28, 2020 09:36

DavisRayM marked this pull request as ready for review September 29, 2020 08:21

DavisRayM requested review from lognaturel and MartijnR October 9, 2020 13:31

MartijnR mentioned this pull request Oct 9, 2020

current() specification is too narrow getodk/xforms-spec#276

Open

MartijnR requested changes Oct 9, 2020

View reviewed changes

pyxform/tests_v1/test_repeat.py Outdated Show resolved Hide resolved

DavisRayM changed the title ~~Add support for choice from previous repeat answers~~ [WIP] Add support for choice from previous repeat answers Oct 9, 2020

Test that a relative nodeset is generated for select from previous

aa7d9b5

lognaturel requested changes Oct 23, 2020

View reviewed changes

lognaturel reviewed Oct 23, 2020

View reviewed changes

pyxform/survey.py Show resolved Hide resolved

DavisRayM added 2 commits October 24, 2020 14:40

Update choice from previous repeat answers with choice_filter test case

2d020ba

Populate itemset instead of list_name value

8eb48d0

DavisRayM force-pushed the choice-from-previous-repeat-answers branch from c21aaee to 8eb48d0 Compare October 28, 2020 06:52

DavisRayM added 2 commits October 28, 2020 09:54

Disable "run_odk_validate" functionality for select from previous tests

37d0312

Reference shared parent if context is a child of the target xpath parent

4aebb2c

DavisRayM requested a review from lognaturel October 28, 2020 09:28

lognaturel previously approved these changes Oct 28, 2020

View reviewed changes

pyxform/tests_v1/test_repeat.py Outdated Show resolved Hide resolved

Remove Validate run

32b17c7

DavisRayM dismissed lognaturel’s stale review via 32b17c7 October 29, 2020 09:24

DavisRayM mentioned this pull request Oct 29, 2020

Add "select one from" to the supported question types XLSForm/xlsform.github.io#210

Closed

lognaturel approved these changes Oct 29, 2020

View reviewed changes

MartijnR approved these changes Nov 5, 2020

View reviewed changes

lognaturel merged commit 9520bd2 into master Nov 6, 2020

lognaturel deleted the choice-from-previous-repeat-answers branch November 6, 2020 17:11

lognaturel mentioned this pull request Dec 9, 2020

Make value ref dynamic for selects from repeats #493

Merged

4 tasks

joshuaberetta mentioned this pull request Jun 2, 2021

Support choice from previous repeat answers for select_one kobotoolbox/formpack#249

Merged

Add support for choice from previous repeat answers #472

Add support for choice from previous repeat answers #472

Conversation

DavisRayM commented Sep 22, 2020

Why is this the best possible solution? Were any other approaches considered?

What are the regression risks?

Does this change require updates to documentation? If so, please file an issue here and include the link below.

Before submitting this PR, please make sure you have:

codecov-commenter commented Sep 22, 2020 • edited Loading

Codecov Report

lognaturel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartijnR Sep 24, 2020 • edited Loading

Choose a reason for hiding this comment

MartijnR Sep 24, 2020 • edited Loading

Choose a reason for hiding this comment

lognaturel Sep 24, 2020 • edited Loading

Choose a reason for hiding this comment

DavisRayM Sep 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavisRayM Oct 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lognaturel left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lognaturel Oct 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lognaturel Oct 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavisRayM Oct 28, 2020 • edited Loading

Choose a reason for hiding this comment

lognaturel left a comment

Choose a reason for hiding this comment

DavisRayM commented Oct 29, 2020

MartijnR commented Nov 2, 2020 via email • edited Loading

lognaturel commented Nov 4, 2020

MartijnR commented Nov 4, 2020

lognaturel commented Nov 4, 2020

MartijnR commented Nov 4, 2020 • edited Loading

lognaturel commented Nov 4, 2020

MartijnR commented Nov 4, 2020 • edited Loading

lognaturel commented Nov 4, 2020 • edited Loading

MartijnR commented Nov 4, 2020 • edited Loading

lognaturel commented Nov 5, 2020

MartijnR left a comment

Choose a reason for hiding this comment

lognaturel commented Nov 6, 2020

codecov-commenter commented Sep 22, 2020 •

edited

Loading

MartijnR Sep 24, 2020 •

edited

Loading

MartijnR Sep 24, 2020 •

edited

Loading

lognaturel Sep 24, 2020 •

edited

Loading

DavisRayM Sep 29, 2020 •

edited

Loading

DavisRayM Oct 19, 2020 •

edited

Loading

lognaturel left a comment •

edited

Loading

lognaturel Oct 23, 2020 •

edited

Loading

lognaturel Oct 23, 2020 •

edited

Loading

DavisRayM Oct 28, 2020 •

edited

Loading

MartijnR commented Nov 2, 2020 via email •

edited

Loading

MartijnR commented Nov 4, 2020 •

edited

Loading

MartijnR commented Nov 4, 2020 •

edited

Loading

lognaturel commented Nov 4, 2020 •

edited

Loading

MartijnR commented Nov 4, 2020 •

edited

Loading