fixed further PR comments

Author: Mat001

optimizely/decision_service.py

@@ -16,12 +16,12 @@
 from six import string_types
 
 from . import bucketer
+from .decision.optimizely_decide_option import OptimizelyDecideOption
 from .helpers import audience as audience_helper
 from .helpers import enums
 from .helpers import experiment as experiment_helper
 from .helpers import validator
 from .optimizely_user_context import OptimizelyUserContext
-from .decision.optimizely_decide_option import OptimizelyDecideOption
 from .user_profile import UserProfile
 
 Decision = namedtuple('Decision', 'experiment variation source')
@@ -44,14 +44,14 @@ def __init__(self, logger, user_profile_service):
     def _get_bucketing_id(self, user_id, attributes):
         """ Helper method to determine bucketing ID for the user.
 
-    Args:
-      user_id: ID for user.
-      attributes: Dict representing user attributes. May consist of bucketing ID to be used.
+        Args:
+          user_id: ID for user.
+          attributes: Dict representing user attributes. May consist of bucketing ID to be used.
 
-    Returns:
-      String representing bucketing ID if it is a String type in attributes else return user ID
-      array of log messages representing decision making.
-    """
+        Returns:
+          String representing bucketing ID if it is a String type in attributes else return user ID
+          array of log messages representing decision making.
+        """
         decide_reasons = []
         attributes = attributes or {}
         bucketing_id = attributes.get(enums.ControlAttributes.BUCKETING_ID)
@@ -68,15 +68,15 @@ def _get_bucketing_id(self, user_id, attributes):
     def set_forced_variation(self, project_config, experiment_key, user_id, variation_key):
         """ Sets users to a map of experiments to forced variations.
 
-      Args:
-        project_config: Instance of ProjectConfig.
-        experiment_key: Key for experiment.
-        user_id: The user ID.
-        variation_key: Key for variation. If None, then clear the existing experiment-to-variation mapping.
+          Args:
+            project_config: Instance of ProjectConfig.
+            experiment_key: Key for experiment.
+            user_id: The user ID.
+            variation_key: Key for variation. If None, then clear the existing experiment-to-variation mapping.
 
-      Returns:
-        A boolean value that indicates if the set completed successfully.
-    """
+          Returns:
+            A boolean value that indicates if the set completed successfully.
+        """
         experiment = project_config.get_experiment_from_key(experiment_key)
         if not experiment:
             # The invalid experiment key will be logged inside this call.
@@ -126,15 +126,15 @@ def set_forced_variation(self, project_config, experiment_key, user_id, variatio
     def get_forced_variation(self, project_config, experiment_key, user_id):
         """ Gets the forced variation key for the given user and experiment.
 
-      Args:
-        project_config: Instance of ProjectConfig.
-        experiment_key: Key for experiment.
-        user_id: The user ID.
+          Args:
+            project_config: Instance of ProjectConfig.
+            experiment_key: Key for experiment.
+            user_id: The user ID.
 
-      Returns:
-        The variation which the given user and experiment should be forced into and
-         array of log messages representing decision making.
-    """
+          Returns:
+            The variation which the given user and experiment should be forced into and
+             array of log messages representing decision making.
+        """
         decide_reasons = []
         if user_id not in self.forced_variation_map:
             message = 'User "%s" is not in the forced variation map.' % user_id
@@ -174,15 +174,15 @@ def get_whitelisted_variation(self, project_config, experiment, user_id):
         """ Determine if a user is forced into a variation (through whitelisting)
         for the given experiment and return that variation.
 
-    Args:
-      project_config: Instance of ProjectConfig.
-      experiment: Object representing the experiment for which user is to be bucketed.
-      user_id: ID for the user.
+        Args:
+          project_config: Instance of ProjectConfig.
+          experiment: Object representing the experiment for which user is to be bucketed.
+          user_id: ID for the user.
 
-    Returns:
-      Variation in which the user with ID user_id is forced into. None if no variation and
-       array of log messages representing decision making.
-    """
+        Returns:
+          Variation in which the user with ID user_id is forced into. None if no variation and
+           array of log messages representing decision making.
+        """
         decide_reasons = []
         forced_variations = experiment.forcedVariations
 
@@ -202,14 +202,14 @@ def get_whitelisted_variation(self, project_config, experiment, user_id):
     def get_stored_variation(self, project_config, experiment, user_profile):
         """ Determine if the user has a stored variation available for the given experiment and return that.
 
-    Args:
-      project_config: Instance of ProjectConfig.
-      experiment: Object representing the experiment for which user is to be bucketed.
-      user_profile: UserProfile object representing the user's profile.
+        Args:
+          project_config: Instance of ProjectConfig.
+          experiment: Object representing the experiment for which user is to be bucketed.
+          user_profile: UserProfile object representing the user's profile.
 
-    Returns:
-      Variation if available. None otherwise.
-    """
+        Returns:
+          Variation if available. None otherwise.
+        """
         user_id = user_profile.user_id
         variation_id = user_profile.get_variation_for_experiment(experiment.id)
 
@@ -228,22 +228,22 @@ def get_stored_variation(self, project_config, experiment, user_profile):
     def get_variation(self, project_config, experiment, user_context, options=None):
         """ Top-level function to help determine variation user should be put in.
 
-    First, check if experiment is running.
-    Second, check if user is forced in a variation.
-    Third, check if there is a stored decision for the user and return the corresponding variation.
-    Fourth, figure out if user is in the experiment by evaluating audience conditions if any.
-    Fifth, bucket the user and return the variation.
-
-    Args:
-      project_config: Instance of ProjectConfig.
-      experiment: Experiment for which user variation needs to be determined.
-      user_context: contains user id and attributes
-      options: Decide options.
-
-    Returns:
-      Variation user should see. None if user is not in experiment or experiment is not running
-      And an array of log messages representing decision making.
-    """
+        First, check if experiment is running.
+        Second, check if user is forced in a variation.
+        Third, check if there is a stored decision for the user and return the corresponding variation.
+        Fourth, figure out if user is in the experiment by evaluating audience conditions if any.
+        Fifth, bucket the user and return the variation.
+
+        Args:
+          project_config: Instance of ProjectConfig.
+          experiment: Experiment for which user variation needs to be determined.
+          user_context: contains user id and attributes
+          options: Decide options.
+
+        Returns:
+          Variation user should see. None if user is not in experiment or experiment is not running
+          And an array of log messages representing decision making.
+        """
         user_id = user_context.user_id
         attributes = user_context.get_user_attributes()
 
@@ -333,21 +333,21 @@ def get_variation(self, project_config, experiment, user_context, options=None):
         decide_reasons.append(message)
         return None, decide_reasons
 
-    def get_variation_for_rollout(self, project_config, feature, user, options):
+    def get_variation_for_rollout(self, project_config, feature, user):
         """ Determine which experiment/variation the user is in for a given rollout.
             Returns the variation of the first experiment the user qualifies for.
 
-    Args:
-      project_config: Instance of ProjectConfig.
-      flagKey: Feature key.
-      rollout: Rollout for which we are getting the variation.
-      user: ID and attributes for user.
-      options: Decide options.
+        Args:
+          project_config: Instance of ProjectConfig.
+          flagKey: Feature key.
+          rollout: Rollout for which we are getting the variation.
+          user: ID and attributes for user.
+          options: Decide options.
 
-    Returns:
-      Decision namedtuple consisting of experiment and variation for the user and
-      array of log messages representing decision making.
-    """
+        Returns:
+          Decision namedtuple consisting of experiment and variation for the user and
+          array of log messages representing decision making.
+        """
         decide_reasons = []
 
         if not feature or not feature.rolloutId:
@@ -366,8 +366,7 @@ def get_variation_for_rollout(self, project_config, feature, user, options):
         while index < len(rollout_rules):
             decision_response, reasons_received = self.get_variation_from_delivery_rule(project_config,
                                                                                         feature,
-                                                                                        rollout_rules, index, user,
-                                                                                        options)
+                                                                                        rollout_rules, index, user)
 
             decide_reasons += reasons_received
 
@@ -406,8 +405,7 @@ def get_variation_from_experiment_rule(self, config, flag_key, rule, user, optio
         optimizely_decision_context = OptimizelyUserContext.OptimizelyDecisionContext(flag_key, rule.key)
 
         forced_decision_variation, reasons_received = user.find_validated_forced_decision(
-            optimizely_decision_context,
-            options)
+            optimizely_decision_context)
         decide_reasons += reasons_received
 
         if forced_decision_variation:
@@ -418,7 +416,7 @@ def get_variation_from_experiment_rule(self, config, flag_key, rule, user, optio
         decide_reasons += variation_reasons
         return decision_variation, decide_reasons
 
-    def get_variation_from_delivery_rule(self, config, feature, rules, rule_index, user, options):
+    def get_variation_from_delivery_rule(self, config, feature, rules, rule_index, user):
         """ Checks for delivery rule if decision is forced and returns it.
             Otherwise returns a regular decision.
 
@@ -428,7 +426,6 @@ def get_variation_from_delivery_rule(self, config, feature, rules, rule_index, u
           rules: Experiment rule.
           rule_index: integer index of the rule in the list.
           user: ID and attributes for user.
-          options: Decide options.
 
         Returns:
           If forced decision, it returns namedtuple consisting of forced_decision_variation and skip_to_everyone_else
@@ -444,8 +441,7 @@ def get_variation_from_delivery_rule(self, config, feature, rules, rule_index, u
         # check forced decision first
         rule = rules[rule_index]
         optimizely_decision_context = OptimizelyUserContext.OptimizelyDecisionContext(feature.key, rule.key)
-        forced_decision_variation, reasons_received = user.find_validated_forced_decision(optimizely_decision_context,
-                                                                                          options)
+        forced_decision_variation, reasons_received = user.find_validated_forced_decision(optimizely_decision_context)
 
         decide_reasons += reasons_received
 
@@ -504,15 +500,15 @@ def get_variation_from_delivery_rule(self, config, feature, rules, rule_index, u
     def get_variation_for_feature(self, project_config, feature, user_context, options=None):
         """ Returns the experiment/variation the user is bucketed in for the given feature.
 
-    Args:
-      project_config: Instance of ProjectConfig.
-      feature: Feature for which we are determining if it is enabled or not for the given user.
-      user: user context for user.
-      attributes: Dict representing user attributes.
-      options: Decide options.
+        Args:
+          project_config: Instance of ProjectConfig.
+          feature: Feature for which we are determining if it is enabled or not for the given user.
+          user: user context for user.
+          attributes: Dict representing user attributes.
+          options: Decide options.
 
-    Returns:
-      Decision namedtuple consisting of experiment and variation for the user.
+        Returns:
+          Decision namedtuple consisting of experiment and variation for the user.
     """
         decide_reasons = []
 
@@ -528,8 +524,4 @@ def get_variation_for_feature(self, project_config, feature, user_context, optio
                     if variation:
                         return Decision(experiment, variation, enums.DecisionSources.FEATURE_TEST), decide_reasons
 
-        # Next check if user is part of a rollout
-        if feature.rolloutId:
-            return self.get_variation_for_rollout(project_config, feature, user_context, options)
-        else:
-            return Decision(None, None, enums.DecisionSources.ROLLOUT), decide_reasons
+        return self.get_variation_for_rollout(project_config, feature, user_context)

optimizely/event/user_event_factory.py

@@ -53,7 +53,7 @@ def create_impression_event(
             variation = project_config.get_variation_from_id_by_experiment_id(experiment_id, variation_id)
         # need this condition when we send events involving forced decisions
         elif variation_id and flag_key:
-            variation = project_config.get_flag_variation_by_id(flag_key, variation_id)
+            variation = project_config.get_flag_variation(flag_key, 'id', variation_id)
         event_context = user_event.EventContext(
             project_config.account_id, project_config.project_id, project_config.revision, project_config.anonymize_ip,
         )

optimizely/optimizely.py

@@ -1036,8 +1036,7 @@ def _decide(self, user_context, key, decide_options=None):
 
         # Check forced decisions first
         optimizely_decision_context = OptimizelyUserContext.OptimizelyDecisionContext(flag_key=key, rule_key=rule_key)
-        forced_decision_response = user_context.find_validated_forced_decision(optimizely_decision_context,
-                                                                               options=decide_options)
+        forced_decision_response = user_context.find_validated_forced_decision(optimizely_decision_context)
         variation, decision_reasons = forced_decision_response
         reasons += decision_reasons
 
@@ -1184,31 +1183,3 @@ def _decide_for_keys(self, user_context, keys, decide_options=None):
                 continue
             decisions[key] = decision
         return decisions
-
-    def get_flag_variation_by_key(self, flag_key, variation_key):
-        """
-        Gets variation by key.
-        variation_key can be a string or in case of forced decisions, it can be an object.
-
-        Args:
-            flag_key: flag key
-            variation_key: variation key
-
-        Returns:
-            Variation as a map.
-        """
-        config = self.config_manager.get_config()
-
-        if not config:
-            return None
-
-        if not flag_key:
-            return None
-
-        variations = config.flag_variations_map.get(flag_key)
-
-        for variation in variations:
-            if variation.key == variation_key:
-                return variation
-
-        return None

optimizely/optimizely_user_context.py

@@ -51,6 +51,10 @@ def __init__(self, optimizely_client, user_id, user_attributes=None):
 
     # decision context
     class OptimizelyDecisionContext(object):
+        """ Using class with attributes here instead of namedtuple because
+            class is extensible, it's easy to add another attribute if we wanted
+            to extend decision context.
+        """
         def __init__(self, flag_key, rule_key=None):
             self.flag_key = flag_key
             self.rule_key = rule_key
@@ -184,7 +188,7 @@ def get_forced_decision(self, decision_context):
 
         forced_decision = self.find_forced_decision(decision_context)
 
-        return forced_decision if forced_decision else None
+        return forced_decision
 
     def remove_forced_decision(self, decision_context):
         """
@@ -224,16 +228,32 @@ def remove_all_forced_decisions(self):
         return True
 
     def find_forced_decision(self, decision_context):
+        """
+        Gets forced decision from forced decision map.
+
+        Args:
+            decision_context: a decision context.
 
+        Returns:
+            Forced decision.
+        """
         with self.lock:
             if not self.forced_decisions_map:
                 return None
 
             # must allow None to be returned for the Flags only case
             return self.forced_decisions_map.get(decision_context)
 
-    def find_validated_forced_decision(self, decision_context, options):
+    def find_validated_forced_decision(self, decision_context):
+        """
+        Gets forced decisions based on flag key, rule key and variation.
+
+        Args:
+            decision context: a decision context
 
+        Returns:
+            Variation of the forced decision.
+        """
         reasons = []
 
         forced_decision = self.find_forced_decision(decision_context)
@@ -242,7 +262,10 @@ def find_validated_forced_decision(self, decision_context, options):
         rule_key = decision_context.rule_key
 
         if forced_decision:
-            variation = self.client.get_flag_variation_by_key(flag_key, forced_decision.variation_key)
+            # we use config here so we can use get_flag_variation() function which is defined in project_config
+            # otherwise we would us self.client instead of config
+            config = self.client.config_manager.get_config() if self.client else None
+            variation = config.get_flag_variation(flag_key, 'key', forced_decision.variation_key)
             if variation:
                 if rule_key:
                     user_has_forced_decision = enums.ForcedDecisionLogs \