Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 39 additions & 2 deletions app/controllers/account_reset/cancel_controller.rb
Original file line number Diff line number Diff line change
@@ -1,9 +1,24 @@
module AccountReset
class CancelController < ApplicationController
before_action :check_feature_enabled

def show
return render :show unless token
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If there isn't a token, we want to know about it, and display a helpful message to the user. This keeps it consistent with what AccountReset::Cancel does. Since the token validation now happens in two classes, it probably makes sense to extract it into a validator as Jim suggested, and include it in both classes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I'll need to refactor cancel now to use the validator

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I just copied what DeleteAccountController was doing and it renders a missing token with no message. They will get an error message either way because the post won't work.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, I realized after I made that comment that this is because of the whole remove the token from the URL thing.


result = AccountReset::ValidateCancelToken.new(token).call
track_event(result)

if result.success?
handle_valid_token
else
handle_invalid_token(result)
end
end

def create
result = AccountReset::Cancel.new(params[:token]).call
result = AccountReset::Cancel.new(session[:cancel_token]).call

analytics.track_event(Analytics::ACCOUNT_RESET, result.to_h)
track_event(result)

handle_success if result.success?
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I know this was here before, but I wonder if we can take this opportunity to make the experience better for the user by displaying the flash error message if they happen to end up on the confirm screen without a token? For example, if somehow there is a bug and the link in the email doesn't include the token, they end up on the account_reset/cancel page, and when they click the button to confirm, they get redirected to the sign in page without any kind of message, which could confuse the user.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I believe the message they would get is Invalid token on the post. They do get the confirm screen with no token. The same thing would occur for truncated link/token. I believe this is how delete works now too.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agreed. I'm talking about an edge case where someone ended up visiting the account_reset/cancel path without any token in the params. In that case, no error would show up because of the first line render :show and return unless token. Then, if they submit the form, they will get redirected to the sign in page without an error message. So, instead of this:

def create
  result = AccountReset::Cancel.new(session[:cancel_token]).call
  track_event(result)
  handle_success if result.success?
  redirect_to root_url
end

we could do something like this:

def create
  result = AccountReset::Cancel.new(session[:cancel_token]).call

  track_event(result)
  sign_out if current_user

  if result.success?
    flash[:success] = t('two_factor_authentication.account_reset.successful_cancel')
  else
    flash[:error] = result.errors[:token].first
  end

  redirect_to root_url
end

Since this isn't directly related to this PR, I'll leave it up to you if you want to do it now or in another PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes. I confirmed there is no message on the post after no token. I thought there was. I'll update this.


Expand All @@ -12,9 +27,31 @@ def create

private

def check_feature_enabled
redirect_to root_url unless FeatureManagement.account_reset_enabled?
end

def track_event(result)
analytics.track_event(Analytics::ACCOUNT_RESET, result.to_h)
end

def handle_valid_token
session[:cancel_token] = token
redirect_to url_for
end

def handle_invalid_token(result)
flash[:error] = result.errors[:token].first
redirect_to root_url
end

def handle_success
sign_out if current_user
flash[:success] = t('two_factor_authentication.account_reset.successful_cancel')
end

def token
params[:token]
end
end
end
14 changes: 1 addition & 13 deletions app/services/account_reset/cancel.rb
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
module AccountReset
class Cancel
include ActiveModel::Model

validates :token, presence: { message: I18n.t('errors.account_reset.cancel_token_missing') }
validate :valid_token
include CancelTokenValidator

def initialize(token)
@token = token
Expand All @@ -25,12 +23,6 @@ def call

attr_reader :success, :token

def valid_token
return if account_reset_request

errors.add(:token, I18n.t('errors.account_reset.cancel_token_invalid')) if token
end

def notify_user_via_email_of_account_reset_cancellation
UserMailer.account_reset_cancel(user.email).deliver_later
end
Expand All @@ -45,10 +37,6 @@ def update_account_reset_request
granted_token: nil)
end

def account_reset_request
@account_reset_request ||= AccountResetRequest.find_by(request_token: token)
end

def user
account_reset_request&.user || AnonymousUser.new
end
Expand Down
31 changes: 31 additions & 0 deletions app/services/account_reset/validate_cancel_token.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
module AccountReset
class ValidateCancelToken
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would this make more sense under app/validators/ rather than as a service? Mainly keying off of the Validate part of the class name. But I can also see this as a general service or a form. Especially as a form since it returns a FormResponse.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I'll need to refactor cancel now to use the validator

include ActiveModel::Model
include CancelTokenValidator

def initialize(token)
@token = token
end

def call
@success = valid?

FormResponse.new(success: success, errors: errors.messages, extra: extra_analytics_attributes)
end

private

attr_reader :success, :token

def user
account_reset_request&.user || AnonymousUser.new
end

def extra_analytics_attributes
{
event: 'visit',
user_id: user.uuid,
}
end
end
end
24 changes: 24 additions & 0 deletions app/validators/account_reset/cancel_token_validator.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
module AccountReset
module CancelTokenValidator
extend ActiveSupport::Concern

included do
validates :token, presence: { message: I18n.t('errors.account_reset.cancel_token_missing') }
validate :valid_token
end

private

attr_reader :token

def valid_token
return if account_reset_request

errors.add(:token, I18n.t('errors.account_reset.cancel_token_invalid')) if token
end

def account_reset_request
@account_reset_request ||= AccountResetRequest.find_by(request_token: token)
end
end
end
13 changes: 13 additions & 0 deletions app/views/account_reset/cancel/show.html.slim
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
- title t('account_reset.cancel_request.title')

h1.h3.my0 = t('account_reset.cancel_request.title')
br
h4.my2 = t('account_reset.cancel_request.are_you_sure')

= button_to t('account_reset.cancel_request.cancel_button'), \
account_reset_cancel_path, method: :post, \
class: 'btn btn-primary btn-wide'
br
br
hr
= link_to t('account_reset.cancel_request.cancel'), root_url
5 changes: 5 additions & 0 deletions config/locales/account_reset/en.yml
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
---
en:
account_reset:
cancel_request:
are_you_sure: Are you sure you want to cancel your delete account request?
cancel: Exit
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is Exit the right word to use? We don't use that word anywhere else in our app. Perhaps Cancel or Back to sign in page?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Eric had an issue with Cancel since the main purpose of the screen is to cancel something. Back to sign in page doesn't make sense either because they landed on the screen directly from a link in their email.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we need a link at all? What is the user exiting? There isn't anything in progress that they would need to cancel, right?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I suppose these are questions for the design/product team. I don't have any strong feelings.

cancel_button: Cancel delete account
title: Cancel delete account
confirm_delete_account:
cta: You may %{link} or close this window if you're done.
info: The account for <strong>%{email}</strong> has been deleted. We sent an
Expand Down
5 changes: 5 additions & 0 deletions config/locales/account_reset/es.yml
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
---
es:
account_reset:
cancel_request:
are_you_sure: "¿Seguro que quieres cancelar tu solicitud de eliminación de cuenta?"
cancel: Salida
cancel_button: Cancelar la cuenta eliminada
title: Cancelar la cuenta eliminada
confirm_delete_account:
cta: Puede %{link} o cierra esta ventana si ya terminaste.
info: La cuenta para <strong>%{email}</strong> ha sido eliminada. Nosotros enviamos
Expand Down
6 changes: 6 additions & 0 deletions config/locales/account_reset/fr.yml
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
---
fr:
account_reset:
cancel_request:
are_you_sure: Êtes-vous sûr de vouloir annuler votre demande de suppression
de compte?
cancel: Sortie
cancel_button: Annuler supprimer un compte
title: Annuler supprimer un compte
confirm_delete_account:
cta: Vous pouvez %{link} ou fermer cette fenêtre si vous avez terminé.
info: Le compte pour <strong>%{email}</strong> a été supprimé. Nous avons envoyé
Expand Down
3 changes: 2 additions & 1 deletion config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,8 @@

get '/account_reset/request' => 'account_reset/request#show'
post '/account_reset/request' => 'account_reset/request#create'
get '/account_reset/cancel' => 'account_reset/cancel#create'
get '/account_reset/cancel' => 'account_reset/cancel#show'
post '/account_reset/cancel' => 'account_reset/cancel#create'
get '/account_reset/confirm_request' => 'account_reset/confirm_request#show'
get '/account_reset/delete_account' => 'account_reset/delete_account#show'
delete '/account_reset/delete_account' => 'account_reset/delete_account#delete'
Expand Down
45 changes: 41 additions & 4 deletions spec/controllers/account_reset/cancel_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
describe '#create' do
it 'logs a good token to the analytics' do
token = create_account_reset_request_for(user)
session[:cancel_token] = token

stub_analytics
analytics_hash = {
Expand All @@ -23,7 +24,7 @@
expect(@analytics).to receive(:track_event).
with(Analytics::ACCOUNT_RESET, analytics_hash)

post :create, params: { token: token }
post :create
end

it 'logs a bad token to the analytics' do
Expand All @@ -37,8 +38,9 @@

expect(@analytics).to receive(:track_event).
with(Analytics::ACCOUNT_RESET, analytics_hash)
session[:cancel_token] = 'FOO'

post :create, params: { token: 'FOO' }
post :create
end

it 'logs a missing token to the analytics' do
Expand All @@ -63,8 +65,9 @@

it 'redirects to the root with a flash message when the token is valid' do
token = create_account_reset_request_for(user)
session[:cancel_token] = token

post :create, params: { token: token }
post :create

expect(flash[:success]).
to eq t('two_factor_authentication.account_reset.successful_cancel')
Expand All @@ -75,10 +78,44 @@
stub_sign_in(user)

token = create_account_reset_request_for(user)
session[:cancel_token] = token

expect(controller).to receive(:sign_out)

post :create, params: { token: token }
post :create
end
end

describe '#show' do
it 'redirects to root if the token does not match one in the DB' do
stub_analytics
properties = {
user_id: 'anonymous-uuid',
event: 'visit',
success: false,
errors: { token: [t('errors.account_reset.cancel_token_invalid')] },
}
expect(@analytics).
to receive(:track_event).with(Analytics::ACCOUNT_RESET, properties)

get :show, params: { token: 'FOO' }

expect(response).to redirect_to(root_url)
expect(flash[:error]).to eq t('errors.account_reset.cancel_token_invalid')
end

it 'renders the show view if the token is missing' do
get :show

expect(response).to render_template(:show)
end

it 'redirects to root if feature is not enabled' do
allow(FeatureManagement).to receive(:account_reset_enabled?).and_return(false)

get :show

expect(response).to redirect_to root_url
end
end
end
2 changes: 2 additions & 0 deletions spec/features/account_reset/cancel_request_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
click_button t('account_reset.request.yes_continue')
open_last_email
click_email_link_matching(/cancel\?token/)
click_button t('account_reset.cancel_request.cancel_button')

expect(page).to have_current_path new_user_session_path
expect(page).
Expand All @@ -37,6 +38,7 @@
AccountResetService.grant_tokens_and_send_notifications
open_last_email
click_email_link_matching(/cancel\?token/)
click_button t('account_reset.cancel_request.cancel_button')

expect(page).to have_current_path new_user_session_path
expect(page).
Expand Down
14 changes: 14 additions & 0 deletions spec/views/account_reset/cancel/show.html.slim_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
require 'rails_helper'

describe 'account_reset/cancel/show.html.slim' do
it 'has a localized title' do
expect(view).to receive(:title).with(t('account_reset.cancel_request.title'))

render
end

it 'has button to cancel request' do
render
expect(rendered).to have_button t('account_reset.cancel_request.cancel_button')
end
end