-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PC-775 - Allow journey to be completed by users in "Pending" LAs #250
PC-775 - Allow journey to be completed by users in "Pending" LAs #250
Conversation
…uthority is not particpating in sign ups through this service.
…ending LA's. Updated Pending page to add confirmation checkbox for user's understanding, with updated viewmodel and controller to reflect this.
…sed until the LA is active. Updated controller to reflect change of questions. Removed some duplicated text from the default partial.
9f711d1
to
a3063d4
Compare
public bool isTrue => true; | ||
|
||
[ModelBinder(typeof(GovUkCheckboxBoolBinder))] | ||
[Compare("isTrue", ErrorMessage = "You must acknowledge that your application will not be processed until your local authority has signed up to use the service")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think something like
[Compare("isTrue", ErrorMessage = "You must acknowledge that your application will not be processed until your local authority has signed up to use the service")] | |
[Range(typeof(bool), "true", "true", ErrorMessage="...")] |
might work? Which would save us the extra property. And if so then it might be nice to extract it to a custom attribute?
Otherwise let's use nameof(isTrue)
to keep us robust to renames.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't realise you could use the Range validation attribute for booleans! It does seem to work when I've been testing it.
I've changed this over, but I can't figure out how to cleanly separate it into its own custom attribute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Range
thing feels a bit weird to me. It's not very obvious what is going on.
I think you're better off just being explicit about the validation.
You can make the view model implement IValidatableObject
:
public class PendingViewModel : QuestionFlowViewModel, IValidatableObject
{
then implement Validate()
as follows:
public IEnumerable<ValidationResult> Validate(ValidationContext validationContext)
{
if (!UserAcknowledgesApplicationNotProcessedUntilLocalAuthorityLive)
{
yield return new ValidationResult(
"You must acknowledge that your application will not be processed until your local authority has signed up to use the service",
new[] { nameof(UserAcknowledgesApplicationNotProcessedUntilLocalAuthorityLive) });
}
}
Let me know if it would be useful to chat through what is going on here. 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to jump on a call to go through it if you want! Don't think this needs to be a blocker though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though I've just seen Jamie's spoilers, I would be interested in also knowing how to implement it as a custom attribute!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, we can chat about it tomorrow if you like
HerPublicWebsite/Views/Partials/LocalAuthorityMessages/Pending/Default.cshtml
Outdated
Show resolved
Hide resolved
bf6a214
to
e517f5f
Compare
…s' into PC-775-Allow-journey-to-be-completed-by-users-in-Pending-LAs # Conflicts: # HerPublicWebsite.BusinessLogic/Services/QuestionFlow/QuestionFlowService.cs # HerPublicWebsite/Controllers/QuestionnaireController.cs
…ixing merge issue (duplicate definition)
HerPublicWebsite.BusinessLogic/Services/QuestionFlow/QuestionFlowService.cs
Outdated
Show resolved
Hide resolved
public bool isTrue => true; | ||
|
||
[ModelBinder(typeof(GovUkCheckboxBoolBinder))] | ||
[Compare("isTrue", ErrorMessage = "You must acknowledge that your application will not be processed until your local authority has signed up to use the service")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Range
thing feels a bit weird to me. It's not very obvious what is going on.
I think you're better off just being explicit about the validation.
You can make the view model implement IValidatableObject
:
public class PendingViewModel : QuestionFlowViewModel, IValidatableObject
{
then implement Validate()
as follows:
public IEnumerable<ValidationResult> Validate(ValidationContext validationContext)
{
if (!UserAcknowledgesApplicationNotProcessedUntilLocalAuthorityLive)
{
yield return new ValidationResult(
"You must acknowledge that your application will not be processed until your local authority has signed up to use the service",
new[] { nameof(UserAcknowledgesApplicationNotProcessedUntilLocalAuthorityLive) });
}
}
Let me know if it would be useful to chat through what is going on here. 🙂
…s' into PC-775-Allow-journey-to-be-completed-by-users-in-Pending-LAs
@@ -357,7 +357,7 @@ private QuestionFlowStep NotParticipatingForwardDestination(Questionnaire questi | |||
|
|||
private QuestionFlowStep PendingForwardDestination(Questionnaire questionnaire) | |||
{ | |||
return QuestionFlowStep.Pending; | |||
return QuestionFlowStep.HouseholdIncome; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SelectAddressForwardDestination()
(which is the point we should be picking up from) has the following options for next steps:
else if (questionnaire.FoundEpcBandIsTooHigh)
{
return QuestionFlowStep.ReviewEpc;
}
// If the LA has changed and the income band the user selected previously is no longer valid then we don't
// go back to the check your answers page as the user will need to select a new income band.
else if (entryPoint is QuestionFlowStep.Address && questionnaire.IncomeBandIsValid)
{
return QuestionFlowStep.CheckAnswers;
}
return QuestionFlowStep.HouseholdIncome;
I think we need to clarify with DESNZ what we should be doing if we should be showing the ReviewEpc
step.
I would expect that we should show that in preference to the Pending page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, I've moved the ReviewEPC check so it catches people before they're caught by the Pending check :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Glenn-Clarke. That being the case, we need to also change the options in ReviewEpcForwardDestination()
to potentially forward you to Pending
.
HerPublicWebsite.BusinessLogic/Services/QuestionFlow/QuestionFlowService.cs
Show resolved
Hide resolved
…ing page to allow for EPC first, and back flow from following page. Fixed formatting in Pending. Fixing merge issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is all looking good for the most part, however I think there are still some complications around the question flow.
I've added comments, but let me know if it would be easier to chat them through as it is quite nitty-gritty.
@@ -357,7 +357,7 @@ private QuestionFlowStep NotParticipatingForwardDestination(Questionnaire questi | |||
|
|||
private QuestionFlowStep PendingForwardDestination(Questionnaire questionnaire) | |||
{ | |||
return QuestionFlowStep.Pending; | |||
return QuestionFlowStep.HouseholdIncome; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Glenn-Clarke. That being the case, we need to also change the options in ReviewEpcForwardDestination()
to potentially forward you to Pending
.
HerPublicWebsite.BusinessLogic/Services/QuestionFlow/QuestionFlowService.cs
Outdated
Show resolved
Hide resolved
…sit address/EPC, and also when a new address is chosen with a different LA status.
…estionnaire, so the box stays filled if the user returns. It is emptied if the user changes their address or LA.
Just pushing before end of day, still need to check it over tomorrow morning to fix issues |
THIS PR BUILDS UPON PC-776
Updated the user journey to allow those who select a "pending" status LA to continue their journey by acknowledging that they won't have their application processed at the current time, and not until the LA begins processing applications.
If the user acknowledges this, they are directed to the income check page, as if they had been part of an "active" LA.