From 9feec3814d55f93616fff448d849914b367878de Mon Sep 17 00:00:00 2001 From: Alper Akgun <aakgun@gitlab.com> Date: Tue, 9 Mar 2021 18:52:36 +0000 Subject: [PATCH] Context merge for experiment conversions Add context to record_experiment_conversion_event similar to record_experiment_user --- app/models/experiment.rb | 20 ++- .../experimentation/controller_concern.rb | 4 +- .../controller_concern_spec.rb | 2 +- spec/models/experiment_spec.rb | 157 ++++++++++-------- 4 files changed, 104 insertions(+), 79 deletions(-) diff --git a/app/models/experiment.rb b/app/models/experiment.rb index 354b1e0b6b9..ac8b6516d02 100644 --- a/app/models/experiment.rb +++ b/app/models/experiment.rb @@ -14,22 +14,30 @@ class Experiment < ApplicationRecord 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 2b38b12c914..32f9f1b3157 100644 --- a/lib/gitlab/experimentation/controller_concern.rb +++ b/lib/gitlab/experimentation/controller_concern.rb @@ -72,12 +72,12 @@ module Gitlab ::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 1cebe37bea5..bb9d273995a 100644 --- a/spec/lib/gitlab/experimentation/controller_concern_spec.rb +++ b/spec/lib/gitlab/experimentation/controller_concern_spec.rb @@ -534,7 +534,7 @@ RSpec.describe Gitlab::Experimentation::ControllerConcern, type: :controller do 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 22bbf2df8fd..09dd1766acc 100644 --- a/spec/models/experiment_spec.rb +++ b/spec/models/experiment_spec.rb @@ -98,10 +98,11 @@ RSpec.describe Experiment do 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 @@ RSpec.describe Experiment do 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 @@ RSpec.describe Experiment do 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 @@ RSpec.describe Experiment do 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 @@ RSpec.describe Experiment do 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 @@ RSpec.describe Experiment do 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 -- 2.30.9