Refact: Refactored Forced decision (#365)

* project config refactor

* use existing loop to generate flag_variation_map

* get_variation_from_experiment_rule and get_variation_from_delivery_rule removed

* fsc test fix

* comment addressed

* commented code removed

* comments from main forced decision PR resolved

Co-authored-by: ozayr-zaviar <uzairzaviar@gmail.com>

Author: msohailhussain

optimizely/decision_service.py

@@ -349,153 +349,94 @@ def get_variation_for_rollout(self, project_config, feature, user):
           array of log messages representing decision making.
         """
         decide_reasons = []
+        user_id = user.user_id
+        attributes = user.get_user_attributes()
 
         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)
-        rollout_rules = project_config.get_rollout_experiments(rollout)
 
-        if not rollout or not rollout_rules:
+        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)
+            self.logger.debug(message)
+            decide_reasons.append(message)
+            return Decision(None, None, enums.DecisionSources.ROLLOUT), decide_reasons
+
         index = 0
         while index < len(rollout_rules):
-            decision_response, reasons_received = self.get_variation_from_delivery_rule(project_config,
-                                                                                        feature,
-                                                                                        rollout_rules, index, user)
+            skip_to_everyone_else = False
 
+            # check forced decision first
+            rule = rollout_rules[index]
+            optimizely_decision_context = OptimizelyUserContext.OptimizelyDecisionContext(feature.key, rule.key)
+            forced_decision_variation, reasons_received = user.find_validated_forced_decision(
+                optimizely_decision_context)
             decide_reasons += reasons_received
 
-            variation, skip_to_everyone_else = decision_response
-
-            if variation:
-                rule = rollout_rules[index]
-                feature_decision = Decision(experiment=rule, variation=variation,
-                                            source=enums.DecisionSources.ROLLOUT)
-
-                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
-
-        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.
-            Otherwise returns a regular decision.
-
-        Args:
-          config: Instance of ProjectConfig.
-          flag_key: Key of the flag.
-          rule: Experiment rule.
-          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.
-        """
-        decide_reasons = []
-
-        # check forced decision first
-        optimizely_decision_context = OptimizelyUserContext.OptimizelyDecisionContext(flag_key, rule.key)
-
-        forced_decision_variation, reasons_received = user.find_validated_forced_decision(
-            optimizely_decision_context)
-        decide_reasons += reasons_received
-
-        if forced_decision_variation:
-            return forced_decision_variation, decide_reasons
-
-        # regular decision
-        decision_variation, variation_reasons = self.get_variation(config, rule, user, options)
-        decide_reasons += variation_reasons
-        return decision_variation, decide_reasons
-
-    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.
+            if forced_decision_variation:
+                return Decision(experiment=rule, variation=forced_decision_variation,
+                                source=enums.DecisionSources.ROLLOUT), decide_reasons
 
-        Args:
-          config: Instance of ProjectConfig.
-          flag_key: Key of the flag.
-          rules: Experiment rule.
-          rule_index: integer index of the rule in the list.
-          user: ID and attributes for user.
+            bucketing_id, bucket_reasons = self._get_bucketing_id(user_id, attributes)
+            decide_reasons += bucket_reasons
 
-        Returns:
-          If forced decision, it returns namedtuple consisting of forced_decision_variation and skip_to_everyone_else
-          and decision reason log messages.
+            everyone_else = (index == len(rollout_rules) - 1)
+            logging_key = "Everyone Else" if everyone_else else str(index + 1)
 
-          If regular decision it returns a tuple of bucketed_variation and skip_to_everyone_else
-          and decision reason log messages
-        """
-        decide_reasons = []
-        skip_to_everyone_else = False
-        bucketed_variation = None
+            rollout_rule = project_config.get_experiment_from_id(rule.id)
+            audience_conditions = rollout_rule.get_audience_conditions_or_ids()
 
-        # 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)
+            audience_decision_response, reasons_received_audience = audience_helper.does_user_meet_audience_conditions(
+                project_config, audience_conditions, enums.RolloutRuleAudienceEvaluationLogs,
+                logging_key, attributes, self.logger)
 
-        decide_reasons += reasons_received
-
-        if forced_decision_variation:
-            return (forced_decision_variation, skip_to_everyone_else), decide_reasons
+            decide_reasons += reasons_received_audience
 
-        # regular decision
-        user_id = user.user_id
-        attributes = user.get_user_attributes()
-
-        bucketing_id, bucket_reasons = self._get_bucketing_id(user_id, attributes)
-        decide_reasons += bucket_reasons
-
-        everyone_else = (rule_index == len(rules) - 1)
-        logging_key = "Everyone Else" if everyone_else else str(rule_index + 1)
-
-        rollout_rule = config.get_experiment_from_id(rule.id)
-        audience_conditions = rollout_rule.get_audience_conditions_or_ids()
-
-        audience_decision_response, reasons_received_audience = audience_helper.does_user_meet_audience_conditions(
-            config, audience_conditions, enums.RolloutRuleAudienceEvaluationLogs, logging_key, attributes, self.logger)
-
-        decide_reasons += reasons_received_audience
+            if audience_decision_response:
+                message = 'User "{}" meets audience conditions for targeting rule {}.'.format(user_id, logging_key)
+                self.logger.debug(message)
+                decide_reasons.append(message)
 
-        if audience_decision_response:
-            message = 'User "{}" meets audience conditions for targeting rule {}.'.format(user_id, logging_key)
-            self.logger.debug(message)
-            decide_reasons.append(message)
+                bucketed_variation, bucket_reasons = self.bucketer.bucket(project_config, rollout_rule, user_id,
+                                                                          bucketing_id)
+                decide_reasons.extend(bucket_reasons)
 
-            bucketed_variation, bucket_reasons = self.bucketer.bucket(config, rollout_rule, user_id,
-                                                                      bucketing_id)
-            decide_reasons.extend(bucket_reasons)
+                if bucketed_variation:
+                    message = 'User "{}" bucketed into a targeting rule {}.'.format(user_id, logging_key)
+                    self.logger.debug(message)
+                    decide_reasons.append(message)
+                    return Decision(experiment=rule, variation=bucketed_variation,
+                                    source=enums.DecisionSources.ROLLOUT), decide_reasons
+
+                elif not everyone_else:
+                    # skip this logging for EveryoneElse since this has a message not for everyone_else
+                    message = 'User "{}" not bucketed into a targeting rule {}. ' \
+                              'Checking "Everyone Else" rule now.'.format(user_id, logging_key)
+                    self.logger.debug(message)
+                    decide_reasons.append(message)
 
-            if bucketed_variation:
-                message = 'User "{}" bucketed into a targeting rule {}.'.format(user_id, logging_key)
-                self.logger.debug(message)
-                decide_reasons.append(message)
+                    # skip the rest of rollout rules to the everyone-else rule if audience matches but not bucketed.
+                    skip_to_everyone_else = True
 
-            elif not everyone_else:
-                # skip this logging for EveryoneElse since this has a message not for everyone_else
-                message = 'User "{}" not bucketed into a targeting rule {}. ' \
-                          'Checking "Everyone Else" rule now.'.format(user_id, logging_key)
+            else:
+                message = 'User "{}" does not meet audience conditions for targeting rule {}.'.format(
+                    user_id, logging_key)
                 self.logger.debug(message)
                 decide_reasons.append(message)
 
-                # skip the rest of rollout rules to the everyone-else rule if audience matches but not bucketed.
-                skip_to_everyone_else = True
-
-        else:
-            message = 'User "{}" does not meet audience conditions for targeting rule {}.'.format(user_id, logging_key)
-            self.logger.debug(message)
-            decide_reasons.append(message)
+            # the last rule is special for "Everyone Else"
+            index = len(rollout_rules) - 1 if skip_to_everyone_else else index + 1
 
-        return (bucketed_variation, skip_to_everyone_else), decide_reasons
+        return Decision(None, None, enums.DecisionSources.ROLLOUT), decide_reasons
 
     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.
@@ -517,11 +458,34 @@ def get_variation_for_feature(self, project_config, feature, user_context, optio
             # Evaluate each experiment ID and return the first bucketed experiment variation
             for experiment in feature.experimentIds:
                 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, options)
-                    decide_reasons += variation_reasons
-                    if variation:
-                        return Decision(experiment, variation, enums.DecisionSources.FEATURE_TEST), decide_reasons
+                decision_variation = None
 
-        return self.get_variation_for_rollout(project_config, feature, user_context)
+                if experiment:
+                    optimizely_decision_context = OptimizelyUserContext.OptimizelyDecisionContext(feature.key,
+                                                                                                  experiment.key)
+
+                    forced_decision_variation, reasons_received = user_context.find_validated_forced_decision(
+                        optimizely_decision_context)
+                    decide_reasons += reasons_received
+
+                    if forced_decision_variation:
+                        decision_variation = forced_decision_variation
+                    else:
+                        decision_variation, variation_reasons = self.get_variation(project_config,
+                                                                                   experiment, user_context, options)
+                        decide_reasons += variation_reasons
+
+                    if decision_variation:
+                        message = 'User "{}" bucketed into a experiment "{}" of feature "{}".'.format(
+                            user_context.user_id, experiment.key, feature.key)
+                        self.logger.debug(message)
+                        return Decision(experiment, decision_variation,
+                                        enums.DecisionSources.FEATURE_TEST), decide_reasons
+
+        message = 'User "{}" is not bucketed into any of the experiments on the feature "{}".'.format(
+            user_context.user_id, feature.key)
+        self.logger.debug(message)
+        variation, rollout_variation_reasons = self.get_variation_for_rollout(project_config, feature, user_context)
+        if rollout_variation_reasons:
+            decide_reasons += rollout_variation_reasons
+        return variation, decide_reasons

optimizely/project_config.py

@@ -100,6 +100,7 @@ def __init__(self, datafile, logger, error_handler):
         self.variation_variable_usage_map = {}
         self.variation_id_map_by_experiment_id = {}
         self.variation_key_map_by_experiment_id = {}
+        self.flag_variations_map = {}
 
         for experiment in self.experiment_id_map.values():
             self.experiment_key_map[experiment.key] = experiment
@@ -121,45 +122,38 @@ def __init__(self, datafile, logger, error_handler):
 
         self.feature_key_map = self._generate_key_map(self.feature_flags, 'key', entities.FeatureFlag)
 
-        # 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
+        # Dictionary containing dictionary of experiment ID to feature ID.
+        # for checking that experiment is a feature experiment or not.
         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:
+        for feature in self.feature_key_map.values():
+            # 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 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
 
-            # 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_value.id]
-
-        # all rules(experiment rules and delivery rules) for each flag
-        for flag in self.feature_flags:
-            experiments = []
-            if len(flag['experimentIds']) > 0:
-                for exp_id in flag['experimentIds']:
-                    experiments.append(self.experiment_id_map[exp_id])
-            if not flag['rolloutId'] == '':
-                rollout = self.rollout_id_map[flag['rolloutId']]
-
-                rollout_experiments = self.get_rollout_experiments(rollout)
-
-                if rollout and rollout.experiments:
-                    experiments.extend(rollout_experiments)
+            feature.variables = self._generate_key_map(feature.variables, 'key', entities.Variable)
 
-            self.flag_rules_map[flag['key']] = experiments
+            rules = []
+            variations = []
+            for exp_id in feature.experimentIds:
+                # Add this experiment in experiment-feature map.
+                self.experiment_feature_map[exp_id] = [feature.id]
+                rules.append(self.experiment_id_map[exp_id])
+            rollout = None if len(feature.rolloutId) == 0 else self.rollout_id_map[feature.rolloutId]
+            if rollout:
+                for exp in rollout.experiments:
+                    rules.append(self.experiment_id_map[exp['id']])
 
-        # 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 = self.get_all_variations_for_each_rule(self.flag_rules_map)
+            for rule in rules:
+                # variation_id_map_by_experiment_id gives variation entity object while
+                # experiment_id_map will give us dictionary
+                for rule_variation in self.variation_id_map_by_experiment_id.get(rule.id).values():
+                    if len(list(filter(lambda variation: variation.id == rule_variation.id, variations))) == 0:
+                        variations.append(rule_variation)
+            self.flag_variations_map[feature.key] = variations
 
     @staticmethod
     def _generate_key_map(entity_list, key, entity_class):
@@ -639,32 +633,6 @@ def get_variation_from_key_by_experiment_id(self, experiment_id, variation_key):
 
         return {}
 
-    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
-
     def get_flag_variation(self, flag_key, variation_attribute, target_value):
         """
         Gets variation by specified variation attribute.

tests/test_decision_service.py

@@ -1325,8 +1325,8 @@ def test_get_variation_for_feature__returns_variation_for_feature_in_rollout(sel
         )
 
         # Assert no log messages were generated
-        self.assertEqual(0, mock_decision_service_logging.debug.call_count)
-        self.assertEqual(0, len(mock_decision_service_logging.method_calls))
+        self.assertEqual(1, mock_decision_service_logging.debug.call_count)
+        self.assertEqual(1, len(mock_decision_service_logging.method_calls))
 
     def test_get_variation_for_feature__returns_variation_if_user_not_in_experiment_but_in_rollout(
             self,