PDJB-942 - Provide Tenancy Details Later Changes - #1641
Conversation
…utes through to a new Provide this later page.
# Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
…flag is enabled / disabled
…s later button works
…ed for when the ff is disabled
…-later-tenancy-details
…en there are no occupants
…ater to progress to cya
TomHanmer29
left a comment
There was a problem hiding this comment.
LGTM overall, just a few comments on style! Happy to approve after the bit with the feature flag in the journey framework is addressed, and please shout if there's anything you want to chat about around that, as it can be a little confusing :)
Travis-Softwire
left a comment
There was a problem hiding this comment.
I haven't reviewed the tests yet, but I think there are tweaks needed and I had a few questions so posting the review now. I'll come back when I've reviewed the tests as well with any further comments there
Travis-Softwire
left a comment
There was a problem hiding this comment.
And few more minor points on the tests
| val content = stepConfig.getStepSpecificContent(mockState) | ||
|
|
||
| // Assert | ||
| assertEquals("forms.buttons.saveAndContinue", content["submitButtonText"]) |
There was a problem hiding this comment.
V. minor but I think elsewhere we split this into separate tests for content and the template
There was a problem hiding this comment.
Done - split into separate tests
|
@TomHanmer29 @Travis-Softwire thanks for your reviews! |
df6d8c0 to
00f3678
Compare
TomHanmer29
left a comment
There was a problem hiding this comment.
One tiny point, and a quick check on the feature flagging. Otherwise happy, will leave to @Travis-Softwire to approve :)
Travis-Softwire
left a comment
There was a problem hiding this comment.
A few more points, all pretty minor. Should be good to go once they're addressed - the only one that I might want to see again if you disagree with the comment or the change is non-trivial is the point about forcing the user through the rent screens when they update the number of households - shout if that's a problem
| HouseholdStep.ROUTE_SEGMENT, TenantsStep.ROUTE_SEGMENT -> { | ||
| duplicableCheckAnswerTask(journey.householdsAndTenantsTask, { HouseHoldsAndTenantsDependencies(true) }) | ||
| if (featureFlagManager.checkFeature(PROPERTY_REGISTRATION_RESTRUCTURE_AND_SKIPPING)) { | ||
| duplicableCheckAnswerTask(journey.tenancyDetailsTask) |
There was a problem hiding this comment.
Won't this force them through the rent section just to change the number of households or tenants? I don't think we want to do that
There was a problem hiding this comment.
Yeah you are right - this is for the next PR for the new CYA designs. Will get this sorted
| rentAmount = anyOrNull(), | ||
| customPropertyType = anyOrNull(), | ||
| markedJointLandlord = eq(true), | ||
| tenancyProvideLater = anyOrNull(), |
There was a problem hiding this comment.
Do we have tests that check we're correctly saving this when tenancyProvideLater is true?
There was a problem hiding this comment.
Good spot, added a test
| epcExemptionReason: EpcExemptionReason? = null, | ||
| epcMeesExemptionReason: MeesExemptionReason? = null, | ||
| epcProvideLater: Boolean? = null, | ||
| tenancyProvideLater: Boolean = false, |
There was a problem hiding this comment.
Out of interest, why are the other provide later's nullable with a default of null and this one is not nullable with a default of false?
There was a problem hiding this comment.
I can't think of a valid reason, I'll update this to be consistent with the other provide later bools
| mutableListOf<SummaryListRowViewModel>() | ||
| .apply { | ||
| val householdsStep = householdsAndTenantsState.households | ||
| if (householdsStep.outcome == HouseholdMode.PROVIDE_THIS_LATER) { |
There was a problem hiding this comment.
We also call this from in getCheckYourTenancyDetailsAnswersSummaryList which is also calling getRentBillsAndFurnishingsSummaryList which might not have been provided?
There was a problem hiding this comment.
Fixed — added a guard in getCheckYourTenancyDetailsAnswersSummaryList so it only calls getRentBillsAndFurnishingsSummaryList when provideTenancyDetailsLater is false, mirroring the existing guard in getRestructuredOccupiedTenancyDetailsSummaryList.
| getCheckYourHouseHoldsAndTenantsAnswersSummaryList(state, state.householdsAndTenantsTask) + | ||
| getBedroomsRow(state) + | ||
| getRentBillsAndFurnishingsSummaryList(state, messageSource) | ||
| if (state.householdsAndTenantsTask.households.outcome == HouseholdMode.PROVIDE_THIS_LATER) { |
There was a problem hiding this comment.
Do we ever call this function if provide this later is turned on?
There was a problem hiding this comment.
it can be called with provide-later on (when the property is occupied but tenancy details are being provided later)
| <p class="govuk-body" th:text="#{registerProperty.provideTenancyDetailsLater.whenToProvide.paragraph}">registerProperty.provideTenancyDetailsLater.whenToProvide.paragraph</p> | ||
|
|
||
| <p class="govuk-body" th:text="#{registerProperty.provideTenancyDetailsLater.reason}">registerProperty.provideTenancyDetailsLater.reason</p> | ||
| <ul class="govuk-list govuk-list--bullet" aria-label="We will need to know"> |
There was a problem hiding this comment.
We should use a message key for the aria-label as well (if we actually need it? it's included in the content isn't it?)
There was a problem hiding this comment.
Taking a look at Figma
There was a problem hiding this comment.
I don't think we need this so will remove
| nextStep { journey.taskListStep } | ||
| nextDestination { _ -> | ||
| when { | ||
| state.provideTenancyDetailsLater -> Destination(state.cyaStep) |
There was a problem hiding this comment.
I think these should be journey.<> rather than state.<>
| withHeadingMessageKey("registerProperty.taskList.register.restructureAndSkipping.heading", false) | ||
| } else { | ||
| withHeadingMessageKey("registerProperty.taskList.register.old.heading") | ||
| withHeadingMessageKey("registerProperty.taskList.register.heading") |
There was a problem hiding this comment.
I think the removing of the 'old' segment from this key might be a merge mistake?
|
|
||
| interface PropertyRegistrationJourneyState : | ||
| OccupationState, | ||
| TenancyDetailsState, |
There was a problem hiding this comment.
OccupationState already inherits TenancyDetailsState? Or is the idea that OccupationState will be broken up? If so but we're not ready to do that yet should it be a TODO?
| val taskListPage = | ||
| goToRestructuredPropertyRegistrationTaskList( | ||
| PropertyStateSessionBuilder.beforeTenancyDetails(), | ||
| beforeTenancyDetails(), |
There was a problem hiding this comment.
I think all the other calls to this use PropertyStateSessionBuilder.beforeTenancyDetails
Ticket number
PDJB-942
Goal of change
Populate Provide tenancy details later page. The 'Save and continue' button takes the Landlord to the CYA page, skipping the Rent pages, and marking Tenancy details as Complete.
Description of main change(s)
Screenshots
**Occupied Property > Households > Provide this later**Provide this later

Provide this later > CYA

Note: this will be updated to match Figma in the next PR.
CYA > Provide this later > Households

Tenancy details marked as Complete after provide later

Regression Test: Providing Households & Number of People information goes through to Bills

Anything you'd like to highlight to the reviewer?
Checklist
Delete any that are not applicable, and add explanation below for any that are applicable but haven't been done