Skip to content

fix: Checking variation in flagvariation map instead of checking it only in experiment #729

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Dec 21, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions packages/optimizely-sdk/lib/core/decision_service/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import {
getExperimentAudienceConditions,
getExperimentFromId,
getExperimentFromKey,
getFlagVariationByKey,
getTrafficAllocation,
getVariationIdFromExperimentAndVariationKey,
getVariationFromId,
Expand Down Expand Up @@ -614,7 +615,11 @@ export class DecisionService {
decideReasons.push(...decisionVariation.reasons);
variationKey = decisionVariation.result;
if (variationKey) {
const variation = experiment.variationKeyMap[variationKey];
let variation = null;
variation = experiment.variationKeyMap[variationKey];
Comment on lines +618 to +619
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merge these

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add unit test to catch the bug?

if (!variation) {
variation = getFlagVariationByKey(configObj, feature.key, variationKey);
}
Comment on lines +620 to +622
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as i understand, this was the only change needed here. i am unable to understand why were other changes made at all.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Secondly, optimizely_instance's access is not available in decide_service, that's why moved to project_config

variationForFeatureExperiment = {
experiment: experiment,
variation: variation,
Expand Down Expand Up @@ -1016,7 +1021,7 @@ export class DecisionService {
const decideReasons: (string | number)[][] = [];

// check forced decision first
const forcedDecisionResponse = user.findValidatedForcedDecision(flagKey, rule.key);
const forcedDecisionResponse = user.findValidatedForcedDecision(configObj, flagKey, rule.key);
decideReasons.push(...forcedDecisionResponse.reasons);

const forcedVariaton = forcedDecisionResponse.result;
Expand Down Expand Up @@ -1048,7 +1053,7 @@ export class DecisionService {

// check forced decision first
const rule = rules[ruleIndex];
const forcedDecisionResponse = user.findValidatedForcedDecision(flagKey, rule.key);
const forcedDecisionResponse = user.findValidatedForcedDecision(configObj, flagKey, rule.key);
decideReasons.push(...forcedDecisionResponse.reasons);

const forcedVariaton = forcedDecisionResponse.result;
Expand Down
21 changes: 21 additions & 0 deletions packages/optimizely-sdk/lib/core/project_config/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -508,6 +508,26 @@ export const getExperimentFromId = function(
return null;
};

/**
* Returns flag variation for specified flagKey and variationKey
* @param {flagKey} string
* @param {variationKey} string
* @return {Variation|null}
*/
export const getFlagVariationByKey = function(projectConfig: ProjectConfig, flagKey: string, variationKey: string): Variation | null {
if (!projectConfig) {
return null;
}

const variations = projectConfig.flagVariationsMap[flagKey];
const result = find(variations, item => item.key === variationKey)
if (result) {
return result;
}

return null;
};

/**
* Get feature from provided feature key. Log an error if no feature exists in
* the project config with the given key.
Expand Down Expand Up @@ -817,6 +837,7 @@ export default {
getExperimentFromKey,
getTrafficAllocation,
getExperimentFromId,
getFlagVariationByKey,
getFeatureFromKey,
getVariableForFeature,
getVariableValueForVariation,
Expand Down
22 changes: 1 addition & 21 deletions packages/optimizely-sdk/lib/optimizely/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1481,7 +1481,7 @@ export default class Optimizely {

const allDecideOptions = this.getAllDecideOptions(options);

const forcedDecisionResponse = user.findValidatedForcedDecision(key);
const forcedDecisionResponse = user.findValidatedForcedDecision(configObj, key);
reasons.push(...forcedDecisionResponse.reasons);
const variation = forcedDecisionResponse.result;
if (variation) {
Expand Down Expand Up @@ -1672,24 +1672,4 @@ export default class Optimizely {
return this.decideForKeys(user, allFlagKeys, options);
}

/**
* Returns flag variation for specified flagKey and variationKey
* @param {flagKey} string
* @param {variationKey} string
* @return {OptimizelyVariation|null}
*/
getFlagVariationByKey(flagKey: string, variationKey: string): OptimizelyVariation | null {
const configObj = this.projectConfigManager.getConfig();
if (!configObj) {
return null;
}

const variations = configObj.flagVariationsMap[flagKey];
const result = find(variations, item => item.key === variationKey)
if (result) {
return result;
}

return null;
}
Comment on lines -1681 to -1694
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why remove this and move to project_config and pass that in separately?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per my understanding, project_config is used for mapping all datafile properties and their k/v mappings. Optimizely class is not for this purpose. I have not seen any single mapping in this class. That's why removed it and add it at proper location where you can see all other getter and setters.

}
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ describe('lib/optimizely_user_context', function() {
);
assert.deepEqual(decision, fakeDecision);
});
});
});

describe('#decideForKeys', function() {
it('should return an expected decision results object', function() {
Expand Down Expand Up @@ -381,6 +381,23 @@ describe('lib/optimizely_user_context', function() {
optlyInstance.notificationCenter.sendNotifications.restore();
});

it('should return an expected decision object when forced decision is called and variation of different experiment but same flag key', function() {
var flagKey = 'feature_1';
var ruleKey = 'exp_with_audience';
var variationKey = '3324490633';

var user = optlyInstance.createUserContext(userId);
user.setForcedDecision({ flagKey: flagKey, ruleKey }, { variationKey });
var decision = user.decide(flagKey, options);

assert.equal(decision.variationKey, variationKey);
assert.equal(decision.ruleKey, ruleKey);
assert.equal(decision.enabled, true);
assert.equal(decision.userContext.getUserId(), userId);
assert.deepEqual(decision.userContext.getAttributes(), {});
assert.deepEqual(Object.keys(decision.userContext.forcedDecisionsMap).length, 1);
});

it('should return forced decision object when forced decision is set for a flag and do NOT dispatch an event with DISABLE_DECISION_EVENT passed in decide options', function() {
var user = optlyInstance.createUserContext(userId);
var featureKey = 'feature_1';
Expand Down
12 changes: 9 additions & 3 deletions packages/optimizely-sdk/lib/optimizely_user_context/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,10 @@ import {
UserAttributes,
Variation
} from '../../lib/shared_types';
import {
getFlagVariationByKey,
ProjectConfig,
} from '../core/project_config';
import { LOG_MESSAGES, CONTROL_ATTRIBUTES } from '../utils/enums';

const logger = getLogger();
Expand Down Expand Up @@ -211,22 +215,24 @@ export default class OptimizelyUserContext {

/**
* Finds a validated forced decision for specific flagKey and optional ruleKey.
* @param {ProjectConfig}config A projectConfig.
* @param {string} flagKey A flagKey.
* @param {ruleKey} ruleKey A ruleKey (optional).
* @return {DecisionResponse<Variation|null>} DecisionResponse object containing valid variation object and decide reasons.
*/
findValidatedForcedDecision(
config: ProjectConfig,
Copy link
Contributor

@zashraf1985 zashraf1985 Dec 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like an unusual pattern. why do we need to pass in config from outside when we have access to optimizely instance.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

optimizelyInstance.config may have different version if we try to access it from the optimizely instance. That's why the config we capture in the start of any API, we use it across all methods.

flagKey: string,
ruleKey?: string
ruleKey?: string,
): DecisionResponse<Variation | null> {

const decideReasons: (string | number)[][] = [];
const forcedDecision = this.findForcedDecision({ flagKey, ruleKey });
let variation = null;
let variationKey;
if (forcedDecision) {
if (config && forcedDecision) {
variationKey = forcedDecision.variationKey;
variation = this.optimizely.getFlagVariationByKey(flagKey, variationKey);
variation = getFlagVariationByKey(config, flagKey, variationKey);
Copy link
Contributor

@zashraf1985 zashraf1985 Dec 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did this had to change? this looks like an unusual pattern to pass in config from outside.

if (variation) {
if (ruleKey) {
logger.info(
Expand Down
5 changes: 4 additions & 1 deletion packages/optimizely-sdk/lib/shared_types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ import { EventProcessor } from '@optimizely/js-sdk-event-processor';

import { NotificationCenter } from '@optimizely/js-sdk-utils';

import { ProjectConfig } from './core/project_config';

export interface BucketerParams {
experimentId: string;
experimentKey: string;
Expand Down Expand Up @@ -381,8 +383,9 @@ export interface OptimizelyUserContext {
): { [key: string]: OptimizelyDecision };
trackEvent(eventName: string, eventTags?: EventTags): void;
findValidatedForcedDecision(
config: ProjectConfig,
flagKey: string,
ruleKey?: string,
ruleKey?: string
): DecisionResponse<Variation | null>;
setForcedDecision(context: OptimizelyDecisionContext, decision: OptimizelyForcedDecision): boolean;
getForcedDecision(context: OptimizelyDecisionContext): OptimizelyForcedDecision | null;
Expand Down