Merge branch '328601-properly-track-acceptance-of-invites' into 'master'

Properly track acceptance of invites

See merge request gitlab-org/gitlab!59918

app/controllers/concerns/accepts_pending_invitations.rb

@@ -6,7 +6,14 @@ module AcceptsPendingInvitations
   def accept_pending_invitations
     return unless resource.active_for_authentication?
 
-    clear_stored_location_for_resource if resource.accept_pending_invitations!.any?
+    if resource.accept_pending_invitations!.any?
+      clear_stored_location_for_resource
+      after_pending_invitations_hook
+    end
+  end
+
+  def after_pending_invitations_hook
+    # no-op
   end
 
   def clear_stored_location_for_resource

app/controllers/invites_controller.rb

@@ -3,10 +3,10 @@
 class InvitesController < ApplicationController
   include Gitlab::Utils::StrongMemoize
 
+  prepend_before_action :authenticate_user!, :track_invite_join_click, only: :show
   before_action :member
   before_action :ensure_member_exists
   before_action :invite_details
-  before_action :set_invite_type, only: :show
   skip_before_action :authenticate_user!, only: :decline
 
   helper_method :member?, :current_user_matches_invite?
@@ -16,16 +16,11 @@ class InvitesController < ApplicationController
   feature_category :authentication_and_authorization
 
   def show
-    experiment('members/invite_email', actor: member).track(:opened) if initial_invite_email?
-
     accept if skip_invitation_prompt?
   end
 
   def accept
     if member.accept_invite!(current_user)
-      experiment('members/invite_email', actor: member).track(:accepted) if initial_invite_email?
-      session.delete(:invite_type)
-
       redirect_to invite_details[:path], notice: helpers.invite_accepted_notice(member)
     else
       redirect_back_or_default(options: { alert: _("The invitation could not be accepted.") })
@@ -52,14 +47,6 @@ class InvitesController < ApplicationController
 
   private
 
-  def set_invite_type
-    session[:invite_type] = params[:invite_type] if params[:invite_type].in?([Members::InviteEmailExperiment::INVITE_TYPE])
-  end
-
-  def initial_invite_email?
-    session[:invite_type] == Members::InviteEmailExperiment::INVITE_TYPE
-  end
-
   def skip_invitation_prompt?
     !member? && current_user_matches_invite?
   end
@@ -87,13 +74,17 @@ class InvitesController < ApplicationController
     render_404
   end
 
+  def track_invite_join_click
+    experiment('members/invite_email', actor: member).track(:join_clicked) if member && Members::InviteEmailExperiment.initial_invite_email?(params[:invite_type])
+  end
+
   def authenticate_user!
     return if current_user
 
     store_location_for(:user, invite_landing_url) if member
 
     if user_sign_up?
-      session[:invite_email] = member.invite_email
+      set_session_invite_params
 
       redirect_to new_user_registration_path(invite_email: member.invite_email), notice: _("To accept this invitation, create an account or sign in.")
     else
@@ -101,6 +92,12 @@ class InvitesController < ApplicationController
     end
   end
 
+  def set_session_invite_params
+    session[:invite_email] = member.invite_email
+
+    session[:originating_member_id] = member.id if Members::InviteEmailExperiment.initial_invite_email?(params[:invite_type])
+  end
+
   def sign_in_redirect_params
     member ? { invite_email: member.invite_email } : {}
   end

app/controllers/registrations_controller.rb

@@ -187,6 +187,20 @@ class RegistrationsController < Devise::RegistrationsController
   def set_invite_params
     @invite_email = ActionController::Base.helpers.sanitize(params[:invite_email])
   end
+
+  def after_pending_invitations_hook
+    member_id = session.delete(:originating_member_id)
+
+    return unless member_id
+
+    # if invited multiple times to different projects, only the email clicked will be counted as accepted
+    # for the specific member on a project or group
+    member = resource.members.find_by(id: member_id) # rubocop: disable CodeReuse/ActiveRecord
+
+    return unless member
+
+    experiment('members/invite_email', actor: member).track(:accepted)
+  end
 end
 
 RegistrationsController.prepend_if_ee('EE::RegistrationsController')

app/experiments/members/invite_email_experiment.rb

@@ -7,6 +7,10 @@ module Members
 
     INVITE_TYPE = 'initial_email'
 
+    def self.initial_invite_email?(invite_type)
+      invite_type == INVITE_TYPE
+    end
+
     def resolve_variant_name
       RoundRobin.new(feature_flag_name, %i[avatar permission_info control]).execute
     end

spec/controllers/invites_controller_spec.rb

@@ -8,11 +8,12 @@ RSpec.describe InvitesController do
   let(:raw_invite_token) { member.raw_invite_token }
   let(:project_members) { member.source.users }
   let(:md5_member_global_id) { Digest::MD5.hexdigest(member.to_global_id.to_s) }
-  let(:params) { { id: raw_invite_token } }
+  let(:extra_params) { {} }
+  let(:params) { { id: raw_invite_token }.merge(extra_params) }
 
   shared_examples 'invalid token' do
     context 'when invite token is not valid' do
-      let(:params) { { id: '_bogus_token_' } }
+      let(:raw_invite_token) { '_bogus_token_' }
 
       it 'renders the 404 page' do
         request
@@ -25,6 +26,37 @@ RSpec.describe InvitesController do
   describe 'GET #show' do
     subject(:request) { get :show, params: params }
 
+    context 'when it is part of our invite email experiment' do
+      let(:extra_params) { { invite_type: 'initial_email' } }
+
+      it 'tracks the experiment' do
+        experiment = double(track: true)
+        allow(controller).to receive(:experiment).with('members/invite_email', actor: member).and_return(experiment)
+
+        request
+
+        expect(experiment).to have_received(:track).with(:join_clicked)
+      end
+
+      context 'when member does not exist' do
+        let(:raw_invite_token) { '_bogus_token_' }
+
+        it 'does not track the experiment' do
+          expect(controller).not_to receive(:experiment).with('members/invite_email', actor: member)
+
+          request
+        end
+      end
+    end
+
+    context 'when it is not part of our invite email experiment' do
+      it 'does not track via experiment' do
+        expect(controller).not_to receive(:experiment).with('members/invite_email', actor: member)
+
+        request
+      end
+    end
+
     context 'when logged in' do
       before do
         sign_in(user)
@@ -51,32 +83,10 @@ RSpec.describe InvitesController do
       end
 
       it_behaves_like 'invalid token'
-
-      context 'when invite comes from the initial email invite' do
-        let(:params) { { id: raw_invite_token, invite_type: Members::InviteEmailExperiment::INVITE_TYPE } }
-
-        it 'tracks via experiment', :aggregate_failures do
-          experiment = double(track: true)
-          allow(controller).to receive(:experiment).and_return(experiment)
-
-          request
-
-          expect(experiment).to have_received(:track).with(:opened)
-          expect(experiment).to have_received(:track).with(:accepted)
-        end
-      end
-
-      context 'when invite does not come from initial email invite' do
-        it 'does not track via experiment' do
-          expect(controller).not_to receive(:experiment)
-
-          request
-        end
-      end
     end
 
     context 'when not logged in' do
-      context 'when inviter is a member' do
+      context 'when invite token belongs to a valid member' do
         context 'when instance allows sign up' do
           it 'indicates an account can be created in notice' do
             request
@@ -121,6 +131,30 @@ RSpec.describe InvitesController do
 
               expect(response).to redirect_to(new_user_registration_path(invite_email: member.invite_email))
             end
+
+            it 'sets session keys for auto email confirmation on sign up' do
+              request
+
+              expect(session[:invite_email]).to eq(member.invite_email)
+            end
+
+            context 'when it is part of our invite email experiment' do
+              let(:extra_params) { { invite_type: 'initial_email' } }
+
+              it 'sets session key for invite acceptance tracking on sign-up' do
+                request
+
+                expect(session[:originating_member_id]).to eq(member.id)
+              end
+            end
+
+            context 'when it is not part of our invite email experiment' do
+              it 'does not set the session key for invite acceptance tracking on sign-up' do
+                request
+
+                expect(session[:originating_member_id]).to be_nil
+              end
+            end
           end
         end
 
@@ -157,7 +191,7 @@ RSpec.describe InvitesController do
         end
       end
 
-      context 'when inviter is not a member' do
+      context 'when invite token does not belong to a valid member' do
         let(:params) { { id: '_bogus_token_' } }
 
         it 'is redirected to a new session' do
@@ -177,25 +211,6 @@ RSpec.describe InvitesController do
     subject(:request) { post :accept, params: params }
 
     it_behaves_like 'invalid token'
-
-    context 'when invite comes from the initial email invite' do
-      it 'tracks via experiment' do
-        experiment = double(track: true)
-        allow(controller).to receive(:experiment).and_return(experiment)
-
-        post :accept, params: params, session: { invite_type: Members::InviteEmailExperiment::INVITE_TYPE }
-
-        expect(experiment).to have_received(:track).with(:accepted)
-      end
-    end
-
-    context 'when invite does not come from initial email invite' do
-      it 'does not track via experiment' do
-        expect(controller).not_to receive(:experiment)
-
-        request
-      end
-    end
   end
 
   describe 'POST #decline for link in UI' do

spec/controllers/registrations_controller_spec.rb

@@ -20,8 +20,12 @@ RSpec.describe RegistrationsController do
   end
 
   describe '#create' do
-    let(:base_user_params) { { first_name: 'first', last_name: 'last', username: 'new_username', email: 'new@user.com', password: 'Any_password' } }
-    let(:user_params) { { user: base_user_params } }
+    let_it_be(:base_user_params) do
+      { first_name: 'first', last_name: 'last', username: 'new_username', email: 'new@user.com', password: 'Any_password' }
+    end
+
+    let_it_be(:user_params) { { user: base_user_params } }
+
     let(:session_params) { {} }
 
     subject { post(:create, params: user_params, session: session_params) }
@@ -151,6 +155,38 @@ RSpec.describe RegistrationsController do
           end
 
           context 'when registration is triggered from an accepted invite' do
+            context 'when it is part of our invite email experiment', :experiment do
+              let_it_be(:member) { create(:project_member, :invited, invite_email: user_params.dig(:user, :email)) }
+
+              let(:originating_member_id) { member.id }
+              let(:session_params) do
+                {
+                  invite_email: user_params.dig(:user, :email),
+                  originating_member_id: originating_member_id
+                }
+              end
+
+              context 'when member exists from the session key value' do
+                it 'tracks the experiment' do
+                  expect(experiment('members/invite_email')).to track(:accepted)
+                                                                  .with_context(actor: member)
+                                                                  .on_next_instance
+
+                  subject
+                end
+              end
+
+              context 'when member does not exist from the session key value' do
+                let(:originating_member_id) { -1 }
+
+                it 'tracks the experiment' do
+                  expect(experiment('members/invite_email')).not_to track(:accepted)
+
+                  subject
+                end
+              end
+            end
+
             context 'when invite email matches email used on registration' do
               let(:session_params) { { invite_email: user_params.dig(:user, :email) } }
 

spec/experiments/members/invite_email_experiment_spec.rb

@@ -11,6 +11,16 @@ RSpec.describe Members::InviteEmailExperiment, :clean_gitlab_redis_shared_state
     allow(invite_email).to receive(:enabled?).and_return(true)
   end
 
+  describe ".initial_invite_email?" do
+    it "is an initial invite email" do
+      expect(described_class.initial_invite_email?('initial_email')).to be(true)
+    end
+
+    it "is not an initial invite email" do
+      expect(described_class.initial_invite_email?('_bogus_')).to be(false)
+    end
+  end
+
   describe "exclusions", :experiment do
     it "excludes when created by is nil" do
       expect(experiment('members/invite_email')).to exclude(actor: double(created_by: nil))

spec/features/invites_spec.rb

@@ -3,10 +3,11 @@
 require 'spec_helper'
 
 RSpec.describe 'Group or Project invitations', :aggregate_failures do
+  let_it_be(:owner) { create(:user, name: 'John Doe') }
+  let_it_be(:group) { create(:group, name: 'Owned') }
+  let_it_be(:project) { create(:project, :repository, namespace: group) }
+
   let(:user) { create(:user, email: 'user@example.com') }
-  let(:owner) { create(:user, name: 'John Doe') }
-  let(:group) { create(:group, name: 'Owned') }
-  let(:project) { create(:project, :repository, namespace: group) }
   let(:group_invite) { group.group_members.invite.last }
 
   before do
@@ -90,11 +91,15 @@ RSpec.describe 'Group or Project invitations', :aggregate_failures do
     let(:new_user) { build_stubbed(:user) }
     let(:invite_email) { new_user.email }
     let(:group_invite) { create(:group_member, :invited, group: group, invite_email: invite_email, created_by: owner) }
+    let(:send_email_confirmation) { true }
+
+    before do
+      stub_application_setting(send_user_confirmation_email: send_email_confirmation)
+    end
 
     context 'when registering using invitation email' do
       before do
-        stub_application_setting(send_user_confirmation_email: send_email_confirmation)
-        visit invite_path(group_invite.raw_invite_token)
+        visit invite_path(group_invite.raw_invite_token, invite_type: Members::InviteEmailExperiment::INVITE_TYPE)
       end
 
       context 'with admin approval required enabled' do
@@ -102,8 +107,6 @@ RSpec.describe 'Group or Project invitations', :aggregate_failures do
           stub_application_setting(require_admin_approval_after_user_signup: true)
         end
 
-        let(:send_email_confirmation) { true }
-
         it 'does not sign the user in' do
           fill_in_sign_up_form(new_user)
 
@@ -136,7 +139,15 @@ RSpec.describe 'Group or Project invitations', :aggregate_failures do
       end
 
       context 'email confirmation enabled' do
-        let(:send_email_confirmation) { true }
+        context 'with members/invite_email experiment', :experiment do
+          it 'tracks the accepted invite' do
+            expect(experiment('members/invite_email')).to track(:accepted)
+                                                            .with_context(actor: group_invite)
+                                                            .on_next_instance
+
+            fill_in_sign_up_form(new_user)
+          end
+        end
 
         context 'when soft email confirmation is not enabled' do
           before do
@@ -201,8 +212,6 @@ RSpec.describe 'Group or Project invitations', :aggregate_failures do
     end
 
     context 'when declining the invitation' do
-      let(:send_email_confirmation) { true }
-
       context 'as an existing user' do
         let(:group_invite) { create(:group_member, user: user, group: group, created_by: owner) }
 
@@ -246,8 +255,6 @@ RSpec.describe 'Group or Project invitations', :aggregate_failures do
     end
 
     context 'when accepting the invitation as an existing user' do
-      let(:send_email_confirmation) { true }
-
       before do
         sign_in(user)
         visit invite_path(group_invite.raw_invite_token)