refac(config): Moves forced variations mapping into DecisionService. (#174)

Author: rashidsp

lib/optimizely.rb

@@ -173,7 +173,16 @@ def get_variation(experiment_key, user_id, attributes = nil)
     # @return [Boolean] indicates if the set completed successfully.
 
     def set_forced_variation(experiment_key, user_id, variation_key)
-      @config.set_forced_variation(experiment_key, user_id, variation_key)
+      unless @is_valid
+        @logger.log(Logger::ERROR, InvalidDatafileError.new('set_forced_variation').message)
+        return nil
+      end
+
+      input_values = {experiment_key: experiment_key, user_id: user_id}
+      input_values[:variation_key] = variation_key unless variation_key.nil?
+      return false unless Optimizely::Helpers::Validator.inputs_valid?(input_values, @logger, Logger::ERROR)
+
+      @decision_service.set_forced_variation(@config, experiment_key, user_id, variation_key)
     end
 
     # Gets the forced variation for a given user and experiment.
@@ -184,8 +193,20 @@ def set_forced_variation(experiment_key, user_id, variation_key)
     # @return [String] The forced variation key.
 
     def get_forced_variation(experiment_key, user_id)
+      unless @is_valid
+        @logger.log(Logger::ERROR, InvalidDatafileError.new('get_forced_variation').message)
+        return nil
+      end
+
+      return nil unless Optimizely::Helpers::Validator.inputs_valid?(
+        {
+          experiment_key: experiment_key,
+          user_id: user_id
+        }, @logger, Logger::ERROR
+      )
+
       forced_variation_key = nil
-      forced_variation = @config.get_forced_variation(experiment_key, user_id)
+      forced_variation = @decision_service.get_forced_variation(@config, experiment_key, user_id)
       forced_variation_key = forced_variation['key'] if forced_variation
 
       forced_variation_key

lib/optimizely/decision_service.rb

@@ -33,6 +33,10 @@ class DecisionService
 
     attr_reader :bucketer
 
+    # Hash of user IDs to a Hash of experiments to variations.
+    # This contains all the forced variations set by the user by calling setForcedVariation.
+    attr_reader :forced_variation_map
+
     Decision = Struct.new(:experiment, :variation, :source)
 
     DECISION_SOURCES = {
@@ -44,6 +48,7 @@ def initialize(logger, user_profile_service = nil)
       @logger = logger
       @user_profile_service = user_profile_service
       @bucketer = Bucketer.new(logger)
+      @forced_variation_map = {}
     end
 
     def get_variation(project_config, experiment_key, user_id, attributes = nil)
@@ -70,7 +75,7 @@ def get_variation(project_config, experiment_key, user_id, attributes = nil)
       end
 
       # Check if a forced variation is set for the user
-      forced_variation = project_config.get_forced_variation(experiment_key, user_id)
+      forced_variation = get_forced_variation(project_config, experiment_key, user_id)
       return forced_variation['id'] if forced_variation
 
       # Check if user is in a white-listed variation
@@ -266,6 +271,86 @@ def get_variation_for_feature_rollout(project_config, feature_flag, user_id, att
       nil
     end
 
+    def set_forced_variation(project_config, experiment_key, user_id, variation_key)
+      # Sets a Hash of user IDs to a Hash of experiments to forced variations.
+      #
+      # project_config - Instance of ProjectConfig
+      # experiment_key - String Key for experiment
+      # user_id - String ID for user.
+      # variation_key - String Key for variation. If null, then clear the existing experiment-to-variation mapping
+      #
+      # Returns a boolean value that indicates if the set completed successfully
+
+      experiment = project_config.get_experiment_from_key(experiment_key)
+      experiment_id = experiment['id'] if experiment
+      #  check if the experiment exists in the datafile
+      return false if experiment_id.nil? || experiment_id.empty?
+
+      #  clear the forced variation if the variation key is null
+      if variation_key.nil?
+        @forced_variation_map[user_id].delete(experiment_id) if @forced_variation_map.key? user_id
+        @logger.log(Logger::DEBUG, "Variation mapped to experiment '#{experiment_key}' has been removed for user "\
+                    "'#{user_id}'.")
+        return true
+      end
+
+      variation_id = project_config.get_variation_id_from_key(experiment_key, variation_key)
+
+      #  check if the variation exists in the datafile
+      unless variation_id
+        #  this case is logged in get_variation_id_from_key
+        return false
+      end
+
+      @forced_variation_map[user_id] = {} unless @forced_variation_map.key? user_id
+      @forced_variation_map[user_id][experiment_id] = variation_id
+      @logger.log(Logger::DEBUG, "Set variation '#{variation_id}' for experiment '#{experiment_id}' and "\
+                  "user '#{user_id}' in the forced variation map.")
+      true
+    end
+
+    def get_forced_variation(project_config, experiment_key, user_id)
+      # Gets the forced variation for the given user and experiment.
+      #
+      # project_config - Instance of ProjectConfig
+      # experiment_key - String Key for experiment
+      # user_id - String ID for user
+      #
+      # Returns Variation The variation which the given user and experiment should be forced into
+
+      unless @forced_variation_map.key? user_id
+        @logger.log(Logger::DEBUG, "User '#{user_id}' is not in the forced variation map.")
+        return nil
+      end
+
+      experiment_to_variation_map = @forced_variation_map[user_id]
+      experiment = project_config.get_experiment_from_key(experiment_key)
+      experiment_id = experiment['id'] if experiment
+      # check for nil and empty string experiment ID
+      # this case is logged in get_experiment_from_key
+      return nil if experiment_id.nil? || experiment_id.empty?
+
+      unless experiment_to_variation_map.key? experiment_id
+        @logger.log(Logger::DEBUG, "No experiment '#{experiment_key}' mapped to user '#{user_id}' "\
+                    'in the forced variation map.')
+        return nil
+      end
+
+      variation_id = experiment_to_variation_map[experiment_id]
+      variation_key = ''
+      variation = project_config.get_variation_from_id(experiment_key, variation_id)
+      variation_key = variation['key'] if variation
+
+      # check if the variation exists in the datafile
+      # this case is logged in get_variation_from_id
+      return nil if variation_key.empty?
+
+      @logger.log(Logger::DEBUG, "Variation '#{variation_key}' is mapped to experiment '#{experiment_key}' "\
+                  "and user '#{user_id}' in the forced variation map")
+
+      variation
+    end
+
     private
 
     def get_whitelisted_variation_id(project_config, experiment_key, user_id)

lib/optimizely/project_config.rb

@@ -59,12 +59,6 @@ class ProjectConfig
     attr_reader :variation_id_to_variable_usage_map
     attr_reader :variation_key_map
 
-    # Hash of user IDs to a Hash
-    # of experiments to variations. This contains all the forced variations
-    # set by the user by calling setForcedVariation (it is not the same as the
-    # whitelisting forcedVariations data structure in the Experiments class).
-    attr_reader :forced_variation_map
-
     def initialize(datafile, logger, error_handler)
       # ProjectConfig init method to fetch and set project config data
       #
@@ -108,7 +102,6 @@ def initialize(datafile, logger, error_handler)
       @audience_id_map = @audience_id_map.merge(generate_key_map(@typed_audiences, 'id')) unless @typed_audiences.empty?
       @variation_id_map = {}
       @variation_key_map = {}
-      @forced_variation_map = {}
       @variation_id_to_variable_usage_map = {}
       @variation_id_to_experiment_map = {}
       @experiment_key_map.each_value do |exp|
@@ -280,95 +273,6 @@ def get_whitelisted_variations(experiment_key)
       @error_handler.handle_error InvalidExperimentError
     end
 
-    def get_forced_variation(experiment_key, user_id)
-      # Gets the forced variation for the given user and experiment.
-      #
-      # experiment_key - String Key for experiment.
-      # user_id - String ID for user
-      #
-      # Returns Variation The variation which the given user and experiment should be forced into.
-
-      return nil unless Optimizely::Helpers::Validator.inputs_valid?(
-        {
-          experiment_key: experiment_key,
-          user_id: user_id
-        }, @logger, Logger::DEBUG
-      )
-
-      unless @forced_variation_map.key? user_id
-        @logger.log(Logger::DEBUG, "User '#{user_id}' is not in the forced variation map.")
-        return nil
-      end
-
-      experiment_to_variation_map = @forced_variation_map[user_id]
-      experiment = get_experiment_from_key(experiment_key)
-      experiment_id = experiment['id'] if experiment
-      # check for nil and empty string experiment ID
-      # this case is logged in get_experiment_from_key
-      return nil if experiment_id.nil? || experiment_id.empty?
-
-      unless experiment_to_variation_map.key? experiment_id
-        @logger.log(Logger::DEBUG, "No experiment '#{experiment_key}' mapped to user '#{user_id}' "\
-                    'in the forced variation map.')
-        return nil
-      end
-
-      variation_id = experiment_to_variation_map[experiment_id]
-      variation_key = ''
-      variation = get_variation_from_id(experiment_key, variation_id)
-      variation_key = variation['key'] if variation
-
-      # check if the variation exists in the datafile
-      # this case is logged in get_variation_from_id
-      return nil if variation_key.empty?
-
-      @logger.log(Logger::DEBUG, "Variation '#{variation_key}' is mapped to experiment '#{experiment_key}' "\
-                  "and user '#{user_id}' in the forced variation map")
-
-      variation
-    end
-
-    def set_forced_variation(experiment_key, user_id, variation_key)
-      # Sets a Hash of user IDs to a Hash of experiments to forced variations.
-      #
-      # experiment_key - String Key for experiment.
-      # user_id - String ID for user.
-      # variation_key - String Key for variation. If null, then clear the existing experiment-to-variation mapping.
-      #
-      # Returns a boolean value that indicates if the set completed successfully.
-
-      input_values = {experiment_key: experiment_key, user_id: user_id}
-      input_values[:variation_key] = variation_key unless variation_key.nil?
-      return false unless Optimizely::Helpers::Validator.inputs_valid?(input_values, @logger, Logger::DEBUG)
-
-      experiment = get_experiment_from_key(experiment_key)
-      experiment_id = experiment['id'] if experiment
-      #  check if the experiment exists in the datafile
-      return false if experiment_id.nil? || experiment_id.empty?
-
-      #  clear the forced variation if the variation key is null
-      if variation_key.nil?
-        @forced_variation_map[user_id].delete(experiment_id) if @forced_variation_map.key? user_id
-        @logger.log(Logger::DEBUG, "Variation mapped to experiment '#{experiment_key}' has been removed for user "\
-                    "'#{user_id}'.")
-        return true
-      end
-
-      variation_id = get_variation_id_from_key(experiment_key, variation_key)
-
-      #  check if the variation exists in the datafile
-      unless variation_id
-        #  this case is logged in get_variation_id_from_key
-        return false
-      end
-
-      @forced_variation_map[user_id] = {} unless @forced_variation_map.key? user_id
-      @forced_variation_map[user_id][experiment_id] = variation_id
-      @logger.log(Logger::DEBUG, "Set variation '#{variation_id}' for experiment '#{experiment_id}' and "\
-                  "user '#{user_id}' in the forced variation map.")
-      true
-    end
-
     def get_attribute_id(attribute_key)
       # Get attribute ID for the provided attribute key.
       #

spec/decision_service_spec.rb

@@ -40,7 +40,7 @@
     end
 
     it 'should return the correct variation ID for a given user for whom a variation has been forced' do
-      config.set_forced_variation('test_experiment', 'test_user', 'variation')
+      decision_service.set_forced_variation(config, 'test_experiment', 'test_user', 'variation')
       expect(decision_service.get_variation(config, 'test_experiment', 'test_user')).to eq('111129')
       # Setting forced variation should short circuit whitelist check, bucketing and audience evaluation
       expect(decision_service).not_to have_received(:get_whitelisted_variation_id)
@@ -53,7 +53,7 @@
         'browser_type' => 'firefox',
         Optimizely::Helpers::Constants::CONTROL_ATTRIBUTES['BUCKETING_ID'] => 'pid'
       }
-      config.set_forced_variation('test_experiment_with_audience', 'test_user', 'control_with_audience')
+      decision_service.set_forced_variation(config, 'test_experiment_with_audience', 'test_user', 'control_with_audience')
       expect(decision_service.get_variation(config, 'test_experiment_with_audience', 'test_user', user_attributes)).to eq('122228')
       # Setting forced variation should short circuit whitelist check, bucketing and audience evaluation
       expect(decision_service).not_to have_received(:get_whitelisted_variation_id)
@@ -713,4 +713,90 @@
       expect(spy_logger).not_to have_received(:log)
     end
   end
+
+  # Only those log messages have been asserted, which are directly logged in these methods.
+  # Messages that are logged in some internal function calls, are asserted in their respective function test cases.
+  describe 'get_forced_variation' do
+    user_id = 'test_user'
+    invalid_experiment_key = 'invalid_experiment'
+    valid_experiment = {id: '111127', key: 'test_experiment'}
+
+    # User ID is not defined in the forced variation map
+    it 'should log a message and return nil when user is not in forced variation map' do
+      expect(decision_service.get_forced_variation(config, valid_experiment[:key], user_id)).to eq(nil)
+      expect(spy_logger).to have_received(:log).with(Logger::DEBUG,
+                                                     "User '#{user_id}' is not in the forced variation map.")
+    end
+    # Experiment key does not exist in the datafile
+    it 'should return nil when experiment key is not in datafile' do
+      expect(decision_service.get_forced_variation(config, invalid_experiment_key, user_id)).to eq(nil)
+    end
+  end
+
+  # Only those log messages have been asserted, which are directly logged in these methods.
+  # Messages that are logged in some internal function calls, are asserted in their respective function test cases.
+  describe 'set_forced_variation' do
+    user_id = 'test_user'
+    invalid_experiment_key = 'invalid_experiment'
+    invalid_variation_key = 'invalid_variation'
+    valid_experiment = {id: '111127', key: 'test_experiment'}
+    valid_variation = {id: '111128', key: 'control'}
+
+    # Experiment key does not exist in the datafile
+    it 'return nil when experiment key is not in datafile' do
+      expect(decision_service.set_forced_variation(config, invalid_experiment_key, user_id, valid_variation[:key])).to eq(false)
+    end
+    # Variation key does not exist in the datafile
+    it 'return false when variation_key is not in datafile' do
+      expect(decision_service.set_forced_variation(config, valid_experiment[:key], user_id, invalid_variation_key)).to eq(false)
+    end
+  end
+
+  describe 'set/get forced variations multiple calls' do
+    user_id = 'test_user'
+    user_id_2 = 'test_user_2'
+    valid_experiment = {id: '111127', key: 'test_experiment'}
+    valid_variation = {id: '111128', key: 'control'}
+    valid_variation_2 = {id: '111129', key: 'variation'}
+    valid_experiment_2 = {id: '122227', key: 'test_experiment_with_audience'}
+    valid_variation_for_exp_2 = {id: '122228', key: 'control_with_audience'}
+    # Call set variation with different variations on one user/experiment to confirm that each set is expected.
+    it 'should set and return expected variations when different variations are set and removed for one user/experiment' do
+      expect(decision_service.set_forced_variation(config, valid_experiment[:key], user_id, valid_variation[:key])).to eq(true)
+      variation = decision_service.get_forced_variation(config, valid_experiment[:key], user_id)
+      expect(variation['id']).to eq(valid_variation[:id])
+      expect(variation['key']).to eq(valid_variation[:key])
+
+      expect(decision_service.set_forced_variation(config, valid_experiment[:key], user_id, valid_variation_2[:key])).to eq(true)
+      variation = decision_service.get_forced_variation(config, valid_experiment[:key], user_id)
+      expect(variation['id']).to eq(valid_variation_2[:id])
+      expect(variation['key']).to eq(valid_variation_2[:key])
+    end
+
+    # Set variation on multiple experiments for one user.
+    it 'should set and return expected variations when variation is set for multiple experiments for one user' do
+      expect(decision_service.set_forced_variation(config, valid_experiment[:key], user_id, valid_variation[:key])).to eq(true)
+      variation = decision_service.get_forced_variation(config, valid_experiment[:key], user_id)
+      expect(variation['id']).to eq(valid_variation[:id])
+      expect(variation['key']).to eq(valid_variation[:key])
+
+      expect(decision_service.set_forced_variation(config, valid_experiment_2[:key], user_id, valid_variation_for_exp_2[:key])).to eq(true)
+      variation = decision_service.get_forced_variation(config, valid_experiment_2[:key], user_id)
+      expect(variation['id']).to eq(valid_variation_for_exp_2[:id])
+      expect(variation['key']).to eq(valid_variation_for_exp_2[:key])
+    end
+
+    # Set variations for multiple users.
+    it 'should set and return expected variations when variations are set for multiple users' do
+      expect(decision_service.set_forced_variation(config, valid_experiment[:key], user_id, valid_variation[:key])).to eq(true)
+      variation = decision_service.get_forced_variation(config, valid_experiment[:key], user_id)
+      expect(variation['id']).to eq(valid_variation[:id])
+      expect(variation['key']).to eq(valid_variation[:key])
+
+      expect(decision_service.set_forced_variation(config, valid_experiment[:key], user_id_2, valid_variation[:key])).to eq(true)
+      variation = decision_service.get_forced_variation(config, valid_experiment[:key], user_id_2)
+      expect(variation['id']).to eq(valid_variation[:id])
+      expect(variation['key']).to eq(valid_variation[:key])
+    end
+  end
 end