From f9fc878405d4c811f1abe54349f1b2f5532ee8f7 Mon Sep 17 00:00:00 2001
From: Robin Phung <robin.phung@clio.com>
Date: Thu, 7 May 2020 15:59:11 -0700
Subject: [PATCH 1/2] Introduce enable/disable experiment cohorting

---
 lib/split/dashboard.rb                  | 11 ++++++++
 lib/split/dashboard/public/dashboard.js | 10 +++++++
 lib/split/dashboard/public/style.css    |  5 ++++
 lib/split/dashboard/views/_controls.erb | 12 +++++++++
 lib/split/experiment.rb                 | 14 ++++++++++
 lib/split/trial.rb                      | 17 +++++++-----
 spec/dashboard_spec.rb                  | 22 +++++++++++++++
 spec/experiment_spec.rb                 | 36 ++++++++++++++++++++++++-
 spec/trial_spec.rb                      | 27 +++++++++++++++++++
 9 files changed, 147 insertions(+), 7 deletions(-)

diff --git a/lib/split/dashboard.rb b/lib/split/dashboard.rb
index b6257839..3c0f82e5 100755
--- a/lib/split/dashboard.rb
+++ b/lib/split/dashboard.rb
@@ -65,6 +65,17 @@ class Dashboard < Sinatra::Base
       redirect url('/')
     end
 
+    post '/update_cohorting' do
+      @experiment = Split::ExperimentCatalog.find(params[:experiment])
+      case params[:cohorting_action].downcase
+      when "enable"
+        @experiment.enable_cohorting
+      when "disable"
+        @experiment.disable_cohorting
+      end
+      redirect url('/')
+    end
+
     delete '/experiment' do
       @experiment = Split::ExperimentCatalog.find(params[:experiment])
       @experiment.delete
diff --git a/lib/split/dashboard/public/dashboard.js b/lib/split/dashboard/public/dashboard.js
index 0f779046..ebfa28c4 100644
--- a/lib/split/dashboard/public/dashboard.js
+++ b/lib/split/dashboard/public/dashboard.js
@@ -22,3 +22,13 @@ function confirmReopen() {
   var agree = confirm("This will reopen the experiment. Are you sure?");
   return agree ? true : false;
 }
+
+function confirmEnableCohorting(){
+  var agree = confirm("This will enable the cohorting of the experiment. Are you sure?");
+  return agree ? true : false;
+}
+
+function confirmDisableCohorting(){
+  var agree = confirm("This will disable the cohorting of the experiment. Note: Existing participants will continue to receive their alternative and may continue to convert. Are you sure?");
+  return agree ? true : false;
+}
diff --git a/lib/split/dashboard/public/style.css b/lib/split/dashboard/public/style.css
index cd7c5b1b..3c2c640e 100644
--- a/lib/split/dashboard/public/style.css
+++ b/lib/split/dashboard/public/style.css
@@ -326,3 +326,8 @@ a.button.green:focus, button.green:focus, input[type="submit"].green:focus {
   display: inline-block;
   padding: 5px;
 }
+
+.divider {
+  display: inline-block;
+  margin-left: 10px;
+}
diff --git a/lib/split/dashboard/views/_controls.erb b/lib/split/dashboard/views/_controls.erb
index 1ec32ded..e8e5ec45 100644
--- a/lib/split/dashboard/views/_controls.erb
+++ b/lib/split/dashboard/views/_controls.erb
@@ -1,3 +1,15 @@
+<% if experiment.cohorting_disabled? %>
+  <form action="<%= url "/update_cohorting?experiment=#{experiment.name}" %>" method='post' onclick="return confirmEnableCohorting()">
+    <input type="hidden" name="cohorting_action" value="enable">
+    <input type="submit" value="Enable Cohorting" class="green">
+  </form>
+<% else %>
+  <form action="<%= url "/update_cohorting?experiment=#{experiment.name}" %>" method='post' onclick="return confirmDisableCohorting()">
+    <input type="hidden" name="cohorting_action" value="disable">
+    <input type="submit" value="Disable Cohorting" class="red">
+  </form>
+<% end %>
+<span class="divider">|</span>
 <% if experiment.has_winner? %>
   <form action="<%= url "/reopen?experiment=#{experiment.name}" %>" method='post' onclick="return confirmReopen()">
     <input type="submit" value="Reopen Experiment">
diff --git a/lib/split/experiment.rb b/lib/split/experiment.rb
index 4c97bead..998c4903 100644
--- a/lib/split/experiment.rb
+++ b/lib/split/experiment.rb
@@ -235,6 +235,7 @@ def delete
       end
       reset_winner
       redis.srem(:experiments, name)
+      redis.hdel(experiment_config_key, :cohorting)
       remove_experiment_configuration
       Split.configuration.on_experiment_delete.call(self)
       increment_version
@@ -387,6 +388,19 @@ def jstring(goal = nil)
       js_id.gsub('/', '--')
     end
 
+    def cohorting_disabled?
+      value = redis.hget(experiment_config_key, :cohorting)
+      value.nil? ? false : value.downcase == "true"
+    end
+
+    def disable_cohorting
+      redis.hset(experiment_config_key, :cohorting, true)
+    end
+
+    def enable_cohorting
+      redis.hset(experiment_config_key, :cohorting, false)
+    end
+
     protected
 
     def experiment_config_key
diff --git a/lib/split/trial.rb b/lib/split/trial.rb
index 527a1b60..589738fa 100644
--- a/lib/split/trial.rb
+++ b/lib/split/trial.rb
@@ -53,6 +53,7 @@ def choose!(context = nil)
       # Only run the process once
       return alternative if @alternative_choosen
 
+      new_participant = @user[@experiment.key].nil?
       if override_is_alternative?
         self.alternative = @options[:override]
         if should_store_alternative? && !@user[@experiment.key]
@@ -70,19 +71,23 @@ def choose!(context = nil)
         else
           self.alternative = @user[@experiment.key]
           if alternative.nil?
-            self.alternative = @experiment.next_alternative
+            if @experiment.cohorting_disabled?
+              self.alternative = @experiment.control
+            else
+              self.alternative = @experiment.next_alternative
 
-            # Increment the number of participants since we are actually choosing a new alternative
-            self.alternative.increment_participation
+              # Increment the number of participants since we are actually choosing a new alternative
+              self.alternative.increment_participation
 
-            run_callback context, Split.configuration.on_trial_choose
+              run_callback context, Split.configuration.on_trial_choose
+            end
           end
         end
       end
 
-      @user[@experiment.key] = alternative.name if !@experiment.has_winner? && should_store_alternative?
+      @user[@experiment.key] = alternative.name unless @experiment.has_winner? || !should_store_alternative? || (new_participant && @experiment.cohorting_disabled?)
       @alternative_choosen = true
-      run_callback context, Split.configuration.on_trial unless @options[:disabled] || Split.configuration.disabled?
+      run_callback context, Split.configuration.on_trial unless @options[:disabled] || Split.configuration.disabled? || (new_participant && @experiment.cohorting_disabled?) 
       alternative
     end
 
diff --git a/spec/dashboard_spec.rb b/spec/dashboard_spec.rb
index b47de7bd..ce3fbe6c 100644
--- a/spec/dashboard_spec.rb
+++ b/spec/dashboard_spec.rb
@@ -161,6 +161,28 @@ def link(color)
     end
   end
 
+  describe "update cohorting" do
+    it "calls enable of cohorting when action is enable" do
+      post "/update_cohorting?experiment=#{experiment.name}", { "cohorting_action": "enable" }
+
+      expect(experiment.cohorting_disabled?).to eq false
+    end
+
+    it "calls disable of cohorting when action is disable" do
+      post "/update_cohorting?experiment=#{experiment.name}", { "cohorting_action": "disable" }
+      
+      expect(experiment.cohorting_disabled?).to eq true
+    end
+
+    it "calls neither enable or disable cohorting when passed invalid action" do
+      previous_value = experiment.cohorting_disabled?
+
+      post "/update_cohorting?experiment=#{experiment.name}", { "cohorting_action": "other" }
+
+      expect(experiment.cohorting_disabled?).to eq previous_value
+    end
+  end
+
   it "should reset an experiment" do
     red_link.participant_count = 5
     blue_link.participant_count = 7
diff --git a/spec/experiment_spec.rb b/spec/experiment_spec.rb
index c90f865d..bace4418 100644
--- a/spec/experiment_spec.rb
+++ b/spec/experiment_spec.rb
@@ -223,8 +223,14 @@ def alternative(color)
       experiment.delete
       expect(experiment.start_time).to be_nil
     end
-  end
 
+    it "should default cohorting back to false" do
+      experiment.disable_cohorting
+      expect(experiment.cohorting_disabled?).to eq(true)
+      experiment.delete
+      expect(experiment.cohorting_disabled?).to eq(false)
+    end
+  end
 
   describe 'winner' do
     it "should have no winner initially" do
@@ -392,6 +398,34 @@ def alternative(color)
     end
   end
 
+  describe "#cohorting_disabled?" do
+    it "returns false when nothing has been configured" do
+      expect(experiment.cohorting_disabled?).to eq false
+    end
+
+    it "returns true when enable_cohorting is performed" do
+      experiment.enable_cohorting
+      expect(experiment.cohorting_disabled?).to eq false
+    end
+
+    it "returns false when nothing has been configured" do
+      experiment.disable_cohorting
+      expect(experiment.cohorting_disabled?).to eq true
+    end
+  end
+
+  describe "#disable_cohorting" do
+    it "saves a new key in redis" do
+      expect(experiment.disable_cohorting).to eq true
+    end
+  end
+
+  describe "#enable_cohorting" do
+    it "saves a new key in redis" do
+      expect(experiment.enable_cohorting).to eq true
+    end
+  end
+
   describe 'changing an existing experiment' do
     def same_but_different_alternative
       Split::ExperimentCatalog.find_or_create('link_color', 'blue', 'yellow', 'orange')
diff --git a/spec/trial_spec.rb b/spec/trial_spec.rb
index b16164a7..5abc05bb 100644
--- a/spec/trial_spec.rb
+++ b/spec/trial_spec.rb
@@ -198,6 +198,33 @@ def expect_alternative(trial, alternative_name)
         expect(trial.alternative.name).to_not be_empty
         Split.configuration.on_trial_choose = nil
       end
+
+      it "assigns user to an alternative" do
+        trial.choose! context
+
+        expect(alternatives).to include(user[experiment.name])
+      end
+
+      context "when cohorting is disabled" do
+        before(:each) { allow(experiment).to receive(:cohorting_disabled?).and_return(true) }
+
+        it "picks the control and does not run on_trial callbacks" do
+          Split.configuration.on_trial = :on_trial_callback
+
+          expect(experiment).to_not receive(:next_alternative)
+          expect(context).not_to receive(:on_trial_callback)
+          expect_alternative(trial, 'basket')
+
+          Split.configuration.enabled = true
+          Split.configuration.on_trial = nil
+        end
+
+        it "user is not assigned an alternative" do
+          trial.choose! context
+
+          expect(user[experiment]).to eq(nil)
+        end
+      end
     end
   end
 

From 0dcc853e8d185d510bb7c3d57defe7c04f50d976 Mon Sep 17 00:00:00 2001
From: Robin Phung <robin.phung@clio.com>
Date: Wed, 27 May 2020 11:27:42 -0700
Subject: [PATCH 2/2] disable cohorting revision

---
 lib/split/dashboard/views/_controls.erb | 25 +++++++++++++------------
 lib/split/experiment.rb                 | 15 ++++++++++++---
 lib/split/trial.rb                      |  6 ++++--
 3 files changed, 29 insertions(+), 17 deletions(-)

diff --git a/lib/split/dashboard/views/_controls.erb b/lib/split/dashboard/views/_controls.erb
index e8e5ec45..2472b9ee 100644
--- a/lib/split/dashboard/views/_controls.erb
+++ b/lib/split/dashboard/views/_controls.erb
@@ -1,20 +1,21 @@
-<% if experiment.cohorting_disabled? %>
-  <form action="<%= url "/update_cohorting?experiment=#{experiment.name}" %>" method='post' onclick="return confirmEnableCohorting()">
-    <input type="hidden" name="cohorting_action" value="enable">
-    <input type="submit" value="Enable Cohorting" class="green">
-  </form>
-<% else %>
-  <form action="<%= url "/update_cohorting?experiment=#{experiment.name}" %>" method='post' onclick="return confirmDisableCohorting()">
-    <input type="hidden" name="cohorting_action" value="disable">
-    <input type="submit" value="Disable Cohorting" class="red">
-  </form>
-<% end %>
-<span class="divider">|</span>
 <% if experiment.has_winner? %>
   <form action="<%= url "/reopen?experiment=#{experiment.name}" %>" method='post' onclick="return confirmReopen()">
     <input type="submit" value="Reopen Experiment">
   </form>
+<% else %>
+  <% if experiment.cohorting_disabled? %>
+    <form action="<%= url "/update_cohorting?experiment=#{experiment.name}" %>" method='post' onclick="return confirmEnableCohorting()">
+      <input type="hidden" name="cohorting_action" value="enable">
+      <input type="submit" value="Enable Cohorting" class="green">
+    </form>
+  <% else %>
+    <form action="<%= url "/update_cohorting?experiment=#{experiment.name}" %>" method='post' onclick="return confirmDisableCohorting()">
+      <input type="hidden" name="cohorting_action" value="disable">
+      <input type="submit" value="Disable Cohorting" class="red">
+    </form>
+  <% end %>
 <% end %>
+<span class="divider">|</span>
 <% if experiment.start_time %>
   <form action="<%= url "/reset?experiment=#{experiment.name}" %>" method='post' onclick="return confirmReset()">
     <input type="submit" value="Reset Data">
diff --git a/lib/split/experiment.rb b/lib/split/experiment.rb
index 998c4903..e50d0d90 100644
--- a/lib/split/experiment.rb
+++ b/lib/split/experiment.rb
@@ -235,7 +235,7 @@ def delete
       end
       reset_winner
       redis.srem(:experiments, name)
-      redis.hdel(experiment_config_key, :cohorting)
+      remove_experiment_cohorting
       remove_experiment_configuration
       Split.configuration.on_experiment_delete.call(self)
       increment_version
@@ -389,15 +389,19 @@ def jstring(goal = nil)
     end
 
     def cohorting_disabled?
-      value = redis.hget(experiment_config_key, :cohorting)
-      value.nil? ? false : value.downcase == "true"
+      @cohorting_disabled ||= begin
+        value = redis.hget(experiment_config_key, :cohorting)
+        value.nil? ? false : value.downcase == "true"
+      end
     end
 
     def disable_cohorting
+      @cohorting_disabled = true
       redis.hset(experiment_config_key, :cohorting, true)
     end
 
     def enable_cohorting
+      @cohorting_disabled = false
       redis.hset(experiment_config_key, :cohorting, false)
     end
 
@@ -482,5 +486,10 @@ def experiment_configuration_has_changed?
     def goals_collection
       Split::GoalsCollection.new(@name, @goals)
     end
+
+    def remove_experiment_cohorting
+      @cohorting_disabled = false
+      redis.hdel(experiment_config_key, :cohorting)
+    end
   end
 end
diff --git a/lib/split/trial.rb b/lib/split/trial.rb
index 589738fa..cce7e347 100644
--- a/lib/split/trial.rb
+++ b/lib/split/trial.rb
@@ -85,9 +85,11 @@ def choose!(context = nil)
         end
       end
 
-      @user[@experiment.key] = alternative.name unless @experiment.has_winner? || !should_store_alternative? || (new_participant && @experiment.cohorting_disabled?)
+      new_participant_and_cohorting_disabled = new_participant && @experiment.cohorting_disabled?
+
+      @user[@experiment.key] = alternative.name unless @experiment.has_winner? || !should_store_alternative? || new_participant_and_cohorting_disabled
       @alternative_choosen = true
-      run_callback context, Split.configuration.on_trial unless @options[:disabled] || Split.configuration.disabled? || (new_participant && @experiment.cohorting_disabled?) 
+      run_callback context, Split.configuration.on_trial unless @options[:disabled] || Split.configuration.disabled? || new_participant_and_cohorting_disabled
       alternative
     end