Revert "Send event on feature access (#75)"

aliabbasrizvi · aliabbasrizvi · commit 73aa36f65ef9 · 2017-12-18T17:06:37.000-08:00
This reverts commit 70e1d44.
diff --git a/optimizely/decision_service.py b/optimizely/decision_service.py
@@ -12,7 +12,6 @@
 # limitations under the License.
 
 import sys
-from collections import namedtuple
 
 from . import bucketer
 from .helpers import audience as audience_helper
@@ -22,7 +21,6 @@
 from .user_profile import UserProfile
 
 
-Decision = namedtuple('Decision', 'experiment variation')
 RESERVED_BUCKETING_ID_ATTRIBUTE = '$opt_bucketing_id'
 
 
@@ -106,7 +104,7 @@ def get_variation(self, experiment, user_id, attributes, ignore_user_profile=Fal
     Fifth, bucket the user and return the variation.
 
     Args:
-      experiment: Experiment for which user variation needs to be determined.
+      experiment_key: Experiment for which user variation needs to be determined.
       user_id: ID for user.
       attributes: Dict representing user attributes.
       ignore_user_profile: True to ignore the user profile lookup. Defaults to False.
@@ -178,7 +176,7 @@ def get_variation(self, experiment, user_id, attributes, ignore_user_profile=Fal
     return None
 
   def get_variation_for_rollout(self, rollout, user_id, attributes=None):
-    """ Determine which experiment/variation the user is in for a given rollout.
+    """ Determine which variation the user is in for a given rollout.
     Returns the variation of the first experiment the user qualifies for.
 
     Args:
@@ -187,7 +185,7 @@ def get_variation_for_rollout(self, rollout, user_id, attributes=None):
       attributes: Dict representing user attributes.
 
     Returns:
-      Decision namedtuple consisting of experiment and variation for the user.
+      Variation the user should see. None if the user is not in any of the rollout's targeting rules.
     """
 
     # Go through each experiment in order and try to get the variation for the user
@@ -210,7 +208,7 @@ def get_variation_for_rollout(self, rollout, user_id, attributes=None):
         if variation:
           self.logger.log(enums.LogLevels.DEBUG,
                           'User "%s" is in variation %s of experiment %s.' % (user_id, variation.key, experiment.key))
-          return Decision(experiment, variation)
+          return variation
         else:
           # Evaluate no further rules
           self.logger.log(enums.LogLevels.DEBUG,
@@ -229,9 +227,9 @@ def get_variation_for_rollout(self, rollout, user_id, attributes=None):
         if variation:
           self.logger.log(enums.LogLevels.DEBUG,
                           'User "%s" meets conditions for targeting rule "Everyone Else".' % user_id)
-          return Decision(everyone_else_experiment, variation)
+          return variation
 
-    return Decision(None, None)
+    return None
 
   def get_experiment_in_group(self, group, bucketing_id):
     """ Determine which experiment in the group the user is bucketed into.
@@ -260,18 +258,17 @@ def get_experiment_in_group(self, group, bucketing_id):
     return None
 
   def get_variation_for_feature(self, feature, user_id, attributes=None):
-    """ Returns the experiment/variation the user is bucketed in for the given feature.
+    """ Returns the variation the user is bucketed in for the given feature.
 
     Args:
       feature: Feature for which we are determining if it is enabled or not for the given user.
       user_id: ID for user.
       attributes: Dict representing user attributes.
 
     Returns:
-      Decision namedtuple consisting of experiment and variation for the user.
+      Variation that the user is bucketed in. None if the user is not in any variation.
     """
 
-    experiment = None
     variation = None
     bucketing_id = self._get_bucketing_id(user_id, attributes)
 
@@ -303,6 +300,6 @@ def get_variation_for_feature(self, feature, user_id, attributes=None):
     # Next check if user is part of a rollout
     if not variation and feature.rolloutId:
       rollout = self.config.get_rollout_from_id(feature.rolloutId)
-      return self.get_variation_for_rollout(rollout, user_id, attributes)
+      variation = self.get_variation_for_rollout(rollout, user_id, attributes)
 
-    return Decision(experiment, variation)
+    return variation
diff --git a/optimizely/optimizely.py b/optimizely/optimizely.py
@@ -21,7 +21,6 @@
 from .error_handler import NoOpErrorHandler as noop_error_handler
 from .event_dispatcher import EventDispatcher as default_event_dispatcher
 from .helpers import enums
-from .helpers import event_tag_utils
 from .helpers import validator
 from .logger import NoOpLogger as noop_logger
 from .logger import SimpleLogger
@@ -155,33 +154,6 @@ def _get_decisions(self, event, user_id, attributes):
 
     return decisions
 
-  def _send_impression_event(self, experiment, variation, user_id, attributes):
-    """ Helper method to send impression event.
-
-    Args:
-      experiment: Experiment for which impression event is being sent.
-      variation: Variation picked for user for the given experiment.
-      user_id: ID for user.
-      attributes: Dict representing user attributes and values which need to be recorded.
-    """
-
-    impression_event = self.event_builder.create_impression_event(experiment,
-                                                                  variation.id,
-                                                                  user_id,
-                                                                  attributes)
-
-    self.logger.log(enums.LogLevels.DEBUG,
-                    'Dispatching impression event to URL %s with params %s.' % (impression_event.url,
-                                                                                impression_event.params))
-
-    try:
-      self.event_dispatcher.dispatch_event(impression_event)
-    except:
-      error = sys.exc_info()[1]
-      self.logger.log(enums.LogLevels.ERROR, 'Unable to dispatch impression event. Error: %s' % str(error))
-    self.notification_center.send_notifications(enums.NotificationTypes.ACTIVATE,
-                                                experiment, user_id, attributes, variation, impression_event)
-
   def activate(self, experiment_key, user_id, attributes=None):
     """ Buckets visitor and sends impression event to Optimizely.
 
@@ -205,12 +177,19 @@ def activate(self, experiment_key, user_id, attributes=None):
       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)
-
-    # Create and dispatch impression event
+    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._send_impression_event(experiment, variation, user_id, attributes)
+    self.logger.log(enums.LogLevels.DEBUG,
+                    'Dispatching impression event to URL %s with params %s.' % (impression_event.url,
+                                                                                impression_event.params))
+    try:
+      self.event_dispatcher.dispatch_event(impression_event)
+    except:
+      error = sys.exc_info()[1]
+      self.logger.log(enums.LogLevels.ERROR, 'Unable to dispatch impression event. Error: %s' % str(error))
 
     return variation.key
 
@@ -321,13 +300,9 @@ def is_feature_enabled(self, feature_key, user_id, attributes=None):
     if not feature:
       return False
 
-    decision = self.decision_service.get_variation_for_feature(feature, user_id, attributes)
-    if decision.variation:
+    variation = self.decision_service.get_variation_for_feature(feature, user_id, attributes)
+    if variation:
       self.logger.log(enums.LogLevels.INFO, 'Feature "%s" is enabled for user "%s".' % (feature_key, user_id))
-      self._send_impression_event(decision.experiment,
-                                  decision.variation,
-                                  user_id,
-                                  attributes)
       return True
 
     self.logger.log(enums.LogLevels.INFO, 'Feature "%s" is not enabled for user "%s".' % (feature_key, user_id))
diff --git a/tests/test_decision_service.py b/tests/test_decision_service.py
@@ -14,7 +14,6 @@
 import json
 import mock
 
-from optimizely import decision_service
 from optimizely import entities
 from optimizely import optimizely
 from optimizely import user_profile
@@ -377,8 +376,7 @@ def test_get_variation_for_rollout__returns_none_if_no_experiments(self, mock_lo
     """ Test that get_variation_for_rollout returns None if there are no experiments (targeting rules). """
 
     no_experiment_rollout = self.project_config.get_rollout_from_id('201111')
-    self.assertEqual(decision_service.Decision(None, None),
-                     self.decision_service.get_variation_for_rollout(no_experiment_rollout, 'test_user'))
+    self.assertIsNone(self.decision_service.get_variation_for_rollout(no_experiment_rollout, 'test_user'))
 
     # Assert no log messages were generated
     self.assertEqual(0, mock_logging.call_count)
@@ -438,8 +436,7 @@ def test_get_variation_for_rollout__skips_to_everyone_else_rule(self, mock_loggi
 
     with mock.patch('optimizely.helpers.audience.is_user_in_experiment', return_value=True) as mock_audience_check,\
       mock.patch('optimizely.bucketer.Bucketer.bucket', return_value=None):
-      self.assertEqual(decision_service.Decision(None, None),
-                       self.decision_service.get_variation_for_rollout(rollout, 'test_user'))
+      self.assertIsNone(self.decision_service.get_variation_for_rollout(rollout, 'test_user'))
 
     # Check that after first experiment, it skips to the last experiment to check
     self.assertEqual(
@@ -461,8 +458,7 @@ def test_get_variation_for_rollout__returns_none_for_user_not_in_rollout(self, m
     rollout = self.project_config.get_rollout_from_id('211111')
 
     with mock.patch('optimizely.helpers.audience.is_user_in_experiment', return_value=False) as mock_audience_check:
-      self.assertEqual(decision_service.Decision(None, None),
-                       self.decision_service.get_variation_for_rollout(rollout, 'test_user'))
+      self.assertIsNone(self.decision_service.get_variation_for_rollout(rollout, 'test_user'))
 
     # Check that all experiments in rollout layer were checked
     self.assertEqual(
@@ -483,12 +479,11 @@ def test_get_variation_for_feature__returns_variation_for_feature_in_experiment(
 
     feature = self.project_config.get_feature_from_key('test_feature_in_experiment')
 
-    expected_experiment = self.project_config.get_experiment_from_key('test_experiment')
     expected_variation = self.project_config.get_variation_from_id('test_experiment', '111129')
-    with mock.patch('optimizely.decision_service.DecisionService.get_variation',
-                    return_value=expected_variation) as mock_decision:
-      self.assertEqual(decision_service.Decision(expected_experiment, expected_variation),
-                       self.decision_service.get_variation_for_feature(feature, 'user1'))
+    with mock.patch(
+      'optimizely.decision_service.DecisionService.get_variation',
+      return_value=expected_variation) as mock_decision:
+      self.assertEqual(expected_variation, self.decision_service.get_variation_for_feature(feature, 'user1'))
 
     mock_decision.assert_called_once_with(
       self.project_config.get_experiment_from_key('test_experiment'), 'test_user', None
@@ -521,14 +516,12 @@ def test_get_variation_for_feature__returns_variation_if_user_not_in_experiment_
 
     feature = self.project_config.get_feature_from_key('test_feature_in_experiment_and_rollout')
 
-    expected_experiment = self.project_config.get_experiment_from_key('211127')
     expected_variation = self.project_config.get_variation_from_id('211127', '211129')
     with mock.patch(
       'optimizely.helpers.audience.is_user_in_experiment',
       side_effect=[False, True]) as mock_audience_check, \
       mock.patch('optimizely.bucketer.Bucketer.bucket', return_value=expected_variation):
-      self.assertEqual(decision_service.Decision(expected_experiment, expected_variation),
-                       self.decision_service.get_variation_for_feature(feature, 'user1'))
+      self.assertEqual(expected_variation, self.decision_service.get_variation_for_feature(feature, 'user1'))
 
     self.assertEqual(2, mock_audience_check.call_count)
     mock_audience_check.assert_any_call(self.project_config,
@@ -540,20 +533,21 @@ def test_get_variation_for_feature__returns_variation_for_feature_in_group(self,
     """ Test that get_variation_for_feature returns the variation of
      the experiment the user is bucketed in the feature's group. """
 
-    feature = self.project_config.get_feature_from_key('test_feature_in_group')
+    opt_obj = optimizely.Optimizely(json.dumps(self.config_dict_with_features))
+    project_config = opt_obj.config
+    decision_service = opt_obj.decision_service
+    feature = project_config.get_feature_from_key('test_feature_in_group')
 
-    expected_experiment = self.project_config.get_experiment_from_key('group_exp_1')
-    expected_variation = self.project_config.get_variation_from_id('group_exp_1', '28901')
+    expected_variation = project_config.get_variation_from_id('group_exp_1', '28901')
     with mock.patch(
       'optimizely.decision_service.DecisionService.get_experiment_in_group',
-      return_value=self.project_config.get_experiment_from_key('group_exp_1')) as mock_get_experiment_in_group, \
+      return_value=project_config.get_experiment_from_key('group_exp_1')) as mock_get_experiment_in_group, \
       mock.patch('optimizely.decision_service.DecisionService.get_variation',
                  return_value=expected_variation) as mock_decision:
-      self.assertEqual(decision_service.Decision(expected_experiment, expected_variation),
-                       self.decision_service.get_variation_for_feature(feature, 'user1'))
+      self.assertEqual(expected_variation, decision_service.get_variation_for_feature(feature, 'user1'))
 
-    mock_get_experiment_in_group.assert_called_once_with(self.project_config.get_group('19228'), 'test_user')
-    mock_decision.assert_called_once_with(self.project_config.get_experiment_from_key('group_exp_1'), 'test_user', None)
+    mock_get_experiment_in_group.assert_called_once_with(project_config.get_group('19228'), 'user1')
+    mock_decision.assert_called_once_with(project_config.get_experiment_from_key('group_exp_1'), 'user1', None)
 
   def test_get_variation_for_feature__returns_none_for_user_not_in_group(self, _):
     """ Test that get_variation_for_feature returns None for
@@ -564,8 +558,7 @@ def test_get_variation_for_feature__returns_none_for_user_not_in_group(self, _):
     with mock.patch('optimizely.decision_service.DecisionService.get_experiment_in_group',
                     return_value=None) as mock_get_experiment_in_group, \
       mock.patch('optimizely.decision_service.DecisionService.get_variation') as mock_decision:
-      self.assertEqual(decision_service.Decision(None, None),
-                       self.decision_service.get_variation_for_feature(feature, 'user1'))
+      self.assertIsNone(self.decision_service.get_variation_for_feature(feature, 'user1'))
 
     mock_get_experiment_in_group.assert_called_once_with(self.project_config.get_group('19228'), 'test_user')
     self.assertFalse(mock_decision.called)
@@ -574,11 +567,9 @@ def test_get_variation_for_feature__returns_none_for_user_not_in_experiment(self
     """ Test that get_variation_for_feature returns None for user not in the associated experiment. """
 
     feature = self.project_config.get_feature_from_key('test_feature_in_experiment')
-    expected_experiment = self.project_config.get_experiment_from_key('test_experiment')
 
     with mock.patch('optimizely.decision_service.DecisionService.get_variation', return_value=None) as mock_decision:
-      self.assertEqual(decision_service.Decision(expected_experiment, None),
-                       self.decision_service.get_variation_for_feature(feature, 'user1'))
+      self.assertIsNone(self.decision_service.get_variation_for_feature(feature, 'user1'))
 
     mock_decision.assert_called_once_with(
       self.project_config.get_experiment_from_key('test_experiment'), 'test_user', None
@@ -589,12 +580,10 @@ def test_get_variation_for_feature__returns_none_for_user_in_group_experiment_no
     not targeting a feature, then None is returned. """
 
     feature = self.project_config.get_feature_from_key('test_feature_in_group')
-    expected_experiment = self.project_config.get_experiment_from_key('group_exp_2')
 
     with mock.patch('optimizely.decision_service.DecisionService.get_experiment_in_group',
                     return_value=self.project_config.get_experiment_from_key('group_exp_2')) as mock_decision:
-      self.assertEqual(decision_service.Decision(expected_experiment, None),
-                       self.decision_service.get_variation_for_feature(feature, 'user_1'))
+      self.assertIsNone(self.decision_service.get_variation_for_feature(feature, 'user_1'))
 
     mock_decision.assert_called_once_with(self.project_config.get_group('19228'), 'test_user')
 
diff --git a/tests/test_optimizely.py b/tests/test_optimizely.py
