From 70c281ef0fa05f949509e18436068f3d377f5aaa Mon Sep 17 00:00:00 2001 From: esrakartalOpt Date: Thu, 13 Nov 2025 15:52:56 -0600 Subject: [PATCH 1/2] [FSSDK-11572] Python: Update impression event handling and send notification for global holdout --- optimizely/decision_service.py | 4 +- optimizely/optimizely.py | 2 +- optimizely/project_config.py | 6 +- tests/test_decision_service_holdout.py | 366 ++++++++++++++++++++++++- 4 files changed, 361 insertions(+), 17 deletions(-) diff --git a/optimizely/decision_service.py b/optimizely/decision_service.py index 78be89d8..5c9698aa 100644 --- a/optimizely/decision_service.py +++ b/optimizely/decision_service.py @@ -672,7 +672,7 @@ def get_variation_for_feature( - 'error': Boolean indicating if an error occurred during the decision process. - 'reasons': List of log messages representing decision making for the feature. """ - holdouts = project_config.get_holdouts_for_flag(feature.key) + holdouts = project_config.get_holdouts_for_flag(feature.id) if holdouts: # Has holdouts - use get_decision_for_flag which checks holdouts first @@ -708,7 +708,7 @@ def get_decision_for_flag( user_id = user_context.user_id # Check holdouts - holdouts = project_config.get_holdouts_for_flag(feature_flag.key) + holdouts = project_config.get_holdouts_for_flag(feature_flag.id) for holdout in holdouts: holdout_decision = self.get_variation_for_holdout(holdout, user_context, project_config) reasons.extend(holdout_decision['reasons']) diff --git a/optimizely/optimizely.py b/optimizely/optimizely.py index b4c281ed..7b27d849 100644 --- a/optimizely/optimizely.py +++ b/optimizely/optimizely.py @@ -453,7 +453,7 @@ def _get_feature_variable_for_type( f'Returning default value for variable "{variable_key}" of feature flag "{feature_key}".' ) - if decision.source == enums.DecisionSources.FEATURE_TEST: + if decision.source in (enums.DecisionSources.FEATURE_TEST, enums.DecisionSources.HOLDOUT): source_info = { 'experiment_key': decision.experiment.key if decision.experiment else None, 'variation_key': self._get_variation_key(decision.variation), diff --git a/optimizely/project_config.py b/optimizely/project_config.py index 23508bb6..7c044c1e 100644 --- a/optimizely/project_config.py +++ b/optimizely/project_config.py @@ -834,11 +834,11 @@ def get_flag_variation( return None - def get_holdouts_for_flag(self, flag_key: str) -> list[HoldoutDict]: + def get_holdouts_for_flag(self, flag_id: str) -> list[HoldoutDict]: """ Helper method to get holdouts from an applied feature flag. Args: - flag_key: Key of the feature flag. + flag_id: (REQUIRED) ID of the feature flag. Returns: The holdouts that apply for a specific flag. @@ -846,7 +846,7 @@ def get_holdouts_for_flag(self, flag_key: str) -> list[HoldoutDict]: if not self.holdouts: return [] - return self.flag_holdouts_map.get(flag_key, []) + return self.flag_holdouts_map.get(flag_id, []) def get_holdout(self, holdout_id: str) -> Optional[HoldoutDict]: """ Helper method to get holdout from holdout ID. diff --git a/tests/test_decision_service_holdout.py b/tests/test_decision_service_holdout.py index d4238218..0f47e997 100644 --- a/tests/test_decision_service_holdout.py +++ b/tests/test_decision_service_holdout.py @@ -278,20 +278,49 @@ def test_user_bucketed_into_holdout_returns_before_experiments(self): user_context = self.opt_obj.create_user_context('testUserId', {}) - decision_result = self.decision_service_with_holdouts.get_variation_for_feature( - self.config_with_holdouts, - feature_flag, - user_context + # Mock get_holdouts_for_flag to return holdouts + holdout = self.config_with_holdouts.holdouts[0] if self.config_with_holdouts.holdouts else None + self.assertIsNotNone(holdout) + + holdout_variation = holdout['variations'][0] + + # Create a holdout decision + mock_holdout_decision = decision_service.Decision( + experiment=holdout, + variation=holdout_variation, + source=enums.DecisionSources.HOLDOUT, + cmab_uuid=None ) - self.assertIsNotNone(decision_result) + mock_holdout_result = { + 'decision': mock_holdout_decision, + 'error': False, + 'reasons': [] + } - # Decision should be valid - if decision_result.get('decision'): - decision = decision_result['decision'] - self.assertEqual(decision.source, enums.DecisionSources.HOLDOUT) - self.assertIsNotNone(decision.variation) - self.assertIsNone(decision.experiment) + # Mock get_holdouts_for_flag to return holdouts so the holdout path is taken + with mock.patch.object( + self.config_with_holdouts, + 'get_holdouts_for_flag', + return_value=[holdout] + ): + with mock.patch.object( + self.opt_obj.decision_service, + 'get_variation_for_holdout', + return_value=mock_holdout_result + ): + decision_result = self.opt_obj.decision_service.get_variation_for_feature( + self.config_with_holdouts, + feature_flag, + user_context + ) + + self.assertIsNotNone(decision_result) + + # Decision should be valid and from holdout + decision = decision_result['decision'] + self.assertEqual(decision.source, enums.DecisionSources.HOLDOUT) + self.assertIsNotNone(decision.variation) def test_no_holdout_decision_falls_through_to_experiment_and_rollout(self): """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): self.assertIsNotNone(decision_result) self.assertIn('decision', decision_result) self.assertIn('reasons', decision_result) + + # Holdout Impression Events tests + + def test_decide_with_holdout_sends_impression_event(self): + """Should send impression event for holdout decision.""" + # Create optimizely instance with mocked event processor + spy_event_processor = mock.MagicMock() + + config_dict_with_holdouts = self.config_dict_with_features.copy() + config_dict_with_holdouts['holdouts'] = [ + { + 'id': 'holdout_1', + 'key': 'test_holdout', + 'status': 'Running', + 'includedFlags': [], + 'excludedFlags': [], + 'audienceIds': [], + 'variations': [ + { + 'id': 'holdout_var_1', + 'key': 'holdout_control', + 'featureEnabled': True, + 'variables': [] + }, + { + 'id': 'holdout_var_2', + 'key': 'holdout_treatment', + 'featureEnabled': False, + 'variables': [] + } + ], + 'trafficAllocation': [ + { + 'entityId': 'holdout_var_1', + 'endOfRange': 5000 + }, + { + 'entityId': 'holdout_var_2', + 'endOfRange': 10000 + } + ] + } + ] + + config_json = json.dumps(config_dict_with_holdouts) + opt_with_mocked_events = optimizely_module.Optimizely( + datafile=config_json, + logger=self.spy_logger, + error_handler=self.error_handler, + event_processor=spy_event_processor + ) + + try: + # Use a specific user ID that will be bucketed into a holdout + test_user_id = 'user_bucketed_into_holdout' + + config = opt_with_mocked_events.config_manager.get_config() + feature_flag = config.get_feature_from_key('test_feature_in_experiment') + self.assertIsNotNone(feature_flag, "Feature flag 'test_feature_in_experiment' should exist") + + user_attributes = {} + + user_context = opt_with_mocked_events.create_user_context(test_user_id, user_attributes) + decision = user_context.decide(feature_flag.key) + + self.assertIsNotNone(decision, 'Decision should not be None') + + # Find the actual holdout if this is a holdout decision + actual_holdout = None + if config.holdouts and decision.rule_key: + actual_holdout = next( + (h for h in config.holdouts if h.get('key') == decision.rule_key), + None + ) + + # Only continue if this is a holdout decision + if actual_holdout: + self.assertEqual(decision.flag_key, feature_flag.key) + + holdout_variation = next( + (v for v in actual_holdout['variations'] if v.get('key') == decision.variation_key), + None + ) + + self.assertIsNotNone( + holdout_variation, + f"Variation '{decision.variation_key}' should be from the chosen holdout '{actual_holdout['key']}'" + ) + + self.assertEqual( + decision.enabled, + holdout_variation.get('featureEnabled'), + "Enabled flag should match holdout variation's featureEnabled value" + ) + + # Verify impression event was sent + self.assertGreater(spy_event_processor.process.call_count, 0) + + # Verify impression event contains correct user details + call_args_list = spy_event_processor.process.call_args_list + user_event_found = False + for call_args in call_args_list: + if call_args[0]: # Check positional args + user_event = call_args[0][0] + if hasattr(user_event, 'user_id') and user_event.user_id == test_user_id: + user_event_found = True + break + + self.assertTrue(user_event_found, 'Impression event should contain correct user ID') + finally: + opt_with_mocked_events.close() + + def test_decide_with_holdout_does_not_send_impression_when_disabled(self): + """Should not send impression event when DISABLE_DECISION_EVENT option is used.""" + # Create optimizely instance with mocked event processor + spy_event_processor = mock.MagicMock() + + config_dict_with_holdouts = self.config_dict_with_features.copy() + config_dict_with_holdouts['holdouts'] = [ + { + 'id': 'holdout_1', + 'key': 'test_holdout', + 'status': 'Running', + 'includedFlags': [], + 'excludedFlags': [], + 'audienceIds': [], + 'variations': [ + { + 'id': 'holdout_var_1', + 'key': 'holdout_control', + 'featureEnabled': True, + 'variables': [] + } + ], + 'trafficAllocation': [ + { + 'entityId': 'holdout_var_1', + 'endOfRange': 10000 + } + ] + } + ] + + config_json = json.dumps(config_dict_with_holdouts) + opt_with_mocked_events = optimizely_module.Optimizely( + datafile=config_json, + logger=self.spy_logger, + error_handler=self.error_handler, + event_processor=spy_event_processor + ) + + try: + test_user_id = 'user_bucketed_into_holdout' + + config = opt_with_mocked_events.config_manager.get_config() + feature_flag = config.get_feature_from_key('test_feature_in_experiment') + self.assertIsNotNone(feature_flag) + + user_attributes = {} + + user_context = opt_with_mocked_events.create_user_context(test_user_id, user_attributes) + decision = user_context.decide( + feature_flag.key, + [OptimizelyDecideOption.DISABLE_DECISION_EVENT] + ) + + self.assertIsNotNone(decision, 'Decision should not be None') + + # Find the chosen holdout if this is a holdout decision + chosen_holdout = None + if config.holdouts and decision.rule_key: + chosen_holdout = next( + (h for h in config.holdouts if h.get('key') == decision.rule_key), + None + ) + + if chosen_holdout: + self.assertEqual(decision.flag_key, feature_flag.key) + + # Verify no impression event was sent + spy_event_processor.process.assert_not_called() + finally: + opt_with_mocked_events.close() + + def test_decide_with_holdout_sends_correct_notification_content(self): + """Should send correct notification content for holdout decision.""" + captured_notifications = [] + + def notification_callback(notification_type, user_id, user_attributes, decision_info): + captured_notifications.append(decision_info.copy()) + + config_dict_with_holdouts = self.config_dict_with_features.copy() + config_dict_with_holdouts['holdouts'] = [ + { + 'id': 'holdout_1', + 'key': 'test_holdout', + 'status': 'Running', + 'includedFlags': [], + 'excludedFlags': [], + 'audienceIds': [], + 'variations': [ + { + 'id': 'holdout_var_1', + 'key': 'holdout_control', + 'featureEnabled': True, + 'variables': [] + } + ], + 'trafficAllocation': [ + { + 'entityId': 'holdout_var_1', + 'endOfRange': 10000 + } + ] + } + ] + + config_json = json.dumps(config_dict_with_holdouts) + opt_with_mocked_events = optimizely_module.Optimizely( + datafile=config_json, + logger=self.spy_logger, + error_handler=self.error_handler + ) + + try: + opt_with_mocked_events.notification_center.add_notification_listener( + enums.NotificationTypes.DECISION, + notification_callback + ) + + test_user_id = 'holdout_test_user' + config = opt_with_mocked_events.config_manager.get_config() + feature_flag = config.get_feature_from_key('test_feature_in_experiment') + holdout = config.holdouts[0] if config.holdouts else None + self.assertIsNotNone(holdout, 'Should have at least one holdout configured') + + holdout_variation = holdout['variations'][0] + self.assertIsNotNone(holdout_variation, 'Holdout should have at least one variation') + + mock_experiment = mock.MagicMock() + mock_experiment.key = holdout['key'] + mock_experiment.id = holdout['id'] + + # Mock the decision service to return a holdout decision + holdout_decision = decision_service.Decision( + experiment=mock_experiment, + variation=holdout_variation, + source=enums.DecisionSources.HOLDOUT, + cmab_uuid=None + ) + + holdout_decision_result = { + 'decision': holdout_decision, + 'error': False, + 'reasons': [] + } + + # Mock get_variations_for_feature_list to return holdout decision + with mock.patch.object( + opt_with_mocked_events.decision_service, + 'get_variations_for_feature_list', + return_value=[holdout_decision_result] + ): + user_context = opt_with_mocked_events.create_user_context(test_user_id, {}) + decision = user_context.decide(feature_flag.key) + + self.assertIsNotNone(decision, 'Decision should not be None') + self.assertEqual(len(captured_notifications), 1, 'Should have captured exactly one decision notification') + + notification = captured_notifications[0] + rule_key = notification.get('rule_key') + + self.assertEqual(rule_key, holdout['key'], 'RuleKey should match holdout key') + + # Verify holdout notification structure + self.assertIn('flag_key', notification, 'Holdout notification should contain flag_key') + self.assertIn('enabled', notification, 'Holdout notification should contain enabled flag') + self.assertIn('variation_key', notification, 'Holdout notification should contain variation_key') + self.assertIn('experiment_id', notification, 'Holdout notification should contain experiment_id') + self.assertIn('variation_id', notification, 'Holdout notification should contain variation_id') + + flag_key = notification.get('flag_key') + self.assertEqual(flag_key, 'test_feature_in_experiment', 'FlagKey should match the requested flag') + + experiment_id = notification.get('experiment_id') + self.assertEqual(experiment_id, holdout['id'], 'ExperimentId in notification should match holdout ID') + + variation_id = notification.get('variation_id') + self.assertEqual(variation_id, holdout_variation['id'], 'VariationId should match holdout variation ID') + + variation_key = notification.get('variation_key') + self.assertEqual( + variation_key, + holdout_variation['key'], + 'VariationKey in notification should match holdout variation key' + ) + + enabled = notification.get('enabled') + self.assertIsNotNone(enabled, 'Enabled flag should be present in notification') + self.assertEqual( + enabled, + holdout_variation.get('featureEnabled'), + "Enabled flag should match holdout variation's featureEnabled value" + ) + + self.assertIn(flag_key, config.feature_key_map, f"FlagKey '{flag_key}' should exist in config") + + self.assertIn('variables', notification, 'Notification should contain variables') + self.assertIn('reasons', notification, 'Notification should contain reasons') + self.assertIn( + 'decision_event_dispatched', notification, + 'Notification should contain decision_event_dispatched' + ) + finally: + opt_with_mocked_events.close() From f9101e95b4ed53ab30e99be0952852182a814a6a Mon Sep 17 00:00:00 2001 From: esrakartalOpt Date: Tue, 18 Nov 2025 10:26:34 -0600 Subject: [PATCH 2/2] Implement suggested comments --- optimizely/decision_service.py | 4 ++-- optimizely/project_config.py | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/optimizely/decision_service.py b/optimizely/decision_service.py index 5c9698aa..78be89d8 100644 --- a/optimizely/decision_service.py +++ b/optimizely/decision_service.py @@ -672,7 +672,7 @@ def get_variation_for_feature( - 'error': Boolean indicating if an error occurred during the decision process. - 'reasons': List of log messages representing decision making for the feature. """ - holdouts = project_config.get_holdouts_for_flag(feature.id) + holdouts = project_config.get_holdouts_for_flag(feature.key) if holdouts: # Has holdouts - use get_decision_for_flag which checks holdouts first @@ -708,7 +708,7 @@ def get_decision_for_flag( user_id = user_context.user_id # Check holdouts - holdouts = project_config.get_holdouts_for_flag(feature_flag.id) + holdouts = project_config.get_holdouts_for_flag(feature_flag.key) for holdout in holdouts: holdout_decision = self.get_variation_for_holdout(holdout, user_context, project_config) reasons.extend(holdout_decision['reasons']) diff --git a/optimizely/project_config.py b/optimizely/project_config.py index 7c044c1e..23508bb6 100644 --- a/optimizely/project_config.py +++ b/optimizely/project_config.py @@ -834,11 +834,11 @@ def get_flag_variation( return None - def get_holdouts_for_flag(self, flag_id: str) -> list[HoldoutDict]: + def get_holdouts_for_flag(self, flag_key: str) -> list[HoldoutDict]: """ Helper method to get holdouts from an applied feature flag. Args: - flag_id: (REQUIRED) ID of the feature flag. + flag_key: Key of the feature flag. Returns: The holdouts that apply for a specific flag. @@ -846,7 +846,7 @@ def get_holdouts_for_flag(self, flag_id: str) -> list[HoldoutDict]: if not self.holdouts: return [] - return self.flag_holdouts_map.get(flag_id, []) + return self.flag_holdouts_map.get(flag_key, []) def get_holdout(self, holdout_id: str) -> Optional[HoldoutDict]: """ Helper method to get holdout from holdout ID.