Refactor NewPhoneForm class for clarity, consistency, conventions

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)
 

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

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?

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

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
 

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)

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,

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

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