Improve repeat count XForm output #435

MartijnR · 2020-03-02T23:15:10Z

I think there ~~are 2~~ is an issue with how pyxform transforms repeat_count values:

1.It places a new count node as sibling of the repeat. This means that any relevant condition on an ancestor of that repeat clears the repeat_count values and removes all repeats. This is not always desirable for implementations that want to keep irrelevant values stored in the model.

The output is more verbose than necessary for simple ${node} references or static numbers.

See example below with the 3 different types of repeat_count values we support. I propose to change the output ~~of all 3~~ the first and the third.

type	name	label	repeat_count
integer	count	Enter count
begin repeat	rep1	Repeat with user entered count	${count}
text	text1	Enter text
end repeat
begin repeat	rep2	Repeat with calculated count	${count} + 2
text	text2	Enter text
end repeat
begin repeat	rep3	Repeat with fixed count	4
text	text3	Enter text
end repeat

Google Sheets

current output (relevant parts):

        <model odk:xforms-version="1.0.0">
            <instance>
                <data id="repeat-count-cases">
                    <count/>
                    <group>
                        <rep1_count/>
                        <rep1 jr:template="">
                            ...
                        </rep1>
                        <rep1>
                            ...
                        </rep1>
                        <rep2_count/>
                        <rep2 jr:template="">
                            ...
                        </rep2>
                        <rep2>
                            ...
                        </rep2>
                        <rep3_count/>
                        <rep3 jr:template="">
                            ...
                        </rep3>
                        <rep3>
                            ...
                        </rep3>
                    </group>
                    ...
                </data>
            </instance>
            <bind nodeset="/data/count" type="int"/>
            <bind calculate=" /data/count " nodeset="/data/group/rep1_count" readonly="true()" type="string"/>
            <bind calculate=" /data/count  + 2" nodeset="/data/group/rep2_count" readonly="true()" type="string"/>
            <bind calculate="4" nodeset="/data/group/rep3_count" readonly="true()" type="string"/>
            ...
        </model>
    </h:head>
    <h:body>
        <input ref="/data/count">
            <label>Enter count</label>
        </input>
        <group ref="/data/group">
            <group ref="/data/group/rep1">
                <label>Repeat with user entered count</label>
                <repeat jr:count=" /data/group/rep1_count " nodeset="/data/group/rep1">
                    ...
                </repeat>
            </group>
            <group ref="/data/group/rep2">
                <label>Repeat with calculated count</label>
                <repeat jr:count=" /data/group/rep2_count " nodeset="/data/group/rep2">
                    ...
                </repeat>
            </group>
            <group ref="/data/group/rep3">
                <label>Repeat with fixed count</label>
                <repeat jr:count=" /data/group/rep3_count " nodeset="/data/group/rep3">
                    ...
                </repeat>
            </group>
        </group>
    </h:body>

A cleaner, and less problematic, output would be:

<model odk:xforms-version="1.0.0">
            <instance>
                <data id="repeat-count-cases">
                    <count/>
                    <group>
                        <rep1 jr:template=""><!-- NOTE: see body and bind, no additional node or calculation has to be created by pyxform for this case-->
                            ...
                        </rep1>
                        <rep1>
                            ....
                        </rep1>
                        <rep2_count/>
                        <rep2 jr:template=""><!-- NOTE: unchanged -->
                            ....
                        </rep2>
                        <rep2>
                           ...
                        </rep2>
                    
                        <rep3 jr:template=""><!-- NOTE: see body and bind, no additional node or calculation has to be created by pyxform for this case-->
                           ....
                        </rep3>
                        <rep3>
                           ...
                        </rep3>
                           ...
                      </group>
                </data>
            </instance>
            ....

            <bind calculate=" /data/count  + 2" nodeset="/data/group/rep2_count" readonly="true()" type="string"/>
            
             ....
        </model>
    </h:head>
    <h:body>
        <input ref="/data/count">
            <label>Enter count</label>
        </input>
        ....
            <repeat jr:count=" /data/count " nodeset="/data/rep1">
                 .....
            </repeat>
       .....
            <repeat jr:count=" /data/group/rep2_count " nodeset="/data/rep2">
                 .....
            </repeat>
        ....
            <repeat jr:count="4" nodeset="/data/rep3">
                .....
            </repeat>
    </h:body>

MartijnR · 2020-03-16T16:24:44Z

Would a PR for this, with useful tests for these 3 ways of using repeat-count, be welcomed?

gushil · 2020-04-27T09:33:37Z

Hi @MartijnR I'm trying to fix this issue. Which best route should I go first? Currently I'm trying to fix this by changing workbook_to_json method in xls2json.py. Is that the right one?

gushil · 2020-04-27T14:00:11Z

Also, could you please attach desired xml file here? I found that desired xml above cannot pass odk validator. Thanks!

MartijnR · 2020-04-27T15:21:35Z

Hi @gushil,

Here is the XForm:
repeat-count-cases.xml.txt

I confirmed ODK Validate doesn't accept the 3rd case jr:count="4". However, the syntax is correct so that looks like a bug in Javarosa (used in ODK Collect & ODK Validate). => getodk/javarosa#399

I'll have a look at pyxform code to see if I can tell where to make the changes. I'm not much of a pyxform expert though.

MartijnR · 2020-04-27T17:40:47Z

@gushil, yes, I believe workbook_to_json is the place to be!

Btw, make sure to the use the 'new' style of tests that use markdown (and not XForm files).

Have fun!

lognaturel · 2020-04-27T20:26:27Z

Thanks! We'll try to get the JavaRosa fix sooner than later. @gushil, our typical policy is to wait about a year to merge something that will lead to breaking behavior with one of either ODK Collect or Enketo. You may prefer to take on an issue that could be merged in a more timely way.

lognaturel · 2020-04-27T20:41:49Z

This means that any relevant condition on an ancestor of that repeat clears the repeat_count values and removes all repeats

I don't think this is what is supposed to happen per W3C XForms. @tiritea pointed out a while back that relevance is supposed to be only a display concern and not affect calculations. We may at some point need to explicitly describe what the intended behavior is in ODK XForms and verify that we are aligned across clients.

tiritea · 2020-04-27T22:49:22Z

Its worth noting that although relevance should not affect calculations per se, relevance is still inherited, and so any visible input questions within any enclosed repeats/sub groups should - as a consequence of being made irrelevant due to inheritance - be blanked upon submission.

MartijnR · 2020-04-28T14:58:57Z

Its worth noting that although relevance should not affect calculations per se,

Thanks @lognaturel and @tiritea. I vaguely remember this discussion. Was it in Slack?

our typical policy is to wait about a year to merge something that will lead to breaking behavior with one of either ODK Collect or Enketo.

Good point. We could rewrite the third option to make it the same as the first option for now (if wanting to proceed).

gushil · 2020-04-29T13:33:46Z

@MartijnR I've created a PR #441 but I think some review required. Thanks.

MartijnR · 2020-04-29T20:26:39Z

Thanks @gushil! We're just discussing another approach would actually solve the OpenClinica issue (and would be implemented on the Enketo side). It doesn't directly affect this pyxform issue, which will still be worthwhile, but makes it less of a priority perhaps.

In any case, we'd either have to adjust the specs of this issue to prevent an issue in older ODK Collect clients or just wait (perhaps for a year) before merging it. So I'd still like to review your PR, but will get back to you after deliberating a possible Enketo change first.

MartijnR · 2020-04-29T21:02:05Z

Another excellent issue @lognaturel brought up is that changing the location of the repeat count (second repeat_count case) changes the structure of the record. This could create issues when updating an existing form.

MartijnR · 2020-04-29T21:06:21Z

Another thing that came out of the discussion with @tiritea and @lognaturel is that:

The underlying issue 1 mentioned in the OP, is not actually an XForms issue, because XForms would "relevant prune" non-relevant nodes (including ones without a UI) upon submission, not in the middle of a session.

I'll update the issue by:

removing reason 1
not moving the node for repeat_count case 2

gushil · 2020-04-30T08:12:50Z

@MartijnR PR #441 is updated.

Thanks

lognaturel · 2020-05-27T22:57:03Z

There's an interesting plot twist that I discovered when looking at a possible JavaRosa change. It turns out that JavaRosa very explicitly only supports references and that this is baked in pretty deep. JavaRosa is the originator of jr:count so it seems the ODK XForms spec should have matched that. More at getodk/javarosa#399 (comment).

MartijnR · 2020-05-28T21:19:48Z

Interesting twist indeed! If it's not easy to add support in Javarosa, I'm fine with correcting our spec. (Funnily enough, Enketo didn't support it either until a few months ago when I discovered the spec non-compliance - but it was a simple fix in our case). That would only leave case 1 (which bothers me most anyway because of the duplicate nodes), but as you mentioned in #441 while Aggregate is still used quite a bit, probably best to leave this for now.

- Given a form with an item named "a_count" that holds the desired repeat count, and a repeat called "a", when pyxform injects an internal item suffixed with "_count" to hold the repeat count for "a", then pyxform emits a duplicate survey element error (2x "a_count"). - As described in XLSForm#435 (case 1), when the repeat count is just a reference to another item, then that item could be used directly, instead of injecting an item with a calculate item, thereby causing a name clash and thus the error. - for future reference, this code is a bit confusing but the dict structure of json["control"]["jr:count"] is a result of processing in dealias_and_group_headers (repeat_count -> control::jr:count -> dict). - adds new condition to existing logic, where if the expression parses to a single pyxform reference, then the item injection is skipped.

MartijnR mentioned this issue Mar 2, 2020

See Form Behavior issues email on 2/25 OpenClinica/enketo-express-oc#308

Closed

MartijnR mentioned this issue Apr 27, 2020

Form fails to load when jr:count is set to a number getodk/javarosa#399

Open

gushil mentioned this issue Apr 28, 2020

Improve repeat count XForm output #441

Closed

lognaturel mentioned this issue Oct 12, 2020

jr:count can only be set to a reference getodk/xforms-spec#277

Open

lognaturel mentioned this issue Feb 1, 2022

Use of _count in variable name results in duplicate survey element error #576

Closed

lindsay-stevens mentioned this issue Dec 1, 2023

576: fix _count suffix name clash with repeats targeting another item #674

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve repeat count XForm output #435

Improve repeat count XForm output #435

MartijnR commented Mar 2, 2020 •

edited

Loading

MartijnR commented Mar 16, 2020

gushil commented Apr 27, 2020

gushil commented Apr 27, 2020 •

edited

Loading

MartijnR commented Apr 27, 2020 •

edited

Loading

MartijnR commented Apr 27, 2020

lognaturel commented Apr 27, 2020 •

edited

Loading

lognaturel commented Apr 27, 2020

tiritea commented Apr 27, 2020

MartijnR commented Apr 28, 2020 •

edited

Loading

gushil commented Apr 29, 2020

MartijnR commented Apr 29, 2020

MartijnR commented Apr 29, 2020

MartijnR commented Apr 29, 2020 •

edited

Loading

gushil commented Apr 30, 2020

lognaturel commented May 27, 2020

MartijnR commented May 28, 2020

Improve repeat count XForm output #435

Improve repeat count XForm output #435

Comments

MartijnR commented Mar 2, 2020 • edited Loading

MartijnR commented Mar 16, 2020

gushil commented Apr 27, 2020

gushil commented Apr 27, 2020 • edited Loading

MartijnR commented Apr 27, 2020 • edited Loading

MartijnR commented Apr 27, 2020

lognaturel commented Apr 27, 2020 • edited Loading

lognaturel commented Apr 27, 2020

tiritea commented Apr 27, 2020

MartijnR commented Apr 28, 2020 • edited Loading

gushil commented Apr 29, 2020

MartijnR commented Apr 29, 2020

MartijnR commented Apr 29, 2020

MartijnR commented Apr 29, 2020 • edited Loading

gushil commented Apr 30, 2020

lognaturel commented May 27, 2020

MartijnR commented May 28, 2020

MartijnR commented Mar 2, 2020 •

edited

Loading

gushil commented Apr 27, 2020 •

edited

Loading

MartijnR commented Apr 27, 2020 •

edited

Loading

lognaturel commented Apr 27, 2020 •

edited

Loading

MartijnR commented Apr 28, 2020 •

edited

Loading

MartijnR commented Apr 29, 2020 •

edited

Loading