From 2cc424917ba8e9ec3c2263dceb84d517f6bbbfd7 Mon Sep 17 00:00:00 2001
From: Nicholas Firth-McCoy <nicholas@2suggestions.com.au>
Date: Tue, 1 Sep 2015 16:04:36 +1000
Subject: [PATCH] Fix caching of winning alternative to prevent recalculation
 each time the dashboard is loaded

It looks like when this feature was added, the calculation of winning
alternatives was meant to take place only once per day.

The #calc_winning_alternatives method was never called, which was meant
to be saving the experiment's last calc_time. Update the experiment view
to call this method instead of the #estimate_winning_alternative method
directly. Fix caching so that the #calc_time= method is called, rather
than assigning to a local variable. Update calc_time so that number of
days since epoch is stored, rather than the day of month (1-31). Ensure
we're comparing integer values, rather than the string value Redis
returns from #hget.
---
 lib/split/dashboard/views/_experiment.erb |  4 +---
 lib/split/experiment.rb                   | 23 ++++++++++++++---------
 spec/experiment_spec.rb                   |  7 +++++++
 3 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/lib/split/dashboard/views/_experiment.erb b/lib/split/dashboard/views/_experiment.erb
index d8e93f83..6b45f8a8 100644
--- a/lib/split/dashboard/views/_experiment.erb
+++ b/lib/split/dashboard/views/_experiment.erb
@@ -4,9 +4,7 @@
   <% experiment_class = "experiment" %>
 <% end %>
 
-<% unless experiment.calc_time == Time.now.day %>
-  <% experiment.estimate_winning_alternative %>
-<% end %>
+<% experiment.calc_winning_alternatives %>
 
 <div class="<%= experiment_class %>">
   <div class="experiment-header">
diff --git a/lib/split/experiment.rb b/lib/split/experiment.rb
index fbd24271..acf6f653 100644
--- a/lib/split/experiment.rb
+++ b/lib/split/experiment.rb
@@ -274,17 +274,22 @@ def load_from_redis
     end
 
     def calc_winning_alternatives
-      if goals.empty?
-        self.estimate_winning_alternative
-      else
-        goals.each do |goal|
-          self.estimate_winning_alternative(goal)
+      # Super simple cache so that we only recalculate winning alternatives once per day
+      days_since_epoch = Time.now.utc.to_i / 86400
+
+      if self.calc_time != days_since_epoch
+        if goals.empty?
+          self.estimate_winning_alternative
+        else
+          goals.each do |goal|
+            self.estimate_winning_alternative(goal)
+          end
         end
-      end
 
-      calc_time = Time.now.day
+        self.calc_time = days_since_epoch
 
-      self.save
+        self.save
+      end
     end
 
     def estimate_winning_alternative(goal = nil)
@@ -390,7 +395,7 @@ def calc_time=(time)
     end
 
     def calc_time
-      Split.redis.hget(experiment_config_key, :calc_time)
+      Split.redis.hget(experiment_config_key, :calc_time).to_i
     end
 
     def jstring(goal = nil)
diff --git a/spec/experiment_spec.rb b/spec/experiment_spec.rb
index 226d98db..1bdcdc6a 100644
--- a/spec/experiment_spec.rb
+++ b/spec/experiment_spec.rb
@@ -428,6 +428,13 @@ def same_but_different_goals
       p_goal2 = alt.p_winner(goal2)
       expect(p_goal1).not_to be_within(0.04).of(p_goal2)
     end
+
+    it "should return nil and not re-calculate probabilities if they have already been calculated today" do
+      experiment = Split::ExperimentCatalog.find_or_create({'link_color3' => ["purchase", "refund"]}, 'blue', 'red', 'green')
+      experiment_calc_time = Time.now.utc.to_i / 86400
+      experiment.calc_time = experiment_calc_time
+      expect(experiment.calc_winning_alternatives).to be nil
+    end
   end
 
 end