APS 889 - Add OOS bed update page #1980

richpjames · 2024-07-10T13:03:02Z

Context

Users need to be able to update an Out of service bed record with revisions.

Jira

Changes in this PR

We add the form to enable revisions
Allow the revision to be posted to the API
We add error handling to ensure the users see errors the API returns

Screenshots of UI changes

Mock

Development

edavey

Really clear, thks!

A couple of suggestions:

I think we ought to be explicit (even though verbose) when referring to an OutOfServiceBed as distinct from a Bed
it's worth considering modelling these updates as separate REST resources in your URL scheme. You've already recognised their difference with a separate controller, but with a separate url scheme we could drop the bedId (and even the premisesId) from the URL scheme. Not sure if that would be a "now" refactoring or not though!

edavey · 2024-07-10T15:26:08Z

.env.example

@@ -13,8 +13,6 @@ CAS1_E2E_EMERGENCY_ASSESSOR_NAME_TO_ALLOCATE_TO="JIM SNOW"
 CAS1_E2E_FUTURE_MANAGER_PASSWORD=*REDACTED*
 CAS1_E2E_FUTURE_MANAGER_USERNAME=APPROVEDPREMISESTESTUSER
 CAS1_E2E_LEGACY_MANAGER_PASSWORD=secret
-CAS1_E2E_LEGACY_MANAGER_PASSWORD=secret
-CAS1_E2E_LEGACY_MANAGER_USERNAME=JIMSNOWLDAP


edavey · 2024-07-10T15:45:59Z

server/controllers/v2Manage/updateOutOfServiceBedsController.test.ts

+
+      expect(response.render).toHaveBeenCalledWith(
+        'v2Manage/outOfServiceBeds/update',
+        expect.objectContaining({
+          premisesId: request.params.premisesId,
+          bedId: request.params.bedId,
+          id: request.params.id,
+        }),


another way to approach this would be to think of the update as a fresh resource rather than an additional action on an existing resource, e.g. with a url scheme like:

GET manage/out-of-service-beds/{outOfServiceBedId}/update/new POST manage/out-of-service-beds/{outOfServiceBedId}/update

which services an OutOfServiceBedsUpdateController

I'm not sure how much of the deeply nested url information is necessary?

Currently we have manage/premises/{premisesId}/beds/{bedId}/out-of-service-beds/{outOfServiceBedId}/update

With the OOSB id we will have access to the IDs and names of the bed, room and premises:

"bed": { "id": "3fa85f64-5717-4562-b3fc-2c963f66afa6", "name": "string" }, "room": { "id": "3fa85f64-5717-4562-b3fc-2c963f66afa6", "name": "string" }, "premises": { "id": "3fa85f64-5717-4562-b3fc-2c963f66afa6", "name": "string" },

see the Swagger docs

edavey · 2024-07-10T15:47:46Z

server/controllers/v2Manage/updateOutOfServiceBedsController.ts

+      const { premisesId, bedId, id } = req.params
+
+      const outOfServiceBedReasons = await this.outOfServiceBedService.getOutOfServiceBedReasons(req.user.token)
+      const outOfServiceBed = await this.outOfServiceBedService.getOutOfServiceBed(req.user.token, premisesId, id)
+
+      res.render('v2Manage/outOfServiceBeds/update', {
+        pageHeading: 'updateOutOfServiceBedsController',
+        premisesId,
+        bedId,


I think bedId is probably dangerous here? That outOfServiceBedId is going to be more helpful, even though more verbose?

edavey · 2024-07-10T16:06:02Z

server/paths/manage.ts

+    update: outOfServiceBedPath.path('/update'),
    cancel: singlePremisesPath.path('out-of-service-beds').path(':id').path('cancellations'),
  },


Just wondering whether more smaller things might be easier to manage? See my later question about whether an entirely new "resource" might be clearer? e.g. something like:

const outOfServiceBedsUpdatesPath = outOfServiceBedsIndexPath.path(':outOfServiceBedId') // Path<"/manage/out-of-service-beds/:outOfServiceBedId"> outOfServiceBedUpdates: { new: outOfServiceBedsUpdatesPath.path('new'), create: outOfServiceBedsUpdatesPath, },

edavey · 2024-07-10T16:10:40Z

server/views/v2Manage/outOfServiceBeds/show.njk

@@ -30,7 +30,7 @@
              items: [
                {
                  text: "Update record",
-                  href: paths.v2Manage.outOfServiceBeds.update({premisesId: premisesId, id: id})
+                  href: paths.v2Manage.outOfServiceBeds.update({premisesId: premisesId, id: id, bedId: bedId})


Probably I'm missing something but I think we want to be using the OutOfServiceBed ID here rather than the Bed ID? When we're making a new OOSB then clearly the Bed is a key component, but when we are creating an OOSBUpdate, we are concerned with the OOSB ID and the bed id is no longer important?

edavey · 2024-07-10T16:13:38Z

integration_tests/pages/page.ts

+  shouldShowOutOfServiceBedDetails(bedDetails: OutOfServiceBed | OutOfServiceBedRevision) {
+    if (bedDetails.startDate) {


Again I wonder if we should be a bit stricter with our terminology to avoid future confusion when reading this: i.e. this variable is more correctly an outOfServiceBedDetails isn't it?

edavey · 2024-07-10T16:15:26Z

integration_tests/tests/v2Manage/future_manager/updatesOutOfServiceBed.cy.ts

+    updatePage.shouldShowOutOfServiceBedDetails(outOfServiceBed)
+    updatePage.formShouldBePrepopulated()


edavey · 2024-07-10T16:20:47Z

integration_tests/pages/v2Manage/outOfServiceBeds/outOfServiceBedShow.ts

@@ -71,6 +71,10 @@ export class OutOfServiceBedShowPage extends Page {
    })
  }

+  shouldShowUpdateConfirmationMessage() {
+    this.shouldShowBanner('The out of service bed has been updated')


👍 I see this matches what we have in the 'create' action: https://github.com/ministryofjustice/hmpps-approved-premises-ui/blob/main/server/controllers/v2Manage/outOfServiceBedsController.test.ts#L95-L125

To my mind this does read better than "The out of service bed record has been updated" but we can see what others think...

edavey · 2024-07-10T16:22:04Z

server/controllers/v2Manage/updateOutOfServiceBedsController.test.ts

+        paths.v2Manage.outOfServiceBeds.show({
+          premisesId,
+          bedId: outOfServiceBed.bed.id,
+          id: outOfServiceBed.id,
+          tab: 'timeline',


pageHeading is the name of the variable in the view

We will soon use this is multiple places so we should add a constant to reduce duplication

Previously we didn't use the update OoS bed path in v2Manage so the path wasn't updated. But now we need it to work.

This was copied over when the lost beds controller was. For new development, given the size of the out of service beds controller, I think it's best we start a new controller for the updating of OoS beds so these methods can be removed

We will need this on multiple Pages soon so to remove duplication I have extracted this method to the top level Page class. There's a lot of methods on the Page class and it would make things clearer if we used more inheritance, but this is out of scope for now.

We are using this same style of heading in multiple places so it makes sense to keep it DRY. Especially as we may soon link to the premises/bed from this heading.

There is a lot of boiler plate here but we are essentially adding a new controller that renders the form that allows the bed to be updated. We also add a test to verify that it loads

When the form loads initially we want to show the existing data in the fields for the user to edit and update rather than have to start from fresh

Now we allow the form to be submitted and the updated bed to be submitted to the API.

We need to handle errors provided by the API so that we show them in the UI

Before this change if the API threw an error when the page was loaded the inputs of the form would be populated with the data from the OoS bed not the erroneously inputted data. We want to show the erroneous data so the user can see where they've gone wrong and correct their mistake. I'm not happy with the amount of casting I've had to do here but time doesn't allow for type assertions currently.

richpjames force-pushed the APS-889-create-OoS-bed-update-page branch 3 times, most recently from 26555d8 to acacbe2 Compare July 10, 2024 13:29

richpjames marked this pull request as ready for review July 10, 2024 13:42

richpjames force-pushed the APS-889-create-OoS-bed-update-page branch 3 times, most recently from 7a326e3 to 742f091 Compare July 10, 2024 14:36

edavey approved these changes Jul 10, 2024

View reviewed changes

rich added 9 commits July 11, 2024 10:39

Remove duplicates from .example.env

cccd603

Correct heading

d244da7

pageHeading is the name of the variable in the view

Create a constant for the outOfServiceBed path

0571723

We will soon use this is multiple places so we should add a constant to reduce duplication

Correct update path

47068b2

Previously we didn't use the update OoS bed path in v2Manage so the path wasn't updated. But now we need it to work.

Remove update method from OoS beds controller

0202507

This was copied over when the lost beds controller was. For new development, given the size of the out of service beds controller, I think it's best we start a new controller for the updating of OoS beds so these methods can be removed

Use macro for OoS bed page

427d9a4

We are using this same style of heading in multiple places so it makes sense to keep it DRY. Especially as we may soon link to the premises/bed from this heading.

Add view for OoS bed update form

fe85fd5

There is a lot of boiler plate here but we are essentially adding a new controller that renders the form that allows the bed to be updated. We also add a test to verify that it loads

Verify prepopulation of OoS bed data

03a8d33

When the form loads initially we want to show the existing data in the fields for the user to edit and update rather than have to start from fresh

richpjames force-pushed the APS-889-create-OoS-bed-update-page branch from 742f091 to 5d281d0 Compare July 11, 2024 09:40

rich added 3 commits July 11, 2024 11:00

Allow for form to be submitted

aec075d

Now we allow the form to be submitted and the updated bed to be submitted to the API.

Add error handling to update form

e74583f

We need to handle errors provided by the API so that we show them in the UI

richpjames force-pushed the APS-889-create-OoS-bed-update-page branch from 5d281d0 to 300b5cd Compare July 11, 2024 10:00

richpjames enabled auto-merge July 11, 2024 10:00

richpjames merged commit f54be31 into main Jul 11, 2024
7 checks passed

richpjames deleted the APS-889-create-OoS-bed-update-page branch July 11, 2024 10:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

APS 889 - Add OOS bed update page #1980

APS 889 - Add OOS bed update page #1980

richpjames commented Jul 10, 2024 •

edited

Loading

edavey left a comment

edavey Jul 10, 2024

edavey Jul 10, 2024

edavey Jul 10, 2024

edavey Jul 10, 2024

edavey Jul 10, 2024

edavey Jul 10, 2024

edavey Jul 10, 2024

edavey Jul 10, 2024

edavey Jul 10, 2024

		shouldShowOutOfServiceBedDetails(bedDetails: OutOfServiceBed \| OutOfServiceBedRevision) {
		if (bedDetails.startDate) {

		updatePage.shouldShowOutOfServiceBedDetails(outOfServiceBed)
		updatePage.formShouldBePrepopulated()

APS 889 - Add OOS bed update page #1980

APS 889 - Add OOS bed update page #1980

Conversation

richpjames commented Jul 10, 2024 • edited Loading

Context

Changes in this PR

Screenshots of UI changes

Mock

Development

edavey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richpjames commented Jul 10, 2024 •

edited

Loading