Feat/prsd 792 update landlord date of birth #245

Kiran-R-K · 2025-02-27T18:01:37Z

Created the landlord update date of birth step
Ensured that a landlord with their identity verified can't update their name or date of birth

Kiran-R-K · 2025-02-27T18:07:10Z

.../kotlin/uk/gov/communities/prsdb/webapp/models/viewModels/summaryModels/LandlordViewModel.kt

I refactored this file to be in line with the pattern in PropertyDetailsLandlordViewModel

Bit of a nitpick, but I've had a look at the addRow extension. Couldn't we make the "addRow" logic live on a companion factory function for SummaryListRowViewModel?

Not really the focus of this PR though, so happy to leave it.

AEPR

Couple of very small suggestions on main code. I've not looked through tests yet, but I will now as none of my suggestions would stop me approving!

AEPR · 2025-02-28T12:07:51Z

src/main/kotlin/uk/gov/communities/prsdb/webapp/helpers/JourneyDataHelper.kt

+        fun getValueByKey(
+            journeyData: JourneyData,
+            key: String,
+        ): String? = journeyData[key]?.toString()
+


I think you can probably just do this inline in getIsIdentityVerified, but if you want to keep it I think it should be getStringValueByKey

AEPR · 2025-02-28T12:09:50Z

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

+            nextAction = { journeyData, _ ->
+                if (UpdateLandlordDetailsJourneyDataHelper.getIsIdentityVerified(journeyData)) {
+                    Pair(UpdateDetailsStepId.UpdatePhoneNumber, null)
+                } else {
+                    Pair(UpdateDetailsStepId.UpdateDateOfBirth, null)
+                }
+            },
+            saveAfterSubmit = false,


I don't think we need to repeat the check here - this can just go straight to date of birth because we can't reach the name step if the identity is verified.

We just discussed this, and I will add tests to ensure that date of birth is not reachable by a verified user and then will remove the conditional here

AEPR · 2025-02-28T12:17:41Z

.../kotlin/uk/gov/communities/prsdb/webapp/models/viewModels/summaryModels/LandlordViewModel.kt

Bit of a nitpick, but I've had a look at the addRow extension. Couldn't we make the "addRow" logic live on a companion factory function for SummaryListRowViewModel?

Not really the focus of this PR though, so happy to leave it.

AEPR · 2025-02-28T12:18:29Z

.../kotlin/uk/gov/communities/prsdb/webapp/models/viewModels/summaryModels/LandlordViewModel.kt

-                SummaryListRowViewModel(
+                    if (!landlord.isVerified) "$UPDATE_ROUTE/name" else null,
+                    withChangeLinks,
+                )
+                addRow(
                    "landlordDetails.personalDetails.dateOfBirth",
                    landlord.dateOfBirth,
-                    // TODO: PRSD-792 toggleChangeLink("$UPDATE_ROUTE/date-of-birth"),
-                    null,
-                ),
-                SummaryListRowViewModel(
+                    if (!landlord.isVerified) "$UPDATE_ROUTE/date-of-birth" else null,
+                    withChangeLinks,
+                )


Doing this whole refactor really pays off in making this more readable!

AEPR · 2025-02-28T12:19:28Z

.../kotlin/uk/gov/communities/prsdb/webapp/models/viewModels/summaryModels/LandlordViewModel.kt

+                    addRow(
+                        "landlordDetails.personalDetails.country",
+                        landlord.countryOfResidence,
+                        // TODO: PRSD-688 toggleChangeLink("$UPDATE_ROUTE/country-of-residence"),
+                        null,
+                        withChangeLinks,
+                    )
+                    addRow(
+                        "landlordDetails.personalDetails.nonEnglandOrWalesAddress",
+                        landlord.nonEnglandOrWalesAddress,
+                        // TODO: PRSD-688 toggleChangeLink("$UPDATE_ROUTE/address"),
+                        null,
+                        withChangeLinks,
+                    )
+                    addRow(
+                        "landlordDetails.personalDetails.englandOrWalesAddress",
+                        landlord.address.singleLineAddress,
+                        // TODO: PRSD-688 toggleChangeLink("$UPDATE_ROUTE/contact-address"),
+                        null,
+                        withChangeLinks,


Can you update these comments to remove the now defunct toggleChangeLink?

AEPR · 2025-02-28T12:19:39Z

.../kotlin/uk/gov/communities/prsdb/webapp/models/viewModels/summaryModels/LandlordViewModel.kt

+                    "$UPDATE_ROUTE/phone-number",
+                    withChangeLinks,
+                )
+                addRow(
                    "landlordDetails.personalDetails.englandOrWalesResident",
                    isEnglandOrWalesResident,
                    // TODO: PRSD-688 toggleChangeLink("$UPDATE_ROUTE/country-of-residence"),


And here too.

AEPR

Two more nitpicks on the tests - other than that and my comments from earlier, LGTM.

AEPR · 2025-02-28T14:19:27Z

...in/uk/gov/communities/prsdb/webapp/models/viewModels/summaryModels/LandlordViewModelTests.kt

    @Test
-    fun `LandlordViewModel populates change links in rows that should have them`() {
+    fun `LandlordViewModel populates change links in rows that should have them for an unverified landlord`() {
        // Arrange
-        val testLandlord = MockLandlordData.createLandlord()
+        val testLandlord = MockLandlordData.createLandlord(isVerified = false)
        val changeablePersonalDetailKeys =
            listOf(
                "landlordDetails.personalDetails.name",
                "landlordDetails.personalDetails.emailAddress",
                "landlordDetails.personalDetails.telephoneNumber",
                "landlordDetails.personalDetails.contactAddress",
+                "landlordDetails.personalDetails.dateOfBirth",
+            )
+
+        // Act
+        val viewModel = LandlordViewModel(testLandlord)
+
+        // Assert
+        for (i in viewModel.personalDetails.filter { detail -> detail.fieldHeading in changeablePersonalDetailKeys }) {
+            assertNotNull(i.changeUrl)
+        }
+
+        for (i in viewModel.personalDetails.filter { detail -> detail.fieldHeading !in changeablePersonalDetailKeys }) {
+            assertNull(i.changeUrl)
+        }
+
+        // TODO PRSD-746 change assertion for consentInformation once links have been added
+        viewModel.consentInformation.forEach { consentInformation -> assertNull(consentInformation.changeUrl) }
+    }
+
+    @Test
+    fun `LandlordViewModel populates change links in rows that should have them for a verified landlord`() {
+        // Arrange
+        val testLandlord = MockLandlordData.createLandlord(isVerified = true)
+        val changeablePersonalDetailKeys =
+            listOf(
+                "landlordDetails.personalDetails.emailAddress",
+                "landlordDetails.personalDetails.telephoneNumber",
+                "landlordDetails.personalDetails.contactAddress",


I think it's probably worth commonising and parameterising these tests to reduce repetition. You can use something like:

@ValueSource(booleans = [true, false]) fun `Test name when` (isVerified: Boolean) {

AEPR · 2025-02-28T14:23:46Z

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

+        val newDateOfBirth = LocalDate.of(1992, 2, 2)
+        val updateModel =
+            LandlordUpdateModel(
+                "newEmail",
+                "newName",
+                "new phone number",
+                AddressDataModel.fromAddress(newAddress),
+                newDateOfBirth,


You can declare this date inline in the LandlordUpdateModel constructor because we reference the updateModel in the assertion below.

Kiran-R-K added 3 commits February 27, 2025 17:29

feat/PRSD-792 update landlords date of birth

2564c0b

feat/PRSD-792 added and updated tests for landlords date of birth

bd9c60e

feat/PRSD-792 refactor UpdateLandlordDetailsJourneyTests

08c5b91

Kiran-R-K commented Feb 27, 2025

View reviewed changes

Kiran-R-K added 2 commits February 27, 2025 18:36

feat/PRSD-792 post merge fix

b09fcda

feat/PRSD-792 added data to test

6997f32

Kiran-R-K marked this pull request as draft February 28, 2025 10:50

feat/PRSD-792 store id verification status in journeydata

1d6909f

Kiran-R-K marked this pull request as ready for review February 28, 2025 11:26

AEPR reviewed Feb 28, 2025

View reviewed changes

AEPR approved these changes Feb 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/prsd 792 update landlord date of birth #245

Feat/prsd 792 update landlord date of birth #245

Kiran-R-K commented Feb 27, 2025 •

edited

Loading

Kiran-R-K Feb 27, 2025

AEPR Feb 28, 2025

AEPR left a comment

AEPR Feb 28, 2025

AEPR Feb 28, 2025

Kiran-R-K Feb 28, 2025

AEPR Feb 28, 2025

AEPR Feb 28, 2025

AEPR Feb 28, 2025

AEPR Feb 28, 2025

AEPR left a comment

AEPR Feb 28, 2025

AEPR Feb 28, 2025

Feat/prsd 792 update landlord date of birth #245

Are you sure you want to change the base?

Feat/prsd 792 update landlord date of birth #245

Conversation

Kiran-R-K commented Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AEPR 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

AEPR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kiran-R-K commented Feb 27, 2025 •

edited

Loading