diff --git a/lib/vanity/adapters/abstract_adapter.rb b/lib/vanity/adapters/abstract_adapter.rb index fe94d7ea..70271da0 100644 --- a/lib/vanity/adapters/abstract_adapter.rb +++ b/lib/vanity/adapters/abstract_adapter.rb @@ -142,6 +142,16 @@ def destroy_experiment(experiment) fail "Not implemented" end + private + + def with_ab_seen_deprecation(experiment, identity, alternative) + if alternative.respond_to?(:id) + Vanity.configuration.logger.warn(%q{Deprecated: #ab_seen should be passed the alternative id, not an Alternative instance}) + yield experiment, identity, alternative.id + else + yield experiment, identity, alternative + end + end end end end diff --git a/lib/vanity/adapters/active_record_adapter.rb b/lib/vanity/adapters/active_record_adapter.rb index a901ad51..cf900150 100644 --- a/lib/vanity/adapters/active_record_adapter.rb +++ b/lib/vanity/adapters/active_record_adapter.rb @@ -255,9 +255,11 @@ def ab_add_participant(experiment, alternative, identity) end # Determines if a participant already has seen this alternative in this experiment. - def ab_seen(experiment, identity, alternative) - participant = VanityParticipant.retrieve(experiment, identity, false) - participant && participant.seen == alternative.id + def ab_seen(experiment, identity, alternative_or_id) + with_ab_seen_deprecation(experiment, identity, alternative_or_id) do |expt, ident, alt_id| + participant = VanityParticipant.retrieve(expt, ident, false) + participant && participant.seen == alt_id + end end # Returns the participant's seen alternative in this experiment, if it exists diff --git a/lib/vanity/adapters/mock_adapter.rb b/lib/vanity/adapters/mock_adapter.rb index 2747251b..72e79754 100644 --- a/lib/vanity/adapters/mock_adapter.rb +++ b/lib/vanity/adapters/mock_adapter.rb @@ -137,9 +137,11 @@ def ab_add_participant(experiment, alternative, identity) alt[:participants] << identity end - def ab_seen(experiment, identity, alternative) - if ab_assigned(experiment, identity) == alternative.id - alternative + def ab_seen(experiment, identity, alternative_or_id) + with_ab_seen_deprecation(experiment, identity, alternative_or_id) do |expt, ident, alt_id| + if ab_assigned(expt, ident) == alt_id + alt_id + end end end diff --git a/lib/vanity/adapters/mongodb_adapter.rb b/lib/vanity/adapters/mongodb_adapter.rb index 52cade48..ed58b0d0 100644 --- a/lib/vanity/adapters/mongodb_adapter.rb +++ b/lib/vanity/adapters/mongodb_adapter.rb @@ -200,9 +200,11 @@ def ab_add_participant(experiment, alternative, identity) end # Determines if a participant already has seen this alternative in this experiment. - def ab_seen(experiment, identity, alternative) - participant = @participants.find(:experiment=>experiment, :identity=>identity).limit(1).projection(:seen=>1).first - participant && participant["seen"].first == alternative.id + def ab_seen(experiment, identity, alternative_or_id) + with_ab_seen_deprecation(experiment, identity, alternative_or_id) do |expt, ident, alt_id| + participant = @participants.find(:experiment=>expt, :identity=>ident).limit(1).projection(:seen=>1).first + participant && participant["seen"].first == alt_id + end end # Returns the participant's seen alternative in this experiment, if it exists diff --git a/lib/vanity/adapters/redis_adapter.rb b/lib/vanity/adapters/redis_adapter.rb index 53eda97a..3bb74962 100644 --- a/lib/vanity/adapters/redis_adapter.rb +++ b/lib/vanity/adapters/redis_adapter.rb @@ -175,12 +175,14 @@ def ab_add_participant(experiment, alternative, identity) end end - def ab_seen(experiment, identity, alternative) - call_redis_with_failover(experiment, identity, alternative) do - if @experiments.sismember "#{experiment}:alts:#{alternative.id}:participants", identity - alternative - else - nil + def ab_seen(experiment, identity, alternative_or_id) + with_ab_seen_deprecation(experiment, identity, alternative_or_id) do |expt, ident, alt_id| + call_redis_with_failover(expt, ident, alt_id) do + if @experiments.sismember "#{expt}:alts:#{alt_id}:participants", ident + alt_id + else + nil + end end end end diff --git a/lib/vanity/experiment/ab_test.rb b/lib/vanity/experiment/ab_test.rb index 840d6405..3852640f 100644 --- a/lib/vanity/experiment/ab_test.rb +++ b/lib/vanity/experiment/ab_test.rb @@ -635,7 +635,7 @@ def call_on_assignment_if_available(identity, index) # if we have an on_assignment block, call it on new assignments if defined?(@on_assignment_block) && @on_assignment_block assignment = alternatives[index] - if !connection.ab_seen @id, identity, assignment + if !connection.ab_seen @id, identity, index @on_assignment_block.call(Vanity.context, identity, assignment, self) end end diff --git a/test/adapters/shared_tests.rb b/test/adapters/shared_tests.rb index 34f439ce..f1a4f3f7 100644 --- a/test/adapters/shared_tests.rb +++ b/test/adapters/shared_tests.rb @@ -207,20 +207,57 @@ def with_experiments_start_enabled(enabled) end describe '#ab_seen' do - before do - @alternative_instance = DummyAlternative.new(alternative) - end + describe 'called with an Alternative instance' do + def capture_logs + require 'stringio' + original_logger = Vanity.configuration.logger + log_output = StringIO.new + Vanity.configuration.logger = Logger.new(log_output) + yield + log_output.string + ensure + Vanity.configuration.logger = original_logger + end - it 'returns a truthy value if the identity is assigned to the alternative' do - @subject.ab_add_participant(experiment, alternative, identity) + before do + @alternative_instance = DummyAlternative.new(alternative) + end + + it 'emits a deprecation warning' do + @subject.ab_add_participant(experiment, alternative, identity) + + out = capture_logs do + @subject.ab_seen(experiment, identity, @alternative_instance) + end + + assert_match(/Deprecated/, out) + end - assert(@subject.ab_seen(experiment, identity, @alternative_instance)) + it 'returns a truthy value if the identity is assigned to the alternative' do + @subject.ab_add_participant(experiment, alternative, identity) + + assert(@subject.ab_seen(experiment, identity, @alternative_instance)) + end + + it 'returns a falsey value if the identity is not assigned to the alternative' do + @subject.ab_add_participant(experiment, alternative, identity) + + refute(@subject.ab_seen(experiment, identity, DummyAlternative.new(2))) + end end - it 'returns a falsey value if the identity is not assigned to the alternative' do - @subject.ab_add_participant(experiment, alternative, identity) + describe 'called with an alternative id' do + it 'returns a truthy value if the identity is assigned to the alternative' do + @subject.ab_add_participant(experiment, alternative, identity) + + assert(@subject.ab_seen(experiment, identity, alternative)) + end - refute(@subject.ab_seen(experiment, identity, DummyAlternative.new(2))) + it 'returns a falsey value if the identity is not assigned to the alternative' do + @subject.ab_add_participant(experiment, alternative, identity) + + refute(@subject.ab_seen(experiment, identity, 2)) + end end end diff --git a/test/vanity_test.rb b/test/vanity_test.rb index d4caa987..7963ac38 100644 --- a/test/vanity_test.rb +++ b/test/vanity_test.rb @@ -108,12 +108,16 @@ f.write(connection_config) end - Vanity.reset! - Vanity.disconnect! - Vanity::Connection.stubs(:new) - Vanity.connect! + out, _err = capture_io do + Vanity.reset! + Vanity.configuration.logger = Logger.new($stdout) + Vanity.disconnect! + Vanity::Connection.stubs(:new) + Vanity.connect! + end assert_equal false, Vanity.configuration.collecting + assert_match(/Deprecated/, out) end end end @@ -178,4 +182,4 @@ refute_same original_configuration, Vanity.reload! end end -end \ No newline at end of file +end