address PR comments

Mat001 · Mat001 · commit a71f50e6851d · 2021-11-15T16:12:00.000-08:00
diff --git a/optimizely/decision_service.py b/optimizely/decision_service.py
@@ -357,7 +357,7 @@ def get_variation_for_rollout(self, project_config, feature, user, options):
             decide_reasons.append(message)
             return Decision(None, None, enums.DecisionSources.ROLLOUT), decide_reasons
 
-        rollout_rules = project_config.get_rollout_experiments_map(rollout)
+        rollout_rules = project_config.get_rollout_experiments(rollout)
 
         if not rollout_rules:
             message = 'Rollout {} has no experiments.'.format(rollout.id)
diff --git a/optimizely/optimizely.py b/optimizely/optimizely.py
@@ -444,20 +444,20 @@ def activate(self, experiment_key, user_id, attributes=None):
             self.logger.error(enums.Errors.INVALID_PROJECT_CONFIG.format('activate'))
             return None
 
-        variation_key = self.get_variation(experiment_key, user_id, attributes)
+        variation = self.get_variation(experiment_key, user_id, attributes)
 
-        # check for case where variation_key can be None when attributes are invalid
-        if not variation_key:
+        if not variation:
             self.logger.info('Not activating user "%s".' % user_id)
             return None
 
-        # variation_key is normally a tuple object
-        if not variation_key[0]:
+        variation_key, reasons = variation
+
+        if not variation_key:
             self.logger.info('Not activating user "%s".' % user_id)
             return None
 
         experiment = project_config.get_experiment_from_key(experiment_key)
-        variation = project_config.get_variation_from_key(experiment_key, variation_key)
+        variation = project_config.get_variation_from_key(experiment_key, variation_key.key)
 
         # Create and dispatch impression event
         self.logger.info('Activating user "%s" in experiment "%s".' % (user_id, experiment.key))
@@ -1040,8 +1040,8 @@ def _decide(self, user_context, key, decide_options=None):
         forced_decision_response = user_context.find_validated_forced_decision(optimizely_decision_context,
                                                                                options=decide_options)
 
-        variation, received_response = forced_decision_response
-        reasons += received_response
+        variation, decision_reasons = forced_decision_response
+        reasons += decision_reasons
 
         if variation:
             decision = Decision(None, variation, enums.DecisionSources.FEATURE_TEST)
@@ -1203,12 +1203,10 @@ def get_flag_variation_by_key(self, flag_key, variation_key):
         if not config:
             return None
 
-        # this will take care of force decision objects which contain variation_key inside them
-        if isinstance(variation_key, OptimizelyUserContext.OptimizelyForcedDecision):
-            variation_key = variation_key.variation_key
-
         variations = config.flag_variations_map[flag_key]
 
         for variation in variations:
             if variation.key == variation_key:
                 return variation
+
+        return None
diff --git a/optimizely/optimizely_user_context.py b/optimizely/optimizely_user_context.py
@@ -46,8 +46,7 @@ def __init__(self, optimizely_client, user_id, user_attributes=None):
 
         self._user_attributes = user_attributes.copy() if user_attributes else {}
         self.lock = threading.Lock()
-        with self.lock:
-            self.forced_decisions = {}
+        self.forced_decisions = {}
         self.log = logger.SimpleLogger(min_level=enums.LogLevels.INFO)
 
     # decision context
@@ -149,70 +148,61 @@ def as_json(self):
             'attributes': self.get_user_attributes(),
         }
 
-    def set_forced_decision(self, OptimizelyDecisionContext, OptimizelyForcedDecision):
+    def set_forced_decision(self, decision_context, decision):
         """
         Sets the forced decision for a given decision context.
 
         Args:
-            OptimizelyDecisionContext: a decision context.
-            OptimizelyForcedDecision: a forced decision.
+            decision_context: a decision context.
+            decision: a forced decision.
 
         Returns:
             True if the forced decision has been set successfully.
         """
-        config = self.client.get_optimizely_config()
-
-        if self.client is None or config is None:
+        if not self.client.config_manager.get_config():
             self.log.logger.error(OptimizelyDecisionMessage.SDK_NOT_READY)
             return False
 
-        context = OptimizelyDecisionContext
-        decision = OptimizelyForcedDecision
-
         with self.lock:
-            self.forced_decisions[context] = decision
+            self.forced_decisions[decision_context] = decision
 
         return True
 
-    def get_forced_decision(self, OptimizelyDecisionContext):
+    def get_forced_decision(self, decision_context):
         """
         Gets the forced decision (variation key) for a given decision context.
 
         Args:
-            OptimizelyDecisionContext: a decision context.
+            decision_context: a decision context.
 
         Returns:
             A variation key or None if forced decisions are not set for the parameters.
         """
-        config = self.client.get_optimizely_config()
-
-        if self.client is None or config is None:
+        if not self.client.config_manager.get_config():
             self.log.logger.error(OptimizelyDecisionMessage.SDK_NOT_READY)
             return None
 
-        forced_decision_key = self.find_forced_decision(OptimizelyDecisionContext)
+        forced_decision_key = self.find_forced_decision(decision_context)
 
         return forced_decision_key if forced_decision_key else None
 
-    def remove_forced_decision(self, OptimizelyDecisionContext):
+    def remove_forced_decision(self, decision_context):
         """
         Removes the forced decision for a given flag and an optional rule.
 
         Args:
-            OptimizelyDecisionContext: a decision context.
+            decision_context: a decision context.
 
         Returns:
             Returns: true if the forced decision has been removed successfully.
         """
-        config = self.client.get_optimizely_config()
-
-        if self.client is None or config is None:
+        if not self.client.config_manager.get_config():
             self.log.logger.error(OptimizelyDecisionMessage.SDK_NOT_READY)
             return False
 
         with self.lock:
-            if OptimizelyDecisionContext in self.forced_decisions.keys():
-                del self.forced_decisions[OptimizelyDecisionContext]
+            if decision_context in self.forced_decisions.keys():
+                del self.forced_decisions[decision_context]
                 return True
 
         return False
@@ -224,9 +214,7 @@ def remove_all_forced_decisions(self):
         Returns:
             True if forced decisions have been removed successfully.
         """
-        config = self.client.get_optimizely_config()
-
-        if self.client is None or config is None:
+        if not self.client.config_manager.get_config():
             self.log.logger.error(OptimizelyDecisionMessage.SDK_NOT_READY)
             return False
 
@@ -235,23 +223,23 @@ def remove_all_forced_decisions(self):
 
         return True
 
-    def find_forced_decision(self, OptimizelyDecisionContext):
+    def find_forced_decision(self, decision_context):
 
         with self.lock:
             if not self.forced_decisions:
                 return None
 
             # must allow None to be returned for the Flags only case
-            return self.forced_decisions.get(OptimizelyDecisionContext)
+            return self.forced_decisions.get(decision_context)
 
-    def find_validated_forced_decision(self, OptimizelyDecisionContext, options):
+    def find_validated_forced_decision(self, decision_context, options):
 
         reasons = []
 
-        forced_decision_response = self.find_forced_decision(OptimizelyDecisionContext)
+        forced_decision_response = self.find_forced_decision(decision_context)
 
-        flag_key = OptimizelyDecisionContext.flag_key
-        rule_key = OptimizelyDecisionContext.rule_key
+        flag_key = decision_context.flag_key
+        rule_key = decision_context.rule_key
 
         if forced_decision_response:
             variation = self.client.get_flag_variation_by_key(flag_key, forced_decision_response.variation_key)
diff --git a/optimizely/project_config.py b/optimizely/project_config.py
@@ -150,7 +150,7 @@ def __init__(self, datafile, logger, error_handler):
             if not flag['rolloutId'] == '':
                 rollout = self.rollout_id_map[flag['rolloutId']]
 
-                rollout_experiments = self.get_rollout_experiments_map(rollout)
+                rollout_experiments = self.get_rollout_experiments(rollout)
 
                 if rollout and rollout.experiments:
                     experiments.extend(rollout_experiments)
@@ -213,7 +213,7 @@ def _deserialize_audience(audience_map):
 
         return audience_map
 
-    def get_rollout_experiments_map(self, rollout):
+    def get_rollout_experiments(self, rollout):
         """ Helper method to get rollout experiments as a map.
 
         Args:
@@ -387,8 +387,8 @@ def get_audience(self, audience_id):
         self.logger.error('Audience ID "%s" is not in datafile.' % audience_id)
         self.error_handler.handle_error(exceptions.InvalidAudienceException((enums.Errors.INVALID_AUDIENCE)))
 
-    def get_variation_from_key(self, experiment_key, variation):
-        """ Get variation given experiment and variation.
+    def get_variation_from_key(self, experiment_key, variation_key):
+        """ Get variation given experiment and variation key.
 
         Args:
             experiment: Key representing parent experiment of variation.
@@ -399,28 +399,20 @@ def get_variation_from_key(self, experiment_key, variation):
             Object representing the variation.
         """
 
-        variation_key = None
+        variation_map = self.variation_key_map.get(experiment_key)
 
-        if isinstance(variation, tuple):
-            if isinstance(variation[0], entities.Variation):
-                variation_key, received_reasons = variation
-        else:
-            variation_map = self.variation_key_map.get(experiment_key)
-
-            if variation_map:
-                variation_key = variation_map.get(variation)
+        if variation_map:
+            variation = variation_map.get(variation_key)
+            if variation:
+                return variation
             else:
-                self.logger.error('Experiment key "%s" is not in datafile.' % experiment_key)
-                self.error_handler.handle_error(
-                    exceptions.InvalidExperimentException(enums.Errors.INVALID_EXPERIMENT_KEY))
+                self.logger.error('Variation key "%s" is not in datafile.' % variation_key)
+                self.error_handler.handle_error(exceptions.InvalidVariationException(enums.Errors.INVALID_VARIATION))
                 return None
 
-        if variation_key:
-            return variation_key
-        else:
-            self.logger.error('Variation key "%s" is not in datafile.' % variation)
-            self.error_handler.handle_error(exceptions.InvalidVariationException(enums.Errors.INVALID_VARIATION))
-            return None
+        self.logger.error('Experiment key "%s" is not in datafile.' % experiment_key)
+        self.error_handler.handle_error(exceptions.InvalidExperimentException(enums.Errors.INVALID_EXPERIMENT_KEY))
+        return None
 
     def get_variation_from_id(self, experiment_key, variation_id):
         """ Get variation given experiment and variation ID.
diff --git a/tests/test_user_context.py b/tests/test_user_context.py
