775: Support Entities from repeats #778
Conversation
- the .xls files in the question_types directory haven't changed since 2014 and since then new question types have just been added to QUESTION_TYPE_DICT and documented externally. - deleted associated unused code from question_type_dictionary.py and xls2json.py which was mostly a duplication of existing workbook reading code. - moved print_pyobj_to_json to utils.py since it doesn't relate to anything specific to xls2json.py and it so simplifies module imports
- loops currently are undocumented but in the interest of not breaking
forms that might be using them, test coverage was improved.
- test_for_loop.py: deleted because it's not actually testing loops,
and the aspects of repeats it does test are covered by test_repeat.py
- test_loop.py:
- converted older tests about internal data structures to use markdown
instead of XLS to make it easier to see what it's testing.
- deleted "another_loop.xls" since it's not used anywhere else
- retained "simple_loop.xls" since it's used by other ser/des tests
- added test cases for:
- general behaviour of loops
- error conditions: groups, repeats, and reference variables
- maybe more incompatibilities but these seemed like a good start
- existing case in not_closed_group_test.xls was equivalent to the new case test_group__no_end_error__with_another_closed_group - no other tests seem to look for these errors or check the conditions in these new tests. - moved older builder-related tests so there's one group test module - add docstrings to recently converted loop tests
- for unmatched 'begin [type]', state the row number of the 'begin', previously the error just said the group name - for unmatched 'end [type]', state the row and type. The previous control type is probably not useful information, and often the 'end' doesn't have a name so the control name would be 'None' which is also not useful. - L545: remove the if/else because the stack will never be empty, it would always at least have the survey; when the stack.pop() is done that's after a check if only one item remains in the stack. - L560/L775: remove unused 'question_name' / 'control_name' variables - L795: remove code that was commented out since 2013
- some of this is implicitly tested in other places but this commit adds a reasonably thorough and explicitly set of tests for naming validation rules about uniqueness of questions, groups, and repeats: - questions vs groups, repeats, survey - groups vs repeats, survey - repeats vs repeats, survey - test_fields.py: - moved choices-related tests into test_choices_sheet.py - test_builder.py: - deleted old commented code, and old XLS test covered by new tests
- main changes:
- case-insensitive duplicate is a warning rather than an error since
it is not a hard requirement and only applies to some use cases
- group names can be re-used in a different context rather than having
to be unique anywhere in the survey structure (like repeats)
- if pyxform reference variables ${} are used the target question,
group, or repeat still has to be unique anywhere
- add uniqueness validation to xls2json so the errors can use row refs
- add an error message specific to the 'meta' reserved name
- use same validation funcs in xls2json and section.py/survey.py
- update test assertions to use message templates
lindsay-stevens
force-pushed
the
pyxform-775
branch
from
3260f5d to
f8fd743
Compare
- to match source structure and organise related tests - the name suffix "survey" means that these tests are related to declaring entities at the survey level, as opposed to inside of repeats, nested repeats, etc.
- as much processing as possible in entities_parsing.py - most tests about entity repeat position are in test_create_repeat.py - tests specific to update-related fields in test_update_repeat.py - add more entities xpath helpers
lognaturel
left a comment
lognaturel
left a comment
There was a problem hiding this comment.
This is FANTASTIC! The commit breakdown and the commit comments really helped with review on this one and help make it feel low-risk. Thanks!
The only thing I see missing is the Entity spec version. If there's an Entity from a repeat, the spec version should be v2025.1.0.
| ) | ||
|
|
||
| def test_entity_repeat_in_repeat__error(self): | ||
| """Should raise an error if the entity repeat is inside a repeat.""" |
There was a problem hiding this comment.
This would be a fine restriction to start with but is it necessary? My apologies if we've discussed this, I can't find a record of it! @ktuite Central would support this, right?
There was a problem hiding this comment.
I'm not sure but it didn't seem part of the spec, and also having the lowest common ancestor repeat inside of a repeat affects how relative paths are calculated (relevant to #777)
What do you think about pulling this out into a markdown file? Could be in a
Another I can think of is when using a |
- feedback from pyxform 778, noting:
- for the name in the error message for unmatched end to be useful,
the user would have to have written the name which is optional
- may or may not be helpful to additionally show the name from the
previous control with a different type - could be due to a missing
row or a typo in the control type.
- also fix typo in test_loop.py
- feedback in pyxform 778 - remove unnecessary jargon for users "context" - remove potentially unfamiliar latin "i.e." - clarify that case-insensitive issue is a "should" not a "must"
It may be more visible on xlsform.org so I opened XLSForm/xlsform.github.io#281. Or it could go in the template XLSForm, or ODK docs as well? |
lognaturel
left a comment
lognaturel
left a comment
There was a problem hiding this comment.
Updates and answers to questions so far look great, thanks!
|
As I was trying this out, I forgot to specify the repeat reference on the |
|
It looks like all of the references produced in binds are off by one parenting level: lindsay-stevens/pyxform@pyxform-775...lognaturel:pyxform:hm_comments I'm pretty sure that's because in It doesn't feel directly related to #777 because I've made a list of must-haves for this PR in the original description so we don't forget anything. Thanks, @lindsay-stevens, lots of tricky stuff here that will hopefully benefit users a ton! |
|
In one of the forms I was experimenting with, I saw this line but it only had |
There were cases where it was tripped up by groups and/or repeats on the path between the reference source and lowest common ancestor repeat (LCAR), and groups and/or repeats on the path between the reference target and the LCAR. So that could affect where the |
- in addition to diff resolution, entities_parsing.py was updated to use pyxform_reference.py instead of the regex directly. This required adding a `match_full` param parse_pyxform_references because the `is_pyxform_reference` func returns bool but for entities the match result is needed, and there are additional constraints to apply. Updated test cases in test_create_repeat.py to show when the different error types would be triggered.
- similar to question.py `get_setvalue_node_for_dynamic_default` except that the default isn't a factor and there are different output attrs. - also consolidated entity xpath assertions for model/setvalue
- combine assertions about the entities meta block
- model_instance_meta maybe a little unwieldy but probably preferable
to having many similar but slightly different xpaths
- apply entities xpath helper to existing xpath test assertions
|
As of df6eee6 (but not necessarily due to that commit) the validate CI job (merged in c736045) is failing for 2 tests, due to a javarosa error in SetValueAction.java It looks like the error message doesn't show the actual references because in the javarosa source code, the quotes surrounding the "references" symbol are escaped. The tests that fail don't seem like they should fail, especially considering that tests.entities.test_create_repeat.TestEntitiesCreateRepeat.test_other_controls_after__ok tests.entities.test_create_repeat.TestEntitiesCreateRepeat.test_other_controls_before_and_after__ok If I remove the If I remove the |
Whoops 😬
It does look like Collect manages to do the right thing when there's only one repeat but things break when there are multiple and I bet are even worse when there's nesting involved. The ideal fix would be to put the |
|
There's a bug in JavaRosa that allowed the non-compliant form structure to go through Validate: getodk/javarosa#829 These were really good tests to add, thanks. A concrete example -- validate-issue-entity-repeat.xlsx:
|
- action elements (setvalue, setgeopoint, recordaudio) defined and
generated in a few different ways, this commit goes some way towards:
- define the supported action, event types, and combinations thereof
- allow more than one action per element e.g. first-load, new-repeat
- split first-load from new-repeat to avoid bugs from output location
- declare them via "actions" property in xls2json (except triggers)
- generate from "actions" via xml_actions (except triggers)
- triggers are a special case that seemed to require some trickier
refactoring to consolidate with xml_action at this stage
- calculate and store if the default is dynamic during xls2json step
to avoid re-parsing the default (falls back to re-parsing if the new
key isn't found)
|
@lognaturel the setvalue issue is addressed in 1e481b9 - overview in commit message but below are some detailed notes on old vs. new. I found actions generation quite confusing but I think it's clearer now and hopefully you agree. One downside of making actions 1:1 with event types is that trigger expressions can appear verbatim in 2 places for when the trigger is in a repeat, but I would expect that usually these expressions are not tremendously long such that they would dramatically increase XForm file size. I couldn't see any other reason besides brevity to combine the setvalue for that case but please let me know otherwise - it should be possible to allow a sequence of events to combine the actions again. Also I assumed it was a typo that the entity Old output locations, call paths, action/event types, reasons for generation
New output locations, call paths, action/event types, reasons for generation
|
- the bind for the entity label was one path too low to find the correct target, because the `insert_xpaths` context was the entity rather than the label. It seems like the path is OK for other entity attributes because their context is the entity element. In order to use existing insert_xpaths functionality that navigates the form structure using .parent/.children, the entity label must be a SurveyElement so a new class is created for it.
@lognaturel in 026bfe0 I've fixed this for label - see commit message for details. The paths for the other attribute bindings seemed like they should be OK, assuming that For example from And there's a |
|
So exciting to see you've got fixes galore! Looks good after a first pass, will keep digging. One quick thing jumps out:
As I understand it, |
|
If |
| https://getodk.github.io/xforms-spec/#action:setgeopoint | ||
| """ | ||
|
|
||
| name = "odk:setgeopoint" |
There was a problem hiding this comment.
It feels slightly uncomfortable to me to hard code the namespace prefix here since it has to match the namespace declaration. An alternative would be to define a constant for the prefix somewhere and use it consistently here and in the declaration. It's not super important because it's an obviously good convention that doesn't require much discipline to maintain but wanted to mention it. I probably feel discomfort because I'm used to seeing code on the form client side where it's important to support any prefix.
There was a problem hiding this comment.
good point for improvement though it was hard-coded previously (question_type_dictionary.py etc)
|
It's feeling really close! I think the only must-have is getting clarity on
You are not alone. They've been conceptually tricky to fit in everywhere in the ODK XForms world and because we've introduced different actions and events over time the implementations have all gotten messy. Did I understand correctly that dynamic defaults in repeats were correctly generating
I can't either so it looks fine to me. As far as I know, any client implementation would need to separate out the actions by triggering event anyway so it makes no difference on the client side.
Oh yes, good catch! I feel like my brain tripped up on that a few times but not enough to notice it didn't make sense. |
|
Here's a way to confirm that the "current" node for an attribute is the attribute itself, not its parent:
|
- the bind for the entity attributes was one path too low to find the
correct target, because the `insert_xpaths` context was the entity
rather than the attribute. In order to use existing insert_xpaths
functionality that navigates the form structure using
.parent/.children, the entity attributes must be SurveyElements so a
new class is created for it.
- overall it's moving from procedural generation in entity_declaration
to a declarative generation in entities_parsing.py, and attributes
which are the target of bind/actions/etc are responsible for the
generation of those nodes.
- also simplify xls2json.py entities sheet processing since
EntityColumns contains all the valid column names, and the slot names
add a lot of column names which should not be user-specified.
lindsay-stevens
force-pushed
the
pyxform-775
branch
from
d7c6899 to
0014ca9
Compare
|
Yes it seems now like a pretty foundational concept; I didn't consider attributes as a "node" as such. But the spec says everything is a node, and 0014ca9 is a moving towards everything is a SurveyElement. I still think the https://www.w3.org/TR/xforms11/%23context-xpath-evaluation#fn-current
https://www.w3.org/TR/1999/REC-xpath-19991116/#section-Introduction
https://www.w3.org/TR/1999/REC-xpath-19991116/#data-model
|
- in this case a top-level entity instance would be generated and so it wouldn't be populated properly by the item in the repeat. Either a mistake in where the save_to was placed, or the entity repeat column was left blank by mistake.
- added checks to entities tests that extra setvalue nodes not emitted - moved truth-table docstring over from entity_declaration.py - reformatted docstring cols to sort all ascending - renamed docstring cols / func symbols to match column names - organised if expressions to match docstring cols order
lognaturel
left a comment
lognaturel
left a comment
There was a problem hiding this comment.
Thanks so much for bringing this to the finish line along with some nice misc improvements! 🎉
3 participants
Closes #775
Closes #619 (
unique_names.pyerrorNAMES005)Addresses question name issue in #504 (questions only have to be unique within their parent context)
TODO:
Relative expressions in Entity binds are off by one level (775: Support Entities from repeats #778 (comment))labels done in 026bfe0 attributes done in 0014ca9setvalueaction in repeats should be nested in the control (775: Support Entities from repeats #778 (comment))If the Entity declaration is from repeat, the Entities spec version should be v2025.1.0 (775: Support Entities from repeats #778 (review))done in 0af166aSet entity id for new repeat instances for create forms (775: Support Entities from repeats #778 (comment))done in 2caa1e2File follow-up issue:
Consider error when saveto is not in Entity scope (775: Support Entities from repeats #778 (comment))done in 983b990Why is this the best possible solution? Were any other approaches considered?
The spec involves putting a "meta" block inside each repeat as well as at the top-level survey/main-instance node. This conflicts with existing name validation. However it seems that the name rules in pyxform are inconsistent and too restrictive. To enable the new entities feature, the modifications are made to name validation such that the new rules would be as outlined below.
The main modification to the existing rules is to allow a group to be unique only within it's context, rather than anywhere in the survey (as will be the case for the
metablock). The main constraint seems to be that Central will produce export files for the top-level survey, and any repeats; any group names in such files are kebab'd together within the context of the survey OR lowest common ancestor repeat e.g. 'g1-g2-question'.As for the entity repeats changes, this (as far as I understand it) adheres to the specs, Q&A docs, forum posts available. The tests in
test_create_repeat.pyare somewhat prescriptive so these should be reviewed to ensure they aren't preventing anything that should be allowed, or allowing something that should be prevented.Name rules
Not all of these are enforced well, or at all
${}reference.What are the regression risks?
I did some git archaeology but I might've missed a critical reason why groups have historically been required to be unique at any depth - maybe Aggregate used to split by group not just repeat? Otherwise hopefully it's relaxing existing rules and improving error messages.
Does this change require updates to documentation? If so, please file an issue here and include the link below.
getodk/xforms-spec#330
getodk/docs#1987
XLSForm/xlsform.github.io#281
Before submitting this PR, please make sure you have:
testspython -m unittestand verified all tests passruff format pyxform testsandruff check pyxform teststo lint code