Skip to content

Commit b91dc09

Browse files
esrakartalOptesrakartalOpt
committed
[AI-FSSDK] [FSSDK-12735] Add holdout exclusion logic for Targeted Delivery rules
1 parent 3eaa6cb commit b91dc09

4 files changed

Lines changed: 350 additions & 9 deletions

File tree

‎optimizely/decision_service.py‎

Lines changed: 35 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -610,9 +610,11 @@ def get_variation_for_rollout(
610610
return Decision(experiment=rule, variation=forced_decision_variation,
611611
source=enums.DecisionSources.ROLLOUT, cmab_uuid=None), decide_reasons
612612

613-
# Check local holdouts targeting this specific delivery rule (FSSDK-12369)
614613
local_holdouts = project_config.get_holdouts_for_rule(rule.id)
615614
for holdout in local_holdouts:
615+
if holdout.exclude_targeted_deliveries:
616+
continue
617+
616618
local_holdout_decision = self.get_variation_for_holdout(
617619
holdout, user_context, project_config
618620
)
@@ -751,14 +753,15 @@ def get_decision_for_flag(
751753
reasons = decide_reasons.copy() if decide_reasons else []
752754
user_id = user_context.user_id
753755

756+
global_holdout_result: DecisionResult | None = None
757+
754758
# Check global holdouts (flag level — before any rules are evaluated)
755759
global_holdouts = project_config.get_global_holdouts()
756760
for holdout in global_holdouts:
757761
holdout_decision = self.get_variation_for_holdout(holdout, user_context, project_config)
758762
reasons.extend(holdout_decision['reasons'])
759763

760764
decision = holdout_decision['decision']
761-
# Check if user was bucketed into holdout (has a variation)
762765
if decision.variation is None:
763766
continue
764767

@@ -768,18 +771,32 @@ def get_decision_for_flag(
768771
)
769772
self.logger.info(message)
770773
reasons.append(message)
771-
return {
772-
'decision': holdout_decision['decision'],
773-
'error': False,
774-
'reasons': reasons
775-
}
776774

777-
# If no global holdout decision, check experiments then rollouts
775+
if not holdout.exclude_targeted_deliveries:
776+
return {
777+
'decision': holdout_decision['decision'],
778+
'error': False,
779+
'reasons': reasons
780+
}
781+
782+
message = (
783+
f"Holdout '{holdout.key}' excludes targeted deliveries. "
784+
f"Targeted delivery rules will be evaluated."
785+
)
786+
self.logger.info(message)
787+
reasons.append(message)
788+
global_holdout_result = holdout_decision
789+
break
790+
791+
# Check experiments then rollouts
778792
if feature_flag.experimentIds:
779793
for experiment_id in feature_flag.experimentIds:
780794
experiment = project_config.get_experiment_from_id(experiment_id)
781795

782796
if experiment:
797+
if global_holdout_result is not None and experiment.type != enums.ExperimentTypes.td:
798+
continue
799+
783800
# Check for forced decision
784801
optimizely_decision_context = OptimizelyUserContext.OptimizelyDecisionContext(
785802
feature_flag.key, experiment.key)
@@ -796,9 +813,11 @@ def get_decision_for_flag(
796813
'reasons': reasons
797814
}
798815

799-
# Check local holdouts targeting this specific experiment rule (FSSDK-12369)
800816
local_holdouts = project_config.get_holdouts_for_rule(experiment.id)
801817
for holdout in local_holdouts:
818+
if holdout.exclude_targeted_deliveries and experiment.type == enums.ExperimentTypes.td:
819+
continue
820+
802821
local_holdout_decision = self.get_variation_for_holdout(
803822
holdout, user_context, project_config
804823
)
@@ -863,6 +882,13 @@ def get_decision_for_flag(
863882
else:
864883
self.logger.debug(f'User "{user_id}" not bucketed into any rollout for feature "{feature_flag.key}".')
865884

885+
if global_holdout_result is not None and not has_variation:
886+
return {
887+
'decision': global_holdout_result['decision'],
888+
'error': False,
889+
'reasons': reasons
890+
}
891+
866892
return {
867893
'decision': rollout_decision,
868894
'error': False,

‎optimizely/entities.py‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -224,6 +224,7 @@ def __init__(
224224
audienceIds: list[str],
225225
audienceConditions: Optional[Sequence[str | list[str]]] = None,
226226
includedRules: Optional[list[str]] = None,
227+
excludeTargetedDeliveries: bool = False,
227228
**kwargs: Any
228229
):
229230
self.id = id
@@ -236,6 +237,7 @@ def __init__(
236237
# Per-rule targeting for local holdouts. Scope comes from the datafile
237238
# section, not this field; ProjectConfig strips it on 'holdouts' entries.
238239
self.included_rules: Optional[list[str]] = includedRules
240+
self.exclude_targeted_deliveries: bool = excludeTargetedDeliveries
239241

240242
def get_audience_conditions_or_ids(self) -> Sequence[str | list[str]]:
241243
"""Returns audienceConditions if present, otherwise audienceIds.

‎tests/test_decision_service_holdout.py‎

Lines changed: 282 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1708,3 +1708,285 @@ def test_no_holdouts_at_all_falls_through_to_experiment(self):
17081708
self.assertIsNotNone(result)
17091709
decision = result['decision']
17101710
self.assertNotEqual(decision.source, enums.DecisionSources.HOLDOUT)
1711+
1712+
1713+
# ---------------------------------------------------------------------------
1714+
# Exclude Targeted Deliveries Tests
1715+
# ---------------------------------------------------------------------------
1716+
1717+
def _holdout_with_etd(holdout_id, key, exclude_targeted_deliveries=False,
1718+
included_rules=None, traffic=None, status='Running'):
1719+
"""Build a holdout dict with excludeTargetedDeliveries field."""
1720+
h = {
1721+
'id': holdout_id,
1722+
'key': key,
1723+
'status': status,
1724+
'audienceIds': [],
1725+
'variations': _HOLDOUT_VARIATION,
1726+
'trafficAllocation': traffic or _FULL_TRAFFIC,
1727+
'excludeTargetedDeliveries': exclude_targeted_deliveries,
1728+
}
1729+
if included_rules is not None:
1730+
h['includedRules'] = included_rules
1731+
return h
1732+
1733+
1734+
class ExcludeTargetedDeliveriesTest(base.BaseTest):
1735+
"""Tests for exclude_targeted_deliveries behavior on holdouts.
1736+
1737+
When a holdout has exclude_targeted_deliveries=True:
1738+
- TD (targeted delivery) experiments should bypass the holdout
1739+
- A/B and other experiment types should still be blocked by the holdout
1740+
- Delivery (rollout) rules should bypass local holdouts with this flag
1741+
"""
1742+
1743+
def setUp(self):
1744+
base.BaseTest.setUp(self)
1745+
self.error_handler = error_handler.NoOpErrorHandler()
1746+
self.spy_logger = mock.MagicMock(spec=logger.SimpleLogger)
1747+
self.spy_logger.logger = self.spy_logger
1748+
self.spy_user_profile_service = mock.MagicMock()
1749+
self.spy_cmab_service = mock.MagicMock()
1750+
1751+
def tearDown(self):
1752+
if hasattr(self, 'opt_obj'):
1753+
self.opt_obj.close()
1754+
1755+
def _make_opt_with_td(self, holdouts, local_holdouts=None, experiment_type=None):
1756+
"""Build Optimizely instance, optionally setting type on experiment '111127'."""
1757+
cfg = self.config_dict_with_features.copy()
1758+
1759+
if experiment_type is not None:
1760+
experiments = []
1761+
for exp in cfg['experiments']:
1762+
exp_copy = exp.copy()
1763+
if exp_copy['id'] == '111127':
1764+
exp_copy['type'] = experiment_type
1765+
experiments.append(exp_copy)
1766+
cfg['experiments'] = experiments
1767+
1768+
if local_holdouts is None:
1769+
globals_list: list = []
1770+
locals_list: list = []
1771+
for h in holdouts:
1772+
if h.get('includedRules') is not None:
1773+
locals_list.append(h)
1774+
else:
1775+
globals_list.append(h)
1776+
cfg['holdouts'] = globals_list
1777+
if locals_list:
1778+
cfg['localHoldouts'] = locals_list
1779+
else:
1780+
cfg['holdouts'] = holdouts
1781+
cfg['localHoldouts'] = local_holdouts
1782+
1783+
self.opt_obj = optimizely_module.Optimizely(json.dumps(cfg))
1784+
return self.opt_obj
1785+
1786+
def _decision_svc(self):
1787+
return decision_service.DecisionService(
1788+
self.spy_logger,
1789+
self.spy_user_profile_service,
1790+
self.spy_cmab_service,
1791+
)
1792+
1793+
# ------------------------------------------------------------------
1794+
# Test 1: Global holdout with exclude_targeted_deliveries=False (default)
1795+
# ------------------------------------------------------------------
1796+
1797+
def test_global_holdout_exclude_td_false_blocks_all_rules(self):
1798+
"""Global holdout with exclude_targeted_deliveries=False blocks all rules as before."""
1799+
opt = self._make_opt_with_td(
1800+
[_holdout_with_etd('gh1', 'global_no_exclude', exclude_targeted_deliveries=False)],
1801+
experiment_type='td',
1802+
)
1803+
config = opt.config_manager.get_config()
1804+
feature_flag = config.get_feature_from_key('test_feature_in_experiment')
1805+
1806+
ds = self._decision_svc()
1807+
1808+
with mock.patch.object(ds, 'get_variation', wraps=ds.get_variation) as mock_get_var:
1809+
user_ctx = opt.create_user_context('user_blocked', {})
1810+
result = ds.get_decision_for_flag(feature_flag, user_ctx, config)
1811+
1812+
decision = result['decision']
1813+
self.assertEqual(decision.source, enums.DecisionSources.HOLDOUT)
1814+
mock_get_var.assert_not_called()
1815+
1816+
# ------------------------------------------------------------------
1817+
# Test 2: Global holdout with exclude_targeted_deliveries=True, TD experiment
1818+
# ------------------------------------------------------------------
1819+
1820+
def test_global_holdout_exclude_td_true_allows_td_experiment(self):
1821+
"""Global holdout with exclude_targeted_deliveries=True allows TD rules through."""
1822+
opt = self._make_opt_with_td(
1823+
[_holdout_with_etd('gh1', 'global_exclude_td', exclude_targeted_deliveries=True)],
1824+
experiment_type='td',
1825+
)
1826+
config = opt.config_manager.get_config()
1827+
feature_flag = config.get_feature_from_key('test_feature_in_experiment')
1828+
1829+
ds = self._decision_svc()
1830+
user_ctx = opt.create_user_context('testUserId', {})
1831+
result = ds.get_decision_for_flag(feature_flag, user_ctx, config)
1832+
1833+
decision = result['decision']
1834+
# TD experiment should be evaluated, not blocked by holdout
1835+
self.assertNotEqual(decision.source, enums.DecisionSources.HOLDOUT)
1836+
1837+
# ------------------------------------------------------------------
1838+
# Test 3: Global holdout with exclude_targeted_deliveries=True, A/B experiment
1839+
# ------------------------------------------------------------------
1840+
1841+
def test_global_holdout_exclude_td_true_blocks_ab_experiment(self):
1842+
"""Global holdout with exclude_targeted_deliveries=True still blocks A/B rules."""
1843+
opt = self._make_opt_with_td(
1844+
[_holdout_with_etd('gh1', 'global_exclude_td', exclude_targeted_deliveries=True)],
1845+
experiment_type='ab',
1846+
)
1847+
config = opt.config_manager.get_config()
1848+
feature_flag = config.get_feature_from_key('test_feature_in_experiment')
1849+
1850+
ds = self._decision_svc()
1851+
1852+
with mock.patch.object(ds, 'get_variation', wraps=ds.get_variation) as mock_get_var:
1853+
user_ctx = opt.create_user_context('user_blocked_ab', {})
1854+
result = ds.get_decision_for_flag(feature_flag, user_ctx, config)
1855+
1856+
decision = result['decision']
1857+
# A/B experiment should be skipped; user stays in holdout
1858+
self.assertEqual(decision.source, enums.DecisionSources.HOLDOUT)
1859+
mock_get_var.assert_not_called()
1860+
1861+
# ------------------------------------------------------------------
1862+
# Test 4: Global holdout with exclude_targeted_deliveries=True, no TD matches
1863+
# ------------------------------------------------------------------
1864+
1865+
def test_global_holdout_exclude_td_true_no_td_falls_back_to_holdout(self):
1866+
"""When exclude_targeted_deliveries=True but no TD experiments match,
1867+
falls back to holdout decision."""
1868+
# No type set on experiment (defaults to None, not 'td')
1869+
opt = self._make_opt_with_td(
1870+
[_holdout_with_etd('gh1', 'global_exclude_td', exclude_targeted_deliveries=True)],
1871+
)
1872+
config = opt.config_manager.get_config()
1873+
feature_flag = config.get_feature_from_key('test_feature_in_experiment')
1874+
1875+
ds = self._decision_svc()
1876+
user_ctx = opt.create_user_context('user_no_td', {})
1877+
result = ds.get_decision_for_flag(feature_flag, user_ctx, config)
1878+
1879+
decision = result['decision']
1880+
# No TD experiment available, so holdout decision should be returned
1881+
self.assertEqual(decision.source, enums.DecisionSources.HOLDOUT)
1882+
1883+
# ------------------------------------------------------------------
1884+
# Test 5: Local holdout on delivery rule with exclude_targeted_deliveries=True
1885+
# ------------------------------------------------------------------
1886+
1887+
def test_local_holdout_delivery_rule_exclude_td_true_skips_holdout(self):
1888+
"""Local holdout on delivery rule with exclude_targeted_deliveries=True
1889+
skips the holdout and evaluates the delivery rule."""
1890+
delivery_rule_id = '211147'
1891+
opt = self._make_opt_with_td(
1892+
[_holdout_with_etd(
1893+
'lh1', 'local_delivery_exclude',
1894+
exclude_targeted_deliveries=True,
1895+
included_rules=[delivery_rule_id],
1896+
)],
1897+
)
1898+
config = opt.config_manager.get_config()
1899+
feature_flag = config.get_feature_from_key('test_feature_in_rollout')
1900+
1901+
ds = self._decision_svc()
1902+
user_ctx = opt.create_user_context('user_delivery_exclude', {})
1903+
result = ds.get_decision_for_flag(feature_flag, user_ctx, config)
1904+
1905+
decision = result['decision']
1906+
# Delivery rules are targeted deliveries, so holdout should be skipped
1907+
self.assertNotEqual(decision.source, enums.DecisionSources.HOLDOUT)
1908+
1909+
# ------------------------------------------------------------------
1910+
# Test 6: Local holdout on experiment rule (TD type) with exclude_targeted_deliveries=True
1911+
# ------------------------------------------------------------------
1912+
1913+
def test_local_holdout_td_experiment_exclude_td_true_skips_holdout(self):
1914+
"""Local holdout on TD experiment with exclude_targeted_deliveries=True
1915+
skips the holdout."""
1916+
experiment_rule_id = '111127'
1917+
opt = self._make_opt_with_td(
1918+
[_holdout_with_etd(
1919+
'lh1', 'local_td_exclude',
1920+
exclude_targeted_deliveries=True,
1921+
included_rules=[experiment_rule_id],
1922+
)],
1923+
experiment_type='td',
1924+
)
1925+
config = opt.config_manager.get_config()
1926+
feature_flag = config.get_feature_from_key('test_feature_in_experiment')
1927+
1928+
ds = self._decision_svc()
1929+
user_ctx = opt.create_user_context('testUserId', {})
1930+
result = ds.get_decision_for_flag(feature_flag, user_ctx, config)
1931+
1932+
decision = result['decision']
1933+
# TD experiment should bypass the local holdout
1934+
self.assertNotEqual(decision.source, enums.DecisionSources.HOLDOUT)
1935+
1936+
# ------------------------------------------------------------------
1937+
# Test 7: Local holdout on experiment rule (A/B type) with exclude_targeted_deliveries=True
1938+
# ------------------------------------------------------------------
1939+
1940+
def test_local_holdout_ab_experiment_exclude_td_true_still_applies(self):
1941+
"""Local holdout on A/B experiment with exclude_targeted_deliveries=True
1942+
still applies the holdout (A/B is not a targeted delivery)."""
1943+
experiment_rule_id = '111127'
1944+
opt = self._make_opt_with_td(
1945+
[_holdout_with_etd(
1946+
'lh1', 'local_ab_exclude',
1947+
exclude_targeted_deliveries=True,
1948+
included_rules=[experiment_rule_id],
1949+
)],
1950+
experiment_type='ab',
1951+
)
1952+
config = opt.config_manager.get_config()
1953+
feature_flag = config.get_feature_from_key('test_feature_in_experiment')
1954+
1955+
ds = self._decision_svc()
1956+
1957+
with mock.patch.object(ds, 'get_variation', wraps=ds.get_variation) as mock_get_var:
1958+
user_ctx = opt.create_user_context('user_ab_holdout', {})
1959+
result = ds.get_decision_for_flag(feature_flag, user_ctx, config)
1960+
1961+
decision = result['decision']
1962+
# A/B experiment should still be blocked by local holdout
1963+
self.assertEqual(decision.source, enums.DecisionSources.HOLDOUT)
1964+
mock_get_var.assert_not_called()
1965+
1966+
# ------------------------------------------------------------------
1967+
# Test 8: Missing field (backward compatibility)
1968+
# ------------------------------------------------------------------
1969+
1970+
def test_missing_exclude_td_field_defaults_to_false(self):
1971+
"""When excludeTargetedDeliveries is not in the datafile, defaults to False
1972+
and holdout works as before (blocks all rules)."""
1973+
# Use _holdout helper which does NOT set excludeTargetedDeliveries
1974+
opt = self._make_opt_with_td(
1975+
[_holdout('gh1', 'legacy_holdout', traffic=_FULL_TRAFFIC)],
1976+
experiment_type='td',
1977+
)
1978+
config = opt.config_manager.get_config()
1979+
feature_flag = config.get_feature_from_key('test_feature_in_experiment')
1980+
1981+
ds = self._decision_svc()
1982+
user_ctx = opt.create_user_context('user_legacy', {})
1983+
result = ds.get_decision_for_flag(feature_flag, user_ctx, config)
1984+
1985+
decision = result['decision']
1986+
# Without the field, holdout should block all rules including TD
1987+
self.assertEqual(decision.source, enums.DecisionSources.HOLDOUT)
1988+
1989+
# Verify entity defaults
1990+
holdout = config.holdouts[0] if config.holdouts else None
1991+
self.assertIsNotNone(holdout)
1992+
self.assertFalse(holdout.exclude_targeted_deliveries)

0 commit comments

Comments
 (0)