Refactor integrity checks in TextUnitBatchImporterService

This is preparatory work to implement the new mode: KEEP_STATUS_IF_SAME_TARGET.

That mode will be similar to the legacy KEEP_STATUS_IF_REJECTED_AND_SAME_TARGET, but it will also retain the status when the integrity checker does not fail.

This is an extension of the legacy behavior to allow marking a translation as invalid when the integrity check didn’t catch the issue, eventually breaking a build.

webapp/src/main/java/com/box/l10n/mojito/rest/textunit/TextUnitWS.java

@@ -1,5 +1,7 @@
 package com.box.l10n.mojito.rest.textunit;
 
+import static com.box.l10n.mojito.service.tm.importer.TextUnitBatchImporterService.IntegrityChecksType.fromLegacy;
+
 import com.box.l10n.mojito.entity.Asset;
 import com.box.l10n.mojito.entity.AssetTextUnit;
 import com.box.l10n.mojito.entity.Locale;
@@ -296,9 +298,9 @@ public PollableTask importTextUnitBatch(@RequestBody String string) {
     PollableFuture<Void> pollableFuture =
         textUnitBatchImporterService.asyncImportTextUnits(
             importTextUnitsBatch.getTextUnits(),
-            importTextUnitsBatch.isIntegrityCheckSkipped(),
-            importTextUnitsBatch.isIntegrityCheckKeepStatusIfFailedAndSameTarget());
-
+            fromLegacy(
+                importTextUnitsBatch.isIntegrityCheckSkipped(),
+                importTextUnitsBatch.isIntegrityCheckKeepStatusIfFailedAndSameTarget()));
     return pollableFuture.getPollableTask();
   }
 

webapp/src/main/java/com/box/l10n/mojito/service/asset/ImportTextUnitJob.java

@@ -3,6 +3,8 @@
 import com.box.l10n.mojito.quartz.QuartzPollableJob;
 import com.box.l10n.mojito.service.pollableTask.PollableTaskService;
 import com.box.l10n.mojito.service.tm.importer.TextUnitBatchImporterService;
+import com.box.l10n.mojito.service.tm.search.TextUnitDTO;
+import java.util.List;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.springframework.beans.factory.annotation.Autowired;
@@ -24,10 +26,9 @@ public class ImportTextUnitJob extends QuartzPollableJob<ImportTextUnitJobInput,
   @Override
   public Void call(ImportTextUnitJobInput input) throws Exception {
     logger.debug("Run ImportTextUnitJob");
-    textUnitBatchImporterService.importTextUnits(
-        input.getTextUnitDTOs(),
-        input.isIntegrityCheckSkipped(),
-        input.isIntegrityCheckKeepStatusIfFailedAndSameTarget());
+    List<TextUnitDTO> textUnitDTOs = input.getTextUnitDTOs();
+
+    textUnitBatchImporterService.importTextUnits(textUnitDTOs, input.getIntegrityChecksType());
     return null;
   }
 }

webapp/src/main/java/com/box/l10n/mojito/service/asset/ImportTextUnitJobInput.java

@@ -1,5 +1,6 @@
 package com.box.l10n.mojito.service.asset;
 
+import com.box.l10n.mojito.service.tm.importer.TextUnitBatchImporterService;
 import com.box.l10n.mojito.service.tm.search.TextUnitDTO;
 import java.util.List;
 
@@ -8,26 +9,16 @@
  */
 public class ImportTextUnitJobInput {
 
-  boolean integrityCheckSkipped;
-  boolean integrityCheckKeepStatusIfFailedAndSameTarget;
   List<TextUnitDTO> textUnitDTOs;
+  TextUnitBatchImporterService.IntegrityChecksType integrityChecksType;
 
-  public boolean isIntegrityCheckSkipped() {
-    return integrityCheckSkipped;
+  public TextUnitBatchImporterService.IntegrityChecksType getIntegrityChecksType() {
+    return integrityChecksType;
   }
 
-  public void setIntegrityCheckSkipped(boolean integrityCheckSkipped) {
-    this.integrityCheckSkipped = integrityCheckSkipped;
-  }
-
-  public boolean isIntegrityCheckKeepStatusIfFailedAndSameTarget() {
-    return integrityCheckKeepStatusIfFailedAndSameTarget;
-  }
-
-  public void setIntegrityCheckKeepStatusIfFailedAndSameTarget(
-      boolean integrityCheckKeepStatusIfFailedAndSameTarget) {
-    this.integrityCheckKeepStatusIfFailedAndSameTarget =
-        integrityCheckKeepStatusIfFailedAndSameTarget;
+  public void setIntegrityChecksType(
+      TextUnitBatchImporterService.IntegrityChecksType integrityChecksType) {
+    this.integrityChecksType = integrityChecksType;
   }
 
   public List<TextUnitDTO> getTextUnitDTOs() {

webapp/src/main/java/com/box/l10n/mojito/service/asset/VirtualAssetService.java

@@ -2,6 +2,7 @@
 
 import static com.box.l10n.mojito.entity.TMTextUnitVariant.Status.APPROVED;
 import static com.box.l10n.mojito.quartz.QuartzSchedulerManager.DEFAULT_SCHEDULER_NAME;
+import static com.box.l10n.mojito.service.tm.importer.TextUnitBatchImporterService.IntegrityChecksType.fromLegacy;
 import static org.slf4j.LoggerFactory.getLogger;
 
 import com.box.l10n.mojito.entity.Asset;
@@ -341,7 +342,8 @@ public PollableFuture<Void> importLocalizedTextUnits(
       textUnitDTOs.add(textUnitDTO);
     }
 
-    return textUnitBatchImporterService.asyncImportTextUnits(textUnitDTOs, false, false);
+    return textUnitBatchImporterService.asyncImportTextUnits(
+        textUnitDTOs, fromLegacy(false, false));
   }
 
   @Transactional

webapp/src/main/java/com/box/l10n/mojito/service/machinetranslation/RepositoryMachineTranslationService.java

@@ -1,5 +1,7 @@
 package com.box.l10n.mojito.service.machinetranslation;
 
+import static com.box.l10n.mojito.service.tm.importer.TextUnitBatchImporterService.IntegrityChecksType.fromLegacy;
+
 import com.box.l10n.mojito.entity.Repository;
 import com.box.l10n.mojito.service.pollableTask.Pollable;
 import com.box.l10n.mojito.service.pollableTask.PollableFuture;
@@ -127,7 +129,7 @@ public PollableFuture<Void> translateRepository(
                         .collect(Collectors.toList());
 
                 textUnitBatchImporterService.importTextUnits(
-                    machineTranslatedTextUnitDTOs, false, true);
+                    machineTranslatedTextUnitDTOs, fromLegacy(false, true));
               });
     }
 

webapp/src/main/java/com/box/l10n/mojito/service/thirdparty/ThirdPartyTMSPhrase.java

@@ -1,5 +1,7 @@
 package com.box.l10n.mojito.service.thirdparty;
 
+import static com.box.l10n.mojito.service.tm.importer.TextUnitBatchImporterService.IntegrityChecksType.fromLegacy;
+
 import com.box.l10n.mojito.JSR310Migration;
 import com.box.l10n.mojito.android.strings.AndroidStringDocument;
 import com.box.l10n.mojito.android.strings.AndroidStringDocumentMapper;
@@ -426,8 +428,9 @@ private void pullLocale(
     textUnitDTOS.forEach(t -> t.setComment(null));
 
     Stopwatch importStopWatch = Stopwatch.createStarted();
+
     textUnitBatchImporterService.importTextUnits(
-        textUnitDTOS, false, integrityCheckKeepStatusIfFailedAndSameTarget);
+        textUnitDTOS, fromLegacy(false, integrityCheckKeepStatusIfFailedAndSameTarget));
     logger.info("Time importing text units: {}", importStopWatch.elapsed());
   }
 

webapp/src/main/java/com/box/l10n/mojito/service/thirdparty/ThirdPartyTMSSmartlingWithJson.java

@@ -2,6 +2,7 @@
 
 import static com.box.l10n.mojito.service.thirdparty.ThirdPartyTMSUtils.isFileEqualToPreviousRun;
 import static com.box.l10n.mojito.service.thirdparty.smartling.SmartlingFileUtils.isPluralFile;
+import static com.box.l10n.mojito.service.tm.importer.TextUnitBatchImporterService.IntegrityChecksType.fromLegacy;
 
 import com.box.l10n.mojito.entity.Repository;
 import com.box.l10n.mojito.entity.RepositoryLocale;
@@ -250,7 +251,9 @@ && isFileEqualToPreviousRun(
                               t.setRepositoryName(repository.getName());
                               t.setTargetLocale(repositoryLocale.getLocale().getBcp47Tag());
                             });
-                    textUnitBatchImporterService.importTextUnits(textUnitDTOS, false, true);
+
+                    textUnitBatchImporterService.importTextUnits(
+                        textUnitDTOS, fromLegacy(false, true));
                   });
         });
   }

webapp/src/main/java/com/box/l10n/mojito/service/thirdparty/smartling/quartz/SmartlingPullLocaleFileJob.java

@@ -1,6 +1,7 @@
 package com.box.l10n.mojito.service.thirdparty.smartling.quartz;
 
 import static com.box.l10n.mojito.service.thirdparty.ThirdPartyTMSUtils.isFileEqualToPreviousRun;
+import static com.box.l10n.mojito.service.tm.importer.TextUnitBatchImporterService.IntegrityChecksType.fromLegacy;
 
 import com.box.l10n.mojito.LocaleMappingHelper;
 import com.box.l10n.mojito.android.strings.AndroidStringDocumentMapper;
@@ -111,7 +112,8 @@ && matchesChecksumFromPreviousSync(input, localeTag, fileName, fileContent)) {
 
       if (!input.isDryRun()) {
         logger.debug("Importing text units for locale: {}", smartlingLocale);
-        textUnitBatchImporterService.importTextUnits(textUnits, false, true);
+
+        textUnitBatchImporterService.importTextUnits(textUnits, fromLegacy(false, true));
       }
     }
     return null;

webapp/src/main/java/com/box/l10n/mojito/service/tm/importer/TextUnitBatchImporterService.java

@@ -96,6 +96,40 @@ public class TextUnitBatchImporterService {
   @Value("${l10n.textUnitBatchImporterService.quartz.schedulerName:" + DEFAULT_SCHEDULER_NAME + "}")
   String schedulerName;
 
+  public enum IntegrityChecksType {
+    /** Don't run integrity checks */
+    SKIP,
+    /** Always use the status from the integrity checker (legacy behavior 1) */
+    ALWAYS,
+    /**
+     * Run integrity checks. If it fails and the target is the same, keep the current status;
+     * otherwise, reject (legacy behavior 2).
+     */
+    KEEP_STATUS_IF_REJECTED_AND_SAME_TARGET,
+    /**
+     * Run integrity checks. If the target is the same, keep the current status.
+     *
+     * <p>This is an extension of the legacy behavior that allows marking a translation as invalid
+     * when the integrity check did not catch the issue, eventually causing a build failure.
+     */
+    KEEP_STATUS_IF_SAME_TARGET;
+
+    public static IntegrityChecksType fromLegacy(
+        boolean integrityCheckSkipped, boolean integrityCheckKeepStatusIfFailedAndSameTarget) {
+      IntegrityChecksType legacy = IntegrityChecksType.SKIP;
+
+      if (!integrityCheckSkipped) {
+        if (!integrityCheckKeepStatusIfFailedAndSameTarget) {
+          legacy = ALWAYS;
+        } else {
+          legacy = KEEP_STATUS_IF_REJECTED_AND_SAME_TARGET;
+        }
+      }
+
+      return legacy;
+    }
+  }
+
   /**
    * Imports a batch of text units.
    *
@@ -115,25 +149,19 @@ public class TextUnitBatchImporterService {
    * @return
    */
   public PollableFuture<Void> asyncImportTextUnits(
-      List<TextUnitDTO> textUnitDTOs,
-      boolean integrityCheckSkipped,
-      boolean integrityCheckKeepStatusIfFailedAndSameTarget) {
+      List<TextUnitDTO> textUnitDTOs, IntegrityChecksType integrityChecksType) {
 
     ImportTextUnitJobInput importTextUnitJobInput = new ImportTextUnitJobInput();
     importTextUnitJobInput.setTextUnitDTOs(textUnitDTOs);
-    importTextUnitJobInput.setIntegrityCheckSkipped(integrityCheckSkipped);
-    importTextUnitJobInput.setIntegrityCheckKeepStatusIfFailedAndSameTarget(
-        integrityCheckKeepStatusIfFailedAndSameTarget);
+    importTextUnitJobInput.setIntegrityChecksType(integrityChecksType);
 
     return quartzPollableTaskScheduler.scheduleJob(
         ImportTextUnitJob.class, importTextUnitJobInput, schedulerName);
   }
 
   @StopWatch
   public PollableFuture<Void> importTextUnits(
-      List<TextUnitDTO> textUnitDTOs,
-      boolean integrityCheckSkipped,
-      boolean integrityCheckKeepStatusIfFailedAndSameTarget) {
+      List<TextUnitDTO> textUnitDTOs, IntegrityChecksType integrityChecksType) {
 
     return meterRegistry
         .timer("TextUnitBatchImporterService.importTextUnits")
@@ -164,7 +192,7 @@ public PollableFuture<Void> importTextUnits(
 
                             mapTextUnitsToImportWithExistingTextUnits(
                                 locale, asset, textUnitsForBatchImport);
-                            if (!integrityCheckSkipped) {
+                            if (!IntegrityChecksType.SKIP.equals(integrityChecksType)) {
                               try (var timer2 =
                                   Timer.resource(
                                           meterRegistry,
@@ -173,9 +201,7 @@ public PollableFuture<Void> importTextUnits(
                                       .tag("asset", asset.getPath())) {
 
                                 applyIntegrityChecks(
-                                    asset,
-                                    textUnitsForBatchImport,
-                                    integrityCheckKeepStatusIfFailedAndSameTarget);
+                                    asset, textUnitsForBatchImport, integrityChecksType);
                               }
                             }
                             importTextUnitsOfLocaleAndAsset(locale, asset, textUnitsForBatchImport);
@@ -294,16 +320,39 @@ boolean isUpdateNeeded(TextUnitForBatchMatcherImport textUnitForBatchImport) {
 
     TextUnitDTO currentTextUnit = textUnitForBatchImport.getCurrentTextUnit();
 
-    return currentTextUnit.getTarget() == null
-        || tmService.isUpdateNeededForTmTextUnitVariant(
-            currentTextUnit.getStatus(),
-            DigestUtils.md5Hex(currentTextUnit.getTarget()),
-            currentTextUnit.isIncludedInLocalizedFile(),
-            currentTextUnit.getTargetComment(),
-            textUnitForBatchImport.getStatus(),
-            DigestUtils.md5Hex(textUnitForBatchImport.getContent()),
-            textUnitForBatchImport.isIncludedInLocalizedFile(),
-            textUnitForBatchImport.getTargetComment());
+    boolean result;
+    if (currentTextUnit.getTarget() == null) {
+      result = true;
+    } else {
+      result =
+          tmService.isUpdateNeededForTmTextUnitVariant(
+              currentTextUnit.getStatus(),
+              DigestUtils.md5Hex(currentTextUnit.getTarget()),
+              currentTextUnit.isIncludedInLocalizedFile(),
+              currentTextUnit.getTargetComment(),
+              textUnitForBatchImport.getStatus(),
+              DigestUtils.md5Hex(textUnitForBatchImport.getContent()),
+              textUnitForBatchImport.isIncludedInLocalizedFile(),
+              textUnitForBatchImport.getTargetComment());
+
+      // we don't want to override the "rejected" field, if it was set manually. This would happen
+      // when a build is
+      // broken and a string needs to be excluded. Right now, on the next sync the string would be
+      // re-included
+      // Of course that means now that if a string was wrongly marked as rejected, eg by a faulty
+      // integrity check
+      // re-running the check won't fix the issue.
+      // I thought I added an option to handle that though.
+      // Maybe I could have a re-apply checks ... the problem with that is ..
+      // we can make that an option too
+      if (result
+          && !currentTextUnit.isIncludedInLocalizedFile()
+          && currentTextUnit.getTarget().equals(textUnitForBatchImport.getContent())) {
+        result = false;
+      }
+    }
+
+    return result;
   }
 
   List<TextUnitDTO> getTextUnitTDOsForLocaleAndAsset(Locale locale, Asset asset) {
@@ -314,7 +363,7 @@ List<TextUnitDTO> getTextUnitTDOsForLocaleAndAsset(Locale locale, Asset asset) {
   void applyIntegrityChecks(
       Asset asset,
       List<TextUnitForBatchMatcherImport> textUnitsForBatchImport,
-      boolean keepStatusIfCheckFailedAndSameTarget) {
+      IntegrityChecksType integrityChecksType) {
 
     Set<TextUnitIntegrityChecker> textUnitCheckers =
         integrityCheckerFactory.getTextUnitCheckers(asset);
@@ -338,7 +387,7 @@ void applyIntegrityChecks(
           boolean hasSameTarget =
               textUnitForBatchImport.getContent().equals(currentTextUnit.getTarget());
 
-          if (hasSameTarget && keepStatusIfCheckFailedAndSameTarget) {
+          if (hasSameTarget && !IntegrityChecksType.ALWAYS.equals(integrityChecksType)) {
             textUnitForBatchImport.setIncludedInLocalizedFile(
                 currentTextUnit.isIncludedInLocalizedFile());
             textUnitForBatchImport.setStatus(currentTextUnit.getStatus());

webapp/src/test/java/com/box/l10n/mojito/service/branch/notification/BranchNotificationServiceTest.java

@@ -1,5 +1,7 @@
 package com.box.l10n.mojito.service.branch.notification;
 
+import static com.box.l10n.mojito.service.tm.importer.TextUnitBatchImporterService.IntegrityChecksType.fromLegacy;
+
 import com.box.l10n.mojito.entity.AssetContent;
 import com.box.l10n.mojito.entity.Branch;
 import com.box.l10n.mojito.entity.BranchNotification;
@@ -152,8 +154,9 @@ void translateBranch(Branch branch) throws ExecutionException, InterruptedExcept
     textUnitBatchImporterService
         .asyncImportTextUnits(
             importTextUnitsBatch.getTextUnits(),
-            importTextUnitsBatch.isIntegrityCheckSkipped(),
-            importTextUnitsBatch.isIntegrityCheckKeepStatusIfFailedAndSameTarget())
+            fromLegacy(
+                importTextUnitsBatch.isIntegrityCheckSkipped(),
+                importTextUnitsBatch.isIntegrityCheckKeepStatusIfFailedAndSameTarget()))
         .get();
 
     // make sure the stats are ready for whatever is done next in notificaiton

webapp/src/test/java/com/box/l10n/mojito/service/thirdparty/ThirdPartyTMSSmartlingTest.java

@@ -2,6 +2,7 @@
 
 import static com.box.l10n.mojito.android.strings.AndroidStringDocumentReader.fromText;
 import static com.box.l10n.mojito.quartz.QuartzSchedulerManager.DEFAULT_SCHEDULER_NAME;
+import static com.box.l10n.mojito.service.tm.importer.TextUnitBatchImporterService.IntegrityChecksType.fromLegacy;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.Assert.assertThrows;
 import static org.junit.Assert.assertTrue;
@@ -199,8 +200,7 @@ public void setUp() throws SchedulerException {
             anyString());
     doReturn(null)
         .when(mockTextUnitBatchImporterService)
-        .importTextUnits(any(), eq(false), eq(true));
-
+        .importTextUnits(any(), eq(fromLegacy(false, true)));
     resultProcessor = new StubSmartlingResultProcessor();
     tmsSmartling =
         new ThirdPartyTMSSmartling(