From 64ddcad0d76e246aa6c69ad0f376f3519b06b8da Mon Sep 17 00:00:00 2001 From: Guille Date: Tue, 25 Sep 2018 10:15:14 +0200 Subject: [PATCH 1/8] Add failing test to start fixing the PARENT_KEY generation code --- .../opendatakit/briefcase/export/Model.java | 2 +- .../briefcase/export/XmlElementTest.java | 102 ++++++++++++++++++ 2 files changed, 103 insertions(+), 1 deletion(-) create mode 100644 test/java/org/opendatakit/briefcase/export/XmlElementTest.java diff --git a/src/org/opendatakit/briefcase/export/Model.java b/src/org/opendatakit/briefcase/export/Model.java index af7f8966f..3f5171f9a 100644 --- a/src/org/opendatakit/briefcase/export/Model.java +++ b/src/org/opendatakit/briefcase/export/Model.java @@ -231,7 +231,7 @@ boolean isRoot() { return countAncestors() == 0; } - private boolean hasParent() { + boolean hasParent() { return model.getParent() != null; } diff --git a/test/java/org/opendatakit/briefcase/export/XmlElementTest.java b/test/java/org/opendatakit/briefcase/export/XmlElementTest.java new file mode 100644 index 000000000..0a4ab27b1 --- /dev/null +++ b/test/java/org/opendatakit/briefcase/export/XmlElementTest.java @@ -0,0 +1,102 @@ +/* + * Copyright (C) 2018 Nafundi + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. You may obtain a copy of + * the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ + +package org.opendatakit.briefcase.export; + +import static java.util.Collections.emptyMap; +import static org.hamcrest.Matchers.is; +import static org.javarosa.core.model.instance.TreeReference.DEFAULT_MULTIPLICITY; +import static org.javarosa.core.model.instance.TreeReference.INDEX_TEMPLATE; +import static org.junit.Assert.assertThat; + +import java.io.IOException; +import java.io.StringReader; +import org.javarosa.core.model.DataType; +import org.javarosa.core.model.instance.TreeElement; +import org.junit.Test; +import org.kxml2.io.KXmlParser; +import org.kxml2.kdom.Document; +import org.xmlpull.v1.XmlPullParser; +import org.xmlpull.v1.XmlPullParserException; + +public class XmlElementTest { + @Test + public void knows_how_to_generate_keys_of_a_repeat_nested_in_groups() throws IOException, XmlPullParserException { + Model field = new ModelBuilder() + .addGroup("g1") + .addGroup("g2") + .addGroup("g3") + .addRepeatGroup("r") + .build(); + XmlElement xmlElement = buildXmlElementFrom(field); + + assertThat(xmlElement.getParentLocalId("uuid:SOMELONGUUID"), is("uuid:SOMELONGUUID")); + } + + private static Document parse(String xml) throws XmlPullParserException, IOException { + Document tempDoc = new Document(); + KXmlParser parser = new KXmlParser(); + parser.setInput(new StringReader(xml)); + parser.setFeature(XmlPullParser.FEATURE_PROCESS_NAMESPACES, true); + tempDoc.parse(parser); + return tempDoc; + } + + class ModelBuilder { + private TreeElement current = new TreeElement(null, DEFAULT_MULTIPLICITY); + + ModelBuilder addGroup(String name) { + TreeElement child = new TreeElement(name, DEFAULT_MULTIPLICITY); + child.setDataType(DataType.NULL.value); + child.setParent(current); + current.addChild(child); + current = child; + return this; + } + + ModelBuilder addRepeatGroup(String name) { + TreeElement child = new TreeElement(name, INDEX_TEMPLATE); + child.setDataType(DataType.NULL.value); + child.setParent(current); + current.addChild(child); + current = child; + return this; + } + + ModelBuilder addField(String name, DataType dataType) { + TreeElement child = new TreeElement(name, DEFAULT_MULTIPLICITY); + child.setDataType(dataType.value); + child.setParent(current); + current.addChild(child); + current = child; + return this; + } + + Model build() { + return new Model(current, emptyMap()); + } + } + + private static XmlElement buildXmlElementFrom(Model field) throws IOException, XmlPullParserException { + Model current = field; + String xml = "<" + current.getName() + "/>"; + while (current.hasParent()) { + current = current.getParent(); + xml = "<" + current.getName() + ">" + xml + ""; + } + return XmlElement.of(parse(xml)).findElement(field.getName()).get(); + } +} \ No newline at end of file From a5b5b59fb7669fbea1acd436c9591a27068c67e4 Mon Sep 17 00:00:00 2001 From: Guille Date: Tue, 25 Sep 2018 10:18:33 +0200 Subject: [PATCH 2/8] Add the Model field as a required arg to produce the KEYS - We need to know which step is a non-repeat group to ignore it --- .../export/CsvSubmissionMappers.java | 10 ++++----- .../briefcase/export/XmlElement.java | 21 ++++++------------- .../briefcase/export/XmlElementTest.java | 2 +- 3 files changed, 11 insertions(+), 22 deletions(-) diff --git a/src/org/opendatakit/briefcase/export/CsvSubmissionMappers.java b/src/org/opendatakit/briefcase/export/CsvSubmissionMappers.java index 9f61d7e32..18bc93121 100644 --- a/src/org/opendatakit/briefcase/export/CsvSubmissionMappers.java +++ b/src/org/opendatakit/briefcase/export/CsvSubmissionMappers.java @@ -20,7 +20,6 @@ import static java.util.stream.Collectors.joining; import static java.util.stream.Collectors.toList; import static java.util.stream.Collectors.toSet; - import static org.javarosa.core.model.DataType.DATE; import static org.javarosa.core.model.DataType.DATE_TIME; import static org.javarosa.core.model.DataType.GEOPOINT; @@ -34,7 +33,6 @@ import java.util.Set; import java.util.stream.Collectors; import java.util.stream.Stream; - import org.javarosa.core.model.DataType; import org.javarosa.core.model.SelectChoice; import org.opendatakit.briefcase.reused.Pair; @@ -85,15 +83,15 @@ static CsvSubmissionMapper repeat(Model groupModel, ExportConfiguration configur submission.getElements(groupModel.fqn()).stream().map(element -> { List cols = new ArrayList<>(); cols.addAll(groupModel.flatMap(field -> getMapper(field, configuration.resolveExplodeChoiceLists()).apply( - element.getCurrentLocalId(submission.getInstanceId(true)), + element.getCurrentLocalId(field, submission.getInstanceId(true)), submission.getWorkingDir(), field, element.findElement(field.getName()), configuration ).map(CsvSubmissionMappers::encodeRepeatValue)).collect(toList())); - cols.add(encode(element.getParentLocalId(submission.getInstanceId(true)), false)); - cols.add(encode(element.getCurrentLocalId(submission.getInstanceId(true)), false)); - cols.add(encode(element.getGroupLocalId(submission.getInstanceId(true)), false)); + cols.add(encode(element.getParentLocalId(groupModel, submission.getInstanceId(true)), false)); + cols.add(encode(element.getCurrentLocalId(groupModel, submission.getInstanceId(true)), false)); + cols.add(encode(element.getGroupLocalId(groupModel, submission.getInstanceId(true)), false)); return cols.stream().collect(joining(",")); }).collect(toList()) ); diff --git a/src/org/opendatakit/briefcase/export/XmlElement.java b/src/org/opendatakit/briefcase/export/XmlElement.java index 3e533a49e..d707c0015 100644 --- a/src/org/opendatakit/briefcase/export/XmlElement.java +++ b/src/org/opendatakit/briefcase/export/XmlElement.java @@ -58,35 +58,26 @@ static XmlElement of(Document document) { /** * Builds and returns this {@link XmlElement} instance's parent's local ID. * This ID is used to cross-reference values in different exported files. - * - * @param instanceId the Form submission's instance ID - * @return a {@link String} with this {@link XmlElement} instance's parent's local ID. */ - String getParentLocalId(String instanceId) { - return isFirstLevelGroup() ? instanceId : getParent().getCurrentLocalId(instanceId); + String getParentLocalId(Model field, String instanceId) { + return isFirstLevelGroup() ? instanceId : getParent().getCurrentLocalId(field, instanceId); } /** * Builds and returns this {@link XmlElement} instance's current local ID. * This ID is used to cross-reference values in different exported files. - * - * @param instanceId the Form submission's instance ID - * @return a {@link String} with this {@link XmlElement} instance's current local ID. */ - 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); return prefix + "/" + getName() + "[" + getPlaceAmongSameTagSiblings() + "]"; } /** * Builds and returns this {@link XmlElement} instance's group local ID. * This ID is used to cross-reference values in different exported files. - * - * @param instanceId the Form submission's instance ID - * @return a {@link String} with this {@link XmlElement} instance's group local ID. */ - 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); return prefix + "/" + getName(); } diff --git a/test/java/org/opendatakit/briefcase/export/XmlElementTest.java b/test/java/org/opendatakit/briefcase/export/XmlElementTest.java index 0a4ab27b1..c720573c3 100644 --- a/test/java/org/opendatakit/briefcase/export/XmlElementTest.java +++ b/test/java/org/opendatakit/briefcase/export/XmlElementTest.java @@ -43,7 +43,7 @@ public void knows_how_to_generate_keys_of_a_repeat_nested_in_groups() throws IOE .build(); XmlElement xmlElement = buildXmlElementFrom(field); - assertThat(xmlElement.getParentLocalId("uuid:SOMELONGUUID"), is("uuid:SOMELONGUUID")); + assertThat(xmlElement.getParentLocalId(field, "uuid:SOMELONGUUID"), is("uuid:SOMELONGUUID")); } private static Document parse(String xml) throws XmlPullParserException, IOException { From 0a6fd11ff705de74491f598fad160482138c9307 Mon Sep 17 00:00:00 2001 From: Guille Date: Tue, 25 Sep 2018 10:34:36 +0200 Subject: [PATCH 3/8] Fix tests and implementation of current, parent, and group keys --- .../briefcase/export/XmlElement.java | 10 ++++---- .../briefcase/export/XmlElementTest.java | 23 +++++++++++++++++-- 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/src/org/opendatakit/briefcase/export/XmlElement.java b/src/org/opendatakit/briefcase/export/XmlElement.java index d707c0015..a678d907f 100644 --- a/src/org/opendatakit/briefcase/export/XmlElement.java +++ b/src/org/opendatakit/briefcase/export/XmlElement.java @@ -60,7 +60,7 @@ static XmlElement of(Document document) { * This ID is used to cross-reference values in different exported files. */ String getParentLocalId(Model field, String instanceId) { - return isFirstLevelGroup() ? instanceId : getParent().getCurrentLocalId(field, instanceId); + return isFirstLevelGroup() ? instanceId : getParent().getCurrentLocalId(field.getParent(), instanceId); } /** @@ -68,8 +68,10 @@ String getParentLocalId(Model field, String instanceId) { * This ID is used to cross-reference values in different exported files. */ String getCurrentLocalId(Model field, String instanceId) { - String prefix = isFirstLevelGroup() ? instanceId : getParent().getCurrentLocalId(field, instanceId); - return prefix + "/" + getName() + "[" + getPlaceAmongSameTagSiblings() + "]"; + String prefix = isFirstLevelGroup() ? instanceId : getParent().getCurrentLocalId(field.getParent(), instanceId); + return field.isRepeatable() + ? prefix + "/" + getName() + "[" + getPlaceAmongSameTagSiblings() + "]" + : prefix; } /** @@ -77,7 +79,7 @@ String getCurrentLocalId(Model field, String instanceId) { * This ID is used to cross-reference values in different exported files. */ String getGroupLocalId(Model field, String instanceId) { - String prefix = isFirstLevelGroup() ? instanceId : getParent().getCurrentLocalId(field, instanceId); + String prefix = isFirstLevelGroup() ? instanceId : getParent().getCurrentLocalId(field.getParent(), instanceId); return prefix + "/" + getName(); } diff --git a/test/java/org/opendatakit/briefcase/export/XmlElementTest.java b/test/java/org/opendatakit/briefcase/export/XmlElementTest.java index c720573c3..d54bc3b3c 100644 --- a/test/java/org/opendatakit/briefcase/export/XmlElementTest.java +++ b/test/java/org/opendatakit/briefcase/export/XmlElementTest.java @@ -19,7 +19,6 @@ import static java.util.Collections.emptyMap; import static org.hamcrest.Matchers.is; import static org.javarosa.core.model.instance.TreeReference.DEFAULT_MULTIPLICITY; -import static org.javarosa.core.model.instance.TreeReference.INDEX_TEMPLATE; import static org.junit.Assert.assertThat; import java.io.IOException; @@ -43,7 +42,25 @@ public void knows_how_to_generate_keys_of_a_repeat_nested_in_groups() throws IOE .build(); XmlElement xmlElement = buildXmlElementFrom(field); + assertThat(xmlElement.getCurrentLocalId(field, "uuid:SOMELONGUUID"), is("uuid:SOMELONGUUID/r[1]")); assertThat(xmlElement.getParentLocalId(field, "uuid:SOMELONGUUID"), is("uuid:SOMELONGUUID")); + assertThat(xmlElement.getGroupLocalId(field, "uuid:SOMELONGUUID"), is("uuid:SOMELONGUUID/r")); + } + + @Test + public void knows_how_to_generate_keys_of_nested_repeats() throws IOException, XmlPullParserException { + Model field = new ModelBuilder() + .addGroup("g1") + .addGroup("g2") + .addGroup("g3") + .addRepeatGroup("r1") + .addRepeatGroup("r2") + .build(); + XmlElement xmlElement = buildXmlElementFrom(field); + + assertThat(xmlElement.getCurrentLocalId(field, "uuid:SOMELONGUUID"), is("uuid:SOMELONGUUID/r1[1]/r2[1]")); + assertThat(xmlElement.getParentLocalId(field, "uuid:SOMELONGUUID"), is("uuid:SOMELONGUUID/r1[1]")); + assertThat(xmlElement.getGroupLocalId(field, "uuid:SOMELONGUUID"), is("uuid:SOMELONGUUID/r1[1]/r2")); } private static Document parse(String xml) throws XmlPullParserException, IOException { @@ -61,6 +78,7 @@ class ModelBuilder { ModelBuilder addGroup(String name) { TreeElement child = new TreeElement(name, DEFAULT_MULTIPLICITY); child.setDataType(DataType.NULL.value); + child.setRepeatable(false); child.setParent(current); current.addChild(child); current = child; @@ -68,8 +86,9 @@ ModelBuilder addGroup(String name) { } ModelBuilder addRepeatGroup(String name) { - TreeElement child = new TreeElement(name, INDEX_TEMPLATE); + TreeElement child = new TreeElement(name, DEFAULT_MULTIPLICITY); child.setDataType(DataType.NULL.value); + child.setRepeatable(true); child.setParent(current); current.addChild(child); current = child; From 3f522fbb9d1335c6b1cc4d406c90dfa69411d91a Mon Sep 17 00:00:00 2001 From: Guille Date: Tue, 25 Sep 2018 10:37:56 +0200 Subject: [PATCH 4/8] Add note in the export format doc about how we ignore non-repeat groups --- docs/export-format.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/export-format.md b/docs/export-format.md index df94fcc5c..c951e46ce 100644 --- a/docs/export-format.md +++ b/docs/export-format.md @@ -48,6 +48,8 @@ The file names of repeat groups depend on their position inside the model tree o | Top group | `{INSTANCE ID}` | `uuid:00000000-0000-0000-0000-000000000000` | | Descendant group | `{PARENT KEY}` | `uuid:00000000-0000-0000-0000-000000000000/group1[1]` | +_Note: non-repeat groups in the chain of ancestors are always ignored_ + ### `KEY` | | Pattern | Example | @@ -56,6 +58,7 @@ The file names of repeat groups depend on their position inside the model tree o | Descendant group | `{PARENT KEY}/{GROUP NAME}[{ORDERING}]` | `uuid:00000000-0000-0000-0000-000000000000/group1[1]/group2[1]` | _Note: the `[{ORDERING}]` part is 1-indexed_ +_Note: non-repeat groups in the chain of ancestors are always ignored_ ### `SET-OF-{GROUP NAME}` @@ -64,6 +67,8 @@ _Note: the `[{ORDERING}]` part is 1-indexed_ | Top group | `{INSTANCE ID}/{GROUP NAME}` | `uuid:00000000-0000-0000-0000-000000000000/group1` | | Descendant group | `{PARENT KEY}/{GROUP NAME}` | `uuid:00000000-0000-0000-0000-000000000000/group1[1]/group2` | +_Note: non-repeat groups in the chain of ancestors are always ignored_ + The SET-OF columns may be named differently in the two output files. For example, if form X contains a non-repeat group Y, which contains a repeat group Z, then: - The main output file will have a column named SET-OF-Y-Z (the long name of the repeat group). From 7a41bb01c489bc8a208c02b56dafe9d84aebc43c Mon Sep 17 00:00:00 2001 From: Guille Date: Tue, 25 Sep 2018 10:50:13 +0200 Subject: [PATCH 5/8] Add test that verifies that we use fqn for repeat output files --- src/org/opendatakit/briefcase/export/Csv.java | 12 +++- .../briefcase/export/FormDefinition.java | 3 +- .../opendatakit/briefcase/export/CsvTest.java | 66 +++++++++++++++++++ .../briefcase/export/XmlElementTest.java | 2 +- 4 files changed, 77 insertions(+), 6 deletions(-) create mode 100644 test/java/org/opendatakit/briefcase/export/CsvTest.java diff --git a/src/org/opendatakit/briefcase/export/Csv.java b/src/org/opendatakit/briefcase/export/Csv.java index 6787b6a36..0f10aa745 100644 --- a/src/org/opendatakit/briefcase/export/Csv.java +++ b/src/org/opendatakit/briefcase/export/Csv.java @@ -3,7 +3,6 @@ import static java.nio.file.StandardOpenOption.APPEND; import static java.nio.file.StandardOpenOption.CREATE; import static java.nio.file.StandardOpenOption.TRUNCATE_EXISTING; - import static org.opendatakit.briefcase.export.CsvSubmissionMappers.getMainHeader; import static org.opendatakit.briefcase.export.CsvSubmissionMappers.getRepeatHeader; import static org.opendatakit.briefcase.reused.UncheckedFiles.write; @@ -12,7 +11,6 @@ import java.nio.file.Files; import java.nio.file.Path; import java.util.stream.Stream; - import org.opendatakit.briefcase.reused.UncheckedFiles; /** @@ -60,8 +58,16 @@ static Csv repeat(FormDefinition formDefinition, Model groupModel, ExportConfigu String repeatFileNameBase = configuration.getExportFileName() .map(UncheckedFiles::stripFileExtension) .orElse(stripIllegalChars(formDefinition.getFormName())); + String suffix = groupModel.getName(); + Model current = groupModel; + while (current.hasParent()) { + current = current.getParent(); + suffix = current.getName() != null + ? current.getName() + "-" + suffix + : suffix; + } Path output = configuration.getExportDir().resolve( - repeatFileNameBase + "-" + groupModel.getName() + ".csv" + repeatFileNameBase + "-" + suffix + ".csv" ); return new Csv( groupModel.fqn(), diff --git a/src/org/opendatakit/briefcase/export/FormDefinition.java b/src/org/opendatakit/briefcase/export/FormDefinition.java index a1f037d5c..8221d7791 100644 --- a/src/org/opendatakit/briefcase/export/FormDefinition.java +++ b/src/org/opendatakit/briefcase/export/FormDefinition.java @@ -32,7 +32,6 @@ import java.util.Map; import java.util.Optional; import java.util.stream.Stream; - import org.javarosa.core.model.FormDef; import org.javarosa.core.model.IDataReference; import org.javarosa.core.model.IFormElement; @@ -56,7 +55,7 @@ public class FormDefinition { private final Model model; private final List repeatFields; - private FormDefinition(String id, Path formFile, String name, boolean isEncrypted, Model model) { + FormDefinition(String id, Path formFile, String name, boolean isEncrypted, Model model) { this.id = id; this.name = name; this.formFile = formFile; diff --git a/test/java/org/opendatakit/briefcase/export/CsvTest.java b/test/java/org/opendatakit/briefcase/export/CsvTest.java new file mode 100644 index 000000000..463d90cb2 --- /dev/null +++ b/test/java/org/opendatakit/briefcase/export/CsvTest.java @@ -0,0 +1,66 @@ +/* + * Copyright (C) 2018 Nafundi + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. You may obtain a copy of + * the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ + +package org.opendatakit.briefcase.export; + +import static org.junit.Assert.assertThat; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Optional; +import org.junit.Test; +import org.opendatakit.briefcase.matchers.PathMatchers; +import org.opendatakit.briefcase.reused.OverridableBoolean; + +public class CsvTest { + @Test + public void includes_non_repeat_groups_in_repeat_filenames() throws IOException { + Model group = new XmlElementTest.ModelBuilder() + .addGroup("g1") + .addGroup("g2") + .addGroup("g3") + .addRepeatGroup("r") + .build(); + + FormDefinition formDef = new FormDefinition( + "some_form", + Files.createTempFile("briefcase_some_form", ".xml"), + "some_form", + false, + group.getParent().getParent().getParent() + ); + + Path exportDir = Files.createTempDirectory("briefcase_export_dir"); + + ExportConfiguration conf = new ExportConfiguration( + Optional.of("some_form.csv"), + Optional.of(exportDir), + Optional.empty(), + Optional.empty(), + Optional.empty(), + OverridableBoolean.FALSE, + OverridableBoolean.TRUE, + OverridableBoolean.TRUE, + Optional.of(false) + ); + + Csv repeat = Csv.repeat(formDef, group, conf); + repeat.prepareOutputFiles(); + + assertThat(exportDir.resolve("some_form-g1-g2-g3-r.csv"), PathMatchers.exists()); + } +} \ No newline at end of file diff --git a/test/java/org/opendatakit/briefcase/export/XmlElementTest.java b/test/java/org/opendatakit/briefcase/export/XmlElementTest.java index d54bc3b3c..c289f1851 100644 --- a/test/java/org/opendatakit/briefcase/export/XmlElementTest.java +++ b/test/java/org/opendatakit/briefcase/export/XmlElementTest.java @@ -72,7 +72,7 @@ private static Document parse(String xml) throws XmlPullParserException, IOExcep return tempDoc; } - class ModelBuilder { + static class ModelBuilder { private TreeElement current = new TreeElement(null, DEFAULT_MULTIPLICITY); ModelBuilder addGroup(String name) { From 5a52af0ba2b5ae61a77d986fee7627d519a77c17 Mon Sep 17 00:00:00 2001 From: Guille Date: Tue, 25 Sep 2018 11:01:45 +0200 Subject: [PATCH 6/8] Account for the first node, which is the instance root - We don't want that to be included in the output filename - The instance root has a parent with null name --- src/org/opendatakit/briefcase/export/Csv.java | 6 ++---- test/java/org/opendatakit/briefcase/export/CsvTest.java | 1 + 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/org/opendatakit/briefcase/export/Csv.java b/src/org/opendatakit/briefcase/export/Csv.java index 0f10aa745..80ee17411 100644 --- a/src/org/opendatakit/briefcase/export/Csv.java +++ b/src/org/opendatakit/briefcase/export/Csv.java @@ -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) { current = current.getParent(); - suffix = current.getName() != null - ? current.getName() + "-" + suffix - : suffix; + suffix = current.getName() + "-" + suffix; } Path output = configuration.getExportDir().resolve( repeatFileNameBase + "-" + suffix + ".csv" diff --git a/test/java/org/opendatakit/briefcase/export/CsvTest.java b/test/java/org/opendatakit/briefcase/export/CsvTest.java index 463d90cb2..fb2bad4a1 100644 --- a/test/java/org/opendatakit/briefcase/export/CsvTest.java +++ b/test/java/org/opendatakit/briefcase/export/CsvTest.java @@ -30,6 +30,7 @@ public class CsvTest { @Test public void includes_non_repeat_groups_in_repeat_filenames() throws IOException { Model group = new XmlElementTest.ModelBuilder() + .addGroup("data") .addGroup("g1") .addGroup("g2") .addGroup("g3") From 17d8617edb1a7946cb5852fce5ce3b6e63c8e771 Mon Sep 17 00:00:00 2001 From: Guille Date: Tue, 25 Sep 2018 11:08:35 +0200 Subject: [PATCH 7/8] Extract a method to provide semantics and a doc block explaining it --- src/org/opendatakit/briefcase/export/Csv.java | 25 ++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/src/org/opendatakit/briefcase/export/Csv.java b/src/org/opendatakit/briefcase/export/Csv.java index 80ee17411..9ae60ecaf 100644 --- a/src/org/opendatakit/briefcase/export/Csv.java +++ b/src/org/opendatakit/briefcase/export/Csv.java @@ -60,7 +60,7 @@ static Csv repeat(FormDefinition formDefinition, Model groupModel, ExportConfigu .orElse(stripIllegalChars(formDefinition.getFormName())); String suffix = groupModel.getName(); Model current = groupModel; - while (current.hasParent() && current.getParent().hasParent() && current.getParent().getParent().getName() != null) { + while (grandParentIsRoot(current)) { current = current.getParent(); suffix = current.getName() + "-" + suffix; } @@ -77,6 +77,29 @@ static Csv repeat(FormDefinition formDefinition, Model groupModel, ExportConfigu ); } + /** + * Returns true if the grandparent node of the given Model is the model's root + *

+ * Example 1: + *

+ * 

+   * <data>
+   *   </some_field>
+   * </data>
+   *
+ *

+ * In this example: + *

    + *
  • <data> has a parent with null name
    • + *
  • Returns true on <some_field>
    • + *
+ */ + private static boolean grandParentIsRoot(Model current) { + return current.hasParent() + && current.getParent().hasParent() // Check if current has a grandparent + && current.getParent().getParent().getName() != null; // The root node has a null name + } + /** * This method ensures that the output file is ready to receive new * contents by appending lines. From aa8d8ff99efd95cf8c2f6f5b18e7fd862c88ad4b Mon Sep 17 00:00:00 2001 From: Guille Date: Tue, 25 Sep 2018 11:12:45 +0200 Subject: [PATCH 8/8] Fix existing tests to work out repeat output files correctly --- .../opendatakit/briefcase/export/ExportToCsvScenario.java | 6 ++++-- ...sv.expected => nested-repeats-g1-g2-append.csv.expected} | 0 ...expected => nested-repeats-g1-g2-g3-append.csv.expected} | 0 ...ected => nested-repeats-g1-g2-g3-overwrite.csv.expected} | 0 ...g3.csv.expected => nested-repeats-g1-g2-g3.csv.expected} | 0 ...expected => nested-repeats-g1-g2-overwrite.csv.expected} | 0 ...ts-g2.csv.expected => nested-repeats-g1-g2.csv.expected} | 0 7 files changed, 4 insertions(+), 2 deletions(-) rename test/resources/org/opendatakit/briefcase/export/{nested-repeats-g2-append.csv.expected => nested-repeats-g1-g2-append.csv.expected} (100%) rename test/resources/org/opendatakit/briefcase/export/{nested-repeats-g3-append.csv.expected => nested-repeats-g1-g2-g3-append.csv.expected} (100%) rename test/resources/org/opendatakit/briefcase/export/{nested-repeats-g3-overwrite.csv.expected => nested-repeats-g1-g2-g3-overwrite.csv.expected} (100%) rename test/resources/org/opendatakit/briefcase/export/{nested-repeats-g3.csv.expected => nested-repeats-g1-g2-g3.csv.expected} (100%) rename test/resources/org/opendatakit/briefcase/export/{nested-repeats-g2-overwrite.csv.expected => nested-repeats-g1-g2-overwrite.csv.expected} (100%) rename test/resources/org/opendatakit/briefcase/export/{nested-repeats-g2.csv.expected => nested-repeats-g1-g2.csv.expected} (100%) diff --git a/test/java/org/opendatakit/briefcase/export/ExportToCsvScenario.java b/test/java/org/opendatakit/briefcase/export/ExportToCsvScenario.java index efaf5bd72..2c654f2c0 100644 --- a/test/java/org/opendatakit/briefcase/export/ExportToCsvScenario.java +++ b/test/java/org/opendatakit/briefcase/export/ExportToCsvScenario.java @@ -176,9 +176,11 @@ void assertSameMedia(String suffix) { } void assertSameContentRepeats(String suffix, String... groupNames) { + final StringBuilder groupPrefix = new StringBuilder(); Arrays.asList(groupNames).forEach(groupName -> { - String oldOutput = new String(readAllBytes(getPath(formDef.getFormId() + "-" + groupName + (suffix.isEmpty() ? "" : "-" + suffix) + ".csv.expected"))); - String newOutput = new String(readAllBytes(outputDir.resolve("new").resolve(stripIllegalChars(formDef.getFormName()) + "-" + groupName + ".csv"))); + String oldOutput = new String(readAllBytes(getPath(formDef.getFormId() + "-" + groupPrefix.toString() + groupName + (suffix.isEmpty() ? "" : "-" + suffix) + ".csv.expected"))); + String newOutput = new String(readAllBytes(outputDir.resolve("new").resolve(stripIllegalChars(formDef.getFormName()) + "-" + groupPrefix.toString() + groupName + ".csv"))); + groupPrefix.append(groupName).append("-"); assertThat(newOutput, is(oldOutput)); }); } diff --git a/test/resources/org/opendatakit/briefcase/export/nested-repeats-g2-append.csv.expected b/test/resources/org/opendatakit/briefcase/export/nested-repeats-g1-g2-append.csv.expected similarity index 100% rename from test/resources/org/opendatakit/briefcase/export/nested-repeats-g2-append.csv.expected rename to test/resources/org/opendatakit/briefcase/export/nested-repeats-g1-g2-append.csv.expected diff --git a/test/resources/org/opendatakit/briefcase/export/nested-repeats-g3-append.csv.expected b/test/resources/org/opendatakit/briefcase/export/nested-repeats-g1-g2-g3-append.csv.expected similarity index 100% rename from test/resources/org/opendatakit/briefcase/export/nested-repeats-g3-append.csv.expected rename to test/resources/org/opendatakit/briefcase/export/nested-repeats-g1-g2-g3-append.csv.expected diff --git a/test/resources/org/opendatakit/briefcase/export/nested-repeats-g3-overwrite.csv.expected b/test/resources/org/opendatakit/briefcase/export/nested-repeats-g1-g2-g3-overwrite.csv.expected similarity index 100% rename from test/resources/org/opendatakit/briefcase/export/nested-repeats-g3-overwrite.csv.expected rename to test/resources/org/opendatakit/briefcase/export/nested-repeats-g1-g2-g3-overwrite.csv.expected diff --git a/test/resources/org/opendatakit/briefcase/export/nested-repeats-g3.csv.expected b/test/resources/org/opendatakit/briefcase/export/nested-repeats-g1-g2-g3.csv.expected similarity index 100% rename from test/resources/org/opendatakit/briefcase/export/nested-repeats-g3.csv.expected rename to test/resources/org/opendatakit/briefcase/export/nested-repeats-g1-g2-g3.csv.expected diff --git a/test/resources/org/opendatakit/briefcase/export/nested-repeats-g2-overwrite.csv.expected b/test/resources/org/opendatakit/briefcase/export/nested-repeats-g1-g2-overwrite.csv.expected similarity index 100% rename from test/resources/org/opendatakit/briefcase/export/nested-repeats-g2-overwrite.csv.expected rename to test/resources/org/opendatakit/briefcase/export/nested-repeats-g1-g2-overwrite.csv.expected diff --git a/test/resources/org/opendatakit/briefcase/export/nested-repeats-g2.csv.expected b/test/resources/org/opendatakit/briefcase/export/nested-repeats-g1-g2.csv.expected similarity index 100% rename from test/resources/org/opendatakit/briefcase/export/nested-repeats-g2.csv.expected rename to test/resources/org/opendatakit/briefcase/export/nested-repeats-g1-g2.csv.expected