PDJB-1168: add change links to org landlord CYA page - #1628
Conversation
ede3c8f to
ae21b59
Compare
Travis-Softwire
left a comment
There was a problem hiding this comment.
One Q - if we've tested this works fine then LGTM. If it doesn't then we might want to recombine the CYA pages rather than trying to figure out a (probably brittle) way of supporting two at once.
e1b3096 to
68cbd95
Compare
68cbd95 to
ff69328
Compare
ff69328 to
1866b39
Compare
1866b39 to
b60759a
Compare
reviewed before major refactor
…ndlord-cya-change-links
samyou-softwire
left a comment
There was a problem hiding this comment.
happy with the tasks now, but I think there's some functional changes on the CYA page with subtasks. I think several route segments pointing to the same CYA journey is likely an antipattern (unless they truly are the 'same' step like the various country charity num questions)
| import uk.gov.communities.prsdb.webapp.journeys.landlordRegistration.stepConfig.OrgIsRegisteredCharityStep | ||
|
|
||
| interface OrgCharityState : JourneyState { | ||
| val orgCharityStep: OrgIsRegisteredCharityStep |
There was a problem hiding this comment.
this name is out of date
| import uk.gov.communities.prsdb.webapp.journeys.landlordRegistration.tasks.OrgGovBodyTask | ||
|
|
||
| interface OrgCompaniesHouseState : JourneyState { | ||
| val orgCompaniesHouseStep: OrgIsRegisteredCompanyStep |
| } | ||
|
|
||
| OrgAddressStep.ROUTE_SEGMENT -> { | ||
| checkAnswerStep(journey.orgLandlordRegistrationTask.orgAddressStep, OrgAddressStep.ROUTE_SEGMENT) |
There was a problem hiding this comment.
maybe since this will need to be updated in PDJB-1133 we should pop a TODO here too
| OrgIsRegisteredCharityStep.ROUTE_SEGMENT, | ||
| OrgCharityRegisteredWithStep.ROUTE_SEGMENT, | ||
| OrgCharityNumberEnglandAndWalesStep.ROUTE_SEGMENT, | ||
| OrgCharityNumberNorthernIrelandStep.ROUTE_SEGMENT, | ||
| OrgCharityNumberScotlandStep.ROUTE_SEGMENT, |
There was a problem hiding this comment.
should these all be in the same branch? if I click change charity number then I shouldn't be sent into the whole charity task
| OrgIsRegisteredCompanyStep.ROUTE_SEGMENT, | ||
| OrgCompanyNumberStep.ROUTE_SEGMENT, |
There was a problem hiding this comment.
I think same here; we could add the company number step CYA page now
| } | ||
| } | ||
|
|
||
| private fun destinationForSelectedLandlordType( |
There was a problem hiding this comment.
is there a precedent for a method like this? worth a comment explaining what this mechanism is for?
| contactAddress: Contact address | ||
| localCouncil: Local Council | ||
| landlordType: Landlord type | ||
| orgCheckAnswers: |
There was a problem hiding this comment.
I wonder if this layout is still useful; implies orgCheckAnswers is still a separate page, though this may be fine as they are quite separate. what do you think?
Travis-Softwire
left a comment
There was a problem hiding this comment.
I think there's some more changes required here but happy for this to merge once the functional ones are addressed - the refactoring suggestions can go in a separate PR as this is getting quite hard to review already!
| (org.governingBodyMembersMap ?: emptyMap()) | ||
| .values | ||
| .toList() | ||
| securityContextService.refreshContext() |
There was a problem hiding this comment.
Minor, but would probably use an else instead of an early return here and put the refresh context call at the end?
| } | ||
| } | ||
|
|
||
| private fun destinationForSelectedLandlordType( |
There was a problem hiding this comment.
I think this would be clearer as a new internal step that computes the outcome of the check as it's mode? That lets us keep the DSL nice and clean
| parents { journey.orgLandlordRegistrationTask.isComplete() } | ||
| parents { | ||
| OrParents( | ||
| journey.individualLandlordLocationTask.isComplete(), |
There was a problem hiding this comment.
I think this needs to be a bit more sophisticated - I think we need to check the landlord type is individual + individual LL location task is complete, or that the landlord type is org + org LL task is complete. Otherwise you could get all the way to CYA as an individual, back out all the way to LL type, change your answer, and then navigate straight back to CYA which would be reachable but none of the relevant fields would have been completed -> null assertion exceptions I think
| } | ||
| } | ||
| } | ||
| step(journey.orgGovBodyWhoToProvideStep) { |
There was a problem hiding this comment.
Didn't we previously override the back destination here so that if you're 'adding another' you can back out to the member list instead of 'hasAnyGovBodyMemberStep'?
There was a problem hiding this comment.
This behaviour is now back since re-refactoring the tasks :)
| nextStep { mode -> | ||
| when (mode) { | ||
| AnyMembers.SOME_MEMBERS -> journey.orgGovBodyMemberListStep | ||
| AnyMembers.NO_MEMBERS -> journey.orgGovBodyWhoToProvideStep |
There was a problem hiding this comment.
I don't think this is right is it? Removing the last member should take them back to hasAnyGovBodyMembersStep
There was a problem hiding this comment.
yes, you're right that it should do that in the initial journey, I routed it like this so the CYA would not take you back outside this inner journey to the page that you can click the links of, but maybe it's simpler to route it normally and fix it by just getting rid of the remove link on the CYA change journey as discussed in handover. I forgot it would break the regular journey
There was a problem hiding this comment.
In fact, I think this was just confusing with the task structure we had before - the hasAnyGovBodyMembersStep is just internal for the looping, if we have no members that's for the initial run through so it should always go to the who to provide step, otherwise it goes to the member list, just like in joint landlords, so fine as is.
| interface OrgCompaniesHouseState : JourneyState { | ||
| val orgCompaniesHouseStep: OrgIsRegisteredCompanyStep | ||
| val orgCompanyNumberStep: OrgCompanyNumberStep | ||
| val orgGovBodyTask: OrgGovBodyTask |
There was a problem hiding this comment.
It feels odd for this to be a child of the CH task - I don't think it needs to be?
There was a problem hiding this comment.
haha yes me and Sammy had a big discussion about this in person which Sammy summarised in this comment so I ended up making it a child
There was a problem hiding this comment.
Hmm, yeah I'm not sure I agree with that conclusion - it feels unintuitive to access gov body fields by going via journey.orgCompaniesHouseTask.orgGovBodyTask.<>. They feel like mutually exclusive siblings to me rather than parent/child. I'd rather it be clear with a bit of repetition in the update journey than confusing but require less duplication
There was a problem hiding this comment.
Cool, I'll move it back out :) The other one I moved to be a subchild at that time was the lead trustee as a subchild of org type - should I change that back too?
|
Will give this one last check over functionality wise, and if there are no problems I'll merge - I'll leave all the other refactor comments (those I've not replied to or reacted to) to another PR as this is getting pretty large |
Ticket number
PDJB-1168
Goal of change
Adds the functionality to the change links to the CYA page for org landlords
Description of main change(s)
Refactors some of the elements of the journey into individual tasks to let them be used as a change link.
Adds the "landlord type" section to the CYA page for individual landlords when the flag is on.
Doesn't include the CYA for org type or companies house
Checklist
Delete any that are not applicable, and add explanation below for any that are applicable but haven't been done
and any related functionality)
mention that here