PDJB-990: Skip licensing details during property registration - #1607
PDJB-990: Skip licensing details during property registration#1607bnjn-mt wants to merge 19 commits into
Conversation
da6f948 to
41d3700
Compare
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| val address = addressService.findOrCreateAddress(addressModel) | ||
|
|
||
| val license = | ||
| if (licenseType != LicensingType.NO_LICENSING) { |
There was a problem hiding this comment.
what happened to no_licensing?
There was a problem hiding this comment.
If the user clicks the provide this later button on the licensing type page, licenseType is null and this variable will also be null. This state means the user still needs to provide this information and will see the "Provide this later" text on other pages (CYA/update etc.)
We can create a license with every value of the LicensingType enum.
My reasoning for this is so we can differentiate between NO_LICENSING and null (provide this later).
There was a problem hiding this comment.
makes sense, though I worry that the previous code signature here suggests that a license type of NO_LICENSING used to be stored as null. also, in the LicenseService there is this snippet in the license updating code
if (updateLicenceType == LicensingType.NO_LICENSING) {
license?.let { licenseRepository.delete(license) }
null
}so we may need more work to update the assumption that 'provide later' => null & 'no_licensing' is stored explicitly
There was a problem hiding this comment.
I've reverted this change and created a LicenseService.licenceShouldBeStored method that allows us to keep the existing code signature.
| fun licenceShouldBeStored( | ||
| licenceType: LicensingType?, | ||
| licenseProvideLater: Boolean = false, | ||
| ): Boolean = licenceType != null && licenceType != LicensingType.NO_LICENSING && !licenseProvideLater |
There was a problem hiding this comment.
to check; when would licenseType be null?
There was a problem hiding this comment.
When the user clicks the 'Provide this later' button during the property registration.
There was a problem hiding this comment.
if licenceProvideLater is false and licenceType is not NO_LICENSING those two would imply that the license is not null?
thinking about it, it feels like the reason this is getting hard to reason about is that it's surprising that NO_LICENSING is a license type registered in the enum but PROVIDE_LATER isn't. it leads to code with some quite surprising conditions where we 'just can't' check licenseType
if the database has been laid out this way that's fine, but I think our service layer is suffering. there are only 5 possible states for a license to be in
- selective
- HMO mandatory
- HMO additional
- no license
- provide later
so I think it'll make this code a lot more clearer if we can make the enum reflect that. it's fine if we don't actually end up putting them all in the database (NO_LICENSINGwas already stored as null), but I think this'll help us out now and down the road when it comes to interpreting the database
then the signature of this method could become
fun licenceShouldBeStored(
licenceType: LicensingType
)There was a problem hiding this comment.
Agree it would be nicer to just add an extra enum value here
Travis-Softwire
left a comment
There was a problem hiding this comment.
A couple of minor points, but otherwise LGTM
| licensingType: Licensing type | ||
| noLicensing: None | ||
| licensingProvideLater: Provide this later | ||
| additionalLandlords: |
There was a problem hiding this comment.
These headings are a bit suspect...
There was a problem hiding this comment.
Would something like: forms.checkPropertyAnswers.propertyDetails.licensing.licensingProvideLater be better?
There was a problem hiding this comment.
Sorry it was the new lines below that were suspect - they don't seem related to this ticket!
|
This is ready to merge if all looks good to you @samyou-softwire :) |
Travis-Softwire
left a comment
There was a problem hiding this comment.
A few more comments from me, plus seconding Sammy's earlier suggestion for adding provide later as a licensing type enum value to make some of the checking logic simpler
|
|
||
| @Test | ||
| fun `licenceShouldBeStored returns false when the licensing details are being provided later`() { | ||
| assertFalse(LicenseService.licenceShouldBeStored(null, licenseProvideLater = true)) |
There was a problem hiding this comment.
I think this is passing because you're passing in null rather than testing the actual functionality? Should it be e.g. assertFalse(LicenseService.licenceShouldBeStored(LicensingType.SELECTIVE_LICENCE, licenseProvideLater = true)) instead?
| fun licenceShouldBeStored( | ||
| licenceType: LicensingType?, | ||
| licenseProvideLater: Boolean = false, | ||
| ): Boolean = licenceType != null && licenceType != LicensingType.NO_LICENSING && !licenseProvideLater |
There was a problem hiding this comment.
Agree it would be nicer to just add an extra enum value here
| ) : Task<LicensingState>() { | ||
| override fun makeSubJourney(state: LicensingState) = | ||
| subJourney(state) { | ||
| // TODO(PDJB-990): route to the 'provide details about licensing later' page when 'Provide this later' is selected behind the FF: pdjb-939-property-registration-restructure-and-skipping/PROPERTY_REGISTRATION_RESTRUCTURE_AND_SKIPPING |
There was a problem hiding this comment.
Looks like we're not using the feature flag manager in this class - should we be? Or should it be removed?
| licensingType: Licensing type | ||
| noLicensing: None | ||
| licensingProvideLater: Provide this later | ||
| additionalLandlords: |
There was a problem hiding this comment.
My previous comment has been marked as resolved, but these message keys that don't seem related to the ticket are still here?
| null | ||
| }, | ||
| licenseType = state.licensingTypeStep.formModel.notNullValue(LicensingTypeFormModel::licensingType), | ||
| licenseType = state.licensingTypeStep.formModelOrNull?.licensingType, |
There was a problem hiding this comment.
The suggestion RE making provide later a licensing type would also mean that we can keep the notNullValue here which guards against us registering a property with a licensing type of null
|
I've made the suggested changes in this commit @Travis-Softwire @samyou-softwire |
Travis-Softwire
left a comment
There was a problem hiding this comment.
This is looking great now, thanks for persevering! I've left one Q, and it looks like there are some merge conflicts to resolve. Assuming those are relatively simple then this all LGTM (if they turn out to require some more substantive changes then shout and I can quickly re-review)
| routeSegment(ProvideLicensingLaterStep.ROUTE_SEGMENT) | ||
| parents { journey.licensingTypeStep.hasOutcome(LicensingTypeMode.PROVIDE_LATER) } | ||
| nextStep { exitStep } | ||
| savable() |
There was a problem hiding this comment.
What's the reason for this step being savable but the others aren't?
Ticket number
PDJB-990
Goal of change
Allow landlords to defer providing licensing details during the property registration journey via a "Provide this later" option, mirroring the existing skip behaviour for compliance certificates.
Description of main change(s)
PROPERTY_REGISTRATION_RESTRUCTURE_AND_SKIPPINGfeature flag.forms.ymlandregisterProperty.yml.Checklist