Feature/ensure alternative

lib/split/alternative.rb

@@ -89,6 +89,10 @@ def increment_participation
       Split.redis.hincrby key, 'participant_count', 1
     end
 
+    def decrement_participation
+      Split.redis.hincrby key, 'participant_count', -1
+    end
+
     def increment_completion(goal = nil)
       field = set_field(goal)
       Split.redis.hincrby(key, field, 1)

lib/split/experiment.rb

@@ -230,6 +230,10 @@ def finished_key
       "#{key}:finished"
     end
 
+    def excluded_key
+      "#{key}:excluded"
+    end
+
     def metadata_key
       "#{name}:metadata"
     end

lib/split/helper.rb

@@ -9,7 +9,7 @@ def ab_test(metric_descriptor, control = nil, *alternatives)
         alternative = if Split.configuration.enabled
           experiment.save
           trial = Trial.new(:user => ab_user, :experiment => experiment,
-              :override => override_alternative(experiment.name), :exclude => exclude_visitor?,
+              :override => override_alternative(experiment.name), :exclude => exclude_visitor?(experiment),
               :disabled => split_generically_disabled?)
           alt = trial.choose!(self)
           alt ? alt.name : nil
@@ -60,12 +60,13 @@ def finish_experiment(experiment, options = {:reset => true})
     end
 
     def finished(metric_descriptor, options = {:reset => true})
-      return if exclude_visitor? || Split.configuration.disabled?
+      return if Split.configuration.disabled?
       metric_descriptor, goals = normalize_metric(metric_descriptor)
       experiments = Metric.possible_experiments(metric_descriptor)
 
       if experiments.any?
         experiments.each do |experiment|
+          next if exclude_visitor?(experiment)
           finish_experiment(experiment, options.merge(:goals => goals))
         end
       end
@@ -92,12 +93,54 @@ def begin_experiment(experiment, alternative_name = nil)
       alternative_name
     end
 
+    # alternative can be either a string or an array of strings
+    def ensure_alternative_or_exclude(experiment_name, alternatives)
+      alternatives = Array(alternatives)
+      experiment   = ExperimentCatalog.find(experiment_name)
+
+      # This is called if the experiment is not yet created. Since we can't created (we need the alternatives
+      # configuration to be passed to this method), we exclude the user, and when `ab_test` is called after
+      # the experiment is created. This means the first user will be excluded from the test and will have
+      # the control alternative
+      if !experiment
+        experiment = Experiment.new(experiment_name)
+        exclude_visitor(experiment)
+        return
+      end
+
+      # If the user doesn't have an alternative set, we override the alternative and we manually increment the
+      # participant count, since split doesn't do it when an alternative is overriden
+      unless ab_user[experiment.key]
+        params[experiment_name] = ab_user[experiment.key] = alternatives.sample
+        Split::Alternative.new(ab_user[experiment.key], experiment_name).increment_participation
+        return
+      end
+
+      current_alternative = ab_user[experiment.key]
+
+      # We don't decrement the participant count if the visitor is included or if the desired alternative
+      # was already chosen by split before
+      if exclude_visitor?(experiment) || alternatives.include?(current_alternative)
+        params[experiment_name] ||= current_alternative
+        return
+      end
+
+      # We decrement the participant count if the desired alternative was not chosen by split before
+      Split::Alternative.new(current_alternative, experiment_name).decrement_participation
+      params[experiment_name] = ab_user[experiment.key] = alternatives.sample
+      exclude_visitor(experiment)
+    end
+
+    def exclude_visitor(experiment)
+      ab_user[experiment.excluded_key] = true
+    end
+
     def ab_user
       @ab_user ||= Split::Persistence.adapter.new(self)
     end
 
-    def exclude_visitor?
-      instance_eval(&Split.configuration.ignore_filter) || is_ignored_ip_address? || is_robot?
+    def exclude_visitor?(experiment)
+      instance_eval(&Split.configuration.ignore_filter) || is_ignored_ip_address? || is_robot? || ab_user[experiment.excluded_key]
     end
 
     def is_robot?
@@ -118,7 +161,7 @@ def active_experiments
       ab_user.keys.each do |key|
         key_without_version = key.split(/\:\d(?!\:)/)[0]
         Metric.possible_experiments(key_without_version).each do |experiment|
-          if !experiment.has_winner?
+          if !experiment.has_winner? and !ab_user.keys.include?(experiment.excluded_key)
             experiment_pairs[key_without_version] = ab_user[key]
           end
         end

spec/helper_spec.rb

@@ -449,7 +449,7 @@ def should_finish_experiment(experiment_name, should_finish=true)
       expect(active_experiments.first[0]).to eq "def"
       expect(active_experiments.first[1]).to eq alternative
     end
-    
+
     it 'should show an active test when an experiment is on a later version' do
       experiment.reset
       expect(experiment.version).to eq(1)
@@ -483,6 +483,269 @@ def should_finish_experiment(experiment_name, should_finish=true)
     end
   end
 
+  describe 'ensure_alternative_or_exclude' do
+    let(:alternatives) { %w[blue red] }
+    let(:current_alternative) { ab_user['link_color'] }
+    let(:other_alternative) { (alternatives - [current_alternative]).first }
+
+    context 'single alternative' do
+
+      context 'when ab_test was called before' do
+
+        before { ab_test('link_color', *alternatives) }
+
+        it 'should decrement the counter if the desired alternative is not the current' do
+          expect(Split::Alternative.new(other_alternative, 'link_color').participant_count).to eq(0)
+          expect(Split::Alternative.new(current_alternative, 'link_color').participant_count).to eq(1)
+
+          ensure_alternative_or_exclude('link_color', other_alternative)
+
+          expect(ab_test('link_color', *alternatives)).to eq(other_alternative)
+          expect(active_experiments).not_to include('link_color')
+
+          expect(Split::Alternative.new(other_alternative, 'link_color').participant_count).to eq(0)
+          expect(Split::Alternative.new(current_alternative, 'link_color').participant_count).to eq(0)
+
+          expect(ab_user['link_color']).to eq(other_alternative)
+        end
+
+        it 'should not decrement the counter if the desired alternative is the current' do
+          expect(Split::Alternative.new(other_alternative, 'link_color').participant_count).to eq(0)
+          expect(Split::Alternative.new(current_alternative, 'link_color').participant_count).to eq(1)
+
+          ensure_alternative_or_exclude('link_color', current_alternative)
+
+          expect(ab_test('link_color', *alternatives)).to eq(current_alternative)
+          expect(active_experiments.keys).to eq(%w[link_color])
+
+          expect(Split::Alternative.new(other_alternative, 'link_color').participant_count).to eq(0)
+          expect(Split::Alternative.new(current_alternative, 'link_color').participant_count).to eq(1)
+
+          expect(ab_user['link_color']).to eq(current_alternative)
+        end
+
+        it 'should not decrement the counter more than once' do
+          expect(Split::Alternative.new(other_alternative, 'link_color').participant_count).to eq(0)
+          expect(Split::Alternative.new(current_alternative, 'link_color').participant_count).to eq(1)
+
+          ensure_alternative_or_exclude('link_color', other_alternative)
+          ensure_alternative_or_exclude('link_color', other_alternative)
+
+          expect(ab_test('link_color', *alternatives)).to eq(other_alternative)
+          expect(active_experiments).not_to include('link_color')
+
+          expect(Split::Alternative.new(other_alternative, 'link_color').participant_count).to eq(0)
+          expect(Split::Alternative.new(current_alternative, 'link_color').participant_count).to eq(0)
+
+          expect(ab_user['link_color']).to eq(other_alternative)
+        end
+
+        it 'should increment the conversions if the forced alternative is the current' do
+          expect(Split::Alternative.new(other_alternative, 'link_color').completed_count).to eq(0)
+          expect(Split::Alternative.new(current_alternative, 'link_color').completed_count).to eq(0)
+
+          ensure_alternative_or_exclude('link_color', current_alternative)
+
+          expect(ab_test('link_color', *alternatives)).to eq(current_alternative)
+          expect(active_experiments.keys).to eq(%w[link_color])
+
+          finished(experiment.name, reset: false)
+
+          expect(Split::Alternative.new(other_alternative, 'link_color').completed_count).to eq(0)
+          expect(Split::Alternative.new(current_alternative, 'link_color').completed_count).to eq(1)
+
+          expect(ab_user['link_color']).to eq(current_alternative)
+        end
+
+        it 'should not increment the conversions if the forced alternative is not the current' do
+          expect(Split::Alternative.new(other_alternative, 'link_color').completed_count).to eq(0)
+          expect(Split::Alternative.new(current_alternative, 'link_color').completed_count).to eq(0)
+
+          ensure_alternative_or_exclude('link_color', other_alternative)
+
+          expect(ab_test('link_color', *alternatives)).to eq(other_alternative)
+          expect(active_experiments).not_to include('link_color')
+
+          finished(experiment.name, reset: false)
+
+          expect(Split::Alternative.new(other_alternative, 'link_color').completed_count).to eq(0)
+          expect(Split::Alternative.new(current_alternative, 'link_color').completed_count).to eq(0)
+
+          expect(ab_user['link_color']).to eq(other_alternative)
+        end
+      end
+
+      context 'when ab_test was not previously called' do
+
+        before { Split::ExperimentCatalog.find_or_create('link_color', *alternatives) }
+
+        it 'should increment the participant counter' do
+          expect {
+            ensure_alternative_or_exclude('link_color', 'red')
+            expect(ab_test('link_color', *alternatives)).to eq('red')
+          }.to change { Split::Alternative.new('red', 'link_color').participant_count }.by(1)
+
+          expect(active_experiments.keys).to eq(%w[link_color])
+          expect(ab_user['link_color']).to eq(current_alternative)
+        end
+
+        it 'should not increment the other alternative participant counter' do
+          expect {
+            ensure_alternative_or_exclude('link_color', 'red')
+            expect(ab_test('link_color', *alternatives)).to eq('red')
+          }.not_to change { Split::Alternative.new('blue', 'link_color').participant_count }
+        end
+
+        it 'should not increment the participant counter more than once' do
+          expect {
+            ensure_alternative_or_exclude('link_color', 'red')
+            ensure_alternative_or_exclude('link_color', 'red')
+            expect(ab_test('link_color', *alternatives)).to eq('red')
+          }.to change { Split::Alternative.new('red', 'link_color').participant_count }.by(1)
+
+          expect(ab_user['link_color']).to eq(current_alternative)
+        end
+
+        it 'should increment the completed counter' do
+          ensure_alternative_or_exclude('link_color', 'red')
+          expect(ab_test('link_color', *alternatives)).to eq('red')
+
+          expect {
+            finished(experiment.name, reset: false)
+          }.to change { Split::Alternative.new(current_alternative, experiment.name).completed_count }.by(1)
+
+          expect(ab_user['link_color']).to eq(current_alternative)
+        end
+
+        it 'should not increment the other alternative completed counter' do
+          ensure_alternative_or_exclude('link_color', 'red')
+          expect(ab_test('link_color', *alternatives)).to eq('red')
+
+          expect {
+            finished(experiment.name, reset: false)
+          }.not_to change { Split::Alternative.new(other_alternative, experiment.name).completed_count }
+        end
+      end
+    end
+
+    context 'multiple alternatives' do
+      let(:alternatives) { %w[blue red black] }
+      let(:other_alternatives) { (alternatives - [current_alternative]) }
+
+      context 'when ab_test was previously called' do
+
+        let!(:current_alternative) { ab_test('link_color', *alternatives) }
+
+        it 'should decrement the counter if the none of the desired alternatives are the current' do
+
+          expect(Split::Alternative.new(other_alternatives.first, 'link_color').participant_count).to eq(0)
+          expect(Split::Alternative.new(other_alternatives.last, 'link_color').participant_count).to eq(0)
+          expect(Split::Alternative.new(current_alternative, 'link_color').participant_count).to eq(1)
+
+          ensure_alternative_or_exclude('link_color', other_alternatives)
+
+          expect(other_alternatives).to include(ab_test('link_color', *alternatives))
+          expect(active_experiments).not_to include('link_color')
+
+          expect(Split::Alternative.new(other_alternatives.first, 'link_color').participant_count).to eq(0)
+          expect(Split::Alternative.new(other_alternatives.last, 'link_color').participant_count).to eq(0)
+          expect(Split::Alternative.new(current_alternative, 'link_color').participant_count).to eq(0)
+
+          expect(other_alternatives).to include(ab_user['link_color'])
+        end
+
+        it 'should not decrement the counter if any of the desired alternative are the current' do
+          expect(Split::Alternative.new(other_alternatives.first, 'link_color').participant_count).to eq(0)
+          expect(Split::Alternative.new(other_alternatives.last, 'link_color').participant_count).to eq(0)
+          expect(Split::Alternative.new(current_alternative, 'link_color').participant_count).to eq(1)
+
+          ensure_alternative_or_exclude('link_color', [current_alternative, other_alternatives.first])
+
+          expect(ab_test('link_color', *alternatives)).to eq(current_alternative)
+          expect(active_experiments.keys).to eq(%w[link_color])
+
+          expect(Split::Alternative.new(other_alternatives.first, 'link_color').participant_count).to eq(0)
+          expect(Split::Alternative.new(other_alternatives.last, 'link_color').participant_count).to eq(0)
+          expect(Split::Alternative.new(current_alternative, 'link_color').participant_count).to eq(1)
+
+          expect(ab_user['link_color']).to eq(current_alternative)
+        end
+      end
+
+      context 'when ab_test was not previously called' do
+
+        let(:valid_alternatives) { %w[red blue] }
+
+        before { Split::ExperimentCatalog.find_or_create('link_color', *alternatives) }
+
+        it 'should increment the participant counter' do
+          ensure_alternative_or_exclude('link_color', valid_alternatives)
+          expect(valid_alternatives).to include(ab_test('link_color', *alternatives))
+          expect(active_experiments.keys).to eq(%w[link_color])
+
+          expect(Split::Alternative.new(other_alternatives.first, 'link_color').participant_count).to eq(0)
+          expect(Split::Alternative.new(other_alternatives.last, 'link_color').participant_count).to eq(0)
+          expect(Split::Alternative.new(current_alternative, 'link_color').participant_count).to eq(1)
+
+          expect(ab_user['link_color']).to eq(current_alternative)
+        end
+
+      end
+    end
+
+    context 'versioned experiment' do
+
+      before do
+        experiment.reset
+        ab_test('link_color', *alternatives)
+      end
+
+      let(:current_alternative) { ab_user[experiment.key] }
+
+      it 'should decrement the counter if the desired alternative is not the current' do
+        expect(Split::Alternative.new(other_alternative, 'link_color').participant_count).to eq(0)
+        expect(Split::Alternative.new(current_alternative, 'link_color').participant_count).to eq(1)
+
+        ensure_alternative_or_exclude('link_color', other_alternative)
+        expect(ab_test('link_color', *alternatives)).to eq(other_alternative)
+        expect(active_experiments).not_to include('link_color')
+
+        expect(Split::Alternative.new(other_alternative, 'link_color').participant_count).to eq(0)
+        expect(Split::Alternative.new(current_alternative, 'link_color').participant_count).to eq(0)
+
+        expect(ab_user['link_color:1']).to eq(other_alternative)
+      end
+
+      it 'should not decrement the counter if the desired alternative is the current' do
+        expect(Split::Alternative.new(other_alternative, 'link_color').participant_count).to eq(0)
+        expect(Split::Alternative.new(current_alternative, 'link_color').participant_count).to eq(1)
+
+        ensure_alternative_or_exclude('link_color', current_alternative)
+
+        expect(ab_test('link_color', *alternatives)).to eq(current_alternative)
+        expect(active_experiments.keys).to eq(%w[link_color])
+
+        expect(Split::Alternative.new(other_alternative, 'link_color').participant_count).to eq(0)
+        expect(Split::Alternative.new(current_alternative, 'link_color').participant_count).to eq(1)
+
+        expect(ab_user['link_color:1']).to eq(current_alternative)
+      end
+    end
+
+    context "when the experiment is not created" do
+
+      it "creates the experiment and excludes the user" do
+        ensure_alternative_or_exclude('link_color', 'red')
+        expect(ab_test('link_color', 'red', 'blue')).to eq('red')
+        expect(ab_user['link_color:excluded']).to eq(true)
+
+        expect(Split::Alternative.new('red', 'link_color').participant_count).to eq(0)
+        expect(Split::Alternative.new('blue', 'link_color').participant_count).to eq(0)
+      end
+
+    end
+  end
+
   describe 'when user is a robot' do
     before(:each) do
       @request = OpenStruct.new(:user_agent => 'Googlebot/2.1 (+http://www.google.com/bot.html)')

spec/spec_helper.rb

@@ -2,6 +2,7 @@
 
 require 'rubygems'
 require 'bundler/setup'
+require 'pry'
 
 require 'coveralls'
 Coveralls.wear!