Allow fields in group to trigger action outside

[lognaturel]

Closes #670

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

Tests.

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

I briefly looked for a way to address groups separately from repeats. However, as far as I know, there’s no way to know whether a step in a reference is repeatable.

Removing the check for a predicate is not necessary to solve the problem but once I understood the code it felt like something low risk I wanted to change so we get good error messages in the case of a target ref that isn’t fully qualified (targeting a repeated field).

I spent a little bit of time trying to get full test coverage for processAction but I didn't want to spend too much time on it. I think FormInstanceParser.verifyActions fails in so many repeat-related cases that we might not be able to get to processAction with the cases that are not tested.

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?

it should fix the limitation and offer a better error when targeting a repeated field.

The biggest risk would be for an action to now target the wrong reference. I’ve spent a lot of time thinking about how this change could lead to that and haven’t come up with anything.

I did discover that the following test leads to a StackOverflow now whereas it just would have failed the parenting test before. That's what happens when a ref with a predicate is expanded using itself as context (and it's always been that way as far as I can tell). Hopefully the exception means we hear about it if someone tries to do it so I don't think it's so bad.

   @Test
    public void parallelRepeats() throws IOException {
        Scenario scenario = Scenario.init("Parallel repeats", html(
            head(
                title("Parallel repeats"),
                model(
                    mainInstance(t("data id=\"parallel-repeats\"",
                        t("repeat1",
                            t("source")),
                        t("repeat2",
                            t("destination")
                        )
                    ))
                )
            ),
            body(
                repeat("/data/repeat1",
                    input("/data/repeat1/source",
                        setvalue("xforms-value-changed", "/data/repeat2[position()=position(current()/..)]/destination", "/data/repeat1/source"))
                ),
                repeat("/data/repeat2",
                    input("/data/repeat2/destination")
                ))));

        scenario.createNewRepeat("/data/repeat1");
        scenario.createNewRepeat("/data/repeat1");
        scenario.createNewRepeat("/data/repeat1");

        scenario.createNewRepeat("/data/repeat2");
        scenario.createNewRepeat("/data/repeat2");
        scenario.createNewRepeat("/data/repeat2");

        scenario.answer("/data/repeat1[0]/source", "foo");
    }

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

See tests.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

no.

[lognaturel]

As far as I can tell, this can never happen. In fact, as I wrote elsewhere, I think FormInstanceParser.verifyActions is being too restrictive at parse time.

[lognaturel]

I have been unable to come up with a form that would result in either an expanded reference set of size 0 or a reference with a template path. It feels safer to leave these cases in for now, though.

[lognaturel]

For 0 refs, I believe FormInstanceParser.verifyActions makes that impossible.

TODO: try a predicate that does lead to a node being found at parse time but not at event time.

[lognaturel]

I can get to the references.size() == 0 and then hasTemplatePath code path here if I comment out the contents of verifyActions and run the following:

   @Test
    public void foo() throws IOException {
        Scenario scenario = Scenario.init("Setvalue into repeat", html(
            head(
                title("Setvalue into repeat"),
                model(
                    mainInstance(t("data id=\"setvalue-into-repeat\"",
                        t("dest_index", "1"),
                        t("source"),
                        t("repeat",
                            t("destination")
                        )
                    ))
                )
            ),
            body(
                input("/data/dest_index"),
                input("/data/source",
                    setvalue("xforms-value-changed", "/data/repeat[position()=/data/dest_index]/destination", "/data/source")),
                repeat("/data/repeat",
                    input("/data/repeat/destination")
                ))));

        scenario.createNewRepeat("/data/repeat");
        scenario.createNewRepeat("/data/repeat");
        scenario.createNewRepeat("/data/repeat");

        scenario.answer("/data/source", "foo");
        assertThat(scenario.answerOf("/data/repeat[0]/destination").getDisplayText(), is("foo"));

        scenario.answer("/data/dest_index", "7");
        scenario.answer("/data/source", "bar");
    }

Again, I think verifyActions is doing more than it should be, but it makes sense to me to wait for a user to report.

[lognaturel]

Same as above -- I tried to come up with a form def that would lead to a null resolved reference and couldn't do it but feel it's safer to leave the case in for now.

[lognaturel]

@seadowg and I had a good chat about this. He's going to chew on it and see if he thinks the tests I added are meaningful and whether he can think of cases we should add.

Whether or not he gets feedback to me by then, I'll merge tomorrow and do a beta.

[seadowg]

@lognaturel just a heads-up that I won't have a chance to review again today

[yanokwa]

Approving blindly

[lognaturel]

Merging so we can do the beta. @seadowg will take a look before we release!