Skip to content

LG-6205: Fix forgot password "Try Again" URL (IdV app)#6374

Merged
aduth merged 2 commits intomainfrom
aduth-lg-6205-forgot-password-subpath
May 19, 2022
Merged

LG-6205: Fix forgot password "Try Again" URL (IdV app)#6374
aduth merged 2 commits intomainfrom
aduth-lg-6205-forgot-password-subpath

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented May 18, 2022

Why: So that a user can refresh the page after returning from "Forgot Password" to "Confirm Password" step.

Testing Instructions:

You will need to set the step as enabled in your local config/application.yml:

development:
  idv_api_enabled_steps: '["password_confirm","personal_key","personal_key_confirm"]'
  1. Navigate to http://localhost:3000
  2. Sign in
  3. Navigate to http://localhost:3000/verify
  4. Complete proofing flow up to password confirmation step
  5. Click "Follow these instructions" (adjacent "Forgot your password?")
  6. Click "Try again"
  7. Reload the page

Before: You see a 404 error
After: You see the same page as before Step 7

**Why**: So that a user can refresh the page after returning from "Forgot Password" to "Confirm Password" step.

changelog: Upcoming Features, Identity Verification, Add password confirmation step
@aduth aduth requested review from a team and nprimak May 18, 2022 18:28
@aduth aduth changed the title LG-6205: Fix forgot password "Try Again" URL LG-6205: Fix forgot password "Try Again" URL (IdV app) May 18, 2022
Comment on lines -47 to +49
return nextValue ? `${prefix}${nextValue}` : window.location.pathname + window.location.search;
return [prefix, nextValue].filter(Boolean).join('');
Copy link
Contributor Author

@aduth aduth May 18, 2022

Choose a reason for hiding this comment

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

I think the idea before was to try to "clean up" the URL if unsetting path and using the hash fragment version of the hook, so that...

setPath('foo');
// /document_capture#foo
setPath(undefined);
// /document_capture

Since it was tailored quite specific to hash fragments, it wasn't working correctly for our path-based flow.

The only consequence to expect for the hash fragment version is it won't be "clean", e.g.

setPath('foo');
// /document_capture#foo
setPath(undefined);
// /document_capture#

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the idea of dropping the # when we can! Is it worth the complexity of trying something like....?

return (prefix === '#') ? nextValue : [prefix, nextValue].filter(Boolean).join('');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The hash fragment feature is only currently used in selfie capture part of document capture, and I expect it will go away entirely (and the feature removed) once we absorb document capture into this flow, e.g. /document_capture#selfie ➡️ /document_capture/selfie. So I don't know that there's much motivation to improve it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense!

So that it doesn't impact other tests (e.g. tests using testing-library's role queries, which rely on getComputedStyle via dom-accessibility-api package)
Copy link
Contributor

@zachmargolis zachmargolis 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 think! Only did a quick pass tbh

Comment on lines 30 to 32
function goBack() {
setPath('password_confirm');
setPath(undefined);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe off topic but is there a way we can leverage the browser history for going back instead of trying to track that part ourselves? Like call navigator.history.popState() or something, and rely on our other hooks for having us update to the right spot/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe off topic but is there a way we can leverage the browser history for going back instead of trying to track that part ourselves? Like call navigator.history.popState() or something, and rely on our other hooks for having us update to the right spot/

A bit off-topic but something I've been thinking about too, which is if and how to support if a user were to reach this page without browser history (e.g. "Open in new tab"). Currently, the "Follow these instructions" link is implemented as a button, so it's not possible to open it in a new tab, but it sorta feels like it should be possible. Doing that means we have to allow for "Try Again" to go back, regardless of browser history. Unfortunately, this also leaves us a question of what to do about the client-side encrypted storage, which uses sessionStorage and won't carry across tabs 😬

But even outside of those considerations, I think the behavior here to push to the browser history is expected, since I think if the user were to hit their browser "Back" button after clicking "Try Again" (goBack), I expect it should take them back to the forgot password screen again.

Password Confirm ▶️ (click "Follow these instructions") ▶️ Forgot Password ▶️ (click "Try again") ▶️ Password Confirm
Password Confirm ▶️ (browser back) ▶️ Forgot Password ▶️ (browser back) ▶️ Password confirm

@aduth aduth merged commit fa9ab96 into main May 19, 2022
@aduth aduth deleted the aduth-lg-6205-forgot-password-subpath branch May 19, 2022 17:10
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.

2 participants