Merge branch 'dreedy-check-should_track-every-time-we-call-publish_to_client' into 'master'

Move extra publisher checks to their respective methods

See merge request gitlab-org/gitlab!67366

app/experiments/application_experiment.rb

@@ -12,17 +12,22 @@ class ApplicationExperiment < Gitlab::Experiment # rubocop:disable Gitlab/Namesp
   def publish(_result = nil)
     super
 
-    publish_to_client if should_track? # publish the experiment data to the client
-    publish_to_database if @record # publish the experiment context to the database
+    publish_to_client
+    publish_to_database
   end
 
   def publish_to_client
+    return unless should_track?
+
     Gon.push({ experiment: { name => signature } }, true)
   rescue NoMethodError
     # means we're not in the request cycle, and can't add to Gon. Log a warning maybe?
   end
 
   def publish_to_database
+    return unless @record
+    return unless should_track?
+
     # if the context contains a namespace, group, project, user, or actor
     value = context.value
     subject = value[:namespace] || value[:group] || value[:project] || value[:user] || value[:actor]

spec/experiments/application_experiment_spec.rb

@@ -57,35 +57,42 @@ RSpec.describe ApplicationExperiment, :experiment do
   end
 
   describe "#publish" do
-    it "doesn't track or publish to the client or database if we can't track", :snowplow do
-      allow(subject).to receive(:should_track?).and_return(false)
+    let(:should_track) { true }
 
-      expect(subject).not_to receive(:publish_to_client)
-      expect(subject).not_to receive(:publish_to_database)
+    before do
+      allow(subject).to receive(:should_track?).and_return(should_track)
+    end
 
+    it "tracks the assignment", :snowplow do
       subject.publish
 
-      expect_no_snowplow_event
+      expect_snowplow_event(
+        category: 'namespaced/stub',
+        action: 'assignment',
+        context: [{ schema: anything, data: anything }]
+      )
     end
 
-    it "tracks the assignment" do
-      expect(subject).to receive(:track).with(:assignment)
+    it "publishes to the client" do
+      expect(subject).to receive(:publish_to_client)
 
       subject.publish
     end
 
-    it "publishes the to the client" do
-      expect(subject).to receive(:publish_to_client)
+    it "publishes to the database if we've opted for that" do
+      expect(subject).to receive(:publish_to_database)
 
       subject.publish
     end
 
-    it "publishes to the database if we've opted for that" do
-      subject.record!
-
-      expect(subject).to receive(:publish_to_database)
+    context 'when we should not track' do
+      let(:should_track) { false }
 
+      it 'does not track an event to Snowplow', :snowplow do
         subject.publish
+
+        expect_no_snowplow_event
+      end
     end
 
     describe "#publish_to_client" do
@@ -101,17 +108,39 @@ RSpec.describe ApplicationExperiment, :experiment do
 
         expect { subject.publish_to_client }.not_to raise_error
       end
+
+      context 'when we should not track' do
+        let(:should_track) { false }
+
+        it 'returns early' do
+          expect(Gon).not_to receive(:push)
+
+          subject.publish_to_client
+        end
+      end
     end
 
-    describe "#publish_to_database" do
+    describe '#publish_to_database' do
+      shared_examples 'does not record to the database' do
+        it 'does not create an experiment record' do
+          expect { subject.publish_to_database }.not_to change(Experiment, :count)
+        end
+
+        it 'does not create an experiment subject record' do
+          expect { subject.publish_to_database }.not_to change(ExperimentSubject, :count)
+        end
+      end
+
+      context 'when we explicitly request to record' do
         using RSpec::Parameterized::TableSyntax
-      let(:context) { { context_key => context_value }}
 
         before do
           subject.record!
         end
 
-      context "when there's a usable subject" do
+        context 'when there is a usable subject' do
+          let(:context) { { context_key => context_value } }
+
           where(:context_key, :context_value, :object_type) do
             :namespace | build(:namespace) | :namespace
             :group     | build(:namespace) | :namespace
@@ -121,7 +150,7 @@ RSpec.describe ApplicationExperiment, :experiment do
           end
 
           with_them do
-          it "creates an experiment and experiment subject record" do
+            it 'creates an experiment and experiment subject record' do
               expect { subject.publish_to_database }.to change(Experiment, :count).by(1)
 
               expect(Experiment.last.name).to eq('namespaced/stub')
@@ -130,21 +159,28 @@ RSpec.describe ApplicationExperiment, :experiment do
           end
         end
 
-      context "when there's not a usable subject" do
+        context 'when there is not a usable subject' do
+          let(:context) { { context_key => context_value } }
+
           where(:context_key, :context_value) do
             :namespace | nil
             :foo       | :bar
           end
 
           with_them do
-          it "doesn't create an experiment record" do
-            expect { subject.publish_to_database }.not_to change(Experiment, :count)
+            include_examples 'does not record to the database'
+          end
         end
 
-          it "doesn't create an experiment subject record" do
-            expect { subject.publish_to_database }.not_to change(ExperimentSubject, :count)
+        context 'but we should not track' do
+          let(:should_track) { false }
+
+          include_examples 'does not record to the database'
         end
       end
+
+      context 'when we have not explicitly requested to record' do
+        include_examples 'does not record to the database'
       end
     end
   end