diff --git a/optimizely/decision_service.py b/optimizely/decision_service.py index b12352c2..c5044511 100644 --- a/optimizely/decision_service.py +++ b/optimizely/decision_service.py @@ -1,4 +1,4 @@ -# Copyright 2017-2022, Optimizely +# Copyright 2017-2022, 2026, Optimizely # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. # You may obtain a copy of the License at @@ -65,7 +65,8 @@ class VariationResult(TypedDict): class _DecisionResultOptional(TypedDict, total=False): - holdout_decision: Decision + holdout_decision: Decision # Deprecated: first entry of holdout_decisions. + holdout_decisions: List[Decision] class DecisionResult(_DecisionResultOptional): @@ -552,7 +553,11 @@ def get_variation( } def get_variation_for_rollout( - self, project_config: ProjectConfig, feature: entities.FeatureFlag, user_context: OptimizelyUserContext + self, + project_config: ProjectConfig, + feature: entities.FeatureFlag, + user_context: OptimizelyUserContext, + holdout_decisions: Optional[list[Decision]] = None ) -> tuple[Decision, list[str]]: """ Determine which experiment/variation the user is in for a given rollout. Returns the variation of the first experiment the user qualifies for. @@ -562,7 +567,7 @@ def get_variation_for_rollout( flagKey: Feature key. rollout: Rollout for which we are getting the variation. user: ID and attributes for user. - options: Decide options. + holdout_decisions: If provided, local holdout decisions for skipped rules are appended to it. Returns: Decision namedtuple consisting of experiment and variation for the user and @@ -606,22 +611,14 @@ def get_variation_for_rollout( return Decision(experiment=rule, variation=forced_decision_variation, source=enums.DecisionSources.ROLLOUT, cmab_uuid=None), decide_reasons - local_holdouts = project_config.get_holdouts_for_rule(rule.id) - for holdout in local_holdouts: - local_holdout_decision = self.get_variation_for_holdout( - holdout, user_context, project_config - ) - decide_reasons.extend(local_holdout_decision['reasons']) - - local_decision = local_holdout_decision['decision'] - if local_decision.variation is not None: - message = ( - f"The user '{user_id}' is bucketed into local holdout '{holdout.key}' " - f"for delivery rule '{rule.key}'." - ) - self.logger.info(message) - decide_reasons.append(message) - return local_decision, decide_reasons + local_holdout = self._get_local_holdout_decision( + project_config, rule, user_context, 'delivery', decide_reasons + ) + if local_holdout is not None: + if holdout_decisions is not None: + holdout_decisions.append(local_holdout) + index += 1 + continue bucketing_id, bucket_reasons = self._get_bucketing_id(user_id, attributes) decide_reasons += bucket_reasons @@ -748,6 +745,7 @@ def get_decision_for_flag( global_holdout_result: DecisionResult | None = None global_holdout_key: str | None = None + holdout_decisions: list[Decision] = [] # Check global holdouts (flag level — before any rules are evaluated) global_holdouts = project_config.get_global_holdouts() @@ -781,6 +779,7 @@ def get_decision_for_flag( reasons.append(message) global_holdout_result = holdout_decision global_holdout_key = holdout.key + holdout_decisions.append(holdout_decision['decision']) break # Check experiments then rollouts @@ -807,30 +806,17 @@ def get_decision_for_flag( 'error': False, 'reasons': reasons } - if global_holdout_result is not None: - result['holdout_decision'] = global_holdout_result['decision'] + if holdout_decisions: + result['holdout_decisions'] = holdout_decisions + result['holdout_decision'] = holdout_decisions[0] return result - local_holdouts = project_config.get_holdouts_for_rule(experiment.id) - for holdout in local_holdouts: - local_holdout_decision = self.get_variation_for_holdout( - holdout, user_context, project_config - ) - reasons.extend(local_holdout_decision['reasons']) - - local_decision = local_holdout_decision['decision'] - if local_decision.variation is not None: - message = ( - f"The user '{user_id}' is bucketed into local holdout '{holdout.key}' " - f"for experiment rule '{experiment.key}'." - ) - self.logger.info(message) - reasons.append(message) - return { - 'decision': local_holdout_decision['decision'], - 'error': False, - 'reasons': reasons - } + local_holdout = self._get_local_holdout_decision( + project_config, experiment, user_context, 'experiment', reasons + ) + if local_holdout is not None: + holdout_decisions.append(local_holdout) + continue # Get variation for experiment variation_result = self.get_variation( @@ -856,8 +842,9 @@ def get_decision_for_flag( 'error': False, 'reasons': reasons } - if global_holdout_result is not None: - result['holdout_decision'] = global_holdout_result['decision'] + if holdout_decisions: + result['holdout_decisions'] = holdout_decisions + result['holdout_decision'] = holdout_decisions[0] return result if global_holdout_result is not None: @@ -870,7 +857,7 @@ def get_decision_for_flag( # If no experiment decision, check rollouts rollout_decision, rollout_reasons = self.get_variation_for_rollout( - project_config, feature_flag, user_context + project_config, feature_flag, user_context, holdout_decisions ) if rollout_reasons: reasons.extend(rollout_reasons) @@ -893,10 +880,33 @@ def get_decision_for_flag( 'error': False, 'reasons': reasons } - if global_holdout_result is not None: - final_result['holdout_decision'] = global_holdout_result['decision'] + if holdout_decisions: + final_result['holdout_decisions'] = holdout_decisions + final_result['holdout_decision'] = holdout_decisions[0] return final_result + def _get_local_holdout_decision( + self, + project_config: ProjectConfig, + rule: entities.Experiment, + user_context: OptimizelyUserContext, + rule_type: str, + reasons: list[str] + ) -> Optional[Decision]: + """Returns the first local holdout decision the user is bucketed into for the rule, if any.""" + for holdout in project_config.get_holdouts_for_rule(rule.id): + holdout_result = self.get_variation_for_holdout(holdout, user_context, project_config) + reasons.extend(holdout_result['reasons']) + if holdout_result['decision'].variation is not None: + message = ( + f"The user '{user_context.user_id}' is bucketed into local holdout '{holdout.key}' " + f"for {rule_type} rule '{rule.key}'. Skipping the rule." + ) + self.logger.info(message) + reasons.append(message) + return holdout_result['decision'] + return None + def get_variation_for_holdout( self, holdout: entities.Holdout, diff --git a/optimizely/optimizely.py b/optimizely/optimizely.py index 0f7c1da5..bcae0f1c 100644 --- a/optimizely/optimizely.py +++ b/optimizely/optimizely.py @@ -1,4 +1,4 @@ -# Copyright 2016-2023, Optimizely +# Copyright 2016-2023, 2026, Optimizely # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. # You may obtain a copy of the License at @@ -1246,7 +1246,7 @@ def _create_optimizely_decision( decision_reasons: Optional[list[str]], decide_options: list[str], project_config: ProjectConfig, - holdout_decision: Optional[Decision] = None + holdout_decisions: Optional[list[Decision]] = None ) -> OptimizelyDecision: user_id = user_context.user_id feature_enabled = False @@ -1265,24 +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 + # Skipped local holdouts and global holdouts bypassed by exclude_targeted_deliveries + if (holdout_decisions 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 - ) + for holdout_decision in holdout_decisions: + 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 + ) decision_event_dispatched = True # Send impression event if Decision came from a feature @@ -1469,13 +1470,13 @@ def _decide_for_keys( user_context, merged_decide_options ) - holdout_decisions: dict[str, Optional[Decision]] = {} + holdout_decisions: dict[str, Optional[list[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') + holdout_decisions[flag_key] = decision_list[i].get('holdout_decisions') # 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, @@ -1496,7 +1497,7 @@ def _decide_for_keys( decision_reasons, merged_decide_options, project_config, - holdout_decision=holdout_decisions.get(key) + holdout_decisions=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.py b/tests/test_decision_service.py index b38a03b2..32d5ffdb 100644 --- a/tests/test_decision_service.py +++ b/tests/test_decision_service.py @@ -1,4 +1,4 @@ -# Copyright 2017-2021, Optimizely +# Copyright 2017-2021, 2026, Optimizely # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. # You may obtain a copy of the License at @@ -1510,7 +1510,7 @@ def test_get_variation_for_feature__returns_variation_for_feature_in_rollout(sel ) mock_get_variation_for_rollout.assert_called_once_with( - self.project_config, feature, user + self.project_config, feature, user, [] ) # Assert no log messages were generated diff --git a/tests/test_decision_service_holdout.py b/tests/test_decision_service_holdout.py index da85ae4e..e0f57ecc 100644 --- a/tests/test_decision_service_holdout.py +++ b/tests/test_decision_service_holdout.py @@ -12,6 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. +import copy import json from unittest import mock @@ -19,6 +20,7 @@ from optimizely import error_handler from optimizely import logger from optimizely import optimizely as optimizely_module +from optimizely import user_profile from optimizely.decision.optimizely_decide_option import OptimizelyDecideOption from optimizely.helpers import enums from tests import base @@ -1467,8 +1469,8 @@ def test_global_holdout_miss_falls_through_to_experiment(self): # Branch 2: Local holdout hit — user bucketed into local holdout for rule X # ------------------------------------------------------------------ - def test_local_holdout_hit_returns_holdout_decision_for_experiment_rule(self): - """User bucketed into local holdout for experiment rule X returns holdout decision. + def test_local_holdout_hit_skips_experiment_rule(self): + """User bucketed into local holdout for experiment rule X is excluded from rule X only. The experiment rule ID is '111127' (test_experiment linked to test_feature_in_experiment). The local holdout targets only this rule with full traffic. @@ -1489,13 +1491,14 @@ def test_local_holdout_hit_returns_holdout_decision_for_experiment_rule(self): result = ds.get_decision_for_flag(feature_flag, user_ctx, config) decision = result['decision'] - # User must be caught by local holdout - self.assertEqual(decision.source, enums.DecisionSources.HOLDOUT) + self.assertNotEqual(decision.source, enums.DecisionSources.HOLDOUT) + self.assertIsNone(decision.variation) # Regular experiment evaluation (get_variation) must not have run mock_get_var.assert_not_called() + self.assertEqual([d.experiment.key for d in result['holdout_decisions']], ['local_for_exp']) - def test_local_holdout_hit_returns_holdout_decision_for_delivery_rule(self): - """User bucketed into local holdout for delivery/rollout rule returns holdout decision. + def test_local_holdout_hit_then_no_rule_serves(self): + """User held out of the everyone-else delivery rule and matching no other rule gets no variation. Rule '211147' is the everyone-else rollout rule for 'test_feature_in_rollout'. The local holdout targets this delivery rule with full traffic. @@ -1512,9 +1515,10 @@ def test_local_holdout_hit_returns_holdout_decision_for_delivery_rule(self): user_ctx = opt.create_user_context('user_delivery_holdout', {}) result = ds.get_decision_for_flag(feature_flag, user_ctx, config) - self.assertIsNotNone(result) decision = result['decision'] - self.assertEqual(decision.source, enums.DecisionSources.HOLDOUT) + self.assertEqual(decision.source, enums.DecisionSources.ROLLOUT) + self.assertIsNone(decision.variation) + self.assertEqual([d.experiment.key for d in result['holdout_decisions']], ['local_for_delivery']) # ------------------------------------------------------------------ # Branch 3: Local holdout miss — user falls through to regular rule evaluation @@ -1541,6 +1545,7 @@ def test_local_holdout_miss_falls_through_to_regular_rule_evaluation(self): decision = result['decision'] # Source must NOT be HOLDOUT — missed the local holdout self.assertNotEqual(decision.source, enums.DecisionSources.HOLDOUT) + self.assertNotIn('holdout_decisions', result) # ------------------------------------------------------------------ # Branch 4: Rule specificity — local holdout for rule X does NOT affect rule Y @@ -1607,7 +1612,8 @@ def test_local_holdout_applies_to_experiment_rule(self): user_ctx = opt.create_user_context('user_exp_holdout', {}) result = ds.get_decision_for_flag(feature_flag, user_ctx, config) - self.assertEqual(result['decision'].source, enums.DecisionSources.HOLDOUT) + self.assertNotEqual(result['decision'].source, enums.DecisionSources.HOLDOUT) + self.assertEqual(len(result['holdout_decisions']), 1) def test_local_holdout_applies_to_rollout_delivery_rule(self): """Local holdout check applies to rollout/delivery rules in get_decision_for_flag.""" @@ -1624,9 +1630,8 @@ def test_local_holdout_applies_to_rollout_delivery_rule(self): user_ctx = opt.create_user_context('user_delivery_holdout', {}) result = ds.get_decision_for_flag(feature_flag, user_ctx, config) - self.assertIsNotNone(result) - decision = result['decision'] - self.assertEqual(decision.source, enums.DecisionSources.HOLDOUT) + self.assertNotEqual(result['decision'].source, enums.DecisionSources.HOLDOUT) + self.assertEqual(len(result['holdout_decisions']), 1) # ------------------------------------------------------------------ # Precedence: Global → Forced → Local → Regular rule @@ -1709,6 +1714,236 @@ def test_no_holdouts_at_all_falls_through_to_experiment(self): decision = result['decision'] self.assertNotEqual(decision.source, enums.DecisionSources.HOLDOUT) + def _make_opt_exp_with_rollout(self, local_holdouts, send_flag_decisions=True): + """Experiment rule '111127' followed by delivery rules of rollout '211111' (everyone-else at 100%).""" + cfg = copy.deepcopy(self.config_dict_with_features) + cfg['sendFlagDecisions'] = send_flag_decisions + for flag in cfg['featureFlags']: + if flag['key'] == 'test_feature_in_experiment': + flag['rolloutId'] = '211111' + for rollout in cfg['rollouts']: + for rule in rollout['experiments']: + if rule['id'] == '211147': + rule['trafficAllocation'] = [{'entityId': '211149', 'endOfRange': 10000}] + cfg['holdouts'] = [] + cfg['localHoldouts'] = local_holdouts + self.opt_obj = optimizely_module.Optimizely(json.dumps(cfg)) + return self.opt_obj + + @staticmethod + def _holdout_impressions(mock_send): + return [c.args[1].key for c in mock_send.call_args_list + if c.args[5] == str(enums.DecisionSources.HOLDOUT)] + + def test_local_holdout_hit_then_later_rule_serves(self): + """Held out of experiment rule; the later delivery rule serves and both events are sent.""" + opt = self._make_opt_exp_with_rollout([_holdout('lh_exp', 'for_exp', included_rules=['111127'])]) + config = opt.config_manager.get_config() + feature_flag = config.get_feature_from_key('test_feature_in_experiment') + + result = self._decision_svc().get_decision_for_flag( + feature_flag, opt.create_user_context('test_user', {}), config + ) + self.assertEqual(result['decision'].source, enums.DecisionSources.ROLLOUT) + self.assertEqual(result['decision'].experiment.id, '211147') + self.assertEqual([d.experiment.key for d in result['holdout_decisions']], ['for_exp']) + + with mock.patch.object(opt, '_send_impression_event', wraps=opt._send_impression_event) as mock_send: + decision = opt.create_user_context('test_user', {}).decide('test_feature_in_experiment') + + self.assertEqual(decision.rule_key, '211147') + self.assertEqual(self._holdout_impressions(mock_send), ['for_exp']) + self.assertEqual(mock_send.call_count, 2) + self.assertEqual(mock_send.call_args_list[1].args[5], str(enums.DecisionSources.ROLLOUT)) + + def test_local_holdout_hits_on_several_rules_send_several_holdout_events(self): + """Each rule's local holdout hit sends its own holdout impression.""" + opt = self._make_opt_exp_with_rollout([ + _holdout('lh_exp', 'for_exp', included_rules=['111127']), + _holdout('lh_del', 'for_del', included_rules=['211147']), + ]) + + with mock.patch.object(opt, '_send_impression_event', wraps=opt._send_impression_event) as mock_send: + decision = opt.create_user_context('test_user', {}).decide('test_feature_in_experiment') + + self.assertIsNone(decision.variation_key) + self.assertFalse(decision.enabled) + self.assertEqual(self._holdout_impressions(mock_send), ['for_exp', 'for_del']) + self.assertEqual(mock_send.call_count, 3) + final_call = mock_send.call_args_list[2] + self.assertEqual(final_call.args[5], str(enums.DecisionSources.ROLLOUT)) + self.assertIsNone(final_call.args[2]) + + def test_deprecated_holdout_decision_is_first_of_holdout_decisions(self): + opt = self._make_opt_exp_with_rollout([ + _holdout('lh_exp', 'for_exp', included_rules=['111127']), + _holdout('lh_del', 'for_del', included_rules=['211147']), + ]) + config = opt.config_manager.get_config() + + result = self._decision_svc().get_decision_for_flag( + config.get_feature_from_key('test_feature_in_experiment'), opt.create_user_context('test_user', {}), config + ) + self.assertEqual([d.experiment.key for d in result['holdout_decisions']], ['for_exp', 'for_del']) + self.assertIs(result['holdout_decision'], result['holdout_decisions'][0]) + + def test_one_local_holdout_on_two_rules_sends_two_holdout_events(self): + """A single local holdout targeting two rules is hit on each rule and sends an event per hit.""" + opt = self._make_opt_exp_with_rollout([ + _holdout('lh_shared', 'shared', included_rules=['111127', '211147']), + ]) + config = opt.config_manager.get_config() + + result = self._decision_svc().get_decision_for_flag( + config.get_feature_from_key('test_feature_in_experiment'), opt.create_user_context('test_user', {}), config + ) + self.assertEqual([d.experiment.key for d in result['holdout_decisions']], ['shared', 'shared']) + + with mock.patch.object(opt, '_send_impression_event', wraps=opt._send_impression_event) as mock_send: + decision = opt.create_user_context('test_user', {}).decide('test_feature_in_experiment') + + self.assertIsNone(decision.variation_key) + self.assertEqual(self._holdout_impressions(mock_send), ['shared', 'shared']) + self.assertEqual(mock_send.call_count, 3) + + def test_local_holdout_hit_beats_stored_variation_without_profile_save(self): + """A sticky stored variation for a held-out rule is not served and the profile is not updated.""" + opt = self._make_opt_exp_with_rollout([_holdout('lh_exp', 'for_exp', included_rules=['111127'])]) + config = opt.config_manager.get_config() + feature_flag = config.get_feature_from_key('test_feature_in_experiment') + ups = mock.MagicMock() + ups.lookup.return_value = { + 'user_id': 'test_user', 'experiment_bucket_map': {'111127': {'variation_id': '111129'}} + } + tracker = user_profile.UserProfileTracker('test_user', ups) + tracker.load_user_profile() + + result = self._decision_svc().get_decision_for_flag( + feature_flag, opt.create_user_context('test_user', {}), config, user_profile_tracker=tracker + ) + + self.assertEqual(result['decision'].experiment.id, '211147') + self.assertEqual([d.experiment.key for d in result['holdout_decisions']], ['for_exp']) + self.assertFalse(tracker.profile_updated) + + def test_local_holdout_hit_on_delivery_rule_sends_event_with_send_flag_decisions_off(self): + """Holdout impression is sent for a delivery-rule hit even when sendFlagDecisions is false.""" + opt = self._make_opt_exp_with_rollout( + [_holdout('lh_del', 'for_del', included_rules=['211147'])], send_flag_decisions=False + ) + + with mock.patch.object(opt, '_send_impression_event', wraps=opt._send_impression_event) as mock_send: + decision = opt.create_user_context('test_user', {}).decide('test_feature_in_rollout') + + self.assertIsNone(decision.variation_key) + self.assertEqual(self._holdout_impressions(mock_send), ['for_del']) + self.assertEqual(mock_send.call_count, 1) + + def test_legacy_api_skips_local_holdout_rule_without_holdout_event(self): + """is_feature_enabled skips a locally held-out rule without sending a holdout impression.""" + opt = self._make_opt_exp_with_rollout([_holdout('lh_exp', 'for_exp', included_rules=['111127'])]) + + with mock.patch.object(opt, '_send_impression_event', wraps=opt._send_impression_event) as mock_send: + opt.is_feature_enabled('test_feature_in_experiment', 'test_user') + + self.assertEqual(self._holdout_impressions(mock_send), []) + self.assertEqual([(c.args[4], c.args[5]) for c in mock_send.call_args_list], + [('211147', str(enums.DecisionSources.ROLLOUT))]) + + def test_disable_decision_event_suppresses_local_holdout_event(self): + """DISABLE_DECISION_EVENT suppresses the holdout impression for a skipped rule.""" + opt = self._make_opt_exp_with_rollout([_holdout('lh_exp', 'for_exp', included_rules=['111127'])]) + + with mock.patch.object(opt, '_send_impression_event', wraps=opt._send_impression_event) as mock_send: + decision = opt.create_user_context('test_user', {}).decide( + 'test_feature_in_experiment', [OptimizelyDecideOption.DISABLE_DECISION_EVENT] + ) + + self.assertEqual(decision.rule_key, '211147') + mock_send.assert_not_called() + + def test_local_holdout_hit_on_first_experiment_then_second_experiment_serves(self): + """Held out of the first experiment rule; the second experiment rule serves.""" + cfg = copy.deepcopy(self.config_dict_with_features) + cfg['experiments'].append({ + 'key': 'second_experiment', + 'status': 'Running', + 'forcedVariations': {}, + 'layerId': '111183', + 'audienceIds': [], + 'trafficAllocation': [{'entityId': '111141', 'endOfRange': 10000}], + 'id': '111140', + 'variations': [{'key': 'second_variation', 'id': '111141', 'featureEnabled': True, 'variables': []}], + }) + for flag in cfg['featureFlags']: + if flag['key'] == 'test_feature_in_experiment': + flag['experimentIds'] = ['111127', '111140'] + cfg['holdouts'] = [] + cfg['localHoldouts'] = [_holdout('lh_exp', 'for_exp', included_rules=['111127'])] + self.opt_obj = opt = optimizely_module.Optimizely(json.dumps(cfg)) + config = opt.config_manager.get_config() + + result = self._decision_svc().get_decision_for_flag( + config.get_feature_from_key('test_feature_in_experiment'), opt.create_user_context('test_user', {}), config + ) + self.assertEqual(result['decision'].source, enums.DecisionSources.FEATURE_TEST) + self.assertEqual(result['decision'].experiment.id, '111140') + self.assertEqual([d.experiment.key for d in result['holdout_decisions']], ['for_exp']) + + with mock.patch.object(opt, '_send_impression_event', wraps=opt._send_impression_event) as mock_send: + decision = opt.create_user_context('test_user', {}).decide('test_feature_in_experiment') + + self.assertEqual(decision.rule_key, 'second_experiment') + self.assertEqual(decision.variation_key, 'second_variation') + self.assertEqual(self._holdout_impressions(mock_send), ['for_exp']) + self.assertEqual(mock_send.call_count, 2) + self.assertEqual(mock_send.call_args_list[1].args[5], str(enums.DecisionSources.FEATURE_TEST)) + + def test_local_holdout_hit_then_later_rule_error_sends_no_impressions(self): + """Held out of the first rule; a CMAB error on the later rule yields an error decision and no events.""" + cfg = copy.deepcopy(self.config_dict_with_features) + cfg['experiments'].append({ + 'key': 'cmab_experiment', + 'status': 'Running', + 'forcedVariations': {}, + 'layerId': '111183', + 'audienceIds': [], + 'trafficAllocation': [{'entityId': '111141', 'endOfRange': 10000}], + 'id': '111140', + 'variations': [{'key': 'cmab_variation', 'id': '111141', 'featureEnabled': True, 'variables': []}], + 'cmab': {'attributeIds': [], 'trafficAllocation': 10000}, + }) + for flag in cfg['featureFlags']: + if flag['key'] == 'test_feature_in_experiment': + flag['experimentIds'] = ['111127', '111140'] + cfg['holdouts'] = [] + cfg['localHoldouts'] = [_holdout('lh_exp', 'for_exp', included_rules=['111127'])] + self.opt_obj = opt = optimizely_module.Optimizely(json.dumps(cfg)) + config = opt.config_manager.get_config() + holdout_hit = ("The user 'test_user' is bucketed into local holdout 'for_exp' " + "for experiment rule 'test_experiment'.") + cmab_error = enums.Errors.CMAB_FETCH_FAILED_DETAILED.format('cmab_experiment') + + with mock.patch.object(opt.decision_service.cmab_service, 'get_decision', side_effect=Exception('boom')): + result = opt.decision_service.get_decision_for_flag( + config.get_feature_from_key('test_feature_in_experiment'), + opt.create_user_context('test_user', {}), config + ) + self.assertTrue(result['error']) + self.assertEqual(result['decision'].experiment.id, '111140') + self.assertNotIn('holdout_decisions', result) + self.assertTrue(any(r.startswith(holdout_hit) for r in result['reasons'])) + self.assertIn(cmab_error, result['reasons']) + + with mock.patch.object(opt, '_send_impression_event', wraps=opt._send_impression_event) as mock_send: + decision = opt.create_user_context('test_user', {}).decide('test_feature_in_experiment') + + self.assertIsNone(decision.variation_key) + self.assertIsNone(decision.rule_key) + self.assertFalse(decision.enabled) + self.assertIn(cmab_error, decision.reasons) + mock_send.assert_not_called() + # --------------------------------------------------------------------------- # Exclude Targeted Deliveries Tests @@ -1858,7 +2093,7 @@ def test_global_holdout_exclude_td_true_blocks_ab_experiment(self): decision = result['decision'] self.assertNotEqual(decision.source, enums.DecisionSources.HOLDOUT) mock_get_var.assert_not_called() - self.assertIsNotNone(result.get('holdout_decision')) + self.assertIsNotNone(result.get('holdout_decisions')) # ------------------------------------------------------------------ # Test 4: Global holdout with exclude_targeted_deliveries=True, no TD matches @@ -1880,7 +2115,7 @@ def test_global_holdout_exclude_td_true_no_td_returns_non_holdout_decision(self) decision = result['decision'] self.assertNotEqual(decision.source, enums.DecisionSources.HOLDOUT) - self.assertIsNotNone(result.get('holdout_decision')) + self.assertIsNotNone(result.get('holdout_decisions')) expected_reason = ( "Holdout \"global_exclude_td\" has excludeTargetedDeliveries enabled, " @@ -1911,7 +2146,8 @@ def test_local_holdout_delivery_rule_exclude_td_true_still_applies(self): result = ds.get_decision_for_flag(feature_flag, user_ctx, config) decision = result['decision'] - self.assertEqual(decision.source, enums.DecisionSources.HOLDOUT) + self.assertNotEqual(decision.source, enums.DecisionSources.HOLDOUT) + self.assertEqual([d.experiment.key for d in result['holdout_decisions']], ['local_delivery_exclude']) # ------------------------------------------------------------------ # Test 6: Local holdout on experiment rule (TD type) with exclude_targeted_deliveries=True @@ -1937,7 +2173,8 @@ def test_local_holdout_td_experiment_exclude_td_true_still_applies(self): result = ds.get_decision_for_flag(feature_flag, user_ctx, config) decision = result['decision'] - self.assertEqual(decision.source, enums.DecisionSources.HOLDOUT) + self.assertNotEqual(decision.source, enums.DecisionSources.HOLDOUT) + self.assertEqual([d.experiment.key for d in result['holdout_decisions']], ['local_td_exclude']) # ------------------------------------------------------------------ # Test 7: Local holdout on experiment rule (A/B type) with exclude_targeted_deliveries=True @@ -1966,8 +2203,9 @@ def test_local_holdout_ab_experiment_exclude_td_true_still_applies(self): decision = result['decision'] # A/B experiment should still be blocked by local holdout - self.assertEqual(decision.source, enums.DecisionSources.HOLDOUT) + self.assertNotEqual(decision.source, enums.DecisionSources.HOLDOUT) mock_get_var.assert_not_called() + self.assertEqual([d.experiment.key for d in result['holdout_decisions']], ['local_ab_exclude']) # ------------------------------------------------------------------ # Test 8: Missing field (backward compatibility) @@ -1998,12 +2236,12 @@ def test_missing_exclude_td_field_defaults_to_false(self): self.assertFalse(holdout.exclude_targeted_deliveries) # ------------------------------------------------------------------ - # Test 9: TD match returns holdout_decision in result + # Test 9: TD match returns holdout_decisions 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.""" + the result contains holdout_decisions with the bypassed holdout.""" opt = self._make_opt_with_td( [_holdout_with_etd('gh1', 'global_exclude_td', exclude_targeted_deliveries=True)], experiment_type='td', @@ -2017,16 +2255,16 @@ def test_global_holdout_exclude_td_true_td_match_has_holdout_decision(self): 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) + self.assertEqual(len(result['holdout_decisions']), 1) + self.assertEqual(result['holdout_decisions'][0].source, enums.DecisionSources.HOLDOUT) # ------------------------------------------------------------------ - # Test 10: No TD match returns non-holdout with holdout_decision attached + # Test 10: No TD match returns non-holdout with holdout_decisions 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, + result is non-holdout but holdout_decisions 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)], @@ -2040,9 +2278,9 @@ def test_global_holdout_exclude_td_true_no_td_has_holdout_decision(self): 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) + holdout_decs = result.get('holdout_decisions') + self.assertEqual(len(holdout_decs), 1) + self.assertEqual(holdout_decs[0].source, enums.DecisionSources.HOLDOUT) expected_reason = ( "Holdout \"global_exclude_td\" has excludeTargetedDeliveries enabled, "