Merge branch '255979-add-converted_at-timestamp-column-to-the-experiment_users-table' into 'master'

Allow us to record conversion events for experiment participants

See merge request gitlab-org/gitlab!47093

app/models/experiment.rb

@@ -6,13 +6,19 @@ class Experiment < ApplicationRecord
   validates :name, presence: true, uniqueness: true, length: { maximum: 255 }
 
   def self.add_user(name, group_type, user)
-    return unless experiment = find_or_create_by(name: name)
+    find_or_create_by!(name: name).record_user_and_group(user, group_type)
+  end
 
-    experiment.record_user_and_group(user, group_type)
+  def self.record_conversion_event(name, user)
+    find_or_create_by!(name: name).record_conversion_event_for_user(user)
   end
 
   # Create or update the recorded experiment_user row for the user in this experiment.
   def record_user_and_group(user, group_type)
     experiment_users.find_or_initialize_by(user: user).update!(group_type: group_type)
   end
+
+  def record_conversion_event_for_user(user)
+    experiment_users.find_by(user: user, converted_at: nil)&.touch(:converted_at)
+  end
 end

changelogs/unreleased/255979-add-converted_at-timestamp-column-to-the-experiment_users-table.yml

+---
+title: Add `converted_at` timestamp column to `experiment_users` to record when
+  the user performs an experiment's conversion action
+merge_request: 47093
+author:
+type: changed

db/migrate/20201106193452_add_converted_at_to_experiment_users.rb

+# frozen_string_literal: true
+
+class AddConvertedAtToExperimentUsers < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  def change
+    add_column :experiment_users, :converted_at, :datetime_with_timezone
+  end
+end

db/schema_migrations/20201106193452

+dedc2eba6614c61df6e907ddd9813eea2b00fc43bccc6c3325674ad39950df62
\ No newline at end of file

db/structure.sql

@@ -12015,7 +12015,8 @@ CREATE TABLE experiment_users (
     user_id bigint NOT NULL,
     group_type smallint DEFAULT 0 NOT NULL,
     created_at timestamp with time zone NOT NULL,
-    updated_at timestamp with time zone NOT NULL
+    updated_at timestamp with time zone NOT NULL,
+    converted_at timestamp with time zone
 );
 
 CREATE SEQUENCE experiment_users_id_seq

doc/development/experiment_guide/index.md

@@ -232,6 +232,39 @@ describe('event tracking', () => {
 });
 ```
 
+### Record experiment user
+
+In addition to the anonymous tracking of events, we can also record which users have participated in which experiments and whether they were given the control experience or the experimental experience.
+
+The `record_experiment_user` helper method is available to all controllers, and it enables you to record these experiment participants (the current user) and which experience they were given:
+
+```ruby
+before_action do
+  record_experiment_user(:signup_flow)
+end
+```
+
+Subsequent calls to this method for the same experiment and the same user have no effect unless the user has gets enrolled into a different experience. This happens when we roll out the experimental experience to a greater percentage of users.
+
+Note that this data is completely separate from the [events tracking data](#implement-the-tracking-events). They are not linked together in any way.
+
+### Record experiment conversion event
+
+Along with the tracking of backend and frontend events and the [recording of experiment participants](#record-experiment-user), we can also record when a user performs the desired conversion event action. For example:
+
+- **Experimental experience:** Show an in-product nudge to see if it causes more people to sign up for trials.
+- **Conversion event:** The user starts a trial.
+
+The `record_experiment_conversion_event` helper method is available to all controllers, and enables us to easily record the conversion event for the current user, regardless of whether they are in the control or experimental group:
+
+```ruby
+before_action do
+  record_experiment_conversion_event(:signup_flow)
+end
+```
+
+Note that the use of this method requires that we have first [recorded the user as being part of the experiment](#record-experiment-user).
+
 ### Enable the experiment
 
 After all merge requests have been merged, use [`chatops`](../../ci/chatops/README.md) in the

lib/gitlab/experimentation/controller_concern.rb

@@ -67,6 +67,14 @@ module Gitlab
         ::Experiment.add_user(experiment_key, tracking_group(experiment_key), current_user)
       end
 
+      def record_experiment_conversion_event(experiment_key)
+        return if dnt_enabled?
+        return unless current_user
+        return unless Experimentation.enabled?(experiment_key)
+
+        ::Experiment.record_conversion_event(experiment_key, current_user)
+      end
+
       def experiment_tracking_category_and_group(experiment_key)
         "#{tracking_category(experiment_key)}:#{tracking_group(experiment_key, '_group')}"
       end

spec/factories/experiment_users.rb

+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :experiment_user do
+    experiment
+    user
+    group_type { :control }
+    converted_at { nil }
+  end
+end

spec/lib/gitlab/experimentation/controller_concern_spec.rb

@@ -418,6 +418,56 @@ RSpec.describe Gitlab::Experimentation::ControllerConcern, type: :controller do
     end
   end
 
+  describe '#record_experiment_conversion_event' do
+    let(:user) { build(:user) }
+
+    before do
+      allow(controller).to receive(:dnt_enabled?).and_return(false)
+      allow(controller).to receive(:current_user).and_return(user)
+      stub_experiment(test_experiment: true)
+    end
+
+    subject(:record_conversion_event) do
+      controller.record_experiment_conversion_event(:test_experiment)
+    end
+
+    it 'records the conversion event for the experiment & user' do
+      expect(::Experiment).to receive(:record_conversion_event).with(:test_experiment, user)
+      record_conversion_event
+    end
+
+    shared_examples 'does not record the conversion event' do
+      it 'does not record the conversion event' do
+        expect(::Experiment).not_to receive(:record_conversion_event)
+        record_conversion_event
+      end
+    end
+
+    context 'when DNT is enabled' do
+      before do
+        allow(controller).to receive(:dnt_enabled?).and_return(true)
+      end
+
+      include_examples 'does not record the conversion event'
+    end
+
+    context 'when there is no current user' do
+      before do
+        allow(controller).to receive(:current_user).and_return(nil)
+      end
+
+      include_examples 'does not record the conversion event'
+    end
+
+    context 'when the experiment is not enabled' do
+      before do
+        stub_experiment(test_experiment: false)
+      end
+
+      include_examples 'does not record the conversion event'
+    end
+  end
+
   describe '#experiment_tracking_category_and_group' do
     let_it_be(:experiment_key) { :test_something }
 

spec/models/experiment_spec.rb

@@ -57,6 +57,75 @@ RSpec.describe Experiment do
     end
   end
 
+  describe '.record_conversion_event' do
+    let_it_be(:user) { build(:user) }
+
+    let(:experiment_key) { :test_experiment }
+
+    subject(:record_conversion_event) { described_class.record_conversion_event(experiment_key, user) }
+
+    context 'when no matching experiment exists' do
+      it 'creates the experiment and uses it' do
+        expect_next_instance_of(described_class) do |experiment|
+          expect(experiment).to receive(:record_conversion_event_for_user)
+        end
+        expect { record_conversion_event }.to change { described_class.count }.by(1)
+      end
+
+      context 'but we are unable to successfully create one' do
+        let(:experiment_key) { nil }
+
+        it 'raises a RecordInvalid error' do
+          expect { record_conversion_event }.to raise_error(ActiveRecord::RecordInvalid)
+        end
+      end
+    end
+
+    context 'when a matching experiment already exists' do
+      before do
+        create(:experiment, name: experiment_key)
+      end
+
+      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)
+        end
+        record_conversion_event
+      end
+    end
+  end
+
+  describe '#record_conversion_event_for_user' do
+    let_it_be(:user) { create(:user) }
+    let_it_be(:experiment) { create(:experiment) }
+
+    subject(:record_conversion_event_for_user) { experiment.record_conversion_event_for_user(user) }
+
+    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 }
+      end
+    end
+
+    context 'when an existing experiment_user exists for the given user' do
+      context 'but it has already been converted' 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 }
+        end
+      end
+
+      context 'and it has not yet been converted' 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 }
+        end
+      end
+    end
+  end
+
   describe '#record_user_and_group' do
     let_it_be(:experiment) { create(:experiment) }
     let_it_be(:user) { create(:user) }