diff --git a/lib/split/helper.rb b/lib/split/helper.rb index 95949427..9d7e9006 100644 --- a/lib/split/helper.rb +++ b/lib/split/helper.rb @@ -115,16 +115,7 @@ def is_ignored_ip_address? end def active_experiments - experiment_pairs = {} - 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? - experiment_pairs[key_without_version] = ab_user[key] - end - end - end - return experiment_pairs + ab_user.active_experiments end def normalize_metric(metric_descriptor) diff --git a/lib/split/trial.rb b/lib/split/trial.rb index f6613f9c..260c3fa6 100644 --- a/lib/split/trial.rb +++ b/lib/split/trial.rb @@ -49,7 +49,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 + @user.cleanup_old_experiments! # Only run the process once return alternative if @alternative_choosen @@ -102,22 +102,12 @@ def should_store_alternative? def cleanup_old_versions if @experiment.version > 0 - keys = @user.keys.select { |k| k.match(Regexp.new(@experiment.name)) } - keys_without_experiment(keys).each { |key| @user.delete(key) } + @user.cleanup_old_versions!(@experiment) end end def exclude_user? - @options[:exclude] || @experiment.start_time.nil? || max_experiments_reached? - end - - def max_experiments_reached? - !Split.configuration.allow_multiple_experiments && - keys_without_experiment(@user.keys).length > 0 - end - - def keys_without_experiment(keys) - keys.reject { |k| k.match(Regexp.new("^#{@experiment.key}(:finished)?$")) } + @options[:exclude] || @experiment.start_time.nil? || @user.max_experiments_reached?(@experiment.key) end end end diff --git a/lib/split/user.rb b/lib/split/user.rb index b75b8f23..9eb6ddbd 100644 --- a/lib/split/user.rb +++ b/lib/split/user.rb @@ -8,7 +8,7 @@ def initialize(context) @user = Split::Persistence.adapter.new(context) end - def cleanup_old_experiments + 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? @@ -17,6 +17,34 @@ def cleanup_old_experiments end end + def max_experiments_reached?(experiment_key) + !Split.configuration.allow_multiple_experiments && + keys_without_experiment(user.keys, experiment_key).length > 0 + end + + def cleanup_old_versions!(experiment) + keys = user.keys.select { |k| k.match(Regexp.new(experiment.name)) } + keys_without_experiment(keys, experiment.key).each { |key| user.delete(key) } + end + + def active_experiments + experiment_pairs = {} + user.keys.each do |key| + Metric.possible_experiments(key_without_version(key)).each do |experiment| + if !experiment.has_winner? + experiment_pairs[key_without_version(key)] = user[key] + end + end + end + experiment_pairs + end + + private + + def keys_without_experiment(keys, experiment_key) + keys.reject { |k| k.match(Regexp.new("^#{experiment_key}(:finished)?$")) } + end + def key_without_version(key) key.split(/\:\d(?!\:)/)[0] end diff --git a/spec/user_spec.rb b/spec/user_spec.rb index 407890f2..5c48d2b1 100644 --- a/spec/user_spec.rb +++ b/spec/user_spec.rb @@ -4,9 +4,9 @@ require 'split/user' describe Split::User do - let(:context) do - double(:session => { split: { 'link_color' => 'blue' } }) - end + let(:user_keys) { { 'link_color' => 'blue' } } + let(:context) { double(:session => { split: user_keys }) } + let(:experiment) { Split::Experiment.new('link_color') } before(:each) do @subject = described_class.new(context) @@ -16,11 +16,18 @@ expect(@subject['link_color']).to eq(@subject.user['link_color']) end - context '#cleanup_old_experiments' do - let(:experiment) { Split::Experiment.new('link_color') } + context '#cleanup_old_versions!' do + let(:user_keys) { { 'link_color:1' => 'blue' } } + + it 'removes key if old experiment is found' do + @subject.cleanup_old_versions!(experiment) + expect(@subject.keys).to be_empty + end + end + context '#cleanup_old_experiments!' do it 'removes key if experiment is not found' do - @subject.cleanup_old_experiments + @subject.cleanup_old_experiments! expect(@subject.keys).to be_empty end @@ -28,14 +35,14 @@ allow(Split::ExperimentCatalog).to receive(:find).with('link_color').and_return(experiment) allow(experiment).to receive(:start_time).and_return(Date.today) allow(experiment).to receive(:has_winner?).and_return(true) - @subject.cleanup_old_experiments + @subject.cleanup_old_experiments! expect(@subject.keys).to be_empty end it 'removes key if experiment has not started yet' do allow(Split::ExperimentCatalog).to receive(:find).with('link_color').and_return(experiment) allow(experiment).to receive(:has_winner?).and_return(false) - @subject.cleanup_old_experiments + @subject.cleanup_old_experiments! expect(@subject.keys).to be_empty end end