From 341d131756bd165da77204c085c4cbe068294f0a Mon Sep 17 00:00:00 2001 From: Moncef Belyamani Date: Tue, 5 Sep 2017 09:11:39 -0400 Subject: [PATCH] Update personal key UI **Why**: For a better user experience. --- app/assets/stylesheets/components/_btn.scss | 1 - .../users/personal_keys_controller.rb | 15 ++++- app/controllers/verify/sessions_controller.rb | 2 + .../actions/_manage_personal_key.html.slim | 8 +-- app/views/shared/_personal_key.html.slim | 6 +- config/routes.rb | 1 + .../users/personal_keys_controller_spec.rb | 62 ++++++++++++------- ...assword_recovery_via_recovery_code_spec.rb | 26 +------- .../users/regenerate_personal_key_spec.rb | 6 +- .../shared_examples_for_personal_keys.rb | 2 +- spec/views/accounts/show.html.slim_spec.rb | 3 +- .../personal_keys/show.html.slim_spec.rb | 8 +-- 12 files changed, 71 insertions(+), 69 deletions(-) diff --git a/app/assets/stylesheets/components/_btn.scss b/app/assets/stylesheets/components/_btn.scss index f8f473d80fb..c0599bdfdc3 100644 --- a/app/assets/stylesheets/components/_btn.scss +++ b/app/assets/stylesheets/components/_btn.scss @@ -51,7 +51,6 @@ &:active, &:focus, &:hover, { - border: 0; box-shadow: none; text-decoration: underline; } diff --git a/app/controllers/users/personal_keys_controller.rb b/app/controllers/users/personal_keys_controller.rb index 0316b74c412..b116ab2a0a4 100644 --- a/app/controllers/users/personal_keys_controller.rb +++ b/app/controllers/users/personal_keys_controller.rb @@ -5,16 +5,25 @@ class PersonalKeysController < ApplicationController before_action :confirm_two_factor_authenticated def show - @code = create_new_code - analytics.track_event(Analytics::PROFILE_PERSONAL_KEY_CREATE) + personal_key = user_session[:personal_key] + + return redirect_to account_url if personal_key.blank? - flash.now[:success] = t('notices.send_code.personal_key') if params[:resend].present? + @code = personal_key end def update + user_session.delete(:personal_key) redirect_to next_step end + def create + user_session[:personal_key] = create_new_code + analytics.track_event(Analytics::PROFILE_PERSONAL_KEY_CREATE) + flash[:success] = t('notices.send_code.personal_key') if params[:resend].present? + redirect_to manage_personal_key_path + end + private def next_step diff --git a/app/controllers/verify/sessions_controller.rb b/app/controllers/verify/sessions_controller.rb index 51fa5f27cd4..41b1e112b74 100644 --- a/app/controllers/verify/sessions_controller.rb +++ b/app/controllers/verify/sessions_controller.rb @@ -2,6 +2,7 @@ module Verify class SessionsController < ApplicationController include IdvSession include IdvFailureConcern + include PersonalKeyConcern before_action :confirm_two_factor_authenticated, except: [:destroy] before_action :confirm_idv_attempts_allowed @@ -75,6 +76,7 @@ def step def handle_idv_redirect redirect_to account_path and return if current_user.personal_key.present? + user_session[:personal_key] = create_new_code redirect_to manage_personal_key_path end diff --git a/app/views/accounts/actions/_manage_personal_key.html.slim b/app/views/accounts/actions/_manage_personal_key.html.slim index d851c482497..06b0464c5dd 100644 --- a/app/views/accounts/actions/_manage_personal_key.html.slim +++ b/app/views/accounts/actions/_manage_personal_key.html.slim @@ -1,4 +1,4 @@ -= link_to manage_personal_key_path do - span.hide - = t('account.items.personal_key') - = t('account.links.regenerate_personal_key') += button_to(create_new_personal_key_url, method: :post, + class: 'btn btn-link ml1', form_class: 'inline-block') do + span.hide = t('account.items.personal_key') + = t('account.links.regenerate_personal_key') diff --git a/app/views/shared/_personal_key.html.slim b/app/views/shared/_personal_key.html.slim index 3afded3388a..6bc71db332d 100644 --- a/app/views/shared/_personal_key.html.slim +++ b/app/views/shared/_personal_key.html.slim @@ -7,8 +7,10 @@ p.mt-tiny.mb0 = render 'partials/personal_key/key', code: code .mb3.right-align - = link_to t('users.personal_key.get_another'), manage_personal_key_path(resend: true), - class: 'btn-border ico ico-refresh text-decoration-none' + = button_to(t('users.personal_key.get_another'), create_new_personal_key_path(resend: true), + method: :post, + class: 'btn btn-link ml1 btn-border ico ico-refresh text-decoration-none', + form_class: 'inline-block') = link_to t('users.personal_key.print'), '#', data: { print: true }, diff --git a/config/routes.rb b/config/routes.rb index 569ea9c65ec..7897c1af1f3 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -90,6 +90,7 @@ get '/manage/phone' => 'users/phones#edit' match '/manage/phone' => 'users/phones#update', via: %i[patch put] get '/manage/personal_key' => 'users/personal_keys#show', as: :manage_personal_key + post '/account/personal_key' => 'users/personal_keys#create', as: :create_new_personal_key post '/manage/personal_key' => 'users/personal_keys#update' get '/otp/send' => 'users/two_factor_authentication#send_code' diff --git a/spec/controllers/users/personal_keys_controller_spec.rb b/spec/controllers/users/personal_keys_controller_spec.rb index 1df96234cb8..6045947b4fb 100644 --- a/spec/controllers/users/personal_keys_controller_spec.rb +++ b/spec/controllers/users/personal_keys_controller_spec.rb @@ -2,34 +2,13 @@ describe Users::PersonalKeysController do describe '#show' do - context 'when user signed in' do - before do + context 'when user signed in but user_session[:personal_key] is not present' do + it 'redirects to account_url' do stub_sign_in - end - - it 'tracks an analytics event' do - stub_analytics - - expect(@analytics).to receive(:track_event).with(Analytics::PROFILE_PERSONAL_KEY_CREATE) - - get :show - end - - it 'generates a new personal key' do - generator = instance_double(PersonalKeyGenerator) - allow(PersonalKeyGenerator).to receive(:new). - with(subject.current_user).and_return(generator) - - expect(generator).to receive(:create) get :show - end - it 'populates the flash when resending code' do - expect(flash[:sucess]).to be_nil - - get :show, params: { resend: true } - expect(flash.now[:success]).to eq t('notices.send_code.personal_key') + expect(response).to redirect_to(account_url) end end @@ -67,5 +46,40 @@ expect(response).to redirect_to reactivate_account_url end end + + it 'deletes user_session[:personal_key]' do + stub_sign_in + controller.user_session[:personal_key] = 'foo' + + post :update + + expect(controller.user_session[:personal_key]).to be_nil + end + end + + describe '#create' do + it 'generates a new personal key, tracks an analytics event, and redirects' do + stub_sign_in + stub_analytics + + generator = instance_double(PersonalKeyGenerator) + allow(PersonalKeyGenerator).to receive(:new). + with(subject.current_user).and_return(generator) + + expect(generator).to receive(:create) + expect(@analytics).to receive(:track_event).with(Analytics::PROFILE_PERSONAL_KEY_CREATE) + + post :create + + expect(response).to redirect_to manage_personal_key_path + end + + it 'populates the flash when resending code' do + stub_sign_in + expect(flash[:success]).to be_nil + + post :create, params: { resend: true } + expect(flash[:success]).to eq t('notices.send_code.personal_key') + end end end diff --git a/spec/features/users/password_recovery_via_recovery_code_spec.rb b/spec/features/users/password_recovery_via_recovery_code_spec.rb index d220b1975be..8c5028ffdaa 100644 --- a/spec/features/users/password_recovery_via_recovery_code_spec.rb +++ b/spec/features/users/password_recovery_via_recovery_code_spec.rb @@ -39,7 +39,7 @@ expect(page).not_to have_content(t('headings.account.verified_account')) - visit reactivate_account_path + click_link t('account.index.reactivation.link') click_on t('links.account.reactivate.without_key') click_on t('forms.buttons.continue') click_idv_begin @@ -50,30 +50,6 @@ expect(page).to have_content(t('headings.account.verified_account')) end - scenario 'resets password, makes personal key, attempts reactivate profile', email: true do - allow(FeatureManagement).to receive(:prefill_otp_codes?).and_return(true) - - _personal_key = personal_key_from_pii(user, pii) - - trigger_reset_password_and_click_email_link(user.email) - - reset_password_and_sign_back_in(user, new_password) - click_submit_default - - visit manage_personal_key_path - - new_personal_key = scrape_personal_key - click_acknowledge_personal_key - - expect(current_path).to eq reactivate_account_path - - click_on t('links.account.reactivate.with_key') - fill_in 'personal_key', with: new_personal_key - click_on t('forms.buttons.continue') - - expect(page).to have_content t('errors.messages.personal_key_incorrect') - end - scenario 'resets password, uses personal key as 2fa', email: true do personal_key = personal_key_from_pii(user, pii) diff --git a/spec/features/users/regenerate_personal_key_spec.rb b/spec/features/users/regenerate_personal_key_spec.rb index dcd88ffc7e1..0cd01608dd5 100644 --- a/spec/features/users/regenerate_personal_key_spec.rb +++ b/spec/features/users/regenerate_personal_key_spec.rb @@ -20,7 +20,7 @@ user = sign_in_and_2fa_user old_code = user.personal_key - click_link t('account.links.regenerate_personal_key') + click_button t('account.links.regenerate_personal_key') expect(user.reload.personal_key).to_not eq old_code end @@ -45,7 +45,7 @@ context 'personal key actions and information' do before do @user = sign_in_and_2fa_user - click_link t('account.links.regenerate_personal_key') + click_button t('account.links.regenerate_personal_key') end it_behaves_like 'personal key page' @@ -58,7 +58,7 @@ it 'prompts the user to enter their personal key to confirm they have it' do sign_in_and_2fa_user - click_link t('account.links.regenerate_personal_key') + click_button t('account.links.regenerate_personal_key') expect_accordion_content_to_be_hidden_by_default diff --git a/spec/support/shared_examples_for_personal_keys.rb b/spec/support/shared_examples_for_personal_keys.rb index a3f5ef0f11d..3be6aa7c51e 100644 --- a/spec/support/shared_examples_for_personal_keys.rb +++ b/spec/support/shared_examples_for_personal_keys.rb @@ -5,7 +5,7 @@ scenario 'displays a flash message and a new code' do old_code = @user.reload.personal_key - click_link t('users.personal_key.get_another') + click_button t('users.personal_key.get_another') expect(@user.reload.personal_key).to_not eq old_code expect(page).to have_content t('notices.send_code.personal_key') diff --git a/spec/views/accounts/show.html.slim_spec.rb b/spec/views/accounts/show.html.slim_spec.rb index 73851ae246c..2ca904e422e 100644 --- a/spec/views/accounts/show.html.slim_spec.rb +++ b/spec/views/accounts/show.html.slim_spec.rb @@ -65,7 +65,8 @@ expect(rendered).to have_content t('account.items.personal_key') expect(rendered). - to have_link(t('account.links.regenerate_personal_key'), href: manage_personal_key_path) + to have_button t('account.links.regenerate_personal_key') + expect(rendered).to have_xpath("//form[@action='#{create_new_personal_key_url}']") end end diff --git a/spec/views/sign_up/personal_keys/show.html.slim_spec.rb b/spec/views/sign_up/personal_keys/show.html.slim_spec.rb index c7623573e69..0e000395d6e 100644 --- a/spec/views/sign_up/personal_keys/show.html.slim_spec.rb +++ b/spec/views/sign_up/personal_keys/show.html.slim_spec.rb @@ -59,11 +59,9 @@ expect(rendered).to have_content(t('users.personal_key.print')) end - it 'displays a button to refresh the personal key' do + it 'displays a button to get a new personal key' do render - expect(rendered).to have_link( - t('users.personal_key.get_another'), - href: manage_personal_key_path(resend: true) - ) + expect(rendered).to have_xpath("//input[@value='#{t('users.personal_key.get_another')}']") + expect(rendered).to have_xpath("//form[@action='#{sign_up_personal_key_path}']") end end