From ca143317e35bf46d5d96a6fd3a56c9d0f469afcb Mon Sep 17 00:00:00 2001 From: Owais Akbani Date: Fri, 1 Jun 2018 17:03:47 +0500 Subject: [PATCH] :pen: JS Sync impl --- optimizely/decision_service.py | 2 +- optimizely/event_builder.py | 35 ++++------- optimizely/helpers/enums.py | 2 +- optimizely/project_config.py | 21 +++++-- tests/base.py | 2 +- tests/test_config.py | 54 ++++++++++++---- tests/test_event_builder.py | 112 +++++++++++++-------------------- tests/test_optimizely.py | 51 +-------------- 8 files changed, 118 insertions(+), 161 deletions(-) diff --git a/optimizely/decision_service.py b/optimizely/decision_service.py index fd02b5c0..a3d8ee37 100644 --- a/optimizely/decision_service.py +++ b/optimizely/decision_service.py @@ -49,7 +49,7 @@ def _get_bucketing_id(user_id, attributes): """ attributes = attributes or {} - return attributes.get(enums.ReservedAttributes.BUCKETING_ID, user_id) + return attributes.get(enums.ControlAttributes.BUCKETING_ID, user_id) 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. diff --git a/optimizely/event_builder.py b/optimizely/event_builder.py index 4196fad7..7d7b8a45 100644 --- a/optimizely/event_builder.py +++ b/optimizely/event_builder.py @@ -18,7 +18,7 @@ from . import version from .helpers import event_tag_utils -from .helpers.enums import ReservedAttributes +from .helpers.enums import ControlAttributes class Event(object): @@ -186,34 +186,23 @@ def _get_attributes(self, attributes): attribute_value = attributes.get(attribute_key) # Omit falsy attribute values if attribute_value: - # Check for reserved attributes - reserved_attrs = [ReservedAttributes.BUCKETING_ID, ReservedAttributes.USER_AGENT] - if attribute_key in reserved_attrs: + attribute_id = self.config.get_attribute_id(attribute_key) + if attribute_id: params.append({ - 'entity_id': attribute_key, + 'entity_id': attribute_id, 'key': attribute_key, 'type': self.EventParams.CUSTOM, - 'value': attribute_value + 'value': attribute_value, }) - else: - attribute = self.config.get_attribute(attribute_key) - if attribute: - params.append({ - 'entity_id': attribute.id, - 'key': attribute_key, - 'type': self.EventParams.CUSTOM, - 'value': attribute_value, - }) - # Append Bot Filtering Attribute - attribute_key = ReservedAttributes.BOT_FILTERING - params.append({ - 'entity_id': attribute_key, - 'key': attribute_key, - 'type': self.EventParams.CUSTOM, - 'value': self._get_bot_filtering(), - }) + if isinstance(self._get_bot_filtering(), bool): + params.append({ + 'entity_id': ControlAttributes.BOT_FILTERING, + 'key': ControlAttributes.BOT_FILTERING, + 'type': self.EventParams.CUSTOM, + 'value': self._get_bot_filtering(), + }) return params diff --git a/optimizely/helpers/enums.py b/optimizely/helpers/enums.py index b2381300..35cf2ab1 100644 --- a/optimizely/helpers/enums.py +++ b/optimizely/helpers/enums.py @@ -61,7 +61,7 @@ class NotificationTypes(object): TRACK = "TRACK:event_key, user_id, attributes, event_tags, event" -class ReservedAttributes(object): +class ControlAttributes(object): BOT_FILTERING = '$opt_bot_filtering' BUCKETING_ID = '$opt_bucketing_id' USER_AGENT = '$opt_user_agent' diff --git a/optimizely/project_config.py b/optimizely/project_config.py index dd2fb706..bf1c3c51 100644 --- a/optimizely/project_config.py +++ b/optimizely/project_config.py @@ -25,6 +25,8 @@ SUPPORTED_VERSIONS = [V2_CONFIG_VERSION] UNSUPPORTED_VERSIONS = [V1_CONFIG_VERSION] +RESERVED_ATTRIBUTE_PREFIX = '$opt_' + class ProjectConfig(object): """ Representation of the Optimizely project config. """ @@ -56,7 +58,7 @@ def __init__(self, datafile, logger, error_handler): self.feature_flags = config.get('featureFlags', []) self.rollouts = config.get('rollouts', []) self.anonymize_ip = config.get('anonymizeIP', False) - self.bot_filtering = config.get('botFiltering', False) + self.bot_filtering = config.get('botFiltering', None) # Utility maps for quick lookup self.group_id_map = self._generate_key_map(self.groups, 'id', entities.Group) @@ -364,20 +366,29 @@ def get_event(self, event_key): self.error_handler.handle_error(exceptions.InvalidEventException(enums.Errors.INVALID_EVENT_KEY_ERROR)) return None - def get_attribute(self, attribute_key): - """ Get attribute for the provided attribute key. + def get_attribute_id(self, attribute_key): + """ Get attribute ID for the provided attribute key. Args: attribute_key: Attribute key for which attribute is to be fetched. Returns: - Attribute corresponding to the provided attribute key. + Attribute ID corresponding to the provided attribute key. """ attribute = self.attribute_key_map.get(attribute_key) + has_reserved_prefix = attribute_key.startswith(RESERVED_ATTRIBUTE_PREFIX) if attribute: - return attribute + if has_reserved_prefix: + self.logger.log(enums.LogLevels.WARNING, + 'Attribute %s unexpectedly has reserved prefix %s; using attribute ID instead of reserved attribute name.' + % (attribute_key, RESERVED_ATTRIBUTE_PREFIX)) + + return attribute.id + + if has_reserved_prefix and attribute_key != enums.ControlAttributes.BOT_FILTERING: + return attribute_key self.logger.log(enums.LogLevels.ERROR, 'Attribute "%s" is not in datafile.' % attribute_key) self.error_handler.handle_error(exceptions.InvalidAttributeException(enums.Errors.INVALID_ATTRIBUTE_ERROR)) diff --git a/tests/base.py b/tests/base.py index 276b87db..9f3d285b 100644 --- a/tests/base.py +++ b/tests/base.py @@ -150,7 +150,7 @@ def setUp(self): 'accountId': '12001', 'projectId': '111111', 'version': '4', - 'botFiltering': False, + 'botFiltering': True, 'events': [{ 'key': 'test_event', 'experimentIds': ['111127'], diff --git a/tests/test_config.py b/tests/test_config.py index 3159cb38..deb806f1 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -684,15 +684,15 @@ def test_get_project_id(self): def test_get_bot_filtering(self): """ Test that bot filtering is retrieved correctly when using get_bot_filtering_value. """ - # Assert bot filtering is False when not provided in data file + # Assert bot filtering is None when not provided in data file self.assertTrue('botFiltering' not in self.config_dict) - self.assertEqual(False, self.project_config.get_bot_filtering_value()) + self.assertIsNone(self.project_config.get_bot_filtering_value()) # Assert bot filtering is retrieved as provided in the data file opt_obj = optimizely.Optimizely(json.dumps(self.config_dict_with_features)) project_config = opt_obj.config self.assertEqual(self.config_dict_with_features['botFiltering'], - self.project_config.get_bot_filtering_value() + project_config.get_bot_filtering_value() ) def test_get_experiment_from_key__valid_key(self): @@ -803,16 +803,32 @@ def test_get_event__invalid_key(self): self.assertIsNone(self.project_config.get_event('invalid_key')) - def test_get_attribute__valid_key(self): - """ Test that attribute is retrieved correctly for valid attribute key. """ + def test_get_attribute_id__valid_key(self): + """ Test that attribute ID is retrieved correctly for valid attribute key. """ - self.assertEqual(entities.Attribute('111094', 'test_attribute'), - self.project_config.get_attribute('test_attribute')) + self.assertEqual('111094', + self.project_config.get_attribute_id('test_attribute')) - def test_get_attribute__invalid_key(self): + def test_get_attribute_id__invalid_key(self): """ Test that None is returned when provided attribute key is invalid. """ - self.assertIsNone(self.project_config.get_attribute('invalid_key')) + self.assertIsNone(self.project_config.get_attribute_id('invalid_key')) + + def test_get_attribute_id__reserved_key(self): + """ Test that Attribute Key is returned as ID when provided attribute key is invalid. """ + self.assertEqual('$opt_user_agent', + self.project_config.get_attribute_id('$opt_user_agent')) + + def test_get_attribute_id__unknown_key_with_opt_prefix(self): + """ Test that Attribute Key is returned as ID when provided attribute key is not + present in the datafile but has $opt prefix. """ + self.assertEqual('$opt_interesting', + self.project_config.get_attribute_id('$opt_interesting')) + + def test_get_attribute_id__key_is_bot_filtering_enum(self): + """ Test that None is returned when provided attribute key is + equal to '$opt_bot_filtering'. """ + self.assertIsNone(self.project_config.get_attribute_id('$opt_bot_filtering')) def test_get_group__valid_id(self): """ Test that group is retrieved correctly for valid group ID. """ @@ -1152,14 +1168,26 @@ def test_get_event__invalid_key(self): mock_logging.assert_called_once_with(enums.LogLevels.ERROR, 'Event "invalid_key" is not in datafile.') - def test_get_attribute__invalid_key(self): + def test_get_attribute_id__invalid_key(self): """ Test that message is logged when provided attribute key is invalid. """ with mock.patch('optimizely.logger.SimpleLogger.log') as mock_logging: - self.project_config.get_attribute('invalid_key') + self.project_config.get_attribute_id('invalid_key') mock_logging.assert_called_once_with(enums.LogLevels.ERROR, 'Attribute "invalid_key" is not in datafile.') + def test_get_attribute_id__key_with_opt_prefix_but_not_a_control_attribute(self): + """ Test that message is logged when provided attribute key has $opt_ in prefix and + key is not one of the control attributes. """ + self.project_config.attribute_key_map['$opt_abc'] = entities.Attribute('007', '$opt_abc') + + with mock.patch('optimizely.logger.SimpleLogger.log') as mock_logging: + self.project_config.get_attribute_id('$opt_abc') + + mock_logging.assert_called_once_with(enums.LogLevels.WARNING, + ("Attribute $opt_abc unexpectedly has reserved prefix $opt_; " + "using attribute ID instead of reserved attribute name.")) + def test_get_group__invalid_id(self): """ Test that message is logged when provided group ID is invalid. """ @@ -1226,12 +1254,12 @@ def test_get_event__invalid_key(self): enums.Errors.INVALID_EVENT_KEY_ERROR, self.project_config.get_event, 'invalid_key') - def test_get_attribute__invalid_key(self): + def test_get_attribute_id__invalid_key(self): """ Test that exception is raised when provided attribute key is invalid. """ self.assertRaisesRegexp(exceptions.InvalidAttributeException, enums.Errors.INVALID_ATTRIBUTE_ERROR, - self.project_config.get_attribute, 'invalid_key') + self.project_config.get_attribute_id, 'invalid_key') def test_get_group__invalid_id(self): """ Test that exception is raised when provided group ID is invalid. """ diff --git a/tests/test_event_builder.py b/tests/test_event_builder.py index 357d64fc..39af89f2 100644 --- a/tests/test_event_builder.py +++ b/tests/test_event_builder.py @@ -109,11 +109,6 @@ def test_create_impression_event__with_attributes(self): 'value': 'test_value', 'entity_id': '111094', 'key': 'test_attribute' - }, { - 'type': 'custom', - 'value': False, - 'entity_id': '$opt_bot_filtering', - 'key': '$opt_bot_filtering' }], 'snapshots': [{ 'decisions': [{ @@ -148,51 +143,46 @@ def test_create_impression_event__with_attributes(self): event_builder.EventBuilder.HTTP_HEADERS) def test_create_impression_event_when_attribute_is_not_in_datafile(self): - """ Test that create_impression_event creates Event object - with right params when attribute is not in the datafile. """ - - expected_params = { - 'account_id': '12001', - 'project_id': '111001', - 'visitors': [{ - 'visitor_id': 'test_user', - 'attributes': [{ - 'type': 'custom', - 'value': False, - 'entity_id': '$opt_bot_filtering', - 'key': '$opt_bot_filtering' - }], - 'snapshots': [{ - 'decisions': [{ - 'variation_id': '111129', - 'experiment_id': '111127', - 'campaign_id': '111182' - }], - 'events': [{ - 'timestamp': 42123, - 'entity_id': '111182', - 'uuid': 'a68cf1ad-0393-4e18-af87-efe8f01a7c9c', - 'key': 'campaign_activated' + """ Test that create_impression_event creates Event object + with right params when attribute is not in the datafile. """ + + expected_params = { + 'account_id': '12001', + 'project_id': '111001', + 'visitors': [{ + 'visitor_id': 'test_user', + 'attributes': [], + 'snapshots': [{ + 'decisions': [{ + 'variation_id': '111129', + 'experiment_id': '111127', + 'campaign_id': '111182' + }], + 'events': [{ + 'timestamp': 42123, + 'entity_id': '111182', + 'uuid': 'a68cf1ad-0393-4e18-af87-efe8f01a7c9c', + 'key': 'campaign_activated' + }] }] - }] - }], - 'client_name': 'python-sdk', - 'client_version': version.__version__, - 'anonymize_ip': False, - 'revision': '42' - } - - with mock.patch('time.time', return_value=42.123), \ - mock.patch('uuid.uuid4', return_value='a68cf1ad-0393-4e18-af87-efe8f01a7c9c'): - event_obj = self.event_builder.create_impression_event( - self.project_config.get_experiment_from_key('test_experiment'), - '111129', 'test_user', {'do_you_know_me': 'test_value'} - ) - self._validate_event_object(event_obj, - event_builder.EventBuilder.EVENTS_URL, - expected_params, - event_builder.EventBuilder.HTTP_VERB, - event_builder.EventBuilder.HTTP_HEADERS) + }], + 'client_name': 'python-sdk', + 'client_version': version.__version__, + 'anonymize_ip': False, + 'revision': '42' + } + + with mock.patch('time.time', return_value=42.123), \ + mock.patch('uuid.uuid4', return_value='a68cf1ad-0393-4e18-af87-efe8f01a7c9c'): + event_obj = self.event_builder.create_impression_event( + self.project_config.get_experiment_from_key('test_experiment'), + '111129', 'test_user', {'do_you_know_me': 'test_value'} + ) + self._validate_event_object(event_obj, + event_builder.EventBuilder.EVENTS_URL, + expected_params, + event_builder.EventBuilder.HTTP_VERB, + event_builder.EventBuilder.HTTP_HEADERS) def test_create_impression_event__with_user_agent_when_bot_filtering_is_enabled(self): """ Test that create_impression_event creates Event object @@ -291,19 +281,20 @@ def test_create_impression_event__with_user_agent_when_bot_filtering_is_disabled } with mock.patch('time.time', return_value=42.123), \ - mock.patch('uuid.uuid4', return_value='a68cf1ad-0393-4e18-af87-efe8f01a7c9c'): + mock.patch('uuid.uuid4', return_value='a68cf1ad-0393-4e18-af87-efe8f01a7c9c'),\ + mock.patch('optimizely.event_builder.EventBuilder._get_bot_filtering', return_value=False): event_obj = self.event_builder.create_impression_event( self.project_config.get_experiment_from_key('test_experiment'), '111129', 'test_user', {'$opt_user_agent': 'Chrome'} ) - self.assertFalse(self.project_config.get_bot_filtering_value()) self._validate_event_object(event_obj, event_builder.EventBuilder.EVENTS_URL, expected_params, event_builder.EventBuilder.HTTP_VERB, event_builder.EventBuilder.HTTP_HEADERS) + def test_create_conversion_event(self): """ Test that create_conversion_event creates Event object with right params when no attributes are provided. """ @@ -359,11 +350,6 @@ def test_create_conversion_event__with_attributes(self): 'value': 'test_value', 'entity_id': '111094', 'key': 'test_attribute' - }, { - 'type': 'custom', - 'value': False, - 'entity_id': '$opt_bot_filtering', - 'key': '$opt_bot_filtering' }], 'snapshots': [{ 'decisions': [{ @@ -492,12 +478,12 @@ def test_create_conversion_event__with_user_agent_when_bot_filtering_is_disabled } with mock.patch('time.time', return_value=42.123), \ - mock.patch('uuid.uuid4', return_value='a68cf1ad-0393-4e18-af87-efe8f01a7c9c'): + mock.patch('uuid.uuid4', return_value='a68cf1ad-0393-4e18-af87-efe8f01a7c9c'), \ + mock.patch('optimizely.event_builder.EventBuilder._get_bot_filtering', return_value=False): event_obj = self.event_builder.create_conversion_event( 'test_event', 'test_user', {'$opt_user_agent': 'Chrome'}, None, [('111127', '111129')] ) - self.assertFalse(self.project_config.get_bot_filtering_value()) self._validate_event_object(event_obj, event_builder.EventBuilder.EVENTS_URL, expected_params, @@ -517,11 +503,6 @@ def test_create_conversion_event__with_event_tags(self): 'type': 'custom', 'value': 'test_value', 'key': 'test_attribute' - }, { - 'type': 'custom', - 'value': False, - 'entity_id': '$opt_bot_filtering', - 'key': '$opt_bot_filtering' }], 'visitor_id': 'test_user', 'snapshots': [{ @@ -579,11 +560,6 @@ def test_create_conversion_event__with_invalid_event_tags(self): 'type': 'custom', 'value': 'test_value', 'key': 'test_attribute' - }, { - 'type': 'custom', - 'value': False, - 'entity_id': '$opt_bot_filtering', - 'key': '$opt_bot_filtering' }], 'visitor_id': 'test_user', 'snapshots': [{ diff --git a/tests/test_optimizely.py b/tests/test_optimizely.py index 85faaf22..3dd05e0c 100644 --- a/tests/test_optimizely.py +++ b/tests/test_optimizely.py @@ -512,11 +512,6 @@ def test_activate__with_attributes__audience_match(self): 'value': 'test_value', 'entity_id': '111094', 'key': 'test_attribute' - }, { - 'type': 'custom', - 'value': False, - 'entity_id': '$opt_bot_filtering', - 'key': '$opt_bot_filtering' }], 'snapshots': [{ 'decisions': [{ @@ -565,11 +560,6 @@ def test_activate__with_attributes__audience_match__forced_bucketing(self): 'value': 'test_value', 'entity_id': '111094', 'key': 'test_attribute' - }, { - 'type': 'custom', - 'value': False, - 'entity_id': '$opt_bot_filtering', - 'key': '$opt_bot_filtering' }], 'snapshots': [{ 'decisions': [{ @@ -619,16 +609,11 @@ def test_activate__with_attributes__audience_match__bucketing_id_provided(self): 'value': 'user_bucket_value', 'entity_id': '$opt_bucketing_id', 'key': '$opt_bucketing_id' - }, { + },{ 'type': 'custom', 'value': 'test_value', 'entity_id': '111094', 'key': 'test_attribute' - }, { - 'type': 'custom', - 'value': False, - 'entity_id': '$opt_bot_filtering', - 'key': '$opt_bot_filtering' }], 'snapshots': [{ 'decisions': [{ @@ -747,11 +732,6 @@ def test_track__with_attributes(self): 'value': 'test_value', 'entity_id': '111094', 'key': 'test_attribute' - }, { - 'type': 'custom', - 'value': False, - 'entity_id': '$opt_bot_filtering', - 'key': '$opt_bot_filtering' }], 'snapshots': [{ 'decisions': [{ @@ -802,16 +782,11 @@ def test_track__with_attributes__bucketing_id_provided(self): 'value': 'user_bucket_value', 'entity_id': '$opt_bucketing_id', 'key': '$opt_bucketing_id' - }, { + },{ 'type': 'custom', 'value': 'test_value', 'entity_id': '111094', 'key': 'test_attribute' - }, { - 'type': 'custom', - 'value': False, - 'entity_id': '$opt_bot_filtering', - 'key': '$opt_bot_filtering' }], 'snapshots': [{ 'decisions': [{ @@ -886,11 +861,6 @@ def test_track__with_event_tags(self): 'value': 'test_value', 'entity_id': '111094', 'key': 'test_attribute' - }, { - 'type': 'custom', - 'value': False, - 'entity_id': '$opt_bot_filtering', - 'key': '$opt_bot_filtering' }], 'snapshots': [{ 'decisions': [{ @@ -945,11 +915,6 @@ def test_track__with_event_tags_revenue(self): 'type': 'custom', 'value': 'test_value', 'key': 'test_attribute' - }, { - 'type': 'custom', - 'value': False, - 'entity_id': '$opt_bot_filtering', - 'key': '$opt_bot_filtering' }], 'visitor_id': 'test_user', 'snapshots': [{ @@ -1036,11 +1001,6 @@ def test_track__with_event_tags__forced_bucketing(self): 'value': 'test_value', 'entity_id': '111094', 'key': 'test_attribute' - }, { - 'type': 'custom', - 'value': False, - 'entity_id': '$opt_bot_filtering', - 'key': '$opt_bot_filtering' }], 'snapshots': [{ 'decisions': [{ @@ -1094,11 +1054,6 @@ def test_track__with_invalid_event_tags(self): 'type': 'custom', 'value': 'test_value', 'key': 'test_attribute' - }, { - 'type': 'custom', - 'value': False, - 'entity_id': '$opt_bot_filtering', - 'key': '$opt_bot_filtering' }], 'visitor_id': 'test_user', 'snapshots': [{ @@ -1799,7 +1754,6 @@ def test_get_feature_variable__returns_none_if_unable_to_cast(self): class OptimizelyWithExceptionTest(base.BaseTest): - def setUp(self): base.BaseTest.setUp(self) self.optimizely = optimizely.Optimizely(json.dumps(self.config_dict), @@ -1831,7 +1785,6 @@ def test_get_variation__with_attributes__invalid_attributes(self): class OptimizelyWithLoggingTest(base.BaseTest): - def setUp(self): base.BaseTest.setUp(self) self.optimizely = optimizely.Optimizely(json.dumps(self.config_dict), logger=logger.SimpleLogger())