Add "when" column to use value changes as triggers for calculations

[gushil]

#438

[MartijnR]

Thank you! I'll do some black box testing.

I believe the pyxform devs much prefer the new style of markdown-table testing without using xlsx files (and definitely without XForm result files). You basically just cut-and-paste the markdown tables I added to the issue. Sorry about that. I did mention it in the issue though ;).

[MartijnR]

All looks functional to me @gushil! I'm pretty darn excited about this feature! It's huge. Thanks for your work on this.

• There is a minor 'black' code style issue, see circleCI.
• Use the new style of testing instead.

The main issue I'm aware of with using the old-style tests (using files) is that it becomes quite a pain when universal pyxform improvements are made that affect the output of all/most XForms. E.g. adding or changing a version attribute, or changing the syntax for the instanceID bind.

I'll review those converted tests as well when you're ready (note, we should test the absence of form controls as well). When that's done we can ask e.g. @lognaturel to review the code.

[codecov-io]

Codecov Report

Merging #442 into master will increase coverage by 0.28%.
The diff coverage is 95.45%.

@@            Coverage Diff             @@
##           master     #442      +/-   ##
==========================================
+ Coverage   82.83%   83.11%   +0.28%     
==========================================
  Files          23       25       +2     
  Lines        3402     3589     +187     
  Branches      790      833      +43     
==========================================
+ Hits         2818     2983     +165     
- Misses        439      460      +21     
- Partials      145      146       +1     

Impacted Files	Coverage Δ
pyxform/question.py	92.85% <86.66%> (+0.26%)	⬆️
pyxform/builder.py	78.34% <100.00%> (+2.94%)	⬆️
pyxform/survey_element.py	90.17% <100.00%> (+0.08%)	⬆️
pyxform/__init__.py	100.00% <0.00%> (ø)
pyxform/utils.py	83.91% <0.00%> (ø)
pyxform/survey.py	91.62% <0.00%> (+0.22%)	⬆️

---

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 3d2e6dd...3b3b360. Read the comment docs.

[gushil]

Hi @MartijnR thanks for the feedbacks.

All addressed.

Thanks.

[lognaturel]

Thanks so much for this, @gushil, and I'll try to review late this week or early next week. Like @MartijnR said, this is great functionality that has been a long time coming to pyxform!

I should have a Validate/JavaRosa fix shortly for value="". However, @MartijnR, since the value is static in this case, what do you think about outputting <setvalue event="xforms-value-changed" ref="/my/ref"></setvalue> instead? I have a slight preference for going that direction.

[mberg]

What does this allow? Sounds exciting

…

On Wed, May 13, 2020 at 8:20 PM Hélène Martin ***@***.***> wrote: Thanks so much for this, @gushil <https://github.com/gushil>, and I'll try to review late this week or early next week. Like @MartijnR <https://github.com/MartijnR> said, this is great functionality that has been a long time coming to pyxform! I should have a Validate/JavaRosa fix shortly for value="". However, @MartijnR <https://github.com/MartijnR>, since the value is static in this case, what do you think about outputting <setvalue event="xforms-value-changed" ref="/my/ref"></setvalue> instead? I have a slight preference for going that direction. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#442 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAOEQKCT6L7WR2LPDSLL7TRRM2LFANCNFSM4M4CLUHA> .

[lognaturel]

@mberg See more in #438 and https://getodk.github.io/xforms-spec/#event:xforms-value-changed. When one node's value changes, that can trigger another node's value to change exactly once. For example, when you set your name, maybe your location is set to a default but you can override it if you're elsewhere today. It's an extension of dynamic defaults and addresses the limitations described in the tip in that section.

[MartijnR]

Tests look great to me. Thanks @gushil!

However, @MartijnR, since the value is static in this case, what do you think about outputting instead? I have a slight preference for going that direction.

I'm fine with that (especially after checking that Enketo's implementation supports that ;)). I have a slight preference for an explicit value="", but it seems that the W3C spec likes both and even clearly mentions your proposal. Thanks for reviewing @lognaturel!

[gushil]

Hi @MartijnR @lognaturel
Just to confirm. What is the conclusion about explicit/not-explicit value ? Should I make any changes to the code.

Thanks

[MartijnR]

@gushil, let's do what @lognaturel prefers so <setvalue event="xforms-value-changed" ref=" /when-column/d " /> instead of <setvalue event="xforms-value-changed" ref=" /when-column/d " value=""/> It would be great if you could make that change. Thanks!

[dorey]

Hi. I have a few questions about this proposed change.

1. The "when" column only applies to calculate questions questions with "calculation" values? Are there other types of questions where a value in the "when" column would be expected to have meaning?

2. Are all valid values in the format "${other_question}" ? Are there more complicated values that one would expect to work?

3. And lastly, could there be a better name for it than when? Perhaps something more explicit like: "calculate_after": "${some_question}"

I know this has been hardcoded in many locations in this PR but I just wanted to put it up for discussion. Thanks

[codecov-commenter]

Codecov Report

Merging #442 into master will increase coverage by 0.25%.
The diff coverage is 95.45%.

@@            Coverage Diff             @@
##           master     #442      +/-   ##
==========================================
+ Coverage   82.83%   83.08%   +0.25%     
==========================================
  Files          23       25       +2     
  Lines        3402     3589     +187     
  Branches      790      833      +43     
==========================================
+ Hits         2818     2982     +164     
- Misses        439      460      +21     
- Partials      145      147       +2     

Impacted Files	Coverage Δ
pyxform/question.py	92.85% <86.66%> (+0.26%)	⬆️
pyxform/builder.py	78.34% <100.00%> (+2.94%)	⬆️
pyxform/survey_element.py	90.17% <100.00%> (+0.08%)	⬆️
pyxform/__init__.py	100.00% <0.00%> (ø)
pyxform/utils.py	83.91% <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 3d2e6dd...f6bb048. Read the comment docs.

[gushil]

Hi @MartijnR
setvalue value attribute feedback is addressed with latest commit.

Thanks

[lognaturel]

Thanks again, @gushil! I did a preliminary review today but there are still some things I want to verify.

Did you consider performance when coming up with this code structure? Did you model your approach on any other existing code/feature? The approach I had in mind is somewhat different and I’m wondering whether there were particular pressures that led to this solution.

[gushil]

Hi @lognaturel

I haven't implement this with performance in mind. I do this as a learning exercise to better know structure of the code. I'm open to any suggestion.

Thanks

[lognaturel]

I had a hard time putting the directions I had in mind into words and had to try things out in code: master...lognaturel:when-column

The major structural differences are:

• when expressions are identified in the pass over the question dictionary that survey elements are built from rather than in a new pass. Surveys can have thousands of questions and avoiding traversing the whole structure is ideal (even though that's unlikely to be a major performance issue). I think it's also slightly easier to reason about without an additional recursive function.
• when/xforms-value-changed setvalue expressions are stored in a dictionary for quick lookup. I doubt there'd be huge numbers of these in a form but it seemed like a logical way to organize things anyway.
• survey is responsible for holding the map described above and child survey elements look up their nested setvalue actions instead of each survey element keeping track of its own setvalue actions

What are your thoughts on some of these alternatives? I don't think any of them are absolutely necessary but I did want to reason through them.

I also noticed some behavior that wasn't covered:

• when works with triggering nodes of control types other than input (e.g. multiple choice, rank, etc)
• XPath references are expanded in value expressions
• invalid trigger node references cause errors

[gushil]

Thanks for the review @lognaturel Will look into it tomorrow.

[MartijnR]

@gushil, after @dorey's intervention, we discussed further and concluded that we'd like to change when to trigger. I'll update the issue.

[gushil]

@lognaturel
Your implementation is an eye opener for me. I agree with what you mentioned above.
I feel that my implementation is either too complex (adding new recursive, etc) or not in a right place.

[gushil]

Should we merge @lognaturel 's implementation then? Obviously with some modification in changing when column to trigger column.

[MartijnR]

Ah wow, I hadn't realized you implemented this too @lognaturel as part of your review! Thank you!

[lognaturel]

It was really helpful to have @gushil's approach to work from! How about I make the name change on my branch and then get a PR off that? I should be able to do that in the next couple of days.

[gushil]

@lognaturel
Actually I've made a new PR ( #447 ) by merging your when-value branch changes and doing the name changes. Waiting for your review.

[lognaturel]

Replaced by #447