Refactor placement-request page information #2280
Conversation
| }) | ||
| export const licenceExpiryDateRow = (placementRequest: PlacementRequestDetail) => { | ||
| let licenceExpiryDate: string | undefined | ||
| if (placementRequest.application && 'licenceExpiryDate' in placementRequest.application) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| ).toEqual(expectedMatchingDetailsSummaryListItems('')) | ||
| }) | ||
|
|
||
| it(`should generate the expected matching details when placement-request's application is not of type ApprovedPremisesApplication`, () => { |
There was a problem hiding this comment.
justification for this test case is in a comment in the index.ts file of this PR
| }) | ||
| export const licenceExpiryDateRow = (placementRequest: PlacementRequestDetail) => { | ||
| let licenceExpiryDate: string | undefined | ||
| if (placementRequest.application && 'licenceExpiryDate' in placementRequest.application) { |
There was a problem hiding this comment.
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.
| }) | ||
| export const licenceExpiryDateRow = (placementRequest: PlacementRequestDetail) => { | ||
| let licenceExpiryDate: string | undefined | ||
| if (placementRequest.application && 'licenceExpiryDate' in placementRequest.application) { |
There was a problem hiding this comment.
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.
| `) | ||
| }) | ||
|
|
||
| it('should return Not Found if the person is unknown person', () => { |
There was a problem hiding this comment.
Nice - I see these tests are replicated in personUtils.test.ts
|
|
||
| it('should generate the expected summary list', () => { | ||
| expect(placementRequestSummaryList(placementRequest).rows).toEqual( | ||
| expectedSummaryListItems( |
There was a problem hiding this comment.
The long list of parameters is a little hard to follow. Perhaps consider passing an args object so that each parameter is named
| @@ -8,7 +8,7 @@ export const preferredApsRow = (placementRequest: PlacementRequestDetail): Summa | |||
| if (premises.length) { | |||
| const apList = premises.map(p => `<li>${p.name}</li>`) | |||
| return { | |||
| key: { text: 'Preferred APs' }, | |||
| key: { text: 'Preferred AP' }, | |||
There was a problem hiding this comment.
Could you check this with Stevie or Amber. It seems a bit odd to change to singular when clearly it's a list. Maybe it's a typo.
Context
Placement requestpage in theFind and Bookflow. This includes removing the data that overlaps with thre blue banner and moving the rest of the data to a summary list.Changes in this PR
Screenshots of UI changes
Before
After