Skip to content

LG-10228: Updated F/T unlock content for error states#8775

Merged
jmdembe merged 21 commits intomainfrom
LG-10228-face-touch-unlock-content
Jul 19, 2023
Merged

LG-10228: Updated F/T unlock content for error states#8775
jmdembe merged 21 commits intomainfrom
LG-10228-face-touch-unlock-content

Conversation

@jmdembe
Copy link
Contributor

@jmdembe jmdembe commented Jul 13, 2023

🎫 Ticket

LG-10228: Updated F/T unlock content for error states

🛠 Summary of changes

This PR improves messaging for when a user does not complete setup for F/T unlock.

📜 Testing Plan

Scenario 1: User sets up a new account and fails setup

  1. Follow the instructions to set up an account
  2. Choose "Face or touch unlock" as your authentication method
  3. Choose a nickname for the device
  4. When prompted, click "Cancel" to make the setup fail
  5. Observe the error banner with "We were unable to add face or touch unlock. Please try again or choose another method" text.

Scenario 2: Failed setup for an account already has face/touch unlock on same device

  1. Follow the instructions to create an account
  2. Set up an account with Face/Touch unlock as the authentication method
  3. Select "Add Face or Touch Unlock"
  4. Follow the instructions to choose a nickname for the device
  5. When prompted, make the setup fail by clicking "Cancel"
  6. Observe the error banner that says "Face or touch unlock is already registered on this device. Please try adding another authentication method.

Scenario 3: User with an existing account fails Face/Touch unlock setup

  1. Follow the instructions to set up an account
  2. Choose an authentication method except Face/Touch unlock
  3. Follow instructions to complete the setup
  4. On the account page, select "Add Face or Touch unlock
  5. Follow instructions to choose a nickname for the device
  6. When prompted, make the setup fail by clicking "Cancel"
  7. Observe the error banner that says ""We were unable to add face or touch unlock. Please try again or choose another method" text. Note that this banner does not have a hyperlink to choose another method.

👀 Screenshots

Before Before Before
Current preview of the screen when a user fails method setup on account creation Current preview of the screen when a user fails method setup with face or touch unlock already setup Current preview of the screen when a user fails method setup on existing account
After After After
Proposed change preview of the screen when a user fails method setup on account Proposed change preview of the screen when a user fails method setup with face or touch unlock already setup Proposed preview of the screen when a user fails method setup on existing account

@jmdembe jmdembe changed the title Lg 10228 face touch unlock content LG-10228: Updated F/T unlock content for error states Jul 14, 2023
@jmdembe jmdembe marked this pull request as ready for review July 17, 2023 14:33
@jmdembe jmdembe requested a review from a team July 17, 2023 14:33
Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Couple minor comments on specs, but otherwise LGTM 👍

'errors.webauthn_platform_setup.account_setup_error',
link: link_to(
I18n.t('errors.webauthn_platform_setup.choose_another_method'),
'/authentication_methods_setup',
Copy link
Contributor

@aduth aduth Jul 17, 2023

Choose a reason for hiding this comment

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

Can we use the URL helper here, so that it doesn't fall out of sync with the route definition?

Suggested change
'/authentication_methods_setup',
authentication_methods_setup_path,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is something that I am finding tricky to work through. I was using the hard-coded to affirm that the test would pass and then make it in sync. I just don't know which avenue to go down at this point.

With both include ActionView::Helpers::UrlHelper and include Rails.application.routes.url_helpers added to the file, I would get an undefined local variable or method 'controller' error.

Then, if I took out both include helpers, I would get an undefined local variable or method 'authentication_methods_setup_path'

Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, very strange, the error message isn't particularly helpful either.

I was able to get a more useful error stack trace by adding a rescue block with a binding.pry, which points back to this being related to url_options.

=> ["/ruby/gems/3.2.0/gems/rspec-expectations-3.12.3/lib/rspec/matchers.rb:968:in `method_missing'",
 "/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib/rspec/core/example_group.rb:767:in `method_missing'",
 "/ruby/gems/3.2.0/gems/actionview-7.0.5.1/lib/action_view/routing_url_for.rb:125:in `url_options'",
 "/ruby/gems/3.2.0/gems/actionpack-7.0.5.1/lib/action_dispatch/routing/route_set.rb:263:in `call'",
 "/ruby/gems/3.2.0/gems/actionpack-7.0.5.1/lib/action_dispatch/routing/route_set.rb:327:in `block in define_url_helper'",
 "/identity-idp/spec/forms/webauthn_visit_form_spec.rb:106:in `block (5 levels) in <top (required)>'",
...

This code: https://github.com/rails/rails/blob/2884d00/actionview/lib/action_view/routing_url_for.rb#L125

Out of a hunch, I added url_options as a let memoized variable to get it "in scope", and... that seemed enough to get it to pass? 🤷

Passing diff:

diff --git a/spec/forms/webauthn_visit_form_spec.rb b/spec/forms/webauthn_visit_form_spec.rb
index a34241728..c2d32412e 100644
--- a/spec/forms/webauthn_visit_form_spec.rb
+++ b/spec/forms/webauthn_visit_form_spec.rb
@@ -4,4 +4,6 @@ RSpec.describe WebauthnVisitForm do
   include ActionView::Helpers::UrlHelper
+  include Rails.application.routes.url_helpers
 
   let(:user) { build(:user) }
+  let(:url_options) { {} }
   let(:subject) do
@@ -9,3 +11,3 @@ RSpec.describe WebauthnVisitForm do
       user: user,
-      url_options: {},
+      url_options:,
       in_mfa_selection_flow: true,
@@ -104,3 +106,3 @@ RSpec.describe WebauthnVisitForm do
               I18n.t('errors.webauthn_platform_setup.choose_another_method'),
-              '/authentication_methods_setup',
+              authentication_methods_setup_path,
             ),

@jmdembe jmdembe merged commit 3491db1 into main Jul 19, 2023
@jmdembe jmdembe deleted the LG-10228-face-touch-unlock-content branch July 19, 2023 13:41
@aduth aduth mentioned this pull request Jul 19, 2023
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