Do not persist invalid extra_info on ab_record_extra_info.

If a invalid value is persisted on a given alternative, that dashboard is able to validate the data properly now. Added a few specs to ensure extra_info is persisted correctly. Co-authored-by: trostli <[email protected]>
splitrb · Sep 3, 2023 · 9d4c9b5 · 9d4c9b5
1 parent b401721
commit 9d4c9b5
Show file tree

Hide file tree

Showing 3 changed files with 39 additions and 2 deletions.
diff --git a/lib/split/dashboard/views/_experiment.erb b/lib/split/dashboard/views/_experiment.erb
@@ -16,7 +16,8 @@
   summary_texts = {}
   extra_columns.each do |column|
     extra_infos = experiment.alternatives.map(&:extra_info).select{|extra_info| extra_info && extra_info[column] }
-    if extra_infos[0][column].kind_of?(Numeric)
+
+    if extra_infos.length > 0 && extra_infos.all? { |extra_info| extra_info[column].kind_of?(Numeric) }
       summary_texts[column] = extra_infos.inject(0){|sum, extra_info| sum += extra_info[column]}
     else
       summary_texts[column] = "N/A"

diff --git a/lib/split/helper.rb b/lib/split/helper.rb
@@ -86,7 +86,7 @@ def ab_finished(metric_descriptor, options = { reset: true })
     end
 
     def ab_record_extra_info(metric_descriptor, key, value = 1)
-      return if exclude_visitor? || Split.configuration.disabled?
+      return if exclude_visitor? || Split.configuration.disabled? || value.nil?
       metric_descriptor, _ = normalize_metric(metric_descriptor)
       experiments = Metric.possible_experiments(metric_descriptor)
 

diff --git a/spec/helper_spec.rb b/spec/helper_spec.rb
@@ -558,6 +558,42 @@ def should_finish_experiment(experiment_name, should_finish = true)
     end
   end
 
+
+  describe "ab_record_extra_info" do
+    context "for an experiment that the user participates in" do
+      before(:each) do
+        @experiment_name = "link_color"
+        @alternatives = ["blue", "red"]
+        @experiment = Split::ExperimentCatalog.find_or_create(@experiment_name, *@alternatives)
+        @alternative_name = ab_test(@experiment_name, *@alternatives)
+      end
+
+      it "records extra data for a given experiment" do
+        alternative = Split::Alternative.new(@alternative_name, "link_color")
+
+        ab_record_extra_info(@experiment_name, "some_data", 10)
+
+        expect(alternative.extra_info).to eql({ "some_data" => 10 })
+      end
+
+      it "records extra data for a given experiment" do
+        alternative = Split::Alternative.new(@alternative_name, "link_color")
+
+        ab_record_extra_info(@experiment_name, "some_data")
+
+        expect(alternative.extra_info).to eql({ "some_data" => 1 })
+      end
+
+      it "records extra data for a given experiment" do
+        alternative = Split::Alternative.new(@alternative_name, "link_color")
+
+        ab_record_extra_info(@experiment_name, "some_data", nil)
+
+        expect(alternative.extra_info).to eql({})
+      end
+    end
+  end
+
   describe "conversions" do
     it "should return a conversion rate for an alternative" do
       alternative_name = ab_test("link_color", "blue", "red")