address more PR comments

Author: Mat001

optimizely/decision_service.py

@@ -354,44 +354,36 @@ def get_variation_for_rollout(self, project_config, feature, user, options):
             return Decision(None, None, enums.DecisionSources.ROLLOUT), decide_reasons
 
         rollout = project_config.get_rollout_from_id(feature.rolloutId)
-
-        if not rollout:
-            message = 'There is no rollout of feature {}.'.format(feature.key)
-            self.logger.debug(message)
-            decide_reasons.append(message)
-            return Decision(None, None, enums.DecisionSources.ROLLOUT), decide_reasons
-
         rollout_rules = project_config.get_rollout_experiments(rollout)
 
-        if not rollout_rules:
-            message = 'Rollout {} has no experiments.'.format(rollout.id)
+        if not rollout or not rollout_rules:
+            message = 'There is no rollout of feature {}.'.format(feature.key)
             self.logger.debug(message)
             decide_reasons.append(message)
             return Decision(None, None, enums.DecisionSources.ROLLOUT), decide_reasons
 
-        if rollout and len(rollout_rules) > 0:
-            index = 0
-            while index < len(rollout_rules):
-                decision_response, reasons_received = self.get_variation_from_delivery_rule(project_config,
-                                                                                            feature,
-                                                                                            rollout_rules, index, user,
-                                                                                            options)
+        index = 0
+        while index < len(rollout_rules):
+            decision_response, reasons_received = self.get_variation_from_delivery_rule(project_config,
+                                                                                        feature,
+                                                                                        rollout_rules, index, user,
+                                                                                        options)
 
-                decide_reasons += reasons_received
+            decide_reasons += reasons_received
 
-                variation, skip_to_everyone_else = decision_response
+            variation, skip_to_everyone_else = decision_response
 
-                if variation:
-                    rule = rollout_rules[index]
-                    feature_decision = Decision(experiment=rule, variation=variation,
-                                                source=enums.DecisionSources.ROLLOUT)
+            if variation:
+                rule = rollout_rules[index]
+                feature_decision = Decision(experiment=rule, variation=variation,
+                                            source=enums.DecisionSources.ROLLOUT)
 
-                    return feature_decision, decide_reasons
+                return feature_decision, decide_reasons
 
-                # the last rule is special for "Everyone Else"
-                index = len(rollout_rules) - 1 if skip_to_everyone_else else index + 1
+            # the last rule is special for "Everyone Else"
+            index = len(rollout_rules) - 1 if skip_to_everyone_else else index + 1
 
-            return Decision(None, None, enums.DecisionSources.ROLLOUT), decide_reasons
+        return Decision(None, None, enums.DecisionSources.ROLLOUT), decide_reasons
 
     def get_variation_from_experiment_rule(self, config, flag_key, rule, user, options):
         """ Checks for experiment rule if decision is forced and returns it.

optimizely/event/user_event_factory.py

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

optimizely/optimizely.py

@@ -1048,7 +1048,6 @@ def _decide(self, user_context, key, decide_options=None):
         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)
-
         variation, decision_reasons = forced_decision_response
         reasons += decision_reasons
 

optimizely/optimizely_user_context.py

@@ -46,7 +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()
-        self.forced_decisions = {}
+        self.forced_decisions_map = {}
         self.log = logger.SimpleLogger(min_level=enums.LogLevels.INFO)
 
     # decision context
@@ -73,8 +73,8 @@ def _clone(self):
         user_context = OptimizelyUserContext(self.client, self.user_id, self.get_user_attributes())
 
         with self.lock:
-            if self.forced_decisions:
-                user_context.forced_decisions = copy.deepcopy(self.forced_decisions)
+            if self.forced_decisions_map:
+                user_context.forced_decisions_map = copy.deepcopy(self.forced_decisions_map)
 
         return user_context
 
@@ -159,12 +159,12 @@ def set_forced_decision(self, decision_context, decision):
         Returns:
             True if the forced decision has been set successfully.
         """
-        if not self.client.config_manager.get_config():
+        if not self.client or not self.client.config_manager.get_config():
             self.log.logger.error(OptimizelyDecisionMessage.SDK_NOT_READY)
             return False
 
         with self.lock:
-            self.forced_decisions[decision_context] = decision
+            self.forced_decisions_map[decision_context] = decision
 
         return True
 
@@ -178,7 +178,7 @@ def get_forced_decision(self, decision_context):
         Returns:
             A forced_decision or None if forced decisions are not set for the parameters.
         """
-        if not self.client.config_manager.get_config():
+        if not self.client or not self.client.config_manager.get_config():
             self.log.logger.error(OptimizelyDecisionMessage.SDK_NOT_READY)
             return None
 
@@ -196,13 +196,13 @@ def remove_forced_decision(self, decision_context):
         Returns:
             Returns: true if the forced decision has been removed successfully.
         """
-        if not self.client.config_manager.get_config():
+        if not self.client or not self.client.config_manager.get_config():
             self.log.logger.error(OptimizelyDecisionMessage.SDK_NOT_READY)
             return False
 
         with self.lock:
-            if decision_context in self.forced_decisions:
-                del self.forced_decisions[decision_context]
+            if decision_context in self.forced_decisions_map:
+                del self.forced_decisions_map[decision_context]
                 return True
 
         return False
@@ -219,41 +219,41 @@ def remove_all_forced_decisions(self):
             return False
 
         with self.lock:
-            self.forced_decisions.clear()
+            self.forced_decisions_map.clear()
 
         return True
 
     def find_forced_decision(self, decision_context):
 
         with self.lock:
-            if not self.forced_decisions:
+            if not self.forced_decisions_map:
                 return None
 
             # must allow None to be returned for the Flags only case
-            return self.forced_decisions.get(decision_context)
+            return self.forced_decisions_map.get(decision_context)
 
     def find_validated_forced_decision(self, decision_context, options):
 
         reasons = []
 
-        forced_decision_response = self.find_forced_decision(decision_context)
+        forced_decision = self.find_forced_decision(decision_context)
 
         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)
+        if forced_decision:
+            variation = self.client.get_flag_variation_by_key(flag_key, forced_decision.variation_key)
             if variation:
                 if rule_key:
                     user_has_forced_decision = enums.ForcedDecisionLogs \
-                        .USER_HAS_FORCED_DECISION_WITH_RULE_SPECIFIED.format(forced_decision_response.variation_key,
+                        .USER_HAS_FORCED_DECISION_WITH_RULE_SPECIFIED.format(forced_decision.variation_key,
                                                                              flag_key,
                                                                              rule_key,
                                                                              self.user_id)
 
                 else:
                     user_has_forced_decision = enums.ForcedDecisionLogs \
-                        .USER_HAS_FORCED_DECISION_WITHOUT_RULE_SPECIFIED.format(forced_decision_response.variation_key,
+                        .USER_HAS_FORCED_DECISION_WITHOUT_RULE_SPECIFIED.format(forced_decision.variation_key,
                                                                                 flag_key,
                                                                                 self.user_id)
 

optimizely/project_config.py

@@ -224,7 +224,7 @@ def get_rollout_experiments(self, rollout):
         """
 
         rollout_experiments_id_map = self._generate_key_map(rollout.experiments, 'id', entities.Experiment)
-        rollout_experiments = [exper for exper in rollout_experiments_id_map.values()]
+        rollout_experiments = [experiment for experiment in rollout_experiments_id_map.values()]
 
         return rollout_experiments
 

tests/test_optimizely.py

@@ -5067,5 +5067,5 @@ def test_invalid_flag_key(self):
         """
         Tests invalid flag key in function get_flag_variation_by_key().
         """
-        # TODO mock function get_flag_variation_by_key?
+        # TODO mock function get_flag_variation_by_key
         pass

tests/test_user_context.py

@@ -1338,7 +1338,6 @@ def test_forced_decision_return_status__valid_datafile(self):
         status = user_context.remove_all_forced_decisions()
         self.assertTrue(status)
 
-    # TODO - EXAMPLE - THIS TEST IS NOW REFACTORED AND WORKS ----> FIX REMAINING THREE FAILING TESTS JUST LIKE THIS ONE (use flag "test_feature_in_experiment")
     def test_should_return_valid_decision_after_setting_and_removing_forced_decision(self):
         """
         Should return valid forced decision after setting and removing forced decision.
@@ -1592,10 +1591,7 @@ def test_invalid_experiment_rule_return_decision__forced_decision(self):
         # self.assertEqual(decide_decision.rule_key, '211147')
         # self.assertTrue(decide_decision.enabled)
 
-        # TODO - NEW UPDATED - are they supposed to be None ?
-        # TODO THIS DECISION IS DISABLED !!!!!! NOT WHAT TEST DOCSTRING SAYS !!!
-        #  - CHECK W JAE IF THIS TEST IS DOING WHAT IS SUPPOSED TO - IF DOCSTRING IS CORRECT
-        #  - SHOULD IT REALLY RETURN A VALID DECISION??? cause we get None and disabled decision.
+        # TODO - BELOW ARE NEW UPDATED - are they supposed to be None ?
         self.assertEqual(decide_decision.variation_key, None)
         self.assertEqual(decide_decision.rule_key, None)
         self.assertFalse(decide_decision.enabled)
@@ -1616,7 +1612,7 @@ def test_invalid_experiment_rule_return_decision__forced_decision(self):
         #     'User "test_user" bucketed into a targeting rule Everyone Else.'
         # ]
 
-        # TODO - NEW UPDATED REASONS
+        # TODO - BELOW ARE NEW UPDATED REASONS
         expected_reasons = [
             'Evaluating audiences for rule 1: ["11154"].', 'Audiences for rule 1 collectively evaluated to FALSE.',
             'User "test_user" does not meet audience conditions for targeting rule 1.',
@@ -1825,7 +1821,7 @@ def test_forced_decision_clone_return_valid_forced_decision(self):
         user_context_2 = user_context._clone()
         self.assertEqual(user_context_2.user_id, 'test_user')
         self.assertEqual(user_context_2.get_user_attributes(), {})
-        self.assertIsNotNone(user_context_2.forced_decisions)
+        self.assertIsNotNone(user_context_2.forced_decisions_map)
 
         self.assertEqual(user_context_2.get_forced_decision(context_with_flag).variation_key, 'v1')
         self.assertEqual(user_context_2.get_forced_decision(context_with_rule).variation_key, 'v2')