Merge branch 'experiments-table' into 'master'

Add experiments and experiment_users tables

See merge request gitlab-org/gitlab!38397

app/controllers/registrations_controller.rb

@@ -62,7 +62,11 @@ class RegistrationsController < Devise::RegistrationsController
     result = ::Users::SignupService.new(current_user, user_params).execute
 
     if result[:status] == :success
-      track_experiment_event(:onboarding_issues, 'signed_up') if ::Gitlab.com? && show_onboarding_issues_experiment?
+      if ::Gitlab.com? && show_onboarding_issues_experiment?
+        track_experiment_event(:onboarding_issues, 'signed_up')
+        record_experiment_user(:onboarding_issues)
+      end
+
       return redirect_to new_users_sign_up_group_path if experiment_enabled?(:onboarding_issues) && show_onboarding_issues_experiment?
 
       set_flash_message! :notice, :signed_up

app/models/experiment.rb

+# frozen_string_literal: true
+
+class Experiment < ApplicationRecord
+  has_many :experiment_users
+  has_many :users, through: :experiment_users
+  has_many :control_group_users, -> { merge(ExperimentUser.control) }, through: :experiment_users, source: :user
+  has_many :experimental_group_users, -> { merge(ExperimentUser.experimental) }, through: :experiment_users, source: :user
+
+  validates :name, presence: true, uniqueness: true, length: { maximum: 255 }
+
+  def self.add_user(name, group_type, user)
+    experiment = find_or_create_by(name: name)
+
+    return unless experiment
+    return if experiment.experiment_users.where(user: user).exists?
+
+    group_type == ::Gitlab::Experimentation::GROUP_CONTROL ? experiment.add_control_user(user) : experiment.add_experimental_user(user)
+  end
+
+  def add_control_user(user)
+    control_group_users << user
+  end
+
+  def add_experimental_user(user)
+    experimental_group_users << user
+  end
+end

app/models/experiment_user.rb

+# frozen_string_literal: true
+
+class ExperimentUser < ApplicationRecord
+  belongs_to :experiment
+  belongs_to :user
+
+  enum group_type: { control: 0, experimental: 1 }
+
+  validates :group_type, presence: true
+end

changelogs/unreleased/experiments-table.yml

+---
+title: Add experiments and experiment_users tables for tracking which users are enrolled for which experiments.
+merge_request: 38397
+author:
+type: added

db/migrate/20200731085019_create_experiment.rb

+# frozen_string_literal: true
+
+class CreateExperiment < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    unless table_exists?(:experiments)
+      create_table :experiments do |t|
+        t.text :name, null: false
+
+        t.index :name, unique: true
+      end
+    end
+
+    add_text_limit :experiments, :name, 255
+  end
+
+  def down
+    drop_table :experiments
+  end
+end

db/migrate/20200731090553_create_experiment_user.rb

+# frozen_string_literal: true
+
+class CreateExperimentUser < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  def up
+    create_table :experiment_users do |t|
+      t.bigint :experiment_id, null: false
+      t.bigint :user_id, null: false
+      t.integer :group_type, limit: 2, null: false, default: 0
+      t.timestamps_with_timezone null: false
+    end
+
+    add_index :experiment_users, :experiment_id
+    add_index :experiment_users, :user_id
+  end
+
+  def down
+    drop_table :experiment_users
+  end
+end

db/migrate/20200731201408_add_foreign_key_to_experiment_on_experiment_users.rb

+# frozen_string_literal: true
+
+class AddForeignKeyToExperimentOnExperimentUsers < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    with_lock_retries do
+      # There is no need to use add_concurrent_foreign_key since it's an empty table
+      add_foreign_key :experiment_users, :experiments, column: :experiment_id, on_delete: :cascade # rubocop:disable Migration/AddConcurrentForeignKey
+    end
+  end
+
+  def down
+    with_lock_retries do
+      remove_foreign_key :experiment_users, column: :experiment_id
+    end
+  end
+end

db/migrate/20200731201834_add_foreign_key_to_user_on_experiment_users.rb

+# frozen_string_literal: true
+
+class AddForeignKeyToUserOnExperimentUsers < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    with_lock_retries do
+      # There is no need to use add_concurrent_foreign_key since it's an empty table
+      add_foreign_key :experiment_users, :users, column: :user_id, on_delete: :cascade # rubocop:disable Migration/AddConcurrentForeignKey
+    end
+  end
+
+  def down
+    with_lock_retries do
+      remove_foreign_key :experiment_users, column: :user_id
+    end
+  end
+end

db/schema_migrations/20200731085019

+aeeef4762f7ab7c7eefc28995ba198825455a43db38c1ff2aefad8c3c76b5fba
\ No newline at end of file

db/schema_migrations/20200731090553

+c5775e8150285927b74c2fbf6098d03920e7bea52c3b94ad372fec288835110c
\ No newline at end of file

db/schema_migrations/20200731201408

+02f3561a5b8fa79c58dccc23ebaecf2c1b8371c86bb360156456bd11e84d13a2
\ No newline at end of file

db/schema_migrations/20200731201834

+26aa28efa15ebd223feb879d5cb684a53a92bc71e483e8348d0cd9f1ad10e7ae
\ No newline at end of file

db/structure.sql

@@ -11558,6 +11558,39 @@ CREATE SEQUENCE public.evidences_id_seq
 
 ALTER SEQUENCE public.evidences_id_seq OWNED BY public.evidences.id;
 
+CREATE TABLE public.experiment_users (
+    id bigint NOT NULL,
+    experiment_id bigint NOT NULL,
+    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
+);
+
+CREATE SEQUENCE public.experiment_users_id_seq
+    START WITH 1
+    INCREMENT BY 1
+    NO MINVALUE
+    NO MAXVALUE
+    CACHE 1;
+
+ALTER SEQUENCE public.experiment_users_id_seq OWNED BY public.experiment_users.id;
+
+CREATE TABLE public.experiments (
+    id bigint NOT NULL,
+    name text NOT NULL,
+    CONSTRAINT check_e2dda25ed0 CHECK ((char_length(name) <= 255))
+);
+
+CREATE SEQUENCE public.experiments_id_seq
+    START WITH 1
+    INCREMENT BY 1
+    NO MINVALUE
+    NO MAXVALUE
+    CACHE 1;
+
+ALTER SEQUENCE public.experiments_id_seq OWNED BY public.experiments.id;
+
 CREATE TABLE public.external_pull_requests (
     id bigint NOT NULL,
     created_at timestamp with time zone NOT NULL,
@@ -16875,6 +16908,10 @@ ALTER TABLE ONLY public.events ALTER COLUMN id SET DEFAULT nextval('public.event
 
 ALTER TABLE ONLY public.evidences ALTER COLUMN id SET DEFAULT nextval('public.evidences_id_seq'::regclass);
 
+ALTER TABLE ONLY public.experiment_users ALTER COLUMN id SET DEFAULT nextval('public.experiment_users_id_seq'::regclass);
+
+ALTER TABLE ONLY public.experiments ALTER COLUMN id SET DEFAULT nextval('public.experiments_id_seq'::regclass);
+
 ALTER TABLE ONLY public.external_pull_requests ALTER COLUMN id SET DEFAULT nextval('public.external_pull_requests_id_seq'::regclass);
 
 ALTER TABLE ONLY public.feature_gates ALTER COLUMN id SET DEFAULT nextval('public.feature_gates_id_seq'::regclass);
@@ -17893,6 +17930,12 @@ ALTER TABLE ONLY public.events
 ALTER TABLE ONLY public.evidences
     ADD CONSTRAINT evidences_pkey PRIMARY KEY (id);
 
+ALTER TABLE ONLY public.experiment_users
+    ADD CONSTRAINT experiment_users_pkey PRIMARY KEY (id);
+
+ALTER TABLE ONLY public.experiments
+    ADD CONSTRAINT experiments_pkey PRIMARY KEY (id);
+
 ALTER TABLE ONLY public.external_pull_requests
     ADD CONSTRAINT external_pull_requests_pkey PRIMARY KEY (id);
 
@@ -19535,6 +19578,12 @@ CREATE UNIQUE INDEX index_events_on_target_type_and_target_id_and_fingerprint ON
 
 CREATE INDEX index_evidences_on_release_id ON public.evidences USING btree (release_id);
 
+CREATE INDEX index_experiment_users_on_experiment_id ON public.experiment_users USING btree (experiment_id);
+
+CREATE INDEX index_experiment_users_on_user_id ON public.experiment_users USING btree (user_id);
+
+CREATE UNIQUE INDEX index_experiments_on_name ON public.experiments USING btree (name);
+
 CREATE INDEX index_expired_and_not_notified_personal_access_tokens ON public.personal_access_tokens USING btree (id, expires_at) WHERE ((impersonation = false) AND (revoked = false) AND (expire_notification_delivered = false));
 
 CREATE UNIQUE INDEX index_external_pull_requests_on_project_and_branches ON public.external_pull_requests USING btree (project_id, source_branch, target_branch);
@@ -22250,6 +22299,9 @@ ALTER TABLE ONLY public.terraform_states
 ALTER TABLE ONLY public.group_deploy_keys
     ADD CONSTRAINT fk_rails_5682fc07f8 FOREIGN KEY (user_id) REFERENCES public.users(id) ON DELETE RESTRICT;
 
+ALTER TABLE ONLY public.experiment_users
+    ADD CONSTRAINT fk_rails_56d4708b4a FOREIGN KEY (experiment_id) REFERENCES public.experiments(id) ON DELETE CASCADE;
+
 ALTER TABLE ONLY public.issue_user_mentions
     ADD CONSTRAINT fk_rails_57581fda73 FOREIGN KEY (issue_id) REFERENCES public.issues(id) ON DELETE CASCADE;
 
@@ -23012,6 +23064,9 @@ ALTER TABLE ONLY public.ci_job_variables
 ALTER TABLE ONLY public.packages_nuget_metadata
     ADD CONSTRAINT fk_rails_fc0c19f5b4 FOREIGN KEY (package_id) REFERENCES public.packages_packages(id) ON DELETE CASCADE;
 
+ALTER TABLE ONLY public.experiment_users
+    ADD CONSTRAINT fk_rails_fd805f771a FOREIGN KEY (user_id) REFERENCES public.users(id) ON DELETE CASCADE;
+
 ALTER TABLE ONLY public.cluster_groups
     ADD CONSTRAINT fk_rails_fdb8648a96 FOREIGN KEY (cluster_id) REFERENCES public.clusters(id) ON DELETE CASCADE;
 

ee/spec/controllers/registrations_controller_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe RegistrationsController do
+  let_it_be(:user) { create(:user) }
+
   describe '#create' do
     context 'when the user opted-in' do
       let(:user_params) { { user: attributes_for(:user, email_opted_in: '1') } }
@@ -46,7 +48,7 @@ RSpec.describe RegistrationsController do
     subject { get :welcome }
 
     before do
-      sign_in(create(:user))
+      sign_in(user)
     end
 
     it 'renders the checkout layout' do
@@ -56,7 +58,7 @@ RSpec.describe RegistrationsController do
 
   describe '#update_registration' do
     before do
-      sign_in(create(:user))
+      sign_in(user)
     end
 
     subject(:update_registration) { patch :update_registration, params: { user: { role: 'software_developer', setup_for_company: 'false' } } }
@@ -87,10 +89,10 @@ RSpec.describe RegistrationsController do
       end
     end
 
-    describe 'tracking for the onboarding issues experiment' do
+    describe 'recording experiment user and track for the onboarding issues experiment' do
       using RSpec::Parameterized::TableSyntax
 
-      where(:on_gitlab_com, :experiment_enabled, :in_subscription_flow, :in_invitation_flow, :experiment_enabled_for_user, :expected_tracking) do
+      where(:on_gitlab_com, :experiment_enabled, :in_subscription_flow, :in_invitation_flow, :experiment_enabled_for_user, :experiment_group) do
         false | false | false | false | true  | nil
         false | false | false | true  | true  | nil
         false | false | true  | false | true  | nil
@@ -103,8 +105,8 @@ RSpec.describe RegistrationsController do
         true  | false | false | true  | true  | nil
         true  | false | true  | false | true  | nil
         true  | false | true  | true  | true  | nil
-        true  | true  | false | false | true  | 'experimental_group'
-        true  | true  | false | false | false | 'control_group'
+        true  | true  | false | false | true  | :experimental
+        true  | true  | false | false | false | :control
         true  | true  | false | true  | true  | nil
         true  | true  | true  | false | true  | nil
         true  | true  | true  | true  | true  | nil
@@ -119,13 +121,23 @@ RSpec.describe RegistrationsController do
           stub_experiment_for_user(onboarding_issues: experiment_enabled_for_user)
         end
 
-        it 'tracks when appropriate' do
-          if expected_tracking
+        it 'adds a user to experiments when appropriate' do
+          if experiment_group
+            expect(::Experiment).to receive(:add_user).with(:onboarding_issues, experiment_group, user)
+          else
+            expect(::Experiment).not_to receive(:add_user)
+          end
+
+          update_registration
+        end
+
+        it 'tracks a signed_up event when appropriate' do
+          if experiment_group
             expect(Gitlab::Tracking).to receive(:event).with(
               'Growth::Conversion::Experiment::OnboardingIssues',
               'signed_up',
               label: anything,
-              property: expected_tracking
+              property: "#{experiment_group}_group"
             )
           else
             expect(Gitlab::Tracking).not_to receive(:event)

lib/gitlab/experimentation.rb

@@ -62,6 +62,9 @@ module Gitlab
       }
     }.freeze
 
+    GROUP_CONTROL = :control
+    GROUP_EXPERIMENTAL = :experimental
+
     # Controller concern that checks if an `experimentation_subject_id cookie` is present and sets it if absent.
     # Used for A/B testing of experimental features. Exposes the `experiment_enabled?(experiment_name)` method
     # to controllers and views. It returns true when the experiment is enabled and the user is selected as part
@@ -106,6 +109,12 @@ module Gitlab
         end
       end
 
+      def record_experiment_user(experiment_key)
+        return unless Experimentation.enabled?(experiment_key) && current_user
+
+        ::Experiment.add_user(experiment_key, tracking_group(experiment_key), current_user)
+      end
+
       private
 
       def dnt_enabled?
@@ -132,7 +141,7 @@ module Gitlab
         {
           category: tracking_category(experiment_key),
           action: action,
-          property: tracking_group(experiment_key),
+          property: "#{tracking_group(experiment_key)}_group",
           label: experimentation_subject_id,
           value: value
         }.compact
@@ -145,7 +154,7 @@ module Gitlab
       def tracking_group(experiment_key)
         return unless Experimentation.enabled?(experiment_key)
 
-        experiment_enabled?(experiment_key) ? 'experimental_group' : 'control_group'
+        experiment_enabled?(experiment_key) ? GROUP_EXPERIMENTAL : GROUP_CONTROL
       end
 
       def forced_enabled?(experiment_key)

spec/factories/experiments.rb

+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :experiment do
+    name { generate(:title) }
+  end
+end

spec/lib/gitlab/experimentation_spec.rb

@@ -233,6 +233,68 @@ RSpec.describe Gitlab::Experimentation do
         end
       end
     end
+
+    describe '#record_experiment_user' do
+      let(:user) { build(:user) }
+
+      context 'when the experiment is enabled' do
+        before do
+          stub_experiment(test_experiment: true)
+          allow(controller).to receive(:current_user).and_return(user)
+        end
+
+        context 'the user is part of the experimental group' do
+          before do
+            stub_experiment_for_user(test_experiment: true)
+          end
+
+          it 'calls add_user on the Experiment model' do
+            expect(::Experiment).to receive(:add_user).with(:test_experiment, :experimental, user)
+
+            controller.record_experiment_user(:test_experiment)
+          end
+        end
+
+        context 'the user is part of the control group' do
+          before do
+            allow_next_instance_of(described_class) do |instance|
+              allow(instance).to receive(:experiment_enabled?).with(:test_experiment).and_return(false)
+            end
+          end
+
+          it 'calls add_user on the Experiment model' do
+            expect(::Experiment).to receive(:add_user).with(:test_experiment, :control, user)
+
+            controller.record_experiment_user(:test_experiment)
+          end
+        end
+      end
+
+      context 'when the experiment is disabled' do
+        before do
+          stub_experiment(test_experiment: false)
+          allow(controller).to receive(:current_user).and_return(user)
+        end
+
+        it 'does not call add_user on the Experiment model' do
+          expect(::Experiment).not_to receive(:add_user)
+
+          controller.record_experiment_user(:test_experiment)
+        end
+      end
+
+      context 'when there is no current_user' do
+        before do
+          stub_experiment(test_experiment: true)
+        end
+
+        it 'does not call add_user on the Experiment model' do
+          expect(::Experiment).not_to receive(:add_user)
+
+          controller.record_experiment_user(:test_experiment)
+        end
+      end
+    end
   end
 
   describe '.enabled?' do

spec/models/experiment_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Experiment do
+  subject { build(:experiment) }
+
+  describe 'associations' do
+    it { is_expected.to have_many(:experiment_users) }
+    it { is_expected.to have_many(:users) }
+    it { is_expected.to have_many(:control_group_users) }
+    it { is_expected.to have_many(:experimental_group_users) }
+
+    describe 'control_group_users and experimental_group_users' do
+      let(:experiment) { create(:experiment) }
+      let(:control_group_user) { build(:user) }
+      let(:experimental_group_user) { build(:user) }
+
+      before do
+        experiment.control_group_users << control_group_user
+        experiment.experimental_group_users << experimental_group_user
+      end
+
+      describe 'control_group_users' do
+        subject { experiment.control_group_users }
+
+        it { is_expected.to contain_exactly(control_group_user) }
+      end
+
+      describe 'experimental_group_users' do
+        subject { experiment.experimental_group_users }
+
+        it { is_expected.to contain_exactly(experimental_group_user) }
+      end
+    end
+  end
+
+  describe 'validations' do
+    it { is_expected.to validate_presence_of(:name) }
+    it { is_expected.to validate_uniqueness_of(:name) }
+    it { is_expected.to validate_length_of(:name).is_at_most(255) }
+  end
+
+  describe '.add_user' do
+    let(:name) { :experiment_key }
+    let(:user) { build(:user) }
+
+    let!(:experiment) { create(:experiment, name: name) }
+
+    subject { described_class.add_user(name, :control, user) }
+
+    describe 'creating a new experiment record' do
+      context 'an experiment with the provided name already exists' do
+        it 'does not create a new experiment record' do
+          expect { subject }.not_to change(Experiment, :count)
+        end
+      end
+
+      context 'an experiment with the provided name does not exist yet' do
+        let(:experiment) { nil }
+
+        it 'creates a new experiment record' do
+          expect { subject }.to change(Experiment, :count).by(1)
+        end
+      end
+    end
+
+    describe 'creating a new experiment_user record' do
+      context 'an experiment_user record for this experiment already exists' do
+        before do
+          subject
+        end
+
+        it 'does not create a new experiment_user record' do
+          expect { subject }.not_to change(ExperimentUser, :count)
+        end
+      end
+
+      context 'an experiment_user record for this experiment does not exist yet' do
+        it 'creates a new experiment_user record' do
+          expect { subject }.to change(ExperimentUser, :count).by(1)
+        end
+
+        it 'assigns the correct group_type to the experiment_user' do
+          expect { subject }.to change { experiment.control_group_users.count }.by(1)
+        end
+      end
+    end
+  end
+
+  describe '#add_control_user' do
+    let(:experiment) { create(:experiment) }
+    let(:user) { build(:user) }
+
+    subject { experiment.add_control_user(user) }
+
+    it 'creates a new experiment_user record and assigns the correct group_type' do
+      expect { subject }.to change { experiment.control_group_users.count }.by(1)
+    end
+  end
+
+  describe '#add_experimental_user' do
+    let(:experiment) { create(:experiment) }
+    let(:user) { build(:user) }
+
+    subject { experiment.add_experimental_user(user) }
+
+    it 'creates a new experiment_user record and assigns the correct group_type' do
+      expect { subject }.to change { experiment.experimental_group_users.count }.by(1)
+    end
+  end
+end

spec/models/experiment_user_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ExperimentUser do
+  describe 'Associations' do
+    it { is_expected.to belong_to(:experiment) }
+    it { is_expected.to belong_to(:user) }
+  end
+
+  describe 'Validations' do
+    it { is_expected.to validate_presence_of(:group_type) }
+  end
+end