Refactor order of bucketing operations (#47)

optimizely/bucketer.py

@@ -103,16 +103,6 @@ def bucket(self, experiment, user_id):
     if not experiment:
       return None
 
-    # Check if user is white-listed for a variation
-    forced_variations = experiment.forcedVariations
-    if forced_variations and user_id in forced_variations:
-      variation_key = forced_variations.get(user_id)
-      variation = self.config.get_variation_from_key(experiment.key, variation_key)
-      if variation:
-        self.config.logger.log(enums.LogLevels.INFO,
-                               'User "%s" is forced in variation "%s".' % (user_id, variation_key))
-      return variation
-
     # Determine if experiment is in a mutually exclusive group
     if experiment.groupPolicy in GROUP_POLICIES:
       group = self.config.get_group(experiment.groupId)
@@ -143,3 +133,25 @@ def bucket(self, experiment, user_id):
 
     self.config.logger.log(enums.LogLevels.INFO, 'User "%s" is in no variation.' % user_id)
     return None
+
+  def get_forced_variation(self, experiment, user_id):
+    """ Determine if a user is forced into a variation for the given experiment and return that variation.
+
+    Args:
+      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.
+    """
+
+    forced_variations = experiment.forcedVariations
+    if forced_variations and user_id in forced_variations:
+      variation_key = forced_variations.get(user_id)
+      variation = self.config.get_variation_from_key(experiment.key, variation_key)
+      if variation:
+        self.config.logger.log(enums.LogLevels.INFO,
+                               'User "%s" is forced in variation "%s".' % (user_id, variation_key))
+      return variation
+
+    return None

optimizely/event_builder.py

@@ -97,7 +97,7 @@ def _add_common_params(self, user_id, attributes):
 
 
 class EventBuilder(BaseEventBuilder):
-  """ Class which encapsulates methods to build events for tracking 
+  """ Class which encapsulates methods to build events for tracking
   impressions and conversions using the new endpoints. """
 
   IMPRESSION_ENDPOINT = 'https://logx.optimizely.com/log/decision'
@@ -190,7 +190,7 @@ def _add_required_params_for_conversion(self, event_key, user_id, event_tags, va
       event_key: Key representing the event which needs to be recorded.
       user_id: ID for user.
       event_tags: Dict representing metadata associated with the event.
-      valid_experiments: List of tuples representing valid experiments for the event.
+      valid_experiments: List of tuples representing valid experiments IDs and variation IDs.
     """
 
     self.params[self.EventParams.IS_GLOBAL_HOLDBACK] = False
@@ -219,19 +219,18 @@ def _add_required_params_for_conversion(self, event_key, user_id, event_tags, va
         self.params[self.EventParams.EVENT_FEATURES].append(event_feature)
 
     self.params[self.EventParams.LAYER_STATES] = []
-    for experiment in valid_experiments:
-      variation = self.bucketer.bucket(experiment, user_id)
-      if variation:
-        self.params[self.EventParams.LAYER_STATES].append({
-          self.EventParams.LAYER_ID: experiment.layerId,
-          self.EventParams.REVISION: self.config.get_revision(),
-          self.EventParams.ACTION_TRIGGERED: True,
-          self.EventParams.DECISION: {
-            self.EventParams.EXPERIMENT_ID: experiment.id,
-            self.EventParams.VARIATION_ID: variation.id,
-            self.EventParams.IS_LAYER_HOLDBACK: False
-          }
-        })
+    for experiment_id, variation_id in valid_experiments:
+      experiment = self.config.get_experiment_from_id(experiment_id)
+      self.params[self.EventParams.LAYER_STATES].append({
+        self.EventParams.LAYER_ID: experiment.layerId,
+        self.EventParams.REVISION: self.config.get_revision(),
+        self.EventParams.ACTION_TRIGGERED: True,
+        self.EventParams.DECISION: {
+          self.EventParams.EXPERIMENT_ID: experiment.id,
+          self.EventParams.VARIATION_ID: variation_id,
+          self.EventParams.IS_LAYER_HOLDBACK: False
+        }
+      })
 
     self.params[self.EventParams.EVENT_ID] = self.config.get_event(event_key).id
     self.params[self.EventParams.EVENT_NAME] = event_key
@@ -265,7 +264,7 @@ def create_conversion_event(self, event_key, user_id, attributes, event_tags, va
       user_id: ID for user.
       attributes: Dict representing user attributes and values.
       event_tags: Dict representing metadata associated with the event.
-      valid_experiments: List of tuples representing valid experiments for the event.
+      valid_experiments: List of tuples representing experiments IDs and variation IDs.
 
     Returns:
       Event object encapsulating the conversion event.

optimizely/helpers/experiment.py

@@ -25,20 +25,3 @@ def is_experiment_running(experiment):
   """
 
   return experiment.status in ALLOWED_EXPERIMENT_STATUS
-
-
-def is_user_in_forced_variation(forced_variations, user_id):
-  """ Determine if the user is in a forced variation.
-
-  Args:
-    forced_variations: Dict representing forced variations for the experiment.
-    user_id: User to check for in whitelist.
-
-  Returns:
-    Boolean depending on whether user is in forced variation or not.
-  """
-
-  if forced_variations and user_id in forced_variations:
-    return True
-
-  return False

optimizely/optimizely.py

@@ -50,7 +50,7 @@ def __init__(self, datafile, event_dispatcher=None, logger=None, error_handler=N
     self.error_handler = error_handler or noop_error_handler
 
     try:
-      self._validate_inputs(datafile, skip_json_validation)
+      self._validate_instantiation_options(datafile, skip_json_validation)
     except exceptions.InvalidInputException as error:
       self.is_valid = False
       self.logger = SimpleLogger()
@@ -75,15 +75,15 @@ def __init__(self, datafile, event_dispatcher=None, logger=None, error_handler=N
     self.bucketer = bucketer.Bucketer(self.config)
     self.event_builder = event_builder.EventBuilder(self.config, self.bucketer)
 
-  def _validate_inputs(self, datafile, skip_json_validation):
-    """ Helper method to validate all input parameters.
+  def _validate_instantiation_options(self, datafile, skip_json_validation):
+    """ Helper method to validate all instantiation parameters.
 
     Args:
       datafile: JSON string representing the project.
       skip_json_validation: Boolean representing whether JSON schema validation needs to be skipped or not.
 
     Raises:
-      Exception if provided input is invalid.
+      Exception if provided instantiation options are valid.
     """
 
     if not skip_json_validation and not validator.is_datafile_valid(datafile):
@@ -98,39 +98,74 @@ def _validate_inputs(self, datafile, skip_json_validation):
     if not validator.is_error_handler_valid(self.error_handler):
      raise exceptions.InvalidInputException(enums.Errors.INVALID_INPUT_ERROR.format('error_handler'))
 
-  def _validate_preconditions(self, experiment, user_id, attributes):
+  def _validate_preconditions(self, experiment, attributes=None, event_tags=None):
     """ Helper method to validate all pre-conditions before we go ahead to bucket user.
 
     Args:
       experiment: Object representing the experiment.
-      user_id: ID for user.
       attributes: Dict representing user attributes.
 
     Returns:
       Boolean depending upon whether all conditions are met or not.
     """
-
-    if attributes and not validator.are_attributes_valid(attributes):
-      self.logger.log(enums.LogLevels.ERROR, 'Provided attributes are in an invalid format.')
-      self.error_handler.handle_error(exceptions.InvalidAttributeException(enums.Errors.INVALID_ATTRIBUTE_FORMAT))
+    if not self._validate_user_inputs(attributes, event_tags):
       return False
 
     if not experiment_helper.is_experiment_running(experiment):
       self.logger.log(enums.LogLevels.INFO, 'Experiment "%s" is not running.' % experiment.key)
       return False
 
-    if experiment_helper.is_user_in_forced_variation(experiment.forcedVariations, user_id):
-      return True
+    return True
 
-    if not audience_helper.is_user_in_experiment(self.config, experiment, attributes):
-      self.logger.log(
-        enums.LogLevels.INFO,
-        'User "%s" does not meet conditions to be in experiment "%s".' % (user_id, experiment.key)
-      )
+  def _validate_user_inputs(self, attributes=None, event_tags=None):
+    """ Helper method to validate user inputs.
+
+    Args:
+      attributes: Dict representing user attributes.
+      event_tags: Dict representing metadata associated with an event.
+
+    Returns:
+      Boolean True if inputs are valid. False otherwise.
+
+    """
+
+    if attributes and not validator.are_attributes_valid(attributes):
+      self.logger.log(enums.LogLevels.ERROR, 'Provided attributes are in an invalid format.')
+      self.error_handler.handle_error(exceptions.InvalidAttributeException(enums.Errors.INVALID_ATTRIBUTE_FORMAT))
+      return False
+
+    if event_tags and not validator.are_event_tags_valid(event_tags):
+      self.logger.log(enums.LogLevels.ERROR, 'Provided event tags are in an invalid format.')
+      self.error_handler.handle_error(exceptions.InvalidEventTagException(enums.Errors.INVALID_EVENT_TAG_FORMAT))
       return False
 
     return True
 
+  def _get_valid_experiments_for_event(self, event, user_id, attributes):
+    """ Helper method to determine which experiments we should track for the given event.
+
+    Args:
+      event: The event which needs to be recorded.
+      user_id: ID for user.
+      attributes: Dict representing user attributes.
+
+    Returns:
+      List of tuples representing valid experiment IDs and variation IDs into which the user is bucketed.
+    """
+    valid_experiments = []
+    for experiment_id in event.experimentIds:
+      experiment = self.config.get_experiment_from_id(experiment_id)
+      variation_key = self.get_variation(experiment.key, user_id, attributes)
+
+      if not variation_key:
+        self.logger.log(enums.LogLevels.INFO, 'Not tracking user "%s" for experiment "%s".' % (user_id, experiment.key))
+        continue
+
+      variation = self.config.get_variation_from_key(experiment.key, variation_key)
+      valid_experiments.append((experiment_id, variation.id))
+
+    return valid_experiments
+
   def activate(self, experiment_key, user_id, attributes=None):
     """ Buckets visitor and sends impression event to Optimizely.
 
@@ -148,22 +183,15 @@ def activate(self, experiment_key, user_id, attributes=None):
       self.logger.log(enums.LogLevels.ERROR, enums.Errors.INVALID_DATAFILE.format('activate'))
       return None
 
-    experiment = self.config.get_experiment_from_key(experiment_key)
-    if not experiment:
-      self.logger.log(enums.LogLevels.INFO, 'Not activating user "%s".' % user_id)
-      return None
-
-    if not self._validate_preconditions(experiment, user_id, attributes):
-      self.logger.log(enums.LogLevels.INFO, 'Not activating user "%s".' % user_id)
-      return None
-
-    variation = self.bucketer.bucket(experiment, user_id)
+    variation_key = self.get_variation(experiment_key, user_id, attributes)
 
-    if not variation:
+    if not variation_key:
       self.logger.log(enums.LogLevels.INFO, 'Not activating user "%s".' % user_id)
       return None
 
     # Create and dispatch impression event
+    experiment = self.config.get_experiment_from_key(experiment_key)
+    variation = self.config.get_variation_from_key(experiment_key, variation_key)
     impression_event = self.event_builder.create_impression_event(experiment, variation.id, user_id, attributes)
     self.logger.log(enums.LogLevels.INFO, 'Activating user "%s" in experiment "%s".' % (user_id, experiment.key))
     self.logger.log(enums.LogLevels.DEBUG,
@@ -191,11 +219,6 @@ def track(self, event_key, user_id,  attributes=None, event_tags=None):
       self.logger.log(enums.LogLevels.ERROR, enums.Errors.INVALID_DATAFILE.format('track'))
       return
 
-    if attributes and not validator.are_attributes_valid(attributes):
-      self.logger.log(enums.LogLevels.ERROR, 'Provided attributes are in an invalid format.')
-      self.error_handler.handle_error(exceptions.InvalidAttributeException(enums.Errors.INVALID_ATTRIBUTE_FORMAT))
-      return
-
     if event_tags:
       if isinstance(event_tags, numbers.Number):
         event_tags = {
@@ -204,24 +227,16 @@ def track(self, event_key, user_id,  attributes=None, event_tags=None):
         self.logger.log(enums.LogLevels.WARNING,
                         'Event value is deprecated in track call. Use event tags to pass in revenue value instead.')
 
-      if not validator.are_event_tags_valid(event_tags):
-        self.logger.log(enums.LogLevels.ERROR, 'Provided event tags are in an invalid format.')
-        self.error_handler.handle_error(exceptions.InvalidEventTagException(enums.Errors.INVALID_EVENT_TAG_FORMAT))
-        return
+    if not self._validate_user_inputs(attributes, event_tags):
+      return
 
     event = self.config.get_event(event_key)
     if not event:
       self.logger.log(enums.LogLevels.INFO, 'Not tracking user "%s" for event "%s".' % (user_id, event_key))
       return
 
     # Filter out experiments that are not running or that do not include the user in audience conditions
-    valid_experiments = []
-    for experiment_id in event.experimentIds:
-      experiment = self.config.get_experiment_from_id(experiment_id)
-      if not self._validate_preconditions(experiment, user_id, attributes):
-        self.logger.log(enums.LogLevels.INFO, 'Not tracking user "%s" for experiment "%s".' % (user_id, experiment.key))
-        continue
-      valid_experiments.append(experiment)
+    valid_experiments = self._get_valid_experiments_for_event(event, user_id, attributes)
 
     # Create and dispatch conversion event if there are valid experiments
     if valid_experiments:
@@ -259,10 +274,23 @@ def get_variation(self, experiment_key, user_id, attributes=None):
 
     experiment = self.config.get_experiment_from_key(experiment_key)
     if not experiment:
+      self.logger.log(enums.LogLevels.INFO, 'Experiment key "%s" is invalid. Not activating user "%s".' % (experiment_key, user_id))
       return None
 
-    if not self._validate_preconditions(experiment, user_id, attributes):
+    if not self._validate_preconditions(experiment, attributes):
       return None
+
+    forcedVariation = self.bucketer.get_forced_variation(experiment, user_id)
+    if forcedVariation:
+      return forcedVariation.key
+
+    if not audience_helper.is_user_in_experiment(self.config, experiment, attributes):
+      self.logger.log(
+        enums.LogLevels.INFO,
+        'User "%s" does not meet conditions to be in experiment "%s".' % (user_id, experiment.key)
+      )
+      return None
+
     variation = self.bucketer.bucket(experiment, user_id)
 
     if variation:

tests/helpers_tests/test_experiment.py

@@ -33,19 +33,3 @@ def test_is_experiment_running__status_not_running(self):
                       '42', 'test_experiment', 'Some Status', [], [], {}, [], '43')) as mock_get_experiment:
       self.assertFalse(experiment.is_experiment_running(self.project_config.get_experiment_from_key('test_experiment')))
     mock_get_experiment.assert_called_once_with('test_experiment')
-
-  def test_is_user_in_forced_variation__no_forced_variation_dict(self):
-    """ Test that is_user_in_forced_variation returns False when experiment has no forced variations. """
-
-    self.assertFalse(experiment.is_user_in_forced_variation(None, 'test_user'))
-    self.assertFalse(experiment.is_user_in_forced_variation({}, 'test_user'))
-
-  def test_is_user_in_forced_variation__user_not_in_forced_variation(self):
-    """ Test that is_user_in_forced_variation returns False when user is not in experiment's forced variations. """
-
-    self.assertFalse(experiment.is_user_in_forced_variation({'user_1': 'control'}, 'test_user'))
-
-  def test_is_user_in_forced_variation__user_in_forced_variation(self):
-    """ Test that is_user_in_forced_variation returns True when user is in experiment's forced variations. """
-
-    self.assertTrue(experiment.is_user_in_forced_variation({'user_1': 'control'}, 'user_1'))