XLSForm / pyxform

A Python package to create XForms for ODK Collect.
BSD 2-Clause "Simplified" License
77 stars 134 forks source link

When indexed-repeat is used within a repeat the first argument nodeset points to single node using relative path #484

Closed MartijnR closed 3 years ago

MartijnR commented 3 years ago

Software and hardware versions

2.2.1

Problem description

The first argument of indexed-repeat() calls is relative when called from inside a repeat and referring to a question in that repeat. It should be absolute.

A ../relative/path/to/question always returns a single node (in ODK XForms) and according to the spec it should refer to all nodes with that question name, i.e. the nodeset returned by /absolute/path/to/question.

Steps to reproduce the problem

A. using regular calculation with relative index:

type name label calculation
begin_repeat person Person  
integer pos position(..)
text name Enter name  
text prev_name Name in previous repeat instance indexed-repeat(${name}, ${person}, ${pos}-1)
end_repeat      

outputs:

<bind calculate="indexed-repeat( ../name ,  /data/person , ../pos -1)"  ... />

B. using dynamic default:

type name label default
begin_repeat person Person  
text name Enter name  
text prev_name Name in previous repeat instance indexed-repeat(${name}, ${person}, position(..)-1)
end_repeat      

outputs:

<setvalue ... value="indexed-repeat( ../name ,  /data/person , position(..)-1)" />

C. using dreaded nested repeats:

type name label default
begin_repeat family Family  
integer members_number How many members in this family?  
begin_repeat person Person  
text name Enter name  
text prev_name Non-sensible previous name in first family, 2nd person indexed-repeat(${name}, ${family}, 1, ${person}, 2)
end_repeat      
end_repeat      

outputs:

<setvalue .. value="indexed-repeat( ../name ,  /data/family , 1,  ../../person , 2)" />

Expected behavior

A:

<bind calculate="indexed-repeat( /data/person/name ,  /data/person , ../pos - 1)" ... />

B:

<setvalue ... value="indexed-repeat( /data/person/name ,  /data/person , position(..)-1)" />

C:

<setvalue .. value="indexed-repeat( /data/family/person/name ,  /data/family , 1, /data/family/person , 2)" />

So the first argument of indexed-repeat() is an exception to the relative-path rule used to transform ${node} syntax (and the second argument too but that seems to already be the case).

Or actually since indexed-repeat() has either 3, 5, or 7 arguments, this exception applies to the 1st, 2nd, 4th, and 6th arguments.

Other information

Previously using indexed-repeat() inside a repeat was a bit of an edge case. Since the introduction of dynamic defaults it is becoming more common, as you could use the answer of a previous repeat question as the dynamic default of that same question in a new repeat instance (example B).

I'm guessing this issue was introduced in 0.12.0 when we changed to relative node references inside repeats.

MartijnR commented 3 years ago

ping @pbowen-oc

pbowen-oc commented 3 years ago

Thanks for logging this, @MartijnR

@gushil - Can you please work on this?

gushil commented 3 years ago

@pbowen-oc Working on it.

gushil commented 3 years ago

Hi @MartijnR, I've created PR #485 to fix this issue. Waiting for review. Thanks

MartijnR commented 3 years ago

Looks like the form posted here slips through the cracks: https://github.com/OpenClinica/enketo-express-oc/issues/398#issuecomment-745082006 . See both (similar) indexed-repeat() calls.

One of them is: if(position(..) = 1, '', indexed-repeat(${bp_sys}, ${bp_rg}, position(..) - 1))

pbowen-oc commented 3 years ago

@gushil - Can you take a look at this?

gushil commented 3 years ago

@pbowen-oc working on it

gushil commented 3 years ago

@MartijnR I've created a new PR (#499) to solve the issue @pbowen-oc mentioned.

Thanks.