Support Plural Strings with the Same Name but Different Content Acros…

…s Branches

Warning: this is modifying the text unit searcher to add the "branchId" to the TextUnitDTO, this will basically impact everything.

Under normal circumstances, there can only be one active text unit for a given name. However, with multiple branches, it's possible to have multiple text units with the same name. While this isn't an issue for singular strings, it becomes problematic for plural strings, as we group by name when building the AndroidStringDocument. This can lead to too many or duplicated entries for one or more plural forms.

To resolve this, we now include the branch ID in the group by logic. This ensures that plural strings are properly grouped, allowing for correct document generation.

With the Phrase connector, each group will be clearly identified since the ID of each text unit is included in the key. For other connectors that don't use IDs, results may vary.

cli/src/test/java/com/box/l10n/mojito/cli/command/ImportLocalizedAssetCommandTest.java

@@ -113,6 +113,16 @@ public void importAndroidStringsPluralWithThirdPartySync() throws Exception {
  "-s",
  getInputResourcesTestDir("source").getAbsolutePath());
 
+ getL10nJCommander()
+ .run(
+ "push",
+ "-r",
+ repository.getName(),
+ "-s",
+ getInputResourcesTestDir("source2").getAbsolutePath(),
+ "-b",
+ "source2");
+
  getL10nJCommander()
  .run(
  "import",

cli/src/test/resources/com/box/l10n/mojito/cli/command/ImportLocalizedAssetCommandTest_IO/importAndroidStringsPluralWithThirdPartySync/input/source2/res/values/strings.xml

@@ -0,0 +1,7 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<resources>
+ <plurals name="plural_collaborators_other">
+ <item formatted="false" quantity="one">%1$,d collaborator in branch</item>
+ <item formatted="false" quantity="other">%1$,d collaborators in branch</item>
+ </plurals>
+</resources>

webapp/src/main/java/com/box/l10n/mojito/android/strings/AndroidPlural.java

@@ -46,7 +46,7 @@ private Map<AndroidPluralQuantity, AndroidPluralItem> buildItemMap(
  throw new AndroidPluralDuplicateKeyException(
  "A duplicate was found when building an Android Plural. "
  + androidPluralItem
- + " dupplicates: "
+ + " duplicates: "
  + androidPluralItem2);
  }));
  }

webapp/src/main/java/com/box/l10n/mojito/android/strings/AndroidStringDocumentMapper.java

@@ -242,7 +242,19 @@ public static String getKeyToGroupByPluralOtherAndComment(TextUnitDTO textUnit)
  + DEFAULT_ASSET_DELIMITER
  + textUnit.getPluralFormOther()
  + "_"
- + textUnit.getComment();
+ + textUnit.getComment()
+ + "-"
+ // Support plural strings with the same name but different content across branches
+ //
+ // Under normal circumstances, there can only be one active text unit for a given name.
+ // However, with multiple branches, it's possible to have multiple text units with the
+ // same name. While this isn't an issue for singular strings, it becomes problematic for
+ // plural strings, as we group by name when building the AndroidStringDocument. This can
+ // lead to too many or duplicated entries for one or more plural forms.
+ //
+ // To resolve this, we now include the branch ID in the group by logic. This ensures
+ // that plural strings are properly grouped, allowing for correct document generation.
+ + textUnit.getBranchId();
  }
 
  AndroidSingular textUnitToAndroidSingular(TextUnitDTO textUnit, boolean useSource) {

webapp/src/main/java/com/box/l10n/mojito/service/tm/search/TextUnitDTO.java

@@ -34,6 +34,7 @@ public class TextUnitDTO {
  private Long assetTextUnitId;
  private ZonedDateTime tmTextUnitCreatedDate;
  private boolean doNotTranslate;
+ private Long branchId;
 
  public Long getTmTextUnitId() {
  return tmTextUnitId;
@@ -242,4 +243,12 @@ public boolean isDoNotTranslate() {
  public void setDoNotTranslate(boolean doNotTranslate) {
  this.doNotTranslate = doNotTranslate;
  }
+
+ public Long getBranchId() {
+ return branchId;
+ }
+
+ public void setBranchId(Long branchId) {
+ this.branchId = branchId;
+ }
 }

webapp/src/main/java/com/box/l10n/mojito/service/tm/search/TextUnitDTONativeObjectMapper.java

@@ -51,11 +51,12 @@ public TextUnitDTO mapObject(CriteriaResult cr) {
  t.setAssetPath(cr.getString(idx++));
  t.setAssetTextUnitId(cr.getLong(idx++));
  t.setTmTextUnitCreatedDate(JSR310Migration.newDateTimeCtorWithDate(cr.getDate(idx++)));
- t.setDoNotTranslate(Boolean.valueOf(includedInLocalizedFile));
 
  String doNotTranslate = cr.getString(idx++);
  t.setDoNotTranslate(Boolean.valueOf(doNotTranslate));
 
+ t.setBranchId(cr.getLong(idx++));
+
  return t;
  }
 

webapp/src/main/java/com/box/l10n/mojito/service/tm/search/TextUnitSearcher.java

@@ -219,7 +219,8 @@ NativeCriteria getCriteriaForSearch(TextUnitSearcherParameters searchParameters)
  .addProjection("a.path", "assetPath")
  .addProjection("atu.id", "assetTextUnitId")
  .addProjection("tu.created_date", "tmTextUnitCreatedDate")
- .addProjection("atu.do_not_translate", "doNotTranslate"));
+ .addProjection("atu.do_not_translate", "doNotTranslate")
+ .addProjection("atu.branch_id", "branch_id"));
 
  logger.debug("Add search filters");
  NativeJunctionExp conjunction = NativeExps.conjunction();

webapp/src/test/java/com/box/l10n/mojito/android/strings/AndroidStringDocumentMapperTest.java

@@ -217,7 +217,7 @@ public void testReadFromSourceTextUnitsWithPluralsAndWithTmTextUnitIdInName() {
  }
 
  @Test
- public void testReadFromSourceTextUnitsWithDuplicatePlurals() {
+ public void testReadFromSourceTextUnitsWithDuplicatePluralsDifferentAsset() {
  mapper = new AndroidStringDocumentMapper(" _", assetDelimiter);
  textUnits.add(sourceTextUnitDTO(123L, "name0", "content0", "comment0", "my/path0", null, null));
 
@@ -342,7 +342,7 @@ public void testReadFromSourceTextUnitsWithDuplicatePlurals() {
  assertThat(singular.getComment()).isEqualTo("comment0");
 
  assertThat(document.getPlurals()).hasSize(2);
- AndroidPlural plural = document.getPlurals().get(1);
+ AndroidPlural plural = document.getPlurals().get(0);
  assertThat(plural.getName()).isEqualTo("my/path0.xml#@#name1");
  assertThat(plural.getComment()).isEqualTo("comment1");
  assertThat(plural.getItems()).hasSize(6);
@@ -359,7 +359,7 @@ public void testReadFromSourceTextUnitsWithDuplicatePlurals() {
  assertThat(plural.getItems().get(OTHER).getId()).isEqualTo(105L);
  assertThat(plural.getItems().get(OTHER).getContent()).isEqualTo("content1_other");
 
- plural = document.getPlurals().get(0);
+ plural = document.getPlurals().get(1);
  assertThat(plural.getName()).isEqualTo("my/path1.xml#@#name1");
  assertThat(plural.getComment()).isEqualTo("comment1");
  assertThat(plural.getItems()).hasSize(6);
@@ -377,6 +377,175 @@ public void testReadFromSourceTextUnitsWithDuplicatePlurals() {
  assertThat(plural.getItems().get(OTHER).getContent()).isEqualTo("content1_other");
  }
 
+ @Test
+ public void testReadFromSourceTextUnitsWithDuplicatePluralsDifferentBranch() {
+ mapper = new AndroidStringDocumentMapper(" _", assetDelimiter);
+ textUnits.add(sourceTextUnitDTO(123L, "name0", "content0", "comment0", "my/path0", null, null));
+
+ // first group
+ textUnits.add(
+ sourceTextUnitDTO(
+ 100L,
+ "name1 _other",
+ "content1_zero",
+ "comment1",
+ "my/path0.xml",
+ "zero",
+ "name1_other"));
+ textUnits.add(
+ sourceTextUnitDTO(
+ 101L,
+ "name1 _other",
+ "content1_one",
+ "comment1",
+ "my/path0.xml",
+ "one",
+ "name1_other"));
+ textUnits.add(
+ sourceTextUnitDTO(
+ 102L,
+ "name1 _other",
+ "content1_two",
+ "comment1",
+ "my/path0.xml",
+ "two",
+ "name1_other"));
+ textUnits.add(
+ sourceTextUnitDTO(
+ 103L,
+ "name1 _other",
+ "content1_few",
+ "comment1",
+ "my/path0.xml",
+ "few",
+ "name1_other"));
+ textUnits.add(
+ sourceTextUnitDTO(
+ 104L,
+ "name1 _other",
+ "content1_many",
+ "comment1",
+ "my/path0.xml",
+ "many",
+ "name1_other"));
+ textUnits.add(
+ sourceTextUnitDTO(
+ 105L,
+ "name1 _other",
+ "content1_other",
+ "comment1",
+ "my/path0.xml",
+ "other",
+ "name1_other"));
+
+ // second group
+ textUnits.add(
+ sourceTextUnitDTO(
+ 200L,
+ "name1 _other",
+ "content1_zero_b",
+ "comment1",
+ "my/path0.xml",
+ "zero",
+ "name1_other"));
+ textUnits.add(
+ sourceTextUnitDTO(
+ 201L,
+ "name1 _other",
+ "content1_one_b",
+ "comment1",
+ "my/path0.xml",
+ "one",
+ "name1_other"));
+ textUnits.add(
+ sourceTextUnitDTO(
+ 202L,
+ "name1 _other",
+ "content1_two_b",
+ "comment1",
+ "my/path0.xml",
+ "two",
+ "name1_other"));
+ textUnits.add(
+ sourceTextUnitDTO(
+ 203L,
+ "name1 _other",
+ "content1_few_b",
+ "comment1",
+ "my/path0.xml",
+ "few",
+ "name1_other"));
+ textUnits.add(
+ sourceTextUnitDTO(
+ 204L,
+ "name1 _other",
+ "content1_many_b",
+ "comment1",
+ "my/path0.xml",
+ "many",
+ "name1_other"));
+ textUnits.add(
+ sourceTextUnitDTO(
+ 205L,
+ "name1 _other",
+ "content1_other_b",
+ "comment1",
+ "my/path0.xml",
+ "other",
+ "name1_other"));
+
+ // Set different asset extraction id ~= branch to the text units. Keep null on first one
+ int group2StartIdx = 7;
+ for (int i = group2StartIdx; i < group2StartIdx + 6; i++) {
+ textUnits.get(i).setBranchId(123L);
+ }
+
+ document = mapper.readFromSourceTextUnits(textUnits);
+
+ assertThat(document).isNotNull();
+ assertThat(document.getSingulars()).hasSize(1);
+ AndroidSingular singular = document.getSingulars().get(0);
+ assertThat(singular.getId()).isEqualTo(123L);
+ assertThat(singular.getName()).isEqualTo("my/path0#@#name0");
+ assertThat(singular.getContent()).isEqualTo("content0");
+ assertThat(singular.getComment()).isEqualTo("comment0");
+
+ assertThat(document.getPlurals()).hasSize(2);
+ AndroidPlural plural = document.getPlurals().get(0);
+ assertThat(plural.getName()).isEqualTo("my/path0.xml#@#name1");
+ assertThat(plural.getComment()).isEqualTo("comment1");
+ assertThat(plural.getItems()).hasSize(6);
+ assertThat(plural.getItems().get(ZERO).getId()).isEqualTo(100L);
+ assertThat(plural.getItems().get(ZERO).getContent()).isEqualTo("content1_zero");
+ assertThat(plural.getItems().get(ONE).getId()).isEqualTo(101L);
+ assertThat(plural.getItems().get(ONE).getContent()).isEqualTo("content1_one");
+ assertThat(plural.getItems().get(TWO).getId()).isEqualTo(102L);
+ assertThat(plural.getItems().get(TWO).getContent()).isEqualTo("content1_two");
+ assertThat(plural.getItems().get(FEW).getId()).isEqualTo(103L);
+ assertThat(plural.getItems().get(FEW).getContent()).isEqualTo("content1_few");
+ assertThat(plural.getItems().get(MANY).getId()).isEqualTo(104L);
+ assertThat(plural.getItems().get(MANY).getContent()).isEqualTo("content1_many");
+ assertThat(plural.getItems().get(OTHER).getId()).isEqualTo(105L);
+ assertThat(plural.getItems().get(OTHER).getContent()).isEqualTo("content1_other");
+
+ plural = document.getPlurals().get(1);
+ assertThat(plural.getName()).isEqualTo("my/path0.xml#@#name1");
+ assertThat(plural.getComment()).isEqualTo("comment1");
+ assertThat(plural.getItems()).hasSize(6);
+ assertThat(plural.getItems().get(ZERO).getId()).isEqualTo(200L);
+ assertThat(plural.getItems().get(ZERO).getContent()).isEqualTo("content1_zero_b");
+ assertThat(plural.getItems().get(ONE).getId()).isEqualTo(201L);
+ assertThat(plural.getItems().get(ONE).getContent()).isEqualTo("content1_one_b");
+ assertThat(plural.getItems().get(TWO).getId()).isEqualTo(202L);
+ assertThat(plural.getItems().get(TWO).getContent()).isEqualTo("content1_two_b");
+ assertThat(plural.getItems().get(FEW).getId()).isEqualTo(203L);
+ assertThat(plural.getItems().get(FEW).getContent()).isEqualTo("content1_few_b");
+ assertThat(plural.getItems().get(MANY).getId()).isEqualTo(204L);
+ assertThat(plural.getItems().get(MANY).getContent()).isEqualTo("content1_many_b");
+ assertThat(plural.getItems().get(OTHER).getId()).isEqualTo(205L);
+ assertThat(plural.getItems().get(OTHER).getContent()).isEqualTo("content1_other_b");
+ }
+
  @Test
  public void testReadFromTargetTextUnitsForEmptyList() {
  mapper = new AndroidStringDocumentMapper("", assetDelimiter);
@@ -613,7 +782,7 @@ public void testReadFromTargetTextUnitsWithPluralsDuplicated() {
  assertThat(singular.getComment()).isEqualTo("comment0");
 
  assertThat(document.getPlurals()).hasSize(2);
- AndroidPlural plural = document.getPlurals().get(1);
+ AndroidPlural plural = document.getPlurals().get(0);
  assertThat(plural.getName()).isEqualTo("my/path0.xml#@#name1");
  assertThat(plural.getComment()).isEqualTo("comment1");
  assertThat(plural.getItems()).hasSize(6);
@@ -630,7 +799,7 @@ public void testReadFromTargetTextUnitsWithPluralsDuplicated() {
  assertThat(plural.getItems().get(OTHER).getId()).isEqualTo(105L);
  assertThat(plural.getItems().get(OTHER).getContent()).isEqualTo("content1_other");
 
- plural = document.getPlurals().get(0);
+ plural = document.getPlurals().get(1);
  assertThat(plural.getName()).isEqualTo("my/path1.xml#@#name1");
  assertThat(plural.getComment()).isEqualTo("comment1");
  assertThat(plural.getItems()).hasSize(6);