-
Notifications
You must be signed in to change notification settings - Fork 166
LG-361 New LOA1 Cancel Flow #2678
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
14c390a
6044e54
36f2271
85617f5
3f49590
58655c1
ccadc62
14f989d
8ccd009
e3b334d
0de4de6
92eedf5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| module SignUp | ||
| class CancellationsController < ApplicationController | ||
| before_action :ensure_in_setup | ||
|
|
||
| def new | ||
| properties = ParseControllerFromReferer.new(request.referer).call | ||
| analytics.track_event(Analytics::USER_REGISTRATION_CANCELLATION, properties) | ||
| @presenter = CancellationPresenter.new(view_context: view_context) | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def ensure_in_setup | ||
| redirect_to root_url if !session[:user_confirmation_token] && two_factor_enabled | ||
| end | ||
|
|
||
| def two_factor_enabled | ||
| current_user && MfaPolicy.new(current_user).two_factor_enabled? | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,6 @@ | ||
| class UsersController < ApplicationController | ||
| before_action :ensure_in_setup | ||
|
|
||
| def destroy | ||
| track_account_deletion_event | ||
| url_after_cancellation = decorated_session.cancel_link_url | ||
|
|
@@ -19,4 +21,12 @@ def destroy_user | |
| user&.destroy! | ||
| sign_out if user | ||
| end | ||
|
|
||
| def ensure_in_setup | ||
| redirect_to root_url if !session[:user_confirmation_token] && two_factor_enabled | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are we checking for the user confirmation token in the session? What happens if we remove that here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. two_factor_enabled needs to have a user but in the password screen it won't have one.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (password setup)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I finally figured this out and posted some suggestions that may make this easier to follow in Slack. Those are take it or leave it though. |
||
| end | ||
|
|
||
| def two_factor_enabled | ||
| current_user && MfaPolicy.new(current_user).two_factor_enabled? | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| class CancellationPresenter < FailurePresenter | ||
| include ActionView::Helpers::TranslationHelper | ||
| include Rails.application.routes.url_helpers | ||
|
|
||
| delegate :request, to: :view_context | ||
|
|
||
| attr_reader :view_context | ||
|
|
||
| def initialize(view_context:) | ||
| super(:warning) | ||
| @view_context = view_context | ||
| end | ||
|
|
||
| def title | ||
| t('headings.cancellations.prompt') | ||
| end | ||
|
|
||
| def header | ||
| t('headings.cancellations.prompt') | ||
| end | ||
|
|
||
| def cancellation_warnings | ||
| [ | ||
| t('users.delete.bullet_1', app: APP_NAME), | ||
| t('users.delete.bullet_2_loa1'), | ||
| t('users.delete.bullet_3', app: APP_NAME), | ||
| ] | ||
| end | ||
|
|
||
| def go_back_path | ||
| referer_path || two_factor_options_path | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def referer_path | ||
| referer_string = request.env['HTTP_REFERER'] | ||
| return if referer_string.blank? | ||
| referer_uri = URI.parse(referer_string) | ||
| return if referer_uri.scheme == 'javascript' | ||
| return unless referer_uri.host == Figaro.env.domain_name.split(':')[0] | ||
| extract_path_and_query_from_uri(referer_uri) | ||
| end | ||
|
|
||
| def extract_path_and_query_from_uri(uri) | ||
| [uri.path, uri.query].compact.join('?') | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| = render 'shared/failure', presenter: @presenter | ||
|
|
||
| p.mb1.bold = t('sign_up.cancel.warning_header') | ||
|
|
||
| ul class="list-reset #{@presenter.state_color}-dots" | ||
| - @presenter.cancellation_warnings.each do |warning| | ||
| li = warning | ||
|
|
||
| .mt3 = button_to t('forms.buttons.cancel'), destroy_user_path, | ||
| method: :delete, class: 'btn btn-primary sm-col-6 col-12 btn-wide' | ||
| .mt2 = link_to t('links.go_back'), @presenter.go_back_path, | ||
| class: 'btn btn-secondary sm-col-6 col-12 btn-wide' |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| require 'rails_helper' | ||
|
|
||
| describe SignUp::CancellationsController do | ||
| describe '#new' do | ||
| it 'tracks the event in analytics when referer is nil' do | ||
| stub_sign_in | ||
| stub_analytics | ||
| properties = { request_came_from: 'no referer' } | ||
|
|
||
| expect(@analytics).to receive(:track_event).with( | ||
| Analytics::USER_REGISTRATION_CANCELLATION, properties | ||
| ) | ||
|
|
||
| get :new | ||
| end | ||
|
|
||
| it 'tracks the event in analytics when referer is present' do | ||
| stub_sign_in | ||
| stub_analytics | ||
| request.env['HTTP_REFERER'] = 'http://example.com/' | ||
| properties = { request_came_from: 'users/sessions#new' } | ||
|
|
||
| expect(@analytics).to receive(:track_event).with( | ||
| Analytics::USER_REGISTRATION_CANCELLATION, properties | ||
| ) | ||
|
|
||
| get :new | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,7 +26,7 @@ | |
| end | ||
|
|
||
| context 'user cancels on the enter password screen', email: true do | ||
| it 'returns them to the home page' do | ||
| it 'sends them to the cancel page' do | ||
| email = 'test@test.com' | ||
|
|
||
| visit sign_up_email_path | ||
|
|
@@ -36,7 +36,7 @@ | |
|
|
||
| click_on t('links.cancel_account_creation') | ||
|
|
||
| expect(current_path).to eq root_path | ||
| expect(current_path).to eq sign_up_cancel_path | ||
| end | ||
| end | ||
|
|
||
|
|
@@ -64,39 +64,6 @@ | |
| end | ||
|
|
||
| context 'with js', js: true do | ||
| context 'sp loa1' do | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good riddance |
||
| it 'allows the user to toggle the modal' do | ||
| begin_sign_up_with_sp_and_loa(loa3: false) | ||
| expect(page).not_to have_xpath("//div[@id='cancel-action-modal']") | ||
|
|
||
| click_on t('links.cancel') | ||
| expect(page).to have_xpath("//div[@id='cancel-action-modal']") | ||
|
|
||
| click_on t('sign_up.buttons.continue') | ||
| expect(page).not_to have_xpath("//div[@id='cancel-action-modal']") | ||
| end | ||
|
|
||
| it 'allows the user to delete their account and returns them to the branded start page' do | ||
| user = begin_sign_up_with_sp_and_loa(loa3: false) | ||
|
|
||
| click_on t('links.cancel') | ||
| click_on t('sign_up.buttons.cancel') | ||
|
|
||
| expect(page).to have_current_path(sign_up_start_path(request_id: '123')) | ||
| expect { User.find(user.id) }.to raise_error ActiveRecord::RecordNotFound | ||
| end | ||
| end | ||
|
|
||
| context 'sp loa3' do | ||
| it 'behaves like loa1 when user has not finished sign up' do | ||
| begin_sign_up_with_sp_and_loa(loa3: true) | ||
|
|
||
| click_on t('links.cancel') | ||
|
|
||
| expect(page).to have_xpath("//input[@value=\"#{t('sign_up.buttons.cancel')}\"]") | ||
| end | ||
| end | ||
|
|
||
| context 'user enters their email as their password', email: true do | ||
| it 'treats it as a weak password' do | ||
| email = 'test@test.com' | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| require 'rails_helper' | ||
|
|
||
| describe CancellationPresenter do | ||
| let(:good_url) { 'http://example.com/asdf/qwerty' } | ||
| let(:good_url_with_path) { 'http://example.com/asdf?qwerty=123' } | ||
| let(:bad_url) { 'http://evil.com/asdf/qwerty' } | ||
|
|
||
| let(:view_context) { ActionView::Base.new } | ||
|
|
||
| subject { described_class.new(view_context: view_context) } | ||
|
|
||
| describe '#go_back_link' do | ||
| let(:sign_up_path) { '/two_factor_options' } | ||
|
|
||
| before do | ||
| allow(view_context).to receive(:sign_up_path).and_return(sign_up_path) | ||
| request = instance_double(ActionDispatch::Request) | ||
| allow(request).to receive(:env).and_return('HTTP_REFERER' => referer_header) | ||
| allow(view_context).to receive(:request).and_return(request) | ||
| end | ||
|
|
||
| context 'without a referer header' do | ||
| let(:referer_header) { nil } | ||
|
|
||
| it 'returns the sign_up_path' do | ||
| expect(subject.go_back_path).to eq(sign_up_path) | ||
| end | ||
| end | ||
|
|
||
| context 'with a referer header' do | ||
| let(:referer_header) { 'http://www.example.com/asdf/qwerty' } | ||
|
|
||
| it 'returns the path' do | ||
| expect(subject.go_back_path).to eq('/asdf/qwerty') | ||
| end | ||
| end | ||
|
|
||
| context 'with a referer header with query params' do | ||
| let(:referer_header) { 'http://www.example.com/asdf?qwerty=123' } | ||
|
|
||
| it 'returns the path with the query params' do | ||
| expect(subject.go_back_path).to eq('/asdf?qwerty=123') | ||
| end | ||
| end | ||
|
|
||
| context 'with a referer header for a different domain' do | ||
| let(:referer_header) { 'http://www.evil.com/asdf/qwerty' } | ||
|
|
||
| it 'returns the sign_up_path' do | ||
| expect(subject.go_back_path).to eq(sign_up_path) | ||
| end | ||
| end | ||
|
|
||
| context 'with a referer header with a javascript scheme' do | ||
| let(:referer_header) { 'javascript://do-some-evil-stuff' } | ||
|
|
||
| it 'returns the sign_up_path' do | ||
| expect(subject.go_back_path).to eq(sign_up_path) | ||
| end | ||
| end | ||
| end | ||
| end |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated to this change, but worth noting that moving this into the account reset namespace may be a good refactor. Confusing to have this controller out here, makes you think this is the controller for the "delete account" button on the account page.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I'm misreading this. This is actually for cancelling during account setup, ignore the above