From b91dc092bb2c7affdb53d0c34002a73daece1f58 Mon Sep 17 00:00:00 2001 From: esrakartalOpt Date: Tue, 21 Jul 2026 12:37:07 -0500 Subject: [PATCH 1/3] [AI-FSSDK] [FSSDK-12735] Add holdout exclusion logic for Targeted Delivery rules --- optimizely/decision_service.py | 44 +++- optimizely/entities.py | 2 + tests/test_decision_service_holdout.py | 282 +++++++++++++++++++++++++ tests/test_holdout_config.py | 31 +++ 4 files changed, 350 insertions(+), 9 deletions(-) diff --git a/optimizely/decision_service.py b/optimizely/decision_service.py index 11188908..0dcd2671 100644 --- a/optimizely/decision_service.py +++ b/optimizely/decision_service.py @@ -610,9 +610,11 @@ def get_variation_for_rollout( return Decision(experiment=rule, variation=forced_decision_variation, source=enums.DecisionSources.ROLLOUT, cmab_uuid=None), decide_reasons - # Check local holdouts targeting this specific delivery rule (FSSDK-12369) local_holdouts = project_config.get_holdouts_for_rule(rule.id) for holdout in local_holdouts: + if holdout.exclude_targeted_deliveries: + continue + local_holdout_decision = self.get_variation_for_holdout( holdout, user_context, project_config ) @@ -751,6 +753,8 @@ def get_decision_for_flag( reasons = decide_reasons.copy() if decide_reasons else [] user_id = user_context.user_id + global_holdout_result: DecisionResult | None = None + # Check global holdouts (flag level — before any rules are evaluated) global_holdouts = project_config.get_global_holdouts() for holdout in global_holdouts: @@ -758,7 +762,6 @@ def get_decision_for_flag( reasons.extend(holdout_decision['reasons']) decision = holdout_decision['decision'] - # Check if user was bucketed into holdout (has a variation) if decision.variation is None: continue @@ -768,18 +771,32 @@ def get_decision_for_flag( ) self.logger.info(message) reasons.append(message) - return { - 'decision': holdout_decision['decision'], - 'error': False, - 'reasons': reasons - } - # If no global holdout decision, check experiments then rollouts + if not holdout.exclude_targeted_deliveries: + return { + 'decision': holdout_decision['decision'], + 'error': False, + 'reasons': reasons + } + + message = ( + f"Holdout '{holdout.key}' excludes targeted deliveries. " + f"Targeted delivery rules will be evaluated." + ) + self.logger.info(message) + reasons.append(message) + global_holdout_result = holdout_decision + break + + # Check experiments then rollouts if feature_flag.experimentIds: for experiment_id in feature_flag.experimentIds: experiment = project_config.get_experiment_from_id(experiment_id) if experiment: + if global_holdout_result is not None and experiment.type != enums.ExperimentTypes.td: + continue + # Check for forced decision optimizely_decision_context = OptimizelyUserContext.OptimizelyDecisionContext( feature_flag.key, experiment.key) @@ -796,9 +813,11 @@ def get_decision_for_flag( 'reasons': reasons } - # Check local holdouts targeting this specific experiment rule (FSSDK-12369) local_holdouts = project_config.get_holdouts_for_rule(experiment.id) for holdout in local_holdouts: + if holdout.exclude_targeted_deliveries and experiment.type == enums.ExperimentTypes.td: + continue + local_holdout_decision = self.get_variation_for_holdout( holdout, user_context, project_config ) @@ -863,6 +882,13 @@ def get_decision_for_flag( else: self.logger.debug(f'User "{user_id}" not bucketed into any rollout for feature "{feature_flag.key}".') + if global_holdout_result is not None and not has_variation: + return { + 'decision': global_holdout_result['decision'], + 'error': False, + 'reasons': reasons + } + return { 'decision': rollout_decision, 'error': False, diff --git a/optimizely/entities.py b/optimizely/entities.py index 7b0b09e5..39caafe6 100644 --- a/optimizely/entities.py +++ b/optimizely/entities.py @@ -224,6 +224,7 @@ def __init__( audienceIds: list[str], audienceConditions: Optional[Sequence[str | list[str]]] = None, includedRules: Optional[list[str]] = None, + excludeTargetedDeliveries: bool = False, **kwargs: Any ): self.id = id @@ -236,6 +237,7 @@ def __init__( # Per-rule targeting for local holdouts. Scope comes from the datafile # section, not this field; ProjectConfig strips it on 'holdouts' entries. self.included_rules: Optional[list[str]] = includedRules + self.exclude_targeted_deliveries: bool = excludeTargetedDeliveries def get_audience_conditions_or_ids(self) -> Sequence[str | list[str]]: """Returns audienceConditions if present, otherwise audienceIds. diff --git a/tests/test_decision_service_holdout.py b/tests/test_decision_service_holdout.py index 21a269b2..21ed0536 100644 --- a/tests/test_decision_service_holdout.py +++ b/tests/test_decision_service_holdout.py @@ -1708,3 +1708,285 @@ def test_no_holdouts_at_all_falls_through_to_experiment(self): self.assertIsNotNone(result) decision = result['decision'] self.assertNotEqual(decision.source, enums.DecisionSources.HOLDOUT) + + +# --------------------------------------------------------------------------- +# Exclude Targeted Deliveries Tests +# --------------------------------------------------------------------------- + +def _holdout_with_etd(holdout_id, key, exclude_targeted_deliveries=False, + included_rules=None, traffic=None, status='Running'): + """Build a holdout dict with excludeTargetedDeliveries field.""" + h = { + 'id': holdout_id, + 'key': key, + 'status': status, + 'audienceIds': [], + 'variations': _HOLDOUT_VARIATION, + 'trafficAllocation': traffic or _FULL_TRAFFIC, + 'excludeTargetedDeliveries': exclude_targeted_deliveries, + } + if included_rules is not None: + h['includedRules'] = included_rules + return h + + +class ExcludeTargetedDeliveriesTest(base.BaseTest): + """Tests for exclude_targeted_deliveries behavior on holdouts. + + When a holdout has exclude_targeted_deliveries=True: + - TD (targeted delivery) experiments should bypass the holdout + - A/B and other experiment types should still be blocked by the holdout + - Delivery (rollout) rules should bypass local holdouts with this flag + """ + + def setUp(self): + base.BaseTest.setUp(self) + self.error_handler = error_handler.NoOpErrorHandler() + self.spy_logger = mock.MagicMock(spec=logger.SimpleLogger) + self.spy_logger.logger = self.spy_logger + self.spy_user_profile_service = mock.MagicMock() + self.spy_cmab_service = mock.MagicMock() + + def tearDown(self): + if hasattr(self, 'opt_obj'): + self.opt_obj.close() + + def _make_opt_with_td(self, holdouts, local_holdouts=None, experiment_type=None): + """Build Optimizely instance, optionally setting type on experiment '111127'.""" + cfg = self.config_dict_with_features.copy() + + if experiment_type is not None: + experiments = [] + for exp in cfg['experiments']: + exp_copy = exp.copy() + if exp_copy['id'] == '111127': + exp_copy['type'] = experiment_type + experiments.append(exp_copy) + cfg['experiments'] = experiments + + if local_holdouts is None: + globals_list: list = [] + locals_list: list = [] + for h in holdouts: + if h.get('includedRules') is not None: + locals_list.append(h) + else: + globals_list.append(h) + cfg['holdouts'] = globals_list + if locals_list: + cfg['localHoldouts'] = locals_list + else: + cfg['holdouts'] = holdouts + cfg['localHoldouts'] = local_holdouts + + self.opt_obj = optimizely_module.Optimizely(json.dumps(cfg)) + return self.opt_obj + + def _decision_svc(self): + return decision_service.DecisionService( + self.spy_logger, + self.spy_user_profile_service, + self.spy_cmab_service, + ) + + # ------------------------------------------------------------------ + # Test 1: Global holdout with exclude_targeted_deliveries=False (default) + # ------------------------------------------------------------------ + + def test_global_holdout_exclude_td_false_blocks_all_rules(self): + """Global holdout with exclude_targeted_deliveries=False blocks all rules as before.""" + opt = self._make_opt_with_td( + [_holdout_with_etd('gh1', 'global_no_exclude', exclude_targeted_deliveries=False)], + experiment_type='td', + ) + config = opt.config_manager.get_config() + feature_flag = config.get_feature_from_key('test_feature_in_experiment') + + ds = self._decision_svc() + + with mock.patch.object(ds, 'get_variation', wraps=ds.get_variation) as mock_get_var: + user_ctx = opt.create_user_context('user_blocked', {}) + result = ds.get_decision_for_flag(feature_flag, user_ctx, config) + + decision = result['decision'] + self.assertEqual(decision.source, enums.DecisionSources.HOLDOUT) + mock_get_var.assert_not_called() + + # ------------------------------------------------------------------ + # Test 2: Global holdout with exclude_targeted_deliveries=True, TD experiment + # ------------------------------------------------------------------ + + def test_global_holdout_exclude_td_true_allows_td_experiment(self): + """Global holdout with exclude_targeted_deliveries=True allows TD rules through.""" + opt = self._make_opt_with_td( + [_holdout_with_etd('gh1', 'global_exclude_td', exclude_targeted_deliveries=True)], + experiment_type='td', + ) + config = opt.config_manager.get_config() + feature_flag = config.get_feature_from_key('test_feature_in_experiment') + + ds = self._decision_svc() + user_ctx = opt.create_user_context('testUserId', {}) + result = ds.get_decision_for_flag(feature_flag, user_ctx, config) + + decision = result['decision'] + # TD experiment should be evaluated, not blocked by holdout + self.assertNotEqual(decision.source, enums.DecisionSources.HOLDOUT) + + # ------------------------------------------------------------------ + # Test 3: Global holdout with exclude_targeted_deliveries=True, A/B experiment + # ------------------------------------------------------------------ + + def test_global_holdout_exclude_td_true_blocks_ab_experiment(self): + """Global holdout with exclude_targeted_deliveries=True still blocks A/B rules.""" + opt = self._make_opt_with_td( + [_holdout_with_etd('gh1', 'global_exclude_td', exclude_targeted_deliveries=True)], + experiment_type='ab', + ) + config = opt.config_manager.get_config() + feature_flag = config.get_feature_from_key('test_feature_in_experiment') + + ds = self._decision_svc() + + with mock.patch.object(ds, 'get_variation', wraps=ds.get_variation) as mock_get_var: + user_ctx = opt.create_user_context('user_blocked_ab', {}) + result = ds.get_decision_for_flag(feature_flag, user_ctx, config) + + decision = result['decision'] + # A/B experiment should be skipped; user stays in holdout + self.assertEqual(decision.source, enums.DecisionSources.HOLDOUT) + mock_get_var.assert_not_called() + + # ------------------------------------------------------------------ + # Test 4: Global holdout with exclude_targeted_deliveries=True, no TD matches + # ------------------------------------------------------------------ + + def test_global_holdout_exclude_td_true_no_td_falls_back_to_holdout(self): + """When exclude_targeted_deliveries=True but no TD experiments match, + falls back to holdout decision.""" + # No type set on experiment (defaults to None, not 'td') + opt = self._make_opt_with_td( + [_holdout_with_etd('gh1', 'global_exclude_td', exclude_targeted_deliveries=True)], + ) + config = opt.config_manager.get_config() + feature_flag = config.get_feature_from_key('test_feature_in_experiment') + + ds = self._decision_svc() + user_ctx = opt.create_user_context('user_no_td', {}) + result = ds.get_decision_for_flag(feature_flag, user_ctx, config) + + decision = result['decision'] + # No TD experiment available, so holdout decision should be returned + self.assertEqual(decision.source, enums.DecisionSources.HOLDOUT) + + # ------------------------------------------------------------------ + # Test 5: Local holdout on delivery rule with exclude_targeted_deliveries=True + # ------------------------------------------------------------------ + + def test_local_holdout_delivery_rule_exclude_td_true_skips_holdout(self): + """Local holdout on delivery rule with exclude_targeted_deliveries=True + skips the holdout and evaluates the delivery rule.""" + delivery_rule_id = '211147' + opt = self._make_opt_with_td( + [_holdout_with_etd( + 'lh1', 'local_delivery_exclude', + exclude_targeted_deliveries=True, + included_rules=[delivery_rule_id], + )], + ) + config = opt.config_manager.get_config() + feature_flag = config.get_feature_from_key('test_feature_in_rollout') + + ds = self._decision_svc() + user_ctx = opt.create_user_context('user_delivery_exclude', {}) + result = ds.get_decision_for_flag(feature_flag, user_ctx, config) + + decision = result['decision'] + # Delivery rules are targeted deliveries, so holdout should be skipped + self.assertNotEqual(decision.source, enums.DecisionSources.HOLDOUT) + + # ------------------------------------------------------------------ + # Test 6: Local holdout on experiment rule (TD type) with exclude_targeted_deliveries=True + # ------------------------------------------------------------------ + + def test_local_holdout_td_experiment_exclude_td_true_skips_holdout(self): + """Local holdout on TD experiment with exclude_targeted_deliveries=True + skips the holdout.""" + experiment_rule_id = '111127' + opt = self._make_opt_with_td( + [_holdout_with_etd( + 'lh1', 'local_td_exclude', + exclude_targeted_deliveries=True, + included_rules=[experiment_rule_id], + )], + experiment_type='td', + ) + config = opt.config_manager.get_config() + feature_flag = config.get_feature_from_key('test_feature_in_experiment') + + ds = self._decision_svc() + user_ctx = opt.create_user_context('testUserId', {}) + result = ds.get_decision_for_flag(feature_flag, user_ctx, config) + + decision = result['decision'] + # TD experiment should bypass the local holdout + self.assertNotEqual(decision.source, enums.DecisionSources.HOLDOUT) + + # ------------------------------------------------------------------ + # Test 7: Local holdout on experiment rule (A/B type) with exclude_targeted_deliveries=True + # ------------------------------------------------------------------ + + def test_local_holdout_ab_experiment_exclude_td_true_still_applies(self): + """Local holdout on A/B experiment with exclude_targeted_deliveries=True + still applies the holdout (A/B is not a targeted delivery).""" + experiment_rule_id = '111127' + opt = self._make_opt_with_td( + [_holdout_with_etd( + 'lh1', 'local_ab_exclude', + exclude_targeted_deliveries=True, + included_rules=[experiment_rule_id], + )], + experiment_type='ab', + ) + config = opt.config_manager.get_config() + feature_flag = config.get_feature_from_key('test_feature_in_experiment') + + ds = self._decision_svc() + + with mock.patch.object(ds, 'get_variation', wraps=ds.get_variation) as mock_get_var: + user_ctx = opt.create_user_context('user_ab_holdout', {}) + result = ds.get_decision_for_flag(feature_flag, user_ctx, config) + + decision = result['decision'] + # A/B experiment should still be blocked by local holdout + self.assertEqual(decision.source, enums.DecisionSources.HOLDOUT) + mock_get_var.assert_not_called() + + # ------------------------------------------------------------------ + # Test 8: Missing field (backward compatibility) + # ------------------------------------------------------------------ + + def test_missing_exclude_td_field_defaults_to_false(self): + """When excludeTargetedDeliveries is not in the datafile, defaults to False + and holdout works as before (blocks all rules).""" + # Use _holdout helper which does NOT set excludeTargetedDeliveries + opt = self._make_opt_with_td( + [_holdout('gh1', 'legacy_holdout', traffic=_FULL_TRAFFIC)], + experiment_type='td', + ) + config = opt.config_manager.get_config() + feature_flag = config.get_feature_from_key('test_feature_in_experiment') + + ds = self._decision_svc() + user_ctx = opt.create_user_context('user_legacy', {}) + result = ds.get_decision_for_flag(feature_flag, user_ctx, config) + + decision = result['decision'] + # Without the field, holdout should block all rules including TD + self.assertEqual(decision.source, enums.DecisionSources.HOLDOUT) + + # Verify entity defaults + holdout = config.holdouts[0] if config.holdouts else None + self.assertIsNotNone(holdout) + self.assertFalse(holdout.exclude_targeted_deliveries) diff --git a/tests/test_holdout_config.py b/tests/test_holdout_config.py index e9416e11..074f4cde 100644 --- a/tests/test_holdout_config.py +++ b/tests/test_holdout_config.py @@ -427,6 +427,37 @@ def test_is_global_entity_property_independent_of_section(self): self.assertFalse(h_empty.is_global) +class HoldoutEntityExcludeTargetedDeliveriesTest(unittest.TestCase): + """Tests for the Holdout.exclude_targeted_deliveries field storage.""" + + def test_exclude_targeted_deliveries_stored_true(self): + """exclude_targeted_deliveries=True should be stored correctly.""" + holdout = entities.Holdout( + id='h1', key='etd_holdout', status='Running', + variations=HOLDOUT_VARIATION, trafficAllocation=FULL_TRAFFIC, + audienceIds=[], excludeTargetedDeliveries=True, + ) + self.assertTrue(holdout.exclude_targeted_deliveries) + + def test_exclude_targeted_deliveries_stored_false(self): + """exclude_targeted_deliveries=False should be stored correctly.""" + holdout = entities.Holdout( + id='h1', key='etd_holdout', status='Running', + variations=HOLDOUT_VARIATION, trafficAllocation=FULL_TRAFFIC, + audienceIds=[], excludeTargetedDeliveries=False, + ) + self.assertFalse(holdout.exclude_targeted_deliveries) + + def test_exclude_targeted_deliveries_defaults_to_false(self): + """When not provided, exclude_targeted_deliveries defaults to False.""" + holdout = entities.Holdout( + id='h1', key='default_holdout', status='Running', + variations=HOLDOUT_VARIATION, trafficAllocation=FULL_TRAFFIC, + audienceIds=[], + ) + self.assertFalse(holdout.exclude_targeted_deliveries) + + class LocalHoldoutsSectionTest(unittest.TestCase): """Tests for the new top-level 'localHoldouts' datafile section (FSSDK-12760). From c0c1a6c5abdd588a14064ef39766779374249366 Mon Sep 17 00:00:00 2001 From: esrakartalOpt Date: Wed, 22 Jul 2026 14:33:14 -0500 Subject: [PATCH 2/3] [AI-FSSDK] [FSSDK-12735] Add holdout event dispatch, local holdout flag isolation, and TD null passthrough --- optimizely/decision_service.py | 40 +++++------ optimizely/optimizely.py | 27 ++++++- tests/test_decision_service_holdout.py | 98 +++++++++++++++++++++----- 3 files changed, 123 insertions(+), 42 deletions(-) diff --git a/optimizely/decision_service.py b/optimizely/decision_service.py index 0dcd2671..6d98cbd7 100644 --- a/optimizely/decision_service.py +++ b/optimizely/decision_service.py @@ -64,15 +64,11 @@ class VariationResult(TypedDict): variation: Optional[Union[entities.Variation, VariationDict]] -class DecisionResult(TypedDict): - """ - A TypedDict representing the result of a decision process. +class _DecisionResultOptional(TypedDict, total=False): + holdout_decision: Decision - Attributes: - decision (Decision): The decision object containing the outcome of the evaluation. - error (bool): Indicates whether an error occurred during the decision process. - reasons (List[str]): A list of reasons explaining the decision or any errors encountered. - """ + +class DecisionResult(_DecisionResultOptional): decision: Decision error: bool reasons: List[str] @@ -612,9 +608,6 @@ def get_variation_for_rollout( local_holdouts = project_config.get_holdouts_for_rule(rule.id) for holdout in local_holdouts: - if holdout.exclude_targeted_deliveries: - continue - local_holdout_decision = self.get_variation_for_holdout( holdout, user_context, project_config ) @@ -807,17 +800,17 @@ def get_decision_for_flag( if forced_decision_variation: decision = Decision(experiment, forced_decision_variation, enums.DecisionSources.FEATURE_TEST, None) - return { + result: DecisionResult = { 'decision': decision, 'error': False, 'reasons': reasons } + if global_holdout_result is not None: + result['holdout_decision'] = global_holdout_result['decision'] + return result local_holdouts = project_config.get_holdouts_for_rule(experiment.id) for holdout in local_holdouts: - if holdout.exclude_targeted_deliveries and experiment.type == enums.ExperimentTypes.td: - continue - local_holdout_decision = self.get_variation_for_holdout( holdout, user_context, project_config ) @@ -856,11 +849,14 @@ def get_decision_for_flag( decision = Decision(experiment, variation_result['variation'], enums.DecisionSources.FEATURE_TEST, variation_result['cmab_uuid']) - return { + result: DecisionResult = { 'decision': decision, 'error': False, 'reasons': reasons } + if global_holdout_result is not None: + result['holdout_decision'] = global_holdout_result['decision'] + return result # If no experiment decision, check rollouts rollout_decision, rollout_reasons = self.get_variation_for_rollout( @@ -882,18 +878,14 @@ def get_decision_for_flag( else: self.logger.debug(f'User "{user_id}" not bucketed into any rollout for feature "{feature_flag.key}".') - if global_holdout_result is not None and not has_variation: - return { - 'decision': global_holdout_result['decision'], - 'error': False, - 'reasons': reasons - } - - return { + final_result: DecisionResult = { 'decision': rollout_decision, 'error': False, 'reasons': reasons } + if global_holdout_result is not None: + final_result['holdout_decision'] = global_holdout_result['decision'] + return final_result def get_variation_for_holdout( self, diff --git a/optimizely/optimizely.py b/optimizely/optimizely.py index 04162823..94f43c2f 100644 --- a/optimizely/optimizely.py +++ b/optimizely/optimizely.py @@ -1245,7 +1245,8 @@ def _create_optimizely_decision( flag_decision: Decision, decision_reasons: Optional[list[str]], decide_options: list[str], - project_config: ProjectConfig + project_config: ProjectConfig, + holdout_decision: Optional[Decision] = None ) -> OptimizelyDecision: user_id = user_context.user_id feature_enabled = False @@ -1264,6 +1265,25 @@ def _create_optimizely_decision( feature_flag = project_config.feature_key_map.get(flag_key) + # Send holdout impression when user was bucketed into a holdout bypassed due to exclude_targeted_deliveries + if (holdout_decision is not None + and decision_source != DecisionSources.HOLDOUT + and OptimizelyDecideOption.DISABLE_DECISION_EVENT not in decide_options): + holdout_enabled = self._get_feature_enabled(holdout_decision.variation) + holdout_rule_key = holdout_decision.experiment.key if holdout_decision.experiment else '' + self._send_impression_event( + project_config, + holdout_decision.experiment, + holdout_decision.variation, + flag_key, + holdout_rule_key, + str(DecisionSources.HOLDOUT), + holdout_enabled, + user_id, + attributes, + holdout_decision.cmab_uuid + ) + # Send impression event if Decision came from a feature if OptimizelyDecideOption.DISABLE_DECISION_EVENT not in decide_options: if (decision_source == DecisionSources.FEATURE_TEST or @@ -1448,11 +1468,13 @@ def _decide_for_keys( user_context, merged_decide_options ) + holdout_decisions: dict[str, Optional[Decision]] = {} for i in range(0, len(flags_without_forced_decision)): decision = decision_list[i]['decision'] reasons = decision_list[i]['reasons'] error = decision_list[i]['error'] flag_key = flags_without_forced_decision[i].key + holdout_decisions[flag_key] = decision_list[i].get('holdout_decision') # store error decision against key and remove key from valid keys if error: optimizely_decision = OptimizelyDecision.new_error_decision(flags_without_forced_decision[i].key, @@ -1472,7 +1494,8 @@ def _decide_for_keys( flag_decision, decision_reasons, merged_decide_options, - project_config + project_config, + holdout_decision=holdout_decisions.get(key) ) enabled_flags_only_missing = OptimizelyDecideOption.ENABLED_FLAGS_ONLY not in merged_decide_options is_enabled = optimizely_decision.enabled diff --git a/tests/test_decision_service_holdout.py b/tests/test_decision_service_holdout.py index 21ed0536..c0448ded 100644 --- a/tests/test_decision_service_holdout.py +++ b/tests/test_decision_service_holdout.py @@ -1839,7 +1839,9 @@ def test_global_holdout_exclude_td_true_allows_td_experiment(self): # ------------------------------------------------------------------ def test_global_holdout_exclude_td_true_blocks_ab_experiment(self): - """Global holdout with exclude_targeted_deliveries=True still blocks A/B rules.""" + """Global holdout with exclude_targeted_deliveries=True still blocks A/B rules. + The A/B experiment is skipped and a non-holdout decision is returned, + with the holdout decision attached separately.""" opt = self._make_opt_with_td( [_holdout_with_etd('gh1', 'global_exclude_td', exclude_targeted_deliveries=True)], experiment_type='ab', @@ -1854,18 +1856,17 @@ def test_global_holdout_exclude_td_true_blocks_ab_experiment(self): result = ds.get_decision_for_flag(feature_flag, user_ctx, config) decision = result['decision'] - # A/B experiment should be skipped; user stays in holdout - self.assertEqual(decision.source, enums.DecisionSources.HOLDOUT) + self.assertNotEqual(decision.source, enums.DecisionSources.HOLDOUT) mock_get_var.assert_not_called() + self.assertIsNotNone(result.get('holdout_decision')) # ------------------------------------------------------------------ # Test 4: Global holdout with exclude_targeted_deliveries=True, no TD matches # ------------------------------------------------------------------ - def test_global_holdout_exclude_td_true_no_td_falls_back_to_holdout(self): + def test_global_holdout_exclude_td_true_no_td_returns_non_holdout_decision(self): """When exclude_targeted_deliveries=True but no TD experiments match, - falls back to holdout decision.""" - # No type set on experiment (defaults to None, not 'td') + returns a non-holdout decision with the bypassed holdout attached.""" opt = self._make_opt_with_td( [_holdout_with_etd('gh1', 'global_exclude_td', exclude_targeted_deliveries=True)], ) @@ -1877,16 +1878,16 @@ def test_global_holdout_exclude_td_true_no_td_falls_back_to_holdout(self): result = ds.get_decision_for_flag(feature_flag, user_ctx, config) decision = result['decision'] - # No TD experiment available, so holdout decision should be returned - self.assertEqual(decision.source, enums.DecisionSources.HOLDOUT) + self.assertNotEqual(decision.source, enums.DecisionSources.HOLDOUT) + self.assertIsNotNone(result.get('holdout_decision')) # ------------------------------------------------------------------ # Test 5: Local holdout on delivery rule with exclude_targeted_deliveries=True # ------------------------------------------------------------------ - def test_local_holdout_delivery_rule_exclude_td_true_skips_holdout(self): + def test_local_holdout_delivery_rule_exclude_td_true_still_applies(self): """Local holdout on delivery rule with exclude_targeted_deliveries=True - skips the holdout and evaluates the delivery rule.""" + still applies because local holdouts ignore that flag.""" delivery_rule_id = '211147' opt = self._make_opt_with_td( [_holdout_with_etd( @@ -1903,16 +1904,15 @@ def test_local_holdout_delivery_rule_exclude_td_true_skips_holdout(self): result = ds.get_decision_for_flag(feature_flag, user_ctx, config) decision = result['decision'] - # Delivery rules are targeted deliveries, so holdout should be skipped - self.assertNotEqual(decision.source, enums.DecisionSources.HOLDOUT) + self.assertEqual(decision.source, enums.DecisionSources.HOLDOUT) # ------------------------------------------------------------------ # Test 6: Local holdout on experiment rule (TD type) with exclude_targeted_deliveries=True # ------------------------------------------------------------------ - def test_local_holdout_td_experiment_exclude_td_true_skips_holdout(self): + def test_local_holdout_td_experiment_exclude_td_true_still_applies(self): """Local holdout on TD experiment with exclude_targeted_deliveries=True - skips the holdout.""" + still applies because local holdouts ignore that flag.""" experiment_rule_id = '111127' opt = self._make_opt_with_td( [_holdout_with_etd( @@ -1930,8 +1930,7 @@ def test_local_holdout_td_experiment_exclude_td_true_skips_holdout(self): result = ds.get_decision_for_flag(feature_flag, user_ctx, config) decision = result['decision'] - # TD experiment should bypass the local holdout - self.assertNotEqual(decision.source, enums.DecisionSources.HOLDOUT) + self.assertEqual(decision.source, enums.DecisionSources.HOLDOUT) # ------------------------------------------------------------------ # Test 7: Local holdout on experiment rule (A/B type) with exclude_targeted_deliveries=True @@ -1990,3 +1989,70 @@ def test_missing_exclude_td_field_defaults_to_false(self): holdout = config.holdouts[0] if config.holdouts else None self.assertIsNotNone(holdout) self.assertFalse(holdout.exclude_targeted_deliveries) + + # ------------------------------------------------------------------ + # Test 9: TD match returns holdout_decision in result + # ------------------------------------------------------------------ + + def test_global_holdout_exclude_td_true_td_match_has_holdout_decision(self): + """When exclude_targeted_deliveries=True and a TD experiment matches, + the result contains holdout_decision with the bypassed holdout.""" + opt = self._make_opt_with_td( + [_holdout_with_etd('gh1', 'global_exclude_td', exclude_targeted_deliveries=True)], + experiment_type='td', + ) + config = opt.config_manager.get_config() + feature_flag = config.get_feature_from_key('test_feature_in_experiment') + + ds = self._decision_svc() + user_ctx = opt.create_user_context('testUserId', {}) + result = ds.get_decision_for_flag(feature_flag, user_ctx, config) + + decision = result['decision'] + self.assertNotEqual(decision.source, enums.DecisionSources.HOLDOUT) + self.assertIsNotNone(result.get('holdout_decision')) + self.assertEqual(result['holdout_decision'].source, enums.DecisionSources.HOLDOUT) + + # ------------------------------------------------------------------ + # Test 10: No TD match returns non-holdout with holdout_decision attached + # ------------------------------------------------------------------ + + def test_global_holdout_exclude_td_true_no_td_has_holdout_decision(self): + """When exclude_targeted_deliveries=True and no TD matches, + result is non-holdout but holdout_decision key is present.""" + opt = self._make_opt_with_td( + [_holdout_with_etd('gh1', 'global_exclude_td', exclude_targeted_deliveries=True)], + ) + config = opt.config_manager.get_config() + feature_flag = config.get_feature_from_key('test_feature_in_experiment') + + ds = self._decision_svc() + user_ctx = opt.create_user_context('user_no_td', {}) + result = ds.get_decision_for_flag(feature_flag, user_ctx, config) + + decision = result['decision'] + self.assertNotEqual(decision.source, enums.DecisionSources.HOLDOUT) + holdout_dec = result.get('holdout_decision') + self.assertIsNotNone(holdout_dec) + self.assertEqual(holdout_dec.source, enums.DecisionSources.HOLDOUT) + + # ------------------------------------------------------------------ + # Test 11: Holdout impression event dispatched for bypassed holdout + # ------------------------------------------------------------------ + + def test_holdout_impression_sent_when_td_evaluated(self): + """When exclude_targeted_deliveries=True and TD matches, + two impression events are sent: one for holdout, one for TD.""" + opt = self._make_opt_with_td( + [_holdout_with_etd('gh1', 'global_exclude_td', exclude_targeted_deliveries=True)], + experiment_type='td', + ) + with mock.patch.object(opt, '_send_impression_event') as mock_send: + user_ctx = opt.create_user_context('testUserId', {}) + user_ctx.decide('test_feature_in_experiment') + + self.assertEqual(mock_send.call_count, 2) + call_rule_types = [call.args[5] if len(call.args) > 5 else call.kwargs.get('rule_type') + for call in mock_send.call_args_list] + self.assertIn(str(enums.DecisionSources.HOLDOUT), call_rule_types) + self.assertIn(str(enums.DecisionSources.FEATURE_TEST), call_rule_types) From c56a99a39de3ba1d2b5ce5a9f7b5e48e8bf5dd1c Mon Sep 17 00:00:00 2001 From: esrakartalOpt Date: Wed, 22 Jul 2026 16:42:28 -0500 Subject: [PATCH 3/3] [AI-FSSDK] [FSSDK-12735] Fix holdout impression event tracking and add bypass decision reason --- optimizely/decision_service.py | 10 ++++++ optimizely/optimizely.py | 1 + tests/test_decision_service_holdout.py | 42 +++++++++++++++++++++++--- 3 files changed, 49 insertions(+), 4 deletions(-) diff --git a/optimizely/decision_service.py b/optimizely/decision_service.py index 6d98cbd7..2d34060a 100644 --- a/optimizely/decision_service.py +++ b/optimizely/decision_service.py @@ -747,6 +747,7 @@ def get_decision_for_flag( user_id = user_context.user_id global_holdout_result: DecisionResult | None = None + global_holdout_key: str | None = None # Check global holdouts (flag level — before any rules are evaluated) global_holdouts = project_config.get_global_holdouts() @@ -779,6 +780,7 @@ def get_decision_for_flag( self.logger.info(message) reasons.append(message) global_holdout_result = holdout_decision + global_holdout_key = holdout.key break # Check experiments then rollouts @@ -858,6 +860,14 @@ def get_decision_for_flag( result['holdout_decision'] = global_holdout_result['decision'] return result + if global_holdout_result is not None: + message = ( + f"Holdout '{global_holdout_key}' has excludeTargetedDeliveries enabled, " + f"continuing to rollout evaluation." + ) + self.logger.info(message) + reasons.append(message) + # If no experiment decision, check rollouts rollout_decision, rollout_reasons = self.get_variation_for_rollout( project_config, feature_flag, user_context diff --git a/optimizely/optimizely.py b/optimizely/optimizely.py index 94f43c2f..0f7c1da5 100644 --- a/optimizely/optimizely.py +++ b/optimizely/optimizely.py @@ -1283,6 +1283,7 @@ def _create_optimizely_decision( attributes, holdout_decision.cmab_uuid ) + decision_event_dispatched = True # Send impression event if Decision came from a feature if OptimizelyDecideOption.DISABLE_DECISION_EVENT not in decide_options: diff --git a/tests/test_decision_service_holdout.py b/tests/test_decision_service_holdout.py index c0448ded..ab282097 100644 --- a/tests/test_decision_service_holdout.py +++ b/tests/test_decision_service_holdout.py @@ -1866,7 +1866,8 @@ def test_global_holdout_exclude_td_true_blocks_ab_experiment(self): def test_global_holdout_exclude_td_true_no_td_returns_non_holdout_decision(self): """When exclude_targeted_deliveries=True but no TD experiments match, - returns a non-holdout decision with the bypassed holdout attached.""" + returns a non-holdout decision with the bypassed holdout attached + and includes the rollout evaluation reason.""" opt = self._make_opt_with_td( [_holdout_with_etd('gh1', 'global_exclude_td', exclude_targeted_deliveries=True)], ) @@ -1881,6 +1882,12 @@ def test_global_holdout_exclude_td_true_no_td_returns_non_holdout_decision(self) self.assertNotEqual(decision.source, enums.DecisionSources.HOLDOUT) self.assertIsNotNone(result.get('holdout_decision')) + expected_reason = ( + "Holdout 'global_exclude_td' has excludeTargetedDeliveries enabled, " + "continuing to rollout evaluation." + ) + self.assertIn(expected_reason, result['reasons']) + # ------------------------------------------------------------------ # Test 5: Local holdout on delivery rule with exclude_targeted_deliveries=True # ------------------------------------------------------------------ @@ -2019,7 +2026,8 @@ def test_global_holdout_exclude_td_true_td_match_has_holdout_decision(self): def test_global_holdout_exclude_td_true_no_td_has_holdout_decision(self): """When exclude_targeted_deliveries=True and no TD matches, - result is non-holdout but holdout_decision key is present.""" + result is non-holdout but holdout_decision key is present, + and reasons include the rollout evaluation continuation message.""" opt = self._make_opt_with_td( [_holdout_with_etd('gh1', 'global_exclude_td', exclude_targeted_deliveries=True)], ) @@ -2036,18 +2044,38 @@ def test_global_holdout_exclude_td_true_no_td_has_holdout_decision(self): self.assertIsNotNone(holdout_dec) self.assertEqual(holdout_dec.source, enums.DecisionSources.HOLDOUT) + expected_reason = ( + "Holdout 'global_exclude_td' has excludeTargetedDeliveries enabled, " + "continuing to rollout evaluation." + ) + self.assertIn(expected_reason, result['reasons']) + # ------------------------------------------------------------------ # Test 11: Holdout impression event dispatched for bypassed holdout # ------------------------------------------------------------------ def test_holdout_impression_sent_when_td_evaluated(self): """When exclude_targeted_deliveries=True and TD matches, - two impression events are sent: one for holdout, one for TD.""" + two impression events are sent: one for holdout, one for TD, + and decision_event_dispatched is True in notification.""" opt = self._make_opt_with_td( [_holdout_with_etd('gh1', 'global_exclude_td', exclude_targeted_deliveries=True)], experiment_type='td', ) - with mock.patch.object(opt, '_send_impression_event') as mock_send: + + captured_notifications: list[dict[str, object]] = [] + + def capture_notification(notification_type: str, user_id: str, + user_attributes: dict[str, object], + decision_info: dict[str, object]) -> None: + captured_notifications.append(decision_info.copy()) + + opt.notification_center.add_notification_listener( + enums.NotificationTypes.DECISION, + capture_notification + ) + + with mock.patch.object(opt, '_send_impression_event', wraps=opt._send_impression_event) as mock_send: user_ctx = opt.create_user_context('testUserId', {}) user_ctx.decide('test_feature_in_experiment') @@ -2056,3 +2084,9 @@ def test_holdout_impression_sent_when_td_evaluated(self): for call in mock_send.call_args_list] self.assertIn(str(enums.DecisionSources.HOLDOUT), call_rule_types) self.assertIn(str(enums.DecisionSources.FEATURE_TEST), call_rule_types) + + self.assertEqual(len(captured_notifications), 1) + self.assertTrue( + captured_notifications[0].get('decision_event_dispatched'), + 'decision_event_dispatched should be True when holdout impression is sent' + )