XLSForm / pyxform

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

Choice_filter with options containing outputs does not expand ${node} references #515

Closed MartijnR closed 3 years ago

MartijnR commented 3 years ago

Software and hardware versions

pyxform v1.3.4

Problem description

Using a choice list with labels containing ${node} refs (XForm <output>s), does not expand the ${node} refs, if the form contains only 1 language.

Steps to reproduce the problem

Note: ODK Validate does not validate the result, so test with --skip_validate.

choices:

list_name name label
choices one One - ${txt}
choices two Two - ${txt}

survey:

type name label choice_filter default
text txt Enter text   default
select_one choices one Select one 1 < 2  

The output contains:

<instance id="choices">
    <root>
        <item>
            <label>1 - ${txt}</label>
            <name>one</name>
        </item>
        <item>
            <label>2 - ${txt}</label>
            <name>two</name>
        </item>
    </root>
</instance>

It contains no <itext> block.

and:

<itemset nodeset="instance('choices')/root/item[1 &lt; 2]">
    <value ref="name" />
    <label ref="label" />
</itemset>

Expected behavior

The output should contain (I think):

<itext>
    <translation>
        <text id="choices-0">
            <value>
                One -
                <output value=" /data/txt " />
            </value>
        </text>
        <text id="choices-1">
            <value>
                Two -
                <output value=" /data/txt " />
            </value>
        </text>
    </translation>
</itext>
...
<instance id="choices">
    <root>
        <item>
            <itextId>choices-0</itextId>
            <name>one</name>
        </item>
        <item>
            <itextId>choices-1</itextId>
            <name>two</name>
        </item>
    </root>
</instance>

and (see itext() in label ref):

<itemset nodeset="instance('choices')/root/item[1 &lt; 2]">
     <value ref="name" />
     <label ref="jr:itext(itextId)" />
</itemset>

Other information

When changing the same form to use multiple languages (forcing itext usage), the problem does not occur. See here

MartijnR commented 3 years ago

I imagine that writing a useful test for this issue might be a bit of a challenge. Just wanted to highlight #448 in case it is helpful to add as part of this work.

pbowen-oc commented 3 years ago

@MartijnR - If the choice filter is omitted there also doesn't seem to be an <itext> block, but the node reference is expanded as expected in that case. That case also doesn't seem to trigger ODK Validate errors.

@gushil has started working on an update that keeps the existing structure but expands the node references.

We will need guidance on what the preferred solution will be.

MartijnR commented 3 years ago

@pbowen-oc, since a choice-filter is used, the choices will have to be in an <instance> and you cannot put an <output> element in there afaik. The output is entirely different when no choice-filter (fixed list) is used, so I don't think that offers any example for how to fix this bug. But please post a proposed alternative output below as there are likely other solutions.

lognaturel commented 3 years ago

Does that mean #403 should be closed, @MartijnR?

Related: #474, https://github.com/XLSForm/pyxform/issues/403#issuecomment-663754177

MartijnR commented 3 years ago

Ah thanks for looking into this @lognaturel. I had a feeling this was discussed elsewhere... The comment you linked above seems to be exactly this issue. I think #403, should stick to dealing with the name column only and would therefore be different. I'll add a comment about that.

I'll paste your https://github.com/XLSForm/pyxform/issues/403#issuecomment-663754177 below, for helpfulness:

I think the weirdness comes in when there are secondary instances/dynamic choice lists. @MartijnR should output be allowed and generated by pyxform for those labels? I did some quick exploration and there's something really odd going on. Seems that on XLSForm Online translations are not generated for secondary instances if there's only a single language. Seems right to me but it breaks this type of form with a secondary instance because pyxform doesn't expand the ${} references to XPath paths in outputs.

However, I can't seem to reproduce this at the command line with pyxform v1.0.0, v1.1.0 or master. There, I get generated translations with outputs and expanded XPath paths (doesn't seem ideal but does make the original form from the issue work). I wonder whether it could somehow be related to Python version or something crazy like that?

I don't have time to dig in deeply now so I propose we do a release sooner than later and right after the release compare the behavior in XLSForm Online and pyxform to see whether we still have a problem or not.

As for whether <output> should be generated for labels at all for this case, I am a little ambivalent.

I doubt we'll ever figure out how to get this to work with external data, which is an argument not to support it. Also in Enketo this probably won't work in the near future if the selects have a minimal appearance (but that's an Enketo technical problem and shouldn't really be argument).

The real argument for supporting this, and why I posted this issue, is that it works without choice_filters and it works with choice_filters for forms with translations.

gushil commented 3 years ago

@MartijnR I've create PR #518 to solve this issue. Waiting for review.

Thanks.

gushil commented 3 years ago

Hi @MartijnR

Added some changes to address the feedback to PR #518

Waiting for review.

Thanks.

pbowen-oc commented 3 years ago

@lognaturel - Do you have a timeline for tagging a new version with this and #524? We are eager to get these into our validation process and deployed.

lognaturel commented 3 years ago

Will try to do a release Friday.

pbowen-oc commented 3 years ago

@lognaturel - Sounds great. Thanks!