Skip to content

[MODORDERS-1311] Prevent unopening of order when related piece has a request #1252

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.

Sign up for GitHub

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

Merged
damien-git merged 2 commits into from
Mar 3, 2026
+101 −26
Merged

[MODORDERS-1311] Prevent unopening of order when related piece has a request #1252

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b999935
[MODORDERS-1311] Prevent unopening of order when related piece has a …
damien-git Mar 2, 2026
e660b9b
[MODORDERS-1311] Removed unused parameter
damien-git Mar 2, 2026
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
9 changes: 5 additions & 4 deletions src/main/java/org/folio/config/ApplicationConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -746,14 +746,15 @@ PurchaseOrderHelper purchaseOrderHelper(PurchaseOrderLineHelper purchaseOrderLin
ProtectionService protectionService, InventoryItemStatusSyncService itemStatusSyncService,
OpenCompositeOrderManager openCompositeOrderManager, PurchaseOrderStorageService purchaseOrderStorageService,
CommonSettingsCache commonSettingsCache, PoNumberHelper poNumberHelper,
OpenCompositeOrderFlowValidator openCompositeOrderFlowValidator, ReOpenCompositeOrderManager reOpenCompositeOrderManager,
OrderValidationService orderValidationService, PoLineValidationService poLineValidationService) {
ReOpenCompositeOrderManager reOpenCompositeOrderManager,
OrderValidationService orderValidationService, PoLineValidationService poLineValidationService,
UnOpenCompositeOrderManager unOpenCompositeOrderManager) {
return new PurchaseOrderHelper(purchaseOrderLineHelper, orderLinesSummaryPopulateService, encumbranceService,
combinedPopulateService, encumbranceWorkflowStrategyFactory, orderInvoiceRelationService, tagService,
purchaseOrderLineService, titlesService, protectionService, itemStatusSyncService,
openCompositeOrderManager, purchaseOrderStorageService, commonSettingsCache,
poNumberHelper, openCompositeOrderFlowValidator, reOpenCompositeOrderManager,
orderValidationService, poLineValidationService);
poNumberHelper, reOpenCompositeOrderManager,
orderValidationService, poLineValidationService, unOpenCompositeOrderManager);
}

@Bean
Expand Down
18 changes: 12 additions & 6 deletions src/main/java/org/folio/helper/PurchaseOrderHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import static org.folio.orders.utils.OrderStatusTransitionUtil.isOrderReopening;
import static org.folio.orders.utils.OrderStatusTransitionUtil.isTransitionToClosed;
import static org.folio.orders.utils.OrderStatusTransitionUtil.isTransitionToOpen;
import static org.folio.orders.utils.OrderStatusTransitionUtil.isTransitionToPending;
import static org.folio.orders.utils.OrderStatusTransitionUtil.isTransitionToReopen;
import static org.folio.orders.utils.POProtectedFields.getFieldNames;
import static org.folio.orders.utils.PermissionsUtil.*;
Expand Down Expand Up @@ -67,9 +68,9 @@
import org.folio.service.orders.OrderWorkflowType;
import org.folio.service.orders.PurchaseOrderLineService;
import org.folio.service.orders.PurchaseOrderStorageService;
import org.folio.service.orders.flows.update.open.OpenCompositeOrderFlowValidator;
import org.folio.service.orders.flows.update.open.OpenCompositeOrderManager;
import org.folio.service.orders.flows.update.reopen.ReOpenCompositeOrderManager;
import org.folio.service.orders.flows.update.unopen.UnOpenCompositeOrderManager;
import org.folio.service.titles.TitlesService;

import io.vertx.core.Future;
Expand All @@ -92,13 +93,13 @@ public class PurchaseOrderHelper {
private final ProtectionService protectionService;
private final InventoryItemStatusSyncService itemStatusSyncService;
private final OpenCompositeOrderManager openCompositeOrderManager;
private final OpenCompositeOrderFlowValidator openCompositeOrderFlowValidator;
private final PurchaseOrderStorageService purchaseOrderStorageService;
private final CommonSettingsCache commonSettingsCache;
private final PoNumberHelper poNumberHelper;
private final ReOpenCompositeOrderManager reOpenCompositeOrderManager;
private final OrderValidationService orderValidationService;
private final PoLineValidationService poLineValidationService;
private final UnOpenCompositeOrderManager unOpenCompositeOrderManager;

public PurchaseOrderHelper(PurchaseOrderLineHelper purchaseOrderLineHelper,
CompositeOrderDynamicDataPopulateService orderLinesSummaryPopulateService, EncumbranceService encumbranceService,
Expand All @@ -109,9 +110,8 @@ public PurchaseOrderHelper(PurchaseOrderLineHelper purchaseOrderLineHelper,
ProtectionService protectionService, InventoryItemStatusSyncService itemStatusSyncService,
OpenCompositeOrderManager openCompositeOrderManager, PurchaseOrderStorageService purchaseOrderStorageService,
CommonSettingsCache commonSettingsCache, PoNumberHelper poNumberHelper,
OpenCompositeOrderFlowValidator openCompositeOrderFlowValidator,
ReOpenCompositeOrderManager reOpenCompositeOrderManager, OrderValidationService orderValidationService,
PoLineValidationService poLineValidationService) {
PoLineValidationService poLineValidationService, UnOpenCompositeOrderManager unOpenCompositeOrderManager) {
this.purchaseOrderLineHelper = purchaseOrderLineHelper;
this.orderLinesSummaryPopulateService = orderLinesSummaryPopulateService;
this.encumbranceService = encumbranceService;
Expand All @@ -127,10 +127,10 @@ public PurchaseOrderHelper(PurchaseOrderLineHelper purchaseOrderLineHelper,
this.purchaseOrderStorageService = purchaseOrderStorageService;
this.commonSettingsCache = commonSettingsCache;
this.poNumberHelper = poNumberHelper;
this.openCompositeOrderFlowValidator = openCompositeOrderFlowValidator;
this.reOpenCompositeOrderManager = reOpenCompositeOrderManager;
this.orderValidationService = orderValidationService;
this.poLineValidationService = poLineValidationService;
this.unOpenCompositeOrderManager = unOpenCompositeOrderManager;
}

/**
Expand Down Expand Up @@ -226,7 +226,13 @@ public Future<Void> updateOrder(CompositePurchaseOrder compPO, boolean deleteHol
CompositePurchaseOrder clonedPoFromStorage = JsonObject.mapFrom(poFromStorage).mapTo(CompositePurchaseOrder.class);
boolean isTransitionToOpen = isTransitionToOpen(poFromStorage, compPO);
return validateUserUnaffiliatedPoLineLocations(clonedPoFromStorage.getPoLines(), requestContext)
.compose(v -> orderValidationService.validateOrderForUpdate(compPO, poFromStorage, deleteHoldings, requestContext))
.compose(v -> orderValidationService.validateOrderForUpdate(compPO, poFromStorage, requestContext))
.compose(ok -> {
if (isTransitionToPending(poFromStorage, compPO)) {
return unOpenCompositeOrderManager.process(compPO, poFromStorage, deleteHoldings, requestContext);
}
return Future.succeededFuture();
})
.compose(v -> {
if (isTransitionToOpen && CollectionUtils.isEmpty(compPO.getPoLines())) {
compPO.setPoLines(clonedPoFromStorage.getPoLines());
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/org/folio/service/orders/OrderValidationService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ public Future<List<Error>> validateOrderForPut(String orderId, CompositePurchase
* This validation is used for both the PUT endpoint and data import.
*/
public Future<Void> validateOrderForUpdate(CompositePurchaseOrder compPO, CompositePurchaseOrder poFromStorage,
boolean deleteHoldings, RequestContext requestContext) {
RequestContext requestContext) {
logger.info("validateOrderForUpdate :: orderId: {}", compPO.getId());
return validateAcqUnitsOnUpdate(compPO, poFromStorage, requestContext)
.compose(ok -> prefixService.validatePrefixAvailability(compPO.getPoNumberPrefix(), requestContext))
Expand All @@ -196,7 +196,7 @@ public Future<Void> validateOrderForUpdate(CompositePurchaseOrder compPO, Compos
.compose(ok -> {
if (isTransitionToPending(poFromStorage, compPO)) {
checkOrderUnopenPermissions(requestContext);
return unOpenCompositeOrderManager.process(compPO, poFromStorage, deleteHoldings, requestContext);
return unOpenCompositeOrderManager.checkRequests(compPO, requestContext);
}
return Future.succeededFuture();
});
Expand Down
54 changes: 46 additions & 8 deletions src/main/java/org/folio/service/orders/flows/update/unopen/UnOpenCompositeOrderManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import java.util.ArrayList;
import java.util.Collection;
import java.util.EnumSet;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;
Expand Down Expand Up @@ -99,6 +100,48 @@ public Future<Void> process(CompositePurchaseOrder compPO, CompositePurchaseOrde

}

public Future<Void> rollbackInventory(CompositePurchaseOrder compPO, RequestContext requestContext) {
return processPoLines(compPO.getPoLines(), poLine -> processInventory(poLine, true, requestContext));
}

public Future<Void> checkRequests(CompositePurchaseOrder compPO, RequestContext requestContext) {
Copy link
Contributor

@SerhiiNosko SerhiiNosko Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

karate test can be written for non-ecs setup, for standalone tenants.
But in future we should improve running karate tests locally for the ECS, we have stories for it.

List<PoLine> poLines = compPO.getPoLines().stream()
.filter(poLine -> !Boolean.TRUE.equals(poLine.getIsPackage()))
.toList();
HashMap<String, List<String>> tenantIdToPoLineIds = createTenantIdToPoLineIdsMap(poLines);
List<Future<Void>> futures = tenantIdToPoLineIds.keySet().stream()
.map(tenantId -> {
List<String> poLineIds = tenantIdToPoLineIds.get(tenantId);
RequestContext tenantRequestContext = createContextWithNewTenantId(requestContext, tenantId);
return inventoryItemManager.getItemsByPoLineIdsAndStatus(poLineIds, ItemStatus.ON_ORDER.value(), tenantRequestContext)
.compose(onOrderItems -> {
if (CollectionUtils.isEmpty(onOrderItems)) {
return Future.succeededFuture();
}
List<String> itemIds = onOrderItems.stream().map(item -> item.getString(ID)).toList();
return checkRequestsForItems(itemIds, tenantRequestContext);
});
})
.toList();
return HelperUtils.executeAllFailFast(futures);
}

private HashMap<String, List<String>> createTenantIdToPoLineIdsMap(List<PoLine> poLines) {
HashMap<String, List<String>> tenantIdToPoLineIds = new HashMap<>();
poLines.forEach(poLine -> {
List<String> tenantIds = PoLineCommonUtil.getTenantsFromLocations(poLine);
tenantIds.forEach(tenantId -> {
Copy link
Contributor

@SerhiiNosko SerhiiNosko Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will this logic check requests from standalone non ecs tenants like diku? Location object will not have standalone tenants, so we should check them from request headers

Copy link
Contributor Author

@damien-git damien-git Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, with a null tenantId.

List<String> poLineIds = tenantIdToPoLineIds.get(tenantId);
if (poLineIds == null) {
poLineIds = new ArrayList<>();
}
poLineIds.add(poLine.getId());
tenantIdToPoLineIds.put(tenantId, poLineIds);
});
});
return tenantIdToPoLineIds;
}

private Future<CompositePurchaseOrder> updateAndGetOrderWithLines(CompositePurchaseOrder compPO, RequestContext requestContext) {
if (CollectionUtils.isNotEmpty(compPO.getPoLines())) {
return Future.succeededFuture(compPO);
Expand All @@ -121,10 +164,6 @@ private Future<Void> updatePoLinesSummary(List<PoLine> poLines, RequestContext r
return processPoLines(poLines, poLine -> purchaseOrderLineService.saveOrderLine(poLine, requestContext));
}

public Future<Void> rollbackInventory(CompositePurchaseOrder compPO, RequestContext requestContext) {
return processPoLines(compPO.getPoLines(), poLine -> processInventory(poLine, true, requestContext));
}

private Future<Void> processInventory(List<PoLine> poLines, boolean deleteHoldings, RequestContext requestContext) {
return processPoLines(poLines, poLine -> processInventory(poLine, deleteHoldings, requestContext));
}
Expand Down Expand Up @@ -366,7 +405,6 @@ private Future<Void> deletePieceWithItem(String pieceId, RequestContext requestC
.compose(poLine -> purchaseOrderStorageService.getPurchaseOrderById(poLine.getPurchaseOrderId(), requestContext)
.map(purchaseOrder -> holder.withOrderInformation(purchaseOrder, poLine)))
.compose(aHolder -> protectionService.isOperationRestricted(holder.getOriginPurchaseOrder().getAcqUnitIds(), DELETE, requestContext))
.compose(vVoid -> canDeletePieceWithItem(holder.getPieceToDelete(), requestContext))
.compose(aVoid -> pieceStorageService.deletePiece(pieceId, requestContext))
.compose(aVoid -> deletePieceConnectedItem(holder.getPieceToDelete(), itemContext));
}
Expand All @@ -390,9 +428,9 @@ private Future<Void> deletePieceConnectedItem(Piece piece, RequestContext reques
});
}

private Future<Void> canDeletePieceWithItem(Piece piece, RequestContext requestContext) {
return circulationRequestsRetriever.getNumberOfRequestsByItemId(piece.getItemId(), requestContext)
.compose(numOfRequests -> numOfRequests != null && numOfRequests > 0
private Future<Void> checkRequestsForItems(List<String> itemIds, RequestContext requestContext) {
return circulationRequestsRetriever.getNumbersOfRequestsByItemIds(itemIds, requestContext)
.compose(idsAndCounts -> idsAndCounts.values().stream().anyMatch(count -> count > 0)
? Future.failedFuture(new HttpException(422, ErrorCodes.REQUEST_FOUND.toError()))
: Future.succeededFuture());
}
Expand Down
5 changes: 5 additions & 0 deletions src/test/java/org/folio/ApiTestSuite.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@
import org.folio.service.orders.flows.update.open.OpenCompositeOrderManagerTest;
import org.folio.service.orders.flows.update.open.OpenCompositeOrderPieceServiceTest;
import org.folio.service.orders.flows.update.reopen.ReOpenCompositeOrderManagerTest;
import org.folio.service.orders.flows.update.unopen.UnOpenCompositeOrderManagerTest;
import org.folio.service.orders.lines.update.OrderLineUpdateInstanceHandlerTest;
import org.folio.service.orders.lines.update.instance.WithHoldingOrderLineUpdateInstanceStrategyTest;
import org.folio.service.orders.lines.update.instance.WithoutHoldingOrderLineUpdateInstanceStrategyTest;
Expand Down Expand Up @@ -497,6 +498,10 @@ class InvoiceLineServiceTestNested extends InvoiceLineServiceTest {
class ReOpenCompositeOrderManagerTestNested extends ReOpenCompositeOrderManagerTest {
}

@Nested
class UnOpenCompositeOrderManagerTestNested extends UnOpenCompositeOrderManagerTest {
}

@Nested
class OrderLineUpdateInstanceHandlerTestNested extends OrderLineUpdateInstanceHandlerTest {
}
Expand Down
4 changes: 2 additions & 2 deletions src/test/java/org/folio/helper/PurchaseOrderHelperTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ void testPutPendingCompositeOrder() throws IOException {
.when(purchaseOrderLineService).populateOrderLines(any(CompositePurchaseOrder.class), eq(requestContext));
doReturn(succeededFuture(null))
.when(orderValidationService).validateOrderForUpdate(any(CompositePurchaseOrder.class), any(CompositePurchaseOrder.class),
eq(deleteHoldings), eq(requestContext));
eq(requestContext));
doReturn(succeededFuture(null))
.when(purchaseOrderLineHelper).updatePoLines(any(CompositePurchaseOrder.class), any(CompositePurchaseOrder.class),
eq(requestContext));
Expand Down Expand Up @@ -244,7 +244,7 @@ void testPutUnOpenOrderValidationThrow() throws IOException {
// Then
assertTrue(future.failed());

verify(orderValidationService, times(0)).validateOrderForUpdate(any(), any(), anyBoolean(), any());
verify(orderValidationService, times(0)).validateOrderForUpdate(any(), any(), any());
}

@Test
Expand Down
33 changes: 29 additions & 4 deletions ...st/java/org/folio/service/orders/flows/update/unopen/UnOpenCompositeOrderManagerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import static org.folio.TestUtils.getMockAsJson;
import static org.folio.rest.impl.MockServer.BASE_MOCK_DATA_PATH;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.Mockito.any;
import static org.mockito.Mockito.anyBoolean;
import static org.mockito.Mockito.anyList;
Expand All @@ -30,11 +31,13 @@
import java.util.concurrent.ExecutionException;
import java.util.concurrent.TimeoutException;

import io.vertx.core.Future;
import io.vertx.core.json.JsonObject;
import io.vertx.junit5.VertxExtension;
import org.folio.ApiTestSuite;
import org.folio.models.ItemStatus;
import org.folio.orders.utils.ProtectedOperationType;
import org.folio.rest.core.exceptions.HttpException;
import org.folio.rest.core.models.RequestContext;
import org.folio.rest.jaxrs.model.PoLine;
import org.folio.rest.jaxrs.model.CompositePurchaseOrder;
Expand Down Expand Up @@ -99,6 +102,7 @@ public class UnOpenCompositeOrderManagerTest {
private OpenToPendingEncumbranceStrategy openToPendingEncumbranceStrategy;
@Mock
private Map<String, String> okapiHeadersMock;
private AutoCloseable mockitoMocks;
private Context ctxMock;
private RequestContext requestContext;

Expand All @@ -123,15 +127,16 @@ public static void after() {

@BeforeEach
void beforeEach() {
MockitoAnnotations.openMocks(this);
mockitoMocks = MockitoAnnotations.openMocks(this);
autowireDependencies(this);
requestContext = new RequestContext(ctxMock, okapiHeadersMock);
}

@AfterEach
void resetMocks() {
void resetMocks() throws Exception {
clearServiceInteractions();
reset(encumbranceWorkflowStrategyFactory, pieceStorageService, inventoryItemManager, inventoryHoldingManager);
mockitoMocks.close();
}

@Test
Expand Down Expand Up @@ -365,7 +370,7 @@ void testDeleteHoldingForIndependentWorkflowWhenDeleteHoldingIsFalse() {
verify(inventoryItemManager, never()).deleteItems(anyList(), anyBoolean(), any(RequestContext.class));
}

private void prepareInitialSetup(CompositePurchaseOrder order, CompositePurchaseOrder orderFromStorage, PoLine poLine) {
private void prepareInitialSetup(CompositePurchaseOrder order, CompositePurchaseOrder orderFromStorage, PoLine poLine, long numberOfRequests) {
doReturn(openToPendingEncumbranceStrategy).when(encumbranceWorkflowStrategyFactory).getStrategy(eq(OrderWorkflowType.OPEN_TO_PENDING));
doReturn(succeededFuture(null)).when(openToPendingEncumbranceStrategy).processEncumbrances(eq(order), eq(orderFromStorage), any());
JsonObject item = getItem();
Expand All @@ -383,7 +388,8 @@ private void prepareInitialSetup(CompositePurchaseOrder order, CompositePurchase
PurchaseOrder simpleOrder = new PurchaseOrder().withId(order.getId());
doReturn(succeededFuture(simpleOrder)).when(purchaseOrderStorageService).getPurchaseOrderById(order.getId(), requestContext);
doReturn(succeededFuture()).when(protectionService).isOperationRestricted(anyList(), any(ProtectedOperationType.class), any());
doReturn(succeededFuture(0)).when(circulationRequestsRetriever).getNumberOfRequestsByItemId(ITEM_ID, requestContext);
doReturn(succeededFuture(Map.of(ITEM_ID, numberOfRequests)))
.when(circulationRequestsRetriever).getNumbersOfRequestsByItemIds(List.of(ITEM_ID), requestContext);
doReturn(succeededFuture()).when(pieceStorageService).deletePiece(PIECE_ID, requestContext);
doReturn(succeededFuture()).when(inventoryItemManager).deleteItem(piece.getItemId(), true, requestContext);
doReturn(succeededFuture(Collections.emptyList())).when(inventoryItemManager).getItemsByHoldingId(eq(HOLDING_ID), RequestContextMatcher.matchCentralTenant());
Expand All @@ -394,6 +400,10 @@ private void prepareInitialSetup(CompositePurchaseOrder order, CompositePurchase
doReturn(succeededFuture(Collections.emptyList())).when(pieceStorageService).getPiecesByHoldingIds(anyList(), any(RequestContext.class));
}

private void prepareInitialSetup(CompositePurchaseOrder order, CompositePurchaseOrder orderFromStorage, PoLine poLine) {
prepareInitialSetup(order, orderFromStorage, poLine, 0);
}

private JsonObject getItem() {
return new JsonObject()
.put("id", ITEM_ID)
Expand Down Expand Up @@ -510,6 +520,21 @@ void testRollbackInventory_NoHoldingsMapEntry_ShouldSucceed() throws Exception {
verify(inventoryHoldingManager, never()).deleteHoldingById(anyString(), anyBoolean(), any(RequestContext.class));
}

@Test
void testErrorWhenItemHasRequest() {
//given
CompositePurchaseOrder order = getMockAsJson(ORDER_PATH).mapTo(CompositePurchaseOrder.class);
CompositePurchaseOrder orderFromStorage = getMockAsJson(ORDER_PATH).mapTo(CompositePurchaseOrder.class);
PoLine poLine = getPoLine(order);
prepareInitialSetup(order, orderFromStorage, poLine, 1);
//When
Future<Void> future = unOpenCompositeOrderManager.checkRequests(order, requestContext);
//Then
assertTrue(future.failed());
HttpException exception = (HttpException) future.cause();
assertEquals(422, exception.getCode());
}

/**
* Define unit test specific beans to override actual ones
*/
Expand Down