Do not track when experiment is disabled

An event should not be tracked if an experiment is not
enabled. It should be tracked when a user is not part
of the experimental group.

app/assets/javascripts/pages/sessions/new/index.js

@@ -21,9 +21,11 @@ document.addEventListener('DOMContentLoaded', () => {
   // redirected to sign-in after attempting to access a protected URL that included a fragment.
   preserveUrlFragment(window.location.hash);
 
-  const tab = document.querySelector(".new-session-tabs a[href='#register-pane']");
-  const { category, action, ...data } = gon.tracking_data;
-  tab.addEventListener('click', () => {
-    Tracking.event(category, action, data);
-  });
+  if (gon.tracking_data) {
+    const tab = document.querySelector(".new-session-tabs a[href='#register-pane']");
+    const { category, action, ...data } = gon.tracking_data;
+    tab.addEventListener('click', () => {
+      Tracking.event(category, action, data);
+    });
+  }
 });

lib/gitlab/experimentation.rb

@@ -21,7 +21,8 @@ module Gitlab
 
     # 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.
+    # to controllers and views. It returns true when the experiment is enabled and the user is selected as part
+    # of the experimental group.
     #
     module ControllerConcern
       extend ActiveSupport::Concern
@@ -42,15 +43,19 @@ module Gitlab
       end
 
       def experiment_enabled?(experiment_key)
-        Experimentation.enabled?(experiment_key, experimentation_subject_index)
+        Experimentation.enabled_for_user?(experiment_key, experimentation_subject_index)
       end
 
       def track_experiment_event(experiment_key, action)
+        return unless Experimentation.enabled?(experiment_key)
+
         tracking_data = experimentation_tracking_data(experiment_key, action)
         ::Gitlab::Tracking.event(tracking_data.delete(:category), tracking_data.delete(:action), tracking_data)
       end
 
       def frontend_experimentation_tracking_data(experiment_key, action)
+        return unless Experimentation.enabled?(experiment_key)
+
         tracking_data = experimentation_tracking_data(experiment_key, action)
         gon.push(tracking_data: tracking_data)
       end
@@ -81,6 +86,8 @@ module Gitlab
       end
 
       def tracking_group(experiment_key)
+        return unless Experimentation.enabled?(experiment_key)
+
         experiment_enabled?(experiment_key) ? 'experimental_group' : 'control_group'
       end
     end
@@ -90,14 +97,16 @@ module Gitlab
         Experiment.new(EXPERIMENTS[key].merge(key: key))
       end
 
-      def enabled?(experiment_key, experimentation_subject_index)
+      def enabled?(experiment_key)
         return false unless EXPERIMENTS.key?(experiment_key)
 
         experiment = experiment(experiment_key)
+        experiment.feature_toggle_enabled? && experiment.enabled_for_environment?
+      end
 
-        experiment.feature_toggle_enabled? &&
-          experiment.enabled_for_environment? &&
-          experiment.enabled_for_experimentation_subject?(experimentation_subject_index)
+      def enabled_for_user?(experiment_key, experimentation_subject_index)
+        enabled?(experiment_key) &&
+          experiment(experiment_key).enabled_for_experimentation_subject?(experimentation_subject_index)
       end
     end
 

spec/controllers/application_controller_spec.rb

@@ -852,7 +852,7 @@ describe ApplicationController do
     let(:experiment_enabled) { true }
 
     before do
-      stub_experiment(signup_flow: experiment_enabled)
+      stub_experiment_for_user(signup_flow: experiment_enabled)
     end
 
     context 'experiment enabled and user with required role' do

spec/controllers/registrations_controller_spec.rb

@@ -12,9 +12,10 @@ describe RegistrationsController do
   describe '#new' do
     subject { get :new }
 
-    context 'with the experimental signup flow enabled' do
+    context 'with the experimental signup flow enabled and the user is part of the experimental group' do
       before do
         stub_experiment(signup_flow: true)
+        stub_experiment_for_user(signup_flow: true)
       end
 
       it 'tracks the event with the right parameters' do
@@ -33,9 +34,10 @@ describe RegistrationsController do
       end
     end
 
-    context 'with the experimental signup flow disabled' do
+    context 'with the experimental signup flow enabled and the user is part of the control group' do
       before do
-        stub_experiment(signup_flow: false)
+        stub_experiment(signup_flow: true)
+        stub_experiment_for_user(signup_flow: false)
       end
 
       it 'does not track the event' do
@@ -258,30 +260,34 @@ describe RegistrationsController do
       end
     end
 
-    context 'with the experimental signup flow disabled' do
-      before do
-        stub_experiment(signup_flow: false)
-      end
+    describe 'tracking data' do
+      context 'with the experimental signup flow enabled and the user is part of the control group' do
+        before do
+          stub_experiment(signup_flow: true)
+          stub_experiment_for_user(signup_flow: false)
+        end
 
-      it 'tracks the event with the right parameters' do
-        expect(Gitlab::Tracking).to receive(:event).with(
-          'Growth::Acquisition::Experiment::SignUpFlow',
-          'end',
-          label: anything,
-          property: 'control_group'
-        )
-        post :create, params: user_params
+        it 'tracks the event with the right parameters' do
+          expect(Gitlab::Tracking).to receive(:event).with(
+            'Growth::Acquisition::Experiment::SignUpFlow',
+            'end',
+            label: anything,
+            property: 'control_group'
+          )
+          post :create, params: user_params
+        end
       end
-    end
 
-    context 'with the experimental signup flow enabled' do
-      before do
-        stub_experiment(signup_flow: true)
-      end
+      context 'with the experimental signup flow enabled and the user is part of the experimental group' do
+        before do
+          stub_experiment(signup_flow: true)
+          stub_experiment_for_user(signup_flow: true)
+        end
 
-      it 'does not track the event' do
-        expect(Gitlab::Tracking).not_to receive(:event)
-        post :create, params: user_params
+        it 'does not track the event' do
+          expect(Gitlab::Tracking).not_to receive(:event)
+          post :create, params: user_params
+        end
       end
     end
 
@@ -376,6 +382,7 @@ describe RegistrationsController do
   describe '#update_role' do
     before do
       stub_experiment(signup_flow: true)
+      stub_experiment_for_user(signup_flow: true)
       sign_in(create(:user))
     end
 

spec/controllers/sessions_controller_spec.rb

@@ -36,9 +36,9 @@ describe SessionsController do
     end
 
     describe 'tracking data' do
-      context 'with the experimental signup flow enabled' do
+      context 'when the user is part of the experimental group' do
         before do
-          stub_experiment(signup_flow: true)
+          stub_experiment_for_user(signup_flow: true)
         end
 
         it 'doesn\'t pass tracking parameters to the frontend' do
@@ -47,9 +47,10 @@ describe SessionsController do
         end
       end
 
-      context 'with the experimental signup flow disabled' do
+      context 'with the experimental signup flow enabled and the user is part of the control group' do
         before do
-          stub_experiment(signup_flow: false)
+          stub_experiment(signup_flow: true)
+          stub_experiment_for_user(signup_flow: false)
           allow_any_instance_of(described_class).to receive(:experimentation_subject_id).and_return('uuid')
         end
 

spec/features/users/signup_spec.rb

@@ -413,6 +413,7 @@ end
 describe 'With original flow' do
   before do
     stub_experiment(signup_flow: false)
+    stub_experiment_for_user(signup_flow: false)
   end
 
   it_behaves_like 'Signup'
@@ -421,6 +422,7 @@ end
 describe 'With experimental flow' do
   before do
     stub_experiment(signup_flow: true)
+    stub_experiment_for_user(signup_flow: true)
   end
 
   it_behaves_like 'Signup'

spec/lib/gitlab/experimentation_spec.rb

@@ -53,8 +53,8 @@ describe Gitlab::Experimentation do
 
     describe '#experiment_enabled?' do
       context 'cookie is not present' do
-        it 'calls Gitlab::Experimentation.enabled? with the name of the experiment and an experimentation_subject_index of nil' do
-          expect(Gitlab::Experimentation).to receive(:enabled?).with(:test_experiment, nil) # rubocop:disable RSpec/DescribedClass
+        it 'calls Gitlab::Experimentation.enabled_for_user? with the name of the experiment and an experimentation_subject_index of nil' do
+          expect(Gitlab::Experimentation).to receive(:enabled_for_user?).with(:test_experiment, nil) # rubocop:disable RSpec/DescribedClass
           controller.experiment_enabled?(:test_experiment)
         end
       end
@@ -65,91 +65,123 @@ describe Gitlab::Experimentation do
           get :index
         end
 
-        it 'calls Gitlab::Experimentation.enabled? with the name of the experiment and an experimentation_subject_index of the modulo 100 of the hex value of the uuid' do
+        it 'calls Gitlab::Experimentation.enabled_for_user? with the name of the experiment and an experimentation_subject_index of the modulo 100 of the hex value of the uuid' do
           # 'abcd1234'.hex % 100 = 76
-          expect(Gitlab::Experimentation).to receive(:enabled?).with(:test_experiment, 76) # rubocop:disable RSpec/DescribedClass
+          expect(Gitlab::Experimentation).to receive(:enabled_for_user?).with(:test_experiment, 76) # rubocop:disable RSpec/DescribedClass
           controller.experiment_enabled?(:test_experiment)
         end
       end
     end
 
     describe '#track_experiment_event' do
-      context 'part of the experimental group' do
+      context 'when the experiment is enabled' do
         before do
-          allow_any_instance_of(described_class).to receive(:experiment_enabled?).with(:test_experiment).and_return(true)
+          stub_experiment(test_experiment: true)
         end
 
-        it 'tracks the event with the right parameters' do
-          expect(Gitlab::Tracking).to receive(:event).with(
-            'Team',
-            'start',
-            label: nil,
-            property: 'experimental_group'
-          )
-          controller.track_experiment_event(:test_experiment, 'start')
+        context 'the user is part of the experimental group' do
+          before do
+            stub_experiment_for_user(test_experiment: true)
+          end
+
+          it 'tracks the event with the right parameters' do
+            expect(Gitlab::Tracking).to receive(:event).with(
+              'Team',
+              'start',
+              label: nil,
+              property: 'experimental_group'
+            )
+            controller.track_experiment_event(:test_experiment, 'start')
+          end
+        end
+
+        context 'the user is part of the control group' do
+          before do
+            stub_experiment_for_user(test_experiment: false)
+          end
+
+          it 'tracks the event with the right parameters' do
+            expect(Gitlab::Tracking).to receive(:event).with(
+              'Team',
+              'start',
+              label: nil,
+              property: 'control_group'
+            )
+            controller.track_experiment_event(:test_experiment, 'start')
+          end
         end
       end
 
-      context 'part of the control group' do
+      context 'when the experiment is disabled' do
         before do
-          allow_any_instance_of(described_class).to receive(:experiment_enabled?).with(:test_experiment).and_return(false)
+          stub_experiment(test_experiment: false)
         end
 
-        it 'tracks the event with the right parameters' do
-          expect(Gitlab::Tracking).to receive(:event).with(
-            'Team',
-            'start',
-            label: nil,
-            property: 'control_group'
-          )
+        it 'does not track the event' do
+          expect(Gitlab::Tracking).not_to receive(:event)
           controller.track_experiment_event(:test_experiment, 'start')
         end
       end
     end
 
     describe '#frontend_experimentation_tracking_data' do
-      context 'part of the experimental group' do
+      context 'when the experiment is enabled' do
         before do
-          allow_any_instance_of(described_class).to receive(:experiment_enabled?).with(:test_experiment).and_return(true)
+          stub_experiment(test_experiment: true)
         end
 
-        it 'pushes the right parameters to gon' do
-          controller.frontend_experimentation_tracking_data(:test_experiment, 'start')
-          expect(Gon.tracking_data).to eq(
-            {
-              category: 'Team',
-              action: 'start',
-              label: nil,
-              property: 'experimental_group'
-            }
-          )
+        context 'the user is part of the experimental group' do
+          before do
+            stub_experiment_for_user(test_experiment: true)
+          end
+
+          it 'pushes the right parameters to gon' do
+            controller.frontend_experimentation_tracking_data(:test_experiment, 'start')
+            expect(Gon.tracking_data).to eq(
+              {
+                category: 'Team',
+                action: 'start',
+                label: nil,
+                property: 'experimental_group'
+              }
+            )
+          end
+        end
+
+        context 'the user is part of the control group' do
+          before do
+            allow_any_instance_of(described_class).to receive(:experiment_enabled?).with(:test_experiment).and_return(false)
+          end
+
+          it 'pushes the right parameters to gon' do
+            controller.frontend_experimentation_tracking_data(:test_experiment, 'start')
+            expect(Gon.tracking_data).to eq(
+              {
+                category: 'Team',
+                action: 'start',
+                label: nil,
+                property: 'control_group'
+              }
+            )
+          end
         end
       end
 
-      context 'part of the control group' do
+      context 'when the experiment is disabled' do
         before do
-          allow_any_instance_of(described_class).to receive(:experiment_enabled?).with(:test_experiment).and_return(false)
+          stub_experiment(test_experiment: false)
         end
 
-        it 'pushes the right parameters to gon' do
-          controller.frontend_experimentation_tracking_data(:test_experiment, 'start')
-          expect(Gon.tracking_data).to eq(
-            {
-              category: 'Team',
-              action: 'start',
-              label: nil,
-              property: 'control_group'
-            }
-          )
+        it 'does not push data to gon' do
+          expect(Gon.method_defined?(:tracking_data)).to be_falsey
+          controller.track_experiment_event(:test_experiment, 'start')
         end
       end
     end
   end
 
   describe '.enabled?' do
-    subject { described_class.enabled?(:test_experiment, experimentation_subject_index) }
-
-    let(:experimentation_subject_index) { 9 }
+    subject { described_class.enabled?(:test_experiment) }
 
     context 'feature toggle is enabled, we are on the right environment and we are selected' do
       it { is_expected.to be_truthy }
@@ -157,7 +189,7 @@ describe Gitlab::Experimentation do
 
     describe 'experiment is not defined' do
       it 'returns false' do
-        expect(described_class.enabled?(:missing_experiment, experimentation_subject_index)).to be_falsey
+        expect(described_class.enabled?(:missing_experiment)).to be_falsey
       end
     end
 
@@ -200,30 +232,52 @@ describe Gitlab::Experimentation do
         it { is_expected.to be_falsey }
       end
     end
+  end
 
-    describe 'enabled ratio' do
-      context 'enabled ratio is not set' do
-        let(:enabled_ratio) { nil }
+  describe '.enabled_for_user?' do
+    subject { described_class.enabled_for_user?(:test_experiment, experimentation_subject_index) }
 
-        it { is_expected.to be_falsey }
+    let(:experimentation_subject_index) { 9 }
+
+    context 'experiment is disabled' do
+      before do
+        allow(described_class).to receive(:enabled?).and_return(false)
       end
 
-      context 'experimentation_subject_index is not set' do
-        let(:experimentation_subject_index) { nil }
+      it { is_expected.to be_falsey }
+    end
 
-        it { is_expected.to be_falsey }
+    context 'experiment is enabled' do
+      before do
+        allow(described_class).to receive(:enabled?).and_return(true)
       end
 
-      context 'experimentation_subject_index is an empty string' do
-        let(:experimentation_subject_index) { '' }
+      it { is_expected.to be_truthy }
+
+      context 'enabled ratio is not set' do
+        let(:enabled_ratio) { nil }
 
         it { is_expected.to be_falsey }
       end
 
-      context 'experimentation_subject_index outside enabled ratio' do
-        let(:experimentation_subject_index) { 11 }
+      describe 'experimentation_subject_index' do
+        context 'experimentation_subject_index is not set' do
+          let(:experimentation_subject_index) { nil }
 
-        it { is_expected.to be_falsey }
+          it { is_expected.to be_falsey }
+        end
+
+        context 'experimentation_subject_index is an empty string' do
+          let(:experimentation_subject_index) { '' }
+
+          it { is_expected.to be_falsey }
+        end
+
+        context 'experimentation_subject_index outside enabled ratio' do
+          let(:experimentation_subject_index) { 11 }
+
+          it { is_expected.to be_falsey }
+        end
       end
     end
   end

spec/requests/api/users_spec.rb

@@ -636,7 +636,7 @@ describe API::Users do
   describe "GET /users/sign_up" do
     context 'when experimental signup_flow is active' do
       before do
-        stub_experiment(signup_flow: true)
+        stub_experiment_for_user(signup_flow: true)
       end
 
       it "shows sign up page" do
@@ -648,7 +648,7 @@ describe API::Users do
 
     context 'when experimental signup_flow is not active' do
       before do
-        stub_experiment(signup_flow: false)
+        stub_experiment_for_user(signup_flow: false)
       end
 
       it "redirects to sign in page" do

spec/support/helpers/stub_experiments.rb

@@ -9,7 +9,19 @@ module StubExperiments
   # - `stub_experiment(signup_flow: false)` ... Disable `signup_flow` experiment globally.
   def stub_experiment(experiments)
     experiments.each do |experiment_key, enabled|
-      allow(Gitlab::Experimentation).to receive(:enabled?).with(experiment_key, any_args) { enabled }
+      allow(Gitlab::Experimentation).to receive(:enabled?).with(experiment_key) { enabled }
+    end
+  end
+
+  # Stub Experiment for user with `key: true/false`
+  #
+  # @param [Hash] experiment where key is feature name and value is boolean whether enabled or not.
+  #
+  # Examples
+  # - `stub_experiment_for_user(signup_flow: false)` ... Disable `signup_flow` experiment for user.
+  def stub_experiment_for_user(experiments)
+    experiments.each do |experiment_key, enabled|
+      allow(Gitlab::Experimentation).to receive(:enabled_for_user?).with(experiment_key, anything) { enabled }
     end
   end
 end