Merge branch...

Merge branch '326259-experiment-cleanup-members_invite_email-invite-text-and-avatar-variants' into 'master'

Member invite email - implement activity experiment variant permanently

See merge request gitlab-org/gitlab!66560

app/controllers/invites_controller.rb

@@ -75,7 +75,7 @@ def ensure_member_exists
   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])
+    Gitlab::Tracking.event(self.class.name, 'join_clicked', label: 'invite_email', property: member.id.to_s) if member && initial_invite_email?
   end
 
   def authenticate_user!
@@ -95,7 +95,11 @@ def authenticate_user!
   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])
+    session[:originating_member_id] = member.id if initial_invite_email?
+  end
+
+  def initial_invite_email?
+    params[:invite_type] == Emails::Members::INITIAL_INVITE
   end
 
   def sign_in_redirect_params

app/controllers/registrations_controller.rb

@@ -199,7 +199,7 @@ def after_pending_invitations_hook
 
     return unless member
 
-    experiment('members/invite_email', actor: member).track(:accepted)
+    Gitlab::Tracking.event(self.class.name, 'accepted', label: 'invite_email', property: member.id.to_s)
   end
 
   def context_user

app/experiments/members/invite_email_experiment.rb

-# frozen_string_literal: true
-
-module Members
-  class InviteEmailExperiment < ApplicationExperiment
-    exclude { context.actor.created_by.blank? }
-    exclude { context.actor.created_by.avatar_url.nil? }
-
-    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[activity control]).execute
-    end
-  end
-
-  class RoundRobin
-    CacheError = Class.new(StandardError)
-
-    COUNTER_EXPIRE_TIME = 86400 # one day
-
-    def initialize(key, variants)
-      @key = key
-      @variants = variants
-    end
-
-    def execute
-      increment_counter
-      resolve_variant_name
-    end
-
-    # When the counter would expire
-    #
-    # @api private Used internally by SRE and debugging purpose
-    # @return [Integer] Number in seconds until expiration or false if never
-    def counter_expires_in
-      Gitlab::Redis::SharedState.with do |redis|
-        redis.ttl(key)
-      end
-    end
-
-    # Return the actual counter value
-    #
-    # @return [Integer] value
-    def counter_value
-      Gitlab::Redis::SharedState.with do |redis|
-        (redis.get(key) || 0).to_i
-      end
-    end
-
-    # Reset the counter
-    #
-    # @private Used internally by SRE and debugging purpose
-    # @return [Boolean] whether reset was a success
-    def reset!
-      redis_cmd do |redis|
-        redis.del(key)
-      end
-    end
-
-    private
-
-    attr_reader :key, :variants
-
-    # Increase the counter
-    #
-    # @return [Boolean] whether operation was a success
-    def increment_counter
-      redis_cmd do |redis|
-        redis.incr(key)
-        redis.expire(key, COUNTER_EXPIRE_TIME)
-      end
-    end
-
-    def resolve_variant_name
-      remainder = counter_value % variants.size
-
-      variants[remainder]
-    end
-
-    def redis_cmd
-      Gitlab::Redis::SharedState.with { |redis| yield(redis) }
-
-      true
-    rescue CacheError => e
-      Gitlab::AppLogger.warn("GitLab: An unexpected error occurred in writing to Redis: #{e}")
-
-      false
-    end
-  end
-end

app/mailers/emails/members.rb

@@ -6,6 +6,8 @@ module Members
     include MembersHelper
     include Gitlab::Experiment::Dsl
 
+    INITIAL_INVITE = 'initial_email'
+
     included do
       helper_method :member_source, :member
       helper_method :experiment
@@ -53,6 +55,8 @@ def member_invited_email(member_source_type, member_id, token)
 
       return unless member_exists?
 
+      Gitlab::Tracking.event(self.class.name, 'invite_email_sent', label: 'invite_email', property: member_id.to_s)
+
       mail(to: member.invite_email, subject: invite_email_subject, **invite_email_headers) do |format|
         format.html { render layout: 'unknown_user_mailer' }
         format.text { render layout: 'unknown_user_mailer' }

app/views/notify/member_invited_email.html.haml

-- placeholders = { strong_start: '<strong>'.html_safe, strong_end: '</strong>'.html_safe, project_or_group_name: member_source.human_name, project_or_group: member_source.model_name.singular, br_tag: '<br/>'.html_safe, role: member.human_access.downcase }
+- placeholders = { strong_start: '<strong>'.html_safe,
+  strong_end: '</strong>'.html_safe,
+  project_or_group_name: member_source.human_name,
+  project_or_group: member_source.model_name.singular,
+  br_tag: '<br/>'.html_safe,
+  role: member.human_access.downcase }
 
-- experiment('members/invite_email', actor: member) do |experiment_instance|
-  - experiment_instance.use do
-    %tr
-      %td.text-content
-        %h2.invite-header
-          = s_('InviteEmail|You are invited!')
-        %p
-          - if member.created_by
-            = html_escape(s_("InviteEmail|%{inviter} invited you to join the %{strong_start}%{project_or_group_name}%{strong_end}%{br_tag}%{project_or_group} as a %{role}")) % placeholders.merge({ inviter: (link_to member.created_by.name, user_url(member.created_by)).html_safe })
-          - else
-            = html_escape(s_("InviteEmail|You are invited to join the %{strong_start}%{project_or_group_name}%{strong_end}%{br_tag}%{project_or_group} as a %{role}")) % placeholders
-        %p.invite-actions
-          = link_to s_('InviteEmail|Join now'), invite_url(@token, invite_type: Members::InviteEmailExperiment::INVITE_TYPE), class: 'invite-btn-join'
-  - experiment_instance.try(:activity) do
-    %tr
-      %td.text-content{ colspan: 2 }
-        %img.mail-avatar{ height: "60", src: avatar_icon_for_user(member.created_by, 60, only_path: false), width: "60", alt: "" }
-        %p
-          = html_escape(s_("InviteEmail|%{inviter} invited you to join the %{strong_start}%{project_or_group_name}%{strong_end}%{br_tag}%{project_or_group} as a %{role}")) % placeholders.merge({ inviter: (link_to member.created_by.name, user_url(member.created_by)).html_safe })
-        %p.invite-actions
-          = link_to s_('InviteEmail|Join now'), invite_url(@token, invite_type: Members::InviteEmailExperiment::INVITE_TYPE), class: 'invite-btn-join'
-    %tr.border-top
-      %td.text-content.mailer-align-left.half-width
-        %h4
-          = s_('InviteEmail|%{project_or_group} details') % { project_or_group: member_source.model_name.singular.capitalize }
-        %ul
-          %li
-            %div
-              %img.mailer-icon{ alt: '', src: image_url("mailers/members/users.png") }
-              %span
-                - member_count = member_source.members.size
-                = n_('%{bold_start}%{count}%{bold_end} member', '%{bold_start}%{count}%{bold_end} members',
-                  member_count).html_safe % { count: number_with_delimiter(member_count),
-                  bold_start: '<b>'.html_safe,
-                  bold_end: '</b>'.html_safe }
-          %li
-            %div
-              %img.mailer-icon{ alt: '', src: image_url("mailers/members/issues.png") }
-              %span
-                - issue_count = member_source.open_issues_count(member.created_by)
-                = n_('%{bold_start}%{count}%{bold_end} issue', '%{bold_start}%{count}%{bold_end} issues',
-                  issue_count).html_safe % { count: number_with_delimiter(issue_count),
-                  bold_start: '<b>'.html_safe,
-                  bold_end: '</b>'.html_safe }
-          %li
-            %div
-              %img.mailer-icon{ alt: '', src: image_url("mailers/members/merge-request-open.png") }
-              %span
-                - mr_count = member_source.open_merge_requests_count(member.created_by)
-                = n_('%{bold_start}%{count}%{bold_end} opened merge request', '%{bold_start}%{count}%{bold_end} opened merge requests',
-                  mr_count).html_safe % { count: number_with_delimiter(mr_count),
-                  bold_start: '<b>'.html_safe,
-                  bold_end: '</b>'.html_safe }
-      %td.text-content.mailer-align-left.half-width
-        %h4
-          = s_("InviteEmail|What's it about?")
-        %p
-          = invited_to_description(member_source)
+%tr
+  %td.text-content{ colspan: 2 }
+    %img.mail-avatar{ height: "60", src: avatar_icon_for_user(member.created_by, 60, only_path: false), width: "60", alt: "" }
+    %p
+      - if member.created_by
+        = html_escape(s_("InviteEmail|%{inviter} invited you to join the %{strong_start}%{project_or_group_name}%{strong_end}%{br_tag}%{project_or_group} as a %{role}")) % placeholders.merge({ inviter: (link_to member.created_by.name, user_url(member.created_by)).html_safe })
+      - else
+        = html_escape(s_("InviteEmail|You are invited to join the %{strong_start}%{project_or_group_name}%{strong_end}%{br_tag}%{project_or_group} as a %{role}")) % placeholders
+    %p.invite-actions
+      = link_to s_('InviteEmail|Join now'), invite_url(@token, invite_type: Emails::Members::INITIAL_INVITE), class: 'invite-btn-join'
+%tr.border-top
+  %td.text-content.mailer-align-left.half-width
+    %h4
+      = s_('InviteEmail|%{project_or_group} details') % { project_or_group: member_source.model_name.singular.capitalize }
+    %ul
+      %li
+        %div
+          %img.mailer-icon{ alt: '', src: image_url("mailers/members/users.png") }
+          %span
+            - member_count = member_source.members.size
+            = n_('%{bold_start}%{count}%{bold_end} member', '%{bold_start}%{count}%{bold_end} members',
+              member_count).html_safe % { count: number_with_delimiter(member_count),
+              bold_start: '<b>'.html_safe,
+              bold_end: '</b>'.html_safe }
+      %li
+        %div
+          %img.mailer-icon{ alt: '', src: image_url("mailers/members/issues.png") }
+          %span
+            - issue_count = member_source.open_issues_count(member.created_by)
+            = n_('%{bold_start}%{count}%{bold_end} issue', '%{bold_start}%{count}%{bold_end} issues',
+              issue_count).html_safe % { count: number_with_delimiter(issue_count),
+              bold_start: '<b>'.html_safe,
+              bold_end: '</b>'.html_safe }
+      %li
+        %div
+          %img.mailer-icon{ alt: '', src: image_url("mailers/members/merge-request-open.png") }
+          %span
+            - mr_count = member_source.open_merge_requests_count(member.created_by)
+            = n_('%{bold_start}%{count}%{bold_end} opened merge request', '%{bold_start}%{count}%{bold_end} opened merge requests',
+              mr_count).html_safe % { count: number_with_delimiter(mr_count),
+              bold_start: '<b>'.html_safe,
+              bold_end: '</b>'.html_safe }
+  %td.text-content.mailer-align-left.half-width
+    %h4
+      = s_("InviteEmail|What's it about?")
+    %p
+      = invited_to_description(member_source)

config/feature_flags/experiment/members_invite_email.yml

----
-name: members_invite_email
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/51223
-rollout_issue_url: https://gitlab.com/gitlab-org/growth/team-tasks/-/issues/325
-milestone: '13.9'
-type: experiment
-group: group::expansion
-default_enabled: false

locale/gitlab.pot

@@ -17975,9 +17975,6 @@ msgstr ""
 msgid "InviteEmail|You are invited to join the %{strong_start}%{project_or_group_name}%{strong_end}%{br_tag}%{project_or_group} as a %{role}"
 msgstr ""
-msgid "InviteEmail|You are invited!"
-msgstr ""
-
 msgid "InviteEmail|You have been invited to join the %{project_or_group_name} %{project_or_group} as a %{role}"
 msgstr ""

spec/controllers/invites_controller_spec.rb

@@ -25,37 +25,47 @@
     end
   end
 
-  describe 'GET #show' do
+  describe 'GET #show', :snowplow do
     subject(:request) { get :show, params: params }
 
-    context 'when it is part of our invite email experiment' do
+    context 'when it is an initial invite email' 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)
-
+      it 'tracks the initial join click from email' do
         request
 
-        expect(experiment).to have_received(:track).with(:join_clicked)
+        expect_snowplow_event(
+          category: described_class.name,
+          action: 'join_clicked',
+          label: 'invite_email',
+          property: member.id.to_s
+        )
       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)
-
+        it 'does not track join click' do
           request
+
+          expect_no_snowplow_event(
+            category: described_class.name,
+            action: 'join_clicked',
+            label: 'invite_email'
+          )
         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)
-
+    context 'when it is not an initial email' do
+      it 'does not track the join click' do
         request
+
+        expect_no_snowplow_event(
+          category: described_class.name,
+          action: 'join_clicked',
+          label: 'invite_email'
+        )
       end
     end
 

spec/controllers/registrations_controller_spec.rb

@@ -155,7 +155,7 @@
           end
 
           context 'when registration is triggered from an accepted invite' do
-            context 'when it is part of our invite email experiment', :experiment do
+            context 'when it is part from the initial invite email', :snowplow do
               let_it_be(:member) { create(:project_member, :invited, invite_email: user_params.dig(:user, :email)) }
 
               let(:originating_member_id) { member.id }
@@ -167,22 +167,29 @@
               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
-
+                it 'tracks the invite acceptance' do
                   subject
+
+                  expect_snowplow_event(
+                    category: 'RegistrationsController',
+                    action: 'accepted',
+                    label: 'invite_email',
+                    property: member.id.to_s
+                  )
                 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)
-
+                it 'does not track invite acceptance' do
                   subject
+
+                  expect_no_snowplow_event(
+                    category: 'RegistrationsController',
+                    action: 'accepted',
+                    label: 'invite_email'
+                  )
                 end
               end
             end

spec/experiments/members/invite_email_experiment_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe Members::InviteEmailExperiment, :clean_gitlab_redis_shared_state do
-  subject(:invite_email) { experiment('members/invite_email', **context) }
-
-  let(:context) { { actor: double('Member', created_by: double('User', avatar_url: '_avatar_url_')) } }
-
-  before do
-    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))
-    end
-
-    it "excludes when avatar_url is nil" do
-      member_without_avatar_url = double('Member', created_by: double('User', avatar_url: nil))
-
-      expect(experiment('members/invite_email')).to exclude(actor: member_without_avatar_url)
-    end
-  end
-
-  describe "variant resolution" do
-    it "proves out round robin in variant selection", :aggregate_failures do
-      instance_1 = described_class.new('members/invite_email', **context)
-      allow(instance_1).to receive(:enabled?).and_return(true)
-      instance_2 = described_class.new('members/invite_email', **context)
-      allow(instance_2).to receive(:enabled?).and_return(true)
-
-      instance_1.try { }
-
-      expect(instance_1.variant.name).to eq('control')
-
-      instance_2.try { }
-
-      expect(instance_2.variant.name).to eq('activity')
-    end
-  end
-
-  describe Members::RoundRobin do
-    subject(:round_robin) { Members::RoundRobin.new('_key_', %i[variant1 variant2]) }
-
-    describe "execute" do
-      context "when there are 2 variants" do
-        it "proves out round robin in selection", :aggregate_failures do
-          expect(round_robin.execute).to eq :variant2
-          expect(round_robin.execute).to eq :variant1
-          expect(round_robin.execute).to eq :variant2
-        end
-      end
-
-      context "when there are more than 2 variants" do
-        subject(:round_robin) { Members::RoundRobin.new('_key_', %i[variant1 variant2 variant3]) }
-
-        it "proves out round robin in selection", :aggregate_failures do
-          expect(round_robin.execute).to eq :variant2
-          expect(round_robin.execute).to eq :variant3
-          expect(round_robin.execute).to eq :variant1
-
-          expect(round_robin.execute).to eq :variant2
-          expect(round_robin.execute).to eq :variant3
-          expect(round_robin.execute).to eq :variant1
-        end
-      end
-
-      context "when writing to cache fails" do
-        subject(:round_robin) { Members::RoundRobin.new('_key_', []) }
-
-        it "raises an error and logs" do
-          allow(Gitlab::Redis::SharedState).to receive(:with).and_raise(Members::RoundRobin::CacheError)
-          expect(Gitlab::AppLogger).to receive(:warn)
-
-          expect { round_robin.execute }.to raise_error(Members::RoundRobin::CacheError)
-        end
-      end
-    end
-
-    describe "#counter_expires_in" do
-      it 'displays the expiration time in seconds' do
-        round_robin.execute
-
-        expect(round_robin.counter_expires_in).to be_between(0, described_class::COUNTER_EXPIRE_TIME)
-      end
-    end
-
-    describe '#value' do
-      it 'get the count' do
-        expect(round_robin.counter_value).to eq(0)
-
-        round_robin.execute
-
-        expect(round_robin.counter_value).to eq(1)
-      end
-    end
-
-    describe '#reset!' do
-      it 'resets the count down to zero' do
-        3.times { round_robin.execute }
-
-        expect { round_robin.reset! }.to change { round_robin.counter_value }.from(3).to(0)
-      end
-    end
-  end
-end

spec/features/invites_spec.rb

@@ -179,7 +179,7 @@ def fill_in_welcome_form
 
     context 'when registering using invitation email' do
       before do
-        visit invite_path(group_invite.raw_invite_token, invite_type: Members::InviteEmailExperiment::INVITE_TYPE)
+        visit invite_path(group_invite.raw_invite_token, invite_type: Emails::Members::INITIAL_INVITE)
       end
 
       context 'with admin approval required enabled' do
@@ -219,13 +219,16 @@ def fill_in_welcome_form
       end
 
       context 'email confirmation enabled' do
-        context 'with members/invite_email experiment', :experiment do
+        context 'with invite email acceptance', :snowplow 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)
+
+            expect_snowplow_event(
+              category: 'RegistrationsController',
+              action: 'accepted',
+              label: 'invite_email',
+              property: group_invite.id.to_s
+            )
           end
         end
 

spec/mailers/notify_spec.rb

@@ -781,7 +781,9 @@ def invite_to_project(project, inviter:, user: nil)
       let(:project_member) { invite_to_project(project, inviter: inviter) }
       let(:inviter) { maintainer }
 
-      subject { described_class.member_invited_email('project', project_member.id, project_member.invite_token) }
+      subject(:invite_email) do
+        described_class.member_invited_email('project', project_member.id, project_member.invite_token)
+      end
 
       it_behaves_like 'an email sent from GitLab'
       it_behaves_like 'it should not have Gmail Actions links'
@@ -796,23 +798,10 @@ def invite_to_project(project, inviter:, user: nil)
           is_expected.to have_body_text project.full_name
           is_expected.to have_body_text project_member.human_access.downcase
           is_expected.to have_body_text project_member.invite_token
-          is_expected.to have_link('Join now', href: invite_url(project_member.invite_token, invite_type: Members::InviteEmailExperiment::INVITE_TYPE))
-        end
-
-        it 'contains invite link for the group activity' do
-          stub_experiments('members/invite_email': :activity)
-
+          is_expected.to have_link('Join now', href: invite_url(project_member.invite_token, invite_type: Emails::Members::INITIAL_INVITE))
           is_expected.to have_content("#{inviter.name} invited you to join the")
           is_expected.to have_content('Project details')
           is_expected.to have_content("What's it about?")
-          is_expected.not_to have_content('You are invited!')
-          is_expected.not_to have_body_text 'What is a GitLab'
-        end
-
-        it 'has invite link for the control group' do
-          stub_experiments('members/invite_email': :control)
-
-          is_expected.to have_content('You are invited!')
         end
       end
 
@@ -824,6 +813,22 @@ def invite_to_project(project, inviter:, user: nil)
           is_expected.to have_body_text project.full_name
           is_expected.to have_body_text project_member.human_access.downcase
           is_expected.to have_body_text project_member.invite_token
+          is_expected.to have_link('Join now', href: invite_url(project_member.invite_token, invite_type: Emails::Members::INITIAL_INVITE))
+          is_expected.to have_content('Project details')
+          is_expected.to have_content("What's it about?")
+        end
+      end
+
+      context 'when invite email sent is tracked', :snowplow do
+        it 'tracks the sent invite' do
+          invite_email.deliver_now
+
+          expect_snowplow_event(
+            category: 'Notify',
+            action: 'invite_email_sent',
+            label: 'invite_email',
+            property: project_member.id.to_s
+          )
         end
       end