Merge branch...

Merge branch '296966-update-new-user-invitation-email-test-variation-2-inviter-avatar' into 'master'

Update new user invitation email with an avatar

See merge request gitlab-org/gitlab!51223

.rubocop_manual_todo.yml

@@ -1409,7 +1409,6 @@ Gitlab/NamespacedClass:
     - 'app/controllers/user_callouts_controller.rb'
     - 'app/controllers/users_controller.rb'
     - 'app/controllers/whats_new_controller.rb'
-    - 'app/experiments/application_experiment.rb'
     - 'app/finders/abuse_reports_finder.rb'
     - 'app/finders/access_requests_finder.rb'
     - 'app/finders/admin/projects_finder.rb'

app/controllers/invites_controller.rb

@@ -6,6 +6,7 @@ class InvitesController < ApplicationController
   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?
@@ -15,11 +16,16 @@ 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: _("You have been granted %{member_human_access} access to %{title} %{name}.") %
         { member_human_access: member.human_access, title: invite_details[:title], name: invite_details[:name] }
     else
@@ -47,6 +53,14 @@ 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

app/experiments/application_experiment.rb

 # frozen_string_literal: true
 
-class ApplicationExperiment < Gitlab::Experiment
+class ApplicationExperiment < Gitlab::Experiment # rubocop:disable Gitlab/NamespacedClass
   def enabled?
-    return false if Feature::Definition.get(name).nil? # there has to be a feature flag yaml file
+    return false if Feature::Definition.get(feature_flag_name).nil? # there has to be a feature flag yaml file
     return false unless Gitlab.dev_env_or_com? # we're in an environment that allows experiments
 
-    Feature.get(name).state != :off # rubocop:disable Gitlab/AvoidFeatureGet
+    Feature.get(feature_flag_name).state != :off # rubocop:disable Gitlab/AvoidFeatureGet
   end
 
   def publish(_result)
@@ -26,11 +26,15 @@ class ApplicationExperiment < Gitlab::Experiment
   private
 
   def resolve_variant_name
-    return variant_names.first if Feature.enabled?(name, self, type: :experiment, default_enabled: :yaml)
+    return variant_names.first if Feature.enabled?(feature_flag_name, self, type: :experiment, default_enabled: :yaml)
 
     nil # Returning nil vs. :control is important for not caching and rollouts.
   end
 
+  def feature_flag_name
+    name.tr('/', '_')
+  end
+
   # Cache is an implementation on top of Gitlab::Redis::SharedState that also
   # adheres to the ActiveSupport::Cache::Store interface and uses the redis
   # hash data type.
@@ -48,7 +52,7 @@ class ApplicationExperiment < Gitlab::Experiment
   # default cache key strategy. So running `cache.fetch("foo:bar", "value")`
   # would create/update a hash with the key of "foo", with a field named
   # "bar" that has "value" assigned to it.
-  class Cache < ActiveSupport::Cache::Store
+  class Cache < ActiveSupport::Cache::Store # rubocop:disable Gitlab/NamespacedClass
     # Clears the entire cache for a given experiment. Be careful with this
     # since it would reset all resolved variants for the entire experiment.
     def clear(key:)

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'
+
+    private
+
+    def resolve_variant_name
+      # we are overriding here so that when we add another experiment
+      # we can merely add that variant and check of feature flag here
+      if Feature.enabled?(feature_flag_name, self, type: :experiment, default_enabled: :yaml)
+        :avatar
+      else
+        nil # :control
+      end
+    end
+  end
+end

app/mailers/emails/members.rb

@@ -4,9 +4,11 @@ module Emails
   module Members
     extend ActiveSupport::Concern
     include MembersHelper
+    include Gitlab::Experiment::Dsl
 
     included do
       helper_method :member_source, :member
+      helper_method :experiment
     end
 
     def member_access_requested_email(member_source_type, member_id, recipient_id)

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 }
-%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), class: 'invite-btn-join'
+
+- experiment('members/invite_email', actor: member) do |e|
+  - e.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'
+  - e.try(:avatar) do
+    %tr
+      %td.text-content
+        %img.avatar{ height: "60", src: avatar_icon_for_user(member.created_by, 60, only_path: false), style: "display: block; border-radius: 30px; margin: -2px 0;", 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'

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

spec/controllers/invites_controller_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe InvitesController, :snowplow do
+RSpec.describe InvitesController do
   let_it_be(:user) { create(:user) }
   let(:member) { create(:project_member, :invited, invite_email: user.email) }
   let(:raw_invite_token) { member.raw_invite_token }
@@ -51,6 +51,28 @@ RSpec.describe InvitesController, :snowplow 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
@@ -82,6 +104,25 @@ RSpec.describe InvitesController, :snowplow 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/experiments/application_experiment_spec.rb

@@ -3,14 +3,16 @@
 require 'spec_helper'
 
 RSpec.describe ApplicationExperiment, :experiment do
-  subject { described_class.new(:stub) }
+  subject { described_class.new('namespaced/stub') }
 
-  let(:feature_definition) { { name: 'stub', type: 'experiment', group: 'group::adoption', default_enabled: false } }
+  let(:feature_definition) do
+    { name: 'namespaced_stub', type: 'experiment', group: 'group::adoption', default_enabled: false }
+  end
 
   around do |example|
-    Feature::Definition.definitions[:stub] = Feature::Definition.new('stub.yml', feature_definition)
+    Feature::Definition.definitions[:namespaced_stub] = Feature::Definition.new('namespaced_stub.yml', feature_definition)
     example.run
-    Feature::Definition.definitions.delete(:stub)
+    Feature::Definition.definitions.delete(:namespaced_stub)
   end
 
   before do
@@ -18,9 +20,9 @@ RSpec.describe ApplicationExperiment, :experiment do
   end
 
   it "naively assumes a 1x1 relationship to feature flags for tests" do
-    expect(Feature).to receive(:persist_used!).with('stub')
+    expect(Feature).to receive(:persist_used!).with('namespaced_stub')
 
-    described_class.new(:stub)
+    described_class.new('namespaced/stub')
   end
 
   describe "enabled" do
@@ -37,7 +39,7 @@ RSpec.describe ApplicationExperiment, :experiment do
     end
 
     it "isn't enabled if the feature definition doesn't exist" do
-      expect(Feature::Definition).to receive(:get).with('stub').and_return(nil)
+      expect(Feature::Definition).to receive(:get).with('namespaced_stub').and_return(nil)
 
       expect(subject).not_to be_enabled
     end
@@ -49,7 +51,7 @@ RSpec.describe ApplicationExperiment, :experiment do
     end
 
     it "isn't enabled if the feature flag state is :off" do
-      expect(Feature).to receive(:get).with('stub').and_return(double(state: :off))
+      expect(Feature).to receive(:get).with('namespaced_stub').and_return(double(state: :off))
 
       expect(subject).not_to be_enabled
     end
@@ -66,9 +68,9 @@ RSpec.describe ApplicationExperiment, :experiment do
       expect(Gon.global).to receive(:push).with(
         {
           experiment: {
-            'stub' => { # string key because it can be namespaced
-              experiment: 'stub',
-              key: 'e8f65fd8d973f9985dc7ea3cf1614ae1',
+            'namespaced/stub' => { # string key because it can be namespaced
+              experiment: 'namespaced/stub',
+              key: '86208ac54ca798e11f127e8b23ec396a',
               variant: 'control'
             }
           }
@@ -95,7 +97,7 @@ RSpec.describe ApplicationExperiment, :experiment do
       ])
 
       expect_snowplow_event(
-        category: 'stub',
+        category: 'namespaced/stub',
         action: 'action',
         property: '_property_',
         context: [
@@ -105,7 +107,7 @@ RSpec.describe ApplicationExperiment, :experiment do
           },
           {
             schema: 'iglu:com.gitlab/gitlab_experiment/jsonschema/0-3-0',
-            data: { experiment: 'stub', key: 'e8f65fd8d973f9985dc7ea3cf1614ae1', variant: 'control' }
+            data: { experiment: 'namespaced/stub', key: '86208ac54ca798e11f127e8b23ec396a', variant: 'control' }
           }
         ]
       )
@@ -114,13 +116,13 @@ RSpec.describe ApplicationExperiment, :experiment do
 
   describe "variant resolution" do
     it "uses the default value as specified in the yaml" do
-      expect(Feature).to receive(:enabled?).with('stub', subject, type: :experiment, default_enabled: :yaml)
+      expect(Feature).to receive(:enabled?).with('namespaced_stub', subject, type: :experiment, default_enabled: :yaml)
 
       expect(subject.variant.name).to eq('control')
     end
 
     it "returns nil when not rolled out" do
-      stub_feature_flags(stub: false)
+      stub_feature_flags(namespaced_stub: false)
 
       expect(subject.variant.name).to eq('control')
     end
@@ -161,7 +163,7 @@ RSpec.describe ApplicationExperiment, :experiment do
       # every control variant assigned, we'd inflate the cache size and
       # wouldn't be able to roll out to subjects that we'd already assigned to
       # the control.
-      stub_feature_flags(stub: false) # simulate being not rolled out
+      stub_feature_flags(namespaced_stub: false) # simulate being not rolled out
 
       expect(subject.variant.name).to eq('control') # if we ask, it should be control
 

spec/experiments/members/invite_email_experiment_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Members::InviteEmailExperiment do
+  subject do
+    experiment('members/invite_email', actor: double('Member', created_by: double('User', avatar_url: '_avatar_url_')))
+  end
+
+  before do
+    allow(subject).to receive(:enabled?).and_return(true)
+  end
+
+  describe "variant resolution" do
+    it "returns nil when not rolled out" do
+      stub_feature_flags(members_invite_email: false)
+
+      expect(subject.variant.name).to eq('control')
+    end
+
+    context "when rolled out to 100%" do
+      it "returns the first variant name" do
+        subject.try(:avatar) {}
+
+        expect(subject.variant.name).to eq('avatar')
+      end
+    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
+end

spec/mailers/notify_spec.rb

@@ -905,6 +905,19 @@ RSpec.describe Notify do
           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 avatar', :experiment do
+          stub_experiments('members/invite_email': :avatar)
+
+          is_expected.not_to have_content('You are invited!')
+        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
 

spec/support/gitlab_experiment.rb

@@ -8,7 +8,7 @@ require 'gitlab/experiment/rspec'
 class ApplicationExperiment < Gitlab::Experiment # rubocop:disable Gitlab/NamespacedClass
   def initialize(*args)
     super
-    Feature.persist_used!(name)
+    Feature.persist_used!(feature_flag_name)
   end
 end