Standardize success parameter to always be first

app/controllers/idv/otp_verification_controller.rb

@@ -20,8 +20,8 @@ def update
       result = phone_confirmation_otp_verification_form.submit(code: params[:code])
       analytics.idv_phone_confirmation_otp_submitted(**result.to_h)
       irs_attempts_api_tracker.idv_phone_otp_submitted(
-        phone_number: idv_session.user_phone_confirmation_session.phone,
         success: result.success?,
+        phone_number: idv_session.user_phone_confirmation_session.phone,
         failure_reason: result.success? ? {} : result.extra.slice(:code_expired, :code_matches),
       )
 

app/controllers/idv/phone_controller.rb

@@ -32,8 +32,8 @@ def create
       result = idv_form.submit(step_params)
       analytics.idv_phone_confirmation_form_submitted(**result.to_h)
       irs_attempts_api_tracker.idv_phone_submitted(
-        phone_number: step_params[:phone],
         success: result.success?,
+        phone_number: step_params[:phone],
         failure_reason: irs_attempts_api_tracker.parse_failure_reason(result),
       )
       flash[:error] = result.first_error_message if !result.success?

app/controllers/users/two_factor_authentication_controller.rb

@@ -225,15 +225,15 @@ def track_events(otp_delivery_preference:)
 
       if UserSessionContext.reauthentication_context?(context)
         irs_attempts_api_tracker.mfa_login_phone_otp_sent(
-          reauthentication: true,
           success: @telephony_result.success?,
+          reauthentication: true,
           phone_number: parsed_phone.e164,
           otp_delivery_method: otp_delivery_preference,
         )
       elsif UserSessionContext.authentication_context?(context)
         irs_attempts_api_tracker.mfa_login_phone_otp_sent(
-          reauthentication: false,
           success: @telephony_result.success?,
+          reauthentication: false,
           phone_number: parsed_phone.e164,
           otp_delivery_method: otp_delivery_preference,
         )

app/services/irs_attempts_api/tracker_events.rb

@@ -215,14 +215,14 @@ def idv_phone_otp_sent_rate_limited
     end
 
     # Tracks when a user submits OTP code sent to their phone
-    # @param [String] phone_number
     # @param [Boolean] success
+    # @param [String] phone_number
     # @param [Hash<Symbol,Array<Symbol>>] failure_reason
-    def idv_phone_otp_submitted(phone_number:, success:, failure_reason: nil)
+    def idv_phone_otp_submitted(success:, phone_number:, failure_reason: nil)
       track_event(
         :idv_phone_otp_submitted,
-        phone_number: phone_number,
         success: success,
+        phone_number: phone_number,
         failure_reason: failure_reason,
       )
     end
@@ -246,14 +246,14 @@ def idv_phone_send_link_rate_limited(phone_number:)
     end
 
     # Tracks when the user submits their idv phone number
-    # @param [String] phone_number
     # @param [Boolean] success
+    # @param [String] phone_number
     # @param [Hash<Symbol,Array<Symbol>>] failure_reason
-    def idv_phone_submitted(phone_number:, success:, failure_reason: nil)
+    def idv_phone_submitted(success:, phone_number:, failure_reason: nil)
       track_event(
         :idv_phone_submitted,
-        phone_number: phone_number,
         success: success,
+        phone_number: phone_number,
         failure_reason: failure_reason,
       )
     end
@@ -460,8 +460,8 @@ def mfa_enroll_phone_otp_submitted(success:)
     end
 
     # Tracks when the user has attempted to enroll the piv cac MFA method to their account
-    # @param [String] subject_dn
     # @param [Boolean] success
+    # @param [String] subject_dn
     # @param [Hash<Symbol,Array<Symbol>>] failure_reason
     def mfa_enroll_piv_cac(
       success:,
@@ -522,16 +522,16 @@ def mfa_login_backup_code(success:)
       )
     end
 
-    # @param [Boolean] reauthentication - True if the user was already logged in
     # @param [Boolean] success - True if the OTP Verification was sent
+    # @param [Boolean] reauthentication - True if the user was already logged in
     # @param [String] phone_number - The user's phone_number used for multi-factor authentication
     # @param [String] otp_delivery_method - Either SMS or Voice
     # During a login attempt, an OTP code has been sent via SMS or Voice.
-    def mfa_login_phone_otp_sent(reauthentication:, success:, phone_number:, otp_delivery_method:)
+    def mfa_login_phone_otp_sent(success:, reauthentication:, phone_number:, otp_delivery_method:)
       track_event(
         :mfa_login_phone_otp_sent,
-        reauthentication: reauthentication,
         success: success,
+        reauthentication: reauthentication,
         phone_number: phone_number,
         otp_delivery_method: otp_delivery_method,
       )

spec/controllers/idv/otp_verification_controller_spec.rb

@@ -21,7 +21,6 @@
     stub_analytics
     stub_attempts_tracker
     allow(@analytics).to receive(:track_event)
-    allow(@irs_attempts_api_tracker).to receive(:track_event)
 
     sign_in(user)
     stub_verify_steps_one_and_two(user)
@@ -103,30 +102,28 @@
     describe 'track irs analytics event' do
       context 'when the phone otp code is valid' do
         it 'captures success event' do
-          put :update, params: { code: phone_confirmation_otp_code }
-
-          expect(@irs_attempts_api_tracker).to have_received(:track_event).with(
-            :idv_phone_otp_submitted,
-            phone_number: phone,
+          expect(@irs_attempts_api_tracker).to receive(:idv_phone_otp_submitted).with(
             success: true,
+            phone_number: phone,
             failure_reason: {},
           )
+
+          put :update, params: { code: phone_confirmation_otp_code }
         end
       end
 
       context 'when the phone otp code is invalid' do
         it 'captures failure event' do
-          put :update, params: { code: '000' }
-
-          expect(@irs_attempts_api_tracker).to have_received(:track_event).with(
-            :idv_phone_otp_submitted,
-            phone_number: phone,
+          expect(@irs_attempts_api_tracker).to receive(:idv_phone_otp_submitted).with(
             success: false,
+            phone_number: phone,
             failure_reason: {
               code_matches: false,
               code_expired: false,
             },
           )
+
+          put :update, params: { code: '000' }
         end
       end
     end

spec/controllers/users/two_factor_authentication_controller_spec.rb

@@ -3,6 +3,8 @@
 describe Users::TwoFactorAuthenticationController do
   include ActionView::Helpers::DateHelper
 
+  let(:otp_preference_sms) { { otp_delivery_preference: 'sms' } }
+
   describe 'before_actions' do
     it 'includes the appropriate before_actions' do
       expect(subject).to have_actions(
@@ -137,7 +139,7 @@ def index
         expect(Telephony::Test::Message.messages.length).to eq(1)
         expect(Telephony::Test::Call.calls.length).to eq(0)
         expect(response).to redirect_to(
-          login_two_factor_path(otp_delivery_preference: 'sms', reauthn: 'true'),
+          login_two_factor_path(**otp_preference_sms, reauthn: 'true'),
         )
       end
     end
@@ -213,7 +215,7 @@ def index
         expect(Telephony::Test::Message.messages.length).to eq(1)
         expect(Telephony::Test::Call.calls.length).to eq(0)
         expect(response).
-          to redirect_to login_two_factor_path(otp_delivery_preference: 'sms', reauthn: false)
+          to redirect_to login_two_factor_path(**otp_preference_sms, reauthn: false)
       end
 
       context 'when no options are enabled and available for use' do
@@ -271,7 +273,12 @@ def index
   end
 
   describe '#send_code' do
+    let(:otp_delivery_form_sms) { { otp_delivery_selection_form: otp_preference_sms } }
     context 'when selecting SMS OTP delivery' do
+      let(:valid_phone_number) { { phone_number: '+12025551212' } }
+      let(:success_parameters) do
+        { success: true, **valid_phone_number, otp_delivery_method: 'sms' }
+      end
       before do
         @user = create(:user, :with_phone)
         sign_in_before_2fa(@user)
@@ -280,7 +287,7 @@ def index
       end
 
       it 'sends OTP via SMS for sign in' do
-        get :send_code, params: { otp_delivery_selection_form: { otp_delivery_preference: 'sms' } }
+        get :send_code, params: otp_delivery_form_sms
 
         expect(Telephony).to have_received(:send_authentication_otp).with(
           otp: subject.current_user.direct_otp,
@@ -293,7 +300,7 @@ def index
         expect(subject.current_user.direct_otp).not_to eq(@old_otp)
         expect(subject.current_user.direct_otp).not_to be_nil
         expect(response).to redirect_to(
-          login_two_factor_path(otp_delivery_preference: 'sms', reauthn: false),
+          login_two_factor_path(**otp_preference_sms, reauthn: false),
         )
       end
 
@@ -303,7 +310,7 @@ def index
         analytics_hash = {
           success: true,
           errors: {},
-          otp_delivery_preference: 'sms',
+          **otp_preference_sms,
           resend: 'true',
           context: 'authentication',
           country_code: 'US',
@@ -317,41 +324,30 @@ def index
         expect(@analytics).to receive(:track_event).
           ordered.
           with('Telephony: OTP sent', hash_including(
-            resend: 'true', success: true, otp_delivery_preference: 'sms',
+            resend: 'true', success: true, **otp_preference_sms,
           ))
 
-        get :send_code, params: { otp_delivery_selection_form:
-                                  { otp_delivery_preference: 'sms', resend: 'true' } }
+        get :send_code, params: {
+          otp_delivery_selection_form: { **otp_preference_sms, resend: 'true' },
+        }
       end
 
       it 'tracks the verification attempt event' do
         stub_attempts_tracker
         expect(@irs_attempts_api_tracker).to receive(:mfa_login_phone_otp_sent).
-          with(
-            phone_number: '+12025551212',
-            reauthentication: false,
-            success: true,
-            otp_delivery_method: 'sms',
-          )
+          with(reauthentication: false, **success_parameters)
 
-        get :send_code, params: { otp_delivery_selection_form:
-          { otp_delivery_preference: 'sms' } }
+        get :send_code, params: otp_delivery_form_sms
       end
 
       it 'tracks the attempt event when user session context is reauthentication' do
         stub_attempts_tracker
         subject.user_session[:context] = 'reauthentication'
 
         expect(@irs_attempts_api_tracker).to receive(:mfa_login_phone_otp_sent).
-          with(
-            phone_number: '+12025551212',
-            reauthentication: true,
-            success: true,
-            otp_delivery_method: 'sms',
-          )
+          with(reauthentication: true, **success_parameters)
 
-        get :send_code, params: { otp_delivery_selection_form:
-          { otp_delivery_preference: 'sms' } }
+        get :send_code, params: otp_delivery_form_sms
       end
 
       it 'calls OtpRateLimiter#exceeded_otp_send_limit? and #increment' do
@@ -364,7 +360,7 @@ def index
         expect(otp_rate_limiter).to receive(:exceeded_otp_send_limit?).twice
         expect(otp_rate_limiter).to receive(:increment)
 
-        get :send_code, params: { otp_delivery_selection_form: { otp_delivery_preference: 'sms' } }
+        get :send_code, params: otp_delivery_form_sms
       end
 
       it 'marks the user as locked out after too many attempts' do
@@ -375,13 +371,13 @@ def index
 
         stub_attempts_tracker
         expect(@irs_attempts_api_tracker).to receive(:mfa_login_phone_otp_sent_rate_limited).
-          with(phone_number: '+12025551212')
+          with(**valid_phone_number)
 
         freeze_time do
           (IdentityConfig.store.otp_delivery_blocklist_maxretry + 1).times do
             get :send_code, params: {
               otp_delivery_selection_form: {
-                otp_delivery_preference: 'sms',
+                **otp_preference_sms,
                 otp_make_default_number: nil,
               },
             }
@@ -400,9 +396,7 @@ def index
           expect(Telephony).to_not receive(:send_authentication_otp)
           expect(Telephony).to_not receive(:send_confirmation_otp)
 
-          get :send_code, params: {
-            otp_delivery_selection_form: { otp_delivery_preference: 'sms' },
-          }
+          get :send_code, params: otp_delivery_form_sms
         end
       end
 
@@ -415,9 +409,7 @@ def index
         end
 
         it 'redirects to the opt in controller' do
-          get :send_code, params: {
-            otp_delivery_selection_form: { otp_delivery_preference: 'sms' },
-          }
+          get :send_code, params: otp_delivery_form_sms
 
           opt_out = PhoneNumberOptOut.create_or_find_with_phone(
             Telephony::Test::ErrorSimulator::OPT_OUT_PHONE_NUMBER,
@@ -523,7 +515,7 @@ def index
 
         allow(Telephony).to receive(:send_confirmation_otp).and_call_original
 
-        get :send_code, params: { otp_delivery_selection_form: { otp_delivery_preference: 'sms' } }
+        get :send_code, params: otp_delivery_form_sms
 
         expect(Telephony).to have_received(:send_confirmation_otp).with(
           otp: subject.current_user.direct_otp,
@@ -544,7 +536,7 @@ def index
         expect(@irs_attempts_api_tracker).to receive(:mfa_enroll_phone_otp_sent).
           with({ phone_number: '+12025551213', success: true, otp_delivery_method: 'sms' })
 
-        get :send_code, params: { otp_delivery_selection_form: { otp_delivery_preference: 'sms' } }
+        get :send_code, params: otp_delivery_form_sms
       end
 
       it 'rate limits confirmation OTPs on sign up' do
@@ -555,8 +547,7 @@ def index
         freeze_time do
           (IdentityConfig.store.phone_confirmation_max_attempts + 1).times do
             subject.user_session[:unconfirmed_phone] = '+1 (202) 555-1213'
-            get :send_code,
-                params: { otp_delivery_selection_form: { otp_delivery_preference: 'sms' } }
+            get :send_code, params: otp_delivery_form_sms
           end
 
           timeout = distance_of_time_in_words(
@@ -591,7 +582,7 @@ def index
           (IdentityConfig.store.otp_delivery_blocklist_maxretry + 1).times do
             get :send_code, params: {
               otp_delivery_selection_form: {
-                otp_delivery_preference: 'sms',
+                **otp_preference_sms,
                 otp_make_default_number: nil,
               },
             }
@@ -609,8 +600,7 @@ def index
         freeze_time do
           (IdentityConfig.store.phone_confirmation_max_attempts + 1).times do
             subject.user_session[:unconfirmed_phone] = '+1 (202) 555-1213'
-            get :send_code,
-                params: { otp_delivery_selection_form: { otp_delivery_preference: 'sms' } }
+            get :send_code, params: otp_delivery_form_sms
           end
 
           timeout = distance_of_time_in_words(
@@ -633,7 +623,7 @@ def index
         subject.user_session[:unconfirmed_phone] = @unconfirmed_phone
         subject.user_session[:unconfirmed_phone] = '+1 (225) 555-1000'
 
-        get :send_code, params: { otp_delivery_selection_form: { otp_delivery_preference: 'sms' } }
+        get :send_code, params: otp_delivery_form_sms
 
         expect(flash[:error]).to eq(I18n.t('telephony.error.friendly_message.generic'))
       end
@@ -649,7 +639,7 @@ def index
           otp_delivery_selection_form: { otp_delivery_preference: 'pigeon' },
         }
 
-        expect(response).to redirect_to login_two_factor_url(otp_delivery_preference: 'sms')
+        expect(response).to redirect_to login_two_factor_url(**otp_preference_sms)
       end
     end
   end