Commit b1d17607 authored by Doug Stull's avatar Doug Stull Committed by Bob Van Landuyt

Add invite email avatar experiment

- allows us to measure acceptance of invites with avatars
parent 989ac206
...@@ -1410,7 +1410,6 @@ Gitlab/NamespacedClass: ...@@ -1410,7 +1410,6 @@ Gitlab/NamespacedClass:
- 'app/controllers/user_callouts_controller.rb' - 'app/controllers/user_callouts_controller.rb'
- 'app/controllers/users_controller.rb' - 'app/controllers/users_controller.rb'
- 'app/controllers/whats_new_controller.rb' - 'app/controllers/whats_new_controller.rb'
- 'app/experiments/application_experiment.rb'
- 'app/finders/abuse_reports_finder.rb' - 'app/finders/abuse_reports_finder.rb'
- 'app/finders/access_requests_finder.rb' - 'app/finders/access_requests_finder.rb'
- 'app/finders/admin/projects_finder.rb' - 'app/finders/admin/projects_finder.rb'
......
...@@ -6,6 +6,7 @@ class InvitesController < ApplicationController ...@@ -6,6 +6,7 @@ class InvitesController < ApplicationController
before_action :member before_action :member
before_action :ensure_member_exists before_action :ensure_member_exists
before_action :invite_details before_action :invite_details
before_action :set_invite_type, only: :show
skip_before_action :authenticate_user!, only: :decline skip_before_action :authenticate_user!, only: :decline
helper_method :member?, :current_user_matches_invite? helper_method :member?, :current_user_matches_invite?
...@@ -15,11 +16,16 @@ class InvitesController < ApplicationController ...@@ -15,11 +16,16 @@ class InvitesController < ApplicationController
feature_category :authentication_and_authorization feature_category :authentication_and_authorization
def show def show
experiment('members/invite_email', actor: member).track(:opened) if initial_invite_email?
accept if skip_invitation_prompt? accept if skip_invitation_prompt?
end end
def accept def accept
if member.accept_invite!(current_user) 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}.") % 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] } { member_human_access: member.human_access, title: invite_details[:title], name: invite_details[:name] }
else else
...@@ -47,6 +53,14 @@ class InvitesController < ApplicationController ...@@ -47,6 +53,14 @@ class InvitesController < ApplicationController
private 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? def skip_invitation_prompt?
!member? && current_user_matches_invite? !member? && current_user_matches_invite?
end end
......
# frozen_string_literal: true # frozen_string_literal: true
class ApplicationExperiment < Gitlab::Experiment class ApplicationExperiment < Gitlab::Experiment # rubocop:disable Gitlab/NamespacedClass
def enabled? 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 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 end
def publish(_result) def publish(_result)
...@@ -26,11 +26,15 @@ class ApplicationExperiment < Gitlab::Experiment ...@@ -26,11 +26,15 @@ class ApplicationExperiment < Gitlab::Experiment
private private
def resolve_variant_name 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. nil # Returning nil vs. :control is important for not caching and rollouts.
end end
def feature_flag_name
name.tr('/', '_')
end
# Cache is an implementation on top of Gitlab::Redis::SharedState that also # Cache is an implementation on top of Gitlab::Redis::SharedState that also
# adheres to the ActiveSupport::Cache::Store interface and uses the redis # adheres to the ActiveSupport::Cache::Store interface and uses the redis
# hash data type. # hash data type.
...@@ -48,7 +52,7 @@ class ApplicationExperiment < Gitlab::Experiment ...@@ -48,7 +52,7 @@ class ApplicationExperiment < Gitlab::Experiment
# default cache key strategy. So running `cache.fetch("foo:bar", "value")` # 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 # would create/update a hash with the key of "foo", with a field named
# "bar" that has "value" assigned to it. # "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 # Clears the entire cache for a given experiment. Be careful with this
# since it would reset all resolved variants for the entire experiment. # since it would reset all resolved variants for the entire experiment.
def clear(key:) def clear(key:)
......
# 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
...@@ -4,9 +4,11 @@ module Emails ...@@ -4,9 +4,11 @@ module Emails
module Members module Members
extend ActiveSupport::Concern extend ActiveSupport::Concern
include MembersHelper include MembersHelper
include Gitlab::Experiment::Dsl
included do included do
helper_method :member_source, :member helper_method :member_source, :member
helper_method :experiment
end end
def member_access_requested_email(member_source_type, member_id, recipient_id) def member_access_requested_email(member_source_type, member_id, recipient_id)
......
- 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 }
%tr
- experiment('members/invite_email', actor: member) do |e|
- e.use do
%tr
%td.text-content %td.text-content
%h2.invite-header %h2.invite-header
= s_('InviteEmail|You are invited!') = s_('InviteEmail|You are invited!')
...@@ -9,4 +12,12 @@ ...@@ -9,4 +12,12 @@
- else - 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 = 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 %p.invite-actions
= link_to s_('InviteEmail|Join now'), invite_url(@token), class: 'invite-btn-join' = 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'
---
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
...@@ -2,7 +2,7 @@ ...@@ -2,7 +2,7 @@
require 'spec_helper' require 'spec_helper'
RSpec.describe InvitesController, :snowplow do RSpec.describe InvitesController do
let_it_be(:user) { create(:user) } let_it_be(:user) { create(:user) }
let(:member) { create(:project_member, :invited, invite_email: user.email) } let(:member) { create(:project_member, :invited, invite_email: user.email) }
let(:raw_invite_token) { member.raw_invite_token } let(:raw_invite_token) { member.raw_invite_token }
...@@ -51,6 +51,28 @@ RSpec.describe InvitesController, :snowplow do ...@@ -51,6 +51,28 @@ RSpec.describe InvitesController, :snowplow do
end end
it_behaves_like 'invalid token' 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 end
context 'when not logged in' do context 'when not logged in' do
...@@ -82,6 +104,25 @@ RSpec.describe InvitesController, :snowplow do ...@@ -82,6 +104,25 @@ RSpec.describe InvitesController, :snowplow do
subject(:request) { post :accept, params: params } subject(:request) { post :accept, params: params }
it_behaves_like 'invalid token' 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 end
describe 'POST #decline for link in UI' do describe 'POST #decline for link in UI' do
......
...@@ -3,14 +3,16 @@ ...@@ -3,14 +3,16 @@
require 'spec_helper' require 'spec_helper'
RSpec.describe ApplicationExperiment, :experiment do 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| 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 example.run
Feature::Definition.definitions.delete(:stub) Feature::Definition.definitions.delete(:namespaced_stub)
end end
before do before do
...@@ -18,9 +20,9 @@ RSpec.describe ApplicationExperiment, :experiment do ...@@ -18,9 +20,9 @@ RSpec.describe ApplicationExperiment, :experiment do
end end
it "naively assumes a 1x1 relationship to feature flags for tests" do 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 end
describe "enabled" do describe "enabled" do
...@@ -37,7 +39,7 @@ RSpec.describe ApplicationExperiment, :experiment do ...@@ -37,7 +39,7 @@ RSpec.describe ApplicationExperiment, :experiment do
end end
it "isn't enabled if the feature definition doesn't exist" do 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 expect(subject).not_to be_enabled
end end
...@@ -49,7 +51,7 @@ RSpec.describe ApplicationExperiment, :experiment do ...@@ -49,7 +51,7 @@ RSpec.describe ApplicationExperiment, :experiment do
end end
it "isn't enabled if the feature flag state is :off" do 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 expect(subject).not_to be_enabled
end end
...@@ -66,9 +68,9 @@ RSpec.describe ApplicationExperiment, :experiment do ...@@ -66,9 +68,9 @@ RSpec.describe ApplicationExperiment, :experiment do
expect(Gon.global).to receive(:push).with( expect(Gon.global).to receive(:push).with(
{ {
experiment: { experiment: {
'stub' => { # string key because it can be namespaced 'namespaced/stub' => { # string key because it can be namespaced
experiment: 'stub', experiment: 'namespaced/stub',
key: 'e8f65fd8d973f9985dc7ea3cf1614ae1', key: '86208ac54ca798e11f127e8b23ec396a',
variant: 'control' variant: 'control'
} }
} }
...@@ -95,7 +97,7 @@ RSpec.describe ApplicationExperiment, :experiment do ...@@ -95,7 +97,7 @@ RSpec.describe ApplicationExperiment, :experiment do
]) ])
expect_snowplow_event( expect_snowplow_event(
category: 'stub', category: 'namespaced/stub',
action: 'action', action: 'action',
property: '_property_', property: '_property_',
context: [ context: [
...@@ -105,7 +107,7 @@ RSpec.describe ApplicationExperiment, :experiment do ...@@ -105,7 +107,7 @@ RSpec.describe ApplicationExperiment, :experiment do
}, },
{ {
schema: 'iglu:com.gitlab/gitlab_experiment/jsonschema/0-3-0', 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 ...@@ -114,13 +116,13 @@ RSpec.describe ApplicationExperiment, :experiment do
describe "variant resolution" do describe "variant resolution" do
it "uses the default value as specified in the yaml" 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') expect(subject.variant.name).to eq('control')
end end
it "returns nil when not rolled out" do 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') expect(subject.variant.name).to eq('control')
end end
...@@ -161,7 +163,7 @@ RSpec.describe ApplicationExperiment, :experiment do ...@@ -161,7 +163,7 @@ RSpec.describe ApplicationExperiment, :experiment do
# every control variant assigned, we'd inflate the cache size and # 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 # wouldn't be able to roll out to subjects that we'd already assigned to
# the control. # 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 expect(subject.variant.name).to eq('control') # if we ask, it should be control
......
# 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
...@@ -905,6 +905,19 @@ RSpec.describe Notify do ...@@ -905,6 +905,19 @@ RSpec.describe Notify do
is_expected.to have_body_text project.full_name 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.human_access.downcase
is_expected.to have_body_text project_member.invite_token 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
end end
......
...@@ -8,7 +8,7 @@ require 'gitlab/experiment/rspec' ...@@ -8,7 +8,7 @@ require 'gitlab/experiment/rspec'
class ApplicationExperiment < Gitlab::Experiment # rubocop:disable Gitlab/NamespacedClass class ApplicationExperiment < Gitlab::Experiment # rubocop:disable Gitlab/NamespacedClass
def initialize(*args) def initialize(*args)
super super
Feature.persist_used!(name) Feature.persist_used!(feature_flag_name)
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment