diff --git a/app/controllers/concerns/unconfirmed_user_concern.rb b/app/controllers/concerns/unconfirmed_user_concern.rb index a027d47f8c4..f9ee2fe3f73 100644 --- a/app/controllers/concerns/unconfirmed_user_concern.rb +++ b/app/controllers/concerns/unconfirmed_user_concern.rb @@ -40,7 +40,7 @@ def email_confirmation_token_validator_result def email_confirmation_token_validator @email_confirmation_token_validator ||= begin - EmailConfirmationTokenValidator.new(@email_address, current_user) + EmailConfirmationTokenValidator.new(email_address: @email_address, current_user:) end end diff --git a/app/controllers/sign_up/cancellations_controller.rb b/app/controllers/sign_up/cancellations_controller.rb index 185f51056cd..b04993147e5 100644 --- a/app/controllers/sign_up/cancellations_controller.rb +++ b/app/controllers/sign_up/cancellations_controller.rb @@ -38,7 +38,7 @@ def find_user confirmation_token = session[:user_confirmation_token] email_address = EmailAddress.find_with_confirmation_token(confirmation_token) - @token_validator = EmailConfirmationTokenValidator.new(email_address, current_user) + @token_validator = EmailConfirmationTokenValidator.new(email_address:, current_user:) result = @token_validator.submit if result.success? diff --git a/app/controllers/users/email_confirmations_controller.rb b/app/controllers/users/email_confirmations_controller.rb index 0f6d468ffa3..e0f0c989fb4 100644 --- a/app/controllers/users/email_confirmations_controller.rb +++ b/app/controllers/users/email_confirmations_controller.rb @@ -3,8 +3,9 @@ module Users class EmailConfirmationsController < ApplicationController def create + store_from_select_email_flow_in_session result = email_confirmation_token_validator.submit - analytics.add_email_confirmation(**result) + analytics.add_email_confirmation(**result, from_select_email_flow: from_select_email_flow?) if result.success? process_successful_confirmation(email_address) else @@ -28,12 +29,8 @@ def email_address end def email_confirmation_token_validator - @email_confirmation_token_validator ||= begin - EmailConfirmationTokenValidator.new( - email_address, - current_user, - ) - end + @email_confirmation_token_validator ||= + EmailConfirmationTokenValidator.new(email_address:, current_user:) end def email_address_already_confirmed? @@ -42,7 +39,6 @@ def email_address_already_confirmed? def process_successful_confirmation(email_address) confirm_and_notify(email_address) - store_from_select_email_flow_in_session if current_user flash[:success] = t('devise.confirmations.confirmed') if params[:request_id] @@ -107,5 +103,9 @@ def confirmation_params def store_from_select_email_flow_in_session session[:from_select_email_flow] = params[:from_select_email_flow].to_s == 'true' end + + def from_select_email_flow? + session[:from_select_email_flow] == true + end end end diff --git a/app/controllers/users/emails_controller.rb b/app/controllers/users/emails_controller.rb index 09e24410d3e..0a229b41e13 100644 --- a/app/controllers/users/emails_controller.rb +++ b/app/controllers/users/emails_controller.rb @@ -11,16 +11,14 @@ class EmailsController < ApplicationController before_action :confirm_recently_authenticated_2fa def show - analytics.add_email_visit - session[:in_select_email_flow] = params[:in_select_email_flow] + session[:in_select_email_flow] = true if params[:in_select_email_flow] + analytics.add_email_visit(in_select_email_flow: in_select_email_flow?) @add_user_email_form = AddUserEmailForm.new @pending_completions_consent = pending_completions_consent? end def add - @add_user_email_form = AddUserEmailForm.new( - session[:in_select_email_flow], - ) + @add_user_email_form = AddUserEmailForm.new(in_select_email_flow: in_select_email_flow?) result = @add_user_email_form.submit( current_user, permitted_params.merge(request_id:) @@ -83,6 +81,10 @@ def verify private + def in_select_email_flow? + session[:in_select_email_flow] == true + end + def authorize_user_to_edit_email return render_not_found if email_address.user != current_user rescue ActiveRecord::RecordNotFound diff --git a/app/forms/add_user_email_form.rb b/app/forms/add_user_email_form.rb index f34ad455ba5..b7fe652b9d8 100644 --- a/app/forms/add_user_email_form.rb +++ b/app/forms/add_user_email_form.rb @@ -6,12 +6,13 @@ class AddUserEmailForm include ActionView::Helpers::TranslationHelper attr_reader :email, :in_select_email_flow + alias_method :in_select_email_flow?, :in_select_email_flow def self.model_name ActiveModel::Name.new(self, nil, 'User') end - def initialize(in_select_email_flow = nil) + def initialize(in_select_email_flow: false) @in_select_email_flow = in_select_email_flow end @@ -52,13 +53,14 @@ def process_successful_submission @success = true email_address.save! SendAddEmailConfirmation.new(user). - call(email_address:, in_select_email_flow:, request_id:) + call(email_address:, in_select_email_flow: in_select_email_flow?, request_id:) end def extra_analytics_attributes { user_id: existing_user.uuid, domain_name: email&.split('@')&.last, + in_select_email_flow: in_select_email_flow?, } end diff --git a/app/forms/select_email_form.rb b/app/forms/select_email_form.rb index c5b9ca3517d..b00bc3ce636 100644 --- a/app/forms/select_email_form.rb +++ b/app/forms/select_email_form.rb @@ -14,16 +14,25 @@ def initialize(user:, identity: nil) end def submit(params) - @selected_email_id = params[:selected_email_id] + @selected_email_id = params[:selected_email_id].try(:to_i) if !params[:selected_email_id].blank? success = valid? identity.update(email_address_id: selected_email_id) if success && identity - FormResponse.new(success:, errors:, serialize_error_details_only: true) + FormResponse.new( + success:, + errors:, + extra: extra_analytics_attributes, + serialize_error_details_only: true, + ) end private + def extra_analytics_attributes + { selected_email_id: } + end + def validate_owns_selected_email return if user.confirmed_email_addresses.exists?(id: selected_email_id) errors.add(:selected_email_id, :not_found, message: t('email_address.not_found')) diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb index 257cb23c201..e617ff89b31 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -243,14 +243,24 @@ def account_visit # @param [Hash] errors Errors resulting from form validation # @param [Hash] error_details Details for errors that occurred in unsuccessful submission # @param [String] user_id User the email is linked to + # @param [Boolean] from_select_email_flow Whether email was added as part of partner email + # selection. # A user has clicked the confirmation link in an email - def add_email_confirmation(user_id:, success:, errors:, error_details: nil, **extra) + def add_email_confirmation( + user_id:, + success:, + errors:, + from_select_email_flow:, + error_details: nil, + **extra + ) track_event( 'Add Email: Email Confirmation', user_id:, success:, errors:, error_details:, + from_select_email_flow:, **extra, ) end @@ -259,21 +269,33 @@ def add_email_confirmation(user_id:, success:, errors:, error_details: nil, **ex # @param [Hash] errors Errors resulting from form validation # @param [Hash] error_details Details for errors that occurred in unsuccessful submission # @param [String] domain_name Domain name of email address submitted + # @param [Boolean] in_select_email_flow Whether email is being added as part of partner email + # selection. # Tracks request for adding new emails to an account - def add_email_request(success:, errors:, domain_name:, error_details: nil, **extra) + def add_email_request( + success:, + errors:, + domain_name:, + in_select_email_flow:, + error_details: nil, + **extra + ) track_event( 'Add Email Requested', success:, errors:, error_details:, domain_name:, + in_select_email_flow:, **extra, ) end # When a user views the add email address page - def add_email_visit - track_event('Add Email Address Page Visited') + # @param [Boolean] in_select_email_flow Whether email is being added as part of partner email + # selection. + def add_email_visit(in_select_email_flow:, **extra) + track_event('Add Email Address Page Visited', in_select_email_flow:, **extra) end # Tracks When users visit the add phone page @@ -6825,10 +6847,12 @@ def sp_revoke_consent_visited(issuer:, **extra) # User submitted form to change email shared with service provider # @param [Boolean] success Whether form validation was successful # @param [Hash] error_details Details for errors that occurred in unsuccessful submission + # @param [Integer] selected_email_id Selected email address record ID # @param [String, nil] needs_completion_screen_reason Reason for the consent screen being shown, # if user is changing email in consent flow def sp_select_email_submitted( success:, + selected_email_id:, error_details: nil, needs_completion_screen_reason: nil, **extra @@ -6838,6 +6862,7 @@ def sp_select_email_submitted( success:, error_details:, needs_completion_screen_reason:, + selected_email_id:, **extra, ) end @@ -7135,10 +7160,10 @@ def user_registration_email_confirmation( ) track_event( 'User Registration: Email Confirmation', - success: success, - errors: errors, - error_details: error_details, - user_id: user_id, + success:, + errors:, + error_details:, + user_id:, **extra, ) end diff --git a/app/services/email_confirmation_token_validator.rb b/app/services/email_confirmation_token_validator.rb index 49baae8350b..8898071bdf4 100644 --- a/app/services/email_confirmation_token_validator.rb +++ b/app/services/email_confirmation_token_validator.rb @@ -9,7 +9,7 @@ class EmailConfirmationTokenValidator validate :email_not_already_confirmed, if: :email_address_found_with_token? validate :token_not_expired, if: :email_address_found_with_token? - def initialize(email_address, current_user = nil) + def initialize(email_address:, current_user: nil) @current_user = current_user @email_address = email_address @user = email_address&.user diff --git a/spec/controllers/accounts/connected_accounts/selected_email_controller_spec.rb b/spec/controllers/accounts/connected_accounts/selected_email_controller_spec.rb index 0b66e62c194..5bf6e380b58 100644 --- a/spec/controllers/accounts/connected_accounts/selected_email_controller_spec.rb +++ b/spec/controllers/accounts/connected_accounts/selected_email_controller_spec.rb @@ -90,11 +90,15 @@ response - expect(@analytics).to have_logged_event(:sp_select_email_submitted, success: true) + expect(@analytics).to have_logged_event( + :sp_select_email_submitted, + success: true, + selected_email_id: selected_email.id, + ) end context 'with invalid submission' do - let(:params) { super().merge(select_email_form: { selected_email_id: nil }) } + let(:params) { super().merge(select_email_form: { selected_email_id: '' }) } it 'redirects to form with flash' do expect(response).to redirect_to(edit_connected_account_selected_email_path(identity.id)) diff --git a/spec/controllers/sign_up/passwords_controller_spec.rb b/spec/controllers/sign_up/passwords_controller_spec.rb index 9d910e9bf57..e0979324944 100644 --- a/spec/controllers/sign_up/passwords_controller_spec.rb +++ b/spec/controllers/sign_up/passwords_controller_spec.rb @@ -171,7 +171,7 @@ end it 'rejects when confirmation_token is invalid' do - validator = EmailConfirmationTokenValidator.new(user.email_addresses.first) + validator = EmailConfirmationTokenValidator.new(email_address: user.email_addresses.first) result = validator.submit expect(result.success?).to eq false diff --git a/spec/controllers/sign_up/select_email_controller_spec.rb b/spec/controllers/sign_up/select_email_controller_spec.rb index 4966281dccc..8c6f65575fb 100644 --- a/spec/controllers/sign_up/select_email_controller_spec.rb +++ b/spec/controllers/sign_up/select_email_controller_spec.rb @@ -107,6 +107,7 @@ :sp_select_email_submitted, success: true, needs_completion_screen_reason: :new_attributes, + selected_email_id: selected_email.id, ) end @@ -131,6 +132,7 @@ success: false, error_details: { selected_email_id: { not_found: true } }, needs_completion_screen_reason: :new_attributes, + selected_email_id: selected_email.id, ) end end diff --git a/spec/controllers/users/email_confirmations_controller_spec.rb b/spec/controllers/users/email_confirmations_controller_spec.rb index 8d3197ad185..826b9374f01 100644 --- a/spec/controllers/users/email_confirmations_controller_spec.rb +++ b/spec/controllers/users/email_confirmations_controller_spec.rb @@ -4,6 +4,8 @@ describe '#create' do describe 'Valid email confirmation tokens' do it 'tracks a valid email confirmation token event' do + stub_analytics + user = create(:user) new_email = Faker::Internet.email @@ -23,6 +25,14 @@ email_record = add_email_form.email_address_record(new_email) get :create, params: { confirmation_token: email_record.reload.confirmation_token } + + expect(@analytics).to have_logged_event( + 'Add Email: Email Confirmation', + success: true, + errors: {}, + from_select_email_flow: false, + user_id: user.uuid, + ) end context 'when select email feature is disabled' do @@ -126,6 +136,7 @@ end it 'adds an email from the service provider consent flow' do + stub_analytics new_email = Faker::Internet.email add_email_form = AddUserEmailForm.new add_email_form.submit(user, email: new_email, request_id: sp_request_uuid) @@ -134,8 +145,16 @@ get :create, params: { confirmation_token: email_record.reload.confirmation_token, request_id: sp_request_uuid, + from_select_email_flow: 'true', } + expect(@analytics).to have_logged_event( + 'Add Email: Email Confirmation', + success: true, + errors: {}, + from_select_email_flow: true, + user_id: user.uuid, + ) expect(response).to redirect_to(sign_up_select_email_url) end end diff --git a/spec/controllers/users/emails_controller_spec.rb b/spec/controllers/users/emails_controller_spec.rb index bfc142718af..ae29f8e71ea 100644 --- a/spec/controllers/users/emails_controller_spec.rb +++ b/spec/controllers/users/emails_controller_spec.rb @@ -1,6 +1,94 @@ require 'rails_helper' RSpec.describe Users::EmailsController do + describe '#show' do + subject(:response) { get :show, params: params } + let(:params) { {} } + + before do + stub_sign_in + end + + it 'does not session value for email selection flow' do + expect { response }.not_to change { controller.session[:in_select_email_flow] }.from(nil) + end + + it 'logs visit' do + stub_analytics + + response + + expect(@analytics).to have_logged_event( + 'Add Email Address Page Visited', + in_select_email_flow: false, + ) + end + + context 'when adding through partner email selection flow' do + let(:params) { { in_select_email_flow: true } } + + it 'assigns session value for email selection flow' do + expect { response }.to change { controller.session[:in_select_email_flow] }. + from(nil).to(true) + end + + it 'logs visit with selected email value' do + stub_analytics + + response + + expect(@analytics).to have_logged_event( + 'Add Email Address Page Visited', + in_select_email_flow: true, + ) + end + end + end + + describe '#add' do + subject(:response) { post :add, params: params } + let(:user) { create(:user) } + let(:params) { { user: { email:, request_id: } } } + let(:email) { 'new@example.com' } + let(:request_id) { 'request-id-1' } + + before do + stub_sign_in(user) + end + + it 'logs submission' do + stub_analytics + + response + + expect(@analytics).to have_logged_event( + 'Add Email Requested', + success: true, + errors: {}, + domain_name: 'example.com', + in_select_email_flow: false, + user_id: user.uuid, + ) + end + + context 'when adding through partner email selection flow' do + before do + controller.session[:in_select_email_flow] = true + end + + it 'logs submission with selected email value' do + stub_analytics + + response + + expect(@analytics).to have_logged_event( + 'Add Email Requested', + hash_including(in_select_email_flow: true), + ) + end + end + end + describe '#verify' do context 'with malformed payload' do it 'does not blow up' do @@ -21,7 +109,10 @@ it 'renders the show view' do get :show - expect(@analytics).to have_logged_event('Add Email Address Page Visited') + expect(@analytics).to have_logged_event( + 'Add Email Address Page Visited', + in_select_email_flow: false, + ) end end @@ -86,6 +177,7 @@ errors: {}, user_id: user.uuid, domain_name: email.split('@').last, + in_select_email_flow: false, ) post :resend diff --git a/spec/forms/add_user_email_form_spec.rb b/spec/forms/add_user_email_form_spec.rb index 18a1b72b0fe..a6d1d45c1b6 100644 --- a/spec/forms/add_user_email_form_spec.rb +++ b/spec/forms/add_user_email_form_spec.rb @@ -8,8 +8,19 @@ describe '#submit' do let(:new_email) { 'new@example.com' } - - subject(:submit) { form.submit(user, email: new_email) } + let(:request_id) { 'request-id-1' } + + subject(:submit) { form.submit(user, email: new_email, request_id:) } + + it 'returns a successful result' do + expect(submit.to_h).to eq( + success: true, + errors: {}, + domain_name: 'example.com', + in_select_email_flow: false, + user_id: user.uuid, + ) + end it 'creates a new EmailAddress record for a new email address' do expect(EmailAddress.find_with_email(new_email)).to be_nil @@ -79,5 +90,31 @@ end end end + + context 'in select email flow' do + subject(:form) { AddUserEmailForm.new(in_select_email_flow: true) } + + it 'sends email confirm with parameter value' do + send_add_email_confirmation = instance_double(SendAddEmailConfirmation) + expect(SendAddEmailConfirmation).to receive(:new).and_return(send_add_email_confirmation) + expect(send_add_email_confirmation).to receive(:call).with( + email_address: kind_of(EmailAddress), + in_select_email_flow: true, + request_id:, + ) + + submit + end + + it 'includes extra analytics in result for flow value' do + expect(submit.to_h).to eq( + success: true, + errors: {}, + domain_name: 'example.com', + in_select_email_flow: true, + user_id: user.uuid, + ) + end + end end end diff --git a/spec/forms/select_email_form_spec.rb b/spec/forms/select_email_form_spec.rb index 3fea0f38232..539fd7ef340 100644 --- a/spec/forms/select_email_form_spec.rb +++ b/spec/forms/select_email_form_spec.rb @@ -14,7 +14,7 @@ let(:selected_email_id) { user.confirmed_email_addresses.take.id } it 'is successful' do - expect(response.to_h).to eq(success: true) + expect(response.to_h).to eq(success: true, selected_email_id:) end context 'with associated identity' do @@ -29,15 +29,28 @@ end context 'with an invalid email id' do - let(:selected_email_id) { nil } + let(:selected_email_id) { '' } it 'is unsuccessful' do expect(response.to_h).to eq( success: false, error_details: { selected_email_id: { not_found: true } }, + selected_email_id: nil, ) end + context 'with present value that does not convert to numeric' do + let(:selected_email_id) { true } + + it 'is unsuccessful without raising exception' do + expect(response.to_h).to eq( + success: false, + error_details: { selected_email_id: { not_found: true } }, + selected_email_id: nil, + ) + end + end + context 'with associated identity' do let(:identity) do create( @@ -55,7 +68,7 @@ end context 'with an unconfirmed email address added' do - let(:selected_email_id) { user.email_addresses.find_by(confirmed_at: nil) } + let(:selected_email_id) { user.email_addresses.find_by(confirmed_at: nil).id } before do create(:email_address, :unconfirmed, user:) @@ -65,6 +78,7 @@ expect(response.to_h).to eq( success: false, error_details: { selected_email_id: { not_found: true } }, + selected_email_id:, ) end @@ -85,6 +99,7 @@ expect(response.to_h).to eq( success: false, error_details: { selected_email_id: { not_found: true } }, + selected_email_id:, ) end diff --git a/spec/services/email_confirmation_token_validator_spec.rb b/spec/services/email_confirmation_token_validator_spec.rb index 8be91518c75..d63e3a2d747 100644 --- a/spec/services/email_confirmation_token_validator_spec.rb +++ b/spec/services/email_confirmation_token_validator_spec.rb @@ -2,7 +2,7 @@ RSpec.describe EmailConfirmationTokenValidator do describe '#submit' do - subject { described_class.new(email_address, current_user) } + subject { described_class.new(email_address:, current_user:) } context 'the email of the user does not match the user confirming' do let(:current_user) { create(:user, :fully_registered) } @@ -88,7 +88,7 @@ end describe '#email_address_already_confirmed_by_user?' do - subject { described_class.new(email_address) } + subject { described_class.new(email_address:) } context 'the email address was confirmed by the user' do let(:email_address) do