Merge branch 'refactor-experiment-subjects-namespaces' into 'master'

Rename experiment_subjects group_id column

See merge request gitlab-org/gitlab!62696

app/experiments/application_experiment.rb

@@ -58,7 +58,7 @@ class ApplicationExperiment < Gitlab::Experiment # rubocop:disable Gitlab/Namesp
 
   def record_experiment
     subject = context.value[:namespace] || context.value[:group] || context.value[:project] || context.value[:user] || context.value[:actor]
-    return unless subject.is_a?(Group) || subject.is_a?(User) || subject.is_a?(Project)
+    return unless ExperimentSubject.valid_subject?(subject)
 
     variant = :experimental if @variant_name != :control
 

app/models/experiment.rb

@@ -42,10 +42,10 @@ class Experiment < ApplicationRecord
   end
 
   def record_subject_and_variant!(subject, variant)
-    subject = subject.owner if subject.is_a?(Namespace) && subject.user?
-    raise 'Incompatible subject provided!' unless subject.is_a?(Group) || subject.is_a?(User) || subject.is_a?(Project)
+    raise 'Incompatible subject provided!' unless ExperimentSubject.valid_subject?(subject)
 
-    experiment_subject = experiment_subjects.find_or_initialize_by(subject.class.name.downcase => subject)
+    attr_name = subject.class.table_name.singularize.to_sym
+    experiment_subject = experiment_subjects.find_or_initialize_by(attr_name => subject)
     experiment_subject.assign_attributes(variant: variant)
     # We only call save when necessary because this causes the request to stick to the primary DB
     # even when the save is a no-op

app/models/experiment_subject.rb

@@ -5,7 +5,7 @@ class ExperimentSubject < ApplicationRecord
 
   belongs_to :experiment, inverse_of: :experiment_subjects
   belongs_to :user
-  belongs_to :group
+  belongs_to :namespace
   belongs_to :project
 
   validates :experiment, presence: true
@@ -14,15 +14,19 @@ class ExperimentSubject < ApplicationRecord
 
   enum variant: { GROUP_CONTROL => 0, GROUP_EXPERIMENTAL => 1 }
 
+  def self.valid_subject?(subject)
+    subject.is_a?(Namespace) || subject.is_a?(User) || subject.is_a?(Project)
+  end
+
   private
 
   def must_have_one_subject_present
     if non_nil_subjects.length != 1
-      errors.add(:base, s_("ExperimentSubject|Must have exactly one of User, Group, or Project."))
+      errors.add(:base, s_("ExperimentSubject|Must have exactly one of User, Namespace, or Project."))
     end
   end
 
   def non_nil_subjects
-    @non_nil_subjects ||= [user, group, project].reject(&:blank?)
+    @non_nil_subjects ||= [user, namespace, project].reject(&:blank?)
   end
 end

db/migrate/20210527130524_rename_experiment_subjects_group_id_to_namespace_id.rb

+# frozen_string_literal: true
+
+class RenameExperimentSubjectsGroupIdToNamespaceId < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers::V2
+
+  disable_ddl_transaction!
+
+  def up
+    rename_column_concurrently :experiment_subjects, :group_id, :namespace_id
+  end
+
+  def down
+    undo_rename_column_concurrently :experiment_subjects, :group_id, :namespace_id
+  end
+end

db/post_migrate/20210527131039_clean_up_rename_experiment_subjects_group_id_to_namespace_id.rb

+# frozen_string_literal: true
+
+class CleanUpRenameExperimentSubjectsGroupIdToNamespaceId < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers::V2
+
+  disable_ddl_transaction!
+
+  def up
+    cleanup_concurrent_column_rename :experiment_subjects, :group_id, :namespace_id
+  end
+
+  def down
+    undo_cleanup_concurrent_column_rename :experiment_subjects, :group_id, :namespace_id
+  end
+end

db/schema_migrations/20210527130524

+c0d6252fc768a431513754f7d51e61c5127f5573fefb278e7e1673dcd9e1b097
\ No newline at end of file

db/schema_migrations/20210527131039

+c07ebd06892bacc936514798d970eb58ed08b6570049d2de07f787e93b5b3316
\ No newline at end of file

db/structure.sql

@@ -12761,14 +12761,14 @@ CREATE TABLE experiment_subjects (
     id bigint NOT NULL,
     experiment_id bigint NOT NULL,
     user_id bigint,
-    group_id bigint,
     project_id bigint,
     variant smallint DEFAULT 0 NOT NULL,
     created_at timestamp with time zone NOT NULL,
     updated_at timestamp with time zone NOT NULL,
     converted_at timestamp with time zone,
     context jsonb DEFAULT '{}'::jsonb NOT NULL,
-    CONSTRAINT chk_has_one_subject CHECK ((num_nonnulls(user_id, group_id, project_id) = 1))
+    namespace_id bigint,
+    CONSTRAINT check_f6411bc4b5 CHECK ((num_nonnulls(user_id, namespace_id, project_id) = 1))
 );
 
 CREATE SEQUENCE experiment_subjects_id_seq
@@ -23230,7 +23230,7 @@ CREATE INDEX index_evidences_on_release_id ON evidences USING btree (release_id)
 
 CREATE INDEX index_experiment_subjects_on_experiment_id ON experiment_subjects USING btree (experiment_id);
 
-CREATE INDEX index_experiment_subjects_on_group_id ON experiment_subjects USING btree (group_id);
+CREATE INDEX index_experiment_subjects_on_namespace_id ON experiment_subjects USING btree (namespace_id);
 
 CREATE INDEX index_experiment_subjects_on_project_id ON experiment_subjects USING btree (project_id);
 
@@ -25655,6 +25655,9 @@ ALTER TABLE ONLY import_export_uploads
 ALTER TABLE ONLY push_rules
     ADD CONSTRAINT fk_83b29894de FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE;
 
+ALTER TABLE ONLY experiment_subjects
+    ADD CONSTRAINT fk_842649f2f5 FOREIGN KEY (namespace_id) REFERENCES namespaces(id) ON DELETE CASCADE;
+
 ALTER TABLE ONLY merge_request_diffs
     ADD CONSTRAINT fk_8483f3258f FOREIGN KEY (merge_request_id) REFERENCES merge_requests(id) ON DELETE CASCADE;
 
@@ -25670,9 +25673,6 @@ ALTER TABLE ONLY packages_package_files
 ALTER TABLE ONLY ci_builds
     ADD CONSTRAINT fk_87f4cefcda FOREIGN KEY (upstream_pipeline_id) REFERENCES ci_pipelines(id) ON DELETE CASCADE;
 
-ALTER TABLE ONLY experiment_subjects
-    ADD CONSTRAINT fk_88489af1b1 FOREIGN KEY (group_id) REFERENCES namespaces(id) ON DELETE CASCADE;
-
 ALTER TABLE ONLY vulnerabilities
     ADD CONSTRAINT fk_88b4d546ef FOREIGN KEY (start_date_sourcing_milestone_id) REFERENCES milestones(id) ON DELETE SET NULL;
 

doc/development/experiment_guide/gitlab_experiment.md

@@ -398,7 +398,7 @@ No recommendations are provided here at this time.
 
 Snowplow tracking of identifiable users or groups is prohibited, but you can still
 determine if an experiment is successful or not. We're allowed to record the ID of
-a group, project or user in our database. Therefore, we can tell the experiment
+a namespace, project or user in our database. Therefore, we can tell the experiment
 to record their ID together with the assigned experiment variant in the
 `experiment_subjects` database table for later analysis.
 

locale/gitlab.pot

@@ -13308,7 +13308,7 @@ msgstr ""
 msgid "Experienced"
 msgstr ""
 
-msgid "ExperimentSubject|Must have exactly one of User, Group, or Project."
+msgid "ExperimentSubject|Must have exactly one of User, Namespace, or Project."
 msgstr ""
 
 msgid "Expiration"

spec/experiments/application_experiment_spec.rb

@@ -127,8 +127,8 @@ RSpec.describe ApplicationExperiment, :experiment do
 
     context 'when providing a compatible context' do
       where(:context_key, :object_type) do
-        :namespace | :group
-        :group     | :group
+        :namespace | :namespace
+        :group     | :namespace
         :project   | :project
         :user      | :user
         :actor     | :user

spec/models/experiment_spec.rb

@@ -251,7 +251,7 @@ RSpec.describe Experiment do
 
     context 'when an existing experiment_subject exists for the given subject' do
       let_it_be(:experiment_subject) do
-        create(:experiment_subject, experiment: experiment, group: subject_to_record, user: nil, variant: :experimental)
+        create(:experiment_subject, experiment: experiment, namespace: subject_to_record, user: nil, variant: :experimental)
       end
 
       context 'when it belongs to the same variant' do
@@ -273,16 +273,16 @@ RSpec.describe Experiment do
 
     describe 'providing a subject to record' do
       context 'when given a group as subject' do
-        it 'saves the namespace owner as experiment_subject user' do
-          expect(record_subject_and_variant!.group).to eq(subject_to_record)
+        it 'saves the namespace as the experiment subject' do
+          expect(record_subject_and_variant!.namespace).to eq(subject_to_record)
         end
       end
 
       context 'when given a users namespace as subject' do
         let_it_be(:subject_to_record) { build(:namespace) }
 
-        it 'saves the namespace owner as experiment_subject user' do
-          expect(record_subject_and_variant!.user).to eq(subject_to_record.owner)
+        it 'saves the namespace as the experiment_subject' do
+          expect(record_subject_and_variant!.namespace).to eq(subject_to_record)
         end
       end
 

spec/models/experiment_subject_spec.rb

@@ -6,7 +6,7 @@ RSpec.describe ExperimentSubject, type: :model do
   describe 'associations' do
     it { is_expected.to belong_to(:experiment) }
     it { is_expected.to belong_to(:user) }
-    it { is_expected.to belong_to(:group) }
+    it { is_expected.to belong_to(:namespace) }
     it { is_expected.to belong_to(:project) }
   end
 
@@ -14,8 +14,8 @@ RSpec.describe ExperimentSubject, type: :model do
     it { is_expected.to validate_presence_of(:experiment) }
 
     describe 'must_have_one_subject_present' do
-      let(:experiment_subject) { build(:experiment_subject, user: nil, group: nil, project: nil) }
-      let(:error_message) { 'Must have exactly one of User, Group, or Project.' }
+      let(:experiment_subject) { build(:experiment_subject, user: nil, namespace: nil, project: nil) }
+      let(:error_message) { 'Must have exactly one of User, Namespace, or Project.' }
 
       it 'fails when no subject is present' do
         expect(experiment_subject).not_to be_valid
@@ -27,8 +27,8 @@ RSpec.describe ExperimentSubject, type: :model do
         expect(experiment_subject).to be_valid
       end
 
-      it 'passes when group subject is present' do
-        experiment_subject.group = build(:group)
+      it 'passes when namespace subject is present' do
+        experiment_subject.namespace = build(:group)
         expect(experiment_subject).to be_valid
       end
 
@@ -40,7 +40,7 @@ RSpec.describe ExperimentSubject, type: :model do
       it 'fails when more than one subject is present', :aggregate_failures do
         # two subjects
         experiment_subject.user = build(:user)
-        experiment_subject.group = build(:group)
+        experiment_subject.namespace = build(:group)
         expect(experiment_subject).not_to be_valid
         expect(experiment_subject.errors[:base]).to include(error_message)
 
@@ -51,4 +51,22 @@ RSpec.describe ExperimentSubject, type: :model do
       end
     end
   end
+
+  describe '.valid_subject?' do
+    subject(:valid_subject?) { described_class.valid_subject?(subject_class.new) }
+
+    context 'when passing a Group, Namespace, User or Project' do
+      [Group, Namespace, User, Project].each do |subject_class|
+        let(:subject_class) { subject_class }
+
+        it { is_expected.to be(true) }
+      end
+    end
+
+    context 'when passing another object' do
+      let(:subject_class) { Issue }
+
+      it { is_expected.to be(false) }
+    end
+  end
 end