addressed more PR comments, fixed FSC test failuer about impressin events

Author: Mat001

optimizely/decision_service.py

@@ -21,6 +21,7 @@
 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')
@@ -224,9 +225,7 @@ def get_stored_variation(self, project_config, experiment, user_profile):
 
         return None
 
-    def get_variation(
-            self, project_config, experiment, user_context, ignore_user_profile=False
-    ):
+    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.
@@ -239,7 +238,7 @@ def get_variation(
       project_config: Instance of ProjectConfig.
       experiment: Experiment for which user variation needs to be determined.
       user_context: contains user id and attributes
-      ignore_user_profile: True to ignore the user profile lookup. Defaults to False.
+      options: Decide options.
 
     Returns:
       Variation user should see. None if user is not in experiment or experiment is not running
@@ -248,6 +247,11 @@ def get_variation(
         user_id = user_context.user_id
         attributes = user_context.get_user_attributes()
 
+        if options:
+            ignore_user_profile = OptimizelyDecideOption.IGNORE_USER_PROFILE_SERVICE in options
+        else:
+            ignore_user_profile = False
+
         decide_reasons = []
         # Check if experiment is running
         if not experiment_helper.is_experiment_running(experiment):
@@ -346,10 +350,7 @@ def get_variation_for_rollout(self, project_config, feature, user, options):
     """
         decide_reasons = []
 
-        if not feature:
-            return Decision(None, None, enums.DecisionSources.ROLLOUT), decide_reasons
-
-        if not feature.rolloutId:
+        if not feature or not feature.rolloutId:
             return Decision(None, None, enums.DecisionSources.ROLLOUT), decide_reasons
 
         rollout = project_config.get_rollout_from_id(feature.rolloutId)
@@ -378,12 +379,7 @@ def get_variation_for_rollout(self, project_config, feature, user, options):
 
                 decide_reasons += reasons_received
 
-                if not decision_response:
-                    # TODO - MATJAZ - careful - check how this exists the loop and terminates properly
-                    #  when return is hit
-                    return Decision(None, None, enums.DecisionSources.ROLLOUT), decide_reasons
-                else:
-                    variation, skip_to_everyone_else = decision_response
+                variation, skip_to_everyone_else = decision_response
 
                 if variation:
                     rule = rollout_rules[index]
@@ -513,15 +509,15 @@ def get_variation_from_delivery_rule(self, config, feature, rules, rule_index, u
 
         return (bucketed_variation, skip_to_everyone_else), decide_reasons
 
-    def get_variation_for_feature(self, project_config, feature, user_context, ignore_user_profile=False):
+    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.
-      ignore_user_profile: True if we should bypass the user profile service
+      options: Decide options.
 
     Returns:
       Decision namedtuple consisting of experiment and variation for the user.
@@ -535,15 +531,13 @@ def get_variation_for_feature(self, project_config, feature, user_context, ignor
                 experiment = project_config.get_experiment_from_id(experiment)
                 if experiment:
                     variation, variation_reasons = self.get_variation_from_experiment_rule(
-                        project_config, feature.key, experiment, user_context, ignore_user_profile)
+                        project_config, feature.key, experiment, user_context, options)
                     decide_reasons += variation_reasons
                     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, ignore_user_profile)
-
-        # check if not part of rollout
-        if not feature.rolloutId:
+            return self.get_variation_for_rollout(project_config, feature, user_context, options)
+        else:
             return Decision(None, None, enums.DecisionSources.ROLLOUT), decide_reasons

optimizely/event/user_event_factory.py

@@ -43,7 +43,7 @@ def create_impression_event(
     """
 
         if not activated_experiment and rule_type is not enums.DecisionSources.ROLLOUT:
-            return None
+            return None                                                 # TODO - we get this when event not sent. Now we allow None for activated_experiment
 
         variation, experiment_id = None, None
         if activated_experiment:
@@ -60,11 +60,11 @@ def create_impression_event(
         return user_event.ImpressionEvent(
             event_context,
             user_id,
-            activated_experiment,
+            activated_experiment,                       # TODO - check here also. that exper is used, that is not as None,  - or pass empty strings etc
             event_factory.EventFactory.build_attribute_list(user_attributes, project_config),
             variation,
             flag_key,
-            rule_key,
+            rule_key,           # TODO needs to be ampty string here - in relevnt APIs? or is variation that needs to be ampty string?
             rule_type,
             enabled,
             project_config.get_bot_filtering_value(),

optimizely/optimizely.py

@@ -306,9 +306,6 @@ def _get_feature_variable_for_type(
             )
 
         if decision.source == enums.DecisionSources.FEATURE_TEST:
-
-            print('DECISION.EXPERIMENT - ', decision.experiment)
-
             source_info = {
                 'experiment_key': decision.experiment.key,
                 'variation_key': decision.variation.key,
@@ -1046,7 +1043,6 @@ def _decide(self, user_context, key, decide_options=None):
         decision_source = DecisionSources.ROLLOUT
         source_info = {}
         decision_event_dispatched = False
-        ignore_ups = OptimizelyDecideOption.IGNORE_USER_PROFILE_SERVICE in decide_options
 
         # Check forced decisions first
         optimizely_decision_context = OptimizelyUserContext.OptimizelyDecisionContext(flag_key=key, rule_key=rule_key)
@@ -1062,7 +1058,7 @@ def _decide(self, user_context, key, decide_options=None):
             # Regular decision
             decision, decision_reasons = self.decision_service.get_variation_for_feature(config,
                                                                                          feature_flag,
-                                                                                         user_context, ignore_ups)
+                                                                                         user_context, decide_options)
 
             reasons += decision_reasons
 
@@ -1085,6 +1081,7 @@ def _decide(self, user_context, key, decide_options=None):
                 self._send_impression_event(config, experiment, variation, flag_key, rule_key or '',
                                             decision_source, feature_enabled,
                                             user_id, attributes)
+
                 decision_event_dispatched = True
 
         # Generate all variables map if decide options doesn't include excludeVariables