Skip to content

Generate correct refrence when selecting choices from current repeat question#516

Merged
lognaturel merged 6 commits intoXLSForm:masterfrom
DavisRayM:503-select-from-repeat-internal-reference
Apr 16, 2021
Merged

Generate correct refrence when selecting choices from current repeat question#516
lognaturel merged 6 commits intoXLSForm:masterfrom
DavisRayM:503-select-from-repeat-internal-reference

Conversation

@DavisRayM
Copy link
Contributor

@DavisRayM DavisRayM commented Feb 10, 2021

Closes #503

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

Simplest solution with minimal changes

What are the regression risks?

N/A

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

None

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

@DavisRayM DavisRayM requested a review from lognaturel February 10, 2021 08:49
@DavisRayM DavisRayM changed the title Generate correct refrence when selecting choices from current repeat question [WIP] Generate correct refrence when selecting choices from current repeat question Feb 10, 2021
@lognaturel
Copy link
Contributor

From Slack:
"Especially this change here. I'm unsure of whether the path change is referencing something different... Would a change like that mean that it won't pick the value from the current repeat ?"

Is that what makes it a WIP or is there anything else that you expect to do here, @DavisRayM?

@DavisRayM
Copy link
Contributor Author

From Slack:

"Especially this change here. I'm unsure of whether the path change is referencing something different... Would a change like that mean that it won't pick the value from the current repeat ?"

Is that what makes it a WIP or is there anything else that you expect to do here, @DavisRayM?

Yes, that's the only bit of that I wasn't too sure about

md=xlsform_md,
xml__contains=[
'<itemset nodeset="../../household_mem_rep[ ./age &gt; current()/../target_min_age ]">',
'<itemset nodeset="../../household_mem_rep[ ./age &gt; current()/../../selected/target_min_age ]">',
Copy link
Contributor

@lognaturel lognaturel Feb 17, 2021

Choose a reason for hiding this comment

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

Your intuition is correct that this shouldn't change. It's important to stay inside the current repeat instance. When you go out of it and then back in (../selected/), you're going to end up in the first instance of the repeat.

Collect happens to have the desired behavior but it's not strictly supposed to and it won't work in Enketo.

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've reverted this change in the latest commits and ensured the code passes the test

@lognaturel
Copy link
Contributor

The general case is "if you need to reference a repeat nodeset, your expression must include the repeat node explicitly". In the case of itemsets from repeats, there's always a predicate expression after a nodeset expression. There are cases like parameters to the count function where we need to access the raw nodeset. The problem with a relative expression such as ../[predicate] in the form in the issue is that ../ references a single repeat instance, not the whole nodeset.

In the case of ${target_min_age}, we're not referencing the nodeset, we're referencing a specific value in a specific repeat instance.

…repeat

Test that select one from repeat questions generate the correct
reference field when referencing a question within the same repeat
Comment on why the previous change is not necessary / would cause issues:

#516 (comment)
@codecov-io
Copy link

codecov-io commented Apr 7, 2021

Codecov Report

Merging #516 (fa1afbd) into master (43ea039) will decrease coverage by 0.09%.
The diff coverage is 100.00%.

❗ Current head fa1afbd differs from pull request most recent head a2f784b. Consider uploading reports for the commit a2f784b to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #516      +/-   ##
==========================================
- Coverage   83.94%   83.85%   -0.10%     
==========================================
  Files          25       25              
  Lines        3719     3697      -22     
  Branches      867      862       -5     
==========================================
- Hits         3122     3100      -22     
  Misses        452      452              
  Partials      145      145              
Impacted Files Coverage Δ
pyxform/question.py 93.36% <100.00%> (-0.06%) ⬇️
pyxform/survey.py 92.49% <100.00%> (-0.06%) ⬇️
pyxform/utils.py 84.24% <0.00%> (-0.63%) ⬇️
pyxform/xls2json.py 78.33% <0.00%> (-0.20%) ⬇️
pyxform/constants.py 100.00% <0.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 43ea039...a2f784b. Read the comment docs.

@DavisRayM DavisRayM changed the title [WIP] Generate correct refrence when selecting choices from current repeat question Generate correct refrence when selecting choices from current repeat question Apr 7, 2021
@DavisRayM DavisRayM requested a review from lognaturel April 7, 2021 08:20
@DavisRayM
Copy link
Contributor Author

@lognaturel Kindly re-review when you get a chance

lognaturel
lognaturel previously approved these changes Apr 16, 2021
Copy link
Contributor

@lognaturel lognaturel left a comment

Choose a reason for hiding this comment

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

Thanks so much, @DavisRayM!

@lognaturel lognaturel merged commit 3e60bcc into XLSForm:master Apr 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Wrong reference when building select one from repeat inside that repeat

3 participants