Implement suggested comments

Author: esrakartalOpt

optimizely/decision_service.py

@@ -12,7 +12,9 @@
 # limitations under the License.
 
 from __future__ import annotations
-from typing import TYPE_CHECKING, NamedTuple, Optional, Sequence, List, TypedDict, Dict
+from typing import TYPE_CHECKING, NamedTuple, Optional, Sequence, List, TypedDict
+
+from optimizely.helpers.types import HoldoutDict
 
 from . import bucketer
 from . import entities
@@ -83,7 +85,6 @@ class Decision(NamedTuple):
     variation: Optional[entities.Variation]
     source: Optional[str]
     cmab_uuid: Optional[str]
-    holdout: Optional[Dict[str, str]] = None
 
 
 class DecisionService:
@@ -713,7 +714,8 @@ def get_decision_for_flag(
             reasons.extend(holdout_decision['reasons'])
 
             decision = holdout_decision['decision']
-            if (decision.experiment is None and decision.variation is None and decision.holdout is None):
+            # Check if user was bucketed into holdout (has a variation)
+            if decision.variation is None:
                 continue
 
             message = (
@@ -746,15 +748,15 @@ def get_decision_for_flag(
 
     def get_variation_for_holdout(
         self,
-        holdout: Dict[str, str],
+        holdout: HoldoutDict,
         user_context: OptimizelyUserContext,
         project_config: ProjectConfig
     ) -> DecisionResult:
         """
         Get the variation for holdout.
 
         Args:
-            holdout: The holdout configuration.
+            holdout: The holdout configuration (HoldoutDict).
             user_context: The user context.
             project_config: The project config.
 
@@ -773,7 +775,7 @@ def get_variation_for_holdout(
             self.logger.info(message)
             decide_reasons.append(message)
             return {
-                'decision': Decision(None, None, None, None, None),
+                'decision': Decision(None, None, enums.DecisionSources.HOLDOUT, None),
                 'error': False,
                 'reasons': decide_reasons
             }
@@ -801,7 +803,7 @@ def get_variation_for_holdout(
             self.logger.debug(message)
             decide_reasons.append(message)
             return {
-                'decision': Decision(None, None, None, None, None),
+                'decision': Decision(None, None, enums.DecisionSources.HOLDOUT, None),
                 'error': False,
                 'reasons': decide_reasons
             }
@@ -822,13 +824,12 @@ def get_variation_for_holdout(
             self.logger.info(message)
             decide_reasons.append(message)
 
-            # Create Decision with holdout - experiment is None, holdout field contains the holdout dict
+            # Create Decision for holdout - experiment is None, source is HOLDOUT
             holdout_decision: Decision = Decision(
                 experiment=None,
-                variation=None,
+                variation=variation,
                 source=enums.DecisionSources.HOLDOUT,
-                cmab_uuid=None,
-                holdout=holdout
+                cmab_uuid=None
             )
             return {
                 'decision': holdout_decision,
@@ -840,7 +841,7 @@ def get_variation_for_holdout(
         self.logger.info(message)
         decide_reasons.append(message)
         return {
-            'decision': Decision(None, None, None, None, None),
+            'decision': Decision(None, None, enums.DecisionSources.HOLDOUT, None),
             'error': False,
             'reasons': decide_reasons
         }

optimizely/helpers/types.py

@@ -12,7 +12,7 @@
 # limitations under the License.
 from __future__ import annotations
 
-from typing import Optional, Any
+from typing import Literal, Optional, Any
 from sys import version_info
 
 
@@ -115,3 +115,16 @@ class CmabDict(BaseEntity):
     """Cmab dict from parsed datafile json."""
     attributeIds: list[str]
     trafficAllocation: int
+
+
+HoldoutStatus = Literal['Draft', 'Running', 'Concluded', 'Archived']
+
+
+class HoldoutDict(ExperimentDict):
+    """Holdout dict from parsed datafile json.
+
+    Extends ExperimentDict with holdout-specific properties.
+    """
+    holdoutStatus: HoldoutStatus
+    includedFlags: list[str]
+    excludedFlags: list[str]

tests/test_decision_service_holdout.py

@@ -290,8 +290,8 @@ def test_user_bucketed_into_holdout_returns_before_experiments(self):
         if decision_result.get('decision'):
             decision = decision_result['decision']
             self.assertEqual(decision.source, enums.DecisionSources.HOLDOUT)
-            self.assertIsNotNone(decision.holdout)
-            self.assertEqual(decision.holdout.get('key'), 'test_holdout')
+            self.assertIsNotNone(decision.variation)
+            self.assertIsNone(decision.experiment)
 
     def test_no_holdout_decision_falls_through_to_experiment_and_rollout(self):
         """When holdout returns no decision, should fall through to experiment and rollout evaluation."""