[AI-FSSDK] [FSSDK-12735] Add holdout event dispatch, local holdout flag isolation, and TD null passthrough

Author: esrakartalOpt

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,

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

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)