Move experimentation-related method calls into view to avoid side-effect

Move the `record_experiment_group` and `experiment_enabled?` helper
method calls into the view to avoid the side-effect of recording an
experiment subject as part of a single, consolidated helper method.

ee/app/helpers/trial_status_widget_helper.rb

 # frozen_string_literal: true
 
 module TrialStatusWidgetHelper
-  def billing_plans_and_trials_available?
-    ::Gitlab::CurrentSettings.should_check_namespace_plan?
-  end
-
-  def eligible_for_trial_status_widget?(group)
-    group.trial_active? && can?(current_user, :admin_namespace, group)
-  end
-
   def plan_title_for_group(group)
     group.gitlab_subscription&.plan_title
   end
 
   def show_trial_status_widget?(group)
-    return false unless billing_plans_and_trials_available?
-
-    eligible_for_trial_status_widget?(group)
+    billing_plans_and_trials_available? && eligible_for_trial_status_widget?(group)
   end
 
-  # Note: This method has a side-effect in that it records the given group as a
-  # participant in the experiment (if the experiment is active at all) in the
-  # `experiment_subjects` table.
-  def trial_status_widget_experiment_enabled?(group)
-    experiment_key = :show_trial_status_in_sidebar
+  private
 
-    # Record the top-level group as a Growth::Conversion experiment participant
-    record_experiment_group(experiment_key, group)
+  def billing_plans_and_trials_available?
+    ::Gitlab::CurrentSettings.should_check_namespace_plan?
+  end
 
-    experiment_enabled?(experiment_key, subject: group)
+  def eligible_for_trial_status_widget?(group)
+    group.trial_active? && can?(current_user, :admin_namespace, group)
   end
 end

ee/app/views/layouts/nav/sidebar/_group_trial_status_widget.html.haml

 -# Only top-level groups can have trials & plans
 - root_group = group.root_ancestor
 - return unless show_trial_status_widget?(root_group)
-- return unless trial_status_widget_experiment_enabled?(root_group)
+
+-# For the current Growth::Conversion experiment
+- experiment_key = :show_trial_status_in_sidebar
+- record_experiment_group(experiment_key, root_group)
+- return unless experiment_enabled?(experiment_key, subject: root_group)
 
 = nav_link do
   #js-trial-status-widget{ data: { container_id: 'trial-status-sidebar-widget',

ee/spec/helpers/trial_status_widget_helper_spec.rb

@@ -3,54 +3,9 @@
 require 'spec_helper'
 
 RSpec.describe TrialStatusWidgetHelper do
-  using RSpec::Parameterized::TableSyntax
-
-  describe '#billing_plans_and_trials_available?' do
-    before do
-      stub_application_setting(check_namespace_plan: trials_available)
-    end
-
-    subject { helper.billing_plans_and_trials_available? }
-
-    context 'when the check_namespace_plan ApplicationSetting is enabled' do
-      let(:trials_available) { true }
-
-      it { is_expected.to be_truthy }
-    end
-
-    context 'when the check_namespace_plan ApplicationSetting is disabled' do
-      let(:trials_available) { false }
-
-      it { is_expected.to be_falsey }
-    end
-  end
-
-  describe '#eligible_for_trial_status_widget?' do
-    let(:user) { instance_double(User) }
-    let(:group) { instance_double(Group, trial_active?: trial_active) }
-    let(:user_can_admin_group) { true }
-
-    before do
-      allow(helper).to receive(:current_user).and_return(user)
-      allow(helper).to receive(:can?).and_call_original
-      allow(helper).to receive(:can?).with(user, :admin_namespace, group).and_return(user_can_admin_group)
-    end
-
-    subject { helper.eligible_for_trial_status_widget?(group) }
-
-    where :trial_active, :user_can_admin_group, :expected_result do
-      true  | true  | true
-      true  | false | false
-      false | true  | false
-      false | false | false
-    end
-
-    with_them do
-      it { is_expected.to eq(expected_result) }
-    end
-  end
-
   describe '#plan_title_for_group' do
+    using RSpec::Parameterized::TableSyntax
+
     let_it_be(:group) { create(:group) }
 
     subject { helper.plan_title_for_group(group) }
@@ -71,60 +26,26 @@ RSpec.describe TrialStatusWidgetHelper do
   end
 
   describe '#show_trial_status_widget?' do
-    let(:group) { instance_double(Group) }
+    let(:user) { instance_double(User) }
+    let(:group) { instance_double(Group, trial_active?: trial_active) }
 
     before do
-      allow(helper).to receive(:billing_plans_and_trials_available?).and_return(trials_available)
-      allow(helper).to receive(:eligible_for_trial_status_widget?).with(group).and_return(eligible_for_widget)
+      stub_application_setting(check_namespace_plan: trials_available)
+      allow(helper).to receive(:current_user).and_return(user)
+      allow(helper).to receive(:can?).and_call_original
+      allow(helper).to receive(:can?).with(user, :admin_namespace, group).and_return(user_can_admin_group)
     end
 
     subject { helper.show_trial_status_widget?(group) }
 
-    where(:trials_available, :eligible_for_widget, :result) do
-      true  | true  | true
-      true  | false | false
-      false | true  | false
-      false | false | false
-    end
+    where(
+      trials_available: [true, false],
+      trial_active: [true, false],
+      user_can_admin_group: [true, false]
+    )
 
     with_them do
-      it { is_expected.to eq(result) }
-    end
-  end
-
-  describe '#trial_status_widget_experiment_enabled?' do
-    let(:experiment_key) { :show_trial_status_in_sidebar }
-    let(:group) { instance_double(Group) }
-
-    before do
-      allow(helper).to receive(:experiment_enabled?).with(experiment_key, subject: group).and_return(experiment_enabled)
-      allow(helper).to receive(:record_experiment_group)
-    end
-
-    subject { helper.trial_status_widget_experiment_enabled?(group) }
-
-    context 'when the experiment is not enabled for the given group' do
-      let(:experiment_enabled) { false }
-
-      it { is_expected.to be_falsey }
-
-      it 'records the group as an experiment participant' do
-        expect(helper).to receive(:record_experiment_group).with(experiment_key, group)
-
-        subject
-      end
-    end
-
-    context 'when the experiment is enabled for the given group' do
-      let(:experiment_enabled) { true }
-
-      it { is_expected.to be_truthy }
-
-      it 'records the group as an experiment participant' do
-        expect(helper).to receive(:record_experiment_group).with(experiment_key, group)
-
-        subject
-      end
+      it { is_expected.to eq(trials_available && trial_active && user_can_admin_group) }
     end
   end
 end

ee/spec/views/layouts/nav/sidebar/_group.html.haml_spec.rb

@@ -16,16 +16,20 @@ RSpec.describe 'layouts/nav/sidebar/_group' do
 
     let!(:gitlab_subscription) { create(:gitlab_subscription, :active_trial, namespace: group) }
 
+    let(:experiment_key) { :show_trial_status_in_sidebar }
     let(:show_widget) { false }
     let(:experiment_enabled) { false }
 
     before do
       allow(view).to receive(:show_trial_status_widget?).and_return(show_widget)
-      allow(view).to receive(:trial_status_widget_experiment_enabled?).and_return(experiment_enabled)
-      render
+      allow(view).to receive(:experiment_enabled?).with(experiment_key, subject: group).and_return(experiment_enabled)
+      allow(view).to receive(:record_experiment_group)
     end
 
-    subject { rendered }
+    subject do
+      render
+      rendered
+    end
 
     shared_examples 'does not render' do
       it 'does not render' do
@@ -48,15 +52,31 @@ RSpec.describe 'layouts/nav/sidebar/_group' do
       end
     end
 
+    shared_examples 'does record experiment subject' do
+      it 'records the group as an experiment subject' do
+        expect(view).to receive(:record_experiment_group).with(experiment_key, group)
+        subject
+      end
+    end
+
+    shared_examples 'does not record experiment subject' do
+      it 'does not record the group as an experiment subject' do
+        expect(view).not_to receive(:record_experiment_group)
+        subject
+      end
+    end
+
     where :show_widget, :experiment_enabled, :examples_to_include do
-      true  | true  | 'does render'
-      true  | false | 'does not render'
-      false | true  | 'does not render'
-      false | false | 'does not render'
+      true  | true  | ['does record experiment subject', 'does render']
+      true  | false | ['does record experiment subject', 'does not render']
+      false | true  | ['does not record experiment subject', 'does not render']
+      false | false | ['does not record experiment subject', 'does not render']
     end
 
     with_them do
-      include_examples(params[:examples_to_include])
+      params[:examples_to_include].each do |example_set|
+        include_examples(example_set)
+      end
     end
   end
 

spec/lib/gitlab/experimentation/controller_concern_spec.rb

@@ -523,7 +523,6 @@ RSpec.describe Gitlab::Experimentation::ControllerConcern, type: :controller do
   describe '#record_experiment_group' do
     let(:group) { 'a group object' }
     let(:experiment_key) { :some_experiment_key }
-
     let(:dnt_enabled) { false }
     let(:experiment_active) { true }
     let(:rollout_strategy) { :whatever }