Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor placement-request page information #2280

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 4 additions & 13 deletions integration_tests/pages/admin/placementApplications/showPage.ts
Original file line number Diff line number Diff line change
@@ -1,25 +1,16 @@
import { ApprovedPremises, PlacementRequest, PlacementRequestDetail } from '@approved-premises/api'
import Page from '../../page'

import {
ApprovedPremises,
FullPerson,
PlacementRequest,
PlacementRequestDetail,
} from '../../../../server/@types/shared'
import { adminSummary, matchingInformationSummary } from '../../../../server/utils/placementRequests'
import { bookingSummaryList } from '../../../../server/utils/bookings'
import { placementRequestSummaryList } from '../../../../server/utils/placementRequests/placementRequestSummaryList'

export default class ShowPage extends Page {
constructor(private readonly placementRequest: PlacementRequestDetail) {
super((placementRequest.person as FullPerson).name)
super('Placement request')
}

shouldShowSummary(): void {
this.shouldContainSummaryListItems(adminSummary(this.placementRequest).rows)
}

shouldShowMatchingInformationSummary(): void {
this.shouldContainSummaryListItems(matchingInformationSummary(this.placementRequest).rows)
this.shouldContainSummaryListItems(placementRequestSummaryList(this.placementRequest).rows)
}

clickPlacementRequest(placementRequest: PlacementRequest): void {
Expand Down
4 changes: 0 additions & 4 deletions integration_tests/tests/admin/placementRequests.cy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,6 @@ context('Placement Requests', () => {
unmatchedPlacementRequests,
unableToMatchPlacementRequests,
matchedPlacementRequests,
preferredAps,
matchedPlacementRequest,
parolePlacementRequest,
} = stubArtifacts()
Expand Down Expand Up @@ -149,8 +148,6 @@ context('Placement Requests', () => {

// And I should see the information about the placement request
showPage.shouldShowSummary()
showPage.shouldShowMatchingInformationSummary()
showPage.shouldShowPreferredAps(preferredAps)

// And I should not see any booking information
showPage.shouldNotShowBookingInformation()
Expand All @@ -173,7 +170,6 @@ context('Placement Requests', () => {

// And I should see the information about the placement request
showPage.shouldShowSummary()
showPage.shouldShowMatchingInformationSummary()

showPage.shouldNotShowCreateBookingOption()
showPage.shouldShowAmendBookingOption()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import PlacementRequestsController from './placementRequestsController'
import { PlacementRequestService } from '../../../services'
import { userDetailsFactory } from '../../../testutils/factories'
import placementRequestDetail from '../../../testutils/factories/placementRequestDetail'
import { placementRequestSummaryList } from '../../../utils/placementRequests/placementRequestSummaryList'

jest.mock('../../../utils/applications/utils')
jest.mock('../../../utils/applications/getResponses')
Expand Down Expand Up @@ -41,6 +42,7 @@ describe('PlacementRequestsController', () => {

expect(response.render).toHaveBeenCalledWith('admin/placementRequests/show', {
placementRequest,
placementRequestSummaryList: placementRequestSummaryList(placementRequest),
})
expect(placementRequestService.getPlacementRequest).toHaveBeenCalledWith(token, 'some-uuid')
})
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import type { Request, Response, TypedRequestHandler } from 'express'
import { PlacementRequestService } from '../../../services'
import { placementRequestSummaryList } from '../../../utils/placementRequests/placementRequestSummaryList'

export default class PlacementRequestsController {
constructor(private readonly placementRequestService: PlacementRequestService) {}
Expand All @@ -10,6 +11,7 @@ export default class PlacementRequestsController {

res.render('admin/placementRequests/show', {
placementRequest,
placementRequestSummaryList: placementRequestSummaryList(placementRequest),
})
}
}
Expand Down
16 changes: 16 additions & 0 deletions server/testutils/factories/offlineApplication.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import { Factory } from 'fishery'
import { faker } from '@faker-js/faker/locale/en_GB'

import type { OfflineApplication } from '@approved-premises/api'

import { fullPersonFactory, restrictedPersonFactory } from './person'
import { DateFormats } from '../../utils/dateUtils'

class OfflineApplicationFactory extends Factory<OfflineApplication> {}

export default OfflineApplicationFactory.define(() => ({
type: 'CAS1',
id: faker.string.uuid(),
person: faker.helpers.arrayElement([fullPersonFactory.build(), restrictedPersonFactory.build()]),
createdAt: DateFormats.dateObjToIsoDateTime(faker.date.past()),
}))
104 changes: 101 additions & 3 deletions server/utils/match/index.test.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
import type {
ApType,
Application,
ApprovedPremisesApplication,
Cas1SpaceBookingCharacteristic,
FullPerson,
PlacementCriteria,
} from '@approved-premises/api'
import { when } from 'jest-when'
import type { SummaryListItem } from '@approved-premises/ui'
import applyPaths from '../../paths/apply'
import paths from '../../paths/match'
import {
personFactory,
Expand All @@ -23,6 +25,7 @@ import {
apManagerDetailsRow,
apTypeLabelsForRadioInput,
apTypeRow,
apTypeWithViewTimelineActionRow,
arrivalDateRow,
calculateDepartureDate,
characteristicsRow,
Expand All @@ -49,10 +52,11 @@ import {
placementLength,
placementLengthRow,
placementRequestSummaryListForMatching,
postcodeRow,
preferredPostcodeRow,
premisesNameRow,
redirectToSpaceBookingsNew,
releaseTypeRow,
requestedOrEstimatedArrivalDateRow,
requirementsHtmlString,
spaceBookingPersonNeedsSummaryCardRows,
spaceBookingPremisesSummaryCardRows,
Expand All @@ -71,6 +75,7 @@ import { textValue } from '../applications/helpers'
import { preferredApsRow } from '../placementRequests/preferredApsRow'
import { placementRequirementsRow } from '../placementRequests/placementRequirementsRow'
import applicationFactory from '../../testutils/factories/application'
import offlineApplicationFactory from '../../testutils/factories/offlineApplication'

jest.mock('../retrieveQuestionResponseFromFormArtifact')

Expand Down Expand Up @@ -111,6 +116,78 @@ describe('matchUtils', () => {
})
})

describe('requestedOrEstimatedArrivalDateRow', () => {
it('should return Estimated arrival date with date when is parole', () => {
const arrivalDate = '2024-01-28'
expect(requestedOrEstimatedArrivalDateRow(true, arrivalDate)).toEqual({
key: { text: 'Estimated arrival date' },
value: { text: 'Sun 28 Jan 2024' },
})
})

it('should return Requested arrival date with date when is not parole', () => {
const arrivalDate = '2024-01-28'
expect(requestedOrEstimatedArrivalDateRow(false, arrivalDate)).toEqual({
key: { text: 'Requested arrival date' },
value: { text: 'Sun 28 Jan 2024' },
})
})
})

describe('apTypeRow', () => {
it.each(Object.keys(apTypeLabels) as Array<ApType>)(
'should return the correct type for AP Type %s',
(apType: ApType) => {
const placementRequestWithApType = placementRequestDetailFactory.build({
type: apType,
})

expect(apTypeWithViewTimelineActionRow(placementRequestWithApType)).toEqual({
key: {
text: 'Type of AP',
},
value: {
text: apTypeLabels[apType],
},
actions: {
items: [
{
href: `${applyPaths.applications.show({ id: placementRequestWithApType.application.id })}?tab=timeline`,
text: 'View timeline',
},
],
},
})
},
)

it('should return the correct type for AP Type normal without actions when placement-request has no application', () => {
const apType: ApType = 'normal'
const placementRequestWithApType = placementRequestDetailFactory.build({
type: apType,
application: undefined,
})
expect(apTypeWithViewTimelineActionRow(placementRequestWithApType)).toEqual({
key: {
text: 'Type of AP',
},
value: {
text: apTypeLabels[apType],
},
})
})
})

describe('preferredPostcodeRow', () => {
it('returns preferred postcode', () => {
const postcode = 'B71'
expect(preferredPostcodeRow(postcode)).toEqual({
key: { text: 'Preferred postcode' },
value: { text: postcode },
})
})
})

describe('distanceRow', () => {
const spaceSearchResult = spaceSearchResultFactory.build()
const postcodeArea = 'HR1 2AF'
Expand Down Expand Up @@ -417,7 +494,7 @@ describe('matchUtils', () => {
departureDateRow(dates.endDate),
placementLengthRow(dates.placementLength),
releaseTypeRow(placementRequest),
licenceExpiryDateRow(placementRequest.application as ApprovedPremisesApplication),
licenceExpiryDateRow(placementRequest),
totalCapacityRow(totalCapacity),
apManagerDetailsRow(managerDetails),
spaceRequirementsRow(filterOutAPTypes(placementRequest.essentialCriteria)),
Expand All @@ -430,6 +507,27 @@ describe('matchUtils', () => {
)
})

it(`should generate the expected matching details when placement-request's application is undefined`, () => {
const undefinedApplication: Application = undefined
const placementRequestWithoutLicenceExpiry = {
...placementRequest,
application: undefinedApplication,
}
expect(
occupancyViewSummaryListForMatchingDetails(totalCapacity, placementRequestWithoutLicenceExpiry, managerDetails),
).toEqual(expectedMatchingDetailsSummaryListItems(''))
})

it(`should generate the expected matching details when placement-request's application is not of type ApprovedPremisesApplication`, () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

justification for this test case is in a comment in the index.ts file of this PR

const placementRequestWithoutLicenceExpiry = {
...placementRequest,
application: offlineApplicationFactory.build(),
}
expect(
occupancyViewSummaryListForMatchingDetails(totalCapacity, placementRequestWithoutLicenceExpiry, managerDetails),
).toEqual(expectedMatchingDetailsSummaryListItems(''))
})

it(`should generate the expected matching details with blank licence expiry date when application's license-expiry date is not set`, () => {
const placementRequestWithoutLicenceExpiry = {
...placementRequest,
Expand Down Expand Up @@ -625,7 +723,7 @@ describe('matchUtils', () => {
),
),
lengthOfStayRow(placementRequest.duration),
postcodeRow(placementRequest.location),
preferredPostcodeRow(placementRequest.location),
apTypeRow(placementRequest.type),
placementRequirementsRow(placementRequest, 'essential'),
placementRequirementsRow(placementRequest, 'desirable'),
Expand Down
68 changes: 54 additions & 14 deletions server/utils/match/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import { placementRequirementsRow } from '../placementRequests/placementRequirem
import { allReleaseTypes } from '../applications/releaseTypeUtils'
import { placementDates } from './placementDates'
import { occupancyCriteriaMap } from './occupancy'
import paths from '../../paths/apply'

export { placementDates } from './placementDates'
export { occupancySummary } from './occupancySummary'
Expand Down Expand Up @@ -181,14 +182,12 @@ export const occupancyViewSummaryListForMatchingDetails = (
): Array<SummaryListItem> => {
const placementRequestDates = placementDates(placementRequest.expectedArrival, placementRequest.duration)
const essentialCharacteristics = filterOutAPTypes(placementRequest.essentialCriteria)
const application = placementRequest.application as ApprovedPremisesApplication

return [
arrivalDateRow(placementRequestDates.startDate),
departureDateRow(placementRequestDates.endDate),
placementLengthRow(placementRequestDates.placementLength),
releaseTypeRow(placementRequest),
licenceExpiryDateRow(application),
licenceExpiryDateRow(placementRequest),
totalCapacityRow(totalCapacity),
apManagerDetailsRow(managerDetails),
spaceRequirementsRow(essentialCharacteristics),
Expand Down Expand Up @@ -244,6 +243,15 @@ export const arrivalDateRow = (arrivalDate: string) => ({
},
})

export const requestedOrEstimatedArrivalDateRow = (isParole: boolean, arrivalDate: string) => ({
key: {
text: isParole ? 'Estimated arrival date' : 'Requested arrival date',
},
value: {
text: DateFormats.isoDateToUIDate(arrivalDate),
},
})

export const departureDateRow = (departureDate: string) => ({
key: {
text: 'Expected departure date',
Expand Down Expand Up @@ -280,6 +288,31 @@ export const apTypeRow = (apType: ApType) => ({
},
})

export const apTypeWithViewTimelineActionRow = (placementRequest: PlacementRequestDetail) => {
const apTypeItem = {
key: {
text: 'Type of AP',
},
value: {
text: apTypeLabels[placementRequest.type],
},
}
if (placementRequest.application) {
return {
...apTypeItem,
actions: {
items: [
{
href: `${paths.applications.show({ id: placementRequest.application.id })}?tab=timeline`,
text: 'View timeline',
},
],
},
}
}
return apTypeItem
}

export const addressRow = (spaceSearchResult: SpaceSearchResult) => ({
key: {
text: 'Address',
Expand Down Expand Up @@ -376,14 +409,21 @@ export const releaseTypeRow = (placementRequest: PlacementRequest) => ({
},
})

export const licenceExpiryDateRow = (application: ApprovedPremisesApplication) => ({
key: {
text: 'Licence expiry date',
},
value: {
text: application.licenceExpiryDate ? DateFormats.isoDateToUIDate(application.licenceExpiryDate) : '',
},
})
export const licenceExpiryDateRow = (placementRequest: PlacementRequestDetail) => {
let licenceExpiryDate: string | undefined
if (placementRequest.application && 'licenceExpiryDate' in placementRequest.application) {
Copy link
Contributor Author

@gregkhawkins gregkhawkins Jan 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the integration tests saved my neck here as the isParole test case does not have an application associated with the placementRequest (in the test data setup). I assume that the data setup in that test reflects reality. Anyway, this means we need the if (placementRequest.application) check for sure or a runtime exception would occur for the isParole scenario.
I also added the if ('licenceExpiryDate' in placementRequest.application) check as I was growing increasingly paranoid that a placementRequest.application would not be of type ApprovedPremisesApplication. I did check this in the backend previously (when Bob flagged it on previous PR) and it seemed for the occupancy-view page it always would be, however with recent work on offline applications etc I wondered whether we could end up with an application that does not meet this type leading to a runtime exception related to that when we try to access the licenseExpiryDate field (specific to ApprovedPremisesApplication only).
Let me know if this is too defensive or if you can think of a more graceful way to do it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, a placementRequest without an application_id is news to me! In the placement, there is a secondary id offline_application_id with a foreign key to the offline application and constraints to make sure exactly one is populated. It sounds like these kind of checks do not exist for placementRequests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a type in the Application which I guess indicates the type that it may be cast into. That type is not, in itself typed so I don't know that the values are sensible in the test data though.

const application = placementRequest.application as ApprovedPremisesApplication
licenceExpiryDate = application.licenceExpiryDate
}
return {
key: {
text: 'Licence expiry date',
},
value: {
text: licenceExpiryDate ? DateFormats.isoDateToUIDate(licenceExpiryDate) : '',
},
}
}

export const startDateObjFromParams = (params: { startDate: string } | ObjectWithDateParts<'startDate'>) => {
if (params['startDate-day'] && params['startDate-month'] && params['startDate-year']) {
Expand Down Expand Up @@ -444,9 +484,9 @@ export const lengthOfStayRow = (lengthInDays: number) => ({
},
})

export const postcodeRow = (postcodeDistrict: PlacementRequestDetail['location']) => ({
export const preferredPostcodeRow = (postcodeDistrict: PlacementRequestDetail['location']) => ({
key: {
text: 'Postcode',
text: 'Preferred postcode',
},
value: {
text: postcodeDistrict,
Expand All @@ -464,7 +504,7 @@ export const placementRequestSummaryListForMatching = (placementRequest: Placeme
DateFormats.dateObjToIsoDate(calculateDepartureDate(placementRequest.expectedArrival, placementRequest.duration)),
),
lengthOfStayRow(placementRequest.duration),
postcodeRow(placementRequest.location),
preferredPostcodeRow(placementRequest.location),
apTypeRow(placementRequest.type),
]

Expand Down
Loading
Loading