Merge pull request #373 from andreibondarev/auto_finish

Clean up experiments that are gone, stopped or a winner has been chosen

Author: andrew

README.md

@@ -165,6 +165,8 @@ finished(:experiment_name, reset: false)
 
 The user will then always see the alternative they started with.
 
+Any old unfinished experiment key will be deleted from the user's data storage if the experiment had been removed or is over and a winner had been chosen. This allows a user to enroll into any new experiment in cases when the `allow_multiple_experiments` config option is set to `false`.
+
 ### Multiple experiments at once
 
 By default Split will avoid users participating in multiple experiments at once. This means you are less likely to skew results by adding in more variation to your tests.

lib/split.rb

@@ -12,6 +12,7 @@
    persistence
    encapsulated_helper
    trial
+   user
    version
    zscore].each do |f|
   require "split/#{f}"

lib/split/encapsulated_helper.rb

@@ -22,7 +22,7 @@ def initialize(context)
       end
 
       def ab_user
-        @ab_user ||= Split::Persistence.adapter.new(@context)
+        @ab_user ||= Split::User.new(@context)
       end
     end
 

lib/split/helper.rb

@@ -6,7 +6,6 @@ module Helper
     def ab_test(metric_descriptor, control = nil, *alternatives)
       begin
         experiment = ExperimentCatalog.find_or_initialize(metric_descriptor, control, *alternatives)
-
         alternative = if Split.configuration.enabled
           experiment.save
           trial = Trial.new(:user => ab_user, :experiment => experiment,
@@ -94,7 +93,7 @@ def begin_experiment(experiment, alternative_name = nil)
     end
 
     def ab_user
-      @ab_user ||= Split::Persistence.adapter.new(self)
+      @ab_user ||= User.new(self)
     end
 
     def exclude_visitor?

lib/split/trial.rb

@@ -50,6 +50,7 @@ def complete!(goals=[], context = nil)
     # method is guaranteed to only run once, and will skip the alternative choosing process if run
     # a second time.
     def choose!(context = nil)
+      @user.cleanup_old_experiments
       # Only run the process once
       return alternative if @alternative_choosen
 

lib/split/user.rb

@@ -0,0 +1,24 @@
+module Split
+  class User
+    extend Forwardable
+    def_delegators :@user, :keys, :[], :[]=, :delete
+    attr_reader :user
+
+    def initialize(context)
+      @user = Split::Persistence.adapter.new(context)
+    end
+
+    def cleanup_old_experiments
+      user.keys.each do |key|
+        experiment = ExperimentCatalog.find key_without_version(key)
+        if experiment.nil? || experiment.has_winner? || experiment.start_time.nil?
+          user.delete key
+        end
+      end
+    end
+
+    def key_without_version(key)
+      key.split(/\:\d(?!\:)/)[0]
+    end
+  end
+end

spec/encapsulated_helper_spec.rb

@@ -6,7 +6,7 @@
 
   before do
     allow_any_instance_of(Split::EncapsulatedHelper::ContextShim).to receive(:ab_user)
-        .and_return({})
+        .and_return(mock_user)
   end
 
   def params

spec/helper_spec.rb

@@ -172,7 +172,7 @@
     it "should only let a user participate in one experiment at a time" do
       link_color = ab_test('link_color', 'blue', 'red')
       ab_test('button_size', 'small', 'big')
-      expect(ab_user).to eq({'link_color' => link_color})
+      expect(ab_user['link_color']).to eq(link_color)
       big = Split::Alternative.new('big', 'button_size')
       expect(big.participant_count).to eq(0)
       small = Split::Alternative.new('small', 'button_size')
@@ -185,17 +185,18 @@
       end
       link_color = ab_test('link_color', 'blue', 'red')
       button_size = ab_test('button_size', 'small', 'big')
-      expect(ab_user).to eq({'link_color' => link_color, 'button_size' => button_size})
+      expect(ab_user['link_color']).to eq(link_color)
+      expect(ab_user['button_size']).to eq(button_size)
       button_size_alt = Split::Alternative.new(button_size, 'button_size')
       expect(button_size_alt.participant_count).to eq(1)
     end
 
     it "should not over-write a finished key when an experiment is on a later version" do
       experiment.increment_version
       ab_user = { experiment.key => 'blue', experiment.finished_key => true }
-      finshed_session = ab_user.dup
+      finished_session = ab_user.dup
       ab_test('link_color', 'blue', 'red')
-      expect(ab_user).to eq(finshed_session)
+      expect(ab_user).to eq(finished_session)
     end
   end
 
@@ -247,7 +248,8 @@
 
     it "should set experiment's finished key if reset is false" do
       finished(@experiment_name, {:reset => false})
-      expect(ab_user).to eq(@experiment.key => @alternative_name, @experiment.finished_key => true)
+      expect(ab_user[@experiment.key]).to eq(@alternative_name)
+      expect(ab_user[@experiment.finished_key]).to eq(true)
     end
 
     it 'should not increment the counter if reset is false and the experiment has been already finished' do
@@ -279,30 +281,31 @@
     end
 
     it "should clear out the user's participation from their session" do
-      expect(ab_user).to eq(@experiment.key => @alternative_name)
+      expect(ab_user[@experiment.key]).to eq(@alternative_name)
       finished(@experiment_name)
-      expect(ab_user).to eq({})
+      expect(ab_user.keys).to be_empty
     end
 
     it "should not clear out the users session if reset is false" do
-      expect(ab_user).to eq(@experiment.key => @alternative_name)
+      expect(ab_user[@experiment.key]).to eq(@alternative_name)
       finished(@experiment_name, {:reset => false})
-      expect(ab_user).to eq(@experiment.key => @alternative_name, @experiment.finished_key => true)
+      expect(ab_user[@experiment.key]).to eq(@alternative_name)
+      expect(ab_user[@experiment.finished_key]).to eq(true)
     end
 
     it "should reset the users session when experiment is not versioned" do
-      expect(ab_user).to eq(@experiment.key => @alternative_name)
+      expect(ab_user[@experiment.key]).to eq(@alternative_name)
       finished(@experiment_name)
-      expect(ab_user).to eq({})
+      expect(ab_user.keys).to be_empty
     end
 
     it "should reset the users session when experiment is versioned" do
       @experiment.increment_version
       @alternative_name = ab_test(@experiment_name, *@alternatives)
 
-      expect(ab_user).to eq(@experiment.key => @alternative_name)
+      expect(ab_user[@experiment.key]).to eq(@alternative_name)
       finished(@experiment_name)
-      expect(ab_user).to eq({})
+      expect(ab_user.keys).to be_empty
     end
 
     it "should do nothing where the experiment was not started by this user" do
@@ -337,7 +340,8 @@
       experiment = Split::ExperimentCatalog.find :my_experiment
 
       finished :my_experiment
-      expect(ab_user).to eq(experiment.key => alternative, experiment.finished_key => true)
+      expect(ab_user[experiment.key]).to eq(alternative)
+      expect(ab_user[experiment.finished_key]).to eq(true)
     end
   end
 
@@ -618,28 +622,28 @@ def should_finish_experiment(experiment_name, should_finish=true)
     it "should use version zero if no version is present" do
       alternative_name = ab_test('link_color', 'blue', 'red')
       expect(experiment.version).to eq(0)
-      expect(ab_user).to eq({'link_color' => alternative_name})
+      expect(ab_user['link_color']).to eq(alternative_name)
     end
 
     it "should save the version of the experiment to the session" do
       experiment.reset
       expect(experiment.version).to eq(1)
       alternative_name = ab_test('link_color', 'blue', 'red')
-      expect(ab_user).to eq({'link_color:1' => alternative_name})
+      expect(ab_user['link_color:1']).to eq(alternative_name)
     end
 
     it "should load the experiment even if the version is not 0" do
       experiment.reset
       expect(experiment.version).to eq(1)
       alternative_name = ab_test('link_color', 'blue', 'red')
-      expect(ab_user).to eq({'link_color:1' => alternative_name})
+      expect(ab_user['link_color:1']).to eq(alternative_name)
       return_alternative_name = ab_test('link_color', 'blue', 'red')
       expect(return_alternative_name).to eq(alternative_name)
     end
 
     it "should reset the session of a user on an older version of the experiment" do
       alternative_name = ab_test('link_color', 'blue', 'red')
-      expect(ab_user).to eq({'link_color' => alternative_name})
+      expect(ab_user['link_color']).to eq(alternative_name)
       alternative = Split::Alternative.new(alternative_name, 'link_color')
       expect(alternative.participant_count).to eq(1)
 
@@ -656,7 +660,7 @@ def should_finish_experiment(experiment_name, should_finish=true)
 
     it "should cleanup old versions of experiments from the session" do
       alternative_name = ab_test('link_color', 'blue', 'red')
-      expect(ab_user).to eq({'link_color' => alternative_name})
+      expect(ab_user['link_color']).to eq(alternative_name)
       alternative = Split::Alternative.new(alternative_name, 'link_color')
       expect(alternative.participant_count).to eq(1)
 
@@ -666,12 +670,12 @@ def should_finish_experiment(experiment_name, should_finish=true)
       expect(alternative.participant_count).to eq(0)
 
       new_alternative_name = ab_test('link_color', 'blue', 'red')
-      expect(ab_user).to eq({'link_color:1' => new_alternative_name})
+      expect(ab_user['link_color:1']).to eq(new_alternative_name)
     end
 
     it "should only count completion of users on the current version" do
       alternative_name = ab_test('link_color', 'blue', 'red')
-      expect(ab_user).to eq({'link_color' => alternative_name})
+      expect(ab_user['link_color']).to eq(alternative_name)
       alternative = Split::Alternative.new(alternative_name, 'link_color')
 
       experiment.reset

spec/spec_helper.rb

@@ -19,11 +19,15 @@
   config.before(:each) do
     Split.configuration = Split::Configuration.new
     Split.redis.flushall
-    @ab_user = {}
+    @ab_user = mock_user
     params = nil
   end
 end
 
+def mock_user
+  Split::User.new(double(session: {}))
+end
+
 def session
   @session ||= {}
 end

spec/trial_spec.rb

@@ -3,7 +3,7 @@
 require 'split/trial'
 
 describe Split::Trial do
-  let(:user) { Split::Persistence.adapter.new(double(session: {})) }
+  let(:user) { mock_user }
   let(:experiment) do
     Split::Experiment.new('basket_text', :alternatives => ['basket', 'cart']).save
   end