MODBULKOPS-451 - Rework errors preview (#353)

* MODBULKOPS-451 - Rework errors preview * Merge branch 'master' into MODBULKOPS-451 # Conflicts: # src/main/java/org/folio/bulkops/controller/BulkOperationController.java # src/main/java/org/folio/bulkops/service/ErrorService.java # src/main/resources/swagger.api/bulk-operations.yaml # src/test/java/org/folio/bulkops/service/ErrorServiceTest.java * MODBULKOPS-451 - Rework errors preview * MODBULKOPS-451 - Rework errors preview * MODBULKOPS-451 - Rework errors preview
folio-org · Jan 29, 2025 · caedd25 · caedd25
1 parent 2ac7f6f
commit caedd25
Show file tree

Hide file tree

Showing 8 changed files with 94 additions and 47 deletions.
diff --git a/descriptors/ModuleDescriptor-template.json b/descriptors/ModuleDescriptor-template.json
@@ -4,7 +4,7 @@
   "provides": [
     {
       "id": "bulk-operations",
-      "version": "1.5",
+      "version": "1.6",
       "handlers": [
         {
           "methods": [ "POST" ],

diff --git a/src/main/java/org/folio/bulkops/client/BulkEditClient.java b/src/main/java/org/folio/bulkops/client/BulkEditClient.java
@@ -3,13 +3,10 @@
 import java.util.UUID;
 
 import org.folio.bulkops.configs.FeignClientConfiguration;
-import org.folio.bulkops.domain.dto.Errors;
 import org.springframework.cloud.openfeign.FeignClient;
 import org.springframework.http.MediaType;
-import org.springframework.web.bind.annotation.GetMapping;
 import org.springframework.web.bind.annotation.PathVariable;
 import org.springframework.web.bind.annotation.PostMapping;
-import org.springframework.web.bind.annotation.RequestParam;
 import org.springframework.web.bind.annotation.RequestPart;
 import org.springframework.web.multipart.MultipartFile;
 
@@ -20,7 +17,4 @@ public interface BulkEditClient {
 
   @PostMapping(value = "/{jobId}/start")
   void startJob(@PathVariable UUID jobId);
-
-  @GetMapping(value = "/{jobId}/errors")
-  Errors getErrorsPreview(@PathVariable UUID jobId, @RequestParam int limit);
 }
diff --git a/src/main/java/org/folio/bulkops/repository/BulkOperationExecutionContentRepository.java b/src/main/java/org/folio/bulkops/repository/BulkOperationExecutionContentRepository.java
@@ -8,6 +8,7 @@
 import org.folio.spring.data.OffsetRequest;
 import org.springframework.data.domain.Page;
 import org.springframework.data.jpa.repository.JpaRepository;
+import org.springframework.data.jpa.repository.Query;
 import org.springframework.stereotype.Repository;
 
 @Repository
@@ -16,6 +17,7 @@ public interface BulkOperationExecutionContentRepository extends JpaRepository<B
   Page<BulkOperationExecutionContent> findByBulkOperationIdAndErrorMessageIsNotNullAndErrorTypeIsOrderByErrorType(UUID bulkOperationId, OffsetRequest offsetRequest, ErrorType errorType);
   Optional<BulkOperationExecutionContent> findFirstByBulkOperationIdAndIdentifier(UUID bulkOperationId, String identifier);
   int countAllByBulkOperationIdAndErrorMessageIsNotNullAndErrorTypeIs(UUID bulkOperationId, ErrorType errorType);
-
+  @Query("SELECT COUNT(i) FROM BulkOperationExecutionContent i WHERE i.bulkOperationId = :bulkOperationId")
+  long countByBulkOperationId(UUID bulkOperationId);
   void deleteByBulkOperationId(UUID bulkOperationId);
 }
diff --git a/src/main/java/org/folio/bulkops/service/ErrorService.java b/src/main/java/org/folio/bulkops/service/ErrorService.java
@@ -2,6 +2,7 @@
 
 import static java.util.Objects.isNull;
 import static java.util.Objects.nonNull;
+import static java.util.stream.Collectors.toList;
 import static org.apache.commons.lang3.StringUtils.EMPTY;
 import static org.apache.commons.lang3.StringUtils.LF;
 import static org.folio.bulkops.domain.dto.OperationStatusType.COMPLETED;
@@ -14,7 +15,9 @@
 import static org.folio.bulkops.util.Constants.MSG_NO_MARC_CHANGE_REQUIRED;
 import static org.folio.bulkops.util.Constants.MSG_NO_CHANGE_REQUIRED;
 
+import java.io.BufferedReader;
 import java.io.ByteArrayInputStream;
+import java.io.InputStreamReader;
 import java.time.LocalDate;
 import java.util.ArrayList;
 import java.util.List;
@@ -25,10 +28,8 @@
 import lombok.extern.log4j.Log4j2;
 import org.apache.commons.io.FilenameUtils;
 import org.apache.commons.lang3.StringUtils;
-import org.folio.bulkops.client.BulkEditClient;
 import org.folio.bulkops.client.MetadataProviderClient;
 import org.folio.bulkops.client.RemoteFileSystemClient;
-import org.folio.bulkops.domain.bean.BulkOperationsEntity;
 import org.folio.bulkops.domain.bean.JobLogEntry;
 import org.folio.bulkops.domain.bean.StateType;
 import org.folio.bulkops.domain.dto.Error;
@@ -61,7 +62,6 @@ public class ErrorService {
   private final BulkOperationRepository operationRepository;
   private final RemoteFileSystemClient remoteFileSystemClient;
   private final BulkOperationExecutionContentRepository executionContentRepository;
-  private final BulkEditClient bulkEditClient;
   private final MetadataProviderClient metadataProviderClient;
 
   public void saveError(UUID bulkOperationId, String identifier,  String errorMessage, String uiErrorMessage, String link, ErrorType errorType) {
@@ -98,11 +98,17 @@ public Errors getErrorsPreviewByBulkOperationId(UUID bulkOperationId, int limit,
     var bulkOperation = operationRepository.findById(bulkOperationId)
       .orElseThrow(() -> new NotFoundException("BulkOperation was not found by id=" + bulkOperationId));
     if (Set.of(DATA_MODIFICATION, REVIEW_CHANGES, REVIEWED_NO_MARC_RECORDS).contains(bulkOperation.getStatus()) || COMPLETED_WITH_ERRORS == bulkOperation.getStatus() && noCommittedErrors(bulkOperation) && noCommittedWarnings(bulkOperation)) {
-      var errors = bulkEditClient.getErrorsPreview(bulkOperation.getDataExportJobId(), limit);
-      return new Errors().errors(errors.getErrors().stream()
-          .map(this::prepareInternalErrorRepresentation)
-          .toList())
-        .totalRecords(errors.getTotalRecords());
+      var errors = new BufferedReader(new InputStreamReader(remoteFileSystemClient.get(bulkOperation.getLinkToMatchedRecordsErrorsCsvFile())))
+        .lines()
+        .skip(offset)
+        .limit(limit)
+        .map(message -> {
+          var error = message.split(Constants.COMMA_DELIMETER);
+          return new Error().message(error[1]).parameters(List.of(new Parameter().key(IDENTIFIER).value(error[0])));
+        })
+        .toList();
+      return new Errors().errors(errors)
+        .totalRecords(remoteFileSystemClient.getNumOfLines(bulkOperation.getLinkToMatchedRecordsErrorsCsvFile()));
     } else if (COMPLETED == bulkOperation.getStatus() || COMPLETED_WITH_ERRORS == bulkOperation.getStatus()) {
       return getExecutionErrors(bulkOperationId, limit, offset, errorType);
     } else {
@@ -150,18 +156,19 @@ private boolean noCommittedWarnings(BulkOperation bulkOperation) {
     return isNull(bulkOperation.getCommittedNumOfWarnings()) || bulkOperation.getCommittedNumOfWarnings() == 0;
   }
 
-  private Error prepareInternalErrorRepresentation(Error e) {
-    var error= e.getMessage().split(Constants.COMMA_DELIMETER);
-    return new Error().message(error[1]).parameters(List.of(new Parameter().key(IDENTIFIER).value(error[0])));
-  }
-
   public String getErrorsCsvByBulkOperationId(UUID bulkOperationId, int offset, ErrorType errorType) {
     return getErrorsPreviewByBulkOperationId(bulkOperationId, Integer.MAX_VALUE, offset, errorType).getErrors().stream()
       .map(error -> String.join(Constants.COMMA_DELIMETER, ObjectUtils.isEmpty(error.getParameters()) ? EMPTY : error.getParameters().get(0).getValue(), error.getMessage()))
       .collect(Collectors.joining(Constants.NEW_LINE_SEPARATOR));
   }
 
   private Errors getExecutionErrors(UUID bulkOperationId, int limit, int offset, ErrorType errorType) {
+    var totalRecords = (int) executionContentRepository.countByBulkOperationId(bulkOperationId);
+    if (limit == 0) {
+      return new Errors()
+        .errors(List.of())
+        .totalRecords(totalRecords);
+    }
     Page<BulkOperationExecutionContent> errorPage;
     if (isNull(errorType)) {
       errorPage = executionContentRepository.findByBulkOperationIdAndErrorMessageIsNotNullOrderByErrorType(bulkOperationId, OffsetRequest.of(offset, limit));
@@ -173,7 +180,7 @@ private Errors getExecutionErrors(UUID bulkOperationId, int limit, int offset, E
       .toList();
     return new Errors()
       .errors(errors)
-      .totalRecords((int) errorPage.getTotalElements());
+      .totalRecords(totalRecords);
   }
 
   /**

diff --git a/src/main/resources/swagger.api/bulk-operations.yaml b/src/main/resources/swagger.api/bulk-operations.yaml
@@ -311,12 +311,16 @@ paths:
           required: true
           schema:
             type: integer
+            default: 10
+            minimum: 0
           description: The numbers of errors to return
         - in: query
           name: offset
           required: false
           schema:
             type: integer
+            default: 0
+            minimum: 0
           description: Query offset
         - in: query
           name: errorType

diff --git a/src/main/resources/swagger.api/schemas/errors.json b/src/main/resources/swagger.api/schemas/errors.json
@@ -11,7 +11,7 @@
         "$ref": "error.json"
       }
     },
-    "total_records": {
+    "totalRecords": {
       "description": "Total number of errors",
       "type": "integer"
     }

diff --git a/src/test/java/org/folio/bulkops/service/ErrorServiceTest.java b/src/test/java/org/folio/bulkops/service/ErrorServiceTest.java
@@ -13,15 +13,19 @@
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.mockito.ArgumentMatchers.any;
-import static org.mockito.ArgumentMatchers.anyInt;
 import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 import static wiremock.org.hamcrest.MatcherAssert.assertThat;
+import static wiremock.org.hamcrest.Matchers.anyOf;
+import static wiremock.org.hamcrest.Matchers.contains;
 import static wiremock.org.hamcrest.Matchers.equalTo;
 import static wiremock.org.hamcrest.Matchers.hasSize;
 
+import java.io.IOException;
 import java.io.InputStream;
+import java.nio.file.Files;
+import java.nio.file.Paths;
 import java.time.LocalDate;
 import java.util.Arrays;
 import java.util.List;
@@ -42,10 +46,8 @@
 import org.folio.bulkops.domain.bean.SrsRecord;
 import org.folio.bulkops.domain.dto.Error;
 import org.folio.bulkops.domain.dto.ErrorType;
-import org.folio.bulkops.domain.dto.Errors;
 import org.folio.bulkops.domain.dto.IdentifierType;
 import org.folio.bulkops.domain.dto.OperationStatusType;
-import org.folio.bulkops.domain.dto.Parameter;
 import org.folio.bulkops.domain.entity.BulkOperation;
 import org.folio.bulkops.domain.entity.BulkOperationExecutionContent;
 import org.folio.bulkops.domain.entity.BulkOperationProcessingContent;
@@ -155,19 +157,15 @@ void shouldUploadErrorsAndReturnLinkToFile() {
 
   @ParameterizedTest
   @EnumSource(value = OperationStatusType.class, names = { "DATA_MODIFICATION", "COMPLETED" }, mode = EnumSource.Mode.INCLUDE)
-  void shouldGetErrorsPreviewByBulkOperationId(OperationStatusType statusType) {
+  void shouldGetErrorsPreviewByBulkOperationId(OperationStatusType statusType) throws IOException {
     try (var context =  new FolioExecutionContextSetter(folioExecutionContext)) {
       var operationId = bulkOperationRepository.save(BulkOperation.builder().id(UUID.randomUUID()).dataExportJobId(UUID.randomUUID()).status(statusType).build()).getId();
 
-      var expected = List.of(
-        new Error().message("No match found").parameters(List.of(new Parameter().key("IDENTIFIER").value("123"))),
-        new Error().message("Invalid format").parameters(List.of(new Parameter().key("IDENTIFIER").value("456")))
-      );
-
       mockErrorsData(statusType, operationId);
 
-      var actual = errorService.getErrorsPreviewByBulkOperationId(operationId, 2, 0, null);
-      assertThat(actual.getErrors(), hasSize(2));
+      var actualWithOffset = errorService.getErrorsPreviewByBulkOperationId(operationId, 2, 1, null);
+      assertThat(actualWithOffset.getErrors(), hasSize(1));
+      assertThat(actualWithOffset.getTotalRecords(), equalTo(2));
 
       bulkOperationRepository.deleteById(operationId);
     }
@@ -187,7 +185,7 @@ void shouldRejectErrorsPreviewOnWrongOperationStatus(OperationStatusType statusT
 
   @ParameterizedTest
   @EnumSource(value = OperationStatusType.class, names = { "DATA_MODIFICATION", "COMPLETED" }, mode = EnumSource.Mode.INCLUDE)
-  void shouldGetErrorsCsvByBulkOperationId(OperationStatusType statusType) {
+  void shouldGetErrorsCsvByBulkOperationId(OperationStatusType statusType) throws IOException {
     try (var context =  new FolioExecutionContextSetter(folioExecutionContext)) {
       var operationId = bulkOperationRepository.save(BulkOperation.builder().id(UUID.randomUUID()).dataExportJobId(UUID.randomUUID()).status(statusType).build()).getId();
 
@@ -220,7 +218,7 @@ void shouldRejectErrorsCsvOnWrongOperationStatus(OperationStatusType statusType)
 
   @ParameterizedTest
   @ValueSource(ints = {0, 1})
-  void shouldReturnErrorsPreviewOnCompletedWithErrors(int committedErrors) {
+  void shouldReturnErrorsPreviewOnCompletedWithErrors(int committedErrors) throws IOException {
     try (var context =  new FolioExecutionContextSetter(folioExecutionContext)) {
       var jobId = UUID.randomUUID();
 
@@ -250,6 +248,43 @@ void shouldReturnErrorsPreviewOnCompletedWithErrors(int committedErrors) {
       var errors = errorService.getErrorsPreviewByBulkOperationId(operationId, 10, 0, null);
 
       assertThat(errors.getErrors(), hasSize(2));
+      assertThat(errors.getTotalRecords(), equalTo(2));
+    }
+  }
+
+  @ParameterizedTest
+  @ValueSource(ints = {0, 1})
+  void shouldReturnOnlyErrorsTotalNumberPreviewOnCompletedWithErrors(int committedErrors) throws IOException {
+    try (var context =  new FolioExecutionContextSetter(folioExecutionContext)) {
+      var jobId = UUID.randomUUID();
+
+      var operationId = bulkOperationRepository.save(BulkOperation.builder()
+          .id(UUID.randomUUID())
+          .status(COMPLETED_WITH_ERRORS)
+          .committedNumOfErrors(committedErrors)
+          .dataExportJobId(jobId)
+          .build())
+        .getId();
+
+      mockErrorsData(COMPLETED_WITH_ERRORS, operationId);
+
+      if (committedErrors == 1) {
+        executionContentRepository.save(BulkOperationExecutionContent.builder()
+          .bulkOperationId(operationId)
+          .identifier("123")
+          .errorMessage("No match found")
+          .build());
+        executionContentRepository.save(BulkOperationExecutionContent.builder()
+          .bulkOperationId(operationId)
+          .identifier("456")
+          .errorMessage("Invalid format")
+          .build());
+      }
+
+      var errors = errorService.getErrorsPreviewByBulkOperationId(operationId, 0, 0, null);
+
+      assertThat(errors.getErrors(), hasSize(0));
+      assertThat(errors.getTotalRecords(), equalTo(2));
     }
   }
 
@@ -305,7 +340,10 @@ void testSaveErrorsFromDataImport(IdentifierType identifierType, boolean related
     final var instanceInfo = new RelatedInstanceInfo().withIdList(List.of(instanceId)).withHridList(List.of("instance HRID"));
     when(metadataProviderClient.getJobLogEntries(dataImportJobId.toString(), Integer.MAX_VALUE))
       .thenReturn(new JobLogEntryCollection().withEntries(List.of(new JobLogEntry()
-        .withError("some MARC error").withSourceRecordId(sourceRecordId).withRelatedInstanceInfo(
+        .withError("some MARC error #1").withSourceRecordId(sourceRecordId).withRelatedInstanceInfo(
+          relatedInstanceInfo ? instanceInfo : new RelatedInstanceInfo().withIdList(List.of()).withHridList(List.of())
+        ), new JobLogEntry()
+        .withError("some MARC error #2").withSourceRecordId(sourceRecordId).withRelatedInstanceInfo(
           relatedInstanceInfo ? instanceInfo : new RelatedInstanceInfo().withIdList(List.of()).withHridList(List.of())
         ))));
     when(srsClient.getSrsRecordById(sourceRecordId)).thenReturn(new SrsRecord().withExternalIdsHolder(
@@ -316,15 +354,16 @@ void testSaveErrorsFromDataImport(IdentifierType identifierType, boolean related
           .id(UUID.randomUUID())
           .status(COMPLETED_WITH_ERRORS)
           .identifierType(identifierType)
-          .committedNumOfErrors(1)
+          .committedNumOfErrors(2)
           .dataExportJobId(dataExportJobId)
           .build())
         .getId();
       errorService.saveErrorsFromDataImport(operationId, dataImportJobId);
-      var errors = errorService.getErrorsPreviewByBulkOperationId(operationId, 1, 0, ErrorType.ERROR);
+      var errors = errorService.getErrorsPreviewByBulkOperationId(operationId, 2, 1, ErrorType.ERROR);
 
       assertThat(errors.getErrors(), hasSize(1));
-      assertEquals("some MARC error", errors.getErrors().get(0).getMessage());
+      assertThat(errors.getTotalRecords(), equalTo(2));
+      assertThat(errors.getErrors().stream().map(Error::getMessage).toList(), anyOf(contains("some MARC error #1"), contains("some MARC error #2")));
     }
   }
 
@@ -347,7 +386,7 @@ void shouldNotSaveError_IfErrorFromDataImportIsEmpty() {
           .build())
         .getId();
       errorService.saveErrorsFromDataImport(operationId, dataImportJobId);
-      var errors = errorService.getErrorsPreviewByBulkOperationId(operationId, 1, 0, ErrorType.ERROR);
+      var errors = errorService.getErrorsPreviewByBulkOperationId(operationId, 1, 1, ErrorType.ERROR);
 
       assertThat(errors.getErrors(), hasSize(0));
     }
@@ -405,12 +444,12 @@ void testDataImportException() {
     }
   }
 
-  private void mockErrorsData(OperationStatusType statusType, UUID operationId) {
+  private void mockErrorsData(OperationStatusType statusType, UUID operationId) throws IOException {
     if (DATA_MODIFICATION == statusType || COMPLETED_WITH_ERRORS == statusType) {
-      when(bulkEditClient.getErrorsPreview(any(UUID.class), anyInt()))
-        .thenReturn(new Errors()
-          .errors(List.of(new Error().type(ErrorType.ERROR).message("123,No match found"),
-            new Error().type(ErrorType.ERROR).message("456,Invalid format"))));
+      when(remoteFileSystemClient.get(any()))
+        .thenReturn(Files.newInputStream(Paths.get("src/test/resources/files/errors.csv")));
+      when(remoteFileSystemClient.getNumOfLines(any()))
+        .thenReturn(2);
     } else {
       executionContentRepository.save(BulkOperationExecutionContent.builder()
         .bulkOperationId(operationId)

diff --git a/src/test/resources/files/errors.csv b/src/test/resources/files/errors.csv
@@ -1 +1,2 @@
-12345678,Not Found
+123,No match found
+456,Invalid format