From 751ce1e6cf6f20eadf293750d502a52ddfdb98ad Mon Sep 17 00:00:00 2001 From: Boburbek Kadirkhodjaev Date: Tue, 10 Dec 2024 14:42:07 +0500 Subject: [PATCH] [EUREKA-561-5]. Simplify location validation, update tests (#1062) --- .../org/folio/helper/PurchaseOrderHelper.java | 28 ++-- .../folio/helper/PurchaseOrderLineHelper.java | 2 +- .../CompositePoLineValidationService.java | 22 +-- .../folio/helper/PurchaseOrderHelperTest.java | 2 +- .../CompositePoLineValidationServiceTest.java | 137 ++++++++++++------ 5 files changed, 114 insertions(+), 77 deletions(-) diff --git a/src/main/java/org/folio/helper/PurchaseOrderHelper.java b/src/main/java/org/folio/helper/PurchaseOrderHelper.java index c44a558ff..9688506e5 100644 --- a/src/main/java/org/folio/helper/PurchaseOrderHelper.java +++ b/src/main/java/org/folio/helper/PurchaseOrderHelper.java @@ -234,20 +234,10 @@ public Future updateOrder(CompositePurchaseOrder compPO, boolean deleteHol boolean isTransitionToOpen = isTransitionToOpen(poFromStorage, compPO); return orderValidationService.validateOrderForUpdate(compPO, poFromStorage, deleteHoldings, requestContext) .compose(v -> { - var poLineFutures = new ArrayList>(); - if (!compPO.getCompositePoLines().isEmpty()) { - compPO.getCompositePoLines().forEach(poLine -> { - var compPoLineFromStorage = clonedPoFromStorage.getCompositePoLines().stream() - .filter(entry -> StringUtils.equals(entry.getId(), poLine.getId())) - .findFirst().orElse(null); - if (Objects.nonNull(compPoLineFromStorage)) { - var updatedLocations = poLine.getLocations(); - var storedLocations = compPoLineFromStorage.getLocations(); - poLineFutures.add(compositePoLineValidationService.validateUserUnaffiliatedLocationUpdates(poLine.getId(), updatedLocations, storedLocations, requestContext)); - } - }); + if (isTransitionToOpen && CollectionUtils.isEmpty(compPO.getCompositePoLines())) { + compPO.setCompositePoLines(clonedPoFromStorage.getCompositePoLines()); } - return collectResultsOnSuccess(poLineFutures); + return validateUserUnaffiliatedPoLineLocations(compPO, requestContext); }) .compose(v -> { if (isTransitionToClosed(poFromStorage, compPO)) { @@ -257,9 +247,6 @@ public Future updateOrder(CompositePurchaseOrder compPO, boolean deleteHol }) .compose(v -> { if (isTransitionToOpen) { - if (CollectionUtils.isEmpty(compPO.getCompositePoLines())) { - compPO.setCompositePoLines(clonedPoFromStorage.getCompositePoLines()); - } compPO.getCompositePoLines().forEach(poLine -> PoLineCommonUtil.updateLocationsQuantity(poLine.getLocations())); return openCompositeOrderFlowValidator.checkLocationsAndPiecesConsistency(compPO.getCompositePoLines(), requestContext) .compose(ok -> openCompositeOrderFlowValidator.checkFundLocationRestrictions(compPO.getCompositePoLines(), requestContext)); @@ -292,6 +279,15 @@ public Future updateOrder(CompositePurchaseOrder compPO, boolean deleteHol }); } + private Future> validateUserUnaffiliatedPoLineLocations(CompositePurchaseOrder purchaseOrder, RequestContext requestContext) { + var poLineFutures = new ArrayList>(); + purchaseOrder.getCompositePoLines().stream().filter(Objects::nonNull).forEach(poLine -> { + var poLineFuture = compositePoLineValidationService.validateUserUnaffiliatedLocations(poLine.getId(), poLine.getLocations(), requestContext); + poLineFutures.add(poLineFuture); + }); + return collectResultsOnSuccess(poLineFutures); + } + public Future handleFinalOrderStatus(CompositePurchaseOrder compPO, String initialOrdersStatus, RequestContext requestContext) { PurchaseOrder purchaseOrder = convertToPurchaseOrder(compPO); diff --git a/src/main/java/org/folio/helper/PurchaseOrderLineHelper.java b/src/main/java/org/folio/helper/PurchaseOrderLineHelper.java index 9198653dd..33ebe2c77 100644 --- a/src/main/java/org/folio/helper/PurchaseOrderLineHelper.java +++ b/src/main/java/org/folio/helper/PurchaseOrderLineHelper.java @@ -278,7 +278,7 @@ public Future updateOrderLine(CompositePoLine compOrderLine, RequestContex .compose(compOrder -> protectionService.isOperationRestricted(compOrder.getAcqUnitIds(), UPDATE, requestContext) .compose(v -> purchaseOrderLineService.validateAndNormalizeISBNAndProductType(Collections.singletonList(compOrderLine), requestContext)) .compose(v -> validateAccessProviders(compOrderLine, requestContext)) - .compose(v -> compositePoLineValidationService.validateUserUnaffiliatedLocationUpdates(compOrderLine.getId(), compOrderLine.getLocations(), poLineFromStorage.getLocations(), requestContext)) + .compose(v -> compositePoLineValidationService.validateUserUnaffiliatedLocations(compOrderLine.getId(), compOrderLine.getLocations(), requestContext)) .compose(v -> expenseClassValidationService.validateExpenseClassesForOpenedOrder(compOrder, Collections.singletonList(compOrderLine), requestContext)) .compose(v -> processPoLineEncumbrances(compOrder, compOrderLine, poLineFromStorage, requestContext))) .map(v -> compOrderLine.withPoLineNumber(poLineFromStorage.getPoLineNumber())) // PoLine number must not be modified during PoLine update, set original value diff --git a/src/main/java/org/folio/service/orders/CompositePoLineValidationService.java b/src/main/java/org/folio/service/orders/CompositePoLineValidationService.java index e3134a5c8..3af2be40b 100644 --- a/src/main/java/org/folio/service/orders/CompositePoLineValidationService.java +++ b/src/main/java/org/folio/service/orders/CompositePoLineValidationService.java @@ -24,8 +24,8 @@ import io.vertx.core.Future; import org.apache.commons.collections4.CollectionUtils; -import org.apache.commons.collections4.SetUtils; import org.apache.commons.lang3.StringUtils; +import org.apache.http.HttpStatus; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.folio.orders.utils.HelperUtils; @@ -345,27 +345,19 @@ private void validateReceivingWorkflowForBindary(CompositePoLine poLine, List validateUserUnaffiliatedLocationUpdates(String poLineId, List updatedPoLineLocations, - List storedPoLineLocations, RequestContext requestContext) { + public Future validateUserUnaffiliatedLocations(String poLineId, List locations, RequestContext requestContext) { return getUserTenantsIfNeeded(requestContext) .compose(userTenants -> { if (CollectionUtils.isEmpty(userTenants)) { logger.info("validateUserUnaffiliatedLocationUpdates:: User tenants is empty"); return Future.succeededFuture(); } - var storageUnaffiliatedLocations = extractUnaffiliatedLocations(storedPoLineLocations, userTenants); - var updatedUnaffiliatedLocations = extractUnaffiliatedLocations(updatedPoLineLocations, userTenants); - logger.info("validateUserUnaffiliatedLocationUpdates:: Found unaffiliated POL location tenant ids, poLineId: '{}', stored: '{}', updated: '{}'", - poLineId, - storageUnaffiliatedLocations.stream().map(Location::getTenantId).distinct().toList(), - updatedUnaffiliatedLocations.stream().map(Location::getTenantId).distinct().toList()); - if (!SetUtils.isEqualSet(storageUnaffiliatedLocations, updatedUnaffiliatedLocations)) { - logger.info("validateUserUnaffiliatedLocationUpdates:: User is not affiliated with all locations on the POL, poLineId: '{}'", - poLineId); - return Future.failedFuture(new HttpException(422, ErrorCodes.LOCATION_UPDATE_WITHOUT_AFFILIATION)); + var uniqueUnAffiliatedLocations = extractUnaffiliatedLocations(locations, userTenants).stream().map(Location::getTenantId).distinct().toList(); + if (CollectionUtils.isNotEmpty(uniqueUnAffiliatedLocations)) { + logger.info("validateUserUnaffiliatedLocationUpdates:: User has unaffiliated locations on the POL, poLineId: {}, unique locations: {}", poLineId, uniqueUnAffiliatedLocations); + return Future.failedFuture(new HttpException(HttpStatus.SC_UNPROCESSABLE_ENTITY, ErrorCodes.LOCATION_UPDATE_WITHOUT_AFFILIATION)); } - logger.info("validateUserUnaffiliatedLocationUpdates:: User is affiliated with all locations on the POL, poLineId: '{}'", - poLineId); + logger.info("validateUserUnaffiliatedLocationUpdates:: User is affiliated with all {} locations on the POL, poLineId: {}", locations.size(), poLineId); return Future.succeededFuture(); }); } diff --git a/src/test/java/org/folio/helper/PurchaseOrderHelperTest.java b/src/test/java/org/folio/helper/PurchaseOrderHelperTest.java index 3b13e38c7..97d37add7 100644 --- a/src/test/java/org/folio/helper/PurchaseOrderHelperTest.java +++ b/src/test/java/org/folio/helper/PurchaseOrderHelperTest.java @@ -201,7 +201,7 @@ void testPutPendingCompositeOrder() throws IOException { doReturn(succeededFuture(null)) .when(encumbranceService).updateEncumbrancesOrderStatusAndReleaseIfClosed(any(CompositePurchaseOrder.class), eq(requestContext)); doReturn(succeededFuture(null)) - .when(compositePoLineValidationService).validateUserUnaffiliatedLocationUpdates(anyString(), any(), any(), eq(requestContext)); + .when(compositePoLineValidationService).validateUserUnaffiliatedLocations(anyString(), any(), eq(requestContext)); // When Future future = purchaseOrderHelper.putCompositeOrderById(compPO.getId(), deleteHoldings, compPO, requestContext); diff --git a/src/test/java/org/folio/service/orders/CompositePoLineValidationServiceTest.java b/src/test/java/org/folio/service/orders/CompositePoLineValidationServiceTest.java index 423ba2d09..a63797e2a 100644 --- a/src/test/java/org/folio/service/orders/CompositePoLineValidationServiceTest.java +++ b/src/test/java/org/folio/service/orders/CompositePoLineValidationServiceTest.java @@ -1,7 +1,6 @@ package org.folio.service.orders; import static io.vertx.core.Future.succeededFuture; -import static org.folio.TestUtils.getLocationsForTenants; import static org.folio.rest.core.exceptions.ErrorCodes.*; import static org.folio.rest.jaxrs.model.CompositePoLine.OrderFormat.OTHER; import static org.folio.rest.jaxrs.model.CompositePoLine.OrderFormat.P_E_MIX; @@ -12,12 +11,11 @@ import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; -import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.when; -import java.lang.reflect.InvocationTargetException; import java.util.List; import java.util.Optional; import java.util.Set; @@ -25,6 +23,8 @@ import java.util.stream.Collectors; import io.vertx.core.Future; +import io.vertx.junit5.VertxExtension; +import io.vertx.junit5.VertxTestContext; import org.folio.models.consortium.ConsortiumConfiguration; import org.folio.rest.core.exceptions.ErrorCodes; import org.folio.rest.core.exceptions.HttpException; @@ -35,7 +35,6 @@ import org.folio.rest.jaxrs.model.Error; import org.folio.rest.jaxrs.model.Location; import org.folio.rest.jaxrs.model.Physical; -import org.folio.rest.jaxrs.model.PoLine; import org.folio.service.consortium.ConsortiumConfigurationService; import org.folio.service.consortium.ConsortiumUserTenantsRetriever; import org.folio.service.finance.expenceclass.ExpenseClassValidationService; @@ -43,25 +42,24 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.InjectMocks; import org.mockito.Mock; import org.mockito.MockitoAnnotations; +@ExtendWith(VertxExtension.class) public class CompositePoLineValidationServiceTest { + + @Mock private ExpenseClassValidationService expenseClassValidationService; + @Mock private ConsortiumConfigurationService consortiumConfigurationService; + @Mock private ConsortiumUserTenantsRetriever consortiumUserTenantsRetriever; + @Mock private RequestContext requestContext; + @InjectMocks private CompositePoLineValidationService compositePoLineValidationService; + private AutoCloseable mockitoMocks; - @Mock - private ExpenseClassValidationService expenseClassValidationService; - @Mock - private ConsortiumConfigurationService consortiumConfigurationService; - @Mock - private ConsortiumUserTenantsRetriever consortiumUserTenantsRetriever; - @InjectMocks - private CompositePoLineValidationService compositePoLineValidationService; - @Mock - private RequestContext requestContext; @BeforeEach - public void initMocks(){ + public void initMocks() { mockitoMocks = MockitoAnnotations.openMocks(this); } @@ -169,7 +167,6 @@ void shouldReturnErrorIfIncorrectOrderFormatWhenBindaryActive() { assertEquals(ORDER_FORMAT_INCORRECT_FOR_BINDARY_ACTIVE.getCode(), errors.get(0).getCode()); } - @Test void shouldReturnErrorIfIncorrectCreateInventoryWhenBindaryActive() { var compositePoLine = new CompositePoLine() @@ -382,7 +379,6 @@ void testValidatePoLineWithZeroQuantitiesWithoutLocations() { assertThat(errors, hasSize(0)); } - private Set errorsToCodes(List errors) { return errors .stream() @@ -391,38 +387,91 @@ private Set errorsToCodes(List errors) { } @Test - void testValidateUserUnaffiliatedLocationUpdates() throws NoSuchMethodException, InvocationTargetException, IllegalAccessException { - var locationsStored = getLocationsForTenants(List.of("tenant1", "tenant2", "tenant3")); - var locationsUpdated = getLocationsForTenants(List.of("tenant1", "tenant3")); - var storagePoLine = new PoLine().withLocations(locationsStored); - var updatedPoLine = new CompositePoLine().withLocations(locationsUpdated); + void testValidateUserUnaffiliatedLocations(VertxTestContext testContext) { + var locationsUpdated = List.of( + new Location().withLocationId(UUID.randomUUID().toString()).withTenantId("tenant1"), + new Location().withLocationId(UUID.randomUUID().toString()).withTenantId("tenant2")); + var updatedPoLineId = UUID.randomUUID().toString(); + + when(consortiumConfigurationService.getConsortiumConfiguration(eq(requestContext))) + .thenReturn(Future.succeededFuture(Optional.empty())); + when(consortiumUserTenantsRetriever.getUserTenants(eq("consortiumId"), anyString(), eq(requestContext))) + .thenReturn(Future.succeededFuture(List.of("tenant1"))); + + compositePoLineValidationService.validateUserUnaffiliatedLocations(updatedPoLineId, locationsUpdated, requestContext) + .onComplete(testContext.succeeding(result -> testContext.verify(testContext::completeNow))); + } - doReturn(succeededFuture(Optional.of(new ConsortiumConfiguration("tenant1", "consortiumId")))) - .when(consortiumConfigurationService).getConsortiumConfiguration(any(RequestContext.class)); - doReturn(succeededFuture(List.of("tenant1", "tenant2"))) - .when(consortiumUserTenantsRetriever).getUserTenants(eq("consortiumId"), eq("centralTenantId"), any(RequestContext.class)); + @Test + void testValidateUserUnaffiliatedLocationsAllValidLocations(VertxTestContext testContext) { + var locationsUpdated = List.of( + new Location().withLocationId(UUID.randomUUID().toString()).withTenantId("tenant1"), + new Location().withLocationId(UUID.randomUUID().toString()).withTenantId("tenant2")); + var updatedPoLineId = UUID.randomUUID().toString(); + + when(consortiumConfigurationService.getConsortiumConfiguration(eq(requestContext))) + .thenReturn(Future.succeededFuture(Optional.of(new ConsortiumConfiguration("tenant1", "consortiumId")))); + when(consortiumUserTenantsRetriever.getUserTenants(eq("consortiumId"), anyString(), eq(requestContext))) + .thenReturn(Future.succeededFuture(List.of("tenant1", "tenant2"))); + + compositePoLineValidationService.validateUserUnaffiliatedLocations(updatedPoLineId, locationsUpdated, requestContext) + .onComplete(testContext.succeeding(result -> testContext.verify(testContext::completeNow))); + } - var future = compositePoLineValidationService.validateUserUnaffiliatedLocationUpdates(updatedPoLine.getId(), updatedPoLine.getLocations(), storagePoLine.getLocations(), requestContext); + @Test + void testValidateUserUnaffiliatedLocationsAllValidDuplicateTenantLocations(VertxTestContext testContext) { + var locationsUpdated = List.of( + new Location().withLocationId(UUID.randomUUID().toString()).withTenantId("tenant1"), + new Location().withLocationId(UUID.randomUUID().toString()).withTenantId("tenant2"), + new Location().withLocationId(UUID.randomUUID().toString()).withTenantId("tenant2")); + var updatedPoLineId = UUID.randomUUID().toString(); + + when(consortiumConfigurationService.getConsortiumConfiguration(eq(requestContext))) + .thenReturn(Future.succeededFuture(Optional.of(new ConsortiumConfiguration("tenant1", "consortiumId")))); + when(consortiumUserTenantsRetriever.getUserTenants(eq("consortiumId"), anyString(), eq(requestContext))) + .thenReturn(Future.succeededFuture(List.of("tenant1", "tenant2"))); + + compositePoLineValidationService.validateUserUnaffiliatedLocations(updatedPoLineId, locationsUpdated, requestContext) + .onComplete(testContext.succeeding(result -> testContext.verify(testContext::completeNow))); + } - assertTrue(future.succeeded()); + @Test + void testValidateUserUnaffiliatedLocationsOneValidAndOneInvalidLocations(VertxTestContext testContext) { + var locationsUpdated = List.of( + new Location().withLocationId(UUID.randomUUID().toString()).withTenantId("tenant1"), + new Location().withLocationId(UUID.randomUUID().toString()).withTenantId("tenant2")); + var updatedPoLineId = UUID.randomUUID().toString(); + + when(consortiumConfigurationService.getConsortiumConfiguration(eq(requestContext))) + .thenReturn(Future.succeededFuture(Optional.of(new ConsortiumConfiguration("tenant1", "consortiumId")))); + when(consortiumUserTenantsRetriever.getUserTenants(eq("consortiumId"), anyString(), eq(requestContext))) + .thenReturn(Future.succeededFuture(List.of("tenant1"))); + + compositePoLineValidationService.validateUserUnaffiliatedLocations(updatedPoLineId, locationsUpdated, requestContext) + .onComplete(testContext.failing(cause -> testContext.verify(() -> { + assertInstanceOf(HttpException.class, cause); + assertTrue(cause.getMessage().contains(ErrorCodes.LOCATION_UPDATE_WITHOUT_AFFILIATION.getDescription())); + testContext.completeNow(); + }))); } @Test - void testValidateUserUnaffiliatedLocationUpdatesInvalid() { - var locationsStored = getLocationsForTenants(List.of("tenant1", "tenant2", "tenant3")); - var locationsUpdated = getLocationsForTenants(List.of("tenant1", "tenant3")); - locationsUpdated.get(1).withQuantityPhysical(10); - var storagePoLine = new PoLine().withLocations(locationsStored); - var updatedPoLine = new CompositePoLine().withLocations(locationsUpdated); - - doReturn(succeededFuture(Optional.of(new ConsortiumConfiguration("tenant1", "consortiumId")))) - .when(consortiumConfigurationService).getConsortiumConfiguration(any(RequestContext.class)); - doReturn(succeededFuture(List.of("tenant1"))) - .when(consortiumUserTenantsRetriever).getUserTenants(eq("consortiumId"), anyString(), any(RequestContext.class)); - - var future = compositePoLineValidationService.validateUserUnaffiliatedLocationUpdates(updatedPoLine.getId(), updatedPoLine.getLocations(), storagePoLine.getLocations(), requestContext); - - assertTrue(future.failed()); - assertInstanceOf(HttpException.class, future.cause()); + void testValidateUserUnaffiliatedLocationsTwoInvalidLocations(VertxTestContext testContext) { + var locationsUpdated = List.of( + new Location().withLocationId(UUID.randomUUID().toString()).withTenantId("tenant1"), + new Location().withLocationId(UUID.randomUUID().toString()).withTenantId("tenant2")); + var updatedPoLineId = UUID.randomUUID().toString(); + + when(consortiumConfigurationService.getConsortiumConfiguration(eq(requestContext))) + .thenReturn(Future.succeededFuture(Optional.of(new ConsortiumConfiguration("tenant1", "consortiumId")))); + when(consortiumUserTenantsRetriever.getUserTenants(eq("consortiumId"), anyString(), eq(requestContext))) + .thenReturn(Future.succeededFuture(List.of("tenant3"))); + + compositePoLineValidationService.validateUserUnaffiliatedLocations(updatedPoLineId, locationsUpdated, requestContext) + .onComplete(testContext.failing(cause -> testContext.verify(() -> { + assertInstanceOf(HttpException.class, cause); + assertTrue(cause.getMessage().contains(ErrorCodes.LOCATION_UPDATE_WITHOUT_AFFILIATION.getDescription())); + testContext.completeNow(); + }))); } }