diff --git a/app/controllers/users/phone_setup_controller.rb b/app/controllers/users/phone_setup_controller.rb index 449b836048c..d6d5dd5ae73 100644 --- a/app/controllers/users/phone_setup_controller.rb +++ b/app/controllers/users/phone_setup_controller.rb @@ -12,14 +12,14 @@ class PhoneSetupController < ApplicationController def index @new_phone_form = NewPhoneForm.new( - current_user, + user: current_user, setup_voice_preference: setup_voice_preference?, ) track_phone_setup_visit end def create - @new_phone_form = NewPhoneForm.new(current_user) + @new_phone_form = NewPhoneForm.new(user: current_user) result = @new_phone_form.submit(new_phone_form_params) analytics.multi_factor_auth_phone_setup(**result.to_h) diff --git a/app/controllers/users/phones_controller.rb b/app/controllers/users/phones_controller.rb index c166c0fd3cd..d4c63743712 100644 --- a/app/controllers/users/phones_controller.rb +++ b/app/controllers/users/phones_controller.rb @@ -8,11 +8,11 @@ class PhonesController < ReauthnRequiredController def add user_session[:phone_id] = nil - @new_phone_form = NewPhoneForm.new(current_user) + @new_phone_form = NewPhoneForm.new(user: current_user) end def create - @new_phone_form = NewPhoneForm.new(current_user) + @new_phone_form = NewPhoneForm.new(user: current_user) if @new_phone_form.submit(user_params).success? confirm_phone bypass_sign_in current_user diff --git a/app/forms/new_phone_form.rb b/app/forms/new_phone_form.rb index e259e031022..2924a9137ae 100644 --- a/app/forms/new_phone_form.rb +++ b/app/forms/new_phone_form.rb @@ -15,23 +15,26 @@ class NewPhoneForm validate :validate_not_premium_rate validate :validate_allowed_carrier - attr_accessor :phone, :international_code, :otp_delivery_preference, - :otp_make_default_number, :setup_voice_preference + attr_reader :phone, + :international_code, + :otp_delivery_preference, + :otp_make_default_number, + :setup_voice_preference alias_method :setup_voice_preference?, :setup_voice_preference - def initialize(user, setup_voice_preference: false) - self.user = user - self.otp_delivery_preference = user.otp_delivery_preference - self.otp_make_default_number = false - self.setup_voice_preference = setup_voice_preference + def initialize(user:, setup_voice_preference: false) + @user = user + @otp_delivery_preference = user.otp_delivery_preference + @otp_make_default_number = false + @setup_voice_preference = setup_voice_preference end def submit(params) ingest_submitted_params(params) success = valid? - self.phone = submitted_phone unless success + @phone = submitted_phone unless success FormResponse.new(success: success, errors: errors, extra: extra_analytics_attributes) end @@ -64,15 +67,12 @@ def phone_info private - attr_accessor :user, :submitted_phone + attr_reader :user, :submitted_phone def ingest_phone_number(params) - self.international_code = params[:international_code] - self.submitted_phone = params[:phone] - self.phone = PhoneFormatter.format( - submitted_phone, - country_code: international_code, - ) + @international_code = params[:international_code] + @submitted_phone = params[:phone] + @phone = PhoneFormatter.format(submitted_phone, country_code: international_code) end def extra_analytics_attributes @@ -135,8 +135,8 @@ def ingest_submitted_params(params) delivery_prefs = params[:otp_delivery_preference] default_prefs = params[:otp_make_default_number] - self.otp_delivery_preference = delivery_prefs if delivery_prefs - self.otp_make_default_number = true if default_prefs + @otp_delivery_preference = delivery_prefs if delivery_prefs + @otp_make_default_number = true if default_prefs end def confirmed_phone? diff --git a/spec/components/phone_input_component_spec.rb b/spec/components/phone_input_component_spec.rb index 921db8291cf..7b3ec7ef717 100644 --- a/spec/components/phone_input_component_spec.rb +++ b/spec/components/phone_input_component_spec.rb @@ -6,7 +6,7 @@ let(:lookup_context) { ActionView::LookupContext.new(ActionController::Base.view_paths) } let(:view_context) { ActionView::Base.new(lookup_context, {}, controller) } let(:user) { build_stubbed(:user) } - let(:form_object) { NewPhoneForm.new(user) } + let(:form_object) { NewPhoneForm.new(user:) } let(:form_builder) do SimpleForm::FormBuilder.new(form_object.model_name.param_key, form_object, view_context, {}) end diff --git a/spec/controllers/users/phone_setup_controller_spec.rb b/spec/controllers/users/phone_setup_controller_spec.rb index 12385393b60..9e05824a949 100644 --- a/spec/controllers/users/phone_setup_controller_spec.rb +++ b/spec/controllers/users/phone_setup_controller_spec.rb @@ -23,7 +23,7 @@ expect(@analytics).to receive(:track_event). with('User Registration: phone setup visited', enabled_mfa_methods_count: 0) - expect(NewPhoneForm).to receive(:new).with(user, setup_voice_preference: false) + expect(NewPhoneForm).to receive(:new).with(user:, setup_voice_preference: false) get :index diff --git a/spec/forms/new_phone_form_spec.rb b/spec/forms/new_phone_form_spec.rb index 01105092a8c..a9cb258bfad 100644 --- a/spec/forms/new_phone_form_spec.rb +++ b/spec/forms/new_phone_form_spec.rb @@ -11,14 +11,31 @@ otp_delivery_preference: 'sms', } end - subject(:form) { NewPhoneForm.new(user) } + subject(:form) { NewPhoneForm.new(user:) } it_behaves_like 'a phone form' describe 'phone validation' do - it do - should validate_inclusion_of(:international_code). - in_array(PhoneNumberCapabilities::INTERNATIONAL_CODES.keys) + context 'with valid international code' do + it 'is valid' do + PhoneNumberCapabilities::INTERNATIONAL_CODES.keys.each do |code| + result = subject.submit(params.clone.merge(international_code: code)) + + expect(result.to_h[:error_details]).not_to match( + hash_including(international_code: include(:inclusion)), + ) + end + end + end + + context 'with invalid international code' do + it 'is invalid' do + result = subject.submit(params.clone.merge(international_code: 'INVALID')) + + expect(result.to_h[:error_details]).to match( + hash_including(international_code: include(:inclusion)), + ) + end end it 'validates that the number matches the requested international code' do @@ -52,7 +69,7 @@ it 'does not update the user phone attribute' do user = create(:user) - subject = NewPhoneForm.new(user) + subject = NewPhoneForm.new(user:) params[:phone] = '+1 504 444 1643' subject.submit(params) @@ -375,7 +392,7 @@ end context 'with setup_voice_preference present' do - subject(:form) { NewPhoneForm.new(user, setup_voice_preference: true) } + subject(:form) { NewPhoneForm.new(user:, setup_voice_preference: true) } it 'is true' do expect(form.delivery_preference_voice?).to eq(true) diff --git a/spec/views/phone_setup/index.html.erb_spec.rb b/spec/views/phone_setup/index.html.erb_spec.rb index 7f1f223a877..1e7f5d1a7a9 100644 --- a/spec/views/phone_setup/index.html.erb_spec.rb +++ b/spec/views/phone_setup/index.html.erb_spec.rb @@ -6,7 +6,7 @@ allow(view).to receive(:current_user).and_return(user) - @new_phone_form = NewPhoneForm.new(user) + @new_phone_form = NewPhoneForm.new(user:) @presenter = SetupPresenter.new( current_user: user, diff --git a/spec/views/users/phones/add.html.erb_spec.rb b/spec/views/users/phones/add.html.erb_spec.rb index aead0b19307..64ec39a8661 100644 --- a/spec/views/users/phones/add.html.erb_spec.rb +++ b/spec/views/users/phones/add.html.erb_spec.rb @@ -7,7 +7,7 @@ before do user = build_stubbed(:user) - @new_phone_form = NewPhoneForm.new(user) + @new_phone_form = NewPhoneForm.new(user:) end context 'phone vendor outage' do diff --git a/spec/views/users/shared/_otp_delivery_preference_selection.html.erb_spec.rb b/spec/views/users/shared/_otp_delivery_preference_selection.html.erb_spec.rb index 8bc86936219..0180a00c2c7 100644 --- a/spec/views/users/shared/_otp_delivery_preference_selection.html.erb_spec.rb +++ b/spec/views/users/shared/_otp_delivery_preference_selection.html.erb_spec.rb @@ -4,7 +4,7 @@ let(:user) { build_stubbed(:user) } subject(:rendered) do - render 'users/shared/otp_delivery_preference_selection', form_obj: NewPhoneForm.new(user) + render 'users/shared/otp_delivery_preference_selection', form_obj: NewPhoneForm.new(user:) end it 'renders enabled sms option' do