From cc0d1f28b978f5c0e9a5a0f4b6de01c089335880 Mon Sep 17 00:00:00 2001 From: Roman Barannyk <53909129+roman-barannyk@users.noreply.github.com> Date: Mon, 18 Sep 2023 18:42:02 +0300 Subject: [PATCH 1/2] CIRC-1809 add logging to circulation rules package (#1325) * CIRC-1809 add logging to circulation rules package * CIRC-1809 fix typo --- .../rules/AppliedRuleConditions.java | 2 +- .../rules/CirculationRuleMatch.java | 2 +- .../rules/CirculationRulesProcessor.java | 27 ++++++++++++++++++- .../org/folio/circulation/rules/Drools.java | 26 ++++++++++++++++++ .../circulation/rules/ExecutableRules.java | 10 +++++++ .../org/folio/circulation/rules/Match.java | 3 +++ .../rules/cache/CirculationRulesCache.java | 12 +++++---- 7 files changed, 74 insertions(+), 8 deletions(-) diff --git a/src/main/java/org/folio/circulation/rules/AppliedRuleConditions.java b/src/main/java/org/folio/circulation/rules/AppliedRuleConditions.java index 074bcf1ed2..5223251b88 100644 --- a/src/main/java/org/folio/circulation/rules/AppliedRuleConditions.java +++ b/src/main/java/org/folio/circulation/rules/AppliedRuleConditions.java @@ -9,7 +9,7 @@ public class AppliedRuleConditions { boolean isPatronGroupPresent; public AppliedRuleConditions(boolean isItemTypePresent, - boolean isLoanTypePresent, boolean isPatronGroupPresent) { + boolean isLoanTypePresent, boolean isPatronGroupPresent) { this.isItemTypePresent = isItemTypePresent; this.isLoanTypePresent = isLoanTypePresent; diff --git a/src/main/java/org/folio/circulation/rules/CirculationRuleMatch.java b/src/main/java/org/folio/circulation/rules/CirculationRuleMatch.java index 7dfd142b66..7206cf9705 100644 --- a/src/main/java/org/folio/circulation/rules/CirculationRuleMatch.java +++ b/src/main/java/org/folio/circulation/rules/CirculationRuleMatch.java @@ -6,7 +6,7 @@ public class CirculationRuleMatch { private final AppliedRuleConditions appliedRuleConditions; public CirculationRuleMatch(String policyId, - AppliedRuleConditions appliedRuleConditions) { + AppliedRuleConditions appliedRuleConditions) { this.policyId = policyId; this.appliedRuleConditions = appliedRuleConditions; diff --git a/src/main/java/org/folio/circulation/rules/CirculationRulesProcessor.java b/src/main/java/org/folio/circulation/rules/CirculationRulesProcessor.java index 4889f7a6a3..289263b974 100644 --- a/src/main/java/org/folio/circulation/rules/CirculationRulesProcessor.java +++ b/src/main/java/org/folio/circulation/rules/CirculationRulesProcessor.java @@ -38,10 +38,14 @@ public CirculationRulesProcessor(String tenantId, CollectionResourceClient circu public CompletableFuture> getLoanPolicyAndMatch( RulesExecutionParameters params) { + log.debug("getLoanPolicyAndMatch:: parameters params: {}", params); + return executeRules(params, ExecutableRules::determineLoanPolicy); } public CompletableFuture> getLoanPolicies(RulesExecutionParameters params) { + log.debug("getLoanPolicies:: parameters params: {}", params); + return triggerRules(params, (drools, newParams) -> drools.loanPolicies(newParams.toMap(), newParams.getLocation())); } @@ -49,10 +53,14 @@ public CompletableFuture> getLoanPolicies(RulesExecutionParame public CompletableFuture> getLostItemPolicyAndMatch( RulesExecutionParameters params) { + log.debug("getLostItemPolicyAndMatch:: parameters params: {}", params); + return executeRules(params, ExecutableRules::determineLostItemPolicy); } public CompletableFuture> getLostItemPolicies(RulesExecutionParameters params) { + log.debug("getLostItemPolicies:: parameters params: {}", params); + return triggerRules(params, (drools, newParams) -> drools.lostItemPolicies(newParams.toMap(), newParams.getLocation())); } @@ -60,10 +68,14 @@ public CompletableFuture> getLostItemPolicies(RulesExecutionPa public CompletableFuture> getNoticePolicyAndMatch( RulesExecutionParameters params) { + log.debug("getNoticePolicyAndMatch:: parameters params: {}", params); + return executeRules(params, ExecutableRules::determineNoticePolicy); } public CompletableFuture> getNoticePolicies(RulesExecutionParameters params) { + log.debug("getNoticePolicies:: parameters params: {}", params); + return triggerRules(params, (drools, newParams) -> drools.noticePolicies(newParams.toMap(), newParams.getLocation())); } @@ -71,10 +83,14 @@ public CompletableFuture> getNoticePolicies(RulesExecutionPara public CompletableFuture> getOverduePolicyAndMatch( RulesExecutionParameters params) { + log.debug("getOverduePolicyAndMatch:: parameters params: {}", params); + return executeRules(params, ExecutableRules::determineOverduePolicy); } public CompletableFuture> getOverduePolicies(RulesExecutionParameters params) { + log.debug("getOverduePolicies:: parameters params: {}", params); + return triggerRules(params, (drools, newParams) -> drools.overduePolicies(newParams.toMap(), newParams.getLocation())); } @@ -82,10 +98,14 @@ public CompletableFuture> getOverduePolicies(RulesExecutionPar public CompletableFuture> getRequestPolicyAndMatch( RulesExecutionParameters params) { + log.debug("getRequestPolicyAndMatch:: parameters params: {}", params); + return executeRules(params, ExecutableRules::determineRequestPolicy); } public CompletableFuture> getRequestPolicies(RulesExecutionParameters params) { + log.debug("getRequestPolicies:: parameters params: {}", params); + return triggerRules(params, (drools, newParams) -> drools.requestPolicies(newParams.toMap(), newParams.getLocation())); } @@ -114,8 +134,13 @@ private CompletableFuture> executeRules(RulesExecutionParameters p rulesExecutor.apply(rules, parametersWithLocation))); } - private CompletableFuture> fetchLocation(RulesExecutionParameters params) { + private CompletableFuture> fetchLocation( + RulesExecutionParameters params) { + + log.debug("fetchLocation:: parameters params: {}", params); + if (params.getLocation() != null) { + log.info("fetchLocation:: location is not null"); return ofAsync(() -> params); } diff --git a/src/main/java/org/folio/circulation/rules/Drools.java b/src/main/java/org/folio/circulation/rules/Drools.java index 6f27e67192..529ccb9a20 100644 --- a/src/main/java/org/folio/circulation/rules/Drools.java +++ b/src/main/java/org/folio/circulation/rules/Drools.java @@ -5,11 +5,14 @@ import static org.folio.circulation.resources.AbstractCirculationRulesEngineResource.LOCATION_ID_NAME; import static org.folio.circulation.resources.AbstractCirculationRulesEngineResource.PATRON_TYPE_ID_NAME; import static org.folio.circulation.support.json.JsonPropertyWriter.write; +import static org.folio.circulation.support.utils.LogUtil.asJson; import java.util.Collections; import java.util.Set; import java.util.stream.Collectors; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import org.drools.core.definitions.rule.impl.RuleImpl; import org.drools.core.event.DefaultAgendaEventListener; import org.folio.circulation.domain.Location; @@ -33,6 +36,7 @@ public class Drools { // https://docs.jboss.org/drools/release/6.2.0.CR1/drools-docs/html/ch19.html // http://www.deepakgaikwad.net/index.php/2016/05/16/drools-tutorial-beginners.html + private static final Logger log = LogManager.getLogger(CirculationRulesProcessor.class); private final KieContainer kieContainer; /** @@ -59,6 +63,8 @@ public Drools(String tenantId, String drools) { } private KieSession createSession(MultiMap params, Location location, Match match) { + log.debug("createSession:: parameters params: {}, location: {}, match: {}", params.size(), + location, match); String itemTypeId = params.get(ITEM_TYPE_ID_NAME); String loanTypeId = params.get(LOAN_TYPE_ID_NAME); String patronGroupId = params.get(PATRON_TYPE_ID_NAME); @@ -71,6 +77,7 @@ private KieSession createSession(MultiMap params, Location location, Match match kieSession.insert(new PatronGroup(patronGroupId)); kieSession.insert(new ItemLocation(locationId)); if (location != null) { + log.info("createSession:: location is not null"); kieSession.insert(new Institution(location.getInstitutionId())); kieSession.insert(new Campus(location.getCampusId())); kieSession.insert(new Library(location.getLibraryId())); @@ -86,6 +93,7 @@ private KieSession createSession(MultiMap params, Location location, Match match * @return CirculationRuleMatch object with the name of the loan policy and rule conditions */ public CirculationRuleMatch loanPolicy(MultiMap params, Location location) { + log.debug("loanPolicy:: params params: {}, location: {}", params.size(), location); final var match = new Match(); final KieSession kieSession = createSession(params, location, match); final RuleEventListener ruleEventListener = new RuleEventListener(); @@ -110,6 +118,7 @@ public CirculationRuleMatch loanPolicy(MultiMap params, Location location) { * @return matches, each match has a loanPolicyId and a circulationRuleLine field */ public JsonArray loanPolicies(MultiMap params, Location location) { + log.debug("loanPolicies:: params params: {}, location: {}", params.size(), location); final var match = new Match(); final KieSession kieSession = createSession(params, location, match); @@ -125,6 +134,7 @@ public JsonArray loanPolicies(MultiMap params, Location location) { } kieSession.dispose(); + log.info("loanPolicies:: result: {}", () -> asJson(array.stream().toList())); return array; } @@ -135,6 +145,7 @@ public JsonArray loanPolicies(MultiMap params, Location location) { * @return CirculationRuleMatch object with the name of the loan policy and rule conditions */ public CirculationRuleMatch requestPolicy(MultiMap params, Location location) { + log.debug("requestPolicy:: parameters params: {}, location: {}", params, location); final var match = new Match(); final KieSession kieSession = createSession(params, location, match); @@ -153,6 +164,8 @@ public CirculationRuleMatch requestPolicy(MultiMap params, Location location) { * @return matches, each match has a requestPolicyId and a circulationRuleLine field */ public JsonArray requestPolicies(MultiMap params, Location location) { + log.debug("requestPolicy:: parameters params: {}, location: {}", params, location); + final var match = new Match(); final KieSession kieSession = createSession(params, location, match); @@ -169,6 +182,7 @@ public JsonArray requestPolicies(MultiMap params, Location location) { kieSession.dispose(); + log.info("requestPolicies:: result: {}", () -> asJson(array.stream().toList())); return array; } @@ -179,6 +193,7 @@ public JsonArray requestPolicies(MultiMap params, Location location) { * @return CirculationRuleMatch object with the name of the loan policy and rule conditions */ public CirculationRuleMatch noticePolicy(MultiMap params, Location location) { + log.debug("noticePolicy:: parameters params: {}, location: {}", params.size(), location); final var match = new Match(); final KieSession kieSession = createSession(params, location, match); @@ -197,6 +212,7 @@ public CirculationRuleMatch noticePolicy(MultiMap params, Location location) { * @return matches, each match has a noticePolicyId and a circulationRuleLine field */ public JsonArray noticePolicies(MultiMap params, Location location) { + log.debug("noticePolicies:: parameters params: {}, location: {}", params.size(), location); final var match = new Match(); final KieSession kieSession = createSession(params, location, match); @@ -212,6 +228,7 @@ public JsonArray noticePolicies(MultiMap params, Location location) { } kieSession.dispose(); + log.info("noticePolicies:: result: {}", () -> asJson(array.stream().toList())); return array; } @@ -223,6 +240,7 @@ public JsonArray noticePolicies(MultiMap params, Location location) { * @return CirculationRuleMatch object with the name of the loan policy and rule conditions */ public CirculationRuleMatch overduePolicy(MultiMap params, Location location) { + log.debug("overduePolicy:: parameters params: {}, location: {}", params, location); final var match = new Match(); final KieSession kieSession = createSession(params, location, match); @@ -241,6 +259,7 @@ public CirculationRuleMatch overduePolicy(MultiMap params, Location location) { * @return matches, each match has a overduePolicyId and a circulationRuleLine field */ public JsonArray overduePolicies(MultiMap params, Location location) { + log.debug("overduePolicies:: parameters params: {}, location: {}", params, location); final var match = new Match(); final KieSession kieSession = createSession(params, location, match); @@ -256,6 +275,8 @@ public JsonArray overduePolicies(MultiMap params, Location location) { } kieSession.dispose(); + log.info("overduePolicies:: result: {}", () -> asJson(array.stream().toList())); + return array; } @@ -266,6 +287,8 @@ public JsonArray overduePolicies(MultiMap params, Location location) { * @return CirculationRuleMatch object with the name of the loan policy and rule conditions */ public CirculationRuleMatch lostItemPolicy(MultiMap params, Location location) { + log.debug("lostItemPolicy:: parameters params: {}, location: {}", params, location); + final var match = new Match(); final KieSession kieSession = createSession(params, location, match); @@ -284,6 +307,7 @@ public CirculationRuleMatch lostItemPolicy(MultiMap params, Location location) { * @return matches, each match has a lostItemPolicyId and a circulationRuleLine field */ public JsonArray lostItemPolicies(MultiMap params, Location location) { + log.debug("lostItemPolicies:: parameters params: {}, location: {}", params, location); final var match = new Match(); final KieSession kieSession = createSession(params, location, match); @@ -299,6 +323,7 @@ public JsonArray lostItemPolicies(MultiMap params, Location location) { } kieSession.dispose(); + log.info("lostItemPolicies:: result: {}", () -> asJson(array.stream().toList())); return array; } @@ -346,6 +371,7 @@ public void afterMatchFired(AfterMatchFiredEvent event) { RuleImpl rule = (RuleImpl) event.getMatch().getRule(); if (rule.getLhs() != null && rule.getLhs().getChildren() != null) { + log.info("afterMatchFired:: getting rule conditions"); ruleConditionElements = rule.getLhs().getChildren().stream() .map(Object::toString) .map(this::getRuleConditionFromStringRuleRepresentation) diff --git a/src/main/java/org/folio/circulation/rules/ExecutableRules.java b/src/main/java/org/folio/circulation/rules/ExecutableRules.java index 3c0e92b593..5809348651 100644 --- a/src/main/java/org/folio/circulation/rules/ExecutableRules.java +++ b/src/main/java/org/folio/circulation/rules/ExecutableRules.java @@ -31,22 +31,32 @@ public ExecutableRules(String text, Drools drools) { } public Result determineLoanPolicy(RulesExecutionParameters parameters) { + log.debug("determineLoanPolicy:: parameters parameters: {}", parameters); + return determinePolicy(parameters, drools::loanPolicy, "loan policy"); } public Result determineRequestPolicy(RulesExecutionParameters parameters) { + log.debug("determineRequestPolicy:: parameters parameters: {}", parameters); + return determinePolicy(parameters, drools::requestPolicy, "request policy"); } public Result determineNoticePolicy(RulesExecutionParameters parameters) { + log.debug("determineNoticePolicy:: parameters parameters: {}", parameters); + return determinePolicy(parameters, drools::noticePolicy, "notice policy"); } public Result determineLostItemPolicy(RulesExecutionParameters parameters) { + log.debug("determineLostItemPolicy:: parameters parameters: {}", parameters); + return determinePolicy(parameters, drools::lostItemPolicy, "lost item policy"); } public Result determineOverduePolicy(RulesExecutionParameters parameters) { + log.debug("determineOverduePolicy:: parameters parameters: {}", parameters); + return determinePolicy(parameters, drools::overduePolicy, "overdude policy"); } diff --git a/src/main/java/org/folio/circulation/rules/Match.java b/src/main/java/org/folio/circulation/rules/Match.java index e91161229e..8ac885c756 100644 --- a/src/main/java/org/folio/circulation/rules/Match.java +++ b/src/main/java/org/folio/circulation/rules/Match.java @@ -1,8 +1,11 @@ package org.folio.circulation.rules; +import lombok.ToString; + /** * Store the result of a rule match. */ +@ToString public class Match { /** loan policy of the matching rule */ @SuppressWarnings("squid:ClassVariableVisibilityCheck") // Drools directly uses public fields diff --git a/src/main/java/org/folio/circulation/rules/cache/CirculationRulesCache.java b/src/main/java/org/folio/circulation/rules/cache/CirculationRulesCache.java index 2188d0f7d6..81060de3ae 100644 --- a/src/main/java/org/folio/circulation/rules/cache/CirculationRulesCache.java +++ b/src/main/java/org/folio/circulation/rules/cache/CirculationRulesCache.java @@ -42,14 +42,16 @@ public void dropCache() { } private boolean rulesExist(String tenantId) { + log.debug("rulesExist:: parameters tenantId: {}", tenantId); Rules rules = rulesMap.get(tenantId); - + log.info("rulesExist:: result: {}", rules != null); return rules != null; } public CompletableFuture> reloadRules(String tenantId, CollectionResourceClient circulationRulesClient) { - log.info("Reloading rules for tenant {}", tenantId); + + log.debug("reloadRules:: parameters tenant: {}", tenantId); return circulationRulesClient.get() .thenApply(r -> r.map(response -> getRulesAsText(response, tenantId))) @@ -57,11 +59,11 @@ public CompletableFuture> reloadRules(String tenantId, } private static String getRulesAsText(Response response, String tenantId) { - log.info("Fetched rules for tenant {}", tenantId); - + log.debug("getRulesAsText:: parameters tenantId: {}", tenantId); + final var circulationRules = new JsonObject(response.getBody()); var encodeRules = circulationRules.encodePrettily(); - log.info("circulationRules = {}", encodeRules); + log.info("getRulesAsText:: circulationRules: {}", encodeRules); return circulationRules.getString("rulesAsText"); } From 194a208c8768832cd42639c8fb1919daf2d72fbe Mon Sep 17 00:00:00 2001 From: Alexander Kurash Date: Tue, 19 Sep 2023 15:50:01 +0300 Subject: [PATCH 2/2] CIRC-1885 Fix missing pickup locations when Page request is edited (#1324) * CIRC-1885 Initial implementation, no new tests * CIRC-1885 Add a test * CIRC-1885 Add logging --- descriptors/ModuleDescriptor-template.json | 2 +- ramls/circulation.raml | 17 +- .../domain/AllowedServicePointsRequest.java | 30 ++ .../storage/requests/RequestRepository.java | 10 + .../AllowedServicePointsResource.java | 83 ++++- .../services/AllowedServicePointsService.java | 55 ++- .../AllowedServicePointsAPITests.java | 325 +++++++++++------- 7 files changed, 378 insertions(+), 144 deletions(-) diff --git a/descriptors/ModuleDescriptor-template.json b/descriptors/ModuleDescriptor-template.json index b0febcfd7f..a12c9c4097 100644 --- a/descriptors/ModuleDescriptor-template.json +++ b/descriptors/ModuleDescriptor-template.json @@ -629,7 +629,7 @@ }, { "id": "allowed-service-points", - "version": "0.1", + "version": "1.0", "handlers": [ { "methods": [ diff --git a/ramls/circulation.raml b/ramls/circulation.raml index e93e80b952..ff2081a21b 100644 --- a/ramls/circulation.raml +++ b/ramls/circulation.raml @@ -322,15 +322,24 @@ resourceTypes: minProperties: 2 maxProperties: 2 properties: - requester: + operation: + description: "Operation (create, replace or move)" + type: string + enum: [create, replace, move] + required: false + requestId: + description: "Request 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 + requesterId: description: "Requester 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: true - item: + required: false + itemId: description: "Item 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 - instance: + instanceId: 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 diff --git a/src/main/java/org/folio/circulation/domain/AllowedServicePointsRequest.java b/src/main/java/org/folio/circulation/domain/AllowedServicePointsRequest.java index 6da222245e..cbdee9b6ed 100644 --- a/src/main/java/org/folio/circulation/domain/AllowedServicePointsRequest.java +++ b/src/main/java/org/folio/circulation/domain/AllowedServicePointsRequest.java @@ -1,5 +1,10 @@ package org.folio.circulation.domain; +import java.lang.invoke.MethodHandles; + +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; + import lombok.AllArgsConstructor; import lombok.Getter; import lombok.ToString; @@ -8,7 +13,32 @@ @Getter @ToString public class AllowedServicePointsRequest { + private static final Logger log = LogManager.getLogger(MethodHandles.lookup().lookupClass()); + + private Request.Operation operation; private String requesterId; private String instanceId; private String itemId; + private String requestId; + + public void updateWithRequestInformation(Request request) { + log.debug("updateWithRequestInformation:: parameters request: {}", request); + + if (request != null) { + log.info("updateWithRequestInformation:: request in not null"); + this.requesterId = request.getRequesterId(); + + if (request.isItemLevel()) { + this.itemId = request.getItemId(); + } + + if (request.isTitleLevel()) { + this.instanceId = request.getInstanceId(); + } + } + } + + public boolean isImplyingItemStatusIgnore() { + return operation == Request.Operation.REPLACE; + } } diff --git a/src/main/java/org/folio/circulation/infrastructure/storage/requests/RequestRepository.java b/src/main/java/org/folio/circulation/infrastructure/storage/requests/RequestRepository.java index 0ca34ea1e7..3377e6f7b9 100644 --- a/src/main/java/org/folio/circulation/infrastructure/storage/requests/RequestRepository.java +++ b/src/main/java/org/folio/circulation/infrastructure/storage/requests/RequestRepository.java @@ -79,6 +79,13 @@ public RequestRepository(org.folio.circulation.support.Clients clients, loanRepository, servicePointRepository, patronGroupRepository, new InstanceRepository(clients)); } + /** + * Simplified constructor for fetching request records only, ignoring related records + */ + public RequestRepository(org.folio.circulation.support.Clients clients) { + this(clients, null, null, null, null, null); + } + private RequestRepository(Clients clients, ItemRepository itemRepository, UserRepository userRepository, LoanRepository loanRepository, ServicePointRepository servicePointRepository, @@ -173,7 +180,10 @@ private CompletableFuture> exists(String id) { public CompletableFuture> getById(String id) { return fetchRequest(id) .thenCompose(r -> r.after(this::fetchRelatedRecords)); + } + public CompletableFuture> getByIdIgnoringRelatedRecords(String id) { + return fetchRequest(id); } public CompletableFuture> fetchRelatedRecords(Request request) { diff --git a/src/main/java/org/folio/circulation/resources/AllowedServicePointsResource.java b/src/main/java/org/folio/circulation/resources/AllowedServicePointsResource.java index 8ccd74c10d..03a8b4c130 100644 --- a/src/main/java/org/folio/circulation/resources/AllowedServicePointsResource.java +++ b/src/main/java/org/folio/circulation/resources/AllowedServicePointsResource.java @@ -16,6 +16,7 @@ import org.apache.logging.log4j.Logger; import org.folio.circulation.domain.AllowedServicePoint; import org.folio.circulation.domain.AllowedServicePointsRequest; +import org.folio.circulation.domain.Request; import org.folio.circulation.domain.RequestType; import org.folio.circulation.services.AllowedServicePointsService; import org.folio.circulation.support.BadRequestFailure; @@ -60,32 +61,35 @@ private void get(RoutingContext routingContext) { private static Result buildRequest(RoutingContext routingContext) { MultiMap queryParams = routingContext.queryParams(); - AllowedServicePointsRequest request = new AllowedServicePointsRequest( - queryParams.get("requester"), - queryParams.get("instance"), - queryParams.get("item")); - return validateRequest(request); + Request.Operation operation = queryParams.get("operation") == null ? null + : Request.Operation.valueOf(queryParams.get("operation").toUpperCase()); + + AllowedServicePointsRequest request = new AllowedServicePointsRequest(operation, + queryParams.get("requesterId"), queryParams.get("instanceId"), queryParams.get("itemId"), + queryParams.get("requestId")); + + return validateAllowedServicePointsRequest(request); } - private static Result validateRequest( - AllowedServicePointsRequest request) { + private static Result validateAllowedServicePointsRequest( + AllowedServicePointsRequest allowedServicePointsRequest) { + + log.debug("validateAllowedServicePointsRequest:: parameters allowedServicePointsRequest: {}", + allowedServicePointsRequest); - log.debug("validateRequest:: parameters: request={}", request); + 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<>(); - String requesterId = request.getRequesterId(); - String instanceId = request.getInstanceId(); - String itemId = request.getItemId(); - if (requesterId == null) { - errors.add("Request query parameters must contain 'requester'."); - } else if (!isUuid(requesterId)) { - errors.add(String.format("Requester ID is not a valid UUID: %s.", requesterId)); - } + // Checking UUID validity - if (instanceId == null ^ itemId != null) { - errors.add("Request query parameters must contain either 'instance' or 'item'."); + if (requesterId != null && !isUuid(requesterId)) { + errors.add(String.format("Requester ID is not a valid UUID: %s.", requesterId)); } if (instanceId != null && !isUuid(instanceId)) { @@ -96,13 +100,54 @@ private static Result validateRequest( errors.add(String.format("Item ID is not a valid UUID: %s.", itemId)); } + if (requestId != null && !isUuid(requestId)) { + errors.add(String.format("Request ID is not a valid UUID: %s.", requestId)); + } + + // Checking parameter combinations + + boolean allowedCombinationOfParametersDetected = false; + + if (operation == Request.Operation.CREATE && requesterId != null && instanceId != null && + itemId == null && requestId == null) { + + 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(request); + 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 7bd48cde7e..6e6ce2de09 100644 --- a/src/main/java/org/folio/circulation/services/AllowedServicePointsService.java +++ b/src/main/java/org/folio/circulation/services/AllowedServicePointsService.java @@ -3,6 +3,9 @@ import static java.lang.String.format; import static java.util.function.UnaryOperator.identity; import static java.util.stream.Collectors.toMap; +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.folio.circulation.support.AsyncCoordinationUtil.allOf; import static org.folio.circulation.support.results.Result.failed; import static org.folio.circulation.support.results.Result.ofAsync; @@ -20,6 +23,7 @@ import java.util.Set; import java.util.UUID; import java.util.concurrent.CompletableFuture; +import java.util.function.BiFunction; import java.util.stream.Collectors; import org.apache.logging.log4j.LogManager; @@ -27,6 +31,7 @@ import org.folio.circulation.domain.AllowedServicePoint; import org.folio.circulation.domain.AllowedServicePointsRequest; import org.folio.circulation.domain.Item; +import org.folio.circulation.domain.Request; import org.folio.circulation.domain.RequestType; import org.folio.circulation.domain.RequestTypeItemStatusWhiteList; import org.folio.circulation.domain.User; @@ -34,6 +39,7 @@ import org.folio.circulation.infrastructure.storage.ServicePointRepository; import org.folio.circulation.infrastructure.storage.inventory.ItemRepository; import org.folio.circulation.infrastructure.storage.requests.RequestPolicyRepository; +import org.folio.circulation.infrastructure.storage.requests.RequestRepository; import org.folio.circulation.infrastructure.storage.users.UserRepository; import org.folio.circulation.storage.ItemByInstanceIdFinder; import org.folio.circulation.support.Clients; @@ -45,6 +51,7 @@ public class AllowedServicePointsService { private static final Logger log = LogManager.getLogger(MethodHandles.lookup().lookupClass()); private final ItemRepository itemRepository; private final UserRepository userRepository; + private final RequestRepository requestRepository; private final RequestPolicyRepository requestPolicyRepository; private final ServicePointRepository servicePointRepository; private final ItemByInstanceIdFinder itemFinder; @@ -52,6 +59,7 @@ public class AllowedServicePointsService { public AllowedServicePointsService(Clients clients) { itemRepository = new ItemRepository(clients); userRepository = new UserRepository(clients); + requestRepository = new RequestRepository(clients); requestPolicyRepository = new RequestPolicyRepository(clients); servicePointRepository = new ServicePointRepository(clients); itemFinder = new ItemByInstanceIdFinder(clients.holdingsStorage(), itemRepository); @@ -62,11 +70,26 @@ public AllowedServicePointsService(Clients clients) { log.debug("getAllowedServicePoints:: parameters request: {}", request); - return userRepository.getUser(request.getRequesterId()) + return fetchRequestIfNeeded(request) + .thenCompose(r -> r.after(f -> userRepository.getUser(request.getRequesterId()))) .thenApply(r -> r.next(user -> refuseIfUserIsNotFound(request, user))) .thenCompose(r -> r.after(user -> getAllowedServicePoints(request, user))); } + private CompletableFuture> fetchRequestIfNeeded( + AllowedServicePointsRequest allowedServicePointsRequest) { + + if (allowedServicePointsRequest.getRequestId() != null) { + log.info("fetchRequestIfNeeded:: requestId is no null, fetching request"); + + return requestRepository.getByIdIgnoringRelatedRecords(allowedServicePointsRequest.getRequestId()) + .thenApply(r -> r.peek(allowedServicePointsRequest::updateWithRequestInformation)) + .thenApply(r -> succeeded(allowedServicePointsRequest)); + } + + return ofAsync(allowedServicePointsRequest); + } + private Result refuseIfUserIsNotFound(AllowedServicePointsRequest request, User user) { if (user == null) { log.error("refuseIfUserIsNotFound:: user is null"); @@ -83,9 +106,15 @@ private Result refuseIfUserIsNotFound(AllowedServicePointsRequest request, log.debug("getAllowedServicePoints:: parameters request: {}, user: {}", request, user); + BiFunction, CompletableFuture>>>> mappingFunction = request.isImplyingItemStatusIgnore() + ? this::extractAllowedServicePointsIgnoringItemStatus + : this::extractAllowedServicePointsConsideringItemStatus; + return fetchItemsAndLookupRequestPolicies(request, user) - .thenCompose(r -> r.after(policies -> allOf(policies, this::extractAllowedServicePoints))) + .thenCompose(r -> r.after(policies -> allOf(policies, mappingFunction))) .thenApply(r -> r.map(this::combineAllowedServicePoints)); + // TODO: remove irrelevant request types for REPLACE } private CompletableFuture>>> fetchItemsAndLookupRequestPolicies( @@ -134,9 +163,23 @@ private Result refuseIfItemIsNotFound(Item item, AllowedServicePointsReque } private CompletableFuture>>> - extractAllowedServicePoints(RequestPolicy requestPolicy, Set items) { + extractAllowedServicePointsIgnoringItemStatus(RequestPolicy requestPolicy, Set items) { + + return extractAllowedServicePoints(requestPolicy, items, true); + } + + private CompletableFuture>>> + extractAllowedServicePointsConsideringItemStatus(RequestPolicy requestPolicy, Set items) { + + return extractAllowedServicePoints(requestPolicy, items, false); + } + + private CompletableFuture>>> + extractAllowedServicePoints(RequestPolicy requestPolicy, Set items, + boolean ignoreItemStatus) { - log.debug("extractAllowedServicePoints:: parameters requestPolicy: {}", requestPolicy); + log.debug("extractAllowedServicePoints:: parameters requestPolicy: {}, items: {}, " + + "ignoreItemStatus: {}", requestPolicy, items, ignoreItemStatus); List requestTypesAllowedByPolicy = Arrays.stream(RequestType.values()) .filter(requestPolicy::allowsType) @@ -149,7 +192,9 @@ private Result refuseIfItemIsNotFound(Item item, AllowedServicePointsReque var servicePointAllowedByPolicy = requestPolicy.getAllowedServicePoints(); - List requestTypesAllowedByItemStatus = items.stream() + List requestTypesAllowedByItemStatus = ignoreItemStatus + ? List.of(PAGE, RECALL, HOLD) + : items.stream() .map(Item::getStatus) .map(RequestTypeItemStatusWhiteList::getRequestTypesAllowedForItemStatus) .flatMap(Collection::stream) diff --git a/src/test/java/api/requests/AllowedServicePointsAPITests.java b/src/test/java/api/requests/AllowedServicePointsAPITests.java index 2585755cf3..19d338bbd9 100644 --- a/src/test/java/api/requests/AllowedServicePointsAPITests.java +++ b/src/test/java/api/requests/AllowedServicePointsAPITests.java @@ -5,12 +5,20 @@ import static api.support.matchers.AllowedServicePointsMatchers.allowedServicePointMatcher; import static api.support.matchers.JsonObjectMatcher.hasNoJsonPath; import static java.lang.Boolean.TRUE; +import static org.folio.circulation.domain.ItemStatus.AVAILABLE; +import static org.folio.circulation.domain.ItemStatus.CHECKED_OUT; +import static org.folio.circulation.domain.RequestLevel.ITEM; +import static org.folio.circulation.domain.RequestLevel.TITLE; +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.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasSize; +import java.time.ZonedDateTime; import java.util.ArrayList; import java.util.HashMap; import java.util.List; @@ -35,9 +43,10 @@ import api.support.APITests; import api.support.builders.ItemBuilder; +import api.support.builders.RequestBuilder; +import api.support.builders.RequestPolicyBuilder; import api.support.builders.ServicePointBuilder; import api.support.dto.AllowedServicePoint; -import api.support.builders.RequestPolicyBuilder; import api.support.fixtures.policies.PoliciesToActivate; import api.support.http.IndividualResource; import api.support.http.QueryStringParameter; @@ -53,8 +62,8 @@ void cleanUp() { @Test void getFailsWithBadRequestWhenRequesterIdIsNull() { - Response response = get(null, randomId(), null, HttpStatus.SC_BAD_REQUEST); - assertThat(response.getBody(), equalTo("Request query parameters must contain 'requester'.")); + Response response = getCreateOp(null, randomId(), null, HttpStatus.SC_BAD_REQUEST); + assertThat(response.getBody(), equalTo("Invalid combination of query parameters")); } @ParameterizedTest @@ -65,8 +74,8 @@ void getFailsWithBadRequestWhenRequesterIdIsNull() { void getFailsWithBadRequestWhenRequestDoesNotContainExactlyTwoParameters(String requesterId, String instanceId, String itemId) { - Response response = get(requesterId, instanceId, itemId, HttpStatus.SC_BAD_REQUEST); - assertThat(response.getBody(), equalTo("Request query parameters must contain either 'instance' or 'item'.")); + Response response = getCreateOp(requesterId, instanceId, itemId, HttpStatus.SC_BAD_REQUEST); + assertThat(response.getBody(), equalTo("Invalid combination of query parameters")); } @ParameterizedTest @@ -77,16 +86,15 @@ void getFailsWithBadRequestWhenRequestDoesNotContainExactlyTwoParameters(String "not-a-uuid, not-a-uuid, not-a-uuid", }, nullValues={"NULL"}) void getFailsWhenRequestContainsInvalidUUID(String requesterId, String instanceId, String itemId) { - Response response = get(requesterId, instanceId, itemId, HttpStatus.SC_BAD_REQUEST); + Response response = getCreateOp(requesterId, instanceId, itemId, HttpStatus.SC_BAD_REQUEST); assertThat(response.getBody(), containsString("ID is not a valid UUID")); } @Test void getFailsWithMultipleErrors() { - Response response = get(null, "instanceId", "itemId", HttpStatus.SC_BAD_REQUEST); - assertThat(response.getBody(), equalTo("Request query parameters must contain 'requester'. " + - "Request query parameters must contain either 'instance' or 'item'. " + - "Instance ID is not a valid UUID: instanceId. Item ID is not a valid UUID: itemId.")); + Response response = getCreateOp(null, "instanceId", "itemId", HttpStatus.SC_BAD_REQUEST); + assertThat(response.getBody(), equalTo("Instance ID is not a valid UUID: instanceId. " + + "Item ID is not a valid UUID: itemId. Invalid combination of query parameters")); } public static Object[] shouldReturnListOfAllowedServicePointsForRequestParameters() { @@ -99,18 +107,18 @@ public static Object[] shouldReturnListOfAllowedServicePointsForRequestParameter List oneAndTwo = List.of(sp1, sp2); return new Object[][]{ - {RequestType.PAGE, RequestLevel.ITEM, ItemStatus.AVAILABLE, oneAndTwo, oneAndTwo}, - {RequestType.HOLD, RequestLevel.ITEM, ItemStatus.AVAILABLE, oneAndTwo, none}, - {RequestType.RECALL, RequestLevel.ITEM, ItemStatus.AVAILABLE, oneAndTwo, none}, - {RequestType.PAGE, RequestLevel.TITLE, ItemStatus.AVAILABLE, oneAndTwo, oneAndTwo}, - {RequestType.HOLD, RequestLevel.TITLE, ItemStatus.AVAILABLE, oneAndTwo, none}, - {RequestType.RECALL, RequestLevel.TITLE, ItemStatus.AVAILABLE, oneAndTwo, none}, - {RequestType.PAGE, RequestLevel.ITEM, ItemStatus.CHECKED_OUT, oneAndTwo, none}, - {RequestType.HOLD, RequestLevel.ITEM, ItemStatus.CHECKED_OUT, oneAndTwo, oneAndTwo}, - {RequestType.RECALL, RequestLevel.ITEM, ItemStatus.CHECKED_OUT, oneAndTwo, oneAndTwo}, - {RequestType.PAGE, RequestLevel.TITLE, ItemStatus.CHECKED_OUT, oneAndTwo, none}, - {RequestType.HOLD, RequestLevel.TITLE, ItemStatus.CHECKED_OUT, oneAndTwo, oneAndTwo}, - {RequestType.RECALL, RequestLevel.TITLE, ItemStatus.CHECKED_OUT, oneAndTwo, oneAndTwo}, + {PAGE, ITEM, AVAILABLE, oneAndTwo, oneAndTwo}, + {HOLD, ITEM, AVAILABLE, oneAndTwo, none}, + {RECALL, ITEM, AVAILABLE, oneAndTwo, none}, + {PAGE, TITLE, AVAILABLE, oneAndTwo, oneAndTwo}, + {HOLD, TITLE, AVAILABLE, oneAndTwo, none}, + {RECALL, TITLE, AVAILABLE, oneAndTwo, none}, + {PAGE, ITEM, CHECKED_OUT, oneAndTwo, none}, + {HOLD, ITEM, CHECKED_OUT, oneAndTwo, oneAndTwo}, + {RECALL, ITEM, CHECKED_OUT, oneAndTwo, oneAndTwo}, + {PAGE, TITLE, CHECKED_OUT, oneAndTwo, none}, + {HOLD, TITLE, CHECKED_OUT, oneAndTwo, oneAndTwo}, + {RECALL, TITLE, CHECKED_OUT, oneAndTwo, oneAndTwo}, }; } @@ -137,9 +145,78 @@ void shouldReturnListOfAllowedServicePointsForRequest(RequestType requestType, .map(UUID::fromString) .collect(Collectors.toSet())); - var response = requestLevel == RequestLevel.TITLE - ? get(requesterId, instanceId, null, HttpStatus.SC_OK).getJson() - : get(requesterId, null, itemId, HttpStatus.SC_OK).getJson(); + var response = requestLevel == TITLE + ? 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))); + } + + public static Object[] shouldReturnListOfAllowedServicePointsForRequestReplacementParameters() { + String sp1Id = randomId(); + String sp2Id = randomId(); + var sp1 = new AllowedServicePoint(sp1Id, "SP One"); + var sp2 = new AllowedServicePoint(sp2Id, "SP Two"); + + List none = List.of(); + List oneAndTwo = List.of(sp1, sp2); + + return new Object[][]{ + {PAGE, ITEM, AVAILABLE, oneAndTwo, oneAndTwo}, + {PAGE, TITLE, AVAILABLE, oneAndTwo, oneAndTwo}, + {HOLD, ITEM, CHECKED_OUT, oneAndTwo, oneAndTwo}, + {RECALL, ITEM, CHECKED_OUT, oneAndTwo, oneAndTwo}, + {HOLD, TITLE, CHECKED_OUT, oneAndTwo, oneAndTwo}, + {RECALL, TITLE, CHECKED_OUT, oneAndTwo, oneAndTwo}, + }; + } + + @ParameterizedTest + @MethodSource("shouldReturnListOfAllowedServicePointsForRequestReplacementParameters") + void shouldReturnListOfAllowedServicePointsForRequestReplacement( + RequestType requestType, RequestLevel requestLevel, ItemStatus itemStatus, + List allowedSpByPolicy, List allowedSpInResponse) { + + var requesterId = usersFixture.steve().getId().toString(); + var items = itemsFixture.createMultipleItemForTheSameInstance(1, + List.of(ib -> ib.withStatus(itemStatus.getValue()))); + var item = items.get(0); + var itemId = item.getId().toString(); + var holdingsRecordId = item.getHoldingsRecordId().toString(); + var instanceId = item.getInstanceId().toString(); + + allowedSpByPolicy.forEach(sp -> servicePointsFixture.create(servicePointBuilder() + .withId(UUID.fromString(sp.getId())) + .withName(sp.getName()) + )); + + setRequestPolicyWithAllowedServicePoints(requestType, allowedSpByPolicy.stream() + .map(AllowedServicePoint::getId) + .map(UUID::fromString) + .collect(Collectors.toSet())); + + configurationsFixture.enableTlrFeature(); + var pickupLocationId = allowedSpByPolicy.stream() + .findFirst() + .map(AllowedServicePoint::getId) + .map(UUID::fromString) + .orElse(null); + + IndividualResource request = requestsFixture.place(new RequestBuilder() + .withRequestType(requestType.toString()) + .fulfillToHoldShelf() + .withRequestLevel(requestLevel.toString()) + .withInstanceId(UUID.fromString(instanceId)) + .withItemId(requestLevel == TITLE ? null : UUID.fromString(itemId)) + .withHoldingsRecordId(requestLevel == TITLE ? null : UUID.fromString(holdingsRecordId)) + .withRequestDate(ZonedDateTime.now()) + .withRequesterId(UUID.fromString(requesterId)) + .withPickupServicePointId(pickupLocationId)); + + var requestId = request == null ? null : request.getId().toString(); + + var response = + get("replace", null, null, null, requestId, HttpStatus.SC_OK).getJson(); assertThat(response, allowedServicePointMatcher(Map.of(requestType, allowedSpInResponse))); } @@ -156,18 +233,18 @@ public static Object[] shouldReturnOnlyExistingAllowedServicePointForRequestPara List oneAndTwo = List.of(sp1, sp2); return new Object[][]{ - {RequestType.PAGE, RequestLevel.ITEM, ItemStatus.AVAILABLE, oneAndTwo, two}, - {RequestType.HOLD, RequestLevel.ITEM, ItemStatus.AVAILABLE, oneAndTwo, none}, - {RequestType.RECALL, RequestLevel.ITEM, ItemStatus.AVAILABLE, oneAndTwo, none}, - {RequestType.PAGE, RequestLevel.TITLE, ItemStatus.AVAILABLE, oneAndTwo, two}, - {RequestType.HOLD, RequestLevel.TITLE, ItemStatus.AVAILABLE, oneAndTwo, none}, - {RequestType.RECALL, RequestLevel.TITLE, ItemStatus.AVAILABLE, oneAndTwo, none}, - {RequestType.PAGE, RequestLevel.ITEM, ItemStatus.CHECKED_OUT, oneAndTwo, none}, - {RequestType.HOLD, RequestLevel.ITEM, ItemStatus.CHECKED_OUT, oneAndTwo, two}, - {RequestType.RECALL, RequestLevel.ITEM, ItemStatus.CHECKED_OUT, oneAndTwo, two}, - {RequestType.PAGE, RequestLevel.TITLE, ItemStatus.CHECKED_OUT, oneAndTwo, none}, - {RequestType.HOLD, RequestLevel.TITLE, ItemStatus.CHECKED_OUT, oneAndTwo, two}, - {RequestType.RECALL, RequestLevel.TITLE, ItemStatus.CHECKED_OUT, oneAndTwo, two}, + {PAGE, ITEM, AVAILABLE, oneAndTwo, two}, + {HOLD, ITEM, AVAILABLE, oneAndTwo, none}, + {RECALL, ITEM, AVAILABLE, oneAndTwo, none}, + {PAGE, TITLE, AVAILABLE, oneAndTwo, two}, + {HOLD, TITLE, AVAILABLE, oneAndTwo, none}, + {RECALL, TITLE, AVAILABLE, oneAndTwo, none}, + {PAGE, ITEM, CHECKED_OUT, oneAndTwo, none}, + {HOLD, ITEM, CHECKED_OUT, oneAndTwo, two}, + {RECALL, ITEM, CHECKED_OUT, oneAndTwo, two}, + {PAGE, TITLE, CHECKED_OUT, oneAndTwo, none}, + {HOLD, TITLE, CHECKED_OUT, oneAndTwo, two}, + {RECALL, TITLE, CHECKED_OUT, oneAndTwo, two}, }; } @@ -196,9 +273,9 @@ void shouldReturnOnlyExistingAllowedServicePointForRequest(RequestType requestTy .map(UUID::fromString) .collect(Collectors.toSet())); - var response = requestLevel == RequestLevel.TITLE - ? get(requesterId, instanceId, null, HttpStatus.SC_OK).getJson() - : get(requesterId, null, itemId, HttpStatus.SC_OK).getJson(); + var response = requestLevel == TITLE + ? getCreateOp(requesterId, instanceId, null, HttpStatus.SC_OK).getJson() + : getCreateOp(requesterId, null, itemId, HttpStatus.SC_OK).getJson(); assertThat(response, allowedServicePointMatcher(Map.of( requestType, allowedSpInResponse @@ -218,18 +295,18 @@ void shouldReturnOnlyExistingAllowedServicePointForRequest(RequestType requestTy List oneAndTwo = List.of(sp1, sp2); return new Object[][]{ - {RequestType.PAGE, RequestLevel.ITEM, ItemStatus.AVAILABLE, oneAndTwo, none}, - {RequestType.HOLD, RequestLevel.ITEM, ItemStatus.AVAILABLE, oneAndTwo, none}, - {RequestType.RECALL, RequestLevel.ITEM, ItemStatus.AVAILABLE, oneAndTwo, none}, - {RequestType.PAGE, RequestLevel.TITLE, ItemStatus.AVAILABLE, oneAndTwo, none}, - {RequestType.HOLD, RequestLevel.TITLE, ItemStatus.AVAILABLE, oneAndTwo, none}, - {RequestType.RECALL, RequestLevel.TITLE, ItemStatus.AVAILABLE, oneAndTwo, none}, - {RequestType.PAGE, RequestLevel.ITEM, ItemStatus.CHECKED_OUT, oneAndTwo, none}, - {RequestType.HOLD, RequestLevel.ITEM, ItemStatus.CHECKED_OUT, oneAndTwo, none}, - {RequestType.RECALL, RequestLevel.ITEM, ItemStatus.CHECKED_OUT, oneAndTwo, none}, - {RequestType.PAGE, RequestLevel.TITLE, ItemStatus.CHECKED_OUT, oneAndTwo, none}, - {RequestType.HOLD, RequestLevel.TITLE, ItemStatus.CHECKED_OUT, oneAndTwo, none}, - {RequestType.RECALL, RequestLevel.TITLE, ItemStatus.CHECKED_OUT, oneAndTwo, none}, + {PAGE, ITEM, AVAILABLE, oneAndTwo, none}, + {HOLD, ITEM, AVAILABLE, oneAndTwo, none}, + {RECALL, ITEM, AVAILABLE, oneAndTwo, none}, + {PAGE, TITLE, AVAILABLE, oneAndTwo, none}, + {HOLD, TITLE, AVAILABLE, oneAndTwo, none}, + {RECALL, TITLE, AVAILABLE, oneAndTwo, none}, + {PAGE, ITEM, CHECKED_OUT, oneAndTwo, none}, + {HOLD, ITEM, CHECKED_OUT, oneAndTwo, none}, + {RECALL, ITEM, CHECKED_OUT, oneAndTwo, none}, + {PAGE, TITLE, CHECKED_OUT, oneAndTwo, none}, + {HOLD, TITLE, CHECKED_OUT, oneAndTwo, none}, + {RECALL, TITLE, CHECKED_OUT, oneAndTwo, none}, }; } @@ -251,9 +328,9 @@ void shouldReturnNoAllowedServicePointsIfAllowedServicePointDoesNotExist( .map(UUID::fromString) .collect(Collectors.toSet())); - var response = requestLevel == RequestLevel.TITLE - ? get(requesterId, instanceId, null, HttpStatus.SC_OK).getJson() - : get(requesterId, null, itemId, HttpStatus.SC_OK).getJson(); + var response = requestLevel == TITLE + ? getCreateOp(requesterId, instanceId, null, HttpStatus.SC_OK).getJson() + : getCreateOp(requesterId, null, itemId, HttpStatus.SC_OK).getJson(); assertThat(response, allowedServicePointMatcher(Map.of( requestType, allowedSpInResponse @@ -273,18 +350,18 @@ void shouldReturnNoAllowedServicePointsIfAllowedServicePointDoesNotExist( List oneAndTwo = List.of(sp1, sp2); return new Object[][]{ - {RequestType.PAGE, RequestLevel.ITEM, ItemStatus.AVAILABLE, oneAndTwo, none}, - {RequestType.HOLD, RequestLevel.ITEM, ItemStatus.AVAILABLE, oneAndTwo, none}, - {RequestType.RECALL, RequestLevel.ITEM, ItemStatus.AVAILABLE, oneAndTwo, none}, - {RequestType.PAGE, RequestLevel.TITLE, ItemStatus.AVAILABLE, oneAndTwo, none}, - {RequestType.HOLD, RequestLevel.TITLE, ItemStatus.AVAILABLE, oneAndTwo, none}, - {RequestType.RECALL, RequestLevel.TITLE, ItemStatus.AVAILABLE, oneAndTwo, none}, - {RequestType.PAGE, RequestLevel.ITEM, ItemStatus.CHECKED_OUT, oneAndTwo, none}, - {RequestType.HOLD, RequestLevel.ITEM, ItemStatus.CHECKED_OUT, oneAndTwo, none}, - {RequestType.RECALL, RequestLevel.ITEM, ItemStatus.CHECKED_OUT, oneAndTwo, none}, - {RequestType.PAGE, RequestLevel.TITLE, ItemStatus.CHECKED_OUT, oneAndTwo, none}, - {RequestType.HOLD, RequestLevel.TITLE, ItemStatus.CHECKED_OUT, oneAndTwo, none}, - {RequestType.RECALL, RequestLevel.TITLE, ItemStatus.CHECKED_OUT, oneAndTwo, none}, + {PAGE, ITEM, AVAILABLE, oneAndTwo, none}, + {HOLD, ITEM, AVAILABLE, oneAndTwo, none}, + {RECALL, ITEM, AVAILABLE, oneAndTwo, none}, + {PAGE, TITLE, AVAILABLE, oneAndTwo, none}, + {HOLD, TITLE, AVAILABLE, oneAndTwo, none}, + {RECALL, TITLE, AVAILABLE, oneAndTwo, none}, + {PAGE, ITEM, CHECKED_OUT, oneAndTwo, none}, + {HOLD, ITEM, CHECKED_OUT, oneAndTwo, none}, + {RECALL, ITEM, CHECKED_OUT, oneAndTwo, none}, + {PAGE, TITLE, CHECKED_OUT, oneAndTwo, none}, + {HOLD, TITLE, CHECKED_OUT, oneAndTwo, none}, + {RECALL, TITLE, CHECKED_OUT, oneAndTwo, none}, }; } @@ -312,9 +389,9 @@ void shouldReturnNoAllowedServicePointsIfAllowedServicePointIsNotPickupLocation( .map(UUID::fromString) .collect(Collectors.toSet())); - var response = requestLevel == RequestLevel.TITLE - ? get(requesterId, instanceId, null, HttpStatus.SC_OK).getJson() - : get(requesterId, null, itemId, HttpStatus.SC_OK).getJson(); + var response = requestLevel == TITLE + ? getCreateOp(requesterId, instanceId, null, HttpStatus.SC_OK).getJson() + : getCreateOp(requesterId, null, itemId, HttpStatus.SC_OK).getJson(); assertThat(response, allowedServicePointMatcher(Map.of( requestType, allowedSpInResponse @@ -323,18 +400,18 @@ void shouldReturnNoAllowedServicePointsIfAllowedServicePointIsNotPickupLocation( public static Object[] shouldReturnOnlyExistingServicePointsWhenRequestPolicyDoesNotHaveAnyParameters() { return new Object[][]{ - {RequestType.PAGE, RequestLevel.ITEM, ItemStatus.AVAILABLE, false}, - {RequestType.HOLD, RequestLevel.ITEM, ItemStatus.AVAILABLE, true}, - {RequestType.RECALL, RequestLevel.ITEM, ItemStatus.AVAILABLE, true}, - {RequestType.PAGE, RequestLevel.TITLE, ItemStatus.AVAILABLE, false}, - {RequestType.HOLD, RequestLevel.TITLE, ItemStatus.AVAILABLE, true}, - {RequestType.RECALL, RequestLevel.TITLE, ItemStatus.AVAILABLE, true}, - {RequestType.PAGE, RequestLevel.ITEM, ItemStatus.CHECKED_OUT, true}, - {RequestType.HOLD, RequestLevel.ITEM, ItemStatus.CHECKED_OUT, false}, - {RequestType.RECALL, RequestLevel.ITEM, ItemStatus.CHECKED_OUT, false}, - {RequestType.PAGE, RequestLevel.TITLE, ItemStatus.CHECKED_OUT, true}, - {RequestType.HOLD, RequestLevel.TITLE, ItemStatus.CHECKED_OUT, false}, - {RequestType.RECALL, RequestLevel.TITLE, ItemStatus.CHECKED_OUT, false}, + {PAGE, ITEM, AVAILABLE, false}, + {HOLD, ITEM, AVAILABLE, true}, + {RECALL, ITEM, AVAILABLE, true}, + {PAGE, TITLE, AVAILABLE, false}, + {HOLD, TITLE, AVAILABLE, true}, + {RECALL, TITLE, AVAILABLE, true}, + {PAGE, ITEM, CHECKED_OUT, true}, + {HOLD, ITEM, CHECKED_OUT, false}, + {RECALL, ITEM, CHECKED_OUT, false}, + {PAGE, TITLE, CHECKED_OUT, true}, + {HOLD, TITLE, CHECKED_OUT, false}, + {RECALL, TITLE, CHECKED_OUT, false}, }; } @@ -351,9 +428,9 @@ void shouldReturnOnlyExistingServicePointsWhenRequestPolicyDoesNotHaveAny( var itemId = item.getId().toString(); var instanceId = item.getInstanceId().toString(); - var response = requestLevel == RequestLevel.TITLE - ? get(requesterId, instanceId, null, HttpStatus.SC_OK).getJson() - : get(requesterId, null, itemId, HttpStatus.SC_OK).getJson(); + var response = requestLevel == TITLE + ? getCreateOp(requesterId, instanceId, null, HttpStatus.SC_OK).getJson() + : getCreateOp(requesterId, null, itemId, HttpStatus.SC_OK).getJson(); var allowedServicePoints = response.getJsonArray(requestType.getValue()); var servicePointsWithPickupLocation = servicePointsFixture.getAllServicePoints().stream() @@ -373,9 +450,9 @@ void shouldReturnErrorIfRequesterDoesNotExist() { var requesterId = randomId(); var itemId = itemsFixture.basedUponNod().getId().toString(); var cd1Id = servicePointsFixture.cd1().getId(); - setRequestPolicyWithAllowedServicePoints(RequestType.PAGE, Set.of(cd1Id)); + setRequestPolicyWithAllowedServicePoints(PAGE, Set.of(cd1Id)); - Response response = get(requesterId, null, itemId, HttpStatus.SC_UNPROCESSABLE_ENTITY); + Response response = getCreateOp(requesterId, null, itemId, HttpStatus.SC_UNPROCESSABLE_ENTITY); assertThat(response.getBody(), containsString("User with id=" + requesterId + " cannot be found")); } @@ -385,9 +462,9 @@ void shouldReturnErrorIfItemDoesNotExist() { var requesterId = usersFixture.steve().getId().toString(); var itemId = randomId(); var cd1Id = servicePointsFixture.cd1().getId(); - setRequestPolicyWithAllowedServicePoints(RequestType.PAGE, Set.of(cd1Id)); + setRequestPolicyWithAllowedServicePoints(PAGE, Set.of(cd1Id)); - Response response = get(requesterId, null, itemId, HttpStatus.SC_UNPROCESSABLE_ENTITY); + Response response = getCreateOp(requesterId, null, itemId, HttpStatus.SC_UNPROCESSABLE_ENTITY); assertThat(response.getBody(), containsString("Item with id=" + itemId + " cannot be found")); } @@ -397,9 +474,9 @@ void shouldReturnErrorIfInstanceDoesNotExist() { var requesterId = usersFixture.steve().getId().toString(); var instanceId = randomId(); var cd1Id = servicePointsFixture.cd1().getId(); - setRequestPolicyWithAllowedServicePoints(RequestType.PAGE, Set.of(cd1Id)); + setRequestPolicyWithAllowedServicePoints(PAGE, Set.of(cd1Id)); - Response response = get(requesterId, instanceId, null, HttpStatus.SC_UNPROCESSABLE_ENTITY); + Response response = getCreateOp(requesterId, instanceId, null, HttpStatus.SC_UNPROCESSABLE_ENTITY); assertThat(response.getBody(), containsString("There are no holdings for this instance")); } @@ -415,21 +492,21 @@ void shouldReturnListOfAllowedServicePointsForBothTypesOfRequest(RequestLevel re var cd4 = servicePointsFixture.cd4(); var cd5 = servicePointsFixture.cd5(); final Map> allowedServicePointsInPolicy = new HashMap<>(); - allowedServicePointsInPolicy.put(RequestType.PAGE, Set.of(cd1.getId(), cd2.getId())); - allowedServicePointsInPolicy.put(RequestType.HOLD, Set.of(cd4.getId(), cd5.getId())); + allowedServicePointsInPolicy.put(PAGE, Set.of(cd1.getId(), cd2.getId())); + allowedServicePointsInPolicy.put(HOLD, Set.of(cd4.getId(), cd5.getId())); var requestPolicy = requestPoliciesFixture .createRequestPolicyWithAllowedServicePoints(allowedServicePointsInPolicy, - RequestType.PAGE, RequestType.HOLD); + PAGE, HOLD); policiesActivation.use(PoliciesToActivate.builder().requestPolicy(requestPolicy)); - var response = requestLevel == RequestLevel.TITLE - ? get(requesterId, instanceId, null, HttpStatus.SC_OK).getJson() - : get(requesterId, null, itemId, HttpStatus.SC_OK).getJson(); + var response = requestLevel == TITLE + ? getCreateOp(requesterId, instanceId, null, HttpStatus.SC_OK).getJson() + : getCreateOp(requesterId, null, itemId, HttpStatus.SC_OK).getJson(); - assertThat(response, hasNoJsonPath(RequestType.HOLD.getValue())); - assertThat(response, hasNoJsonPath(RequestType.RECALL.getValue())); + assertThat(response, hasNoJsonPath(HOLD.getValue())); + assertThat(response, hasNoJsonPath(RECALL.getValue())); - final JsonArray allowedPageServicePoints = response.getJsonArray(RequestType.PAGE.getValue()); + final JsonArray allowedPageServicePoints = response.getJsonArray(PAGE.getValue()); assertServicePointsMatch(allowedPageServicePoints, List.of(cd1, cd2)); } @@ -448,11 +525,11 @@ void shouldReturnAllowedServicePointsForAllEnabledRequestTypes() { var cd1 = servicePointsFixture.cd1(); var cd2 = servicePointsFixture.cd2(); final Map> allowedServicePointsInPolicy = new HashMap<>(); - allowedServicePointsInPolicy.put(RequestType.PAGE, Set.of(cd1.getId())); - allowedServicePointsInPolicy.put(RequestType.HOLD, Set.of(cd2.getId())); + allowedServicePointsInPolicy.put(PAGE, Set.of(cd1.getId())); + allowedServicePointsInPolicy.put(HOLD, Set.of(cd2.getId())); policiesActivation.use(new RequestPolicyBuilder( UUID.randomUUID(), - List.of(RequestType.PAGE, RequestType.HOLD, RequestType.RECALL), + List.of(PAGE, HOLD, RECALL), "Test request policy", "Test description", allowedServicePointsInPolicy)); @@ -463,14 +540,14 @@ void shouldReturnAllowedServicePointsForAllEnabledRequestTypes() { // ILR scenario - var responseIlr = get(requesterId, null, itemId, HttpStatus.SC_OK).getJson(); + var responseIlr = getCreateOp(requesterId, null, itemId, HttpStatus.SC_OK).getJson(); final JsonArray allowedPageServicePointsIlr = - responseIlr.getJsonArray(RequestType.PAGE.getValue()); + responseIlr.getJsonArray(PAGE.getValue()); final JsonArray allowedHoldServicePointsIlr = - responseIlr.getJsonArray(RequestType.HOLD.getValue()); + responseIlr.getJsonArray(HOLD.getValue()); final JsonArray allowedRecallServicePointsIlr = - responseIlr.getJsonArray(RequestType.RECALL.getValue()); + responseIlr.getJsonArray(RECALL.getValue()); assertServicePointsMatch(allowedPageServicePointsIlr, List.of(cd1)); assertThat(allServicePointsWithPickupLocation, hasSize(2)); @@ -479,14 +556,14 @@ void shouldReturnAllowedServicePointsForAllEnabledRequestTypes() { // TLR scenario - var responseTlr = get(requesterId, instanceId, null, HttpStatus.SC_OK).getJson(); + var responseTlr = getCreateOp(requesterId, instanceId, null, HttpStatus.SC_OK).getJson(); final JsonArray allowedPageServicePointsTlr = - responseTlr.getJsonArray(RequestType.PAGE.getValue()); + responseTlr.getJsonArray(PAGE.getValue()); final JsonArray allowedHoldServicePointsTlr = - responseTlr.getJsonArray(RequestType.HOLD.getValue()); + responseTlr.getJsonArray(HOLD.getValue()); final JsonArray allowedRecallServicePointsTlr = - responseTlr.getJsonArray(RequestType.RECALL.getValue()); + responseTlr.getJsonArray(RECALL.getValue()); assertServicePointsMatch(allowedPageServicePointsTlr, List.of(cd1)); assertServicePointsMatch(allowedHoldServicePointsTlr, List.of(cd2)); @@ -516,16 +593,34 @@ private void assertServicePointsMatch(JsonArray response, .map(sp -> sp.getJson().getString("name")).toArray(String[]::new))); } - private Response get(String requesterId, String instanceId, String itemId, int expectedStatusCode) { + private Response getCreateOp(String requesterId, String instanceId, String itemId, + int expectedStatusCode) { + + return get("create", requesterId, instanceId, itemId, null, expectedStatusCode); + } + + private Response getReplaceOp(String requesterId, String instanceId, String itemId, + int expectedStatusCode) { + + return get("replace", requesterId, instanceId, itemId, null, expectedStatusCode); + } + + private Response get(String operation, String requesterId, String instanceId, String itemId, + String requestId, int expectedStatusCode) { + List queryParams = new ArrayList<>(); + queryParams.add(namedParameter("operation", operation)); if (requesterId != null) { - queryParams.add(namedParameter("requester", requesterId)); + queryParams.add(namedParameter("requesterId", requesterId)); } if (instanceId != null) { - queryParams.add(namedParameter("instance", instanceId)); + queryParams.add(namedParameter("instanceId", instanceId)); } if (itemId != null) { - queryParams.add(namedParameter("item", itemId)); + queryParams.add(namedParameter("itemId", itemId)); + } + if (requestId != null) { + queryParams.add(namedParameter("requestId", requestId)); } return restAssuredClient.get(allowedServicePointsUrl(), queryParams, expectedStatusCode,