From 6d5f174dd745b9d94ac5b6099069efb7c64cac4f Mon Sep 17 00:00:00 2001 From: ozayr-zaviar Date: Mon, 13 Dec 2021 22:00:20 +0500 Subject: [PATCH 1/7] init --- optimizely/event/user_event_factory.py | 9 +++++---- optimizely/project_config.py | 8 ++++---- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/optimizely/event/user_event_factory.py b/optimizely/event/user_event_factory.py index 38217883..cc3887b0 100644 --- a/optimizely/event/user_event_factory.py +++ b/optimizely/event/user_event_factory.py @@ -49,11 +49,12 @@ def create_impression_event( if activated_experiment: experiment_id = activated_experiment.id - if variation_id and experiment_id: - variation = project_config.get_variation_from_id_by_experiment_id(experiment_id, variation_id) - # need this condition when we send events involving forced decisions - elif variation_id and flag_key: + if variation_id and flag_key: variation = project_config.get_flag_variation(flag_key, 'id', variation_id) + elif variation_id and experiment_id: + # need this condition when we send events involving forced decisions + variation = project_config.get_variation_from_id_by_experiment_id(experiment_id, variation_id) + event_context = user_event.EventContext( project_config.account_id, project_config.project_id, project_config.revision, project_config.anonymize_ip, ) diff --git a/optimizely/project_config.py b/optimizely/project_config.py index 494df542..1c5cf791 100644 --- a/optimizely/project_config.py +++ b/optimizely/project_config.py @@ -612,8 +612,8 @@ def get_variation_from_id_by_experiment_id(self, experiment_id, variation_id): variation_id in self.variation_id_map_by_experiment_id[experiment_id]): return self.variation_id_map_by_experiment_id[experiment_id][variation_id] - self.logger.error('Variation with id "%s" not defined in the datafile for experiment "%s".', - variation_id, experiment_id) + self.logger.error('Variation with id "%s" not defined in the datafile for experiment "%s".' % + (variation_id, experiment_id)) return {} @@ -628,8 +628,8 @@ def get_variation_from_key_by_experiment_id(self, experiment_id, variation_key): variation_key in self.variation_key_map_by_experiment_id[experiment_id]): return self.variation_key_map_by_experiment_id[experiment_id][variation_key] - self.logger.error('Variation with key "%s" not defined in the datafile for experiment "%s".', - variation_key, experiment_id) + self.logger.error('Variation with key "%s" not defined in the datafile for experiment "%s".' % + (variation_key, experiment_id)) return {} From e3af5627fb091139094dced6dd2d39dc8052074e Mon Sep 17 00:00:00 2001 From: ozayr-zaviar Date: Mon, 13 Dec 2021 22:44:08 +0500 Subject: [PATCH 2/7] conditions fixed --- optimizely/event/user_event_factory.py | 2 +- optimizely/project_config.py | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/optimizely/event/user_event_factory.py b/optimizely/event/user_event_factory.py index cc3887b0..d395064e 100644 --- a/optimizely/event/user_event_factory.py +++ b/optimizely/event/user_event_factory.py @@ -51,7 +51,7 @@ def create_impression_event( if variation_id and flag_key: variation = project_config.get_flag_variation(flag_key, 'id', variation_id) - elif variation_id and experiment_id: + if not variation and variation_id and experiment_id: # need this condition when we send events involving forced decisions variation = project_config.get_variation_from_id_by_experiment_id(experiment_id, variation_id) diff --git a/optimizely/project_config.py b/optimizely/project_config.py index 1c5cf791..82da17c9 100644 --- a/optimizely/project_config.py +++ b/optimizely/project_config.py @@ -661,8 +661,9 @@ def get_flag_variation(self, flag_key, variation_attribute, target_value): return None variations = self.flag_variations_map.get(flag_key) - for variation in variations: - if getattr(variation, variation_attribute) == target_value: - return variation + if variations: + for variation in variations: + if getattr(variation, variation_attribute) == target_value: + return variation return None From e480d8d5a37f3a707041ff2975cc8b4a3dab634d Mon Sep 17 00:00:00 2001 From: ozayr-zaviar Date: Tue, 14 Dec 2021 19:21:07 +0500 Subject: [PATCH 3/7] unit test added for no active experiment in create impression event --- optimizely/event/user_event_factory.py | 2 +- tests/test_user_event_factory.py | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/optimizely/event/user_event_factory.py b/optimizely/event/user_event_factory.py index d395064e..60928c1b 100644 --- a/optimizely/event/user_event_factory.py +++ b/optimizely/event/user_event_factory.py @@ -54,7 +54,7 @@ def create_impression_event( if not variation and variation_id and experiment_id: # need this condition when we send events involving forced decisions variation = project_config.get_variation_from_id_by_experiment_id(experiment_id, variation_id) - + event_context = user_event.EventContext( project_config.account_id, project_config.project_id, project_config.revision, project_config.anonymize_ip, ) diff --git a/tests/test_user_event_factory.py b/tests/test_user_event_factory.py index e723a823..84b56e50 100644 --- a/tests/test_user_event_factory.py +++ b/tests/test_user_event_factory.py @@ -70,6 +70,17 @@ def test_impression_event__with_attributes(self): [x.__dict__ for x in expected_attrs], [x.__dict__ for x in impression_event.visitor_attributes], ) + def test_impression_event__no_active_experiment(self): + project_config = self.project_config + user_id = 'test_user' + + user_attributes = {'test_attribute': 'test_value', 'boolean_key': True} + + impression_event = UserEventFactory.create_impression_event( + project_config, None, '111128', 'flag_key', 'rule_key', 'rule_type', True, user_id, user_attributes + ) + self.assertIsNone(impression_event) + def test_conversion_event(self): project_config = self.project_config user_id = 'test_user' From 9eed3ecbf557ecb486477985141d624b21b98cba Mon Sep 17 00:00:00 2001 From: msohailhussain Date: Tue, 14 Dec 2021 11:34:00 -0800 Subject: [PATCH 4/7] Apply suggestions from code review Co-authored-by: Jae Kim <45045038+jaeopt@users.noreply.github.com> --- optimizely/event/user_event_factory.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/optimizely/event/user_event_factory.py b/optimizely/event/user_event_factory.py index 60928c1b..4f1a331a 100644 --- a/optimizely/event/user_event_factory.py +++ b/optimizely/event/user_event_factory.py @@ -51,8 +51,8 @@ def create_impression_event( if variation_id and flag_key: variation = project_config.get_flag_variation(flag_key, 'id', variation_id) - if not variation and variation_id and experiment_id: - # need this condition when we send events involving forced decisions + elif variation_id and experiment_id: + # need this condition when we send events involving forced decisions (F-to-D or E-to-D with any ruleKey/variationKey combinations) variation = project_config.get_variation_from_id_by_experiment_id(experiment_id, variation_id) event_context = user_event.EventContext( From 15ca2c68e0dd931133885349b31deee31b8f8bc8 Mon Sep 17 00:00:00 2001 From: ozayr-zaviar Date: Wed, 15 Dec 2021 23:11:53 +0500 Subject: [PATCH 5/7] linting fixed --- optimizely/event/user_event_factory.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/optimizely/event/user_event_factory.py b/optimizely/event/user_event_factory.py index 4f1a331a..36a5593b 100644 --- a/optimizely/event/user_event_factory.py +++ b/optimizely/event/user_event_factory.py @@ -52,7 +52,8 @@ def create_impression_event( if variation_id and flag_key: variation = project_config.get_flag_variation(flag_key, 'id', variation_id) elif variation_id and experiment_id: - # need this condition when we send events involving forced decisions (F-to-D or E-to-D with any ruleKey/variationKey combinations) + # need this condition when we send events involving forced decisions + # (F-to-D or E-to-D with any ruleKey/variationKey combinations) variation = project_config.get_variation_from_id_by_experiment_id(experiment_id, variation_id) event_context = user_event.EventContext( From 2f500729da5fea7527b15b126a55aafd64d79382 Mon Sep 17 00:00:00 2001 From: ozayr-zaviar Date: Thu, 16 Dec 2021 00:47:13 +0500 Subject: [PATCH 6/7] comments addressed --- optimizely/event/user_event_factory.py | 4 ++-- tests/test_user_context.py | 25 +++++++++++++++++++++++++ tests/test_user_event_factory.py | 11 ----------- 3 files changed, 27 insertions(+), 13 deletions(-) diff --git a/optimizely/event/user_event_factory.py b/optimizely/event/user_event_factory.py index 36a5593b..fb5c70ed 100644 --- a/optimizely/event/user_event_factory.py +++ b/optimizely/event/user_event_factory.py @@ -50,10 +50,10 @@ def create_impression_event( experiment_id = activated_experiment.id if variation_id and flag_key: - variation = project_config.get_flag_variation(flag_key, 'id', variation_id) - elif variation_id and experiment_id: # need this condition when we send events involving forced decisions # (F-to-D or E-to-D with any ruleKey/variationKey combinations) + variation = project_config.get_flag_variation(flag_key, 'id', variation_id) + elif variation_id and experiment_id: variation = project_config.get_variation_from_id_by_experiment_id(experiment_id, variation_id) event_context = user_event.EventContext( diff --git a/tests/test_user_context.py b/tests/test_user_context.py index 4b88e87a..dc52c648 100644 --- a/tests/test_user_context.py +++ b/tests/test_user_context.py @@ -1521,6 +1521,31 @@ def test_should_return_valid_experiment_decision_after_setting_forced_decision(s self.assertEqual(decide_decision.reasons, expected_reasons) + def test_should_return_valid_decision_after_setting_variation_of_different_experiment_in_forced_decision(self): + """ + Should return valid decision after setting setting variation of different experiment in forced decision. + """ + opt_obj = optimizely.Optimizely(json.dumps(self.config_dict_with_features)) + user_context = opt_obj.create_user_context("test_user", {}) + + context = OptimizelyUserContext.OptimizelyDecisionContext('test_feature_in_experiment_and_rollout', + 'group_exp_2') + decision = OptimizelyUserContext.OptimizelyForcedDecision('211129') + + status = user_context.set_forced_decision(context, decision) + self.assertTrue(status) + status = user_context.get_forced_decision(context) + self.assertEqual(status.variation_key, '211129') + + decide_decision = user_context.decide('test_feature_in_experiment_and_rollout', ['INCLUDE_REASONS']) + + self.assertEqual(decide_decision.variation_key, '211129') + self.assertEqual(decide_decision.rule_key, 'group_exp_2') + self.assertTrue(decide_decision.enabled) + self.assertEqual(decide_decision.flag_key, 'test_feature_in_experiment_and_rollout') + self.assertEqual(decide_decision.user_context.user_id, 'test_user') + self.assertEqual(decide_decision.user_context.get_user_attributes(), {}) + def test_should_return_valid_decision_after_setting_invalid_delivery_rule_variation_in_forced_decision(self): """ Should return valid decision after setting invalid delivery rule variation in forced decision. diff --git a/tests/test_user_event_factory.py b/tests/test_user_event_factory.py index 84b56e50..e723a823 100644 --- a/tests/test_user_event_factory.py +++ b/tests/test_user_event_factory.py @@ -70,17 +70,6 @@ def test_impression_event__with_attributes(self): [x.__dict__ for x in expected_attrs], [x.__dict__ for x in impression_event.visitor_attributes], ) - def test_impression_event__no_active_experiment(self): - project_config = self.project_config - user_id = 'test_user' - - user_attributes = {'test_attribute': 'test_value', 'boolean_key': True} - - impression_event = UserEventFactory.create_impression_event( - project_config, None, '111128', 'flag_key', 'rule_key', 'rule_type', True, user_id, user_attributes - ) - self.assertIsNone(impression_event) - def test_conversion_event(self): project_config = self.project_config user_id = 'test_user' From 06addb3c3e8385523fb830df9510df70c09472a1 Mon Sep 17 00:00:00 2001 From: ozayr-zaviar Date: Tue, 21 Dec 2021 18:59:43 +0500 Subject: [PATCH 7/7] test cases fix --- tests/test_event_factory.py | 28 ++++++++++++++-------------- tests/test_user_event_factory.py | 4 ++-- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/tests/test_event_factory.py b/tests/test_event_factory.py index 2e8a6192..ec92a3dd 100644 --- a/tests/test_event_factory.py +++ b/tests/test_event_factory.py @@ -75,7 +75,7 @@ def test_create_impression_event(self): { 'decisions': [ {'variation_id': '111129', 'experiment_id': '111127', 'campaign_id': '111182', - 'metadata': {'flag_key': 'flag_key', + 'metadata': {'flag_key': '', 'rule_key': 'rule_key', 'rule_type': 'experiment', 'variation_key': 'variation', @@ -107,7 +107,7 @@ def test_create_impression_event(self): self.project_config, self.project_config.get_experiment_from_key('test_experiment'), '111129', - 'flag_key', + '', 'rule_key', 'experiment', False, @@ -138,7 +138,7 @@ def test_create_impression_event__with_attributes(self): { 'decisions': [ {'variation_id': '111129', 'experiment_id': '111127', 'campaign_id': '111182', - 'metadata': {'flag_key': 'flag_key', + 'metadata': {'flag_key': '', 'rule_key': 'rule_key', 'rule_type': 'experiment', 'variation_key': 'variation', @@ -171,7 +171,7 @@ def test_create_impression_event__with_attributes(self): self.project_config, self.project_config.get_experiment_from_key('test_experiment'), '111129', - 'flag_key', + '', 'rule_key', 'experiment', True, @@ -200,7 +200,7 @@ def test_create_impression_event_when_attribute_is_not_in_datafile(self): { 'decisions': [ {'variation_id': '111129', 'experiment_id': '111127', 'campaign_id': '111182', - 'metadata': {'flag_key': 'flag_key', + 'metadata': {'flag_key': '', 'rule_key': 'rule_key', 'rule_type': 'experiment', 'variation_key': 'variation', @@ -233,7 +233,7 @@ def test_create_impression_event_when_attribute_is_not_in_datafile(self): self.project_config, self.project_config.get_experiment_from_key('test_experiment'), '111129', - 'flag_key', + '', 'rule_key', 'experiment', True, @@ -265,7 +265,7 @@ def test_create_impression_event_calls_is_attribute_valid(self): { 'decisions': [ {'variation_id': '111129', 'experiment_id': '111127', 'campaign_id': '111182', - 'metadata': {'flag_key': 'flag_key', + 'metadata': {'flag_key': '', 'flag_type': 'experiment', 'variation_key': 'variation'}, } @@ -313,7 +313,7 @@ def side_effect(*args, **kwargs): self.project_config, self.project_config.get_experiment_from_key('test_experiment'), '111129', - 'flag_key', + '', 'experiment', 'test_user', attributes, @@ -353,7 +353,7 @@ def test_create_impression_event__with_user_agent_when_bot_filtering_is_enabled( { 'decisions': [ {'variation_id': '111129', 'experiment_id': '111127', 'campaign_id': '111182', - 'metadata': {'flag_key': 'flag_key', + 'metadata': {'flag_key': '', 'rule_key': 'rule_key', 'rule_type': 'experiment', 'variation_key': 'variation', @@ -388,7 +388,7 @@ def test_create_impression_event__with_user_agent_when_bot_filtering_is_enabled( self.project_config, self.project_config.get_experiment_from_key('test_experiment'), '111129', - 'flag_key', + '', 'rule_key', 'experiment', False, @@ -425,7 +425,7 @@ def test_create_impression_event__with_empty_attributes_when_bot_filtering_is_en { 'decisions': [ {'variation_id': '111129', 'experiment_id': '111127', 'campaign_id': '111182', - 'metadata': {'flag_key': 'flag_key', + 'metadata': {'flag_key': '', 'rule_key': 'rule_key', 'rule_type': 'experiment', 'variation_key': 'variation', @@ -460,7 +460,7 @@ def test_create_impression_event__with_empty_attributes_when_bot_filtering_is_en self.project_config, self.project_config.get_experiment_from_key('test_experiment'), '111129', - 'flag_key', + '', 'rule_key', 'experiment', False, @@ -503,7 +503,7 @@ def test_create_impression_event__with_user_agent_when_bot_filtering_is_disabled { 'decisions': [ {'variation_id': '111129', 'experiment_id': '111127', 'campaign_id': '111182', - 'metadata': {'flag_key': 'flag_key', + 'metadata': {'flag_key': '', 'rule_key': 'rule_key', 'rule_type': 'experiment', 'variation_key': 'variation', @@ -538,7 +538,7 @@ def test_create_impression_event__with_user_agent_when_bot_filtering_is_disabled self.project_config, self.project_config.get_experiment_from_key('test_experiment'), '111129', - 'flag_key', + '', 'rule_key', 'experiment', True, diff --git a/tests/test_user_event_factory.py b/tests/test_user_event_factory.py index e723a823..009ef05d 100644 --- a/tests/test_user_event_factory.py +++ b/tests/test_user_event_factory.py @@ -28,7 +28,7 @@ def test_impression_event(self): variation = self.project_config.get_variation_from_id(experiment.key, '111128') user_id = 'test_user' - impression_event = UserEventFactory.create_impression_event(project_config, experiment, '111128', 'flag_key', + impression_event = UserEventFactory.create_impression_event(project_config, experiment, '111128', '', 'rule_key', 'rule_type', True, user_id, None) self.assertEqual(self.project_config.project_id, impression_event.event_context.project_id) @@ -51,7 +51,7 @@ def test_impression_event__with_attributes(self): user_attributes = {'test_attribute': 'test_value', 'boolean_key': True} impression_event = UserEventFactory.create_impression_event( - project_config, experiment, '111128', 'flag_key', 'rule_key', 'rule_type', True, user_id, user_attributes + project_config, experiment, '111128', '', 'rule_key', 'rule_type', True, user_id, user_attributes ) expected_attrs = EventFactory.build_attribute_list(user_attributes, project_config)