Skip to content

feat(api): Decision Notification Listener for isFeatureEnabled and getEnabledFeatures APIs. #245

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 13 commits into from
Mar 26, 2019
Merged

feat(api): Decision Notification Listener for isFeatureEnabled and getEnabledFeatures APIs. #245

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
32b9719
feat(api): OnDecision Notification Listener for isFeatureEnabled and …
mfahadahmed Mar 8, 2019
1b6e3be
Move decision listener tests inside notification listeners tests.
mfahadahmed Mar 11, 2019
3b2b61b
Remove unused variables.
mfahadahmed Mar 11, 2019
26aef1a
Address code review comments.
mfahadahmed Mar 12, 2019
c30da19
Explicitly check for featureEnabled is true.
mfahadahmed Mar 12, 2019
b139eec
Remove NOTIFICAITON_TYPES variable.
mfahadahmed Mar 12, 2019
e7e5252
Merge branch 'master' into fahad/on-decision-listener-feature-enabled
aliabbasrizvi Mar 12, 2019
126ce3b
Capitalize decision source values.
mfahadahmed Mar 13, 2019
04376c8
getVariationForFeature method now returns decision source as rollout …
mfahadahmed Mar 14, 2019
7e23dce
Merge remote-tracking branch 'origin/master' into fahad/on-decision-l…
mfahadahmed Mar 14, 2019
d8b6504
Merge remote-tracking branch 'origin/master' into fahad/on-decision-l…
mfahadahmed Mar 20, 2019
9c3ec8a
Fixed case issue
msohailhussain Mar 22, 2019
6365632
Revert "Fixed case issue"
msohailhussain Mar 22, 2019
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
7 changes: 1 addition & 6 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 @@ -303,12 +303,7 @@ DecisionService.prototype.getVariationForFeature = function(feature, userId, att
}

this.logger.log(LOG_LEVEL.DEBUG, sprintf(LOG_MESSAGES.USER_NOT_IN_ROLLOUT, MODULE_NAME, userId, feature.key));

return {
experiment: null,
variation: null,
decisionSource: null,
};
return rolloutDecision;
};

DecisionService.prototype._getVariationForFeatureExperiment = function(feature, userId, attributes) {
Expand Down
22 changes: 11 additions & 11 deletions packages/optimizely-sdk/lib/core/decision_service/index.tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -843,12 +843,12 @@ describe('lib/core/decision_service', function() {
getVariationStub.returns(null);
});

it('returns a decision with no variation', function() {
it('returns a decision with no variation and source rollout', function() {
var decision = decisionServiceInstance.getVariationForFeature(feature, 'user1');
var expectedDecision = {
experiment: null,
variation: null,
decisionSource: null,
decisionSource: DECISION_SOURCES.ROLLOUT,
};
assert.deepEqual(decision, expectedDecision);
sinon.assert.calledWithExactly(mockLogger.log, LOG_LEVEL.DEBUG, 'DECISION_SERVICE: User user1 is not in any experiment on the feature test_feature_for_experiment.');
Expand Down Expand Up @@ -915,12 +915,12 @@ describe('lib/core/decision_service', function() {
getVariationStub.returns(null);
});

it('returns a decision with no experiment and no variation', function() {
it('returns a decision with no experiment, no variation and source rollout', function() {
var decision = decisionServiceInstance.getVariationForFeature(feature, 'user1');
var expectedDecision = {
experiment: null,
variation: null,
decisionSource: null,
decisionSource: DECISION_SOURCES.ROLLOUT,
};
assert.deepEqual(decision, expectedDecision);
sinon.assert.calledWithExactly(mockLogger.log, LOG_LEVEL.DEBUG, 'DECISION_SERVICE: User user1 is not in any experiment on the feature feature_with_group.');
Expand All @@ -932,7 +932,7 @@ describe('lib/core/decision_service', function() {
var expectedDecision = {
experiment: null,
variation: null,
decisionSource: null,
decisionSource: DECISION_SOURCES.ROLLOUT,
};
assert.deepEqual(decision, expectedDecision);
sinon.assert.calledWithExactly(mockLogger.log, LOG_LEVEL.DEBUG, 'DECISION_SERVICE: User user1 is not in any experiment on the feature feature_exp_no_traffic.');
Expand All @@ -946,12 +946,12 @@ describe('lib/core/decision_service', function() {
bucketUserIntoExperimentStub.returns(null);
});

it('returns a decision with no experiment and no variation', function() {
it('returns a decision with no experiment, no variation and source rollout', function() {
var decision = decisionServiceInstance.getVariationForFeature(feature, 'user1');
var expectedDecision = {
experiment: null,
variation: null,
decisionSource: null,
decisionSource: DECISION_SOURCES.ROLLOUT,
};
assert.deepEqual(decision, expectedDecision);
sinon.assert.calledWithExactly(mockLogger.log, LOG_LEVEL.DEBUG, 'DECISION_SERVICE: User user1 is not in any experiment on the feature feature_with_group.');
Expand Down Expand Up @@ -1168,12 +1168,12 @@ describe('lib/core/decision_service', function() {
bucketStub.returns(null);
});

it('returns a decision with no variation and no experiment', function() {
it('returns a decision with no variation, no experiment and source rollout', function() {
var decision = decisionServiceInstance.getVariationForFeature(feature, 'user1');
var expectedDecision = {
experiment: null,
variation: null,
decisionSource: null,
decisionSource: DECISION_SOURCES.ROLLOUT,
};
assert.deepEqual(decision, expectedDecision);
sinon.assert.calledWithExactly(mockLogger.log, LOG_LEVEL.DEBUG, 'DECISION_SERVICE: User user1 does not meet conditions for targeting rule 1.');
Expand Down Expand Up @@ -1378,12 +1378,12 @@ describe('lib/core/decision_service', function() {
feature = configObj.featureKeyMap.unused_flag;
});

it('returns a decision with no variation and no experiment', function() {
it('returns a decision with no variation, no experiment and source rollout', function() {
var decision = decisionServiceInstance.getVariationForFeature(feature, 'user1');
var expectedDecision = {
experiment: null,
variation: null,
decisionSource: null,
decisionSource: DECISION_SOURCES.ROLLOUT,
};
var expectedDecision = assert.deepEqual(decision, expectedDecision);
sinon.assert.calledWithExactly(mockLogger.log, LOG_LEVEL.DEBUG, 'DECISION_SERVICE: Feature unused_flag is not attached to any experiments.');
Expand Down
42 changes: 31 additions & 11 deletions packages/optimizely-sdk/lib/optimizely/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -502,26 +502,46 @@ Optimizely.prototype.isFeatureEnabled = function(featureKey, userId, attributes)
return false;
}

var featureEnabled = false;
var experimentKey = null;
var variationKey = null;
var decision = this.decisionService.getVariationForFeature(feature, userId, attributes);
var variation = decision.variation;

if (!!variation) {
featureEnabled = variation.featureEnabled;
if (decision.decisionSource === DECISION_SOURCES.EXPERIMENT) {
Copy link
Contributor

Choose a reason for hiding this comment

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

DECISION_SOURCES.EXPERIMENT is experiment (lower case) and not EXPERIMENT (upper case). Similarly, DECISION_SOURCES.ROLLOUT is rollout (lower case) and not ROLLOUT (upper case). We should update that,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Capitalized decision source values.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused about this comment. @aliabbasrizvi are you saying that the values (not the enum key) should be lower case or upper case.

This PR is changing it from lowercase to uppercase.

It's unclear what we want the final state to be (uppercase or lowercase)

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, should have been clear. The value should be EXPERIMENT/ROLLOUT.

experimentKey = decision.experiment.key;
variationKey = decision.variation.key;
// got a variation from the exp, so we track the impression
this._sendImpressionEvent(decision.experiment.key, decision.variation.key, userId, attributes);
}
if (variation.featureEnabled === true) {
this.logger.log(
LOG_LEVEL.INFO,
sprintf(LOG_MESSAGES.FEATURE_ENABLED_FOR_USER, MODULE_NAME, featureKey, userId)
);
return true;
}
}
this.logger.log(
LOG_LEVEL.INFO,
sprintf(LOG_MESSAGES.FEATURE_NOT_ENABLED_FOR_USER, MODULE_NAME, featureKey, userId)

if (featureEnabled === true) {
this.logger.log(LOG_LEVEL.INFO, sprintf(LOG_MESSAGES.FEATURE_ENABLED_FOR_USER, MODULE_NAME, featureKey, userId));
} else {
this.logger.log(LOG_LEVEL.INFO, sprintf(LOG_MESSAGES.FEATURE_NOT_ENABLED_FOR_USER, MODULE_NAME, featureKey, userId));
featureEnabled = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this value already be false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it can be undefined when featureEnabled property is missing from the variation as specified in this test

javascript-sdk/packages/optimizely-sdk/lib/optimizely/index.tests.js

Line 2896 in 553e6ff

describe('when the variation is missing the toggle', function() {

}

this.notificationCenter.sendNotifications(
enums.NOTIFICATION_TYPES.DECISION,
{
type: DECISION_INFO_TYPES.FEATURE,
userId: userId,
attributes: attributes || {},
decisionInfo: {
featureKey: featureKey,
featureEnabled: featureEnabled,
source: decision.decisionSource,
sourceExperimentKey: experimentKey,
sourceVariationKey: variationKey,
}
}
);
return false;

return featureEnabled;
} catch (e) {
this.logger.log(LOG_LEVEL.ERROR, e.message);
this.errorHandler.handleError(e);
Expand Down
Loading