Merge pull request #78 from dimko/master

Do not increment the experiment's counter if reset is false and user has already finished it

lib/split/experiment.rb

@@ -75,6 +75,10 @@ def key
       end
     end
 
+    def finished_key
+      "#{key}:finished"
+    end
+
     def reset
       alternatives.each(&:reset)
       reset_winner

lib/split/helper.rb

@@ -25,10 +25,17 @@ def ab_test(experiment_name, control, *alternatives)
     def finished(experiment_name, options = {:reset => true})
       return if exclude_visitor? or !Split.configuration.enabled
       return unless (experiment = Split::Experiment.find(experiment_name))
+      return if !options[:reset] && ab_user[experiment.finished_key]
+
       if alternative_name = ab_user[experiment.key]
         alternative = Split::Alternative.new(alternative_name, experiment_name)
         alternative.increment_completion
-        ab_user.delete(experiment.key) if options[:reset]
+
+        if options[:reset]
+          ab_user.delete(experiment.key)
+        else
+          ab_user[experiment.finished_key] = true
+        end
       end
     rescue => e
       raise unless Split.configuration.db_failover

spec/helper_spec.rb

@@ -99,61 +99,55 @@
   end
 
   describe 'finished' do
-    it 'should increment the counter for the completed alternative' do
-      experiment = Split::Experiment.find_or_create('link_color', 'blue', 'red')
-      alternative_name = ab_test('link_color', 'blue', 'red')
-
-      previous_completion_count = Split::Alternative.new(alternative_name, 'link_color').completed_count
+    before(:each) do
+      @experiment_name = 'link_color'
+      @alternatives = ['blue', 'red']
+      @experiment = Split::Experiment.find_or_create(@experiment_name, *@alternatives)
+      @alternative_name = ab_test(@experiment_name, *@alternatives)
+      @previous_completion_count = Split::Alternative.new(@alternative_name, @experiment_name).completed_count
+    end
 
-      finished('link_color')
+    it 'should increment the counter for the completed alternative' do
+      finished(@experiment_name)
+      new_completion_count = Split::Alternative.new(@alternative_name, @experiment_name).completed_count
+      new_completion_count.should eql(@previous_completion_count + 1)
+    end
 
-      new_completion_count = Split::Alternative.new(alternative_name, 'link_color').completed_count
+    it "should set experiment's finished key if reset is false" do
+      finished(@experiment_name, :reset => false)
+      session[:split].should eql(@experiment.key => @alternative_name, @experiment.finished_key => true)
+    end
 
-      new_completion_count.should eql(previous_completion_count + 1)
+    it 'should not increment the counter if reset is false and the experiment has been already finished' do
+      2.times { finished(@experiment_name, :reset => false) }
+      new_completion_count = Split::Alternative.new(@alternative_name, @experiment_name).completed_count
+      new_completion_count.should eql(@previous_completion_count + 1)
     end
 
     it "should clear out the user's participation from their session" do
-      experiment = Split::Experiment.find_or_create('link_color', 'blue', 'red')
-      alternative_name = ab_test('link_color', 'blue', 'red')
-
-      previous_completion_count = Split::Alternative.new(alternative_name, 'link_color').completed_count
-
-      session[:split].should eql("link_color" => alternative_name)
-      finished('link_color')
+      session[:split].should eql(@experiment.key => @alternative_name)
+      finished(@experiment_name)
       session[:split].should == {}
     end
 
     it "should not clear out the users session if reset is false" do
-      experiment = Split::Experiment.find_or_create('link_color', 'blue', 'red')
-      alternative_name = ab_test('link_color', 'blue', 'red')
-
-      previous_completion_count = Split::Alternative.new(alternative_name, 'link_color').completed_count
-
-      session[:split].should eql("link_color" => alternative_name)
-      finished('link_color', :reset => false)
-      session[:split].should eql("link_color" => alternative_name)
+      session[:split].should eql(@experiment.key => @alternative_name)
+      finished(@experiment_name, :reset => false)
+      session[:split].should eql(@experiment.key => @alternative_name, @experiment.finished_key => true)
     end
 
     it "should reset the users session when experiment is not versioned" do
-      experiment = Split::Experiment.find_or_create('link_color', 'blue', 'red')
-      alternative_name = ab_test('link_color', 'blue', 'red')
-
-      previous_completion_count = Split::Alternative.new(alternative_name, 'link_color').completed_count
-
-      session[:split].should eql(experiment.key => alternative_name)
-      finished('link_color', :reset => true)
+      session[:split].should eql(@experiment.key => @alternative_name)
+      finished(@experiment_name)
       session[:split].should eql({})
     end
 
     it "should reset the users session when experiment is versioned" do
-      experiment = Split::Experiment.find_or_create('link_color', 'blue', 'red')
-      experiment.increment_version
-      alternative_name = ab_test('link_color', 'blue', 'red')
-
-      previous_completion_count = Split::Alternative.new(alternative_name, 'link_color').completed_count
+      @experiment.increment_version
+      @alternative_name = ab_test(@experiment_name, *@alternatives)
 
-      session[:split].should eql(experiment.key => alternative_name)
-      finished('link_color', :reset => true)
+      session[:split].should eql(@experiment.key => @alternative_name)
+      finished(@experiment_name)
       session[:split].should eql({})
     end