Commit 6a695084 authored by Alex Buijs's avatar Alex Buijs

Implement reviewer feedback

parent 66f28b08
......@@ -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?(Namespace) || subject.is_a?(User) || subject.is_a?(Project)
return unless ExperimentSubject.valid_subject?(subject)
variant = :experimental if @variant_name != :control
......
......@@ -42,9 +42,9 @@ class Experiment < ApplicationRecord
end
def record_subject_and_variant!(subject, variant)
raise 'Incompatible subject provided!' unless subject.is_a?(Namespace) || subject.is_a?(User) || subject.is_a?(Project)
raise 'Incompatible subject provided!' unless ExperimentSubject.valid_subject?(subject)
attr_name = subject.is_a?(Namespace) ? :namespace : subject.class.name.downcase.to_sym
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
......
......@@ -14,6 +14,10 @@ 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
......
......@@ -273,7 +273,7 @@ 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
it 'saves the namespace as the experiment subject' do
expect(record_subject_and_variant!.namespace).to eq(subject_to_record)
end
end
......@@ -281,7 +281,7 @@ RSpec.describe Experiment do
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
it 'saves the namespace as the experiment_subject' do
expect(record_subject_and_variant!.namespace).to eq(subject_to_record)
end
end
......
......@@ -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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment