Skip to content

Refactor account personal key reset to use verify flow steps#6453

Merged
aduth merged 5 commits intomainfrom
aduth-reset-personal-key-flow
Jun 7, 2022
Merged

Refactor account personal key reset to use verify flow steps#6453
aduth merged 5 commits intomainfrom
aduth-reset-personal-key-flow

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Jun 3, 2022

Why: So that we're not permanently maintaining two separate implementations, aim to reuse the new FormSteps-based implementation. Future work should remove the "shared/personal_key" partial and "personal-key-page-controller" script once the IdV app has shipped and stabilized.

Open questions:

  • Do we need a no-JS fallback for this since it technically falls outside of identity verification?
    • Option 1: Use our new JavascriptRequiredComponent to enforce JS is enabled
    • Option 2: Add a basic no-JS fallback implementation which just shows the static content (Edit: Implemented in 06c31d1)
  • There may be feature specs associated with this screen which now need to enable JavaScript

aduth added 5 commits June 3, 2022 15:06
**Why**: So that we're not permanently maintaining two separate implementations, aim to reuse the new FormSteps-based implementation. Future work should remove the "shared/personal_key" partial and "personal-key-page-controller" script once the IdV app has shipped and stabilized.

changelog: Upcoming Features, Identity Verification, Add personal key step screen
Specify address verification method to satisfy expected non-null behavior
**Why**: In the future, at least we can remove the JavaScript implementation pieces related to this view, and keep only the static markup
@aduth
Copy link
Contributor Author

aduth commented Jun 7, 2022

Do we need a no-JS fallback for this since it technically falls outside of identity verification?

In 06c31d1, I opted to restore the static markup for the page to support no-JS environments. In the future, assuming we want to continue supporting no-JS for this page, we could at least remove all of the related JavaScript supports (personal-key-page-controller.js, relevant partial markup supports for JavaScript).

@aduth aduth requested a review from a team June 7, 2022 13:25
Comment on lines -62 to -64
# rubocop:disable Layout/LineLength
alias complete_idv_steps_before_confirmation_step complete_idv_steps_with_phone_before_confirmation_step
# rubocop:enable Layout/LineLength
Copy link
Contributor

Choose a reason for hiding this comment

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

oh yikes I'm surpised we used alias instead of alias_method ... glad to see this go

steps={[personalKeyStep, personalKeyConfirmStep]}
initialValues={{ personalKey: appRoot.dataset.personalKey }}
promptOnNavigate={false}
onComplete={() => appForm.submit()}
Copy link
Contributor

Choose a reason for hiding this comment

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

silly question, how do the values get in to this form before we submit it? it looks like the form lives outside of appRoot?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a silly question. The server-side controller doesn't actually care about any submitted values 🙃 The personal key is generated when the page is shown, and the form submission is basically just a redirect.

Page show:

def create
user_session[:personal_key] = create_new_code
analytics.profile_personal_key_create
create_user_event(:new_personal_key)
result = send_new_personal_key_notifications
analytics.profile_personal_key_create_notifications(**result.to_h)
flash[:info] = t('account.personal_key.old_key_will_not_work')
redirect_to manage_personal_key_url
end

Page submit:

def update
user_session.delete(:personal_key)
redirect_to next_step
end

Copy link
Contributor

Choose a reason for hiding this comment

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

🤦 thanks, I forgot that checking the personal key was all JS before anyways

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

@aduth aduth merged commit 8361bb7 into main Jun 7, 2022
@aduth aduth deleted the aduth-reset-personal-key-flow branch June 7, 2022 17:32
aduth added a commit that referenced this pull request Sep 21, 2022
**Why**:

- Since the React implementation is being discontinued and slated for removal (LG-7386)
- To avoid feature drift
- To ensure consistency of personal key confirmation feature flag (#6821 / LG-7162)

Effectively reverts #6453

[skip changelog]
aduth added a commit that referenced this pull request Sep 27, 2022
**Why**:

- Since the React implementation is being discontinued and slated for removal (LG-7386)
- To avoid feature drift
- To ensure consistency of personal key confirmation feature flag (#6821 / LG-7162)

Effectively reverts #6453

[skip changelog]
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