Skip to content

Commit

Permalink
[CIRC-2051] Add ecsRequestRouting parameter to allowed-service-points (…
Browse files Browse the repository at this point in the history 
…#1455)

* CIRC-2051 add ecsRequestRouting to allowed-service-points

* CIRC-2051 refactoring

* CIRC-2051 move common validation to separate method

* CIRC-2051 fix code smells

* CIRC-2051 fix code smell

* CIRC-2051 tests refactoring

* CIRC-2051 rename test
  • Loading branch information
@roman-barannyk
roman-barannyk authored Apr 15, 2024
1 parent 948a2c5 commit 19bf73c
Show file tree
Hide file tree
Showing 11 changed files with 173 additions and 56 deletions.
2 changes: 1 addition & 1 deletion descriptors/ModuleDescriptor-template.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -649,7 +649,7 @@
},
{
"id": "allowed-service-points",
"version": "1.1",
"version": "1.2",
"handlers": [
{
"methods": [
Expand Down
4 changes: 4 additions & 0 deletions ramls/circulation.raml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,10 @@ resourceTypes:
description: "When true, allows to apply circulation rules based on patron group only"
type: boolean
required: false
ecsRequestRouting:
description: "When true, returns only service points with ecsRequestRouting"
type: boolean
required: false
responses:
200:
description: "List of allowed service points was retrieved successfully"
Expand Down
1 change: 1 addition & 0 deletions src/main/java/org/folio/circulation/domain/AllowedServicePointsRequest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ public class AllowedServicePointsRequest {
private String itemId;
private String requestId;
private boolean useStubItem;
private boolean ecsRequestRouting;

public boolean isForTitleLevelRequest() {
return instanceId != null;
Expand Down
16 changes: 9 additions & 7 deletions src/main/java/org/folio/circulation/infrastructure/storage/ServicePointRepository.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -206,22 +206,24 @@ public CompletableFuture<Result<Collection<ServicePoint>>> findServicePointsById
.thenApply(r -> r.map(MultipleRecords::getRecords));
}

public CompletableFuture<Result<Collection<ServicePoint>>> fetchPickupLocationServicePoints() {
public CompletableFuture<Result<Collection<ServicePoint>>> fetchServicePointsByIndexName(
String indexName) {

return createServicePointsFetcher().find(MultipleCqlIndexValuesCriteria.builder()
.indexName("pickupLocation")
.indexName(indexName)
.indexOperator(CqlQuery::matchAny)
.value("true")
.build())
.thenApply(r -> r.map(MultipleRecords::getRecords));
}

public CompletableFuture<Result<Collection<ServicePoint>>> fetchPickupLocationServicePointsByIds(
Set<String> ids) {
public CompletableFuture<Result<Collection<ServicePoint>>>
fetchPickupLocationServicePointsByIdsAndIndexName(Set<String> ids, String indexName) {

log.debug("filterIdsByServicePointsAndPickupLocationExistence:: parameters ids: {}",
() -> collectionAsString(ids));
log.debug("filterIdsByServicePointsAndPickupLocationExistence:: parameters ids: {}, " +
"indexName: {}", () -> collectionAsString(ids), () -> indexName);

Result<CqlQuery> pickupLocationQuery = exactMatch("pickupLocation", "true");
Result<CqlQuery> pickupLocationQuery = exactMatch(indexName, "true");

return createServicePointsFetcher().findByIdIndexAndQuery(ids, "id", pickupLocationQuery)
.thenApply(r -> r.map(MultipleRecords::getRecords));
Expand Down
21 changes: 14 additions & 7 deletions src/main/java/org/folio/circulation/resources/AllowedServicePointsResource.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,8 @@ private void get(RoutingContext routingContext) {

ofAsync(routingContext)
.thenApply(r -> r.next(AllowedServicePointsResource::buildRequest))
.thenCompose(r -> r.after(new AllowedServicePointsService(clients)::getAllowedServicePoints))
.thenCompose(r -> r.after(request -> new AllowedServicePointsService(
clients, request.isEcsRequestRouting()).getAllowedServicePoints(request)))
.thenApply(r -> r.map(AllowedServicePointsResource::toJson))
.thenApply(r -> r.map(JsonHttpResponse::ok))
.exceptionally(CommonFailures::failedDueToServerError)
Expand All @@ -72,6 +73,7 @@ private static Result<AllowedServicePointsRequest> buildRequest(RoutingContext r
String itemId = queryParams.get("itemId");
String requestId = queryParams.get("requestId");
String useStubItem = queryParams.get("useStubItem");
String ecsRequestRouting = queryParams.get("ecsRequestRouting");

List<String> errors = new ArrayList<>();

Expand All @@ -94,11 +96,8 @@ private static Result<AllowedServicePointsRequest> buildRequest(RoutingContext r
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));
}
validateBoolean(useStubItem, "useStubItem", errors);
validateBoolean(ecsRequestRouting, "ecsRequestRouting", errors);
// Checking parameter combinations
boolean allowedCombinationOfParametersDetected = false;

Expand Down Expand Up @@ -137,7 +136,15 @@ private static Result<AllowedServicePointsRequest> buildRequest(RoutingContext r
}

return succeeded(new AllowedServicePointsRequest(operation, requesterId, instanceId, itemId,
requestId, Boolean.parseBoolean(useStubItem)));
requestId, Boolean.parseBoolean(useStubItem), Boolean.parseBoolean(ecsRequestRouting)));
}

private static void validateBoolean(String parameter, String parameterName, List<String> errors) {
if (parameter != null && !"true".equals(parameter) && !"false".equals(parameter)) {
log.warn("validateBoolean:: {} is not a valid boolean: {}",
parameterName, parameter);
errors.add(String.format("%s is not a valid boolean: %s.", parameterName, parameter));
}
}

private static JsonObject toJson(Map<RequestType, Set<AllowedServicePoint>> allowedServicePoints) {
Expand Down
10 changes: 7 additions & 3 deletions src/main/java/org/folio/circulation/services/AllowedServicePointsService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@

public class AllowedServicePointsService {
private static final Logger log = LogManager.getLogger(MethodHandles.lookup().lookupClass());
private static final String ECS_REQUEST_ROUTING_INDEX_NAME = "ecsRequestRouting";
private static final String PICKUP_LOCATION_INDEX_NAME = "pickupLocation";
private final ItemRepository itemRepository;
private final UserRepository userRepository;
private final RequestRepository requestRepository;
Expand All @@ -66,8 +68,9 @@ public class AllowedServicePointsService {
private final ItemByInstanceIdFinder itemFinder;
private final ConfigurationRepository configurationRepository;
private final InstanceRepository instanceRepository;
private final String indexName;

public AllowedServicePointsService(Clients clients) {
public AllowedServicePointsService(Clients clients, boolean isEcsRequestRouting) {
itemRepository = new ItemRepository(clients);
userRepository = new UserRepository(clients);
requestRepository = new RequestRepository(clients);
Expand All @@ -76,6 +79,7 @@ public AllowedServicePointsService(Clients clients) {
configurationRepository = new ConfigurationRepository(clients);
instanceRepository = new InstanceRepository(clients);
itemFinder = new ItemByInstanceIdFinder(clients.holdingsStorage(), itemRepository);
indexName = isEcsRequestRouting ? ECS_REQUEST_ROUTING_INDEX_NAME : PICKUP_LOCATION_INDEX_NAME;
}

public CompletableFuture<Result<Map<RequestType, Set<AllowedServicePoint>>>>
Expand Down Expand Up @@ -360,7 +364,7 @@ private Map<RequestType, Set<AllowedServicePoint>> combineAllowedServicePoints(
}

private CompletableFuture<Result<Set<AllowedServicePoint>>> fetchAllowedServicePoints() {
return servicePointRepository.fetchPickupLocationServicePoints()
return servicePointRepository.fetchServicePointsByIndexName(indexName)
.thenApply(r -> r.map(servicePoints -> servicePoints.stream()
.map(AllowedServicePoint::new)
.collect(Collectors.toSet())));
Expand All @@ -372,7 +376,7 @@ private CompletableFuture<Result<Set<AllowedServicePoint>>> fetchPickupLocationS
log.debug("filterIdsByServicePointsAndPickupLocationExistence:: parameters ids: {}",
() -> collectionAsString(ids));

return servicePointRepository.fetchPickupLocationServicePointsByIds(ids)
return servicePointRepository.fetchPickupLocationServicePointsByIdsAndIndexName(ids, indexName)
.thenApply(servicePointsResult -> servicePointsResult
.map(servicePoints -> servicePoints.stream()
.map(AllowedServicePoint::new)
Expand Down
106 changes: 81 additions & 25 deletions src/test/java/api/requests/AllowedServicePointsAPITests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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, null, null, HttpStatus.SC_OK).getJson()
: get("create", requesterId, null, itemId, null, null, null, HttpStatus.SC_OK).getJson();

assertThat(response, allowedServicePointMatcher(Map.of(requestType, allowedSpInResponse)));
}
Expand Down Expand Up @@ -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, null, null, HttpStatus.SC_OK).getJson();

assertThat(response, allowedServicePointMatcher(Map.of(requestType, allowedSpInResponse)));
}
Expand Down Expand Up @@ -625,7 +625,7 @@ void shouldReturnAllowedServicePointsForAllEnabledRequestTypes() {
void getReplaceFailsWhenRequestDoesNotExist() {
String requestId = randomId();
Response response = get("replace", null, null, null, requestId, null,
HttpStatus.SC_UNPROCESSABLE_ENTITY);
null, HttpStatus.SC_UNPROCESSABLE_ENTITY);
assertThat(response.getJson(), hasErrorWith(hasMessage(
String.format("Request with ID %s was not found", requestId))));
}
Expand All @@ -635,7 +635,7 @@ void getMoveFailsWhenRequestDoesNotExist() {
String requestId = randomId();
String itemId = itemsFixture.basedUponNod().getId().toString();
Response response = get("move", null, null, itemId, requestId, null,
HttpStatus.SC_UNPROCESSABLE_ENTITY);
null, HttpStatus.SC_UNPROCESSABLE_ENTITY);
assertThat(response.getJson(), hasErrorWith(hasMessage(
String.format("Request with ID %s was not found", requestId))));
}
Expand Down Expand Up @@ -696,58 +696,58 @@ 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, null, null, 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);
null, null, 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);
null, null, 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);
null, null, 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);
null, null, 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);
null, null, 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, null, null, 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);
null, null, 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);
null, null, 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);
null, null, 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);
null, null, 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, null, null, HttpStatus.SC_BAD_REQUEST);
assertThat(invalidReplaceResponse5.getBody(),
equalTo("Invalid combination of query parameters"));
}
Expand All @@ -766,14 +766,16 @@ void shouldUseStubItemParameterInCirculationRuleMatchingWhenPresent() {
circulationRulesFixture.updateCirculationRules(createRules("m " + book +
"+ g " + patronGroup, "g " + patronGroup));

var response = getCreateOp(requesterId, instanceId, null, "true", HttpStatus.SC_OK).getJson();
var response = getCreateOp(requesterId, instanceId, null, "true", null, 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();
response = getCreateOp(requesterId, instanceId, null, "false", null, HttpStatus.SC_OK)
.getJson();
assertThat(response, hasNoJsonPath(HOLD.getValue()));
assertThat(response, hasNoJsonPath(RECALL.getValue()));
allowedServicePoints = response.getJsonArray(PAGE.getValue());
Expand All @@ -784,12 +786,62 @@ void shouldUseStubItemParameterInCirculationRuleMatchingWhenPresent() {
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",
@Test
void shouldReturnErrorIfUseStubItemIsInvalid() {
Response errorResponse = getCreateOp(UUID.randomUUID().toString(),
UUID.randomUUID().toString(), null, "invalid", null,
HttpStatus.SC_BAD_REQUEST);
assertThat(errorResponse.getBody(), is("useStubItem is not a valid boolean: invalid."));
}

@Test
void shouldConsiderEcsRequestRoutingParameterForAllowedServicePoints() {
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 cd11 = servicePointsFixture.cd11();

final Map<RequestType, Set<UUID>> allowedServicePointsInPolicy = new HashMap<>();
allowedServicePointsInPolicy.put(PAGE, Set.of(cd1.getId(), cd2.getId(), cd11.getId()));
allowedServicePointsInPolicy.put(HOLD, Set.of(cd4.getId(), cd2.getId(), cd11.getId()));
var requestPolicy = requestPoliciesFixture.createRequestPolicyWithAllowedServicePoints(
allowedServicePointsInPolicy, PAGE, HOLD);
policiesActivation.use(PoliciesToActivate.builder().requestPolicy(requestPolicy));

var response = getCreateOp(requesterId, instanceId, null, "false", "true",
HttpStatus.SC_OK).getJson();
JsonArray allowedServicePoints = response.getJsonArray(PAGE.getValue());
assertServicePointsMatch(allowedServicePoints, List.of(cd11));
assertThat(response, hasNoJsonPath(HOLD.getValue()));
assertThat(response, hasNoJsonPath(RECALL.getValue()));

response = getCreateOp(requesterId, instanceId, null, "false", "false",
HttpStatus.SC_OK).getJson();
allowedServicePoints = response.getJsonArray(PAGE.getValue());
assertServicePointsMatch(allowedServicePoints, List.of(cd1, cd2));
assertThat(response, hasNoJsonPath(HOLD.getValue()));
assertThat(response, hasNoJsonPath(RECALL.getValue()));

response = getCreateOp(requesterId, instanceId, null, "false", null,
HttpStatus.SC_OK).getJson();
allowedServicePoints = response.getJsonArray(PAGE.getValue());
assertServicePointsMatch(allowedServicePoints, List.of(cd1, cd2));
assertThat(response, hasNoJsonPath(HOLD.getValue()));
assertThat(response, hasNoJsonPath(RECALL.getValue()));
}

@Test
void shouldReturnErrorIfEcsRequestRoutingIsInvalid() {
Response errorResponse = getCreateOp(UUID.randomUUID().toString(),
UUID.randomUUID().toString(), null, null, "invalid", HttpStatus.SC_BAD_REQUEST);
assertThat(errorResponse.getBody(), is("ecsRequestRouting is not a valid boolean: invalid."));
}

private void assertServicePointsMatch(JsonArray response,
List<IndividualResource> expectedServicePoints) {

Expand All @@ -814,23 +866,24 @@ private void assertServicePointsMatch(JsonArray response,
}

private Response getCreateOp(String requesterId, String instanceId, String itemId,
String useStubItem, int expectedStatusCode) {
String useStubItem, String ecsRequestRouting, int expectedStatusCode) {

return get("create", requesterId, instanceId, itemId, null, useStubItem, expectedStatusCode);
return get("create", requesterId, instanceId, itemId, null, useStubItem,
ecsRequestRouting, 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, null, 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, null, null, expectedStatusCode);
}

private Response get(String operation, String requesterId, String instanceId, String itemId,
String requestId, String useStubItem, int expectedStatusCode) {
String requestId, String useStubItem, String ecsRequestRouting, int expectedStatusCode) {

List<QueryStringParameter> queryParams = new ArrayList<>();
queryParams.add(namedParameter("operation", operation));
Expand All @@ -849,6 +902,9 @@ private Response get(String operation, String requesterId, String instanceId, St
if (useStubItem != null) {
queryParams.add(namedParameter("useStubItem", useStubItem));
}
if (ecsRequestRouting != null) {
queryParams.add(namedParameter("ecsRequestRouting", ecsRequestRouting));
}

return restAssuredClient.get(allowedServicePointsUrl(), queryParams, expectedStatusCode,
"allowed-service-points");
Expand Down
Loading

0 comments on commit 19bf73c

Please sign in to comment.