Skip to content
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

LG-5072: Add Start Over to additional steps in the IAL2 flow #5394

Merged
merged 14 commits into from
Sep 20, 2021

Conversation

aduth
Copy link
Member

@aduth aduth commented Sep 10, 2021

Why: As an end user, I want an option to start over the identity verification process (IAL2), so that I can try to verify my identity again if I do not want to abandon the process completely.

Pending: Improve spec coverage, possible spec fixes needed.

Screenshots:

Step Before After
Agreement localhost_3000_verify_doc_auth_agreement (1) localhost_3000_verify_doc_auth_agreement
Phone localhost_3000_verify_phone (1) localhost_3000_verify_phone
Phone OTP Method localhost_3000_verify_otp_delivery_method (1) localhost_3000_verify_otp_delivery_method
Phone OTP Confirm localhost_3000_verify_phone_confirmation (1) localhost_3000_verify_phone_confirmation
Review localhost_3000_verify_review (1) localhost_3000_verify_review

Copy link
Contributor

@anniehirshman-gsa anniehirshman-gsa left a comment

Choose a reason for hiding this comment

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

Looks good to me - two questions to confirm:

  1. Does/should the "Start over" button appear on the variation of the "Re-enter your password" screen that is in the verify by mail flow as well? I think that's the only page in the mail flow where it might make sense.
  2. Accessibility: Can you double-check for me that Start Over is focusable and appears in the tab order, if you haven't already?

If those things are good, then all good on my end. Thanks!

@aduth
Copy link
Member Author

aduth commented Sep 10, 2021

Thanks for looking @anniehirshman-gsa . As a general answer to both your questions, we're reusing a lot of existing implementation, so it should behave much the same as other screens.

  1. GPO password re-entry reuses the same "review" screen as non-GPO flow, and "Start Over" is shown there as well.

Screen Shot 2021-09-10 at 4 15 54 PM

  1. "Start Over" and "Cancel" links use the same reusable partial (component) we use elsewhere in the flow, and behaves in the same way.
start-over-focusable.mov

**Why**: Because not all "Start Over" occurs is expected to occur within the FlowStateMachine (e.g. GPO verification, soon others).
**Why**: Because sometimes events are logged without any attributes, and it's tedious / redundant to have to explicitly pass an empty hash, especially when the original call under test doesn't actually have to pass an empty hash.
@aduth aduth force-pushed the aduth-lg-5072-start-over branch from 742ea91 to 911aefc Compare September 10, 2021 20:21
@aduth
Copy link
Member Author

aduth commented Sep 13, 2021

"Start Over" and "Cancel" links use the same reusable partial (component) we use elsewhere in the flow, and behaves in the same way.

Separate from this work, one thing which has stood out to me about our unstyled buttons is that the custom focus styles appear differently than they do for a link, which is rather noticeable when otherwise the visual appearance is mostly the same. I wonder if we ought to align them?

You can see in the above video when tabbing from "Start Over" (an unstyled button) to "Cancel" (a link).

start_over_or_cancel uses cancel shared partial, which relies on specific values to determine whether user is signing up
@anniehirshman-gsa
Copy link
Contributor

"Start Over" and "Cancel" links use the same reusable partial (component) we use elsewhere in the flow, and behaves in the same way.

Separate from this work, one thing which has stood out to me about our unstyled buttons is that the custom focus styles appear differently than they do for a link, which is rather noticeable when otherwise the visual appearance is mostly the same. I wonder if we ought to align them?

You can see in the above video when tabbing from "Start Over" (an unstyled button) to "Cancel" (a link).

Yes, I was thinking the same thing! Lmk if there is a good opportunity to fold in that improvement, or if it makes more sense to ticket separately (happy to do that).

@aduth
Copy link
Member Author

aduth commented Sep 13, 2021

"Start Over" and "Cancel" links use the same reusable partial (component) we use elsewhere in the flow, and behaves in the same way.

Separate from this work, one thing which has stood out to me about our unstyled buttons is that the custom focus styles appear differently than they do for a link, which is rather noticeable when otherwise the visual appearance is mostly the same. I wonder if we ought to align them?
You can see in the above video when tabbing from "Start Over" (an unstyled button) to "Cancel" (a link).

Yes, I was thinking the same thing! Lmk if there is a good opportunity to fold in that improvement, or if it makes more sense to ticket separately (happy to do that).

I'd see it as something we could do from the design system. I can follow-up to track it.

@aduth
Copy link
Member Author

aduth commented Sep 15, 2021

Pulling out discussion from https://github.com/18F/identity-idp/pull/5394/files#r708574782, a couple revisions based on further discussion with @amathews-fs :

  1. Allow SessionsController#destroy to log step, location parameters (fe77f7b)
  2. Use step, location parameters as a substitute for separate Analytics::IDV_VERIFICATION_ATTEMPT_CANCELLED event (cf530b4, 979ab95)

Copy link
Contributor

@amathews-fs amathews-fs left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks, @aduth.

@aduth aduth merged commit e8a3798 into main Sep 20, 2021
@aduth aduth deleted the aduth-lg-5072-start-over branch September 20, 2021 12:58
mitchellhenke pushed a commit that referenced this pull request Sep 22, 2021
* Repurpose Idv::SessionsController#destroy for IAL2 restart

**Why**: Because not all "Start Over" occurs is expected to occur within the FlowStateMachine (e.g. GPO verification, soon others).

* Add "Start Over" link to IAL2 steps

* Allow have_logged_event with absent attributes

**Why**: Because sometimes events are logged without any attributes, and it's tedious / redundant to have to explicitly pass an empty hash, especially when the original call under test doesn't actually have to pass an empty hash.

* Add Idv::SessionsController spec

* Mock missing view values for IdV phone

start_over_or_cancel uses cancel shared partial, which relies on specific values to determine whether user is signing up

* Mock missing view values for IdV OTP delivery method

* Support logging location params for SessionsController#destroy

* Log "clear and start over" via step, location params

* Opt-in verify_account view for ERB linting

* Remove unused analytics constant

* Remove unnecessary ERBLint exclusion

It passes as-is

* Remove more unnecessary ERBLint exclusions

All passing

* Add trailing comma to _start_over_or_cancel

ERBLint

* Pass step to start over link

**Why**: It's already there!
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.

3 participants