LG-6349: Add verify page to IPP flow#6514
Conversation
| <%= render ValidatedFieldComponent.new( | ||
| name: :first_name, | ||
| form: f, | ||
| input_html: { value: pii[:first_name] }, |
There was a problem hiding this comment.
We could avoid assigning the value explicitly if we initialize SimpleForm with a form object that's populated with the values. Maybe something roughly like:
def extra_view_variables
{
form: Idv::StateIdForm.new(flow_session[:pii_from_user]),
# ...
}
end<%= validated_form_for form do |f| %>It might get a little tricky with the input names though, since FlowStateMachine assumes to pick form values from the flow's name (:doc_auth in this case). We could control that by assigning the form's model_name as 'DocAuth' in all cases, but it feels a little hacky. 🤷
identity-idp/app/forms/idv/state_id_form.rb
Lines 10 to 12 in 7bfdd3b
We don't use this pattern very much throughout the codebase, so not really something I'd push too strongly for, but it would seem to simplify some of these screens where we want to restore existing values to the form inputs.
There was a problem hiding this comment.
Oof, yeah, it feels pretty hacky to me with having to name all of the forms 'DocAuth'. How about I add a ticket to look into this post-pilot? I think there'll be some refactoring work to fully pull the doc auth and IPP flows apart and this could be an enhancement that allows
What's the general attitude towards WILLFIX comments left in the code? I'm thinking I could leave such comments linking to the enhancement ticket but they might be around for quite a few weeks depending on when we can get to it
There was a problem hiding this comment.
@sheldon-b Yeah, I wouldn't consider this a blocker, but I think it is highlighting a couple specific issues:
- That we're extending DocAuth base classes in the non-DocAuth flow
- That FlowStateMachine is looking for a specific, constant form key
- This is related to the first bullet point because it's from how the base step is initialized
- However, it might be nice if
flow_paramscould reference the relevant form in a way that it could pull parameters fromform.class.model_nameinstead
Later refactoring may be fine, though I'd wonder if it would eventually be made redundant as FlowStateMachine v2 would seek to reimplement these steps anyways.
To the question about WILLFIX: You may be able to add it, but generally these kinds of comments are prevented by our CodeClimate configuration.
There was a problem hiding this comment.
Yeah that makes sense. I'll create a ticket to track this tech debt, I would hope that we can prioritize it sometime after the pilot
| <div class='margin-top-4 margin-bottom-2'> | ||
| <% if remote_identity_proofing %> | ||
| <div> | ||
| <%= "#{t('doc_auth.forms.first_name')}: #{pii[:first_name]}" %> |
There was a problem hiding this comment.
So I know it's copy-pasted from the other page, but IMO this kind of interpolation is very silly #{} for something trivial inside of <%= , if it's not too much effort, I'd consider breaking these out:
| <%= "#{t('doc_auth.forms.first_name')}: #{pii[:first_name]}" %> | |
| <%= t('doc_auth.forms.first_name') %>: <%= pii[:first_name] %> |
There was a problem hiding this comment.
I've also been eyeing these for some time wondering if there's some more semantic HTML we could be using for this field: value association. With the barcode attention page in #6483, I went with a dl for a similar listing (screenshot):
There was a problem hiding this comment.
I'm up to consider these and use one or the other of these suggestions but I'd prefer to do it in a separate PR. I'm trying to keep this PR's changes to existing prod behavior minimal and then follow-up with a couple PRs that make more specific changes to existing prod behavior
| name: :address1, | ||
| form: f, | ||
| input_html: { value: pii[:address1] }, | ||
| label: t('in_person_proofing.form.address.address1'), | ||
| required: true, | ||
| maxlength: 255, | ||
| label_html: { class: 'usa-label' }, | ||
| maxlength: 255, | ||
| name: :address1, | ||
| required: true, |
There was a problem hiding this comment.
The reason there's so much noise in this file is that I sorted these keys alphabetically in addition to making them pre-populate with whatever values the user previously entered
svalexander
left a comment
There was a problem hiding this comment.
just a few questions to help me understand the repo
| end | ||
| end | ||
|
|
||
| def extra_view_variables |
There was a problem hiding this comment.
what are extra view variables?
There was a problem hiding this comment.
The flow state machine is like a mini version of a rails controller, so the way a controller assigns @variables to be exposed in templates, the flow state machine calls this method that returns a hash that is exposed to the templates
|
LGTM! |
I've added new Spanish and French content string keys to LG-6337 to be properly translated.
📆 Future work for separate PRs
There are a few visual differences in this PR compared to the designs. I'd prefer to make these changes in a separate PR since this PR is already changing a lot of files and the below changes will impact the existing doc auth flow:
📺 Demo
You can try this out in the sbachstein sandbox
Doc auth flow
Screen.Recording.2022-06-24.at.3.02.21.PM.mov
In person flow
Screen.Recording.2022-06-24.at.3.00.18.PM.mov