Issue 646 fix repeat csv key generation #648
Conversation
- We need to know which step is a non-repeat group to ignore it
| .build(); | ||
| XmlElement xmlElement = buildXmlElementFrom(field); | ||
|
|
||
| assertThat(xmlElement.getParentLocalId("uuid:SOMELONGUUID"), is("uuid:SOMELONGUUID")); |
There was a problem hiding this comment.
Don't be bothered by this assertion, which is wrong. I fix this while working on this issue later.
| String getParentLocalId(String instanceId) { | ||
| return isFirstLevelGroup() ? instanceId : getParent().getCurrentLocalId(instanceId); | ||
| String getParentLocalId(Model field, String instanceId) { | ||
| return isFirstLevelGroup() ? instanceId : getParent().getCurrentLocalId(field, instanceId); |
There was a problem hiding this comment.
This is also wrong at this stage. The recursive call to getCurrentLocalId should receive field.getParent(). This is fixed later.
| String getCurrentLocalId(String instanceId) { | ||
| String prefix = isFirstLevelGroup() ? instanceId : getParent().getCurrentLocalId(instanceId); | ||
| String getCurrentLocalId(Model field, String instanceId) { | ||
| String prefix = isFirstLevelGroup() ? instanceId : getParent().getCurrentLocalId(field, instanceId); |
There was a problem hiding this comment.
This is also wrong at this stage. The recursive call to getCurrentLocalId should receive field.getParent(). This is fixed later.
| String getGroupLocalId(String instanceId) { | ||
| String prefix = isFirstLevelGroup() ? instanceId : getParent().getCurrentLocalId(instanceId); | ||
| String getGroupLocalId(Model field, String instanceId) { | ||
| String prefix = isFirstLevelGroup() ? instanceId : getParent().getCurrentLocalId(field, instanceId); |
There was a problem hiding this comment.
This is also wrong at this stage. The recursive call to getCurrentLocalId should receive field.getParent(). This is fixed later.
| child.setParent(current); | ||
| current.addChild(child); | ||
| current = child; | ||
| return this; | ||
| } | ||
|
|
||
| ModelBuilder addRepeatGroup(String name) { | ||
| TreeElement child = new TreeElement(name, INDEX_TEMPLATE); | ||
| TreeElement child = new TreeElement(name, 0); |
There was a problem hiding this comment.
I'm still not sure if 0 would be a correct MULTIPLICITY value for a repeat group in this context...
There was a problem hiding this comment.
I think INDEX_TEMPLATE is right if the underlying form has jr:template set and DEFAULT_MULTIPLICITY is right if the underlying form doesn't have it. So it depends on which you want to match. I expect both would give the same results, though. Is that not the case?
I think using a constant would be better than the raw value.
| @@ -60,11 +60,9 @@ static Csv repeat(FormDefinition formDefinition, Model groupModel, ExportConfigu | |||
| .orElse(stripIllegalChars(formDefinition.getFormName())); | |||
| String suffix = groupModel.getName(); | |||
| Model current = groupModel; | |||
| while (current.hasParent()) { | |||
| while (current.hasParent() && current.getParent().hasParent() && current.getParent().getParent().getName() != null) { | |||
There was a problem hiding this comment.
Was going to ask for a comment and maybe a method and then went to the next commit and found peace. 😄
|
Just a small comment from me at af61b52#r220689972 about repeat multiplicity. Otherwise looks good to me! |
- We don't want that to be included in the output filename - The instance root has a parent with null name
|
Thanks, @lognaturel! I've used |
Closes #646
Closes #647
What has been done to verify that this works as intended?
Why is this the best possible solution? Were any other approaches considered?
This is the only approach I could think of.
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?
This fixes bugs introduced after the refactor or the export code in v1.11, affecting the generation of the keys used to link rows in the exported CSV files of repeat groups with their corresponding rows in the main CSV file.
This PR also fixes the filenames of those repeat group export CSV files.
Does this change require updates to documentation? If so, please file an issue at https://github.com/opendatakit/docs/issues/new and include the link below.
Nope, although this PR comes with new notes to the export format doc in this repo