Skip to content

Commit 53421a6

Browse files
esrakartalOptesrakartalOpt
authored
[FSSDK-11572] Python: Update impression event handling and send notification for global holdout (#469)
* [FSSDK-11572] Python: Update impression event handling and send notification for global holdout * Implement suggested comments
1 parent 1225cfb commit 53421a6

File tree

2 files changed

+356
-12
lines changed

2 files changed

+356
-12
lines changed

‎optimizely/optimizely.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -453,7 +453,7 @@ def _get_feature_variable_for_type(
453453
f'Returning default value for variable "{variable_key}" of feature flag "{feature_key}".'
454454
)
455455

456-
if decision.source == enums.DecisionSources.FEATURE_TEST:
456+
if decision.source in (enums.DecisionSources.FEATURE_TEST, enums.DecisionSources.HOLDOUT):
457457
source_info = {
458458
'experiment_key': decision.experiment.key if decision.experiment else None,
459459
'variation_key': self._get_variation_key(decision.variation),

‎tests/test_decision_service_holdout.py‎

Lines changed: 355 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -278,20 +278,49 @@ def test_user_bucketed_into_holdout_returns_before_experiments(self):
278278

279279
user_context = self.opt_obj.create_user_context('testUserId', {})
280280

281-
decision_result = self.decision_service_with_holdouts.get_variation_for_feature(
282-
self.config_with_holdouts,
283-
feature_flag,
284-
user_context
281+
# Mock get_holdouts_for_flag to return holdouts
282+
holdout = self.config_with_holdouts.holdouts[0] if self.config_with_holdouts.holdouts else None
283+
self.assertIsNotNone(holdout)
284+
285+
holdout_variation = holdout['variations'][0]
286+
287+
# Create a holdout decision
288+
mock_holdout_decision = decision_service.Decision(
289+
experiment=holdout,
290+
variation=holdout_variation,
291+
source=enums.DecisionSources.HOLDOUT,
292+
cmab_uuid=None
285293
)
286294

287-
self.assertIsNotNone(decision_result)
295+
mock_holdout_result = {
296+
'decision': mock_holdout_decision,
297+
'error': False,
298+
'reasons': []
299+
}
288300

289-
# Decision should be valid
290-
if decision_result.get('decision'):
291-
decision = decision_result['decision']
292-
self.assertEqual(decision.source, enums.DecisionSources.HOLDOUT)
293-
self.assertIsNotNone(decision.variation)
294-
self.assertIsNone(decision.experiment)
301+
# Mock get_holdouts_for_flag to return holdouts so the holdout path is taken
302+
with mock.patch.object(
303+
self.config_with_holdouts,
304+
'get_holdouts_for_flag',
305+
return_value=[holdout]
306+
):
307+
with mock.patch.object(
308+
self.opt_obj.decision_service,
309+
'get_variation_for_holdout',
310+
return_value=mock_holdout_result
311+
):
312+
decision_result = self.opt_obj.decision_service.get_variation_for_feature(
313+
self.config_with_holdouts,
314+
feature_flag,
315+
user_context
316+
)
317+
318+
self.assertIsNotNone(decision_result)
319+
320+
# Decision should be valid and from holdout
321+
decision = decision_result['decision']
322+
self.assertEqual(decision.source, enums.DecisionSources.HOLDOUT)
323+
self.assertIsNotNone(decision.variation)
295324

296325
def test_no_holdout_decision_falls_through_to_experiment_and_rollout(self):
297326
"""When holdout returns no decision, should fall through to experiment and rollout evaluation."""
@@ -566,3 +595,318 @@ def test_falls_back_to_experiments_if_no_holdout_decision(self):
566595
self.assertIsNotNone(decision_result)
567596
self.assertIn('decision', decision_result)
568597
self.assertIn('reasons', decision_result)
598+
599+
# Holdout Impression Events tests
600+
601+
def test_decide_with_holdout_sends_impression_event(self):
602+
"""Should send impression event for holdout decision."""
603+
# Create optimizely instance with mocked event processor
604+
spy_event_processor = mock.MagicMock()
605+
606+
config_dict_with_holdouts = self.config_dict_with_features.copy()
607+
config_dict_with_holdouts['holdouts'] = [
608+
{
609+
'id': 'holdout_1',
610+
'key': 'test_holdout',
611+
'status': 'Running',
612+
'includedFlags': [],
613+
'excludedFlags': [],
614+
'audienceIds': [],
615+
'variations': [
616+
{
617+
'id': 'holdout_var_1',
618+
'key': 'holdout_control',
619+
'featureEnabled': True,
620+
'variables': []
621+
},
622+
{
623+
'id': 'holdout_var_2',
624+
'key': 'holdout_treatment',
625+
'featureEnabled': False,
626+
'variables': []
627+
}
628+
],
629+
'trafficAllocation': [
630+
{
631+
'entityId': 'holdout_var_1',
632+
'endOfRange': 5000
633+
},
634+
{
635+
'entityId': 'holdout_var_2',
636+
'endOfRange': 10000
637+
}
638+
]
639+
}
640+
]
641+
642+
config_json = json.dumps(config_dict_with_holdouts)
643+
opt_with_mocked_events = optimizely_module.Optimizely(
644+
datafile=config_json,
645+
logger=self.spy_logger,
646+
error_handler=self.error_handler,
647+
event_processor=spy_event_processor
648+
)
649+
650+
try:
651+
# Use a specific user ID that will be bucketed into a holdout
652+
test_user_id = 'user_bucketed_into_holdout'
653+
654+
config = opt_with_mocked_events.config_manager.get_config()
655+
feature_flag = config.get_feature_from_key('test_feature_in_experiment')
656+
self.assertIsNotNone(feature_flag, "Feature flag 'test_feature_in_experiment' should exist")
657+
658+
user_attributes = {}
659+
660+
user_context = opt_with_mocked_events.create_user_context(test_user_id, user_attributes)
661+
decision = user_context.decide(feature_flag.key)
662+
663+
self.assertIsNotNone(decision, 'Decision should not be None')
664+
665+
# Find the actual holdout if this is a holdout decision
666+
actual_holdout = None
667+
if config.holdouts and decision.rule_key:
668+
actual_holdout = next(
669+
(h for h in config.holdouts if h.get('key') == decision.rule_key),
670+
None
671+
)
672+
673+
# Only continue if this is a holdout decision
674+
if actual_holdout:
675+
self.assertEqual(decision.flag_key, feature_flag.key)
676+
677+
holdout_variation = next(
678+
(v for v in actual_holdout['variations'] if v.get('key') == decision.variation_key),
679+
None
680+
)
681+
682+
self.assertIsNotNone(
683+
holdout_variation,
684+
f"Variation '{decision.variation_key}' should be from the chosen holdout '{actual_holdout['key']}'"
685+
)
686+
687+
self.assertEqual(
688+
decision.enabled,
689+
holdout_variation.get('featureEnabled'),
690+
"Enabled flag should match holdout variation's featureEnabled value"
691+
)
692+
693+
# Verify impression event was sent
694+
self.assertGreater(spy_event_processor.process.call_count, 0)
695+
696+
# Verify impression event contains correct user details
697+
call_args_list = spy_event_processor.process.call_args_list
698+
user_event_found = False
699+
for call_args in call_args_list:
700+
if call_args[0]: # Check positional args
701+
user_event = call_args[0][0]
702+
if hasattr(user_event, 'user_id') and user_event.user_id == test_user_id:
703+
user_event_found = True
704+
break
705+
706+
self.assertTrue(user_event_found, 'Impression event should contain correct user ID')
707+
finally:
708+
opt_with_mocked_events.close()
709+
710+
def test_decide_with_holdout_does_not_send_impression_when_disabled(self):
711+
"""Should not send impression event when DISABLE_DECISION_EVENT option is used."""
712+
# Create optimizely instance with mocked event processor
713+
spy_event_processor = mock.MagicMock()
714+
715+
config_dict_with_holdouts = self.config_dict_with_features.copy()
716+
config_dict_with_holdouts['holdouts'] = [
717+
{
718+
'id': 'holdout_1',
719+
'key': 'test_holdout',
720+
'status': 'Running',
721+
'includedFlags': [],
722+
'excludedFlags': [],
723+
'audienceIds': [],
724+
'variations': [
725+
{
726+
'id': 'holdout_var_1',
727+
'key': 'holdout_control',
728+
'featureEnabled': True,
729+
'variables': []
730+
}
731+
],
732+
'trafficAllocation': [
733+
{
734+
'entityId': 'holdout_var_1',
735+
'endOfRange': 10000
736+
}
737+
]
738+
}
739+
]
740+
741+
config_json = json.dumps(config_dict_with_holdouts)
742+
opt_with_mocked_events = optimizely_module.Optimizely(
743+
datafile=config_json,
744+
logger=self.spy_logger,
745+
error_handler=self.error_handler,
746+
event_processor=spy_event_processor
747+
)
748+
749+
try:
750+
test_user_id = 'user_bucketed_into_holdout'
751+
752+
config = opt_with_mocked_events.config_manager.get_config()
753+
feature_flag = config.get_feature_from_key('test_feature_in_experiment')
754+
self.assertIsNotNone(feature_flag)
755+
756+
user_attributes = {}
757+
758+
user_context = opt_with_mocked_events.create_user_context(test_user_id, user_attributes)
759+
decision = user_context.decide(
760+
feature_flag.key,
761+
[OptimizelyDecideOption.DISABLE_DECISION_EVENT]
762+
)
763+
764+
self.assertIsNotNone(decision, 'Decision should not be None')
765+
766+
# Find the chosen holdout if this is a holdout decision
767+
chosen_holdout = None
768+
if config.holdouts and decision.rule_key:
769+
chosen_holdout = next(
770+
(h for h in config.holdouts if h.get('key') == decision.rule_key),
771+
None
772+
)
773+
774+
if chosen_holdout:
775+
self.assertEqual(decision.flag_key, feature_flag.key)
776+
777+
# Verify no impression event was sent
778+
spy_event_processor.process.assert_not_called()
779+
finally:
780+
opt_with_mocked_events.close()
781+
782+
def test_decide_with_holdout_sends_correct_notification_content(self):
783+
"""Should send correct notification content for holdout decision."""
784+
captured_notifications = []
785+
786+
def notification_callback(notification_type, user_id, user_attributes, decision_info):
787+
captured_notifications.append(decision_info.copy())
788+
789+
config_dict_with_holdouts = self.config_dict_with_features.copy()
790+
config_dict_with_holdouts['holdouts'] = [
791+
{
792+
'id': 'holdout_1',
793+
'key': 'test_holdout',
794+
'status': 'Running',
795+
'includedFlags': [],
796+
'excludedFlags': [],
797+
'audienceIds': [],
798+
'variations': [
799+
{
800+
'id': 'holdout_var_1',
801+
'key': 'holdout_control',
802+
'featureEnabled': True,
803+
'variables': []
804+
}
805+
],
806+
'trafficAllocation': [
807+
{
808+
'entityId': 'holdout_var_1',
809+
'endOfRange': 10000
810+
}
811+
]
812+
}
813+
]
814+
815+
config_json = json.dumps(config_dict_with_holdouts)
816+
opt_with_mocked_events = optimizely_module.Optimizely(
817+
datafile=config_json,
818+
logger=self.spy_logger,
819+
error_handler=self.error_handler
820+
)
821+
822+
try:
823+
opt_with_mocked_events.notification_center.add_notification_listener(
824+
enums.NotificationTypes.DECISION,
825+
notification_callback
826+
)
827+
828+
test_user_id = 'holdout_test_user'
829+
config = opt_with_mocked_events.config_manager.get_config()
830+
feature_flag = config.get_feature_from_key('test_feature_in_experiment')
831+
holdout = config.holdouts[0] if config.holdouts else None
832+
self.assertIsNotNone(holdout, 'Should have at least one holdout configured')
833+
834+
holdout_variation = holdout['variations'][0]
835+
self.assertIsNotNone(holdout_variation, 'Holdout should have at least one variation')
836+
837+
mock_experiment = mock.MagicMock()
838+
mock_experiment.key = holdout['key']
839+
mock_experiment.id = holdout['id']
840+
841+
# Mock the decision service to return a holdout decision
842+
holdout_decision = decision_service.Decision(
843+
experiment=mock_experiment,
844+
variation=holdout_variation,
845+
source=enums.DecisionSources.HOLDOUT,
846+
cmab_uuid=None
847+
)
848+
849+
holdout_decision_result = {
850+
'decision': holdout_decision,
851+
'error': False,
852+
'reasons': []
853+
}
854+
855+
# Mock get_variations_for_feature_list to return holdout decision
856+
with mock.patch.object(
857+
opt_with_mocked_events.decision_service,
858+
'get_variations_for_feature_list',
859+
return_value=[holdout_decision_result]
860+
):
861+
user_context = opt_with_mocked_events.create_user_context(test_user_id, {})
862+
decision = user_context.decide(feature_flag.key)
863+
864+
self.assertIsNotNone(decision, 'Decision should not be None')
865+
self.assertEqual(len(captured_notifications), 1, 'Should have captured exactly one decision notification')
866+
867+
notification = captured_notifications[0]
868+
rule_key = notification.get('rule_key')
869+
870+
self.assertEqual(rule_key, holdout['key'], 'RuleKey should match holdout key')
871+
872+
# Verify holdout notification structure
873+
self.assertIn('flag_key', notification, 'Holdout notification should contain flag_key')
874+
self.assertIn('enabled', notification, 'Holdout notification should contain enabled flag')
875+
self.assertIn('variation_key', notification, 'Holdout notification should contain variation_key')
876+
self.assertIn('experiment_id', notification, 'Holdout notification should contain experiment_id')
877+
self.assertIn('variation_id', notification, 'Holdout notification should contain variation_id')
878+
879+
flag_key = notification.get('flag_key')
880+
self.assertEqual(flag_key, 'test_feature_in_experiment', 'FlagKey should match the requested flag')
881+
882+
experiment_id = notification.get('experiment_id')
883+
self.assertEqual(experiment_id, holdout['id'], 'ExperimentId in notification should match holdout ID')
884+
885+
variation_id = notification.get('variation_id')
886+
self.assertEqual(variation_id, holdout_variation['id'], 'VariationId should match holdout variation ID')
887+
888+
variation_key = notification.get('variation_key')
889+
self.assertEqual(
890+
variation_key,
891+
holdout_variation['key'],
892+
'VariationKey in notification should match holdout variation key'
893+
)
894+
895+
enabled = notification.get('enabled')
896+
self.assertIsNotNone(enabled, 'Enabled flag should be present in notification')
897+
self.assertEqual(
898+
enabled,
899+
holdout_variation.get('featureEnabled'),
900+
"Enabled flag should match holdout variation's featureEnabled value"
901+
)
902+
903+
self.assertIn(flag_key, config.feature_key_map, f"FlagKey '{flag_key}' should exist in config")
904+
905+
self.assertIn('variables', notification, 'Notification should contain variables')
906+
self.assertIn('reasons', notification, 'Notification should contain reasons')
907+
self.assertIn(
908+
'decision_event_dispatched', notification,
909+
'Notification should contain decision_event_dispatched'
910+
)
911+
finally:
912+
opt_with_mocked_events.close()

0 commit comments

Comments
 (0)