refactor

Author: raju-opti

lib/core/decision_service/index.ts

@@ -74,10 +74,12 @@ import {
 } from 'log_message';
 import { OptimizelyError } from '../../error/optimizly_error';
 import { CmabService } from './cmab/cmab_service';
-import { Maybe, OpType, OpValue } from '../../utils/type';
+import { Maybe, OpType } from '../../utils/type';
 import { Value } from '../../utils/promise/operation_value';
 import { Platform } from '../../platform_support';
 
+import { localHoldout } from '../../feature_toggle';
+
 export const EXPERIMENT_NOT_RUNNING = 'Experiment %s is not running.';
 export const RETURNING_STORED_VARIATION =
   'Returning previously activated variation "%s" of experiment "%s" for user "%s" from user profile.';
@@ -135,36 +137,48 @@ interface DecisionServiceOptions {
   cmabService: CmabService;
 }
 
-interface DeliveryRuleResponse<T, K> extends DecisionResponse<T> {
-  skipToEveryoneElse: K;
-  /** Set when a local holdout was hit for this delivery rule (FSSDK-12369). */
-  localHoldoutDecision?: DecisionObj;
-}
 
-interface UserProfileTracker {
-  userProfile: ExperimentBucketMap | null;
-  isProfileUpdated: boolean;
-}
+export type DecisionReason = [string, ...any[]];
 
-type VarationKeyWithCmabParams = {
-  variationKey?: string;
+export type VariationIdWithCmabParams = {
+  variationId? : string;
   cmabUuid?: string;
-  /**
-   * Set when the variation comes from a local holdout (FSSDK-12369).
-   * When present, the caller should use this as the full decision result
-   * instead of constructing one from the experiment.
-   */
-  localHoldoutDecision?: DecisionObj;
 };
-export type DecisionReason = [string, ...any[]];
-export type VariationResult = DecisionResponse<VarationKeyWithCmabParams>;
-export type DecisionResult = DecisionResponse<DecisionObj>;
-type VariationIdWithCmabParams = {
-  variationId? : string;
+
+export type VariationKeyWithCmabParams = {
+  variationKey? : string;
   cmabUuid?: string;
 };
+
+export type DecisionResult = DecisionResponse<DecisionObj>;
+
+
+type LocalHoldoutResult = DecisionResponse<DecisionObj> & {
+  holdoutDecision: true
+}
+
+type ExperimentEvaluationResult = DecisionResponse<VariationKeyWithCmabParams> & {
+  holdoutDecision?: false;
+}
+
+export type ExperimentRuleResponse = ExperimentEvaluationResult | LocalHoldoutResult;
+
+type DeliveryEvaluationResult = DecisionResponse<Variation | null> & {
+  holdoutDecision?: false;
+  skipToEveryoneElse: boolean;
+}
+
+type DeliveryRuleResponse = DeliveryEvaluationResult | LocalHoldoutResult;
+
+type VariationKeyResult = DecisionResponse<VariationKeyWithCmabParams>
+
 export type DecideOptionsMap = Partial<Record<OptimizelyDecideOption, boolean>>;
 
+interface UserProfileTracker {
+  userProfile: ExperimentBucketMap | null;
+  isProfileUpdated: boolean;
+}
+
 export const CMAB_DUMMY_ENTITY_ID= '$'
 
 export const LOGGER_NAME = 'DecisionService';
@@ -222,7 +236,7 @@ export class DecisionService {
     user: OptimizelyUserContext,
     decideOptions: DecideOptionsMap,
     userProfileTracker?: UserProfileTracker,
-  ): Value<OP, VariationResult> { 
+  ): Value<OP, VariationKeyResult> { 
     const userId = user.getUserId();
 
     const experimentKey = experiment.key;
@@ -310,7 +324,7 @@ export class DecisionService {
       this.getDecisionForCmabExperiment(op, configObj, experiment, user, decideOptions) :
       this.getDecisionFromBucketer(op, configObj, experiment, user);
 
-    return decisionVariationValue.then((variationResult): Value<OP, VariationResult> => {
+    return decisionVariationValue.then((variationResult): Value<OP, VariationKeyResult> => {
       decideReasons.push(...variationResult.reasons);
       if (variationResult.error) {
         return Value.of(op, {
@@ -1083,14 +1097,22 @@ export class DecisionService {
         op, configObj, feature, fromIndex + 1, user, decideReasons, decideOptions, userProfileTracker);
     }
 
-    const decisionVariationValue = this.getVariationFromExperimentRule(
+    const experimentResponse = this.getVariationFromExperimentRule(
       op, configObj, feature.key, experiment, user, decideOptions, userProfileTracker,
     );
 
-    return decisionVariationValue.then((decisionVariation) => {
-      decideReasons.push(...decisionVariation.reasons);
+    return experimentResponse.then((experimentResponse) => {
+      decideReasons.push(...experimentResponse.reasons);
 
-      if (decisionVariation.error) {
+      // If a local holdout was hit, return the holdout decision directly (preserves holdout as experiment).
+      if (experimentResponse.holdoutDecision) {
+        return Value.of(op, {
+          result: experimentResponse.result,
+          reasons: decideReasons,
+        });
+      }
+
+      if (experimentResponse.error) {
         return Value.of(op, {
           error: true,
           result: {
@@ -1102,28 +1124,20 @@ export class DecisionService {
         });
       }
 
-      // If a local holdout was hit, return the holdout decision directly (preserves holdout as experiment).
-      if (decisionVariation.result.localHoldoutDecision) {
-        return Value.of(op, {
-          result: decisionVariation.result.localHoldoutDecision,
-          reasons: decideReasons,
-        });
-      }
-
-      if(!decisionVariation.result.variationKey) {
+      if(!experimentResponse.result.variationKey) {
         return this.traverseFeatureExperimentList(
           op, configObj, feature, fromIndex + 1, user, decideReasons, decideOptions, userProfileTracker);
       }
 
-      const variationKey = decisionVariation.result.variationKey;
+      const variationKey = experimentResponse.result.variationKey;
       let variation: Variation | null = experiment.variationKeyMap[variationKey];
       if (!variation) {
         variation = getFlagVariationByKey(configObj, feature.key, variationKey);
       }
 
       return Value.of(op, {
         result: {
-          cmabUuid: decisionVariation.result.cmabUuid,
+          cmabUuid: experimentResponse.result.cmabUuid,
           experiment,
           variation,
           decisionSource: DECISION_SOURCES.FEATURE_TEST,
@@ -1139,11 +1153,10 @@ export class DecisionService {
     user: OptimizelyUserContext,
   ): DecisionResponse<DecisionObj> {
     const decideReasons: DecisionReason[] = [];
-    let decisionObj: DecisionObj;
     if (!feature.rolloutId) {
       this.logger?.debug(NO_ROLLOUT_EXISTS, feature.key);
       decideReasons.push([NO_ROLLOUT_EXISTS, feature.key]);
-      decisionObj = {
+      const decisionObj = {
         experiment: null,
         variation: null,
         decisionSource: DECISION_SOURCES.ROLLOUT,
@@ -1163,7 +1176,7 @@ export class DecisionService {
         feature.key,
       );
       decideReasons.push([INVALID_ROLLOUT_ID, feature.rolloutId, feature.key]);
-      decisionObj = {
+      const decisionObj = {
         experiment: null,
         variation: null,
         decisionSource: DECISION_SOURCES.ROLLOUT,
@@ -1181,7 +1194,7 @@ export class DecisionService {
         feature.rolloutId,
       );
       decideReasons.push([ROLLOUT_HAS_NO_EXPERIMENTS, feature.rolloutId]);
-      decisionObj = {
+      const decisionObj = {
         experiment: null,
         variation: null,
         decisionSource: DECISION_SOURCES.ROLLOUT,
@@ -1191,26 +1204,34 @@ export class DecisionService {
         reasons: decideReasons,
       };
     }
-    let decisionVariation;
-    let skipToEveryoneElse;
-    let variation;
-    let rolloutRule;
+
     let index = 0;
     while (index < rolloutRules.length) {
-      decisionVariation = this.getVariationFromDeliveryRule(configObj, feature.key, rolloutRules, index, user);
-      decideReasons.push(...decisionVariation.reasons);
-      variation = decisionVariation.result;
-      skipToEveryoneElse = decisionVariation.skipToEveryoneElse;
-      if (variation) {
-        // If a local holdout was hit for this delivery rule, use its decision object directly.
-        if (decisionVariation.localHoldoutDecision) {
-          return {
-            result: decisionVariation.localHoldoutDecision,
-            reasons: decideReasons,
-          };
+      const deliveryRuleResponse = this.getVariationFromDeliveryRule(configObj, feature.key, rolloutRules, index, user);
+
+      decideReasons.push(...deliveryRuleResponse.reasons);
+
+      // If a local holdout was hit for this delivery rule, use its decision object directly.
+      if (deliveryRuleResponse.holdoutDecision) {
+        return {
+          reasons: decideReasons,
+          result: deliveryRuleResponse.result,
         }
-        rolloutRule = configObj.experimentIdMap[rolloutRules[index].id];
-        decisionObj = {
+      }
+
+
+      const variation = deliveryRuleResponse.result;
+      const skipToEveryoneElse = deliveryRuleResponse.skipToEveryoneElse;
+
+      if (variation) {
+        // if (decisionVariation.localHoldoutDecision) {
+        //   return {
+        //     result: decisionVariation.localHoldoutDecision,
+        //     reasons: decideReasons,
+        //   };
+        // }
+        const rolloutRule = configObj.experimentIdMap[rolloutRules[index].id];
+        const decisionObj = {
           experiment: rolloutRule,
           variation: variation,
           decisionSource: DECISION_SOURCES.ROLLOUT
@@ -1224,7 +1245,7 @@ export class DecisionService {
       index = skipToEveryoneElse ? (rolloutRules.length - 1) : (index + 1);
     }
 
-    decisionObj = {
+    const decisionObj = {
       experiment: null,
       variation: null,
       decisionSource: DECISION_SOURCES.ROLLOUT,
@@ -1573,7 +1594,7 @@ export class DecisionService {
     user: OptimizelyUserContext,
     decideOptions: DecideOptionsMap,
     userProfileTracker?: UserProfileTracker,
-  ): Value<OP, VariationResult> {
+  ): Value<OP, ExperimentRuleResponse> {
     const decideReasons: DecisionReason[] = [];
 
     // check forced decision first
@@ -1588,44 +1609,34 @@ export class DecisionService {
       });
     }
 
-    // Check local holdouts targeting this specific experiment rule (FSSDK-12369).
-    // Inserted immediately after the forced-decision block, before regular rule evaluation.
-    const localHoldoutsForExperiment = getHoldoutsForRule(configObj, rule.id);
-    for (const holdout of localHoldoutsForExperiment) {
-      const holdoutDecision = this.getVariationForHoldout(configObj, holdout, user);
-      decideReasons.push(...holdoutDecision.reasons);
-      if (holdoutDecision.result.variation) {
-        // Signal the caller to use the holdout decision directly, preserving the holdout as experiment.
-        return Value.of(op, {
-          result: {
-            variationKey: holdoutDecision.result.variation.key,
-            localHoldoutDecision: holdoutDecision.result,
-          },
-          reasons: decideReasons,
-        });
+    if (localHoldout()) {
+      // Check local holdouts targeting this specific experiment rule.
+      // Inserted immediately after the forced-decision block, before regular rule evaluation.
+      const localHoldoutsForExperiment = getHoldoutsForRule(configObj, rule.id);
+      for (const holdout of localHoldoutsForExperiment) {
+        const holdoutDecision = this.getVariationForHoldout(configObj, holdout, user);
+        decideReasons.push(...holdoutDecision.reasons);
+        if (holdoutDecision.result.variation) {
+          // Signal the caller to use the holdout decision directly, preserving the holdout as experiment.
+          return Value.of<OP, LocalHoldoutResult>(op, {
+            result: holdoutDecision.result,
+            reasons: decideReasons,
+            holdoutDecision: true,
+          });
+        }
       }
     }
 
     const decisionVariationValue = this.resolveVariation(op, configObj, rule, user, decideOptions, userProfileTracker);
 
     return decisionVariationValue.then((variationResult) => {
       decideReasons.push(...variationResult.reasons);
-      return Value.of(op, {
+      return Value.of<OP, ExperimentEvaluationResult>(op, {
         error: variationResult.error,
         result: variationResult.result,
         reasons: decideReasons,
       });
     });
-
-    // return response;
-
-    // decideReasons.push(...decisionVariation.reasons);
-    // const variationKey = decisionVariation.result;
-
-    // return {
-    //   result: variationKey,
-    //   reasons: decideReasons,
-    // };
   }
 
   private getVariationFromDeliveryRule(
@@ -1634,7 +1645,7 @@ export class DecisionService {
     rules: Experiment[],
     ruleIndex: number,
     user: OptimizelyUserContext
-  ): DeliveryRuleResponse<Variation | null, boolean> {
+  ): DeliveryRuleResponse {
     const decideReasons: DecisionReason[] = [];
     let skipToEveryoneElse = false;
 
@@ -1652,19 +1663,20 @@ export class DecisionService {
       };
     }
 
-    // Check local holdouts targeting this specific delivery rule (FSSDK-12369).
-    // Inserted immediately after the forced-decision block, before audience and traffic allocation checks.
-    const localHoldoutsForDelivery = getHoldoutsForRule(configObj, rule.id);
-    for (const holdout of localHoldoutsForDelivery) {
-      const holdoutDecision = this.getVariationForHoldout(configObj, holdout, user);
-      decideReasons.push(...holdoutDecision.reasons);
-      if (holdoutDecision.result.variation) {
-        return {
-          result: holdoutDecision.result.variation,
-          reasons: decideReasons,
-          skipToEveryoneElse,
-          localHoldoutDecision: holdoutDecision.result,
-        };
+    if (localHoldout()) {
+      // Check local holdouts targeting this specific delivery rule (FSSDK-12369).
+      // Inserted immediately after the forced-decision block, before audience and traffic allocation checks.
+      const localHoldoutsForDelivery = getHoldoutsForRule(configObj, rule.id);
+      for (const holdout of localHoldoutsForDelivery) {
+        const holdoutDecision = this.getVariationForHoldout(configObj, holdout, user);
+        decideReasons.push(...holdoutDecision.reasons);
+        if (holdoutDecision.result.variation) {
+          return {
+            result: holdoutDecision.result,
+            reasons: decideReasons,
+            holdoutDecision: true,
+          };
+        }
       }
     }
 

lib/feature_toggle.ts

@@ -39,4 +39,7 @@ import { Platform } from './platform_support';
 
 export type IfActive<T extends () => boolean, Y, N = unknown> = ReturnType<T> extends true ? Y : N;
 
+
+export const localHoldout = () => true as const;
+
 export const __platforms: Platform[] = ['__universal__'];