Fix reference-geometry from repeat

[lognaturel]

I did not look at the test output carefully enough for the nodeset reference case. This PR gets to the expected output. For selects from repeats, users specify a reference to a text field in a repeat. For reference-geometry, they specify a reference to the repeat. For now, the only field that we've said must be in the referenced repeat is geometry so we use that as name and label. In the future, we'll likely also support a mode in which a name and label can be displayed and specified, we can handle that addition then.

@lindsay-stevens feel free to take this PR over if you see changes you'd like to make!

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

Overriding _build_itemset_reference feels like a clean use of the infrastructure in place. I verified a converted form in Collect and confirmed it works.

What are the regression risks?

This is very targeted so I think the only risk is to reference_geometry from repeats which doesn't currently work.

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

Before submitting this PR, please make sure you have:

• included test cases for core behavior and edge cases in tests
• run python -m unittest and verified all tests pass
• run ruff format pyxform tests and ruff check pyxform tests to lint code
• verified that any code or assets from external sources are properly credited in comments

[lindsay-stevens]

Is it intended that the user-specified choice_filter completely overrides the default choice_filter expression ./geometry != ''? Or perhaps should the user-specified choice_filter expression be appended - so for ${q2} = 1 rather than /test_name/r1[ ./q2 = 1] pyxform should emit /test_name/r1[./geometry != '' and ./q2 = 1]. Or in more general terms, since you mentioned that this could be extended to allow custom name/value in future: /test_name/r1[./{value_ref} != '' and ./q2 = 1] (this would also help make it more explicit in the code as to which of value_ref or label_ref is being referred to by "./geometry != ''".

Also I want to double check if the name/label aspect is working as expected for the secondary instance types. For the other tests there is consistently the assumption that the value_ref="name" and label_ref="label". So for secondary instances, are Collect and Webforms hard-coding a lookup to a geometry field? Or should pyxform be specifying it (or requiring it to be specified) in some way? On that note I think test_geo.py is missing a couple of tests that touch on the _build_itemset_file pathways for customising the value_ref/label_ref (like how test_choices_sheet__translated__ok does for translated internal choices).

[lognaturel]

Is it intended that the user-specified choice_filter completely overrides the default choice_filter expression ./geometry != ''

That was my intent. That filter is there because we use geometry as the item "name" and in true selects there's a crash if name is blank. But when used as reference geometry it may not matter. We wrote the spec such that malformed items should be silently skipped. So actually now that we talk this through I don't think we need a default choice filter, let me check that.

for secondary instances, are Collect and Webforms hard-coding a lookup to a geometry field?

Yes, exactly. It's the same convention we established for select one from map (https://docs.getodk.org/form-question-types/#select-one-from-map-widget)

We've talked about the possibility of eventually making it a third configurable reference like label and name but haven't had any demand for it yet.

I think test_geo.py is missing a couple of tests that touch on the _build_itemset_file pathways for customising the value_ref/label_ref

Ah yes, I can add a couple here.

[lognaturel]

I don't think we need a default choice filter, let me check that.

Collect doesn't work without it and I'm not sure why yet. I've filed getodk/collect#7276 to get more info.

[lognaturel]

When there's an absolute reference to a repeat in that same repeat, Collect automatically gives it a position predicate according to the current repeat instance. For example, /data/r1 becomes /data/r1[1] in the first repeat instance, /data/r1[2] in the second, etc. This is old behavior we've kept in Collect for legacy reasons. This is one of the reasons we moved to pyxform outputting a lot more relative references a few years ago.

The ideal would be for pyxform to generate a relative reference (../../r1 in that case). This would unambiguously reference the entire repeat.

I'm surprised that survey.insert_xpaths doesn't do this already. I tried to debug it and figure out why but I'm not familiar with it. I also think it's a change that could have broader implications.

I think given that we do want to get this out as soon as possible we should go with the approach with the extra predicate for now. I've added a comment to explain it.

Test with relative ref

def test_pyxform_self_reference_inside_repeat__ok(self):
    """Should find that a child itemset is emitted, with geometry value/label."""
    # RG005 RG007 RG010 RG016
    md = """
    | survey |
    | | type         | name     | label | parameters                 |
    | | begin_repeat | r1       | R1    |                            |
    | | {type}       | geometry | Q1    | reference-geometry=${{r1}} |
    | | end_repeat   | r1       |       |                            |
    """
    for t in GEO_TYPES:
        with self.subTest(t):
            self.assertPyxformXform(
                md=md.format(type=t),
                xml__xpath_match=[
                    xpq.model_instance_item("r1[not(@jr:template)]"),
                    xpq.model_instance_bind("r1/geometry", t),
                    xpq.body_itemset(
                        q_name="r1/geometry",
                        nodeset="../../r1",
                        value_ref="geometry",
                        label_ref="geometry",
                        extra_q_assertions="and not(@reference-geometry)",
                    ),
                ],
            )

[lognaturel]

the _build_itemset_file pathways for customising the value_ref/label_ref

We don't currently allow label and value parameters for the geo types. Clients don't yet have a way to display them anyway so let's wait for that requirement to come up enough to prioritize it.

This is ready for another look with the new context!

[lindsay-stevens]

I'm surprised that survey.insert_xpaths doesn't do this already.

The _process_choice_filter func only does extra processing on the choice filter if the question has a choice filter - otherwise it just appends what the ItemsetNode has to the nodeset. So maybe it behaves as expected if you try having GeoQuestion._build_itemset_reference attach the default choice_filter to the question (if there isn't already one), instead of putting the choice_filter value into the ItemsetNode.

Or perhaps GeoQuestion._build_itemset_reference needs to stay closer to the default implementation, and set the geometry element name overrides as subpaths of path[-1], if that makes sense.

[lognaturel]

It doesn't look like it! It would be the base path that needs to be relative, not the path of the choice filter. I played around with these ideas but couldn't figure anything out that changes things.

I like that the solution in this PR is symmetrical to the same case with select one from repeat (

pyxform/tests/test_repeat.py

Line 314 in 25cb119

def test_choice_from_previous_repeat_answers(self):

, note the name != ''). We could address them both at the same time in the future.

[lognaturel]

Or do feel free to take a stab at it if you have time but I think we should aim to have something merged within 24hrs. We'll get some good feedback once people start actually using it!

[lindsay-stevens]

It looks like the absolute reference for this topology is an intentional result of #791. In that case if the reference source is inside a repeat, and the reference target is a repeat, then the reference is absolute. It was relative for some cases but that was a regression. So I'm not sure if we're talking about a specific scenario (either the case here, or the case(s) in #791) where the reference should be relative, or if we're talking about changing that design generally.

Anyway, changing that would be outside of scope for this PR, so I added a test to document the behaviour and elaborated on the comment. I thought about moving the default choice_filter up into question_types/geo.py but seemed premature if it's maybe not necessary soon. Otherwise it could be moved there later.

Also the test case test_choice_from_previous_repeat_answers I think is a different case, that's a reference to a repeat from outside of the repeat. As opposed to referencing the repeat from inside the repeat. Both give an absolute reference but for different reasons.

[lognaturel]

All great additions, thank you! I thought about filing an issue to consider whether we could/should output relative references in the case of a repeat reference inside that same repeat for both geo questions and select from repeat but I'm not sure it's worth the effort and risk. Let's get some feedback on what we have now and see if there are any user needs driving further work.