Skip to content

LG-11035: Do you have a phone to take photos IdV page#9285

Merged
amirbey merged 14 commits intomainfrom
amirbey/LG-11035-phone-no-phone-page
Oct 2, 2023
Merged

LG-11035: Do you have a phone to take photos IdV page#9285
amirbey merged 14 commits intomainfrom
amirbey/LG-11035-phone-no-phone-page

Conversation

@amirbey
Copy link
Contributor

@amirbey amirbey commented Sep 28, 2023

🎫 Ticket

LG-11035
LG-11078
LG-11079

🛠 Summary of changes

Create route, controller and view of page to ask a user if they have a phone that can take photos

📜 Testing Plan

  • visit localhost:3000/verify/phone_question

👀 Screenshots

Screenshot 2023-09-29 at 4 02 59 PM Screenshot 2023-09-29 at 4 03 11 PM Screenshot 2023-09-29 at 4 03 22 PM

@amirbey amirbey self-assigned this Sep 28, 2023
@amirbey amirbey changed the title Amirbey/lg 11035 phone no phone page LG-11035 Do you have a phone to take photos IdV page Sep 28, 2023
@amirbey amirbey changed the title LG-11035 Do you have a phone to take photos IdV page LG-11035: Do you have a phone to take photos IdV page Sep 28, 2023
@amirbey amirbey marked this pull request as ready for review September 29, 2023 19:32
@amirbey amirbey requested review from a team, charleyf and kellular September 29, 2023 19:32
Copy link
Contributor

@soniaconnolly soniaconnolly left a comment

Choose a reason for hiding this comment

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

LGTM! I tried it out and it renders when I'm in the right place in the flow, and when I put the browser in mobile mode it skipped this view as well as HybridHandoff. Cancel works as expected. This should be good in the 50/50 state since nothing uses the routes yet.

Couple of coding comments.

Copy link
Contributor

@soniaconnolly soniaconnolly Sep 29, 2023

Choose a reason for hiding this comment

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

Would it work to move HybridHandoffController#confirm_hybrid_handoff_needed to IdvStepConcern and share it between the two controllers? I'm uneasy about the pattern of making the same checks, and then redirecting to HybridHandoff to let it redirect again. It's confusing code, which is okay if very temporary, but it also makes it more likely to run up against the limit of 10 redirects before the user gets a "too many redirects" failure.

Copy link
Contributor

Choose a reason for hiding this comment

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

I had to think about why this is here and in HybridHandoffController. I think it's because we delete pii_from_doc from the session after the VerifyInfo step, so we need to make sure they're not past that step when they land 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.

yep ... after verify info accessing pii is ☹️ LG-10603

Copy link
Contributor

@soniaconnolly soniaconnolly Sep 29, 2023

Choose a reason for hiding this comment

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

Could consider adding a spec for the cancel link. Not sure the additional spec would add value or not.

Copy link

@kellular kellular left a comment

Choose a reason for hiding this comment

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

LGTM! I had two comments about adding commas for French and Spanish. (I originally left the comma out of the English text "Yes I have a phone" because I thought it sounded overly formal, but I checked other places we use "Yes..." statements and the comma is included so thanks for adding that in the English text here!).

Copy link

Choose a reason for hiding this comment

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

Nit: Could you add a comma after "Sí"?

Copy link

Choose a reason for hiding this comment

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

Nit: Could you add a comma after "Oui"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, i had asked myself the same question 🤔 but after checking how we translate across the site, not using the comma after yes/no is the pattern we use everywhere else

@amirbey amirbey force-pushed the amirbey/LG-11035-phone-no-phone-page branch from 68ca5b6 to 2716a44 Compare October 2, 2023 17:01
@amirbey amirbey merged commit a5b03ee into main Oct 2, 2023
@amirbey amirbey deleted the amirbey/LG-11035-phone-no-phone-page branch October 2, 2023 17:35
@jmhooper jmhooper mentioned this pull request Oct 3, 2023
@@ -0,0 +1,32 @@
<% title @title %>
Copy link
Contributor

Choose a reason for hiding this comment

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

If the title never changes, could we have just inlined the text to avoid the indirection?

Suggested change
<% title @title %>
<% title t('doc_auth.headings.phone_question') %>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants