From 74be18e9a5e0b927cc0c8ef04cbbd079d0e7d62b Mon Sep 17 00:00:00 2001 From: Tim Harrison Date: Fri, 21 Feb 2025 14:16:40 +0000 Subject: [PATCH 1/2] PRC-439: Integration tests and handling conflicts --- .../hmpps-organisations-api/values.yaml | 4 + .../OrganisationsApiExceptionHandler.kt | 13 + .../sync/SyncOrganisationController.kt | 4 +- .../StubOutboundEventsPublisher.kt | 4 +- .../sync/SyncOrganisationsIntegrationTest.kt | 323 ++++++++++++++++++ 5 files changed, 345 insertions(+), 3 deletions(-) create mode 100644 src/test/kotlin/uk/gov/justice/digital/hmpps/organisationsapi/integration/resource/sync/SyncOrganisationsIntegrationTest.kt diff --git a/helm_deploy/hmpps-organisations-api/values.yaml b/helm_deploy/hmpps-organisations-api/values.yaml index 265c84a..4c105f3 100644 --- a/helm_deploy/hmpps-organisations-api/values.yaml +++ b/helm_deploy/hmpps-organisations-api/values.yaml @@ -23,6 +23,10 @@ generic-service: SERVER_PORT: "8080" APPLICATIONINSIGHTS_CONFIGURATION_FILE: applicationinsights.json DB_SSL_MODE: "verify-full" + FEATURE_EVENTS_SNS_ENABLED: true + FEATURE_EVENT_ORGANISATIONS_API_ORGANISATION_CREATED: true + FEATURE_EVENT_ORGANISATIONS_API_ORGANISATION_UPDATED: true + FEATURE_EVENT_ORGANISATIONS_API_ORGANISATION_DELETED: true # Pre-existing kubernetes secrets to load as environment variables in the deployment. # namespace_secrets: diff --git a/src/main/kotlin/uk/gov/justice/digital/hmpps/organisationsapi/config/OrganisationsApiExceptionHandler.kt b/src/main/kotlin/uk/gov/justice/digital/hmpps/organisationsapi/config/OrganisationsApiExceptionHandler.kt index a6675d4..bf5cc80 100644 --- a/src/main/kotlin/uk/gov/justice/digital/hmpps/organisationsapi/config/OrganisationsApiExceptionHandler.kt +++ b/src/main/kotlin/uk/gov/justice/digital/hmpps/organisationsapi/config/OrganisationsApiExceptionHandler.kt @@ -6,6 +6,7 @@ import jakarta.validation.ValidationException import org.apache.commons.lang3.exception.ExceptionUtils import org.slf4j.LoggerFactory import org.springframework.http.HttpStatus.BAD_REQUEST +import org.springframework.http.HttpStatus.CONFLICT import org.springframework.http.HttpStatus.FORBIDDEN import org.springframework.http.HttpStatus.INTERNAL_SERVER_ERROR import org.springframework.http.HttpStatus.NOT_FOUND @@ -19,6 +20,7 @@ import org.springframework.web.bind.annotation.RestControllerAdvice import org.springframework.web.method.annotation.MethodArgumentTypeMismatchException import org.springframework.web.servlet.resource.NoResourceFoundException import uk.gov.justice.digital.hmpps.organisationsapi.exception.InvalidReferenceCodeGroupException +import uk.gov.justice.digital.hmpps.organisationsapi.service.sync.DuplicateOrganisationException import uk.gov.justice.hmpps.kotlin.common.ErrorResponse import java.time.format.DateTimeParseException @@ -127,6 +129,17 @@ class OrganisationsApiExceptionHandler { ), ) + @ExceptionHandler(DuplicateOrganisationException::class) + fun handleDuplicatePersonException(e: DuplicateOrganisationException): ResponseEntity = ResponseEntity + .status(CONFLICT) + .body( + ErrorResponse( + status = CONFLICT, + userMessage = e.message, + developerMessage = e.message, + ), + ) + @ExceptionHandler(MissingServletRequestParameterException::class) fun handleValidationException(e: MissingServletRequestParameterException): ResponseEntity = ResponseEntity .status(BAD_REQUEST) diff --git a/src/main/kotlin/uk/gov/justice/digital/hmpps/organisationsapi/resource/sync/SyncOrganisationController.kt b/src/main/kotlin/uk/gov/justice/digital/hmpps/organisationsapi/resource/sync/SyncOrganisationController.kt index 6660098..5f5cca6 100644 --- a/src/main/kotlin/uk/gov/justice/digital/hmpps/organisationsapi/resource/sync/SyncOrganisationController.kt +++ b/src/main/kotlin/uk/gov/justice/digital/hmpps/organisationsapi/resource/sync/SyncOrganisationController.kt @@ -27,7 +27,7 @@ import uk.gov.justice.digital.hmpps.organisationsapi.model.response.sync.SyncOrg import uk.gov.justice.digital.hmpps.organisationsapi.swagger.AuthApiResponses import uk.gov.justice.hmpps.kotlin.common.ErrorResponse -@Tag(name = "Sync & Migrate") +@Tag(name = "Migration and synchronisation") @RestController @RequestMapping(value = ["sync"], produces = [MediaType.APPLICATION_JSON_VALUE]) @AuthApiResponses @@ -96,7 +96,7 @@ class SyncOrganisationController(val syncFacade: SyncFacade) { @PostMapping(path = ["/organisation"], produces = [MediaType.APPLICATION_JSON_VALUE]) @ResponseBody @Operation( - summary = "Creates a new organisation", + summary = "Creates a new organisation with a specified ID", description = """ Requires role: ROLE_ORGANISATIONS_MIGRATION. Used to create a new organisation. diff --git a/src/test/kotlin/uk/gov/justice/digital/hmpps/organisationsapi/integration/StubOutboundEventsPublisher.kt b/src/test/kotlin/uk/gov/justice/digital/hmpps/organisationsapi/integration/StubOutboundEventsPublisher.kt index 3ad28ce..eec7e2f 100644 --- a/src/test/kotlin/uk/gov/justice/digital/hmpps/organisationsapi/integration/StubOutboundEventsPublisher.kt +++ b/src/test/kotlin/uk/gov/justice/digital/hmpps/organisationsapi/integration/StubOutboundEventsPublisher.kt @@ -8,7 +8,9 @@ import uk.gov.justice.digital.hmpps.organisationsapi.service.events.OutboundEven import uk.gov.justice.digital.hmpps.organisationsapi.service.events.OutboundEventsPublisher import uk.gov.justice.digital.hmpps.organisationsapi.service.events.OutboundHMPPSDomainEvent -class StubOutboundEventsPublisher(private val receivedEvents: MutableList = mutableListOf()) : OutboundEventsPublisher { +class StubOutboundEventsPublisher( + private val receivedEvents: MutableList = mutableListOf(), +) : OutboundEventsPublisher { companion object { private val logger = LoggerFactory.getLogger(this::class.java) } diff --git a/src/test/kotlin/uk/gov/justice/digital/hmpps/organisationsapi/integration/resource/sync/SyncOrganisationsIntegrationTest.kt b/src/test/kotlin/uk/gov/justice/digital/hmpps/organisationsapi/integration/resource/sync/SyncOrganisationsIntegrationTest.kt new file mode 100644 index 0000000..5e2e4da --- /dev/null +++ b/src/test/kotlin/uk/gov/justice/digital/hmpps/organisationsapi/integration/resource/sync/SyncOrganisationsIntegrationTest.kt @@ -0,0 +1,323 @@ +package uk.gov.justice.digital.hmpps.organisationsapi.integration.resource.sync + +import org.assertj.core.api.Assertions.assertThat +import org.junit.jupiter.api.BeforeEach +import org.junit.jupiter.api.Nested +import org.junit.jupiter.api.Test +import org.springframework.http.HttpStatus +import org.springframework.http.MediaType +import org.springframework.test.context.TestPropertySource +import uk.gov.justice.digital.hmpps.organisationsapi.integration.PostgresIntegrationTestBase +import uk.gov.justice.digital.hmpps.organisationsapi.model.request.sync.SyncCreateOrganisationRequest +import uk.gov.justice.digital.hmpps.organisationsapi.model.request.sync.SyncUpdateOrganisationRequest +import uk.gov.justice.digital.hmpps.organisationsapi.model.response.sync.SyncOrganisationResponse +import uk.gov.justice.digital.hmpps.organisationsapi.service.events.OrganisationInfo +import uk.gov.justice.digital.hmpps.organisationsapi.service.events.OutboundEvent +import uk.gov.justice.digital.hmpps.organisationsapi.service.events.Source +import uk.gov.justice.hmpps.kotlin.common.ErrorResponse +import java.time.LocalDateTime + +@TestPropertySource(properties = ["feature.events.sns.enabled=true"]) +class SyncOrganisationsIntegrationTest : PostgresIntegrationTestBase() { + + @Nested + inner class OrganisationSyncTests { + + @BeforeEach + fun resetEvents() { + stubEvents.reset() + } + + @Test + fun `Sync endpoints should return unauthorized if no token provided`() { + webTestClient.get() + .uri("/sync/organisation/1") + .accept(MediaType.APPLICATION_JSON) + .exchange() + .expectStatus() + .isUnauthorized + + webTestClient.post() + .uri("/sync/organisation") + .accept(MediaType.APPLICATION_JSON) + .contentType(MediaType.APPLICATION_JSON) + .bodyValue(syncCreateOrganisationRequest(5000L)) + .exchange() + .expectStatus() + .isUnauthorized + + webTestClient.put() + .uri("/sync/organisation/1") + .accept(MediaType.APPLICATION_JSON) + .contentType(MediaType.APPLICATION_JSON) + .bodyValue(syncUpdateOrganisationRequest(5000L)) + .exchange() + .expectStatus() + .isUnauthorized + + webTestClient.delete() + .uri("/sync/organisation/1") + .accept(MediaType.APPLICATION_JSON) + .exchange() + .expectStatus() + .isUnauthorized + } + + @Test + fun `Sync endpoints should return forbidden without an authorised role on the token`() { + webTestClient.get() + .uri("/sync/organisation/1") + .accept(MediaType.APPLICATION_JSON) + .headers(setAuthorisation(roles = listOf("ROLE_WRONG"))) + .exchange() + .expectStatus() + .isForbidden + + webTestClient.post() + .uri("/sync/organisation") + .accept(MediaType.APPLICATION_JSON) + .contentType(MediaType.APPLICATION_JSON) + .bodyValue(syncCreateOrganisationRequest(5000L)) + .headers(setAuthorisation(roles = listOf("ROLE_WRONG"))) + .exchange() + .expectStatus() + .isForbidden + + webTestClient.put() + .uri("/sync/organisation/1") + .accept(MediaType.APPLICATION_JSON) + .contentType(MediaType.APPLICATION_JSON) + .bodyValue(syncUpdateOrganisationRequest(5000L)) + .headers(setAuthorisation(roles = listOf("ROLE_WRONG"))) + .exchange() + .expectStatus() + .isForbidden + + webTestClient.delete() + .uri("/sync/organisation/1") + .accept(MediaType.APPLICATION_JSON) + .headers(setAuthorisation(roles = listOf("ROLE_WRONG"))) + .exchange() + .expectStatus() + .isForbidden + } + + @Test + fun `should create and then get an organisation by ID`() { + val organisationCreated = createOrganisationWithFixedId(5001L) + val organisation = getOrganisationById(organisationCreated.organisationId) + + with(organisation) { + assertThat(this.organisationId).isEqualTo(organisationId) + assertThat(organisationName).isEqualTo("Organisation123") + assertThat(programmeNumber).isEqualTo("PRG123") + assertThat(vatNumber).isEqualTo("VAT123") + assertThat(caseloadId).isEqualTo("HEI") + assertThat(comments).isEqualTo("comment123") + assertThat(active).isEqualTo(true) + assertThat(deactivatedDate).isNull() + assertThat(createdBy).isEqualTo("CREATOR") + assertThat(createdTime).isAfter(LocalDateTime.now().minusMinutes(5)) + assertThat(updatedBy).isNull() + assertThat(updatedTime).isNull() + } + + stubEvents.assertHasEvent( + event = OutboundEvent.ORGANISATION_CREATED, + additionalInfo = OrganisationInfo(organisation.organisationId, organisation.organisationId, Source.NOMIS), + ) + } + + @Test + fun `should create a new organisation with fixed ID`() { + val organisation = createOrganisationWithFixedId(5002L) + with(organisation) { + assertThat(this.organisationId).isEqualTo(5002L) + assertThat(organisationName).isEqualTo("Organisation123") + assertThat(programmeNumber).isEqualTo("PRG123") + assertThat(vatNumber).isEqualTo("VAT123") + assertThat(caseloadId).isEqualTo("HEI") + assertThat(comments).isEqualTo("comment123") + assertThat(active).isEqualTo(true) + assertThat(deactivatedDate).isNull() + assertThat(createdBy).isEqualTo("CREATOR") + assertThat(createdTime).isAfter(LocalDateTime.now().minusMinutes(5)) + assertThat(updatedBy).isNull() + assertThat(updatedTime).isNull() + } + + stubEvents.assertHasEvent( + event = OutboundEvent.ORGANISATION_CREATED, + additionalInfo = OrganisationInfo(organisation.organisationId, organisation.organisationId, Source.NOMIS), + ) + } + + @Test + fun `should create and then update an organisation`() { + val organisation = createOrganisationWithFixedId(5003L) + with(organisation) { + assertThat(this.organisationId).isEqualTo(5003L) + assertThat(organisationName).isEqualTo("Organisation123") + } + + val updated = updateOrganisation(organisation.organisationId) + with(updated) { + assertThat(this.organisationId).isEqualTo(5003L) + assertThat(organisationName).isEqualTo("Organisation321") + assertThat(programmeNumber).isEqualTo("PRG321") + assertThat(vatNumber).isEqualTo("VAT321") + assertThat(comments).isEqualTo("comment321") + assertThat(caseloadId).isEqualTo("AGI") + assertThat(active).isEqualTo(true) + assertThat(createdBy).isEqualTo("CREATOR") + assertThat(createdTime).isAfter(LocalDateTime.now().minusMinutes(5)) + assertThat(updatedBy).isEqualTo("UPDATER") + assertThat(updatedTime).isAfter(LocalDateTime.now().minusMinutes(5)) + } + + stubEvents.assertHasEvent( + event = OutboundEvent.ORGANISATION_CREATED, + additionalInfo = OrganisationInfo(organisation.organisationId, organisation.organisationId, Source.NOMIS), + ) + + stubEvents.assertHasEvent( + event = OutboundEvent.ORGANISATION_UPDATED, + additionalInfo = OrganisationInfo(updated.organisationId, updated.organisationId, Source.NOMIS), + ) + } + + @Test + fun `should create and then delete an organisation`() { + val organisation = createOrganisationWithFixedId(5004L) + with(organisation) { + assertThat(this.organisationId).isEqualTo(5004L) + assertThat(organisationName).isEqualTo("Organisation123") + } + + webTestClient.delete() + .uri("/sync/organisation/{organisationId}", organisation.organisationId) + .accept(MediaType.APPLICATION_JSON) + .headers(setAuthorisation(roles = listOf("ROLE_ORGANISATIONS_MIGRATION"))) + .exchange() + .expectStatus() + .isOk + + webTestClient.get() + .uri("/sync/organisations/{organisationId}", organisation.organisationId) + .accept(MediaType.APPLICATION_JSON) + .headers(setAuthorisation(roles = listOf("ROLE_ORGANISATIONS_MIGRATION"))) + .exchange() + .expectStatus() + .isNotFound + + stubEvents.assertHasEvent( + event = OutboundEvent.ORGANISATION_CREATED, + additionalInfo = OrganisationInfo(organisation.organisationId, organisation.organisationId, Source.NOMIS), + ) + + stubEvents.assertHasEvent( + event = OutboundEvent.ORGANISATION_DELETED, + additionalInfo = OrganisationInfo(organisation.organisationId, organisation.organisationId, Source.NOMIS), + ) + } + + @Test + fun `should report a conflict when creating an organisation ID that already exists`() { + val organisation = createOrganisationWithFixedId(5005L) + with(organisation) { + assertThat(this.organisationId).isEqualTo(5005L) + assertThat(organisationName).isEqualTo("Organisation123") + } + + stubEvents.assertHasEvent( + event = OutboundEvent.ORGANISATION_CREATED, + additionalInfo = OrganisationInfo(organisation.organisationId, organisation.organisationId, Source.NOMIS), + ) + + resetEvents() + + val expectedError = webTestClient.post() + .uri("/sync/organisation") + .accept(MediaType.APPLICATION_JSON) + .contentType(MediaType.APPLICATION_JSON) + .headers(setAuthorisation(roles = listOf("ROLE_ORGANISATIONS_MIGRATION"))) + .bodyValue(syncCreateOrganisationRequest(organisation.organisationId)) + .exchange() + .expectStatus() + .is4xxClientError + .expectHeader().contentType(MediaType.APPLICATION_JSON) + .expectBody(ErrorResponse::class.java) + .returnResult().responseBody!! + + assertThat(expectedError.status).isEqualTo(HttpStatus.CONFLICT.value()) + assertThat(expectedError.userMessage).isEqualTo("Sync: Duplicate organisation ID received 5005") + + stubEvents.assertHasNoEvents(OutboundEvent.ORGANISATION_CREATED) + } + + private fun syncUpdateOrganisationRequest(organisationId: Long) = SyncUpdateOrganisationRequest( + organisationId = organisationId, + organisationName = "Organisation321", + programmeNumber = "PRG321", + vatNumber = "VAT321", + caseloadId = "AGI", + comments = "comment321", + active = true, + updatedBy = "UPDATER", + updatedTime = LocalDateTime.now(), + ) + + private fun syncCreateOrganisationRequest(organisationId: Long) = SyncCreateOrganisationRequest( + // Sync creates supply a fixed ID from NOMIS (i.e. the corporate ID) + organisationId = organisationId, + organisationName = "Organisation123", + programmeNumber = "PRG123", + vatNumber = "VAT123", + caseloadId = "HEI", + comments = "comment123", + active = true, + createdTime = LocalDateTime.now(), + createdBy = "CREATOR", + ) + + private fun createOrganisationWithFixedId(organisationId: Long) = + webTestClient.post() + .uri("/sync/organisation") + .accept(MediaType.APPLICATION_JSON) + .contentType(MediaType.APPLICATION_JSON) + .headers(setAuthorisation(roles = listOf("ROLE_ORGANISATIONS_MIGRATION"))) + .bodyValue(syncCreateOrganisationRequest(organisationId)) + .exchange() + .expectStatus() + .isOk + .expectHeader().contentType(MediaType.APPLICATION_JSON) + .expectBody(SyncOrganisationResponse::class.java) + .returnResult().responseBody!! + + private fun getOrganisationById(organisationId: Long) = + webTestClient.get() + .uri("/sync/organisation/{organisationId}", organisationId) + .accept(MediaType.APPLICATION_JSON) + .headers(setAuthorisation(roles = listOf("ROLE_ORGANISATIONS_MIGRATION"))) + .exchange() + .expectStatus() + .isOk + .expectHeader().contentType(MediaType.APPLICATION_JSON) + .expectBody(SyncOrganisationResponse::class.java) + .returnResult().responseBody!! + + private fun updateOrganisation(organisationId: Long) = + webTestClient.put() + .uri("/sync/organisation/{organisationId}", organisationId) + .accept(MediaType.APPLICATION_JSON) + .contentType(MediaType.APPLICATION_JSON) + .headers(setAuthorisation(roles = listOf("ROLE_ORGANISATIONS_MIGRATION"))) + .bodyValue(syncUpdateOrganisationRequest(organisationId)) + .exchange() + .expectStatus() + .isOk + .expectHeader().contentType(MediaType.APPLICATION_JSON) + .expectBody(SyncOrganisationResponse::class.java) + .returnResult().responseBody!! + } +} From 561b50b8712fcd344673c4d2efe25c30b9ee25e6 Mon Sep 17 00:00:00 2001 From: Tim Harrison Date: Fri, 21 Feb 2025 14:25:14 +0000 Subject: [PATCH 2/2] PRC-439: Fix poorly named function --- .../organisationsapi/config/OrganisationsApiExceptionHandler.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/kotlin/uk/gov/justice/digital/hmpps/organisationsapi/config/OrganisationsApiExceptionHandler.kt b/src/main/kotlin/uk/gov/justice/digital/hmpps/organisationsapi/config/OrganisationsApiExceptionHandler.kt index bf5cc80..17f7c05 100644 --- a/src/main/kotlin/uk/gov/justice/digital/hmpps/organisationsapi/config/OrganisationsApiExceptionHandler.kt +++ b/src/main/kotlin/uk/gov/justice/digital/hmpps/organisationsapi/config/OrganisationsApiExceptionHandler.kt @@ -130,7 +130,7 @@ class OrganisationsApiExceptionHandler { ) @ExceptionHandler(DuplicateOrganisationException::class) - fun handleDuplicatePersonException(e: DuplicateOrganisationException): ResponseEntity = ResponseEntity + fun handleDuplicateOrganisationException(e: DuplicateOrganisationException): ResponseEntity = ResponseEntity .status(CONFLICT) .body( ErrorResponse(