Skip to content

Commit

Permalink
NON-287: Disallow adding a non-associations to prisoners with null lo…
Browse files Browse the repository at this point in the history
…cations (#152)

¡There are breaking changes to the api contract!
* Allow for offender search results that have no prison name or ID – there’s a knock on effect on the `GET /prisoner/{prisonerNumber}/non-associations` endpoint such that returned type’s fields become nullable!
* Disallow creating non-associations when a prisoner’s location is null – the `POST /non-associations` now returns 409 CONFLICT if a prisoner’s prison ID is null
* Indicate nullable location types propagated from offender search results in node REST client
  • Loading branch information
ushkarev authored Oct 26, 2023
1 parent 189f17a commit 1e57486
Show file tree
Hide file tree
Showing 12 changed files with 130 additions and 35 deletions.
1 change: 1 addition & 0 deletions clients/node/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ General notes regarding permissions and roles:
- Users also having the `GLOBAL_SEARCH` role can also _add_, _update_ and _close_ non-associations for prisoners in transfer and where one prisoner is not in a prison that’s not in their caseloads
- Users also having the `INACTIVE_BOOKINGS` role can also _add_, _update_ and _close_ non-associations for prisoners outside any establishment / released
- Users must _close_ rather than _delete_ non-associations
- No users should be able to _add_, _update_ or _close_ non-associations for prisoners without a booking / with a null location

Release a new version
---------------------
Expand Down
5 changes: 3 additions & 2 deletions clients/node/src/errorTypes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* Structure representing an error response from the api, wrapped in SanitisedError.
*
* Defined in uk.gov.justice.digital.hmpps.hmppsnonassociationsapi.config.ErrorResponse class
* https://github.com/ministryofjustice/hmpps-non-associations-api/blob/f6002aa1da50b8c4ccd3613e970327d5c67c44ae/src/main/kotlin/uk/gov/justice/digital/hmpps/hmppsnonassociationsapi/config/HmppsNonAssociationsApiExceptionHandler.kt#L240-L261
* see https://github.com/ministryofjustice/hmpps-non-associations-api
*/
export interface ErrorResponse {
status: number
Expand Down Expand Up @@ -34,11 +34,12 @@ export function isErrorResponse(obj: unknown): obj is ErrorResponse {
* Unique codes to discriminate errors returned from the api.
*
* Defined in uk.gov.justice.digital.hmpps.hmppsnonassociationsapi.config.ErrorCode enumeration
* https://github.com/ministryofjustice/hmpps-non-associations-api/blob/f6002aa1da50b8c4ccd3613e970327d5c67c44ae/src/main/kotlin/uk/gov/justice/digital/hmpps/hmppsnonassociationsapi/config/HmppsNonAssociationsApiExceptionHandler.kt#L227-L238
* see https://github.com/ministryofjustice/hmpps-non-associations-api
*/
export enum ErrorCode {
NonAssociationAlreadyClosed = 100,
OpenNonAssociationAlreadyExist = 101,
ValidationFailure = 102,
NullPrisonerLocations = 103,
UserInContextMissing = 401,
}
8 changes: 4 additions & 4 deletions clients/node/src/responseTypes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ interface BaseNonAssociationsListItem extends ObjectWithDates {
roleDescription: Role[keyof Role]
firstName: string
lastName: string
prisonId: string
prisonName: string
prisonId?: string
prisonName?: string
cellLocation?: string
}
}
Expand Down Expand Up @@ -58,8 +58,8 @@ export interface NonAssociationsList<
prisonerNumber: string
firstName: string
lastName: string
prisonId: string
prisonName: string
prisonId?: string
prisonName?: string
cellLocation?: string
openCount: number
closedCount: number
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,21 @@ class HmppsNonAssociationsApiExceptionHandler {
)
}

@ExceptionHandler(NullPrisonerLocationsException::class)
fun handleNullPrisonerLocationsException(e: NullPrisonerLocationsException): ResponseEntity<ErrorResponse?>? {
log.debug("Non-association cannot be created when prisoner locations are null: {}", e.message)
return ResponseEntity
.status(CONFLICT)
.body(
ErrorResponse(
status = CONFLICT,
errorCode = ErrorCode.NullPrisonerLocations,
userMessage = "Non-association cannot be created when prisoner locations are null: ${e.message}",
developerMessage = e.message,
),
)
}

companion object {
private val log = LoggerFactory.getLogger(this::class.java)
}
Expand All @@ -235,6 +250,7 @@ enum class ErrorCode(val errorCode: Int) {
UserInContextMissing(401),
OpenNonAssociationAlreadyExist(101),
ValidationFailure(102),
NullPrisonerLocations(103),
}

@Schema(description = "Error response")
Expand Down Expand Up @@ -267,3 +283,5 @@ class UserInContextMissingException : Exception("There is no user in context for
class OpenNonAssociationAlreadyExistsException(prisoners: List<String>) : Exception("Prisoners $prisoners already have open non-associations")

class NonAssociationNotFoundException(id: Long) : Exception("There is no non-association found for ID = $id")

class NullPrisonerLocationsException(prisoners: List<String>) : Exception("Prisoners $prisoners have null locations")
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@ data class LegacyPrisonerNonAssociations(
val firstName: String,
@Schema(description = "Last name", required = true, example = "Hall")
val lastName: String,
@Schema(description = "Prison ID where prisoner resides or OUT for released", required = true, example = "MDI")
val agencyId: String,
@Schema(description = "Description of the agency (e.g. prison) the offender is assigned to", required = true, example = "Moorland (HMP & YOI)")
val agencyDescription: String,
@Schema(description = "Prison ID where prisoner resides or OUT for released", required = false, example = "MDI")
val agencyId: String?,
@Schema(description = "Description of the agency (e.g. prison) the offender is assigned to", required = false, example = "Moorland (HMP & YOI)")
val agencyDescription: String?,
@Schema(description = "Description of living unit (e.g. cell) the offender is assigned to.", required = false, example = "MDI-1-1-3")
val assignedLivingUnitDescription: String?,
@Schema(description = "Non-associations with other prisoners", required = true)
Expand Down Expand Up @@ -64,10 +64,10 @@ data class LegacyOtherPrisonerDetails(
val reasonCode: LegacyReason,
@Schema(description = "Reason for the non-association", required = true, example = "Perpetrator")
val reasonDescription: String,
@Schema(description = "Prison ID where prisoner resides or OUT for released", required = true, example = "MDI")
val agencyId: String,
@Schema(description = "Description of the agency (e.g. prison) the offender is assigned to", required = true, example = "Moorland (HMP & YOI)")
val agencyDescription: String,
@Schema(description = "Prison ID where prisoner resides or OUT for released", required = false, example = "MDI")
val agencyId: String?,
@Schema(description = "Description of the agency (e.g. prison) the offender is assigned to", required = false, example = "Moorland (HMP & YOI)")
val agencyDescription: String?,
@Schema(description = "Description of living unit (e.g. cell) the offender is assigned to.", required = false, example = "MDI-2-3-4")
val assignedLivingUnitDescription: String?,
)
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,10 @@ data class PrisonerNonAssociations(
val firstName: String,
@Schema(description = "Last name", required = true, example = "Hall")
val lastName: String,
@Schema(description = "ID of the prison the prisoner is assigned to", required = true, example = "MDI")
val prisonId: String,
@Schema(description = "Name of the prison the prisoner is assigned to", required = true, example = "Moorland (HMP & YOI)")
val prisonName: String,
@Schema(description = "ID of the prison the prisoner is assigned to", required = false, example = "MDI")
val prisonId: String?,
@Schema(description = "Name of the prison the prisoner is assigned to", required = false, example = "Moorland (HMP & YOI)")
val prisonName: String?,
@Schema(description = "Cell the prisoner is assigned to", required = false, example = "A-1-002")
val cellLocation: String?,
@Schema(description = "Number of open non-associations (follows includeOtherPrisons filter)", required = true, example = "1", minimum = "0", type = "integer", format = "int32")
Expand Down Expand Up @@ -93,10 +93,10 @@ data class OtherPrisonerDetails(
val firstName: String,
@Schema(description = "Last name", required = true, example = "Bloggs")
val lastName: String,
@Schema(description = "ID of the prison the prisoner is assigned to", required = true, example = "MDI")
val prisonId: String,
@Schema(description = "Name of the prison the prisoner is assigned to", required = true, example = "Moorland (HMP & YOI)")
val prisonName: String,
@Schema(description = "ID of the prison the prisoner is assigned to", required = false, example = "MDI")
val prisonId: String?,
@Schema(description = "Name of the prison the prisoner is assigned to", required = false, example = "Moorland (HMP & YOI)")
val prisonName: String?,
@Schema(description = "Cell the prisoner is assigned to", required = false, example = "B-2-007")
val cellLocation: String?,
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ data class OffenderSearchPrisoner(
val firstName: String,
val lastName: String,

val prisonId: String,
val prisonName: String,
val prisonId: String?,
val prisonName: String?,
val cellLocation: String?,
)
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,12 @@ class NonAssociationsResource(
),
ApiResponse(
responseCode = "404",
description = "Any of the prisoners could not be found.",
description = "Some of the prisoners were not be found.",
content = [Content(mediaType = "application/json", schema = Schema(implementation = ErrorResponse::class))],
),
ApiResponse(
responseCode = "409",
description = "Open non-association already exists or some prisoner’s location is null.",
content = [Content(mediaType = "application/json", schema = Schema(implementation = ErrorResponse::class))],
),
],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import org.springframework.web.server.ResponseStatusException
import uk.gov.justice.digital.hmpps.hmppsnonassociationsapi.config.AuthenticationFacade
import uk.gov.justice.digital.hmpps.hmppsnonassociationsapi.config.FeatureFlagsConfig
import uk.gov.justice.digital.hmpps.hmppsnonassociationsapi.config.NonAssociationAlreadyClosedException
import uk.gov.justice.digital.hmpps.hmppsnonassociationsapi.config.NullPrisonerLocationsException
import uk.gov.justice.digital.hmpps.hmppsnonassociationsapi.config.OpenNonAssociationAlreadyExistsException
import uk.gov.justice.digital.hmpps.hmppsnonassociationsapi.config.UserInContextMissingException
import uk.gov.justice.digital.hmpps.hmppsnonassociationsapi.dto.CloseNonAssociationRequest
Expand Down Expand Up @@ -67,7 +68,14 @@ class NonAssociationsService(
throw OpenNonAssociationAlreadyExistsException(prisonersToKeepApart)
}

offenderSearch.searchByPrisonerNumbers(prisonersToKeepApart)
// ensure that prisoner numbers exist and their locations are not null
val prisonersWithNullLocations = offenderSearch.searchByPrisonerNumbers(prisonersToKeepApart)
.values
.filter { it.prisonId == null }
.map { it.prisonerNumber }
if (prisonersWithNullLocations.isNotEmpty()) {
throw NullPrisonerLocationsException(prisonersWithNullLocations)
}

val nonAssociationJpa = createNonAssociationRequest.toNewEntity(
updatedBy = authenticationFacade.currentUsername
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,41 @@ class NonAssociationsResourceTest : SqsIntegrationTestBase() {
.expectStatus().isEqualTo(409)
}

@Test
fun `cannot create NA if some prisoner has a null location`() {
val firstPrisoner = offenderSearchPrisoners["A1234BC"]!!
val secondPrisoner = offenderSearchPrisoners["D1234DD"]!!

offenderSearchMockServer.stubSearchByPrisonerNumbers(
listOf("A1234BC", "D1234DD"),
listOf(firstPrisoner, secondPrisoner),
)

val request = createNonAssociationRequest(
firstPrisonerNumber = firstPrisoner.prisonerNumber,
firstPrisonerRole = Role.VICTIM,
secondPrisonerNumber = secondPrisoner.prisonerNumber,
secondPrisonerRole = Role.PERPETRATOR,
reason = Reason.VIOLENCE,
restrictionType = RestrictionType.CELL,
comment = "They keep fighting",
)

webTestClient.post()
.uri(url)
.headers(
setAuthorisation(
user = expectedUsername,
roles = listOf("ROLE_WRITE_NON_ASSOCIATIONS"),
scopes = listOf("write", "read"),
),
)
.header("Content-Type", "application/json")
.bodyValue(jsonString(request))
.exchange()
.expectStatus().isEqualTo(409)
}

@Test
fun `can create NA for already closed NA between same prisoners`() {
val firstPrisoner = offenderSearchPrisoners["A1234BC"]!!
Expand Down Expand Up @@ -2512,15 +2547,15 @@ class NonAssociationsResourceTest : SqsIntegrationTestBase() {

@Test
fun `when non-associations are requested between more than 2 prisoners`() {
// language=mermaid
"""
/*
%% language=mermaid
classDiagram
A0000AA -- A1111AA
A0000AA -- A2222AA
A2222AA -- A3333AA
A1111AA -- A4444AA
A4444AA -- A2222AA : closed
"""
*/
createNonAssociation("A0000AA", "A1111AA") // never returned
createNonAssociation("A0000AA", "A2222AA") // returned
createNonAssociation("A2222AA", "A3333AA") // never returned
Expand Down Expand Up @@ -2865,15 +2900,15 @@ class NonAssociationsResourceTest : SqsIntegrationTestBase() {

@Test
fun `when non-associations are requested involving more than 2 prisoners`() {
// language=mermaid
"""
/*
%% language=mermaid
classDiagram
A0000AA -- A1111AA
A0000AA -- A2222AA
A2222AA -- A3333AA
A1111AA -- A4444AA
A4444AA -- A2222AA : closed
"""
*/
createNonAssociation("A0000AA", "A1111AA") // never returned
createNonAssociation("A0000AA", "A2222AA") // returned
createNonAssociation("A2222AA", "A3333AA") // returned
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -167,9 +167,9 @@ class NonAssociationsServiceTest {
NonAssociationsSort.LAST_NAME -> { n -> n.otherPrisonerDetails.lastName }
NonAssociationsSort.FIRST_NAME -> { n -> n.otherPrisonerDetails.firstName }
NonAssociationsSort.PRISONER_NUMBER -> { n -> n.otherPrisonerDetails.prisonerNumber }
NonAssociationsSort.PRISON_ID -> { n -> n.otherPrisonerDetails.prisonId }
NonAssociationsSort.PRISON_NAME -> { n -> n.otherPrisonerDetails.prisonName }
NonAssociationsSort.CELL_LOCATION -> { n -> n.otherPrisonerDetails.cellLocation!! }
NonAssociationsSort.PRISON_ID -> { n -> n.otherPrisonerDetails.prisonId ?: "" }
NonAssociationsSort.PRISON_NAME -> { n -> n.otherPrisonerDetails.prisonName ?: "" }
NonAssociationsSort.CELL_LOCATION -> { n -> n.otherPrisonerDetails.cellLocation ?: "" }
}
Sort.Direction.entries.forEach { sortDirection ->
val listOptions = NonAssociationListOptions(sortBy = sortBy, sortDirection = sortDirection)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,4 +104,31 @@ val offenderSearchPrisoners = mapOf(
prisonName = "Forest Bank",
cellLocation = "FBI-C-2",
),
// In transfer
"C1234CC" to OffenderSearchPrisoner(
prisonerNumber = "C1234CC",
firstName = "MAX",
lastName = "CLARKE",
prisonId = "TRN",
prisonName = "Transfer",
cellLocation = null,
),
// Outside any establishment
"B1234BB" to OffenderSearchPrisoner(
prisonerNumber = "B1234BB",
firstName = "JOE",
lastName = "PETERS",
prisonId = "OUT",
prisonName = "Outside - released from Moorland (HMP)",
cellLocation = null,
),
// Null location, allegedly indicates no booking
"D1234DD" to OffenderSearchPrisoner(
prisonerNumber = "D1234DD",
firstName = "NATHAN",
lastName = "LOST",
prisonId = null,
prisonName = null,
cellLocation = null,
),
)

0 comments on commit 1e57486

Please sign in to comment.