Merge pull request #648 from ggalmazor/issue_646_fix_repeat_csv_key_g…

…eneration

Issue 646 fix repeat csv key generation

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).

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,14 @@ 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 (grandParentIsRoot(current)) {
+      current = current.getParent();
+      suffix = current.getName() + "-" + suffix;
+    }
     Path output = configuration.getExportDir().resolve(
-        repeatFileNameBase + "-" + groupModel.getName() + ".csv"
+        repeatFileNameBase + "-" + suffix + ".csv"
     );
     return new Csv(
         groupModel.fqn(),
@@ -73,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
+   * <p>
+   * Example 1:
+   * <p>
+   * <code><pre>
+   * &lt;data&gt;
+   * &nbsp;&nbsp;&lt;/some_field&gt;
+   * &lt;/data&gt;
+   * </pre></code>
+   * <p>
+   * In this example:
+   * <ul>
+   * <li>&lt;data&gt; has a parent with <code>null</code> name</li>
+   * <li>Returns true on &lt;some_field&gt;</li>
+   * </ul>
+   */
+  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.

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<String> 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())
     );

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<Model> 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;

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;
   }
 

src/org/opendatakit/briefcase/export/XmlElement.java

@@ -58,35 +58,28 @@ 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.getParent(), 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);
-    return prefix + "/" + getName() + "[" + getPlaceAmongSameTagSiblings() + "]";
+  String getCurrentLocalId(Model field, String instanceId) {
+    String prefix = isFirstLevelGroup() ? instanceId : getParent().getCurrentLocalId(field.getParent(), instanceId);
+    return field.isRepeatable()
+        ? prefix + "/" + getName() + "[" + getPlaceAmongSameTagSiblings() + "]"
+        : prefix;
   }
 
   /**
    * 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.getParent(), instanceId);
     return prefix + "/" + getName();
   }
 

test/java/org/opendatakit/briefcase/export/CsvTest.java

@@ -0,0 +1,67 @@
+/*
+ * 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("data")
+        .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());
+  }
+}

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));
     });
   }