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
44 changes: 22 additions & 22 deletions app/controllers/idv/otp_delivery_method_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,17 @@ class OtpDeliveryMethodController < ApplicationController

before_action :confirm_phone_step_complete
before_action :confirm_step_needed
before_action :set_otp_delivery_method_presenter
before_action :set_otp_delivery_selection_form
before_action :idv_phone # Memoize to use ivar in the view

def new; end
def new
analytics.track_event(Analytics::IDV_PHONE_OTP_DELIVERY_SELECTION_VISIT)
end

def create
result = @otp_delivery_selection_form.submit(otp_delivery_selection_params)
result = otp_delivery_selection_form.submit(otp_delivery_selection_params)
analytics.track_event(Analytics::IDV_PHONE_OTP_DELIVERY_SELECTION_SUBMITTED, result.to_h)
if result.success?
prompt_to_confirm_phone(
phone: @otp_delivery_selection_form.phone,
context: 'idv',
selected_delivery_method: @otp_delivery_selection_form.otp_delivery_preference
)
prompt_to_confirm_idv_phone
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.

Pictured here: Me begging for merge conflicts against #2430

Copy link
Copy Markdown
Contributor

@monfresh monfresh Aug 17, 2018

Choose a reason for hiding this comment

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

Here's a wild idea: what if we got rid of this otp_delivery_selection_form altogether? The only thing it's doing is making sure the method is either "sms" or "voice", and the only way it would not be is if someone deliberately manipulated the HTML on the page before submitting the form. How about if we default to "sms" if something other than "sms" or "voice" is submitted?

Copy link
Copy Markdown
Contributor

@monfresh monfresh Aug 17, 2018

Choose a reason for hiding this comment

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

Or maybe default to not do anything and display the delivery selection page again.

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.

It makes some changes to the idv session in the other PR. The delivery preference gets saved there instead of added as a url param. We could move that logic to the controller, but idk if we want to?

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.

Actually, I'm looking at my new code now and I do actually already set it in the controller. Hmm, yeah we can try removing it completely.

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.

Though another downside of not have a form is that we don't have a result object to feed to analytics here.

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.

@monfresh: Anything to add here?

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.

All good. I think we decided to leave this here when we discussed this offline.

else
render :new
end
Expand All @@ -26,32 +24,34 @@ def create
private

def confirm_phone_step_complete
redirect_to idv_review_url if idv_session.vendor_phone_confirmation != true
redirect_to idv_phone_url if idv_session.vendor_phone_confirmation != true
end

def confirm_step_needed
redirect_to idv_review_url if idv_session.address_verification_mechanism != 'phone' ||
idv_session.user_phone_confirmation == true
end

def otp_delivery_selection_params
params.require(:otp_delivery_selection_form).permit(
:otp_delivery_preference
)
def idv_phone
@idv_phone ||= PhoneFormatter.format(idv_session.params[:phone])
end

def set_otp_delivery_method_presenter
@set_otp_delivery_method_presenter = Idv::OtpDeliveryMethodPresenter.new(
idv_session.params[:phone]
def prompt_to_confirm_idv_phone
prompt_to_confirm_phone(
phone: idv_phone,
context: 'idv',
selected_delivery_method: otp_delivery_selection_form.otp_delivery_preference
)
end

def set_otp_delivery_selection_form
@otp_delivery_selection_form = OtpDeliverySelectionForm.new(
current_user,
idv_session.params[:phone],
'idv'
def otp_delivery_selection_params
params.require(:otp_delivery_selection_form).permit(
:otp_delivery_preference
)
end

def otp_delivery_selection_form
@otp_delivery_selection_form ||= Idv::OtpDeliveryMethodForm.new
end
end
end
24 changes: 24 additions & 0 deletions app/forms/idv/otp_delivery_method_form.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
module Idv
class OtpDeliveryMethodForm
include ActiveModel::Model

attr_reader :otp_delivery_preference

validates :otp_delivery_preference, inclusion: { in: %w[sms voice] }

def submit(params)
self.otp_delivery_preference = params[:otp_delivery_preference]
FormResponse.new(success: valid?, errors: errors.messages, extra: extra_analytics_attributes)
end

private

attr_writer :otp_delivery_preference

def extra_analytics_attributes
{
otp_delivery_preference: otp_delivery_preference,
}
end
end
end
13 changes: 12 additions & 1 deletion app/forms/idv/phone_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ def submit(params)
success = valid?
update_idv_params(formatted_phone) if success

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

private
Expand Down Expand Up @@ -51,5 +51,16 @@ def update_idv_params(phone)
def formatted_user_phone
Phonelib.parse(user.phone).international
end

def parsed_phone
@parsed_phone ||= Phonelib.parse(phone)
end

def extra_analytics_attributes
{
country_code: parsed_phone.country,
area_code: parsed_phone.area_code,
}
end
end
end
24 changes: 0 additions & 24 deletions app/presenters/idv/otp_delivery_method_presenter.rb

This file was deleted.

2 changes: 2 additions & 0 deletions app/services/analytics.rb
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,8 @@ def browser
IDV_JURISDICTION_FORM = 'IdV: jurisdiction form submitted'.freeze
IDV_PHONE_CONFIRMATION_FORM = 'IdV: phone confirmation form'.freeze
IDV_PHONE_CONFIRMATION_VENDOR = 'IdV: phone confirmation vendor'.freeze
IDV_PHONE_OTP_DELIVERY_SELECTION_SUBMITTED = 'IdV: Phone OTP Delivery Selection Submitted'.freeze
IDV_PHONE_OTP_DELIVERY_SELECTION_VISIT = 'IdV: Phone OTP delivery Selection Visited'.freeze
IDV_PHONE_RECORD_VISIT = 'IdV: phone of record visited'.freeze
IDV_REVIEW_COMPLETE = 'IdV: review complete'.freeze
IDV_REVIEW_VISIT = 'IdV: review info visited'.freeze
Expand Down
43 changes: 14 additions & 29 deletions app/views/idv/otp_delivery_method/new.html.slim
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
h1.h3.my0 = t('idv.titles.otp_delivery_method')
p.mt1 = t('idv.messages.otp_delivery_method.phone_number_html',
phone: @set_otp_delivery_method_presenter.phone)
= simple_form_for(@otp_delivery_selection_form, url: idv_otp_delivery_method_url,
html: { autocomplete: 'off', method: 'put', role: 'form', class: 'mt3' }) do |f|
phone: @idv_phone)
= form_tag(idv_otp_delivery_method_url, method: 'put', autocomplete: 'off',
role: 'form', class: 'mt3')
fieldset.mb3.p0.border-none
label.btn-border.col-12.mb1
.radio
Expand All @@ -13,30 +13,16 @@ p.mt1 = t('idv.messages.otp_delivery_method.phone_number_html',
= t('devise.two_factor_authentication.otp_delivery_preference.sms')
.regular.gray-dark.fs-10p.mb-tiny
= t('devise.two_factor_authentication.two_factor_choice_options.sms_info')
- if @set_otp_delivery_method_presenter.sms_only?
label.btn-border.col-12.mb0.btn-disabled
.radio
= radio_button_tag 'otp_delivery_selection_form[otp_delivery_preference]',
:voice, false,
disabled: true,
class: :otp_delivery_preference_voice
span.indicator.mt-tiny
span.blue.bold.fs-20p
= t('devise.two_factor_authentication.otp_delivery_preference.voice')
.regular.gray-dark.fs-10p.mb-tiny
= t('devise.two_factor_authentication.two_factor_choice_options.voice_info')
p.mt2.mb0 = @set_otp_delivery_method_presenter.phone_unsupported_message
- else
label.btn-border.col-12.mb0
.radio
= radio_button_tag 'otp_delivery_selection_form[otp_delivery_preference]',
:voice, false,
class: :otp_delivery_preference_voice
span.indicator.mt-tiny
span.blue.bold.fs-20p
= t('devise.two_factor_authentication.otp_delivery_preference.voice')
.regular.gray-dark.fs-10p.mb-tiny
= t('devise.two_factor_authentication.two_factor_choice_options.voice_info')
label.btn-border.col-12.mb0
.radio
= radio_button_tag 'otp_delivery_selection_form[otp_delivery_preference]',
:voice, false,
class: :otp_delivery_preference_voice
span.indicator.mt-tiny
span.blue.bold.fs-20p
= t('devise.two_factor_authentication.otp_delivery_preference.voice')
.regular.gray-dark.fs-10p.mb-tiny
= t('devise.two_factor_authentication.two_factor_choice_options.voice_info')
- if FeatureManagement.enable_usps_verification?
.mt3
= t('idv.form.no_alternate_phone_html',
Expand All @@ -45,8 +31,7 @@ p.mt1 = t('idv.messages.otp_delivery_method.phone_number_html',
= t('instructions.mfa.wrong_number_html',
link: link_to(t('forms.two_factor.try_again'), idv_phone_path))
.mt3
= f.submit t('idv.buttons.send_confirmation_code'),
type: :submit,
= submit_tag t('idv.buttons.send_confirmation_code'),
class: 'sm-col-6 col-12 btn btn-primary'
.mt3.border-top
.mt1
Expand Down
66 changes: 62 additions & 4 deletions spec/controllers/idv/otp_delivery_method_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,9 @@
subject.idv_session.vendor_phone_confirmation = false
end

it 'redirects to the review controller' do
it 'redirects to the phone controller' do
get :new
expect(response).to redirect_to idv_review_path
expect(response).to redirect_to idv_phone_path
end
end

Expand All @@ -51,6 +51,16 @@
expect(response).to render_template :new
end
end

it 'tracks an analytics event' do
stub_analytics
allow(@analytics).to receive(:track_event)

get :new

expect(@analytics).to have_received(:track_event).
with(Analytics::IDV_PHONE_OTP_DELIVERY_SELECTION_VISIT)
end
end

describe '#create' do
Expand Down Expand Up @@ -89,9 +99,9 @@
subject.idv_session.vendor_phone_confirmation = false
end

it 'redirects to the review controller' do
it 'redirects to the phone controller' do
post :create, params: params
expect(response).to redirect_to idv_review_path
expect(response).to redirect_to idv_phone_path
end
end

Expand All @@ -100,6 +110,22 @@
post :create, params: params
expect(response).to redirect_to otp_send_path(params)
end

it 'tracks an analytics event' do
stub_analytics
allow(@analytics).to receive(:track_event)

post :create, params: params

result = {
success: true,
errors: {},
otp_delivery_preference: 'sms',
}

expect(@analytics).to have_received(:track_event).
with(Analytics::IDV_PHONE_OTP_DELIVERY_SELECTION_SUBMITTED, result)
end
end

context 'user has selected voice' do
Expand All @@ -115,6 +141,22 @@
post :create, params: params
expect(response).to redirect_to otp_send_path(params)
end

it 'tracks an analytics event' do
stub_analytics
allow(@analytics).to receive(:track_event)

post :create, params: params

result = {
success: true,
errors: {},
otp_delivery_preference: 'voice',
}

expect(@analytics).to have_received(:track_event).
with(Analytics::IDV_PHONE_OTP_DELIVERY_SELECTION_SUBMITTED, result)
end
end

context 'form is invalid' do
Expand All @@ -130,6 +172,22 @@
post :create, params: params
expect(response).to render_template :new
end

it 'tracks an analytics event' do
stub_analytics
allow(@analytics).to receive(:track_event)

post :create, params: params

result = {
success: false,
errors: { otp_delivery_preference: ['is not included in the list'] },
otp_delivery_preference: '🎷',
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.

love the sax

}

expect(@analytics).to have_received(:track_event).
with(Analytics::IDV_PHONE_OTP_DELIVERY_SELECTION_SUBMITTED, result)
end
end
end
end
17 changes: 12 additions & 5 deletions spec/controllers/idv/phone_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@
end

it 'renders #new' do
put :create, params: { idv_phone_form: { phone: '703', international_code: 'US' } }
put :create, params: { idv_phone_form: { phone: '703' } }

expect(flash[:warning]).to be_nil
expect(subject.idv_session.params).to be_empty
Expand All @@ -87,6 +87,8 @@
errors: {
phone: [t('errors.messages.must_have_us_country_code')],
},
country_code: nil,
area_code: nil,
}

expect(@analytics).to have_received(:track_event).with(
Expand All @@ -106,9 +108,14 @@
user = build(:user, phone: good_phone, phone_confirmed_at: Time.zone.now)
stub_verify_steps_one_and_two(user)

put :create, params: { idv_phone_form: { phone: good_phone, international_code: 'US' } }
put :create, params: { idv_phone_form: { phone: good_phone } }

result = { success: true, errors: {} }
result = {
success: true,
errors: {},
area_code: '703',
country_code: 'US',
}

expect(@analytics).to have_received(:track_event).with(
Analytics::IDV_PHONE_CONFIRMATION_FORM, result
Expand All @@ -120,7 +127,7 @@
user = build(:user, phone: good_phone, phone_confirmed_at: Time.zone.now)
stub_verify_steps_one_and_two(user)

put :create, params: { idv_phone_form: { phone: good_phone, international_code: 'US' } }
put :create, params: { idv_phone_form: { phone: good_phone } }

expect(response).to redirect_to idv_phone_result_path

Expand All @@ -137,7 +144,7 @@
user = build(:user, phone: '+1 (415) 555-0130', phone_confirmed_at: Time.zone.now)
stub_verify_steps_one_and_two(user)

put :create, params: { idv_phone_form: { phone: good_phone, international_code: 'US' } }
put :create, params: { idv_phone_form: { phone: good_phone } }

expect(response).to redirect_to idv_phone_result_path

Expand Down
Loading