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).