Calculations and relevances within newly-relevant groups do not calculate properly (initially)

[GoogleCodeExporter]

What steps will reproduce the problem?
1. Fill out the attached form.
2. Enter any number into the first prompt.
3. Select YES to continue.
4. Swipe forward.

What is the expected output? What do you see instead?

You should see "secondnum" field. Instead, it jumps to the "alwaysnote" field, 
and "anynumtimes2times2" is shown as blank, as is "secondnum".

However, if you swipe to the end and try to save, it jumps back to the 
"secondnum" field, which it suddenly decides is required, and 
"anynumtimes2times2" is now evaluated properly.

Instead of trying to save, you can also go back and change the initial "anynum" 
selection to get things to process properly.

Please use labels and text to provide additional information.

The problem has to do with relevance and calculate expressions inside the 
"consented" group. Initially, the consented group is not relevant. Then, after 
saying YES to "consent", it suddenly becomes relevant. At that point, 
anynumtimes2 should be evaluated, anynumtimes2times2 should be evaluated, and 
the relevance expression should be evaluated. The last two evaluations depend 
on the first evaluation -- but they don't seem to be sequenced properly. 
Rather, anynumtimes2times2 and the relevance are evaluated as if anynumtimes2 
does not yet have a value. Only later -- if anynum is changed or if you try to 
save the form -- are the latter expressions reconsidered.

I have not seen anything like this before, but it seems serious and potentially 
an issue for lots of cases.

I tested with the latest SurveyCTO Collect (which follows the latest ODK 
sources), and with ODK Collect 1.3 (1030) (the latest version from Google Play).

Original issue reported on code.google.com by chrislro...@gmail.com on 11 Aug 2013 at 2:36

Attachments:

• SampleInterForm.xls

[GoogleCodeExporter]

OK. This is similar to the craziness that I saw when debugging the JR issues 
that Ramon uncovered (and for which I requested people report things they've 
noticed).

I think this is either a long-standing (7-month-old) problem dating from change 
1070 on FormEntryActivity.java (the change in the location of the call to 
createRepeatDialog() ) or an issue with 4.x devices, not anything to do with 
any recent changes in ODK Collect or JR.

I think the 'add group' dialog's add action are being processed BEFORE the 
save-values actions for all the widgets on the screen over which the 'add 
group' dialog is being rendered. 

So that group is initially created with stale values, then as those values are 
updated, they propagate as changes into the formulas within the group.

But, more significantly, this would affect relevance determinations even before 
we get to that, and those relevance determinations would never be revisited.

Original comment by mitchellsundt@gmail.com on 14 Aug 2013 at 10:43

• Added labels: Collect

[GoogleCodeExporter]

Ah, yes, I saw your post about the repeat-group issue, and I thought that maybe 
this was totally separate -- but it makes sense that it could be related.

One note is that this particular issue's behavior is totally reproducible 
across 4.x and 2.3.x devices -- so it's definitely not a 4.x issue.

I suppose one easy test is to try this form on a pre-1070 version of Collect. 
Let me see if I can try that right now...

Original comment by chrislro...@gmail.com on 14 Aug 2013 at 11:29

[GoogleCodeExporter]

I just tried this same form with Collect v1.1.7 from May 2012, and it behaved 
in the very same flawed way. So this particular problem is definitely over a 
year old, and it's definitely a problem on older devices...

Original comment by chrislro...@gmail.com on 14 Aug 2013 at 11:35

[GoogleCodeExporter]

I'm glad this is a long-standing problem. I was afraid it was somehow related 
to the v1.4 change to the new UI / target SDK. It enables me to proceed to 
release v1.4 to Google Play.

I think the fix is to ensure that the save of data on the current screen is 
completed prior to throwing up the add-group dialog. Don't have time to look 
into this.

Original comment by mitchellsundt@gmail.com on 15 Aug 2013 at 4:27

[GoogleCodeExporter]

Hmm, but in this case there's actually no add-group dialog, and indeed no 
repeat group at all. It's just a matter of a normal (non-repeat) group going 
from not-relevant to relevant.

If you have any other ideas for places to look, that would be great. Otherwise, 
yes, we are happy to try and chase this down. If it's been around a long time, 
clearly people can live without the fix for a little while...

Original comment by chrislro...@gmail.com on 15 Aug 2013 at 11:06

[GoogleCodeExporter]

ah, even more interesting. 

Original comment by mitchellsundt@gmail.com on 17 Aug 2013 at 12:18

[GoogleCodeExporter]

I think issue 884 is related to this (but haven't tagged them as duplicates).

Original comment by mitchellsundt@gmail.com on 19 Aug 2013 at 5:34

[GoogleCodeExporter]

This has come up for several clients now. While the workaround is to simply 
move calculate fields outside groups with relevances, we'd like to fix it.

In Condition.java, one clear option is to add an additional step to evaluate 
everything inside a newly-relevant group. We're a bit worried about the 
performance implications (particularly for huge groups), but preliminary tests 
suggest that this works reasonably. Any thoughts?

    public void apply (TreeReference ref, Object rawResult, FormInstance model, FormDef f) {
        boolean result = ((Boolean)rawResult).booleanValue();
        int action = result ? trueAction : falseAction;
        TreeElement node = model.resolveReference(ref);
        performAction(node, action);

        // SCTO-721 - If a group becomes relevant, all chlidren should be evaluated again.
        if (action == ACTION_SHOW && !node.isLeaf() && node.getNumChildren() > 0) {
            for (int i = 0 ; i < node.getNumChildren() ; i++) {
                TreeElement childAt = node.getChildAt(i);
                f.triggerTriggerables(childAt.getRef());
            }
        }
    }

Original comment by chrislro...@gmail.com on 5 Jan 2014 at 9:41

[GoogleCodeExporter]

Actually, though, the non-relevant calculation fields ARE being calculated 
earlier; it's just that their values are not being shared because of these code 
snippets:

        //to fix conditions based on non-relevant data, filter the nodeset by relevancy
        for (int i = 0; i < nodesetRefs.size(); i++) {
            if (!m.resolveReference((TreeReference)nodesetRefs.elementAt(i)).isRelevant()) {
                nodesetRefs.removeElementAt(i);
                i--;
            }
        }

And:

        return unpackValue(node.isRelevant() ? node.getValue() : null);

So, in the example form, anynumtimes2 IS being evaluated even when not 
relevant, but then secondnum is getting a null instead of the evaluated value. 
Then, when anynumtimes2 becomes relevant, the trouble is that secondnum doesn't 
re-evaluate.

If we change the code such that non-relevant fields DO supply their values, 
then this form starts working. But obviously, that would be a major change and 
would very likely cause lots of undesirable behavior in other cases.

So the bottom line is: if non-relevant fields are not going to share their 
evaluated values, then when they become relevant those fields that depend on 
them must be re-evaluated. Not sure if our fix (in my last comment above) is 
the best or most efficient way to do that.

Original comment by chrislro...@gmail.com on 6 Jan 2014 at 12:47

[GoogleCodeExporter]

Original comment by mitchellsundt@gmail.com on 13 Mar 2014 at 11:06

• Added labels: Mar2014, Unknown

[GoogleCodeExporter]

We never instituted this particular fix. We were too scared. So it has remained 
something we work around with customers, by making calculated fields always 
relevant and (and correcting any potential issues with if() inside the 
expression).

Original comment by chrislro...@gmail.com on 14 Mar 2014 at 4:20

[GoogleCodeExporter]

Agreed. I am trying to pull in some changes from Commcare -- in particular the 
current() qualifier (similar to instance('name'), but refers to the active 
form).

In the course of that, merge, I am looking at code that appears to widen what 
triggers are considered to be fired when a group or field becomes relevant. 
This may fix this issue.  Tagged as unknown because I don't know whether what 
I've seen is actually handling this case or not.

Original comment by mitchellsundt@gmail.com on 14 Mar 2014 at 11:11

[GoogleCodeExporter]

Thanks, Mitch. Can you test with the sample form an scenario described in the 
original issue? Would be wonderful if this went away.

Original comment by chrislro...@gmail.com on 15 Mar 2014 at 3:18

[GoogleCodeExporter]

Looks like this works on this build!

Original comment by mitchellsundt@gmail.com on 20 Mar 2014 at 12:45

[GoogleCodeExporter]

Fixed in 1.4.3

Original comment by mitchellsundt@gmail.com on 19 May 2014 at 4:24

• Changed state: Fixed

[GoogleCodeExporter]

Fix seems to come with massive performance cost. See 
https://groups.google.com/forum/#!topic/opendatakit-developers/hc8JtvpJyzE for 
more.

Original comment by chrislro...@gmail.com on 4 Dec 2014 at 2:17