Add support for choice from previous repeat answers

[DavisRayM]

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:

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

[codecov-commenter]

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%)	:arrow_up:
pyxform/xls2json.py	77.96% <100.00%> (+0.06%)	:arrow_up:

---

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.

[codecov-io]

Codecov Report

Merging #472 into master will increase coverage by 0.08%. The diff coverage is 95.65%.

@@            Coverage Diff             @@
##           master     #472      +/-   ##
==========================================
+ Coverage   83.53%   83.61%   +0.08%     
==========================================
  Files          25       25              
  Lines        3608     3638      +30     
  Branches      838      845       +7     
==========================================
+ Hits         3014     3042      +28     
  Misses        452      452              
- Partials      142      144       +2     

Impacted Files	Coverage Δ
pyxform/survey.py	92.42% <93.33%> (-0.13%)	:arrow_down:
pyxform/question.py	93.30% <100.00%> (+0.34%)	:arrow_up:
pyxform/xls2json.py	77.89% <100.00%> (ø)

---

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...32b17c7. Read the comment docs.

[DavisRayM]

Documentation Update issue: https://github.com/XLSForm/xlsform.github.io/issues/210

[MartijnR]

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 notifications@github.com wrote:

@lognaturel approved this pull request.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/XLSForm/pyxform/pull/472#pullrequestreview-519890612, 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]

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]

I think there would be two solutions.

1. 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.
2. Enketo re-adds support for relative path references as ref and nodeset attribute values.

[lognaturel]

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]

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]

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]

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]

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]

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]

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?

[lognaturel]

Thanks all, and especially @DavisRayM! 🎉