Improve repeat count XForm output

[MartijnR]

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.

2. 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]

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

[gushil]

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]

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

[MartijnR]

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). => https://github.com/getodk/javarosa/issues/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]

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

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]

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]

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]

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]

@MartijnR I've created a PR https://github.com/XLSForm/pyxform/pull/441 but I think some review required. Thanks.

[MartijnR]

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]

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]

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]

@MartijnR PR #441 is updated.

Thanks

[lognaturel]

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 https://github.com/getodk/javarosa/issues/399#issuecomment-634222377.

[MartijnR]

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.