From 393cb0d26690bf6d2f1f81a7e37602f2cf25991e Mon Sep 17 00:00:00 2001 From: Roman_Barannyk Date: Mon, 15 Apr 2024 11:50:53 +0300 Subject: [PATCH] Revert "CIRC-2050 Add useStubItem parameter to allowed-service-points endpoint (#1447)" This reverts commit e11290cbb2bb8ebb62636513d740368d5e31b03b. --- descriptors/ModuleDescriptor-template.json | 2 +- ramls/circulation.raml | 6 +- .../domain/AllowedServicePointsRequest.java | 3 +- .../requests/RequestPolicyRepository.java | 10 -- .../AllowedServicePointsResource.java | 53 ++++++--- .../services/AllowedServicePointsService.java | 7 -- .../AllowedServicePointsAPITests.java | 106 ++++-------------- 7 files changed, 59 insertions(+), 128 deletions(-) diff --git a/descriptors/ModuleDescriptor-template.json b/descriptors/ModuleDescriptor-template.json index 8a02b239f2..7c3518710b 100644 --- a/descriptors/ModuleDescriptor-template.json +++ b/descriptors/ModuleDescriptor-template.json @@ -649,7 +649,7 @@ }, { "id": "allowed-service-points", - "version": "1.1", + "version": "1.0", "handlers": [ { "methods": [ diff --git a/ramls/circulation.raml b/ramls/circulation.raml index dca970e8e4..ff2081a21b 100644 --- a/ramls/circulation.raml +++ b/ramls/circulation.raml @@ -343,10 +343,6 @@ resourceTypes: description: "Instance ID" pattern: "^[a-fA-F0-9]{8}-[a-fA-F0-9]{4}-[1-5][a-fA-F0-9]{3}-[89abAB][a-fA-F0-9]{3}-[a-fA-F0-9]{12}$" required: false - useStubItem: - description: "When true, allows to apply circulation rules based on patron group only" - type: boolean - required: false responses: 200: description: "List of allowed service points was retrieved successfully" @@ -368,4 +364,4 @@ resourceTypes: description: "Internal server error" body: text/plain: - example: "Internal server error" + example: "Internal server error" \ No newline at end of file diff --git a/src/main/java/org/folio/circulation/domain/AllowedServicePointsRequest.java b/src/main/java/org/folio/circulation/domain/AllowedServicePointsRequest.java index 1f2a13854c..1e4bc603ef 100644 --- a/src/main/java/org/folio/circulation/domain/AllowedServicePointsRequest.java +++ b/src/main/java/org/folio/circulation/domain/AllowedServicePointsRequest.java @@ -19,8 +19,6 @@ public class AllowedServicePointsRequest { private String requesterId; private String instanceId; private String itemId; - private String requestId; - private boolean useStubItem; public boolean isForTitleLevelRequest() { return instanceId != null; @@ -29,6 +27,7 @@ public boolean isForTitleLevelRequest() { public boolean isForItemLevelRequest() { return itemId != null; } + private String requestId; public AllowedServicePointsRequest updateWithRequestInformation(Request request) { log.debug("updateWithRequestInformation:: parameters request: {}", request); diff --git a/src/main/java/org/folio/circulation/infrastructure/storage/requests/RequestPolicyRepository.java b/src/main/java/org/folio/circulation/infrastructure/storage/requests/RequestPolicyRepository.java index 7df5c56de1..60f35c780d 100644 --- a/src/main/java/org/folio/circulation/infrastructure/storage/requests/RequestPolicyRepository.java +++ b/src/main/java/org/folio/circulation/infrastructure/storage/requests/RequestPolicyRepository.java @@ -15,7 +15,6 @@ import java.util.Collection; import java.util.Map; import java.util.Set; -import java.util.UUID; import java.util.concurrent.CompletableFuture; import java.util.function.BinaryOperator; import java.util.stream.Collectors; @@ -96,15 +95,6 @@ public CompletableFuture>>> lookupRequestPol .thenCompose(r -> r.after(this::lookupRequestPolicies)); } - public CompletableFuture> lookupRequestPolicy(User user) { - // Circulation rules need to be executed with the patron group parameter only. - // All the item-related parameters should be random UUIDs. - return lookupRequestPolicyId(UUID.randomUUID().toString(), user.getPatronGroupId(), - UUID.randomUUID().toString(), UUID.randomUUID().toString()) - .thenCompose(r -> r.after(this::lookupRequestPolicy)) - .thenApply(result -> result.map(RequestPolicy::from)); - } - private BinaryOperator> itemsMergeOperator() { return (items1, items2) -> Stream.concat(items1.stream(), items2.stream()) .collect(Collectors.toSet()); diff --git a/src/main/java/org/folio/circulation/resources/AllowedServicePointsResource.java b/src/main/java/org/folio/circulation/resources/AllowedServicePointsResource.java index 4b7c9e9f2d..8e7a81d014 100644 --- a/src/main/java/org/folio/circulation/resources/AllowedServicePointsResource.java +++ b/src/main/java/org/folio/circulation/resources/AllowedServicePointsResource.java @@ -67,39 +67,52 @@ private static Result buildRequest(RoutingContext r .map(String::toUpperCase) .map(Request.Operation::valueOf) .orElse(null); - String requesterId = queryParams.get("requesterId"); - String instanceId = queryParams.get("instanceId"); - String itemId = queryParams.get("itemId"); - String requestId = queryParams.get("requestId"); - String useStubItem = queryParams.get("useStubItem"); + + AllowedServicePointsRequest request = new AllowedServicePointsRequest(operation, + queryParams.get("requesterId"), queryParams.get("instanceId"), queryParams.get("itemId"), + queryParams.get("requestId")); + + return validateAllowedServicePointsRequest(request); + } + + private static Result validateAllowedServicePointsRequest( + AllowedServicePointsRequest allowedServicePointsRequest) { + + log.debug("validateAllowedServicePointsRequest:: parameters allowedServicePointsRequest: {}", + allowedServicePointsRequest); + + Request.Operation operation = allowedServicePointsRequest.getOperation(); + String requesterId = allowedServicePointsRequest.getRequesterId(); + String instanceId = allowedServicePointsRequest.getInstanceId(); + String itemId = allowedServicePointsRequest.getItemId(); + String requestId = allowedServicePointsRequest.getRequestId(); List errors = new ArrayList<>(); // Checking UUID validity + if (requesterId != null && !isUuid(requesterId)) { - log.warn("validateAllowedServicePointsRequest:: requester ID is not a valid UUID: {}", requesterId); + log.warn("Requester ID is not a valid UUID: {}", requesterId); errors.add(String.format("Requester ID is not a valid UUID: %s.", requesterId)); } + if (instanceId != null && !isUuid(instanceId)) { - log.warn("validateAllowedServicePointsRequest:: instance ID is not a valid UUID: {}", - requesterId); + log.warn("Instance ID is not a valid UUID: {}", requesterId); errors.add(String.format("Instance ID is not a valid UUID: %s.", instanceId)); } + if (itemId != null && !isUuid(itemId)) { - log.warn("validateAllowedServicePointsRequest:: item ID is not a valid UUID: {}", itemId); + log.warn("Item ID is not a valid UUID: {}", itemId); errors.add(String.format("Item ID is not a valid UUID: %s.", itemId)); } + if (requestId != null && !isUuid(requestId)) { - log.warn("validateAllowedServicePointsRequest:: request ID is not a valid UUID: {}", - requestId); + log.warn("Request ID is not a valid UUID: {}", requestId); errors.add(String.format("Request ID is not a valid UUID: %s.", requestId)); } - if (useStubItem != null && !"true".equals(useStubItem) && !"false".equals(useStubItem)) { - log.warn("validateAllowedServicePointsRequest:: useStubItem is not a valid boolean: {}", - useStubItem); - errors.add(String.format("useStubItem is not a valid boolean: %s.", useStubItem)); - } + // Checking parameter combinations + boolean allowedCombinationOfParametersDetected = false; if (operation == Request.Operation.CREATE && requesterId != null && instanceId != null && @@ -108,36 +121,40 @@ private static Result buildRequest(RoutingContext r log.info("validateAllowedServicePointsRequest:: TLR request creation case"); allowedCombinationOfParametersDetected = true; } + if (operation == Request.Operation.CREATE && requesterId != null && instanceId == null && itemId != null && requestId == null) { log.info("validateAllowedServicePointsRequest:: ILR request creation case"); allowedCombinationOfParametersDetected = true; } + if (operation == Request.Operation.REPLACE && requesterId == null && instanceId == null && itemId == null && requestId != null) { log.info("validateAllowedServicePointsRequest:: request replacement case"); allowedCombinationOfParametersDetected = true; } + if (operation == Request.Operation.MOVE && requesterId == null && instanceId == null && itemId != null && requestId != null) { log.info("validateAllowedServicePointsRequest:: request movement case"); allowedCombinationOfParametersDetected = true; } + if (!allowedCombinationOfParametersDetected) { String errorMessage = "Invalid combination of query parameters"; errors.add(errorMessage); } + if (!errors.isEmpty()) { String errorMessage = String.join(" ", errors); log.error("validateRequest:: allowed service points request failed: {}", errorMessage); return failed(new BadRequestFailure(errorMessage)); } - return succeeded(new AllowedServicePointsRequest(operation, requesterId, instanceId, itemId, - requestId, Boolean.parseBoolean(useStubItem))); + return succeeded(allowedServicePointsRequest); } private static JsonObject toJson(Map> allowedServicePoints) { diff --git a/src/main/java/org/folio/circulation/services/AllowedServicePointsService.java b/src/main/java/org/folio/circulation/services/AllowedServicePointsService.java index 74140f217a..45fff76324 100644 --- a/src/main/java/org/folio/circulation/services/AllowedServicePointsService.java +++ b/src/main/java/org/folio/circulation/services/AllowedServicePointsService.java @@ -20,7 +20,6 @@ import java.util.Arrays; import java.util.Collection; import java.util.EnumMap; -import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; @@ -155,12 +154,6 @@ private CompletableFuture> fetchUser(AllowedServicePointsRequest re ? this::extractAllowedServicePointsIgnoringItemStatus : this::extractAllowedServicePointsConsideringItemStatus; - if (request.isUseStubItem()) { - return requestPolicyRepository.lookupRequestPolicy(user) - .thenCompose(r -> r.after(policy -> extractAllowedServicePointsIgnoringItemStatus( - policy, new HashSet<>()))); - } - return requestPolicyRepository.lookupRequestPolicies(items, user) .thenCompose(r -> r.after(policies -> allOf(policies, mappingFunction))) .thenApply(r -> r.map(this::combineAllowedServicePoints)); diff --git a/src/test/java/api/requests/AllowedServicePointsAPITests.java b/src/test/java/api/requests/AllowedServicePointsAPITests.java index a8da598cd2..5cf30e0352 100644 --- a/src/test/java/api/requests/AllowedServicePointsAPITests.java +++ b/src/test/java/api/requests/AllowedServicePointsAPITests.java @@ -14,7 +14,6 @@ import static org.folio.circulation.domain.RequestType.HOLD; import static org.folio.circulation.domain.RequestType.PAGE; import static org.folio.circulation.domain.RequestType.RECALL; -import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.containsString; @@ -35,6 +34,7 @@ import org.apache.http.HttpStatus; import org.folio.circulation.domain.ItemStatus; +import org.folio.circulation.domain.Request; import org.folio.circulation.domain.RequestLevel; import org.folio.circulation.domain.RequestType; import org.folio.circulation.support.http.client.Response; @@ -154,8 +154,8 @@ void shouldReturnListOfAllowedServicePointsForRequest(RequestType requestType, .collect(Collectors.toSet())); var response = requestLevel == TITLE - ? get("create", requesterId, instanceId, null, null, null, HttpStatus.SC_OK).getJson() - : get("create", requesterId, null, itemId, null, null, HttpStatus.SC_OK).getJson(); + ? get("create", requesterId, instanceId, null, null, HttpStatus.SC_OK).getJson() + : get("create", requesterId, null, itemId, null, HttpStatus.SC_OK).getJson(); assertThat(response, allowedServicePointMatcher(Map.of(requestType, allowedSpInResponse))); } @@ -223,7 +223,7 @@ void shouldReturnListOfAllowedServicePointsForRequestReplacement( var requestId = request == null ? null : request.getId().toString(); var response = - get("replace", null, null, null, requestId, null, HttpStatus.SC_OK).getJson(); + get("replace", null, null, null, requestId, HttpStatus.SC_OK).getJson(); assertThat(response, allowedServicePointMatcher(Map.of(requestType, allowedSpInResponse))); } @@ -624,7 +624,7 @@ void shouldReturnAllowedServicePointsForAllEnabledRequestTypes() { @Test void getReplaceFailsWhenRequestDoesNotExist() { String requestId = randomId(); - Response response = get("replace", null, null, null, requestId, null, + Response response = get("replace", null, null, null, requestId, HttpStatus.SC_UNPROCESSABLE_ENTITY); assertThat(response.getJson(), hasErrorWith(hasMessage( String.format("Request with ID %s was not found", requestId)))); @@ -634,7 +634,7 @@ void getReplaceFailsWhenRequestDoesNotExist() { void getMoveFailsWhenRequestDoesNotExist() { String requestId = randomId(); String itemId = itemsFixture.basedUponNod().getId().toString(); - Response response = get("move", null, null, itemId, requestId, null, + Response response = get("move", null, null, itemId, requestId, HttpStatus.SC_UNPROCESSABLE_ENTITY); assertThat(response.getJson(), hasErrorWith(hasMessage( String.format("Request with ID %s was not found", requestId)))); @@ -696,100 +696,62 @@ void shouldReturnListOfAllowedServicePointsForRequestMove(RequestLevel requestLe // Valid "move" request var moveResponse = - get("move", null, null, itemToMoveToId, requestId, null, HttpStatus.SC_OK).getJson(); + get("move", null, null, itemToMoveToId, requestId, HttpStatus.SC_OK).getJson(); assertThat(moveResponse, allowedServicePointMatcher(Map.of(HOLD, List.of(sp2)))); // Invalid "move" requests var invalidMoveResponse1 = get("move", null, null, null, requestId, - null, HttpStatus.SC_BAD_REQUEST); + HttpStatus.SC_BAD_REQUEST); assertThat(invalidMoveResponse1.getBody(), equalTo("Invalid combination of query parameters")); var invalidMoveResponse2 = get("move", null, null, itemToMoveToId, null, - null, HttpStatus.SC_BAD_REQUEST); + HttpStatus.SC_BAD_REQUEST); assertThat(invalidMoveResponse2.getBody(), equalTo("Invalid combination of query parameters")); var invalidMoveResponse3 = get("move", null, null, null, null, - null, HttpStatus.SC_BAD_REQUEST); + HttpStatus.SC_BAD_REQUEST); assertThat(invalidMoveResponse3.getBody(), equalTo("Invalid combination of query parameters")); var invalidMoveResponse4 = get("move", requesterId, null, itemToMoveToId, requestId, - null, HttpStatus.SC_BAD_REQUEST); + HttpStatus.SC_BAD_REQUEST); assertThat(invalidMoveResponse4.getBody(), equalTo("Invalid combination of query parameters")); var invalidMoveResponse5 = get("move", null, instanceId, itemToMoveToId, requestId, - null, HttpStatus.SC_BAD_REQUEST); + HttpStatus.SC_BAD_REQUEST); assertThat(invalidMoveResponse5.getBody(), equalTo("Invalid combination of query parameters")); // Valid "replace" request var replaceResponse = - get("replace", null, null, null, requestId, null, HttpStatus.SC_OK).getJson(); + get("replace", null, null, null, requestId, HttpStatus.SC_OK).getJson(); assertThat(replaceResponse, allowedServicePointMatcher(Map.of(HOLD, List.of(sp2)))); // Invalid "replace" requests var invalidReplaceResponse1 = get("replace", null, null, null, null, - null, HttpStatus.SC_BAD_REQUEST); + HttpStatus.SC_BAD_REQUEST); assertThat(invalidReplaceResponse1.getBody(), equalTo("Invalid combination of query parameters")); var invalidReplaceResponse2 = get("replace", requesterId, null, null, requestId, - null, HttpStatus.SC_BAD_REQUEST); + HttpStatus.SC_BAD_REQUEST); assertThat(invalidReplaceResponse2.getBody(), equalTo("Invalid combination of query parameters")); var invalidReplaceResponse3 = get("replace", null, instanceId, null, requestId, - null, HttpStatus.SC_BAD_REQUEST); + HttpStatus.SC_BAD_REQUEST); assertThat(invalidReplaceResponse3.getBody(), equalTo("Invalid combination of query parameters")); var invalidReplaceResponse4 = get("replace", null, null, requestedItemId, requestId, - null, HttpStatus.SC_BAD_REQUEST); + HttpStatus.SC_BAD_REQUEST); assertThat(invalidReplaceResponse4.getBody(), equalTo("Invalid combination of query parameters")); var invalidReplaceResponse5 = get("replace", requesterId, instanceId, - requestedItemId, requestId, null, HttpStatus.SC_BAD_REQUEST); + requestedItemId, requestId, HttpStatus.SC_BAD_REQUEST); assertThat(invalidReplaceResponse5.getBody(), equalTo("Invalid combination of query parameters")); } - @Test - void shouldUseStubItemParameterInCirculationRuleMatchingWhenPresent() { - var requesterId = usersFixture.steve().getId().toString(); - var instanceId = itemsFixture.createMultipleItemsForTheSameInstance(2).get(0) - .getInstanceId().toString(); - var cd1 = servicePointsFixture.cd1(); - var cd2 = servicePointsFixture.cd2(); - var cd4 = servicePointsFixture.cd4(); - var cd5 = servicePointsFixture.cd5(); - final UUID book = materialTypesFixture.book().getId(); - final UUID patronGroup = patronGroupsFixture.regular().getId(); - circulationRulesFixture.updateCirculationRules(createRules("m " + book + - "+ g " + patronGroup, "g " + patronGroup)); - - var response = getCreateOp(requesterId, instanceId, null, "true", HttpStatus.SC_OK).getJson(); - assertThat(response, hasNoJsonPath(PAGE.getValue())); - JsonArray allowedServicePoints = response.getJsonArray(HOLD.getValue()); - assertServicePointsMatch(allowedServicePoints, List.of(cd1, cd2, cd4, cd5)); - allowedServicePoints = response.getJsonArray(RECALL.getValue()); - assertServicePointsMatch(allowedServicePoints, List.of(cd1, cd2, cd4, cd5)); - - response = getCreateOp(requesterId, instanceId, null, "false", HttpStatus.SC_OK).getJson(); - assertThat(response, hasNoJsonPath(HOLD.getValue())); - assertThat(response, hasNoJsonPath(RECALL.getValue())); - allowedServicePoints = response.getJsonArray(PAGE.getValue()); - assertServicePointsMatch(allowedServicePoints, List.of(cd1, cd2, cd4, cd5)); - - response = getCreateOp(requesterId, instanceId, null, HttpStatus.SC_OK).getJson(); - assertThat(response, hasNoJsonPath(HOLD.getValue())); - assertThat(response, hasNoJsonPath(RECALL.getValue())); - allowedServicePoints = response.getJsonArray(PAGE.getValue()); - assertServicePointsMatch(allowedServicePoints, List.of(cd1, cd2, cd4, cd5)); - - Response errorResponse = getCreateOp(requesterId, instanceId, null, "invalid", - HttpStatus.SC_BAD_REQUEST); - assertThat(errorResponse.getBody(), is("useStubItem is not a valid boolean: invalid.")); - } - private void assertServicePointsMatch(JsonArray response, List expectedServicePoints) { @@ -813,24 +775,18 @@ private void assertServicePointsMatch(JsonArray response, .map(sp -> sp.getJson().getString("name")).toArray(String[]::new))); } - private Response getCreateOp(String requesterId, String instanceId, String itemId, - String useStubItem, int expectedStatusCode) { - - return get("create", requesterId, instanceId, itemId, null, useStubItem, expectedStatusCode); - } - private Response getCreateOp(String requesterId, String instanceId, String itemId, int expectedStatusCode) { - return get("create", requesterId, instanceId, itemId, null, null, expectedStatusCode); + return get("create", requesterId, instanceId, itemId, null, expectedStatusCode); } private Response getReplaceOp(String requestId, int expectedStatusCode) { - return get("replace", null, null, null, requestId, null, expectedStatusCode); + return get("replace", null, null, null, requestId, expectedStatusCode); } private Response get(String operation, String requesterId, String instanceId, String itemId, - String requestId, String useStubItem, int expectedStatusCode) { + String requestId, int expectedStatusCode) { List queryParams = new ArrayList<>(); queryParams.add(namedParameter("operation", operation)); @@ -846,9 +802,6 @@ private Response get(String operation, String requesterId, String instanceId, St if (requestId != null) { queryParams.add(namedParameter("requestId", requestId)); } - if (useStubItem != null) { - queryParams.add(namedParameter("useStubItem", useStubItem)); - } return restAssuredClient.get(allowedServicePointsUrl(), queryParams, expectedStatusCode, "allowed-service-points"); @@ -871,21 +824,4 @@ private ServicePointBuilder servicePointBuilder() { .withPickupLocation(TRUE) .withHoldShelfExpriyPeriod(30, "Days"); } - - private String createRules(String firstRuleCondition, String secondRuleCondition) { - final var loanPolicy = loanPoliciesFixture.canCirculateRolling().getId().toString(); - final var allowAllRequestPolicy = requestPoliciesFixture.allowAllRequestPolicy() - .getId().toString(); - final var holdAndRecallRequestPolicy = requestPoliciesFixture.allowHoldAndRecallRequestPolicy() - .getId().toString(); - final var noticePolicy = noticePoliciesFixture.activeNotice().getId().toString(); - final var overdueFinePolicy = overdueFinePoliciesFixture.facultyStandard().getId().toString(); - final var lostItemFeePolicy = lostItemFeePoliciesFixture.facultyStandard().getId().toString(); - - return String.join("\n", - "priority: t, s, c, b, a, m, g", - "fallback-policy: l " + loanPolicy + " r " + allowAllRequestPolicy + " n " + noticePolicy + " o " + overdueFinePolicy + " i " + lostItemFeePolicy, - firstRuleCondition + " : l " + loanPolicy + " r " + allowAllRequestPolicy + " n " + noticePolicy + " o " + overdueFinePolicy + " i " + lostItemFeePolicy, - secondRuleCondition + " : l " + loanPolicy + " r " + holdAndRecallRequestPolicy + " n " + noticePolicy + " o " + overdueFinePolicy + " i " + lostItemFeePolicy); - } }