Skip to content

Commit

Permalink
PP-13030: Use CreateServiceRequest pojo instead of JsonNode (#2653)
Browse files Browse the repository at this point in the history
* PP-13030: Use CreateServiceRequest pojo instead of JsonNode

Following the general pattern in most of our Dropwizard codebases, we want to avoid
JsonNode as the POST request body where possible so we can advantage of using
javax Validation constraints.

It is also generally agreed that using hard coded URL strings makes for
easier-to-read code.

Added a few more test cases in ServiceResourceCreateIT to reflect the pact
tests between selfservice and adminusers:
* [a valid create service request with empty object](https://github.com/alphagov/pay-selfservice/blob/b2053a1049bc28813e8e7363245ad546ef19b675/test/pact/adminusers-client/service/create-service.pact.test.js#L50)
* [a valid create service request with gateway account ids](https://github.com/alphagov/pay-selfservice/blob/b2053a1049bc28813e8e7363245ad546ef19b675/test/pact/adminusers-client/service/create-service.pact.test.js#L85C31-L85C86)
* [a valid create service request with service name](https://github.com/alphagov/pay-selfservice/blob/b2053a1049bc28813e8e7363245ad546ef19b675/test/pact/adminusers-client/service/create-service.pact.test.js#L124)
  • Loading branch information
oswaldquek authored Sep 9, 2024
1 parent f8bfb9f commit 00a0f8a
Show file tree
Hide file tree
Showing 9 changed files with 167 additions and 68 deletions.
20 changes: 8 additions & 12 deletions .secrets.baseline
Original file line number Diff line number Diff line change
Expand Up @@ -75,10 +75,6 @@
{
"path": "detect_secrets.filters.allowlist.is_line_allowlisted"
},
{
"path": "detect_secrets.filters.common.is_baseline_file",
"filename": ".secrets.baseline"
},
{
"path": "detect_secrets.filters.common.is_ignored_due_to_verification_policies",
"min_level": 2
Expand Down Expand Up @@ -139,35 +135,35 @@
"filename": "openapi/adminusers_spec.yaml",
"hashed_secret": "614770647df3ab100b871bfc0d20e72c8625a5c4",
"is_verified": false,
"line_number": 398
"line_number": 400
},
{
"type": "Hex High Entropy String",
"filename": "openapi/adminusers_spec.yaml",
"hashed_secret": "1976a945a8d2733655e4b2453bd49fb59cb7ba19",
"is_verified": false,
"line_number": 663
"line_number": 665
},
{
"type": "Hex High Entropy String",
"filename": "openapi/adminusers_spec.yaml",
"hashed_secret": "a31519136d26754afeb0df49b5f066f387091f88",
"is_verified": false,
"line_number": 697
"line_number": 699
},
{
"type": "Secret Keyword",
"filename": "openapi/adminusers_spec.yaml",
"hashed_secret": "0ea7458942ab65e0a340cf4fd28ca00d93c494f3",
"is_verified": false,
"line_number": 796
"line_number": 798
},
{
"type": "Hex High Entropy String",
"filename": "openapi/adminusers_spec.yaml",
"hashed_secret": "3660e32cde5fccc8d1e4521d0c831c2012388720",
"is_verified": false,
"line_number": 1231
"line_number": 1244
}
],
"src/main/java/uk/gov/pay/adminusers/model/CreateUserRequest.java": [
Expand Down Expand Up @@ -260,14 +256,14 @@
"filename": "src/main/java/uk/gov/pay/adminusers/resources/ServiceResource.java",
"hashed_secret": "614770647df3ab100b871bfc0d20e72c8625a5c4",
"is_verified": false,
"line_number": 149
"line_number": 142
},
{
"type": "Hex High Entropy String",
"filename": "src/main/java/uk/gov/pay/adminusers/resources/ServiceResource.java",
"hashed_secret": "1976a945a8d2733655e4b2453bd49fb59cb7ba19",
"is_verified": false,
"line_number": 383
"line_number": 369
}
],
"src/main/java/uk/gov/pay/adminusers/resources/ToolboxEndpointResource.java": [
Expand Down Expand Up @@ -466,5 +462,5 @@
}
]
},
"generated_at": "2024-09-06T15:07:58Z"
"generated_at": "2024-09-09T09:33:48Z"
}
13 changes: 13 additions & 0 deletions openapi/adminusers_spec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -343,6 +343,8 @@ paths:
schema:
$ref: '#/components/schemas/Service'
description: Created
"400":
description: Service name keys must be one of 'en' (English) or 'cy' (Welsh)
"409":
description: Gateway account IDs provided has already been assigned to another
service
Expand Down Expand Up @@ -1184,6 +1186,17 @@ components:
example: [email protected]
required:
- email
CreateServiceRequest:
type: object
properties:
gateway_account_ids:
type: array
items:
type: string
service_name:
type: object
additionalProperties:
type: string
CreateUserRequest:
type: object
properties:
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package uk.gov.pay.adminusers.model;

import com.fasterxml.jackson.databind.PropertyNamingStrategies;
import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
import com.fasterxml.jackson.databind.annotation.JsonNaming;
import uk.gov.service.payments.commons.model.SupportedLanguage;

import java.util.List;
import java.util.Map;

@JsonNaming(PropertyNamingStrategies.SnakeCaseStrategy.class)
public record CreateServiceRequest(
List<String> gatewayAccountIds,
@JsonDeserialize(using = ServiceNamesDeserializer.class) Map<SupportedLanguage, String> serviceName) {
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
package uk.gov.pay.adminusers.model;

import com.fasterxml.jackson.core.JsonParser;
import com.fasterxml.jackson.core.type.TypeReference;
import com.fasterxml.jackson.databind.DeserializationContext;
import com.fasterxml.jackson.databind.JsonDeserializer;
import uk.gov.service.payments.commons.model.SupportedLanguage;

import java.io.IOException;
import java.util.Map;

import static java.util.stream.Collectors.toUnmodifiableMap;

public class ServiceNamesDeserializer extends JsonDeserializer<Map<SupportedLanguage, String>> {
@Override
public Map<SupportedLanguage, String> deserialize(JsonParser jsonParser, DeserializationContext deserializationContext) throws IOException {
return jsonParser.getCodec().readValue(jsonParser, new TypeReference<Map<String, String>>() {})
.entrySet().stream()
.collect(toUnmodifiableMap(entry -> SupportedLanguage.fromIso639AlphaTwoCode(entry.getKey()), Map.Entry::getValue));
}
}
44 changes: 9 additions & 35 deletions src/main/java/uk/gov/pay/adminusers/resources/ServiceResource.java
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import uk.gov.pay.adminusers.exception.ServiceNotFoundException;
import uk.gov.pay.adminusers.model.CreateServiceRequest;
import uk.gov.pay.adminusers.model.GovUkPayAgreement;
import uk.gov.pay.adminusers.model.SearchServicesResponse;
import uk.gov.pay.adminusers.model.Service;
Expand All @@ -33,7 +34,6 @@
import uk.gov.pay.adminusers.service.StripeAgreementService;
import uk.gov.pay.adminusers.utils.Errors;
import uk.gov.service.payments.commons.api.exception.ValidationException;
import uk.gov.service.payments.commons.model.SupportedLanguage;

import javax.validation.Valid;
import javax.validation.constraints.NotNull;
Expand All @@ -53,13 +53,9 @@
import java.net.UnknownHostException;
import java.time.ZoneOffset;
import java.time.ZonedDateTime;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.stream.Collectors;
import java.util.stream.Stream;

import static java.util.stream.Collectors.toUnmodifiableList;
import static javax.ws.rs.core.MediaType.APPLICATION_JSON;
Expand All @@ -71,15 +67,12 @@
import static javax.ws.rs.core.Response.Status.NO_CONTENT;
import static javax.ws.rs.core.Response.Status.OK;
import static org.apache.commons.lang3.StringUtils.isBlank;
import static uk.gov.pay.adminusers.resources.ServiceResource.SERVICES_RESOURCE;
import static uk.gov.pay.adminusers.service.ServiceUpdater.FIELD_GATEWAY_ACCOUNT_IDS;

@Path(SERVICES_RESOURCE)
@Path("/v1/api/services")
public class ServiceResource {

private static final Logger LOGGER = LoggerFactory.getLogger(ServiceResource.class);
/* default */static final String HEADER_USER_CONTEXT = "GovUkPay-User-Context";
public static final String SERVICES_RESOURCE = "/v1/api/services";

public static final String FIELD_NAME = "name";

Expand Down Expand Up @@ -227,28 +220,21 @@ public Response searchServices(JsonNode payload) {
@ApiResponse(responseCode = "201", description = "Created",
content = @Content(schema = @Schema(implementation = Service.class))),
@ApiResponse(responseCode = "409", description = "Gateway account IDs provided has already been assigned to another service"),
@ApiResponse(responseCode = "400", description = "Service name keys must be one of 'en' (English) or 'cy' (Welsh)"),
@ApiResponse(responseCode = "500", description = "Invalid JSON payload")
}
)
public Response createService(JsonNode payload) {
LOGGER.info("Create Service POST request - [ {} ]", payload);
List<String> gatewayAccountIds = extractGatewayAccountIds(payload);
Map<SupportedLanguage, String> serviceNameVariants = getServiceNameVariants(payload);
public Response createService(@Valid CreateServiceRequest createServiceRequest) {
LOGGER.info("Create Service POST request - [ {} ]", createServiceRequest);

Service service = serviceServicesFactory.serviceCreator().doCreate(gatewayAccountIds, serviceNameVariants);
var nullableCreateServiceRequest = Optional.ofNullable(createServiceRequest);
Service service = serviceServicesFactory.serviceCreator().doCreate(
nullableCreateServiceRequest.map(CreateServiceRequest::gatewayAccountIds).orElse(List.of()),
nullableCreateServiceRequest.map(CreateServiceRequest::serviceName).orElse(Map.of()));
return Response.status(CREATED).entity(service).build();

}

private List<String> extractGatewayAccountIds(JsonNode payload) {
List<String> gatewayAccountIds = new ArrayList<>();
if (payload != null && payload.get(FIELD_GATEWAY_ACCOUNT_IDS) != null) {
payload.get(FIELD_GATEWAY_ACCOUNT_IDS)
.elements().forEachRemaining((node) -> gatewayAccountIds.add(node.textValue()));
}
return List.copyOf(gatewayAccountIds);
}

@Path("/{serviceExternalId}")
@PATCH
@Produces(APPLICATION_JSON)
Expand Down Expand Up @@ -533,16 +519,4 @@ public Response sendLiveAccountCreatedEmail(@Parameter(example = "7d19aff33f8948
sendLiveAccountCreatedEmailService.sendEmail(serviceExternalId);
return Response.status(OK).build();
}

private Map<SupportedLanguage, String> getServiceNameVariants(JsonNode payload) {
if (payload.hasNonNull("service_name")) {
JsonNode serviceName = payload.get("service_name");
return Stream.of(SupportedLanguage.values())
.filter(supportedLanguage -> serviceName.hasNonNull(supportedLanguage.toString()))
.collect(Collectors.toUnmodifiableMap(
supportedLanguage -> supportedLanguage,
supportedLanguage -> serviceName.get(supportedLanguage.toString()).asText()));
}
return Collections.emptyMap();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@

import static javax.ws.rs.core.UriBuilder.fromUri;
import static uk.gov.pay.adminusers.resources.ForgottenPasswordResource.FORGOTTEN_PASSWORDS_RESOURCE;
import static uk.gov.pay.adminusers.resources.ServiceResource.SERVICES_RESOURCE;
import static uk.gov.pay.adminusers.resources.UserResource.USERS_RESOURCE;

public class LinksBuilder {
Expand All @@ -32,7 +31,7 @@ public User decorate(User user) {
}

public Service decorate(Service service) {
URI uri = fromUri(baseUrl).path(SERVICES_RESOURCE).path(String.valueOf(service.getExternalId()))
URI uri = fromUri(baseUrl).path("/v1/api/services").path(String.valueOf(service.getExternalId()))
.build();
Link selfLink = Link.from(Rel.SELF, "GET", uri.toString());
service.setLinks(List.of(selfLink));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ public Response sendEmail() {
@BeforeEach
public void initialise() {
databaseHelper = APP.getDatabaseTestHelper();
databaseHelper.truncateAllData();
}

protected RequestSpecification givenSetup() {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,37 +1,117 @@
package uk.gov.pay.adminusers.resources;

import com.fasterxml.jackson.databind.JsonNode;
import io.restassured.response.ValidatableResponse;
import org.junit.jupiter.api.DisplayNameGeneration;
import org.junit.jupiter.api.DisplayNameGenerator;
import org.junit.jupiter.api.Test;
import uk.gov.service.payments.commons.model.SupportedLanguage;

import java.util.List;
import java.util.Map;

import static io.restassured.http.ContentType.JSON;
import static org.hamcrest.Matchers.emptyIterable;
import static org.hamcrest.Matchers.hasEntry;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.not;
import static org.hamcrest.Matchers.nullValue;
import static uk.gov.service.payments.commons.model.SupportedLanguage.ENGLISH;
import static uk.gov.service.payments.commons.model.SupportedLanguage.WELSH;

@DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class)
class ServiceResourceCreateIT extends IntegrationTest {

@Test
void shouldCreateService() {
JsonNode payload = mapper
.valueToTree(Map.of(
"service_name", Map.of(SupportedLanguage.ENGLISH.toString(), "Service name"),
"gateway_account_ids", List.of("1")
));

givenSetup()
void create_service_with_all_parameters_successfully() {
var validatableResponse = givenSetup()
.when()
.contentType(JSON)
.body(payload)
.body(Map.of("service_name", Map.of(ENGLISH.toString(), "Service name"),
"gateway_account_ids", List.of("1")))
.post("v1/api/services")
.then()
.statusCode(201)
.body("created_date", is(not(nullValue())))
.body("gateway_account_ids", is(List.of("1")))
.body("service_name", hasEntry("en", "Service name"));

assertStandardFields(validatableResponse);
}

@Test
void can_create_default_service_with_empty_request_body() {
var validatableResponse = givenSetup()
.when()
.contentType(JSON)
.post("v1/api/services")
.then()
.statusCode(201)
.body("created_date", is(not(nullValue())))
.body("gateway_account_ids", is(emptyIterable()))
.body("service_name", hasEntry("en", "System Generated"))
.body("name", is("System Generated"));

assertStandardFields(validatableResponse);
}

@Test
void can_create_default_service_with_gateway_account_ids_only() {
var validatableResponse = givenSetup()
.when()
.contentType(JSON)
.body(Map.of("gateway_account_ids", List.of("1", "2")))
.post("v1/api/services")
.then()
.statusCode(201)
.body("created_date", is(not(nullValue())))
.body("gateway_account_ids", is(List.of("1", "2")))
.body("service_name", hasEntry("en", "System Generated"))
.body("name", is("System Generated"));

assertStandardFields(validatableResponse);
}

@Test
void can_create_default_service_with_service_name_only() {
var validatableResponse = givenSetup()
.when()
.contentType(JSON)
.body(Map.of("service_name", Map.of(ENGLISH.toString(), "Service name", WELSH.toString(), "Welsh name")))
.post("v1/api/services")
.then()
.statusCode(201)
.body("created_date", is(not(nullValue())))
.body("gateway_account_ids", is(emptyIterable()))
.body("service_name", hasEntry("en", "Service name"))
.body("service_name", hasEntry("cy", "Welsh name"))
.body("name", is("Service name"));

assertStandardFields(validatableResponse);
}

private void assertStandardFields(ValidatableResponse validatableResponse) {
validatableResponse
.body("current_psp_test_account_stage", is("NOT_STARTED"))
.body("current_go_live_stage", is("NOT_STARTED"))
.body("default_billing_address_country", is("GB"))
.body("agent_initiated_moto_enabled", is(false))
.body("takes_payments_over_phone", is(false))
.body("experimental_features_enabled", is(false))
.body("internal", is(false))
.body("archived", is(false))
.body("redirect_to_service_immediately_on_terminal_state", is(false))
.body("collect_billing_address", is(true));
}

@Test
void return_bad_request_when_invalid_supported_language_provided() {
givenSetup()
.when()
.contentType(JSON)
.body(Map.of("service_name", Map.of("fr", "Service name")))
.post("v1/api/services")
.then()
.statusCode(400)
.body("message", is("Unable to process JSON"))
.body("details", is("fr is not a supported ISO 639-1 code"));
}
}
Loading

0 comments on commit 00a0f8a

Please sign in to comment.