Skip to content

fix: Change experimentKey and variationKey to be null in rollout in decisionInfo payload #644

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.

Sign up for GitHub

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 2 commits into from
Jan 23, 2021
+59 −59
Merged

fix: Change experimentKey and variationKey to be null in rollout in decisionInfo payload #644

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
da09898
Change experimentKey and variationKey from empty string to null in ro…
yavorona Jan 15, 2021
89bc241
Change OptimizelyDecideOptions[] to OptimizelyDecideOption[] to be co…
yavorona Jan 21, 2021
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
4 changes: 2 additions & 2 deletions packages/optimizely-sdk/lib/core/decision_service/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import * as enums from '../../utils/enums';
import projectConfig from '../project_config';
import AudienceEvaluator from '../audience_evaluator';
import * as stringValidator from '../../utils/string_value_validator';
import { OptimizelyDecideOptions } from '../../shared_types';
import { OptimizelyDecideOption } from '../../shared_types';

var MODULE_NAME = 'DECISION_SERVICE';
var ERROR_MESSAGES = enums.ERROR_MESSAGES;
Expand Down Expand Up @@ -100,7 +100,7 @@ DecisionService.prototype.getVariation = function(configObj, experimentKey, user
};
}

var shouldIgnoreUPS = options[OptimizelyDecideOptions.IGNORE_USER_PROFILE_SERVICE];
var shouldIgnoreUPS = options[OptimizelyDecideOption.IGNORE_USER_PROFILE_SERVICE];

// check for sticky bucketing if decide options do not include shouldIgnoreUPS
if (!shouldIgnoreUPS) {
Expand Down
6 changes: 3 additions & 3 deletions packages/optimizely-sdk/lib/index.browser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import * as enums from './utils/enums';
import loggerPlugin from './plugins/logger';
import Optimizely from './optimizely';
import eventProcessorConfigValidator from './utils/event_processor_config_validator';
import { SDKOptions, OptimizelyDecideOptions } from './shared_types';
import { SDKOptions, OptimizelyDecideOption } from './shared_types';

const logger = getLogger();
setLogHandler(loggerPlugin.createLogger());
Expand Down Expand Up @@ -153,7 +153,7 @@ export {
setLogLevel,
createInstance,
__internalResetRetryState,
OptimizelyDecideOptions,
OptimizelyDecideOption,
};

export default {
Expand All @@ -165,5 +165,5 @@ export default {
setLogLevel,
createInstance,
__internalResetRetryState,
OptimizelyDecideOptions,
OptimizelyDecideOption,
};
2 changes: 1 addition & 1 deletion packages/optimizely-sdk/lib/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ declare module '@optimizely/optimizely-sdk' {

export type OptimizelyDecision = import('./optimizely_decision').OptimizelyDecision;

export enum OptimizelyDecideOptions {
export enum OptimizelyDecideOption {
DISABLE_DECISION_EVENT = 'DISABLE_DECISION_EVENT',
ENABLED_FLAGS_ONLY = 'ENABLED_FLAGS_ONLY',
IGNORE_USER_PROFILE_SERVICE = 'IGNORE_USER_PROFILE_SERVICE',
Expand Down
6 changes: 3 additions & 3 deletions packages/optimizely-sdk/lib/index.node.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import configValidator from './utils/config_validator';
import defaultErrorHandler from './plugins/error_handler';
import defaultEventDispatcher from './plugins/event_dispatcher/index.node';
import eventProcessorConfigValidator from './utils/event_processor_config_validator';
import { SDKOptions, OptimizelyDecideOptions } from './shared_types';
import { SDKOptions, OptimizelyDecideOption } from './shared_types';

const logger = getLogger();
setLogLevel(LogLevel.ERROR);
Expand Down Expand Up @@ -116,7 +116,7 @@ export {
setLogHandler as setLogger,
setLogLevel,
createInstance,
OptimizelyDecideOptions,
OptimizelyDecideOption,
};

export default {
Expand All @@ -127,5 +127,5 @@ export default {
setLogger: setLogHandler,
setLogLevel,
createInstance,
OptimizelyDecideOptions,
OptimizelyDecideOption,
};
6 changes: 3 additions & 3 deletions packages/optimizely-sdk/lib/index.react_native.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import defaultErrorHandler from './plugins/error_handler';
import loggerPlugin from './plugins/logger/index.react_native';
import defaultEventDispatcher from './plugins/event_dispatcher/index.browser';
import eventProcessorConfigValidator from './utils/event_processor_config_validator';
import { SDKOptions, OptimizelyDecideOptions } from './shared_types';
import { SDKOptions, OptimizelyDecideOption } from './shared_types';

const logger = getLogger();
setLogHandler(loggerPlugin.createLogger());
Expand Down Expand Up @@ -112,7 +112,7 @@ export {
setLogHandler as setLogger,
setLogLevel,
createInstance,
OptimizelyDecideOptions,
OptimizelyDecideOption,
};

export default {
Expand All @@ -123,5 +123,5 @@ export default {
setLogger: setLogHandler,
setLogLevel,
createInstance,
OptimizelyDecideOptions,
OptimizelyDecideOption,
};
40 changes: 20 additions & 20 deletions packages/optimizely-sdk/lib/optimizely/index.tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import * as logging from '@optimizely/js-sdk-logging';

import Optimizely from './';
import OptimizelyUserContext from '../optimizely_user_context';
import { OptimizelyDecideOptions } from '../shared_types';
import { OptimizelyDecideOption } from '../shared_types';
import AudienceEvaluator from '../core/audience_evaluator';
import bluebird from 'bluebird';
import bucketer from '../core/bucketer';
Expand Down Expand Up @@ -4707,7 +4707,7 @@ describe('lib/optimizely', function() {
optimizely: optlyInstance,
userId,
});
var decision = optlyInstance.decide(user, flagKey, [ OptimizelyDecideOptions.DISABLE_DECISION_EVENT ]);
var decision = optlyInstance.decide(user, flagKey, [ OptimizelyDecideOption.DISABLE_DECISION_EVENT ]);
var expectedDecision = {
variationKey: 'variation_with_traffic',
enabled: true,
Expand Down Expand Up @@ -4747,7 +4747,7 @@ describe('lib/optimizely', function() {
optimizely: optlyInstance,
userId,
});
var decision = optlyInstance.decide(user, flagKey, [ OptimizelyDecideOptions.DISABLE_DECISION_EVENT, OptimizelyDecideOptions.EXCLUDE_VARIABLES ]);
var decision = optlyInstance.decide(user, flagKey, [ OptimizelyDecideOption.DISABLE_DECISION_EVENT, OptimizelyDecideOption.EXCLUDE_VARIABLES ]);
var expectedDecision = {
variationKey: 'variation_with_traffic',
enabled: true,
Expand Down Expand Up @@ -4875,10 +4875,10 @@ describe('lib/optimizely', function() {
});
var decision = optlyInstance.decide(user, flagKey);
var expectedDecision = {
variationKey: '',
variationKey: null,
enabled: false,
variables: expectedVariables,
ruleKey: '',
ruleKey: null,
flagKey: flagKey,
userContext: user,
reasons: [],
Expand All @@ -4896,8 +4896,8 @@ describe('lib/optimizely', function() {
decisionInfo: {
flagKey: 'feature_3',
enabled: false,
ruleKey: '',
variationKey: '',
ruleKey: null,
variationKey: null,
variables: expectedVariables,
decisionEventDispatched: true,
reasons: [],
Expand All @@ -4919,7 +4919,7 @@ describe('lib/optimizely', function() {
logger: createdLogger,
isValidInstance: true,
eventBatchSize: 1,
defaultDecideOptions: [ OptimizelyDecideOptions.EXCLUDE_VARIABLES ],
defaultDecideOptions: [ OptimizelyDecideOption.EXCLUDE_VARIABLES ],
});

sinon.stub(optlyInstance.notificationCenter, 'sendNotifications');
Expand Down Expand Up @@ -4981,7 +4981,7 @@ describe('lib/optimizely', function() {
optimizely: optlyInstance,
userId
});
var decision = optlyInstance.decide(user, flagKey, [ OptimizelyDecideOptions.DISABLE_DECISION_EVENT ]);
var decision = optlyInstance.decide(user, flagKey, [ OptimizelyDecideOption.DISABLE_DECISION_EVENT ]);
var expectedDecisionObj = {
variationKey: 'variation_with_traffic',
enabled: true,
Expand Down Expand Up @@ -5027,7 +5027,7 @@ describe('lib/optimizely', function() {
logger: createdLogger,
isValidInstance: true,
eventBatchSize: 1,
defaultDecideOptions: [ OptimizelyDecideOptions.DISABLE_DECISION_EVENT ],
defaultDecideOptions: [ OptimizelyDecideOption.DISABLE_DECISION_EVENT ],
});

sinon.stub(optlyInstance.notificationCenter, 'sendNotifications');
Expand Down Expand Up @@ -5090,7 +5090,7 @@ describe('lib/optimizely', function() {
logger: createdLogger,
isValidInstance: true,
eventBatchSize: 1,
defaultDecideOptions: [ OptimizelyDecideOptions.INCLUDE_REASONS ],
defaultDecideOptions: [ OptimizelyDecideOption.INCLUDE_REASONS ],
});

sinon.stub(optlyInstance.notificationCenter, 'sendNotifications');
Expand Down Expand Up @@ -5145,7 +5145,7 @@ describe('lib/optimizely', function() {
logger: createdLogger,
isValidInstance: true,
eventBatchSize: 1,
defaultDecideOptions: [ OptimizelyDecideOptions.INCLUDE_REASONS ],
defaultDecideOptions: [ OptimizelyDecideOption.INCLUDE_REASONS ],
});
var user = new OptimizelyUserContext({
optimizely: optlyInstanceWithUserProfile,
Expand Down Expand Up @@ -5694,7 +5694,7 @@ describe('lib/optimizely', function() {
var decision1 = optlyInstanceWithUserProfile.decide(user, flagKey);
// should return variationId2 set by UPS
assert.equal(variationKey2, decision1.variationKey);
var decision2 = optlyInstanceWithUserProfile.decide(user, flagKey, [ OptimizelyDecideOptions.IGNORE_USER_PROFILE_SERVICE ]);
var decision2 = optlyInstanceWithUserProfile.decide(user, flagKey, [ OptimizelyDecideOption.IGNORE_USER_PROFILE_SERVICE ]);
// should ignore variationId2 set by UPS and return variationId1
assert.equal(variationKey1, decision2.variationKey);
// also should not save either
Expand Down Expand Up @@ -5728,7 +5728,7 @@ describe('lib/optimizely', function() {
logger: createdLogger,
isValidInstance: true,
eventBatchSize: 1,
defaultDecideOptions: [ OptimizelyDecideOptions.IGNORE_USER_PROFILE_SERVICE ]
defaultDecideOptions: [ OptimizelyDecideOption.IGNORE_USER_PROFILE_SERVICE ]
});
var user = new OptimizelyUserContext({
optimizely: optlyInstanceWithUserProfile,
Expand Down Expand Up @@ -5827,7 +5827,7 @@ describe('lib/optimizely', function() {
var flagKey2 = 'feature_3';
var user = optlyInstance.createUserContext(userId, { gender: 'female' });
var expectedVariables = optlyInstance.getAllFeatureVariables(flagKey1, userId);
var decisionsMap = optlyInstance.decideForKeys(user, [ flagKey1, flagKey2 ], [ OptimizelyDecideOptions.ENABLED_FLAGS_ONLY ]);
var decisionsMap = optlyInstance.decideForKeys(user, [ flagKey1, flagKey2 ], [ OptimizelyDecideOption.ENABLED_FLAGS_ONLY ]);
var decision = decisionsMap[flagKey1];
var expectedDecision = {
variationKey: 'variation_with_traffic',
Expand Down Expand Up @@ -5897,10 +5897,10 @@ describe('lib/optimizely', function() {
reasons: [],
}
var expectedDecision3 = {
variationKey: '',
variationKey: null,
enabled: false,
variables: expectedVariables3,
ruleKey: '',
ruleKey: null,
flagKey: allFlagKeysArray[2],
userContext: user,
reasons: [],
Expand All @@ -5918,7 +5918,7 @@ describe('lib/optimizely', function() {
var user = optlyInstance.createUserContext(userId, { gender: 'female' });
var expectedVariables1 = optlyInstance.getAllFeatureVariables(flagKey1, userId);
var expectedVariables2 = optlyInstance.getAllFeatureVariables(flagKey2, userId);
var decisionsMap = optlyInstance.decideAll(user, [ OptimizelyDecideOptions.ENABLED_FLAGS_ONLY ]);
var decisionsMap = optlyInstance.decideAll(user, [ OptimizelyDecideOption.ENABLED_FLAGS_ONLY ]);
var decision1 = decisionsMap[flagKey1];
var decision2 = decisionsMap[flagKey2];
var expectedDecision1 = {
Expand Down Expand Up @@ -5957,7 +5957,7 @@ describe('lib/optimizely', function() {
logger: createdLogger,
isValidInstance: true,
eventBatchSize: 1,
defaultDecideOptions: [ OptimizelyDecideOptions.ENABLED_FLAGS_ONLY ],
defaultDecideOptions: [ OptimizelyDecideOption.ENABLED_FLAGS_ONLY ],
});

sinon.stub(optlyInstance.notificationCenter, 'sendNotifications');
Expand Down Expand Up @@ -6004,7 +6004,7 @@ describe('lib/optimizely', function() {
var flagKey1 = 'feature_1';
var flagKey2 = 'feature_2';
var user = optlyInstance.createUserContext(userId, { gender: 'female' });
var decisionsMap = optlyInstance.decideAll(user, [ OptimizelyDecideOptions.EXCLUDE_VARIABLES ]);
var decisionsMap = optlyInstance.decideAll(user, [ OptimizelyDecideOption.EXCLUDE_VARIABLES ]);
var decision1 = decisionsMap[flagKey1];
var decision2 = decisionsMap[flagKey2];
var expectedDecision1 = {
Expand Down
36 changes: 18 additions & 18 deletions packages/optimizely-sdk/lib/optimizely/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import {
FeatureFlag,
FeatureVariable,
OptimizelyOptions,
OptimizelyDecideOptions
OptimizelyDecideOption
} from '../shared_types';
import { OptimizelyDecision, newErrorDecision } from '../optimizely_decision';
import OptimizelyUserContext from '../optimizely_user_context';
Expand Down Expand Up @@ -122,8 +122,8 @@ export default class Optimizely {

const defaultDecideOptions: { [key: string]: boolean } = {};
decideOptionsArray.forEach((option) => {
// Filter out all provided default decide options that are not in OptimizelyDecideOptions[]
if (OptimizelyDecideOptions[option]) {
// Filter out all provided default decide options that are not in OptimizelyDecideOption[]
if (OptimizelyDecideOption[option]) {
defaultDecideOptions[option] = true;
} else {
this.logger.log(
Expand Down Expand Up @@ -1466,7 +1466,7 @@ export default class Optimizely {
decide(
user: OptimizelyUserContext,
key: string,
options: OptimizelyDecideOptions[] = []
options: OptimizelyDecideOption[] = []
): OptimizelyDecision {
const configObj = this.projectConfigManager.getConfig();
const reasons: string[] = [];
Expand Down Expand Up @@ -1496,8 +1496,8 @@ export default class Optimizely {
reasons.push(...decisionVariation.reasons);
const decisionObj = decisionVariation.result;
const decisionSource = decisionObj.decisionSource;
const experimentKey = decision.getExperimentKey(decisionObj);
const variationKey = decision.getVariationKey(decisionObj);
Comment on lines -1499 to -1500
Copy link
Contributor

Choose a reason for hiding this comment

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

Are getExperimentKey & getVariationKey methods no longer used, so should delete them?

Copy link
Contributor Author

@yavorona yavorona Jan 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

These methods are being used in buildImpressionEvent and emitNotificationCenterActivate methods (we are still sending rule_key and variation_key as empty strings inside metadata object in this case). I am not sure if they should be changed to null. @jaeopt maybe you have any thought on that?

Copy link
Contributor

Choose a reason for hiding this comment

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

@yavorona I think we should keep those. Event and notification will have different requirements (empty string instead of null) than optimizelyDecision (null).

const experimentKey = decisionObj.experiment?.key ?? null;
const variationKey = decisionObj.variation?.key ?? null;
const flagEnabled: boolean = decision.getFeatureEnabledFromVariation(decisionObj);
if (flagEnabled === true) {
this.logger.log(
Expand All @@ -1514,7 +1514,7 @@ export default class Optimizely {
const variablesMap: { [key: string]: unknown } = {};
let decisionEventDispatched = false;

if (!allDecideOptions[OptimizelyDecideOptions.EXCLUDE_VARIABLES]) {
if (!allDecideOptions[OptimizelyDecideOption.EXCLUDE_VARIABLES]) {
feature.variables.forEach(variable => {
variablesMap[variable.key] =
this.getFeatureVariableValueFromVariation(
Expand All @@ -1528,7 +1528,7 @@ export default class Optimizely {
}

if (
!allDecideOptions[OptimizelyDecideOptions.DISABLE_DECISION_EVENT] && (
!allDecideOptions[OptimizelyDecideOption.DISABLE_DECISION_EVENT] && (
decisionSource === DECISION_SOURCES.FEATURE_TEST ||
decisionSource === DECISION_SOURCES.ROLLOUT && projectConfig.getSendFlagDecisionsValue(configObj))
) {
Expand All @@ -1542,7 +1542,7 @@ export default class Optimizely {
decisionEventDispatched = true;
}

const shouldIncludeReasons = allDecideOptions[OptimizelyDecideOptions.INCLUDE_REASONS];
const shouldIncludeReasons = allDecideOptions[OptimizelyDecideOption.INCLUDE_REASONS];
const reportedReasons = shouldIncludeReasons ? reasons: [];

const featureInfo = {
Expand Down Expand Up @@ -1575,17 +1575,17 @@ export default class Optimizely {

/**
* Get all decide options.
* @param {OptimizelyDecideOptions[]} options decide options
* @param {OptimizelyDecideOption[]} options decide options
* @return {[key: string]: boolean} Map of all provided decide options including default decide options
*/
private getAllDecideOptions(options: OptimizelyDecideOptions[]): { [key: string]: boolean } {
private getAllDecideOptions(options: OptimizelyDecideOption[]): { [key: string]: boolean } {
const allDecideOptions = {...this.defaultDecideOptions};
if (!Array.isArray(options)) {
this.logger.log(LOG_LEVEL.DEBUG, sprintf(LOG_MESSAGES.INVALID_DECIDE_OPTIONS, MODULE_NAME));
} else {
options.forEach((option) => {
// Filter out all provided decide options that are not in OptimizelyDecideOptions[]
if (OptimizelyDecideOptions[option]) {
// Filter out all provided decide options that are not in OptimizelyDecideOption[]
if (OptimizelyDecideOption[option]) {
allDecideOptions[option] = true;
} else {
this.logger.log(
Expand All @@ -1605,13 +1605,13 @@ export default class Optimizely {
* The SDK will always return an object of decisions. When it cannot process requests, it will return an empty object after logging the errors.
* @param {OptimizelyUserContext} user A user context associated with this OptimizelyClient
* @param {string[]} keys An array of flag keys for which decisions will be made.
* @param {OptimizelyDecideOptions[]} options An array of options for decision-making.
* @param {OptimizelyDecideOption[]} options An array of options for decision-making.
* @return {[key: string]: OptimizelyDecision} An object of decision results mapped by flag keys.
*/
decideForKeys(
user: OptimizelyUserContext,
keys: string[],
options: OptimizelyDecideOptions[] = []
options: OptimizelyDecideOption[] = []
): { [key: string]: OptimizelyDecision } {
const decisionMap: { [key: string]: OptimizelyDecision } = {};
if (!this.isValidInstance()) {
Expand All @@ -1625,7 +1625,7 @@ export default class Optimizely {
const allDecideOptions = this.getAllDecideOptions(options);
keys.forEach(key => {
const optimizelyDecision: OptimizelyDecision = this.decide(user, key, options);
if (!allDecideOptions[OptimizelyDecideOptions.ENABLED_FLAGS_ONLY] || optimizelyDecision.enabled) {
if (!allDecideOptions[OptimizelyDecideOption.ENABLED_FLAGS_ONLY] || optimizelyDecision.enabled) {
decisionMap[key] = optimizelyDecision;
}
});
Expand All @@ -1636,12 +1636,12 @@ export default class Optimizely {
/**
* Returns an object of decision results for all active flag keys.
* @param {OptimizelyUserContext} user A user context associated with this OptimizelyClient
* @param {OptimizelyDecideOptions[]} options An array of options for decision-making.
* @param {OptimizelyDecideOption[]} options An array of options for decision-making.
* @return {[key: string]: OptimizelyDecision} An object of all decision results mapped by flag keys.
*/
decideAll(
user: OptimizelyUserContext,
options: OptimizelyDecideOptions[] = []
options: OptimizelyDecideOption[] = []
): { [key: string]: OptimizelyDecision } {
const configObj = this.projectConfigManager.getConfig();
const decisionMap: { [key: string]: OptimizelyDecision } = {};
Expand Down
Loading