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

mfahadahmed · 2019-03-08T14:23:36Z

Summary

Implemented isFeatureEnabled in activate and getEnabledFeatures APIs.

Test plan

Added unit tests.

…getEnabledFeatures APIs.

coveralls · 2019-03-08T14:35:35Z

Coverage increased (+0.03%) to 97.758% when pulling 6365632 on fahad/on-decision-listener-feature-enabled into b9a9fd0 on master.

aliabbasrizvi

Looks good. Minor feedback. Please address and this will be good to merge.

aliabbasrizvi · 2019-03-11T17:32:51Z

packages/optimizely-sdk/lib/utils/enums/index.js

 */
 exports.NOTIFICATION_TYPES = {
  ACTIVATE: 'ACTIVATE:experiment, user_id,attributes, variation, event',
  TRACK: 'TRACK:event_key, user_id, attributes, event_tags, event',
+  ON_DECISION: 'ON_DECISION:type, userId, attributes, decisionInfo',


nit. It is DECISION and not ON_DECISION.

aliabbasrizvi · 2019-03-11T17:44:00Z

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

+      this.logger.log(LOG_LEVEL.INFO, sprintf(LOG_MESSAGES.FEATURE_NOT_ENABLED_FOR_USER, MODULE_NAME, featureKey, userId));
+      featureEnabled = false;
+    } else {
+      this.logger.log(LOG_LEVEL.INFO, sprintf(LOG_MESSAGES.FEATURE_ENABLED_FOR_USER, MODULE_NAME, featureKey, userId));


Should there be featureEnabled = true here?

The possible values of variation.featureEnabled are true, false or undefined. For falsy values, we explicitly set it to false. Otherwise the value is already true.

I have changed this condition to explicitly check for true.

aliabbasrizvi · 2019-03-11T17:52:19Z

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

@@ -2519,6 +2521,199 @@ describe('lib/optimizely', function() {
        };
        sinon.assert.calledWith(trackListener, expectedArgument);
      });
+
+      describe('OnDecision Listener', function() {


nit Decision Listener

jordangarcia

Make attributes default to {} if not provided

jordangarcia · 2019-03-11T21:17:03Z

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

+      {
+        type: DECISION_INFO_TYPES.FEATURE,
+        userId: userId,
+        attributes: attributes,


make sure attributes defaults to {} if not defined.

Good catch.

aliabbasrizvi · 2019-03-12T17:43:25Z

@jordangarcia can you take a look again?
LGTM after the changes.

aliabbasrizvi · 2019-03-12T19:24:48Z

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

    if (!!variation) {
+      featureEnabled = variation.featureEnabled;
      if (decision.decisionSource === DECISION_SOURCES.EXPERIMENT) {


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,

Capitalized decision source values.

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)

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

aliabbasrizvi · 2019-03-13T11:14:17Z

@jordangarcia can you re-review

jordangarcia · 2019-03-13T15:10:59Z

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

+      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;


shouldn't this value already be false?

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() {

jordangarcia · 2019-03-13T15:11:28Z

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

+      featureEnabled = false;
+    }
+
+    var decisionSource = decision.decisionSource || DECISION_SOURCES.ROLLOUT;


In what cases is the decision.decisionSource not a truthy value?

When the user was not bucketed into an experiment or a rollout, DecisionSource returned as null from decision service. But as per the comments in feature variables PR #246 this implementation has been changed. Updating it accordingly.

jordangarcia · 2019-03-13T15:15:28Z

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

+        optlyInstance.notificationCenter.addNotificationListener(enums.NOTIFICATION_TYPES.DECISION, decisionListener);
+        var result = optlyInstance.getEnabledFeatures('test_user', attributes);
+        assert.strictEqual(result.length, 3);
+        assert.isAbove(result.indexOf('test_feature_2'), -1);


Use

assert.deepEqual(result, ['test_feature2', 'test_feature_for_experiment', 'shared_feature'])

jordangarcia · 2019-03-13T15:18:33Z

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

+        assert.isAbove(result.indexOf('test_feature_for_experiment'), -1);
+        assert.isAbove(result.indexOf('shared_feature'), -1);
+
+        assert.isTrue(decisionListener.getCall(0).calledWith({


Use sinon assertion here

sinon.assert.calledWithExactly(spy.getCall(1), arg1, arg2, ...);

This will give a better error message than assert.isTrue

jordangarcia

Just a few minor changes to the test, and a couple questions. Please answer and I will re-review

…if the user is not evaluated into the experiment.

…istener-feature-enabled # Conflicts: # packages/optimizely-sdk/lib/optimizely/index.js # packages/optimizely-sdk/lib/optimizely/index.tests.js # packages/optimizely-sdk/lib/utils/enums/index.js

jordangarcia · 2019-03-19T22:45:26Z

@ mfahadahmed this needs to be rebased before it can be merged.

…istener-feature-enabled # Conflicts: # packages/optimizely-sdk/lib/optimizely/index.js

This reverts commit 9c3ec8a.

feat(api): OnDecision Notification Listener for isFeatureEnabled and …

32b9719

…getEnabledFeatures APIs.

mfahadahmed mentioned this pull request Mar 8, 2019

feat(api): OnDecision Notification Listener #240

Closed

mfahadahmed requested a review from a team March 8, 2019 15:39

mfahadahmed added 2 commits March 11, 2019 22:18

Move decision listener tests inside notification listeners tests.

1b6e3be

Remove unused variables.

3b2b61b

aliabbasrizvi approved these changes Mar 11, 2019

View reviewed changes

msohailhussain requested a review from a team March 11, 2019 21:14

jordangarcia suggested changes Mar 11, 2019

View reviewed changes

mfahadahmed added 3 commits March 12, 2019 17:32

Address code review comments.

26aef1a

Explicitly check for featureEnabled is true.

c30da19

Remove NOTIFICAITON_TYPES variable.

b139eec

aliabbasrizvi changed the title ~~feat(api): OnDecision Notification Listener for isFeatureEnabled and getEnabledFeatures APIs.~~ feat(api): Decision Notification Listener for isFeatureEnabled and getEnabledFeatures APIs. Mar 12, 2019

Merge branch 'master' into fahad/on-decision-listener-feature-enabled

e7e5252

aliabbasrizvi requested a review from jordangarcia March 12, 2019 17:45

aliabbasrizvi reviewed Mar 12, 2019

View reviewed changes

msohailhussain requested a review from a team March 12, 2019 19:47

Capitalize decision source values.

126ce3b

aliabbasrizvi approved these changes Mar 13, 2019

View reviewed changes

jordangarcia reviewed Mar 13, 2019

View reviewed changes

jordangarcia suggested changes Mar 13, 2019

View reviewed changes

mfahadahmed added 2 commits March 14, 2019 17:30

getVariationForFeature method now returns decision source as rollout …

04376c8

…if the user is not evaluated into the experiment.

Merge remote-tracking branch 'origin/master' into fahad/on-decision-l…

7e23dce

…istener-feature-enabled # Conflicts: # packages/optimizely-sdk/lib/optimizely/index.js # packages/optimizely-sdk/lib/optimizely/index.tests.js # packages/optimizely-sdk/lib/utils/enums/index.js

Merge remote-tracking branch 'origin/master' into fahad/on-decision-l…

d8b6504

…istener-feature-enabled # Conflicts: # packages/optimizely-sdk/lib/optimizely/index.js

Fixed case issue

9c3ec8a

jordangarcia approved these changes Mar 22, 2019

View reviewed changes

Revert "Fixed case issue"

6365632

This reverts commit 9c3ec8a.

mfahadahmed mentioned this pull request Mar 25, 2019

feat(api): Decision Notification Listener for feature variable APIs. #246

Merged

aliabbasrizvi merged commit 09d9433 into master Mar 26, 2019

aliabbasrizvi deleted the fahad/on-decision-listener-feature-enabled branch March 26, 2019 22:15

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

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

Uh oh!

Conversation

mfahadahmed commented Mar 8, 2019

Summary

Test plan

Uh oh!

coveralls commented Mar 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aliabbasrizvi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jordangarcia left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aliabbasrizvi commented Mar 12, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aliabbasrizvi commented Mar 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jordangarcia left a comment

Choose a reason for hiding this comment

Uh oh!

jordangarcia commented Mar 19, 2019

Uh oh!

Uh oh!

coveralls commented Mar 8, 2019 •

edited

Loading