addressed more PR comments

Author: Mat001

optimizely/project_config.py

@@ -124,27 +124,26 @@ def __init__(self, datafile, logger, error_handler):
         # As we cannot create json variables in datafile directly, here we convert
         # the variables of string type and json subType to json type
         # This is needed to fully support json variables
-        for feature in self.feature_key_map:
-            for variable in self.feature_key_map[feature].variables:
+        self.experiment_feature_map = {}
+        self.flag_rules_map = {}
+
+        for feature_key, feature_value in self.feature_key_map.items():
+            for variable in self.feature_key_map[feature_key].variables:
                 sub_type = variable.get('subType', '')
                 if variable['type'] == entities.Variable.Type.STRING and sub_type == entities.Variable.Type.JSON:
                     variable['type'] = entities.Variable.Type.JSON
 
-        # Dict containing map of experiment ID to feature ID.
-        # for checking that experiment is a feature experiment or not.
-        self.experiment_feature_map = {}
-        for feature in self.feature_key_map.values():
-            feature.variables = self._generate_key_map(feature.variables, 'key', entities.Variable)
-            for exp_id in feature.experimentIds:
+            # loop over features=flags already happening
+            # get feature variables for eacg flag/feature
+            feature_value.variables = self._generate_key_map(feature_value.variables, 'key', entities.Variable)
+            for exp_id in feature_value.experimentIds:
                 # Add this experiment in experiment-feature map.
-                self.experiment_feature_map[exp_id] = [feature.id]
+                self.experiment_feature_map[exp_id] = [feature_value.id]
 
         # all rules(experiment rules and delivery rules) for each flag
-        self.flag_rules_map = {}
         for flag in self.feature_flags:
-
             experiments = []
-            if not flag['experimentIds'] == '':
+            if len(flag['experimentIds']) > 0:
                 for exp_id in flag['experimentIds']:
                     experiments.append(self.experiment_id_map[exp_id])
             if not flag['rolloutId'] == '':
@@ -160,19 +159,7 @@ def __init__(self, datafile, logger, error_handler):
         # All variations for each flag
         # Datafile does not contain a separate entity for this.
         # We collect variations used in each rule (experiment rules and delivery rules)
-        self.flag_variations_map = {}
-
-        for flag_key, rules in self.flag_rules_map.items():
-            variations = []
-            for rule in rules:
-                # get variations as objects (rule.variations gives list)
-
-                variation_objects = self.variation_id_map_by_experiment_id[rule.id].values()
-                for variation in variation_objects:
-                    if variation.id not in [var.id for var in variations]:
-                        variations.append(variation)
-
-            self.flag_variations_map[flag_key] = variations
+        self.flag_variations_map = self.get_all_variations_for_each_rule(self.flag_rules_map)
 
     @staticmethod
     def _generate_key_map(entity_list, key, entity_class):
@@ -674,3 +661,29 @@ def get_flag_variation_by_id(self, flag_key, variation_id):
                 return variation
 
         return None
+
+    def get_all_variations_for_each_rule(self, flag_rules_map):
+        """ Helper method to get all variation objects from each flag.
+            collects variations used in each rule (experiment rules and delivery rules).
+
+        Args:
+            flag_rules_map: A dictionary. A map of all rules per flag.
+
+        Returns:
+            Map of flag variations.
+        """
+        flag_variations_map = {}
+
+        for flag_key, rules in flag_rules_map.items():
+            variations = []
+            for rule in rules:
+                # get variations as objects (rule.variations gives list)
+                variation_objects = self.variation_id_map_by_experiment_id[rule.id].values()
+                for variation in variation_objects:
+                    # append variation if it's not already in the list
+                    if variation not in variations:
+                        variations.append(variation)
+
+            flag_variations_map[flag_key] = variations
+
+        return flag_variations_map

tests/test_user_context.py

@@ -1568,8 +1568,10 @@ def test_should_return_valid_decision_after_setting_invalid_delivery_rule_variat
             'rule (211127) and user (test_user) in the forced decision map.'
         ])))
 
-    # TODO - JAE: Can we change the test name and description? Not clear which part is invalid. Also, I see the forced set flag and decide flag is different. Is it intentional?
-    def test_invalid_experiment_rule_return_decision__forced_decision(self):            # TODO - CHECK WITH JAE if this test should return valid decision like docstring says!
+    # TODO - JAE: Can we change the test name and description? Not clear which part is invalid.
+    #  Also, I see the forced set flag and decide flag is different. Is it intentional?
+    # TODO - CHECK WITH JAE if this test should return valid decision like docstring says!
+    def test_invalid_experiment_rule_return_decision__forced_decision(self):
         """
         Should return valid decision after setting invalid experiment
         rule variation in forced decision.
@@ -1596,7 +1598,6 @@ def test_invalid_experiment_rule_return_decision__forced_decision(self):
         self.assertEqual(decide_decision.rule_key, None)
         self.assertFalse(decide_decision.enabled)
 
-
         self.assertEqual(decide_decision.flag_key, 'test_feature_in_rollout')
         self.assertEqual(decide_decision.user_context.user_id, 'test_user')
         self.assertEqual(decide_decision.user_context.get_user_attributes(), {})