diff --git a/lib/core/decision_service/index.spec.ts b/lib/core/decision_service/index.spec.ts index af76674b6..866bb8166 100644 --- a/lib/core/decision_service/index.spec.ts +++ b/lib/core/decision_service/index.spec.ts @@ -1960,55 +1960,7 @@ describe('DecisionService', () => { }); }); - it("should consider global holdout even if local holdout is present", async () => { - const { decisionService } = getDecisionService(); - const datafile = getHoldoutTestDatafile(); - const newEntry = { - id: 'holdout_included_id', - key: 'holdout_included', - status: 'Running', - includedFlags: ['1001'], - excludedFlags: [], - audienceIds: ['4002'], // age_40 audience - audienceConditions: ['or', '4002'], - variations: [ - { - id: 'holdout_variation_included_id', - key: 'holdout_variation_included', - variables: [], - }, - ], - trafficAllocation: [ - { - entityId: 'holdout_variation_included_id', - endOfRange: 5000, - }, - ], - }; - datafile.holdouts = [newEntry, ...datafile.holdouts]; - const config = createProjectConfig(datafile); - const user = new OptimizelyUserContext({ - optimizely: {} as any, - userId: 'tester', - attributes: { - age: 20, // satisfies both global holdout (age_22) and included holdout (age_40) audiences - }, - }); - const feature = config.featureKeyMap['flag_1']; - const value = decisionService.resolveVariationsForFeatureList('async', config, [feature], user, {}).get(); - - expect(value).toBeInstanceOf(Promise); - - const variation = (await value)[0]; - - expect(variation.result).toEqual({ - experiment: config.holdoutIdMap && config.holdoutIdMap['holdout_running_id'], - variation: config.variationIdMap['holdout_variation_running_id'], - decisionSource: DECISION_SOURCES.HOLDOUT, - }); - }); - - it("should consider local holdout if misses global holdout", async () => { + it("should consider next global holdout if misses previous holdouts", async () => { const { decisionService } = getDecisionService(); const datafile = getHoldoutTestDatafile(); @@ -2016,7 +1968,7 @@ describe('DecisionService', () => { id: 'holdout_included_specific_id', key: 'holdout_included_specific', status: 'Running', - includedFlags: ['1001'], + includedFlags: [], excludedFlags: [], audienceIds: ['4002'], // age_60 audience (age <= 60) audienceConditions: ['or', '4002'], @@ -2039,7 +1991,7 @@ describe('DecisionService', () => { optimizely: {} as any, userId: 'test_holdout_user', attributes: { - age: 50, // Does not satisfy global holdout (age_22, age <= 22) but satisfies included holdout (age_60, age <= 60) + age: 50, // Does not satisfy first global holdout (age_22, age <= 22) but satisfies newly added holdout (age_60, age <= 60) }, }); const feature = config.featureKeyMap['flag_1']; @@ -2195,42 +2147,6 @@ describe('DecisionService', () => { }); }); - it('should skip holdouts excluded for specific flags', async () => { - const { decisionService } = getDecisionService(); - const datafile = getHoldoutTestDatafile(); - - datafile.holdouts = datafile.holdouts.map((holdout: any) => { - if(holdout.id === 'holdout_running_id') { - return { - ...holdout, - excludedFlags: ['1001'] - } - } - return holdout; - }); - - const config = createProjectConfig(datafile); - const user = new OptimizelyUserContext({ - optimizely: {} as any, - userId: 'tester', - attributes: { - age: 15, // satisfies age_22 audience condition (age <= 22) for global holdout, but holdout excludes flag_1 - }, - }); - const feature = config.featureKeyMap['flag_1']; - const value = decisionService.resolveVariationsForFeatureList('async', config, [feature], user, {}).get(); - - expect(value).toBeInstanceOf(Promise); - - const variation = (await value)[0]; - - expect(variation.result).toEqual({ - experiment: config.experimentKeyMap['exp_1'], - variation: config.variationIdMap['5001'], - decisionSource: DECISION_SOURCES.FEATURE_TEST, - }); - }); - it('should handle multiple holdouts and use first matching one', async () => { const { decisionService } = getDecisionService(); const datafile = getHoldoutTestDatafile(); diff --git a/lib/core/decision_service/index.ts b/lib/core/decision_service/index.ts index 217550f17..4b26f3804 100644 --- a/lib/core/decision_service/index.ts +++ b/lib/core/decision_service/index.ts @@ -31,7 +31,6 @@ import { getVariationKeyFromId, isActive, ProjectConfig, - getHoldoutsForFlag, } from '../../project_config/project_config'; import { AudienceEvaluator, createAudienceEvaluator } from '../audience_evaluator'; import * as stringValidator from '../../utils/string_value_validator'; @@ -943,7 +942,10 @@ export class DecisionService { reasons: decideReasons, }); } - const holdouts = getHoldoutsForFlag(configObj, feature.key); + + // all global holouts should be evaluated for all flags + // global holdouts are available in configObj.holdouts + const { holdouts } = configObj; for (const holdout of holdouts) { const holdoutDecision = this.getVariationForHoldout(configObj, holdout, user); diff --git a/lib/project_config/project_config.spec.ts b/lib/project_config/project_config.spec.ts index 5cc1b6cba..13c086539 100644 --- a/lib/project_config/project_config.spec.ts +++ b/lib/project_config/project_config.spec.ts @@ -30,7 +30,7 @@ import testDatafile from '../tests/test_data'; import configValidator from '../utils/config_validator'; import { FEATURE_VARIABLE_TYPES } from '../utils/enums'; import { keyBy, sprintf } from '../utils/fns'; -import projectConfig, { ProjectConfig, getHoldoutsForFlag } from './project_config'; +import projectConfig, { ProjectConfig } from './project_config'; const cloneDeep = (obj: any) => JSON.parse(JSON.stringify(obj)); const logger = getMockLogger(); @@ -329,7 +329,7 @@ const getHoldoutDatafile = () => { key: 'holdout_2', status: 'Running', includedFlags: [], - excludedFlags: ['44829230000'], + excludedFlags: [], audienceIds: [], audienceConditions: [], variations: [ @@ -350,7 +350,7 @@ const getHoldoutDatafile = () => { id: 'holdout_id_3', key: 'holdout_3', status: 'Draft', - includedFlags: ['4482920077'], + includedFlags: [], excludedFlags: [], audienceIds: [], audienceConditions: [], @@ -392,22 +392,6 @@ describe('createProjectConfig - holdouts', () => { holdout_id_2: configObj.holdouts[1], holdout_id_3: configObj.holdouts[2], }); - - expect(configObj.globalHoldouts).toHaveLength(2); - expect(configObj.globalHoldouts).toEqual([ - configObj.holdouts[0], // holdout_1 has empty includedFlags - configObj.holdouts[1] // holdout_2 has empty includedFlags - ]); - - expect(configObj.includedHoldouts).toEqual({ - feature_1: [configObj.holdouts[2]], // holdout_3 includes feature_1 (ID: 4482920077) - }); - - expect(configObj.excludedHoldouts).toEqual({ - feature_3: [configObj.holdouts[1]] // holdout_2 excludes feature_3 (ID: 44829230000) - }); - - expect(configObj.flagHoldoutsMap).toEqual({}); }); it('should handle empty holdouts array', function() { @@ -417,10 +401,6 @@ describe('createProjectConfig - holdouts', () => { expect(configObj.holdouts).toEqual([]); expect(configObj.holdoutIdMap).toEqual({}); - expect(configObj.globalHoldouts).toEqual([]); - expect(configObj.includedHoldouts).toEqual({}); - expect(configObj.excludedHoldouts).toEqual({}); - expect(configObj.flagHoldoutsMap).toEqual({}); }); it('should handle undefined includedFlags and excludedFlags in holdout', function() { @@ -436,34 +416,6 @@ describe('createProjectConfig - holdouts', () => { }); }); -describe('getHoldoutsForFlag', () => { - it('should return all applicable holdouts for a flag', () => { - const datafile = getHoldoutDatafile(); - const configObj = projectConfig.createProjectConfig(JSON.parse(JSON.stringify(datafile))); - - const feature1Holdouts = getHoldoutsForFlag(configObj, 'feature_1'); - expect(feature1Holdouts).toHaveLength(3); - expect(feature1Holdouts).toEqual([ - configObj.holdouts[0], - configObj.holdouts[1], - configObj.holdouts[2], - ]); - - const feature2Holdouts = getHoldoutsForFlag(configObj, 'feature_2'); - expect(feature2Holdouts).toHaveLength(2); - expect(feature2Holdouts).toEqual([ - configObj.holdouts[0], - configObj.holdouts[1], - ]); - - const feature3Holdouts = getHoldoutsForFlag(configObj, 'feature_3'); - expect(feature3Holdouts).toHaveLength(1); - expect(feature3Holdouts).toEqual([ - configObj.holdouts[0], - ]); - }); -}); - describe('getExperimentId', () => { let testData: Record; let configObj: ProjectConfig; diff --git a/lib/project_config/project_config.ts b/lib/project_config/project_config.ts index a3a72caf0..9f966549b 100644 --- a/lib/project_config/project_config.ts +++ b/lib/project_config/project_config.ts @@ -113,10 +113,6 @@ export interface ProjectConfig { odpIntegrationConfig: OdpIntegrationConfig; holdouts: Holdout[]; holdoutIdMap?: { [id: string]: Holdout }; - globalHoldouts: Holdout[]; - includedHoldouts: { [key: string]: Holdout[]; } - excludedHoldouts: { [key: string]: Holdout[]; } - flagHoldoutsMap: { [key: string]: Holdout[]; } } const EXPERIMENT_RUNNING_STATUS = 'Running'; @@ -390,70 +386,19 @@ const getEveryoneElseVariation = function( const parseHoldoutsConfig = (projectConfig: ProjectConfig): void => { projectConfig.holdouts = projectConfig.holdouts || []; projectConfig.holdoutIdMap = keyBy(projectConfig.holdouts, 'id'); - projectConfig.globalHoldouts = []; - projectConfig.includedHoldouts = {}; - projectConfig.excludedHoldouts = {}; - projectConfig.flagHoldoutsMap = {}; - - const featureFlagIdMap = keyBy(projectConfig.featureFlags, 'id'); projectConfig.holdouts.forEach((holdout) => { - if (!holdout.includedFlags) { - holdout.includedFlags = []; - } - - if (!holdout.excludedFlags) { - holdout.excludedFlags = []; - } + // Original design of holdouts made use of the includeFlags and excludeFlags fields to identify local holdouts. + // But this was never released. In the current design, these fields are no longer used. These fields are kept + // and assigned empty array to keep the published type `Holdout` unchanged. + holdout.includedFlags = []; + holdout.excludedFlags = []; holdout.variationKeyMap = keyBy(holdout.variations, 'key'); - assignBy(holdout.variations, 'id', projectConfig.variationIdMap); - - if (holdout.includedFlags.length === 0) { - projectConfig.globalHoldouts.push(holdout); - - holdout.excludedFlags.forEach((flagId: string) => { - const flag = featureFlagIdMap[flagId]; - if (flag) { - const flagKey = flag.key; - if (!projectConfig.excludedHoldouts[flagKey]) { - projectConfig.excludedHoldouts[flagKey] = []; - } - projectConfig.excludedHoldouts[flagKey].push(holdout); - } - }); - } else { - holdout.includedFlags.forEach((flagId: string) => { - const flag = featureFlagIdMap[flagId]; - if (flag) { - const flagKey = flag.key; - if (!projectConfig.includedHoldouts[flagKey]) { - projectConfig.includedHoldouts[flagKey] = []; - } - projectConfig.includedHoldouts[flagKey].push(holdout); - } - }) - } }); } -export const getHoldoutsForFlag = (projectConfig: ProjectConfig, flagKey: string): Holdout[] => { - if (projectConfig.flagHoldoutsMap[flagKey]) { - return projectConfig.flagHoldoutsMap[flagKey]; - } - - const flagHoldouts: Holdout[] = [ - ...projectConfig.globalHoldouts.filter((holdout) => { - return !(projectConfig.excludedHoldouts[flagKey] || []).includes(holdout); - }), - ...(projectConfig.includedHoldouts[flagKey] || []), - ]; - - projectConfig.flagHoldoutsMap[flagKey] = flagHoldouts; - return flagHoldouts; -} - /** * Extract all audience segments used in this audience's conditions * @param {Audience} audience Object representing the audience being parsed diff --git a/lib/shared_types.ts b/lib/shared_types.ts index 4d39a317d..5fc10498b 100644 --- a/lib/shared_types.ts +++ b/lib/shared_types.ts @@ -179,12 +179,8 @@ export interface Holdout extends ExperimentCore { } export function isHoldout(obj: Experiment | Holdout): obj is Holdout { - // Holdout has 'status', 'includedFlags', and 'excludedFlags' properties - return ( - (obj as Holdout).status !== undefined && - Array.isArray((obj as Holdout).includedFlags) && - Array.isArray((obj as Holdout).excludedFlags) - ); + // Holdout doesn't have 'layerId' property, while Experiment does + return (obj as Experiment).layerId === undefined; } export enum VariableType {