[MODORDERS-1222] - Error when trying to Unopen an ongoing order to ad…

…d a purchase order line with multiple funds (#1063)

* [MODORDERS-1222] - Error when trying to Unopen an ongoing order to add a purchase order line with multiple funds

* Covered with test

src/main/java/org/folio/helper/PurchaseOrderHelper.java

@@ -213,8 +213,7 @@ public Future<Void> putCompositeOrderById(String orderId, boolean deleteHoldings
         }
         return null;
       }).compose(v -> updateOrder(compPO, deleteHoldings, requestContext))
-      .onSuccess(v -> logger.info("putCompositeOrderById :: Successfully updated order: {}",
-        JsonObject.mapFrom(compPO).encodePrettily()))
+      .onSuccess(v -> logger.info("putCompositeOrderById :: Successfully updated order: {}", compPO.getId()))
       .onFailure(t -> logger.error("putCompositeOrderById :: Failed to update order: {}",
         JsonObject.mapFrom(compPO).encodePrettily(), t));
   }

src/main/java/org/folio/service/finance/transaction/EncumbranceRelationsHoldersBuilder.java

@@ -1,15 +1,12 @@
 package org.folio.service.finance.transaction;
 
-import static java.util.stream.Collectors.groupingBy;
-import static java.util.stream.Collectors.mapping;
-import static java.util.stream.Collectors.toList;
-
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
 import java.util.stream.Collectors;
 
+import io.vertx.core.Future;
 import io.vertx.core.json.JsonObject;
 import org.folio.models.EncumbranceRelationsHolder;
 import org.folio.rest.acq.model.finance.Encumbrance;
@@ -28,15 +25,13 @@
 import org.folio.service.finance.LedgerService;
 import org.folio.service.finance.budget.BudgetService;
 
-import io.vertx.core.Future;
-
 public class EncumbranceRelationsHoldersBuilder extends FinanceHoldersBuilder {
 
   private final EncumbranceService encumbranceService;
 
   public EncumbranceRelationsHoldersBuilder(EncumbranceService encumbranceService, FundService fundService,
-      FiscalYearService fiscalYearService, ExchangeRateProviderResolver exchangeRateProviderResolver,
-      BudgetService budgetService, LedgerService ledgerService) {
+                                            FiscalYearService fiscalYearService, ExchangeRateProviderResolver exchangeRateProviderResolver,
+                                            BudgetService budgetService, LedgerService ledgerService) {
     super(fundService, fiscalYearService, exchangeRateProviderResolver, budgetService, ledgerService);
     this.encumbranceService = encumbranceService;
   }
@@ -76,7 +71,7 @@ private EncumbranceRelationsHolder buildBaseHolder(EncumbranceRelationsHolder ho
   }
 
   public Future<List<EncumbranceRelationsHolder>> withExistingTransactions(List<EncumbranceRelationsHolder> encumbranceHolders,
-      CompositePurchaseOrder poAndLinesFromStorage, RequestContext requestContext) {
+                                                                           CompositePurchaseOrder poAndLinesFromStorage, RequestContext requestContext) {
     if (poAndLinesFromStorage == null) {
       return Future.succeededFuture(encumbranceHolders);
     }
@@ -103,7 +98,7 @@ public void withKnownTransactions(List<EncumbranceRelationsHolder> encumbranceHo
   }
 
   private void withToBeReleasedHolders(List<EncumbranceRelationsHolder> encumbranceHolders,
-      List<Transaction> transactionsFromStorage) {
+                                       List<Transaction> transactionsFromStorage) {
 
     List<EncumbranceRelationsHolder> toBeReleasedHolders = transactionsFromStorage.stream()
       .filter(transaction -> encumbranceHolders.stream()
@@ -135,7 +130,7 @@ private Optional<Transaction> findBestMatch(EncumbranceRelationsHolder holder, L
   }
 
   private EncumbranceRelationsHolder buildToBeReleasedHolder(Transaction transaction,
-      List<EncumbranceRelationsHolder> encumbranceHolders) {
+                                                             List<EncumbranceRelationsHolder> encumbranceHolders) {
     // find the transaction reference in the fund distributions, to be able to remove it if the transaction is deleted
     FundDistribution fd = encumbranceHolders.stream()
       .filter(erh -> erh.getFundDistribution() != null &&
@@ -149,7 +144,7 @@ private EncumbranceRelationsHolder buildToBeReleasedHolder(Transaction transacti
   }
 
   private void mapHoldersToTransactions(List<EncumbranceRelationsHolder> encumbranceHolders,
-      List<Transaction> existingTransactions) {
+                                        List<Transaction> existingTransactions) {
     encumbranceHolders.forEach(holder -> findBestMatch(holder, existingTransactions)
       .ifPresent(existingTransaction -> {
         holder.withOldEncumbrance(existingTransaction);
@@ -167,23 +162,28 @@ private void mapHoldersToTransactions(List<EncumbranceRelationsHolder> encumbran
   }
 
   public Future<List<EncumbranceRelationsHolder>> prepareEncumbranceRelationsHolder(CompositePurchaseOrder compPO,
-      CompositePurchaseOrder poFromStorage, RequestContext requestContext) {
+                                                                                    CompositePurchaseOrder poFromStorage, RequestContext requestContext) {
     List<EncumbranceRelationsHolder> holders = buildBaseHolders(compPO);
     return withFinances(holders, requestContext)
       .compose(v -> withExistingTransactions(holders, poFromStorage, requestContext));
   }
 
   public Future<Map<String, List<CompositePoLine>>> retrieveMapFiscalYearsWithCompPOLines(CompositePurchaseOrder compPO,
-      CompositePurchaseOrder poAndLinesFromStorage, RequestContext requestContext) {
+                                                                                          CompositePurchaseOrder poAndLinesFromStorage,
+                                                                                          RequestContext requestContext) {
     return prepareEncumbranceRelationsHolder(compPO, poAndLinesFromStorage, requestContext)
-      .map(erhList ->
-        erhList
-          .stream()
-          .filter(erh -> Objects.nonNull(erh.getCurrentFiscalYearId()))
-          .collect(groupingBy(
-            EncumbranceRelationsHolder::getCurrentFiscalYearId,
-            mapping(EncumbranceRelationsHolder::getPoLine, toList())
-          ))
+      .map(erhList -> erhList.stream()
+        .filter(erh -> erh.getCurrentFiscalYearId() != null)
+        .collect(Collectors.groupingBy(
+          EncumbranceRelationsHolder::getCurrentFiscalYearId,
+          Collectors.mapping(
+            EncumbranceRelationsHolder::getPoLine,
+            Collectors.collectingAndThen(
+              Collectors.toList(),
+              list -> list.stream().distinct().toList()
+            )
+          )
+        ))
       );
   }
 

src/test/java/org/folio/service/finance/transaction/EncumbranceRelationsHoldersBuilderTest.java

@@ -1,5 +1,6 @@
 package org.folio.service.finance.transaction;
 
+import static io.vertx.core.Future.succeededFuture;
 import static java.util.Collections.emptyList;
 import static java.util.Collections.singletonList;
 import static org.folio.rest.acq.model.finance.Encumbrance.OrderStatus.OPEN;
@@ -11,21 +12,26 @@
 import static org.hamcrest.Matchers.empty;
 import static org.hamcrest.Matchers.everyItem;
 import static org.hamcrest.Matchers.hasItem;
+import static org.hamcrest.Matchers.hasItems;
 import static org.hamcrest.Matchers.hasProperty;
 import static org.hamcrest.Matchers.hasSize;
 import static org.hamcrest.Matchers.nullValue;
 import static org.hamcrest.core.Is.is;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyList;
+import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.when;
 
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
 import java.util.UUID;
 
+import io.vertx.junit5.VertxExtension;
+import io.vertx.junit5.VertxTestContext;
 import org.folio.models.EncumbranceRelationsHolder;
 import org.folio.rest.acq.model.finance.Encumbrance;
 import org.folio.rest.acq.model.finance.Metadata;
@@ -43,13 +49,12 @@
 import org.mockito.InjectMocks;
 import org.mockito.Mock;
 import org.mockito.MockitoAnnotations;
-
-import io.vertx.core.Future;
-import io.vertx.junit5.VertxExtension;
+import org.mockito.Spy;
 
 @ExtendWith(VertxExtension.class)
 public class EncumbranceRelationsHoldersBuilderTest {
 
+  @Spy
   @InjectMocks
   private EncumbranceRelationsHoldersBuilder encumbranceRelationsHoldersBuilder;
 
@@ -252,7 +257,7 @@ void testShouldPopulateHoldersWithOldEncumbranceIfMatchesTransactionFromStorage(
     holders.add(holder2);
 
     when(encumbranceService.getEncumbrancesByIds(anyList(), any()))
-      .thenReturn(Future.succeededFuture(singletonList(encumbranceFromStorage)));
+      .thenReturn(succeededFuture(singletonList(encumbranceFromStorage)));
     //When
     List<EncumbranceRelationsHolder> resultHolders = encumbranceRelationsHoldersBuilder
         .withExistingTransactions(holders, order, requestContextMock).result();
@@ -320,7 +325,7 @@ void testShouldCreateNewHoldersForTransactionsFromStorageThatNotMatchProvidedHol
     holders.add(holder3);
 
     when(encumbranceService.getEncumbrancesByIds(anyList(), any()))
-      .thenReturn(Future.succeededFuture(List.of(encumbranceFromStorage1, encumbranceFromStorage2)));
+      .thenReturn(succeededFuture(List.of(encumbranceFromStorage1, encumbranceFromStorage2)));
 
     //When
     List<EncumbranceRelationsHolder> resultHolders = encumbranceRelationsHoldersBuilder
@@ -335,4 +340,54 @@ void testShouldCreateNewHoldersForTransactionsFromStorageThatNotMatchProvidedHol
     assertThat(resultHolders, hasItem( hasProperty("oldEncumbrance", is(encumbranceFromStorage2))));
   }
 
+  @Test
+  void testRetrieveMapFiscalYearsWithCompPOLines(VertxTestContext vertxTestContext) {
+    String fiscalYearId1 = UUID.randomUUID().toString();
+    String fiscalYearId2 = UUID.randomUUID().toString();
+
+    holder1.withCurrentFiscalYearId(fiscalYearId1);
+    holder2.withCurrentFiscalYearId(fiscalYearId2);
+    holder3.withCurrentFiscalYearId(fiscalYearId1);
+
+    // Make holder1  and holder3 have the same poLine as holder1
+    holder1.withPoLine(line1);
+    holder3.withPoLine(line1);
+
+    List<EncumbranceRelationsHolder> holders = List.of(holder1, holder2, holder3); // Removed holder4
+
+    doReturn(succeededFuture(holders))
+      .when(encumbranceRelationsHoldersBuilder).prepareEncumbranceRelationsHolder(any(), any(), any());
+
+    var future = encumbranceRelationsHoldersBuilder.retrieveMapFiscalYearsWithCompPOLines(order, order, requestContextMock);
+    vertxTestContext.assertComplete(future)
+      .onSuccess(result -> {
+        assertThat(result.keySet(), hasSize(2));
+        assertThat(result.keySet(), hasItems(fiscalYearId1, fiscalYearId2));
+
+        // Check if the list for fiscalYearId1 shouldn't contain duplicates of line1
+        assertThat(result.get(fiscalYearId1), hasSize(1));
+        assertThat(result.get(fiscalYearId1), hasItems(line1));
+
+        assertThat(result.get(fiscalYearId2), hasSize(1));
+        assertThat(result.get(fiscalYearId2), hasItem(line2));
+
+        vertxTestContext.completeNow();
+      });
+  }
+
+  @Test
+  void testRetrieveMapFiscalYearsWithCompPOLinesEmpty(VertxTestContext vertxTestContext) {
+    List<EncumbranceRelationsHolder> holders = List.of(holder1, holder2, holder3);
+
+    doReturn(succeededFuture(holders))
+      .when(encumbranceRelationsHoldersBuilder).prepareEncumbranceRelationsHolder(any(), any(), any());
+
+    var future = encumbranceRelationsHoldersBuilder.retrieveMapFiscalYearsWithCompPOLines(order, order, requestContextMock);
+    vertxTestContext.assertComplete(future)
+      .onSuccess(result -> {
+        //Then
+        assertTrue(result.isEmpty());
+        vertxTestContext.completeNow();
+      });
+  }
 }