comments addressed

Author: ozayr-zaviar

lib/optimizely.rb

@@ -1081,6 +1081,7 @@ def send_impression(config, experiment, variation_key, flag_key, rule_key, enabl
       end
 
       experiment_id = experiment['id']
+      experiment_key = experiment['key']
 
       variation_id = ''
       variation_id = config.get_variation_id_from_key_by_experiment_id(experiment_id, variation_key) if experiment_id != ''
@@ -1097,7 +1098,7 @@ def send_impression(config, experiment, variation_key, flag_key, rule_key, enabl
       @event_processor.process(user_event)
       return unless @notification_center.notification_count(NotificationCenter::NOTIFICATION_TYPES[:ACTIVATE]).positive?
 
-      @logger.log(Logger::INFO, "Activating user '#{user_id}' in experiment '#{experiment_id}'.")
+      @logger.log(Logger::INFO, "Activating user '#{user_id}' in experiment '#{experiment_key}'.")
 
       experiment = nil if experiment_id == ''
       variation = nil

lib/optimizely/config/datafile_project_config.rb

@@ -333,9 +333,9 @@ def get_variation_id_from_key_by_experiment_id(experiment_id, variation_key)
       #
       # Returns the variation or nil if not found
 
-      variation_key_map_by_experiment_id = @variation_key_map_by_experiment_id[experiment_id]
-      if variation_key_map_by_experiment_id
-        variation = variation_key_map_by_experiment_id[variation_key]
+      variation_key_map = @variation_key_map_by_experiment_id[experiment_id]
+      if variation_key_map
+        variation = variation_key_map[variation_key]
         return variation['id'] if variation
 
         @logger.log Logger::ERROR, "Variation key '#{variation_key}' is not in datafile."
@@ -372,16 +372,16 @@ def get_variation_id_from_key(experiment_key, variation_key)
     end
 
     def get_whitelisted_variations(experiment_id)
-      # Retrieves whitelisted variations for a given experiment Key
+      # Retrieves whitelisted variations for a given experiment id
       #
-      # experiment_key - String Key representing the experiment
+      # experiment_id - String id representing the experiment
       #
       # Returns whitelisted variations for the experiment or nil
 
       experiment = @experiment_id_map[experiment_id]
       return experiment['forcedVariations'] if experiment
 
-      @logger.log Logger::ERROR, "Experiment key '#{experiment_id}' is not in datafile."
+      @logger.log Logger::ERROR, "Experiment ID '#{experiment_id}' is not in datafile."
       @error_handler.handle_error InvalidExperimentError
     end
 

lib/optimizely/decision_service.rb

@@ -71,8 +71,9 @@ def get_variation(project_config, experiment_id, user_id, attributes = nil, deci
       experiment = project_config.get_experiment_from_id(experiment_id)
       return nil, decide_reasons if experiment.nil?
 
+      experiment_key = experiment['key']
       unless project_config.experiment_running?(experiment)
-        message = "Experiment '#{experiment_id}' is not running."
+        message = "Experiment '#{experiment_key}' is not running."
         @logger.log(Logger::INFO, message)
         decide_reasons.push(message)
         return nil, decide_reasons
@@ -96,7 +97,7 @@ def get_variation(project_config, experiment_id, user_id, attributes = nil, deci
         saved_variation_id, reasons_received = get_saved_variation_id(project_config, experiment_id, user_profile)
         decide_reasons.push(*reasons_received)
         if saved_variation_id
-          message = "Returning previously activated variation ID #{saved_variation_id} of experiment '#{experiment_id}' for user '#{user_id}' from user profile."
+          message = "Returning previously activated variation ID #{saved_variation_id} of experiment '#{experiment_key}' for user '#{user_id}' from user profile."
           @logger.log(Logger::INFO, message)
           decide_reasons.push(message)
           return saved_variation_id, decide_reasons
@@ -107,7 +108,7 @@ def get_variation(project_config, experiment_id, user_id, attributes = nil, deci
       user_meets_audience_conditions, reasons_received = Audience.user_meets_audience_conditions?(project_config, experiment, attributes, @logger)
       decide_reasons.push(*reasons_received)
       unless user_meets_audience_conditions
-        message = "User '#{user_id}' does not meet the conditions to be in experiment '#{experiment_id}'."
+        message = "User '#{user_id}' does not meet the conditions to be in experiment '#{experiment_key}'."
         @logger.log(Logger::INFO, message)
         decide_reasons.push(message)
         return nil, decide_reasons
@@ -191,7 +192,7 @@ def get_variation_for_feature_experiment(project_config, feature_flag, user_id,
 
         next unless variation_id
 
-        variation = project_config.variation_id_map_by_experiment_id[experiment_id][variation_id]
+        variation = project_config.get_variation_from_id_by_experiment_id(experiment_id, variation_id)
 
         return Decision.new(experiment, variation, DECISION_SOURCES['FEATURE_TEST']), decide_reasons
       end
@@ -346,7 +347,6 @@ def get_forced_variation(project_config, experiment_key, user_id)
       end
 
       experiment_to_variation_map = @forced_variation_map[user_id]
-      # to be corrected
       experiment = project_config.get_experiment_from_key(experiment_key)
       experiment_id = experiment['id'] if experiment
       # check for nil and empty string experiment ID

spec/config/datafile_project_config_spec.rb

@@ -918,7 +918,7 @@
       it 'should log a message when there is no experiment key map for the experiment' do
         config.get_whitelisted_variations('invalid_key')
         expect(spy_logger).to have_received(:log).with(Logger::ERROR,
-                                                       "Experiment key 'invalid_key' is not in datafile.")
+                                                       "Experiment ID 'invalid_key' is not in datafile.")
       end
     end
 

spec/decision_service_spec.rb

@@ -181,10 +181,10 @@
                               "Starting to evaluate audience '11154' with conditions: [\"and\", [\"or\", [\"or\", {\"name\": \"browser_type\", \"type\": \"custom_attribute\", \"value\": \"firefox\"}]]].",
                               "Audience '11154' evaluated to FALSE.",
                               "Audiences for experiment 'test_experiment_with_audience' collectively evaluated to FALSE.",
-                              "User 'test_user' does not meet the conditions to be in experiment '122227'."
+                              "User 'test_user' does not meet the conditions to be in experiment 'test_experiment_with_audience'."
                             ])
       expect(spy_logger).to have_received(:log)
-        .once.with(Logger::INFO, "User 'test_user' does not meet the conditions to be in experiment '122227'.")
+        .once.with(Logger::INFO, "User 'test_user' does not meet the conditions to be in experiment 'test_experiment_with_audience'.")
 
       # should have checked forced variations
       expect(decision_service).to have_received(:get_whitelisted_variation_id).once
@@ -195,9 +195,9 @@
     it 'should return nil if the given experiment is not running' do
       variation_received, reasons = decision_service.get_variation(config, '100027', 'test_user')
       expect(variation_received).to eq(nil)
-      expect(reasons).to eq(["Experiment '100027' is not running."])
+      expect(reasons).to eq(["Experiment 'test_experiment_not_started' is not running."])
       expect(spy_logger).to have_received(:log)
-        .once.with(Logger::INFO, "Experiment '100027' is not running.")
+        .once.with(Logger::INFO, "Experiment 'test_experiment_not_started' is not running.")
 
       # non-running experiments should short circuit whitelisting
       expect(decision_service).not_to have_received(:get_whitelisted_variation_id)
@@ -313,10 +313,10 @@
         variation_received, reasons = decision_service.get_variation(config, '111127', 'test_user')
         expect(variation_received).to eq('111129')
         expect(reasons).to eq([
-                                "Returning previously activated variation ID 111129 of experiment '111127' for user 'test_user' from user profile."
+                                "Returning previously activated variation ID 111129 of experiment 'test_experiment' for user 'test_user' from user profile."
                               ])
         expect(spy_logger).to have_received(:log).once
-                                                 .with(Logger::INFO, "Returning previously activated variation ID 111129 of experiment '111127' for user 'test_user' from user profile.")
+                                                 .with(Logger::INFO, "Returning previously activated variation ID 111129 of experiment 'test_experiment' for user 'test_user' from user profile.")
 
         # saved user profiles should short circuit bucketing
         expect(decision_service.bucketer).not_to have_received(:bucket)

spec/project_spec.rb

@@ -717,7 +717,7 @@ def callback(_args); end
 
       project_instance.activate('test_experiment', 'test_user')
 
-      expect(spy_logger).to have_received(:log).once.with(Logger::INFO, "Activating user 'test_user' in experiment '111127'.")
+      expect(spy_logger).to have_received(:log).once.with(Logger::INFO, "Activating user 'test_user' in experiment 'test_experiment'.")
     end
 
     it 'should log when an exception has occurred during dispatching the impression event' do
@@ -1647,7 +1647,7 @@ def callback(_args); end
       allow(project_instance.decision_service).to receive(:get_variation_for_feature).and_return(decision_to_return)
 
       expect(project_instance.is_feature_enabled('multi_variate_feature', 'test_user')).to be true
-      expect(spy_logger).to have_received(:log).once.with(Logger::INFO, "Activating user 'test_user' in experiment '122230'.")
+      expect(spy_logger).to have_received(:log).once.with(Logger::INFO, "Activating user 'test_user' in experiment 'test_experiment_multivariate'.")
       expect(spy_logger).to have_received(:log).once.with(Logger::INFO, "Feature 'multi_variate_feature' is enabled for user 'test_user'.")
     end
 
@@ -3906,7 +3906,7 @@ def callback(_args); end
                 "Starting to evaluate audience '11154' with conditions: [\"and\", [\"or\", [\"or\", {\"name\": \"browser_type\", \"type\": \"custom_attribute\", \"value\": \"firefox\"}]]].",
                 "Audience '11154' evaluated to UNKNOWN.",
                 "Audiences for experiment 'test_experiment_multivariate' collectively evaluated to FALSE.",
-                "User 'user1' does not meet the conditions to be in experiment '122230'.",
+                "User 'user1' does not meet the conditions to be in experiment 'test_experiment_multivariate'.",
                 "The user 'user1' is not bucketed into any of the experiments on the feature 'multi_variate_feature'.",
                 "Feature flag 'multi_variate_feature' is not used in a rollout."
               ],
@@ -3922,7 +3922,7 @@ def callback(_args); end
               "Starting to evaluate audience '11154' with conditions: [\"and\", [\"or\", [\"or\", {\"name\": \"browser_type\", \"type\": \"custom_attribute\", \"value\": \"firefox\"}]]].",
               "Audience '11154' evaluated to UNKNOWN.",
               "Audiences for experiment 'test_experiment_multivariate' collectively evaluated to FALSE.",
-              "User 'user1' does not meet the conditions to be in experiment '122230'.",
+              "User 'user1' does not meet the conditions to be in experiment 'test_experiment_multivariate'.",
               "The user 'user1' is not bucketed into any of the experiments on the feature 'multi_variate_feature'.",
               "Feature flag 'multi_variate_feature' is not used in a rollout."
             ],
@@ -4255,7 +4255,7 @@ def callback(_args); end
               "Starting to evaluate audience '11154' with conditions: [\"and\", [\"or\", [\"or\", {\"name\": \"browser_type\", \"type\": \"custom_attribute\", \"value\": \"firefox\"}]]].",
               "Audience '11154' evaluated to UNKNOWN.",
               "Audiences for experiment 'test_experiment_multivariate' collectively evaluated to FALSE.",
-              "User 'user1' does not meet the conditions to be in experiment '122230'.",
+              "User 'user1' does not meet the conditions to be in experiment 'test_experiment_multivariate'.",
               "The user 'user1' is not bucketed into any of the experiments on the feature 'multi_variate_feature'.",
               "Feature flag 'multi_variate_feature' is not used in a rollout."
             ],
@@ -4271,7 +4271,7 @@ def callback(_args); end
             "Starting to evaluate audience '11154' with conditions: [\"and\", [\"or\", [\"or\", {\"name\": \"browser_type\", \"type\": \"custom_attribute\", \"value\": \"firefox\"}]]].",
             "Audience '11154' evaluated to UNKNOWN.",
             "Audiences for experiment 'test_experiment_multivariate' collectively evaluated to FALSE.",
-            "User 'user1' does not meet the conditions to be in experiment '122230'.",
+            "User 'user1' does not meet the conditions to be in experiment 'test_experiment_multivariate'.",
             "The user 'user1' is not bucketed into any of the experiments on the feature 'multi_variate_feature'.",
             "Feature flag 'multi_variate_feature' is not used in a rollout."
           ],