diff --git a/app/models/experiment.rb b/app/models/experiment.rb
index 354b1e0b6b984247fb344acf243856b15ea6953a..ac8b6516d02b1515bf9ade29bf8ff3c951f45445 100644
--- a/app/models/experiment.rb
+++ b/app/models/experiment.rb
@@ -14,22 +14,30 @@ def self.add_group(name, variant:, group:)
find_or_create_by!(name: name).record_group_and_variant!(group, variant)
end
- def self.record_conversion_event(name, user)
- find_or_create_by!(name: name).record_conversion_event_for_user(user)
+ def self.record_conversion_event(name, user, context = {})
+ find_or_create_by!(name: name).record_conversion_event_for_user(user, context)
end
# Create or update the recorded experiment_user row for the user in this experiment.
def record_user_and_group(user, group_type, context = {})
experiment_user = experiment_users.find_or_initialize_by(user: user)
- merged_context = experiment_user.context.deep_merge(context.deep_stringify_keys)
- experiment_user.update!(group_type: group_type, context: merged_context)
+ experiment_user.update!(group_type: group_type, context: merged_context(experiment_user, context))
end
- def record_conversion_event_for_user(user)
- experiment_users.find_by(user: user, converted_at: nil)&.touch(:converted_at)
+ def record_conversion_event_for_user(user, context = {})
+ experiment_user = experiment_users.find_by(user: user)
+ return unless experiment_user
+
+ experiment_user.update!(converted_at: Time.current, context: merged_context(experiment_user, context))
end
def record_group_and_variant!(group, variant)
experiment_subjects.find_or_initialize_by(group: group).update!(variant: variant)
end
+
+ private
+
+ def merged_context(experiment_user, new_context)
+ experiment_user.context.deep_merge(new_context.deep_stringify_keys)
+ end
end
diff --git a/lib/gitlab/experimentation/controller_concern.rb b/lib/gitlab/experimentation/controller_concern.rb
index 2b38b12c914d80446bd443e80e0b5171f7a67538..32f9f1b3157a4fe1d1d77958b78aa529fb7ade6c 100644
--- a/lib/gitlab/experimentation/controller_concern.rb
+++ b/lib/gitlab/experimentation/controller_concern.rb
@@ -72,12 +72,12 @@ def record_experiment_user(experiment_key, context = {})
::Experiment.add_user(experiment_key, tracking_group(experiment_key, nil, subject: subject), current_user, context)
end
- def record_experiment_conversion_event(experiment_key)
+ def record_experiment_conversion_event(experiment_key, context = {})
return if dnt_enabled?
return unless current_user
return unless Experimentation.active?(experiment_key)
- ::Experiment.record_conversion_event(experiment_key, current_user)
+ ::Experiment.record_conversion_event(experiment_key, current_user, context)
end
def experiment_tracking_category_and_group(experiment_key, subject: nil)
diff --git a/spec/lib/gitlab/experimentation/controller_concern_spec.rb b/spec/lib/gitlab/experimentation/controller_concern_spec.rb
index 1cebe37bea5bc3015952f3f111a58a041206e7de..bb9d273995a77af2a010f7d214e6d4095400025e 100644
--- a/spec/lib/gitlab/experimentation/controller_concern_spec.rb
+++ b/spec/lib/gitlab/experimentation/controller_concern_spec.rb
@@ -534,7 +534,7 @@ def check_experiment(exp_key = :test_experiment, subject = nil)
end
it 'records the conversion event for the experiment & user' do
- expect(::Experiment).to receive(:record_conversion_event).with(:test_experiment, user)
+ expect(::Experiment).to receive(:record_conversion_event).with(:test_experiment, user, {})
record_conversion_event
end
diff --git a/spec/models/experiment_spec.rb b/spec/models/experiment_spec.rb
index 22bbf2df8fd1c976010220c62eba5481c52d91bb..09dd1766acc7c80bf9138b9064aa2960bf6612e7 100644
--- a/spec/models/experiment_spec.rb
+++ b/spec/models/experiment_spec.rb
@@ -98,10 +98,11 @@
describe '.record_conversion_event' do
let_it_be(:user) { build(:user) }
+ let_it_be(:context) { { a: 42 } }
let(:experiment_key) { :test_experiment }
- subject(:record_conversion_event) { described_class.record_conversion_event(experiment_key, user) }
+ subject(:record_conversion_event) { described_class.record_conversion_event(experiment_key, user, context) }
context 'when no matching experiment exists' do
it 'creates the experiment and uses it' do
@@ -127,22 +128,79 @@
it 'sends record_conversion_event_for_user to the experiment instance' do
expect_next_found_instance_of(described_class) do |experiment|
- expect(experiment).to receive(:record_conversion_event_for_user).with(user)
+ expect(experiment).to receive(:record_conversion_event_for_user).with(user, context)
end
record_conversion_event
end
end
end
+ shared_examples 'experiment user with context' do
+ let_it_be(:context) { { a: 42, 'b' => 34, 'c': { c1: 100, c2: 'c2', e: :e }, d: [1, 3] } }
+ let_it_be(:initial_expected_context) { { 'a' => 42, 'b' => 34, 'c' => { 'c1' => 100, 'c2' => 'c2', 'e' => 'e' }, 'd' => [1, 3] } }
+
+ before do
+ subject
+ experiment.record_user_and_group(user, :experimental, {})
+ end
+
+ it 'has an initial context with stringified keys' do
+ expect(ExperimentUser.last.context).to eq(initial_expected_context)
+ end
+
+ context 'when updated' do
+ before do
+ subject
+ experiment.record_user_and_group(user, :experimental, new_context)
+ end
+
+ context 'with an empty context' do
+ let_it_be(:new_context) { {} }
+
+ it 'keeps the initial context' do
+ expect(ExperimentUser.last.context).to eq(initial_expected_context)
+ end
+ end
+
+ context 'with string keys' do
+ let_it_be(:new_context) { { f: :some_symbol } }
+
+ it 'adds new symbols stringified' do
+ expected_context = initial_expected_context.merge('f' => 'some_symbol')
+ expect(ExperimentUser.last.context).to eq(expected_context)
+ end
+ end
+
+ context 'with atomic values or array values' do
+ let_it_be(:new_context) { { b: 97, d: [99] } }
+
+ it 'overrides the values' do
+ expected_context = { 'a' => 42, 'b' => 97, 'c' => { 'c1' => 100, 'c2' => 'c2', 'e' => 'e' }, 'd' => [99] }
+ expect(ExperimentUser.last.context).to eq(expected_context)
+ end
+ end
+
+ context 'with nested hashes' do
+ let_it_be(:new_context) { { c: { g: 107 } } }
+
+ it 'inserts nested additional values in the same keys' do
+ expected_context = initial_expected_context.deep_merge('c' => { 'g' => 107 })
+ expect(ExperimentUser.last.context).to eq(expected_context)
+ end
+ end
+ end
+ end
+
describe '#record_conversion_event_for_user' do
let_it_be(:user) { create(:user) }
let_it_be(:experiment) { create(:experiment) }
+ let_it_be(:context) { { a: 42 } }
- subject(:record_conversion_event_for_user) { experiment.record_conversion_event_for_user(user) }
+ subject { experiment.record_conversion_event_for_user(user, context) }
context 'when no existing experiment_user record exists for the given user' do
it 'does not update or create an experiment_user record' do
- expect { record_conversion_event_for_user }.not_to change { ExperimentUser.all.to_a }
+ expect { subject }.not_to change { ExperimentUser.all.to_a }
end
end
@@ -151,7 +209,13 @@
let!(:experiment_user) { create(:experiment_user, experiment: experiment, user: user, converted_at: 2.days.ago) }
it 'does not update the converted_at value' do
- expect { record_conversion_event_for_user }.not_to change { experiment_user.converted_at }
+ expect { subject }.not_to change { experiment_user.converted_at }
+ end
+
+ it_behaves_like 'experiment user with context' do
+ before do
+ experiment.record_user_and_group(user, :experimental, context)
+ end
end
end
@@ -159,7 +223,13 @@
let(:experiment_user) { create(:experiment_user, experiment: experiment, user: user) }
it 'updates the converted_at value' do
- expect { record_conversion_event_for_user }.to change { experiment_user.reload.converted_at }
+ expect { subject }.to change { experiment_user.reload.converted_at }
+ end
+
+ it_behaves_like 'experiment user with context' do
+ before do
+ experiment.record_user_and_group(user, :experimental, context)
+ end
end
end
end
@@ -196,24 +266,25 @@
describe '#record_user_and_group' do
let_it_be(:experiment) { create(:experiment) }
let_it_be(:user) { create(:user) }
+ let_it_be(:group) { :control }
+ let_it_be(:context) { { a: 42 } }
- let(:group) { :control }
- let(:context) { { a: 42 } }
-
- subject(:record_user_and_group) { experiment.record_user_and_group(user, group, context) }
+ subject { experiment.record_user_and_group(user, group, context) }
context 'when an experiment_user does not yet exist for the given user' do
it 'creates a new experiment_user record' do
- expect { record_user_and_group }.to change(ExperimentUser, :count).by(1)
+ expect { subject }.to change(ExperimentUser, :count).by(1)
end
it 'assigns the correct group_type to the experiment_user' do
- record_user_and_group
+ subject
+
expect(ExperimentUser.last.group_type).to eq('control')
end
it 'adds the correct context to the experiment_user' do
- record_user_and_group
+ subject
+
expect(ExperimentUser.last.context).to eq({ 'a' => 42 })
end
end
@@ -225,72 +296,18 @@
end
it 'does not create a new experiment_user record' do
- expect { record_user_and_group }.not_to change(ExperimentUser, :count)
+ expect { subject }.not_to change(ExperimentUser, :count)
end
context 'but the group_type and context has changed' do
let(:group) { :experimental }
it 'updates the existing experiment_user record with group_type' do
- expect { record_user_and_group }.to change { ExperimentUser.last.group_type }
+ expect { subject }.to change { ExperimentUser.last.group_type }
end
end
- end
-
- context 'when a context already exists' do
- let_it_be(:context) { { a: 42, 'b' => 34, 'c': { c1: 100, c2: 'c2', e: :e }, d: [1, 3] } }
- let_it_be(:initial_expected_context) { { 'a' => 42, 'b' => 34, 'c' => { 'c1' => 100, 'c2' => 'c2', 'e' => 'e' }, 'd' => [1, 3] } }
-
- before do
- record_user_and_group
- experiment.record_user_and_group(user, :control, {})
- end
-
- it 'has an initial context with stringified keys' do
- expect(ExperimentUser.last.context).to eq(initial_expected_context)
- end
-
- context 'when updated' do
- before do
- record_user_and_group
- experiment.record_user_and_group(user, :control, new_context)
- end
-
- context 'with an empty context' do
- let_it_be(:new_context) { {} }
- it 'keeps the initial context' do
- expect(ExperimentUser.last.context).to eq(initial_expected_context)
- end
- end
-
- context 'with string keys' do
- let_it_be(:new_context) { { f: :some_symbol } }
-
- it 'adds new symbols stringified' do
- expected_context = initial_expected_context.merge('f' => 'some_symbol')
- expect(ExperimentUser.last.context).to eq(expected_context)
- end
- end
-
- context 'with atomic values or array values' do
- let_it_be(:new_context) { { b: 97, d: [99] } }
-
- it 'overrides the values' do
- expected_context = { 'a' => 42, 'b' => 97, 'c' => { 'c1' => 100, 'c2' => 'c2', 'e' => 'e' }, 'd' => [99] }
- expect(ExperimentUser.last.context).to eq(expected_context)
- end
- end
-
- context 'with nested hashes' do
- let_it_be(:new_context) { { c: { g: 107 } } }
-
- it 'inserts nested additional values in the same keys' do
- expected_context = initial_expected_context.deep_merge('c' => { 'g' => 107 })
- expect(ExperimentUser.last.context).to eq(expected_context)
- end
- end
- end
+ it_behaves_like 'experiment user with context'
end
end
end