Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MODBULKOPS-451 - Rework errors preview #353

Merged
merged 8 commits into from
Jan 29, 2025
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion descriptors/ModuleDescriptor-template.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
"provides": [
{
"id": "bulk-operations",
"version": "1.5",
"version": "1.6",
"handlers": [
{
"methods": [ "POST" ],
Expand Down
6 changes: 0 additions & 6 deletions src/main/java/org/folio/bulkops/client/BulkEditClient.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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);
}
27 changes: 14 additions & 13 deletions src/main/java/org/folio/bulkops/service/ErrorService.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -12,7 +13,9 @@
import static org.folio.bulkops.util.Constants.DATA_IMPORT_ERROR_DISCARDED;
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;
Expand All @@ -23,10 +26,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;
Expand Down Expand Up @@ -59,7 +60,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) {
Expand Down Expand Up @@ -95,11 +95,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])));
})
.collect(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 {
Expand Down Expand Up @@ -147,11 +153,6 @@ 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()))
Expand Down
49 changes: 26 additions & 23 deletions src/test/java/org/folio/bulkops/service/ErrorServiceTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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);
}
Expand All @@ -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();

Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -250,6 +248,7 @@ void shouldReturnErrorsPreviewOnCompletedWithErrors(int committedErrors) {
var errors = errorService.getErrorsPreviewByBulkOperationId(operationId, 10, 0, null);

assertThat(errors.getErrors(), hasSize(2));
assertThat(errors.getTotalRecords(), equalTo(2));
}
}

Expand Down Expand Up @@ -305,7 +304,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(
Expand All @@ -316,15 +318,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")));
}
}

Expand All @@ -347,7 +350,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));
}
Expand Down Expand Up @@ -405,12 +408,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)
Expand Down
3 changes: 2 additions & 1 deletion src/test/resources/files/errors.csv
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
12345678,Not Found
123,No match found
456,Invalid format