Occupancy View - add license expiry date to matching details section

[gregkhawkins]

Context

• This relates to this ticket: https://dsdmoj.atlassian.net/browse/APS-1624
• We previously de-scoped including the Licence expiry date data from the Matching details section on the Occupancy View page because the data was not available from the API
• The Licence expiry date data has since been made available from the GET /placement-requests/{id} API endpoint

Changes in this PR

• Add the Licence expiry date data into the Matching details section on the Occupancy View

Screenshots of UI changes

Before

After

[bobmeredith]

Looks good.
A couple of comments.

Has there been an API change to return the whole application along with a placement request? If so, that doesn't sound like a good thing from a performance POV. The page controller could always get the application in a separate call if needed.

[bobmeredith]

Why the switch?
Does this mean that the entire Application is now returned along with the placement request in placementRequestService.getPlacementRequest()
That seems like a performance hit as there will be many places where the application is not needed.

[gregkhawkins]

thanks for looking at this @bobmeredith was half expecting this to be an after xmas review. Appreciate it :)

So, in this test code, this function goes on to call the occupancyViewSummaryListForMatchingDetails() function to assert some expectations. To achieve this it needs to pass the correct type - which has changed because of the data-type change late on in the PR:
https://github.com/ministryofjustice/hmpps-approved-premises-ui/pull/2263/files#diff-cc8326f12e1aed4a0d54a6c3199c6dac2af0450fec3103000188aa31f6df42cb

The justification for that data type change is that, in actual fact, we have always been passing an object of type PlacementRequestDetail into that function but the data type was set incorrectly to it's parent type (i.e. PlacementRequest). It wasn't flagging issues (as we were passing a child of that parent) but the change is now needed so I can access the application from the PlacementRequestDetail (so i can go on to grab the licenceExpiryDate later).

Furthermore, in the occupancy view controller, we make the below call:

As you can see above, from the IDE hint, what we get back has always been of type PlacementRequestDetail (not PlacementRequest). And back to the test:

As you can see we were always stubbing PlacementRequestDetail (not the parent PlacementRequest) which is why there are no side effects of this data type change also

[bobmeredith]

Does this need to check the type, or can we guarantee that this Application is always an ApprovedPremisesApplication

[gregkhawkins]

I had a quick look into the BE code and saw that when we hit the GET /placement-requests/{id} endpoint, it essentially goes off and build the placement request including the application. To build the correct application type it executes this code:

So, the is stuff above determines which application type is attached to this placement request and builds it up accordingly. My understanding is that in this area (i.e. Occupancy view) the placements requests will all be of type ApprovedPremisesApplicationEntity as this Occupancy view area is functionality used for CAS1 applications only. Let me know if this is wrong! If not wrong, conclusion is yes we can guarantee that this Application is always an ApprovedPremisesApplication.

Let me know if I'm missing anything :)

[bobmeredith]

Thanks for the explanation. I appreciate it!

[gregkhawkins]

Thanks again for the review!
Re Has there been an API change to return the whole application along with a placement request? As touched on above the BE is returning the whole application and looking at the BE git history it has been returning the application for a while.
Perhaps this is one to raise as a refactor if we want a BE dev ticket split into two calls instead? May of course have side effects that may need investigating, but also worth flagging if we are worried about performance implications

[bobmeredith]

OK, well in that case LGTM.
I do think we should look to refactor as large unnecessary payloads are not a good thing.