Merge pull request #317 from Futurelearn/deprecate-ab-seen-with-alter…

…native-instance

Deprecate calling #ab_seen with alternative instance

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

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

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
 

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

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

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

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
 

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
+end