Skip to content

Lg 7265 - Inherited Proofing - Display user data on Please Verify page#7065

Merged
holytoastr merged 5 commits intomainfrom
lg-7265-display-verify-data
Sep 30, 2022
Merged

Lg 7265 - Inherited Proofing - Display user data on Please Verify page#7065
holytoastr merged 5 commits intomainfrom
lg-7265-display-verify-data

Conversation

@holytoastr
Copy link
Contributor

🎫 Ticket

LG 7265

🛠 Summary of changes

User data populates on the Inherited Proofing - Please Verify page

👀 Screenshots

Screen Shot 2022-09-29 at 6 53 43 PM

Screen Shot 2022-09-29 at 6 53 49 PM

Why:
Inherited proofing users need to verify their data that has been imported is correct

changelog: Internal, Inherited Proofing, User data on please verify page
@holytoastr holytoastr self-assigned this Sep 29, 2022
@holytoastr holytoastr added the inherited proofing Pull Requests for the Inherited Proofing feature label Sep 29, 2022
Comment on lines +1 to +6
<%#
locals:
pii - user's information
remote_identity_proofing - true if we're doing RIDP, false if we're doing IPP
step_url - generator for step URLs
%>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be using the existing shared template, rather than duplicating it?

https://github.com/18F/identity-idp/blob/main/app/views/idv/shared/_verify.html.erb

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There were enough differences between the two templates that I felt it would be better to use our own. (For at least the time being.) I would, ideally, like to move us to the existing shared one in the future but for MVP functionality this was the cleanest option.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the differences? It looks like it may only be the titles, right? We could have a inheritted_proofing local or something like that to switch those. I think an even easier approach would be to check with @benjaminchait and @artfulaction if they are okay with using the titles from the existing "Verify your information" screen to keep things consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're not allowing users to edit their addresses or ssn at this time, for example. I could put some conditionals in but it started to look really messy and I didn't want to 💩 up the shared template when I wasn't 100% sure we weren't going to restore editing later.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an inconsistency/old version I didn't catch earlier, but yes, the title should read "Verify your information" instead of "Please verify your information". And yes, the addresses or SSN are not edited at this time, so I defer to y'all on whatever is easiest with the template.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another minor thing is the the user info should be displayed in all caps.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@artfulaction Thanks for the careful eye! I have changed the title and put the user info in all caps. (The .upcase call is a quick fix until we get through MVP and can abstract that out of the view.)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@holytoastr @artfulaction -

put the user info in all caps. (The .upcase call is a quick fix until we get through MVP and can abstract that out of the view.)

This seems fine, but going forward--I'd encourage us not to transform the data we get from the VA.

I believe IDV displays whatever is read from an identity document (eg. your name and address are passed to Login.gov in ALL CAPS) but if the VA passes data in Title Case, I'd encourage us to display it that way. (Since this is already merged, don't worry about changing it for now--but if/when we revisit, I'd encourage us to consider whether we should just display what we received from the wire.)

<dt class="display-inline"> <%= t('idv.form.dob') %>: </dt>
<dd class="display-inline margin-left-0"> <%= pii[:dob] %> </dd>
</div>
<% if !remote_identity_proofing %>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're not going to use the existing template then why would we leave this in? This should never be a remote_identity_proofing context, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I missed that one! Thank you, I thought I had caught them all

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it looks like we set this to true which is even more confusing since we are doing remote proofing.

If we end up re-using the template I'd suggest renaming this var to something like should_display_state_id_number to describe what it actually does. If we don't re-use the template we can def get rid of the conditional here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely a template that got a bit confusing on our end, especially with the things that were taken out for MVP

@holytoastr holytoastr merged commit a2417b7 into main Sep 30, 2022
@holytoastr holytoastr deleted the lg-7265-display-verify-data branch September 30, 2022 16:00
@aduth aduth mentioned this pull request Oct 3, 2022
jskinne3 pushed a commit that referenced this pull request Oct 12, 2022
#7065)

* Display user data on the please verify page (#7265)

Why:
Inherited proofing users need to verify their data that has been imported is correct

changelog: Internal, Inherited Proofing, User data on please verify page

* Clean up specs

* Further clean up

* Remove remote proofing check

* Minor styling fixes - quick
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

inherited proofing Pull Requests for the Inherited Proofing feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants