Merge pull request #615 from robin-phung/master

Introduce enable/disable experiment cohorting

Author: andrehjr

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

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;
+}

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;
+}

lib/split/dashboard/views/_controls.erb

@@ -2,7 +2,20 @@
   <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">

lib/split/experiment.rb

@@ -235,6 +235,7 @@ def delete
       end
       reset_winner
       redis.srem(:experiments, name)
+      remove_experiment_cohorting
       remove_experiment_configuration
       Split.configuration.on_experiment_delete.call(self)
       increment_version
@@ -387,6 +388,23 @@ def jstring(goal = nil)
       js_id.gsub('/', '--')
     end
 
+    def cohorting_disabled?
+      @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
+
     protected
 
     def experiment_config_key
@@ -468,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

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,25 @@ 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?
+      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?
+      run_callback context, Split.configuration.on_trial unless @options[:disabled] || Split.configuration.disabled? || new_participant_and_cohorting_disabled
       alternative
     end
 

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

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')

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