Fix relevance for prompts

[seadowg]

Closes getodk/collect#4090.

This changes how repeat relevance is calculated so that it checks the template as well as the node.

What has been done to verify that this works as intended?

Added new tests and verified in Colllect.

Why is this the best possible solution? Were any other approaches considered?

There are a few things to improve:

• The @Ignored FormDefTest test needs rewritten so it works with this new implementation
• We should remove FormDef#isRepeatRelevant

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Should just fix the bug! Might be good to check any weird repeat relevance cases that can be thought of.

Do we need any specific form for testing your changes? If so, please attach one.

This is a good form to check the fix works: https://docs.google.com/spreadsheets/d/1pc9LPOR8s5C2uFjMj1JbuF6_PPLQ_Q2l6X6ytAZvul0/edit#gid=0

[lognaturel]

I did a little more digging and realized that the repeat template is actually part of the mainInstance which is a tree of TreeElements representing the instance being filled in. Templates are represented as TreeElements with a multiplicity of TreeReference.INDEX_TEMPLATE. Like all TreeElements, they keep track of their relevance. I think this leads to a straightforward change to isRepeatRelevant:

public boolean isRepeatRelevant(TreeReference repeatRef) {
    TreeElement repeatNode = mainInstance.resolveReference(repeatRef);

    if (repeatNode == null) {
        TreeElement templateNode = mainInstance.getTemplate(repeatRef);
        return templateNode.isRelevant();
    }

    return repeatNode.isRelevant();
}

It could also be written as a boolean expression but this feels clearer to me. It also doesn't really feel like it belongs in FormDef. It could go in FormEntryModel.

I don't think there's anything else from the logic at https://github.com/getodk/javarosa/pull/581/files#diff-b60f702426880ccc5f1b079019104bd4L537 that we need. To be extra safe, it might make sense to have a test where there's an outer group that's irrelevant and check that it's indeed impossible to add an instance of an inner repeat. But it shouldn't even be possible to navigate inside the outer group to try. Looking forward to seeing what you think, @seadowg.

Side note: I forgot the mainInstance has templates because I think of it as concrete. And also the multiplicity constants are in TreeReference.

[lognaturel]

And to be extra clear, would be great if you could finalize the PR, @seadowg! Since this is a bug, I think we can do a point release. And I think it would be ok to change the public interface by getting rid of FormDef.isRepeatRelevant. We don't know of many non-ODK tools using this library. And for those that do, it's really unlikely they're using that particular method.

[codecov-commenter]

Codecov Report

Merging #595 into master will increase coverage by 0.04%.
The diff coverage is 38.00%.

@@             Coverage Diff              @@
##             master     #595      +/-   ##
============================================
+ Coverage     54.68%   54.72%   +0.04%     
- Complexity     3218     3219       +1     
============================================
  Files           242      242              
  Lines         13320    13332      +12     
  Branches       2562     2563       +1     
============================================
+ Hits           7284     7296      +12     
- Misses         5230     5232       +2     
+ Partials        806      804       -2     

Impacted Files	Coverage Δ	Complexity Δ
...ain/java/org/javarosa/form/api/FormEntryModel.java	52.96% <16.21%> (+0.15%)	66.00 <0.00> (ø)
src/main/java/org/javarosa/core/model/FormDef.java	71.07% <100.00%> (+0.12%)	158.00 <0.00> (-1.00)	⬆️
...n/java/org/javarosa/core/model/TriggerableDag.java	74.38% <100.00%> (+0.55%)	80.00 <0.00> (+1.00)
.../java/org/javarosa/core/model/utils/DateUtils.java	59.26% <0.00%> (+0.28%)	77.00% <0.00%> (+1.00%)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9a1ddc6...1e87049. Read the comment docs.

[seadowg]

And I think it would be ok to change the public interface by getting rid of FormDef.isRepeatRelevant

@lognaturel I agree but there is test coverage to move around, so I think for this PR it'd be best to just add the implementation in FormDef for the moment.

[seadowg]

@lognaturel ach it looks like the template solution doesn't work for dynamic relevance. I've added a test and will keep exploring.

[lognaturel]

This test could possibly have helped us notice the bug here. The relevance of the inner group is based on the position of the outer group but I think I convinced myself it was based on the position of the inner group (and that's what I was thinking when we talked, @seadowg).

In the original test, the first assertion is:

scenario.next();
scenario.next();
assertThat(scenario.refAtIndex(), is(getRef("/data/outer[0]/inner[1]")));

This is wrong because of the bug this PR fixes. Any inner group should be non-relevant based on the outer group's index. The form definition instance only has one instance of the inner repeat. It turns out the event emitted is actually a repeat prompt (because of this bug). The rest of the test makes sense.

Below is a more explicit proposal. I put two instances of the outer repeat and two inner instances in the first outer instance. That way we can check that initially none of the inner repeats in the first outer repeat are relevant and that there's no prompt.

Test
public void nestedRepeatRelevance_updatesBasedOnParentPosition() throws IOException {
    Scenario scenario = Scenario.init("Nested repeat relevance", html(
        head(
            title("Nested repeat relevance"),
            model(
                mainInstance(t("data id=\"nested-repeat-relevance\"",
                    t("outer",
                        t("inner",
                            t("q1")
                        ),
                        t("inner",
                            t("q1")
                        )
                    ),
                    t("outer",
                        t("inner",
                            t("q1")
                        )
                    ),
                    t("relevance-condition", "0")
                )),
                bind("/data/relevance-condition").type("string"),
                bind("/data/outer/inner").relevant("position(..) mod 2 = /data/relevance-condition")
            ),
            body(
                repeat("/data/outer",
                    repeat("/data/outer/inner",
                        input("/data/outer/inner/q1")
                    )
                ),
                input("/data/relevance-condition")
            ))));

    FormDef formDef = scenario.getFormDef();
    FormEntryModel formEntryModel = new FormEntryModel(formDef);

    // For ref /data/outer[0]/inner[0], the parent position is 1 so the boolean expression is false. That means
    // none of the inner groups in /data/outer[0] can be relevant.
    assertThat(formEntryModel.isIndexRelevant(scenario.indexOf("/data/outer[0]/inner[0]")), is(false));

    scenario.next();
    // The repeat is relevant but nothing inside it is.
    assertThat(scenario.refAtIndex(), is(getRef("/data/outer[0]")));

    scenario.next();
    assertThat(scenario.refAtIndex(), is(getRef("/data/outer[1]")));

    scenario.next();
    assertThat(scenario.refAtIndex(), is(getRef("/data/outer[1]/inner[0]")));

    scenario.next();
    assertThat(scenario.refAtIndex(), is(getRef("/data/outer[1]/inner[0]/q1[0]")));

    scenario.answer("/data/relevance-condition", "1");

    scenario.jumpToBeginningOfForm();
    scenario.next();
    assertThat(scenario.refAtIndex(), is(getRef("/data/outer[0]")));

    scenario.next();
    assertThat(scenario.refAtIndex(), is(getRef("/data/outer[0]/inner[0]")));
}

[seadowg]

Right this makes a lot more sense to me now. The way we were thinking about position before was hurting my brain. I think we can incorporate this in this PR.