PRSD-689: Creates UpdateJourney base class #239

isobel-softwire · 2025-02-26T12:36:12Z

Renames landlord update properties where not specific enough
Makes Journey responsible for determining its path segment (rather than JourneyType)
Uses journeyDataKey to index JourneyData
Creates base UpdateJourney base class
Moves PropertyRegistrationJourney.initialiseJourneyDataIfNotInitialised into JourneyWithTaskList
Update affected tests

AEPR

My immediate thought is that we're passing around the journeyDataKey far too much - it's ended up everywhere. As it stands it's going to make the whole journey logic so much harder to understand. Especially because we're often passing both the data and the key around together - which shouldn't be necessary. The purpose of the key is to be able to get the data, after all!

I think for this all to work, we should basically only retrieve the journeyData from the session once per request and then just pass it around.

I've not looked through this whole review thoroughly, but I think that will need to be addressed before we can really look at everything else. If the above ends up requiring anything more than a minimal refactor you should check in with @Travis-Softwire before starting on it, and it could probably get its own PR.

Edit: See other comment!

AEPR · 2025-02-27T07:59:17Z

I've had another thought overnight about how we could make this all work without having to do a big refactor. Since the JourneyDataService is already request scoped, we could cache the key on a class variable on JourneyDataService (with some appropriate guards to fail early if something unexpected happens) and then we can continue to use the JourneyDataService as a source of journey data throughout the code without needing to pass the key around everywhere.

isobel-softwire · 2025-02-27T12:10:08Z

I've had another thought overnight about how we could make this all work without having to do a big refactor. Since the JourneyDataService is already request scoped, we could cache the key on a class variable on JourneyDataService (with some appropriate guards to fail early if something unexpected happens) and then we can continue to use the JourneyDataService as a source of journey data throughout the code without needing to pass the key around everywhere.

Implemented this and it looks like it's working - thanks! The PR's ready for another review

src/main/kotlin/uk/gov/communities/prsdb/webapp/forms/journeys/Journey.kt

src/main/kotlin/uk/gov/communities/prsdb/webapp/forms/journeys/LandlordRegistrationJourney.kt

src/main/kotlin/uk/gov/communities/prsdb/webapp/forms/journeys/JourneyWithTaskList.kt

src/main/kotlin/uk/gov/communities/prsdb/webapp/services/JourneyDataService.kt

…tInitialised into JourneyWithTaskList

AEPR

Lots of good stuff here. I've left a couple of thoughts on stuff I would change though.

I've only reviewed the main code, not the tests yet!

src/main/kotlin/uk/gov/communities/prsdb/webapp/services/JourneyDataService.kt

src/main/kotlin/uk/gov/communities/prsdb/webapp/forms/journeys/JourneyWithTaskList.kt

src/main/kotlin/uk/gov/communities/prsdb/webapp/forms/journeys/Journey.kt

src/main/kotlin/uk/gov/communities/prsdb/webapp/forms/journeys/LaUserRegistrationJourney.kt

src/main/kotlin/uk/gov/communities/prsdb/webapp/forms/journeys/UpdateJourney.kt

AEPR · 2025-02-28T11:59:28Z

src/main/kotlin/uk/gov/communities/prsdb/webapp/forms/journeys/UpdateJourney.kt

+    abstract val updateStepId: StepId
+
+    protected val originalDataKey = "ORIGINAL_$journeyType.name"
+
+    override fun getUnreachableStepRedirect(journeyData: JourneyData) =
+        if (journeyData[originalDataKey] == null) {
+            updateStepId.urlPathSegment
+        } else {
+            last().step.id.urlPathSegment
+        }
+
+    override fun iterator(): Iterator<StepDetails<T>> {
+        val journeyData = journeyDataService.getJourneyDataFromSession()
+
+        val originalData = JourneyDataHelper.getPageData(journeyData, originalDataKey)
+
+        // For any fields where the data is updated, replace the original value with the new value
+        val updatedData =
+            journeyData.keys
+                .union(originalData?.keys ?: setOf())
+                .map { key ->
+                    key to if (journeyData.containsKey(key)) journeyData[key] else originalData?.get(key)
+                }.associate { it }
+
+        return ReachableStepDetailsIterator(updatedData, steps, initialStepId, validator)
+    }


I think there's a problem here with what this abstract class is responsible for and what it delegates to its children. This says:

If originalDataKey has not been set, redirect to the step with the UpdateStepId

Whenever we traverse the journey, combine the originalDataKey data with the main journey data.

This works at the moment because the only subclass does get the originalDataKey set exactly when you redirect to the update step. I think that, conceptually, this abstract class needs to be aware of the data being set and so it can complete the loop of:

If originalDataKey has not been set, redirect to where...

We set data for the originalDataKey so that...

Whenever we traverse the journey, we can combine the originalDataKey data with the main journey data.

What I think that looks like is:

moving initialiseJourneyDataIfNotInitialised to the abstract UpdateJourney

making what is currently createOriginalLandlordJourneyData an override of an abstract method on UpdateJourney

(potentially) creating a function here populateModelAndGetTemplateForUpdateStep, which calls initialise and then populateModel... with the updateStepId

…e initialisation

…method

AEPR

A couple of comments on the tests!

AEPR · 2025-02-28T15:55:30Z

src/main/kotlin/uk/gov/communities/prsdb/webapp/services/JourneyDataService.kt

+    fun getJourneyDataFromSession(): JourneyData =
+        try {
+            objectToStringKeyedMap(session.getAttribute(journeyDataKey)) ?: mapOf()
+        } catch (exception: UninitializedPropertyAccessException) {
+            throw PrsdbWebException("journeyDataKey has not been set")
+        }


As discussed - prefer:

if(!this::journeyDataKey.isInitialized) { throw PrsdbWebException("journeyDataKey has not been set") } objectToStringKeyedMap(session.getAttribute(journeyDataKey)) ?: mapOf()

AEPR · 2025-02-28T16:02:46Z

src/test/kotlin/uk/gov/communities/prsdb/webapp/services/JourneyDataServiceTests.kt

@@ -176,6 +176,7 @@ class JourneyDataServiceTests {
            val journeyType = JourneyType.LANDLORD_REGISTRATION


I think we need to write some new tests covering the new, basic functionality of this service, something like:

Once a key is set is is used

If a different key is used, it throws

If you try to get/set/clear without first setting the key, it throws

AEPR · 2025-02-28T16:09:01Z

src/test/kotlin/uk/gov/communities/prsdb/webapp/forms/PropertyRegistrationJourneyTests.kt

-                argThat { households -> households == 0 },
-                argThat { tenants -> tenants == 0 },
-                any(),
-            )
-        }
-
-        @Test
-        fun `when a custom property type is chosen and then replaced, the custom type is not saved to the database`() {
-            // Arrange
-            val journeyData =
-                JourneyDataBuilder
-                    .propertyDefault(addressDataService, localAuthorityService)
-                    .withPropertyType(PropertyType.OTHER, "Bungalow")
-                    .withPropertyType(PropertyType.FLAT)
-
-            whenever(mockJourneyDataService.getJourneyDataFromSession()).thenReturn(journeyData.build())
-
-            // Act
-            completeStep(RegisterPropertyStepId.Declaration)
-
-            // Assert
-            verify(mockPropertyRegistrationService).registerPropertyAndReturnPropertyRegistrationNumber(
-                any(),
-                argThat { type -> type == PropertyType.FLAT },
-                any(),
-                any(),
-                any(),
-                any(),
-                any(),
-                any(),
-            )
-        }
-
-        @Test
-        fun `when a licence is added and then the licence type is changed, only the correct licence number is saved`() {
-            // Arrange
-            val journeyData =
-                JourneyDataBuilder
-                    .propertyDefault(addressDataService, localAuthorityService)
-                    .withLicensingType(LicensingType.SELECTIVE_LICENCE, LicensingType.SELECTIVE_LICENCE.toString())
-                    .withLicensingType(LicensingType.HMO_MANDATORY_LICENCE, LicensingType.HMO_MANDATORY_LICENCE.toString())
-
-            whenever(mockJourneyDataService.getJourneyDataFromSession()).thenReturn(journeyData.build())
-
-            // Act
-            completeStep(RegisterPropertyStepId.Declaration)
-
-            // Assert
-            verify(mockPropertyRegistrationService).registerPropertyAndReturnPropertyRegistrationNumber(
-                any(),
-                any(),
-                argThat { type -> type == LicensingType.HMO_MANDATORY_LICENCE },
-                argThat { licenceNumber -> licenceNumber == LicensingType.HMO_MANDATORY_LICENCE.toString() },
-                any(),
-                any(),
-                any(),
-                any(),
-            )
-        }
-
-        @Test
-        fun `when a licence is added and then licensing is removed, no licence number is saved`() {
-            // Arrange
-            val journeyData =
-                JourneyDataBuilder
-                    .propertyDefault(addressDataService, localAuthorityService)
-                    .withLicensingType(LicensingType.SELECTIVE_LICENCE, LicensingType.SELECTIVE_LICENCE.toString())
-                    .withLicensingType(LicensingType.NO_LICENSING)
-
-            whenever(mockJourneyDataService.getJourneyDataFromSession()).thenReturn(journeyData.build())
-
-            // Act
-            completeStep(RegisterPropertyStepId.Declaration)
-
-            // Assert
-            verify(mockPropertyRegistrationService).registerPropertyAndReturnPropertyRegistrationNumber(
-                any(),
-                any(),
-                argThat { type -> type == LicensingType.NO_LICENSING },
-                argThat { licenceNumber -> licenceNumber.isNullOrBlank() },
-                any(),
-                any(),
-                any(),
-                any(),
-            )
-        }
-
-        private fun completeStep(
-            stepId: RegisterPropertyStepId,
-            pageData: PageData = mapOf(),
-        ) {
-            testJourney.updateJourneyDataAndGetViewNameOrRedirect(
-                stepId = stepId,
-                pageData = pageData,
-                subPageNumber = null,
-                principal = mock(),
-                model = mock(),
-            )
-        }
-    }


Please don't delete all the tests I wrote for PRSD-804!

AEPR · 2025-02-28T16:11:39Z

src/test/kotlin/uk/gov/communities/prsdb/webapp/forms/JourneyTests.kt

+    @Nested
+    inner class JourneyDataManipulationTests {
+        @Test
+        fun `when there is no journey data in the session or the database, journey data is not loaded`() {
+            val principalName = "principalName"
+            val testJourney =
+                PropertyRegistrationJourney(


Appreciate you've moved the logic for this into Journey so you've moved the tests too, which makes sense. Can you update these tests to use the TestJourney from above instead of the PropertyRegistrationJourney?

isobel-softwire force-pushed the chore/PRSD-689-create-update-journey-class branch from f082376 to 960d45c Compare February 26, 2025 14:04

AEPR requested changes Feb 26, 2025

View reviewed changes

isobel-softwire force-pushed the chore/PRSD-689-create-update-journey-class branch from 960d45c to 90bd1dc Compare February 27, 2025 12:07

isobel-softwire marked this pull request as ready for review February 27, 2025 12:08

isobel-softwire requested a review from AEPR February 27, 2025 12:10

isobel-softwire force-pushed the chore/PRSD-689-create-update-journey-class branch from 90bd1dc to 1a16a66 Compare February 27, 2025 12:10

rowanhill reviewed Feb 27, 2025

View reviewed changes

isobel-softwire added 6 commits February 28, 2025 09:45

PRSD-689: Renames landlord update journey properties to be more specific

848a0a0

PRSD-689: Makes journeys responsible for determining their url segment

3741bd4

PRSD-689: Uses journeyDataKey to index journeyData

af30eeb

PRSD-689: Moves PropertyRegistrationJourney.initialiseJourneyDataIfNo…

039e4c3

…tInitialised into JourneyWithTaskList

PRSD-689: Creates UpdateJourney base class

e883455

PRSD-689: Updates affected tests

060a937

isobel-softwire force-pushed the chore/PRSD-689-create-update-journey-class branch from 1a16a66 to 9d5e593 Compare February 28, 2025 09:50

isobel-softwire added 2 commits February 28, 2025 09:53

PRSD-689: Refactors Journey property visibility

95a7b4a

PRSD-689: Moves journeyData loading from JourneyWithTaskList to Journey

53f0348

isobel-softwire force-pushed the chore/PRSD-689-create-update-journey-class branch from 9d5e593 to 89b40db Compare February 28, 2025 10:24

PRSD-689: Refactors interactions with journeyDataKey

afa9575

isobel-softwire force-pushed the chore/PRSD-689-create-update-journey-class branch from 89b40db to afa9575 Compare February 28, 2025 10:25

AEPR requested changes Feb 28, 2025

View reviewed changes

isobel-softwire added 5 commits February 28, 2025 12:09

PRSD-689: Throws PRSDWebException if journeyDataKey is accessed befor…

8a85530

…e initialisation

PRSD-689: Narrows StepId type where possible

5882ccc

PRSD-689: Makes defaultJourneyKey an attribute rather than part of a …

bb0fef4

…method

PRSD-689: Fixes typo

9cdf547

PRSD-689: Refactors UpdateJourney

6814679

isobel-softwire force-pushed the chore/PRSD-689-create-update-journey-class branch from 8b26e38 to 6814679 Compare February 28, 2025 15:36

AEPR reviewed Feb 28, 2025

View reviewed changes

PRSD-689: Uses if-statements instead of try-catches

976d0e7

PRSD-689: Fixes UpdateLandlordDetailsJourney initialisation

9b9f2f8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PRSD-689: Creates UpdateJourney base class #239

PRSD-689: Creates UpdateJourney base class #239

isobel-softwire commented Feb 26, 2025 •

edited

Loading

AEPR left a comment •

edited

Loading

AEPR commented Feb 27, 2025

isobel-softwire commented Feb 27, 2025

AEPR left a comment

AEPR Feb 28, 2025

AEPR left a comment

AEPR Feb 28, 2025

AEPR Feb 28, 2025

AEPR Feb 28, 2025

AEPR Feb 28, 2025

		@@ -176,6 +176,7 @@ class JourneyDataServiceTests {
		val journeyType = JourneyType.LANDLORD_REGISTRATION

PRSD-689: Creates UpdateJourney base class #239

Are you sure you want to change the base?

PRSD-689: Creates UpdateJourney base class #239

Conversation

isobel-softwire commented Feb 26, 2025 • edited Loading

AEPR left a comment • edited Loading

Choose a reason for hiding this comment

AEPR commented Feb 27, 2025

isobel-softwire commented Feb 27, 2025

AEPR left a comment

Choose a reason for hiding this comment

AEPR Feb 28, 2025

Choose a reason for hiding this comment

AEPR left a comment

Choose a reason for hiding this comment

AEPR Feb 28, 2025

Choose a reason for hiding this comment

AEPR Feb 28, 2025

Choose a reason for hiding this comment

AEPR Feb 28, 2025

Choose a reason for hiding this comment

AEPR Feb 28, 2025

Choose a reason for hiding this comment

isobel-softwire commented Feb 26, 2025 •

edited

Loading

AEPR left a comment •

edited

Loading