Commit 869cdb6c authored by Alper Akgun's avatar Alper Akgun

Merge branch 'revert-82325168' into 'master'

Revert changes to experiment_enabled due bug

See merge request gitlab-org/gitlab!51920
parents 82325168 37bad949
......@@ -6,7 +6,6 @@
# Experiment options:
# - tracking_category (optional, used to set the category when tracking an experiment event)
# - use_backwards_compatible_subject_index (optional, set this to true if you need backwards compatibility -- you likely do not need this, see note in the next paragraph.)
# - rollout_strategy: default is `:cookie` based rollout. We may also set it to `:user` based rollout
#
# Using the backwards-compatible subject index (use_backwards_compatible_subject_index option):
# This option was added when [the calculation of experimentation_subject_index was changed](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/45733/diffs#41af4a6fa5a10c7068559ce21c5188483751d934_157_173). It is not intended to be used by new experiments, it exists merely for the segmentation integrity of in-flight experiments at the time the change was deployed. That is, we want users who were assigned to the "experimental" group or the "control" group before the change to still be in those same groups after the change. See [the original issue](https://gitlab.com/gitlab-org/gitlab/-/issues/270858) and [this related comment](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/48110#note_458223745) for more information.
......@@ -93,19 +92,16 @@ module Gitlab
tracking_category: 'Growth::Conversion::Experiment::TrialDuringSignup'
},
ci_syntax_templates: {
tracking_category: 'Growth::Activation::Experiment::CiSyntaxTemplates',
rollout_strategy: :user
tracking_category: 'Growth::Activation::Experiment::CiSyntaxTemplates'
},
pipelines_empty_state: {
tracking_category: 'Growth::Activation::Experiment::PipelinesEmptyState',
rollout_strategy: :user
tracking_category: 'Growth::Activation::Experiment::PipelinesEmptyState'
},
invite_members_new_dropdown: {
tracking_category: 'Growth::Expansion::Experiment::InviteMembersNewDropdown'
},
show_trial_status_in_sidebar: {
tracking_category: 'Growth::Conversion::Experiment::ShowTrialStatusInSidebar',
rollout_strategy: :group
tracking_category: 'Growth::Conversion::Experiment::ShowTrialStatusInSidebar'
},
trial_onboarding_issues: {
tracking_category: 'Growth::Conversion::Experiment::TrialOnboardingIssues'
......@@ -129,7 +125,6 @@ module Gitlab
def in_experiment_group?(experiment_key, subject:)
return false if subject.blank?
return false unless active?(experiment_key)
raise 'Subject must conform to the rollout strategy' unless valid_subject_for_rollout_strategy?(experiment_key, subject)
experiment = get_experiment(experiment_key)
return false unless experiment
......@@ -137,26 +132,6 @@ module Gitlab
experiment.enabled_for_index?(index_for_subject(experiment, subject))
end
def rollout_strategy(experiment_key)
experiment = get_experiment(experiment_key)
return unless experiment
experiment.rollout_strategy
end
def valid_subject_for_rollout_strategy?(experiment_key, subject)
case rollout_strategy(experiment_key)
when :user
subject.is_a?(User)
when :group
subject.is_a?(Group)
when :cookie
subject.nil? || subject.is_a?(String)
else
false
end
end
private
def index_for_subject(experiment, subject)
......
......@@ -39,7 +39,6 @@ module Gitlab
def experiment_enabled?(experiment_key, subject: nil)
return true if forced_enabled?(experiment_key)
return false if dnt_enabled?
raise "Subject must conform to the rollout strategy for #{experiment_key}" unless Experimentation.valid_subject_for_rollout_strategy?(experiment_key, subject)
subject ||= fallback_experimentation_subject_index(experiment_key)
......@@ -66,9 +65,7 @@ module Gitlab
return if dnt_enabled?
return unless Experimentation.active?(experiment_key) && current_user
subject = Experimentation.rollout_strategy(experiment_key) == :cookie ? nil : current_user
::Experiment.add_user(experiment_key, tracking_group(experiment_key, nil, subject: subject), current_user, context)
::Experiment.add_user(experiment_key, tracking_group(experiment_key, nil, subject: current_user), current_user, context)
end
def record_experiment_conversion_event(experiment_key)
......
......@@ -5,13 +5,12 @@ module Gitlab
class Experiment
FEATURE_FLAG_SUFFIX = "_experiment_percentage"
attr_reader :key, :tracking_category, :use_backwards_compatible_subject_index, :rollout_strategy
attr_reader :key, :tracking_category, :use_backwards_compatible_subject_index
def initialize(key, **params)
@key = key
@tracking_category = params[:tracking_category]
@use_backwards_compatible_subject_index = params[:use_backwards_compatible_subject_index]
@rollout_strategy = params[:rollout_strategy] || :cookie
end
def active?
......
......@@ -10,10 +10,6 @@ RSpec.describe Gitlab::Experimentation::ControllerConcern, type: :controller do
use_backwards_compatible_subject_index: true
},
test_experiment: {
tracking_category: 'Team',
rollout_strategy: rollout_strategy
},
my_experiment: {
tracking_category: 'Team'
}
}
......@@ -24,7 +20,6 @@ RSpec.describe Gitlab::Experimentation::ControllerConcern, type: :controller do
end
let(:enabled_percentage) { 10 }
let(:rollout_strategy) { nil }
controller(ApplicationController) do
include Gitlab::Experimentation::ControllerConcern
......@@ -122,7 +117,6 @@ RSpec.describe Gitlab::Experimentation::ControllerConcern, type: :controller do
end
context 'when subject is given' do
let(:rollout_strategy) { :user }
let(:user) { build(:user) }
it 'uses the subject' do
......@@ -250,7 +244,6 @@ RSpec.describe Gitlab::Experimentation::ControllerConcern, type: :controller do
it "provides the subject's hashed global_id as label" do
experiment_subject = double(:subject, to_global_id: 'abc')
allow(Gitlab::Experimentation).to receive(:valid_subject_for_rollout_strategy?).and_return(true)
controller.track_experiment_event(:test_experiment, 'start', 1, subject: experiment_subject)
......@@ -427,26 +420,6 @@ RSpec.describe Gitlab::Experimentation::ControllerConcern, type: :controller do
controller.record_experiment_user(:test_experiment, context)
end
context 'with a cookie based rollout strategy' do
it 'calls tracking_group with a nil subject' do
expect(controller).to receive(:tracking_group).with(:test_experiment, nil, subject: nil).and_return(:experimental)
allow(::Experiment).to receive(:add_user).with(:test_experiment, :experimental, user, context)
controller.record_experiment_user(:test_experiment, context)
end
end
context 'with a user based rollout strategy' do
let(:rollout_strategy) { :user }
it 'calls tracking_group with a user subject' do
expect(controller).to receive(:tracking_group).with(:test_experiment, nil, subject: user).and_return(:experimental)
allow(::Experiment).to receive(:add_user).with(:test_experiment, :experimental, user, context)
controller.record_experiment_user(:test_experiment, context)
end
end
end
context 'the user is part of the control group' do
......
......@@ -9,8 +9,7 @@ RSpec.describe Gitlab::Experimentation::Experiment do
let(:params) do
{
tracking_category: 'Category1',
use_backwards_compatible_subject_index: true,
rollout_strategy: nil
use_backwards_compatible_subject_index: true
}
end
......
......@@ -102,11 +102,7 @@ RSpec.describe Gitlab::Experimentation do
context 'when subject has a global_id' do
let(:experiment_subject) { double(:subject, to_global_id: 'z') }
it do
allow(Gitlab::Experimentation).to receive(:valid_subject_for_rollout_strategy?).and_return(true)
is_expected.to eq(true)
end
it { is_expected.to eq(true) }
end
context 'when subject is nil' do
......@@ -154,11 +150,7 @@ RSpec.describe Gitlab::Experimentation do
context 'when subject has a global_id' do
let(:experiment_subject) { double(:subject, to_global_id: 'abcd') }
it do
allow(Gitlab::Experimentation).to receive(:valid_subject_for_rollout_strategy?).and_return(true)
is_expected.to eq(true)
end
it { is_expected.to eq(true) }
end
context 'when subject is nil' do
......
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