Skip to content

Commit 3dc944c

Browse files
ozayr-zaviarmsohailhussainjaeopt
authored
fix: Forced variation not available in experiment (#367)
* init * conditions fixed * unit test added for no active experiment in create impression event * Apply suggestions from code review Co-authored-by: Jae Kim <45045038+jaeopt@users.noreply.github.com> * linting fixed * comments addressed * test cases fix Co-authored-by: msohailhussain <mirza.sohailhussain@gmail.com> Co-authored-by: Jae Kim <45045038+jaeopt@users.noreply.github.com>
1 parent c6fd795 commit 3dc944c

File tree

5 files changed

+55
-27
lines changed

5 files changed

+55
-27
lines changed

‎optimizely/event/user_event_factory.py‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -49,11 +49,13 @@ def create_impression_event(
4949
if activated_experiment:
5050
experiment_id = activated_experiment.id
5151

52-
if variation_id and experiment_id:
53-
variation = project_config.get_variation_from_id_by_experiment_id(experiment_id, variation_id)
54-
# need this condition when we send events involving forced decisions
55-
elif variation_id and flag_key:
52+
if variation_id and flag_key:
53+
# need this condition when we send events involving forced decisions
54+
# (F-to-D or E-to-D with any ruleKey/variationKey combinations)
5655
variation = project_config.get_flag_variation(flag_key, 'id', variation_id)
56+
elif variation_id and experiment_id:
57+
variation = project_config.get_variation_from_id_by_experiment_id(experiment_id, variation_id)
58+
5759
event_context = user_event.EventContext(
5860
project_config.account_id, project_config.project_id, project_config.revision, project_config.anonymize_ip,
5961
)

‎optimizely/project_config.py‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -612,8 +612,8 @@ def get_variation_from_id_by_experiment_id(self, experiment_id, variation_id):
612612
variation_id in self.variation_id_map_by_experiment_id[experiment_id]):
613613
return self.variation_id_map_by_experiment_id[experiment_id][variation_id]
614614

615-
self.logger.error('Variation with id "%s" not defined in the datafile for experiment "%s".',
616-
variation_id, experiment_id)
615+
self.logger.error('Variation with id "%s" not defined in the datafile for experiment "%s".' %
616+
(variation_id, experiment_id))
617617

618618
return {}
619619

@@ -628,8 +628,8 @@ def get_variation_from_key_by_experiment_id(self, experiment_id, variation_key):
628628
variation_key in self.variation_key_map_by_experiment_id[experiment_id]):
629629
return self.variation_key_map_by_experiment_id[experiment_id][variation_key]
630630

631-
self.logger.error('Variation with key "%s" not defined in the datafile for experiment "%s".',
632-
variation_key, experiment_id)
631+
self.logger.error('Variation with key "%s" not defined in the datafile for experiment "%s".' %
632+
(variation_key, experiment_id))
633633

634634
return {}
635635

@@ -661,8 +661,9 @@ def get_flag_variation(self, flag_key, variation_attribute, target_value):
661661
return None
662662

663663
variations = self.flag_variations_map.get(flag_key)
664-
for variation in variations:
665-
if getattr(variation, variation_attribute) == target_value:
666-
return variation
664+
if variations:
665+
for variation in variations:
666+
if getattr(variation, variation_attribute) == target_value:
667+
return variation
667668

668669
return None

‎tests/test_event_factory.py‎

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ def test_create_impression_event(self):
7575
{
7676
'decisions': [
7777
{'variation_id': '111129', 'experiment_id': '111127', 'campaign_id': '111182',
78-
'metadata': {'flag_key': 'flag_key',
78+
'metadata': {'flag_key': '',
7979
'rule_key': 'rule_key',
8080
'rule_type': 'experiment',
8181
'variation_key': 'variation',
@@ -107,7 +107,7 @@ def test_create_impression_event(self):
107107
self.project_config,
108108
self.project_config.get_experiment_from_key('test_experiment'),
109109
'111129',
110-
'flag_key',
110+
'',
111111
'rule_key',
112112
'experiment',
113113
False,
@@ -138,7 +138,7 @@ def test_create_impression_event__with_attributes(self):
138138
{
139139
'decisions': [
140140
{'variation_id': '111129', 'experiment_id': '111127', 'campaign_id': '111182',
141-
'metadata': {'flag_key': 'flag_key',
141+
'metadata': {'flag_key': '',
142142
'rule_key': 'rule_key',
143143
'rule_type': 'experiment',
144144
'variation_key': 'variation',
@@ -171,7 +171,7 @@ def test_create_impression_event__with_attributes(self):
171171
self.project_config,
172172
self.project_config.get_experiment_from_key('test_experiment'),
173173
'111129',
174-
'flag_key',
174+
'',
175175
'rule_key',
176176
'experiment',
177177
True,
@@ -200,7 +200,7 @@ def test_create_impression_event_when_attribute_is_not_in_datafile(self):
200200
{
201201
'decisions': [
202202
{'variation_id': '111129', 'experiment_id': '111127', 'campaign_id': '111182',
203-
'metadata': {'flag_key': 'flag_key',
203+
'metadata': {'flag_key': '',
204204
'rule_key': 'rule_key',
205205
'rule_type': 'experiment',
206206
'variation_key': 'variation',
@@ -233,7 +233,7 @@ def test_create_impression_event_when_attribute_is_not_in_datafile(self):
233233
self.project_config,
234234
self.project_config.get_experiment_from_key('test_experiment'),
235235
'111129',
236-
'flag_key',
236+
'',
237237
'rule_key',
238238
'experiment',
239239
True,
@@ -265,7 +265,7 @@ def test_create_impression_event_calls_is_attribute_valid(self):
265265
{
266266
'decisions': [
267267
{'variation_id': '111129', 'experiment_id': '111127', 'campaign_id': '111182',
268-
'metadata': {'flag_key': 'flag_key',
268+
'metadata': {'flag_key': '',
269269
'flag_type': 'experiment',
270270
'variation_key': 'variation'},
271271
}
@@ -313,7 +313,7 @@ def side_effect(*args, **kwargs):
313313
self.project_config,
314314
self.project_config.get_experiment_from_key('test_experiment'),
315315
'111129',
316-
'flag_key',
316+
'',
317317
'experiment',
318318
'test_user',
319319
attributes,
@@ -353,7 +353,7 @@ def test_create_impression_event__with_user_agent_when_bot_filtering_is_enabled(
353353
{
354354
'decisions': [
355355
{'variation_id': '111129', 'experiment_id': '111127', 'campaign_id': '111182',
356-
'metadata': {'flag_key': 'flag_key',
356+
'metadata': {'flag_key': '',
357357
'rule_key': 'rule_key',
358358
'rule_type': 'experiment',
359359
'variation_key': 'variation',
@@ -388,7 +388,7 @@ def test_create_impression_event__with_user_agent_when_bot_filtering_is_enabled(
388388
self.project_config,
389389
self.project_config.get_experiment_from_key('test_experiment'),
390390
'111129',
391-
'flag_key',
391+
'',
392392
'rule_key',
393393
'experiment',
394394
False,
@@ -425,7 +425,7 @@ def test_create_impression_event__with_empty_attributes_when_bot_filtering_is_en
425425
{
426426
'decisions': [
427427
{'variation_id': '111129', 'experiment_id': '111127', 'campaign_id': '111182',
428-
'metadata': {'flag_key': 'flag_key',
428+
'metadata': {'flag_key': '',
429429
'rule_key': 'rule_key',
430430
'rule_type': 'experiment',
431431
'variation_key': 'variation',
@@ -460,7 +460,7 @@ def test_create_impression_event__with_empty_attributes_when_bot_filtering_is_en
460460
self.project_config,
461461
self.project_config.get_experiment_from_key('test_experiment'),
462462
'111129',
463-
'flag_key',
463+
'',
464464
'rule_key',
465465
'experiment',
466466
False,
@@ -503,7 +503,7 @@ def test_create_impression_event__with_user_agent_when_bot_filtering_is_disabled
503503
{
504504
'decisions': [
505505
{'variation_id': '111129', 'experiment_id': '111127', 'campaign_id': '111182',
506-
'metadata': {'flag_key': 'flag_key',
506+
'metadata': {'flag_key': '',
507507
'rule_key': 'rule_key',
508508
'rule_type': 'experiment',
509509
'variation_key': 'variation',
@@ -538,7 +538,7 @@ def test_create_impression_event__with_user_agent_when_bot_filtering_is_disabled
538538
self.project_config,
539539
self.project_config.get_experiment_from_key('test_experiment'),
540540
'111129',
541-
'flag_key',
541+
'',
542542
'rule_key',
543543
'experiment',
544544
True,

‎tests/test_user_context.py‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1521,6 +1521,31 @@ def test_should_return_valid_experiment_decision_after_setting_forced_decision(s
15211521

15221522
self.assertEqual(decide_decision.reasons, expected_reasons)
15231523

1524+
def test_should_return_valid_decision_after_setting_variation_of_different_experiment_in_forced_decision(self):
1525+
"""
1526+
Should return valid decision after setting setting variation of different experiment in forced decision.
1527+
"""
1528+
opt_obj = optimizely.Optimizely(json.dumps(self.config_dict_with_features))
1529+
user_context = opt_obj.create_user_context("test_user", {})
1530+
1531+
context = OptimizelyUserContext.OptimizelyDecisionContext('test_feature_in_experiment_and_rollout',
1532+
'group_exp_2')
1533+
decision = OptimizelyUserContext.OptimizelyForcedDecision('211129')
1534+
1535+
status = user_context.set_forced_decision(context, decision)
1536+
self.assertTrue(status)
1537+
status = user_context.get_forced_decision(context)
1538+
self.assertEqual(status.variation_key, '211129')
1539+
1540+
decide_decision = user_context.decide('test_feature_in_experiment_and_rollout', ['INCLUDE_REASONS'])
1541+
1542+
self.assertEqual(decide_decision.variation_key, '211129')
1543+
self.assertEqual(decide_decision.rule_key, 'group_exp_2')
1544+
self.assertTrue(decide_decision.enabled)
1545+
self.assertEqual(decide_decision.flag_key, 'test_feature_in_experiment_and_rollout')
1546+
self.assertEqual(decide_decision.user_context.user_id, 'test_user')
1547+
self.assertEqual(decide_decision.user_context.get_user_attributes(), {})
1548+
15241549
def test_should_return_valid_decision_after_setting_invalid_delivery_rule_variation_in_forced_decision(self):
15251550
"""
15261551
Should return valid decision after setting invalid delivery rule variation in forced decision.

‎tests/test_user_event_factory.py‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ def test_impression_event(self):
2828
variation = self.project_config.get_variation_from_id(experiment.key, '111128')
2929
user_id = 'test_user'
3030

31-
impression_event = UserEventFactory.create_impression_event(project_config, experiment, '111128', 'flag_key',
31+
impression_event = UserEventFactory.create_impression_event(project_config, experiment, '111128', '',
3232
'rule_key', 'rule_type', True, user_id, None)
3333

3434
self.assertEqual(self.project_config.project_id, impression_event.event_context.project_id)
@@ -51,7 +51,7 @@ def test_impression_event__with_attributes(self):
5151
user_attributes = {'test_attribute': 'test_value', 'boolean_key': True}
5252

5353
impression_event = UserEventFactory.create_impression_event(
54-
project_config, experiment, '111128', 'flag_key', 'rule_key', 'rule_type', True, user_id, user_attributes
54+
project_config, experiment, '111128', '', 'rule_key', 'rule_type', True, user_id, user_attributes
5555
)
5656

5757
expected_attrs = EventFactory.build_attribute_list(user_attributes, project_config)

0 commit comments

Comments
 (0)