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

mfahadahmed · 2019-03-08T15:27:05Z

Summary

Implemented onDecisionListener in activate and getVariation APIs.

Test plan

Added unit tests.

coveralls · 2019-03-08T15:34:22Z

Coverage increased (+0.05%) to 97.792% when pulling 69fb7d6 on fahad/on-decision-listener-feature-variables into 9827911 on master.

…istener-feature-variables

aliabbasrizvi

LGTM

@mjc1283 or @jordangarcia can one of you take a look as well?

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

aliabbasrizvi · 2019-03-13T11:57:11Z

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

+                sandbox.stub(optlyInstance.decisionService, 'getVariationForFeature').returns({
+                  experiment: null,
+                  variation: null,
+                  decisionSource: null,


I feel the actual decisionService needs to be updated to set decisionSource as rollout.

Thoughts @jordangarcia @mjc1283 ?

Yeah I think decisionSource should be rollout even if the user is not evaluated into the experiment, as it's jkind of the bottom of evaluation chain.

jordangarcia

There is some logic that was not implemented correctly around the value of the variable when feature is not enabled.

Also please change the decision service to return ROLLOUT when the variation is null.

Lastly the tests need to be updated to not directly modify configObj that's loaded in the Optimizely instance.

jordangarcia · 2019-03-13T16:41:57Z

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

-    variableValue = projectConfig.getVariableValueForVariation(this.configObj, variable, decision.variation, this.logger);
-    this.logger.log(LOG_LEVEL.INFO, sprintf(LOG_MESSAGES.USER_RECEIVED_VARIABLE_VALUE, MODULE_NAME, variableKey, featureFlag.key, variableValue, userId));
+    featureEnabled = decision.variation.featureEnabled;
+    if (featureEnabled === true) {


This logic is incorrect.

Feature variables should always be the variation variable value, even if the feature is disabled.

@jordangarcia it is actually supposed to be the default feature value

@aliabbasrizvi, agree that this is the default behavior. And the datafile constraint of a feature experiment variation (where featureEnabled == false) always has variables that mirror the variable defaults makes it so this works today.

Given the scope of this PR (just adding notificationCenter listener), i'd prefer not to change this core logic.

Java also mirrors the existing JS implementation https://github.com/optimizely/java-sdk/blob/master/core-api/src/main/java/com/optimizely/ab/Optimizely.java#L615-L668

Modified the logic to return variation's variable value even when the featureEnabled property is set to false.

jordangarcia · 2019-03-13T16:42:16Z

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

+      this.logger.log(LOG_LEVEL.INFO, sprintf(LOG_MESSAGES.USER_RECEIVED_VARIABLE_VALUE, MODULE_NAME, variableKey, featureFlag.key, variableValue, userId));
+    } else {
+      this.logger.log(LOG_LEVEL.INFO, sprintf(LOG_MESSAGES.FEATURE_NOT_ENABLED_RETURN_DEFAULT_VARIABLE_VALUE, MODULE_NAME, featureFlag.key, userId, variableKey));
+      featureEnabled = false;


There is no reason to re-card featureEnabled here.

jordangarcia · 2019-03-13T16:47:24Z

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

+            optlyInstance = new Optimizely({
+              clientEngine: 'node-sdk',
+              datafile: testData.getTestProjectConfigWithFeatures(),
+              eventBuilder: eventBuilder,


eventBuilder isn't a valid option to pass to new Optimizely

jordangarcia · 2019-03-13T16:48:47Z

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

+              describe('when the variation is toggled OFF', function() {
+                beforeEach(function() {
+                  // Setting featureEnabled to false for verifying feature toggle off.
+                  optlyInstance.configObj.experimentKeyMap.testing_my_feature.variations[0].featureEnabled = false;


We shouldn't be reaching into the configObj like this.

We should change the test datafile that were using to include a feature where a variation has featureEnabled = false

jordangarcia · 2019-03-13T16:49:43Z

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

+                beforeEach(function() {
+                  // Setting featureEnabled to false for verifying feature toggle off.
+                  optlyInstance.configObj.experimentKeyMap.testing_my_feature.variations[0].featureEnabled = false;
+                  var experiment = optlyInstance.configObj.experimentKeyMap.testing_my_feature;


use projectConfig.getExperimentFromKey(optlyInstance.configObj, 'testing_my_feature') instead of directly inspecting the configObj

jordangarcia · 2019-03-13T16:50:15Z

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

+            describe('bucketed into variation of a rollout with variable values', function() {
+              describe('when the variation is toggled ON', function() {
+                beforeEach(function() {
+                  var experiment = optlyInstance.configObj.experimentKeyMap['594031'];


use projectConfig function here instead.

jordangarcia · 2019-03-13T16:55:31Z

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

+                sandbox.stub(optlyInstance.decisionService, 'getVariationForFeature').returns({
+                  experiment: null,
+                  variation: null,
+                  decisionSource: null,


Yeah I think decisionSource should be rollout even if the user is not evaluated into the experiment, as it's jkind of the bottom of evaluation chain.

jordangarcia · 2019-03-13T16:55:53Z

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

@@ -2608,6 +3117,7 @@ describe('lib/optimizely', function() {
      sandbox.stub(uuid, 'v4').returns('a68cf1ad-0393-4e18-af87-efe8f01a7c9c');
      sandbox.stub(fns, 'currentTimestamp').returns(1509489766569);
      clock = sinon.useFakeTimers(new Date().getTime());
+      onDecisionListener = sinon.spy();


Is this still needed?

…if the user is not evaluated into the experiment.

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

…ble tests.

…istener-feature-variables # Conflicts: # packages/optimizely-sdk/lib/optimizely/index.js # packages/optimizely-sdk/lib/tests/test_data.js

jordangarcia · 2019-03-25T18:13:53Z

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

-      LOG_LEVEL.INFO,
-      sprintf(LOG_MESSAGES.USER_RECEIVED_DEFAULT_VARIABLE_VALUE, MODULE_NAME, userId, variableKey, featureFlag.key)
-    );
+  	variableValue = variable.defaultValue;


fix this indentation.

jordangarcia · 2019-03-25T18:14:23Z

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

@@ -199,8 +199,8 @@ exports.DECISION_INFO_TYPES = {
 * Optimizely.
 */
 exports.DECISION_SOURCES = {
-  EXPERIMENT: 'experiment',
-  ROLLOUT: 'rollout',
+  EXPERIMENT: 'EXPERIMENT',


This should be lowercase, right?

@aliabbasrizvi

I think it will be upper case as @aliabbasrizvi suggested here
#245 (comment)

aliabbasrizvi · 2019-03-26T22:16:29Z

@jordangarcia can you take a look again and approve/decline?

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

msohailhussain · 2019-03-29T05:40:05Z

@jordangarcia can you please rereview and let me know if any change is needed?

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

…istener-feature-variables

mjc1283

This LGTM, but I am leaving it for @jordangarcia to approve.

jordangarcia · 2019-04-02T23:51:39Z

LGTM

feat(api): OnDecision Notification Listener for feature variable APIs.

eabb383

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:40

mfahadahmed added 3 commits March 12, 2019 18:54

Implement suggested changes.

f401d21

Separate out the unit tests for decision listener.

167294c

Reverted existing unit tests.

26f63e3

msohailhussain requested a review from aliabbasrizvi March 12, 2019 19:48

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

5d6fe51

…istener-feature-variables

mfahadahmed changed the title ~~feat(api): OnDecision Notification Listener for feature variable APIs.~~ feat(api): Decision Notification Listener for feature variable APIs. Mar 13, 2019

aliabbasrizvi approved these changes Mar 13, 2019

View reviewed changes

Capitalize decision source values.

8c80556

jordangarcia self-requested a review March 13, 2019 15:20

Revert decision_service tests change.

cd0e313

jordangarcia suggested changes Mar 13, 2019

View reviewed changes

mfahadahmed mentioned this pull request Mar 14, 2019

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

Merged

mfahadahmed added 12 commits March 14, 2019 18:47

getVariationForFeature method now returns decision source as rollout …

c6dd63a

…if the user is not evaluated into the experiment.

Address code review comments.

79ea03d

Revert "Revert decision_service tests change." commit.

2dec469

Address code review comments.

82b3ce4

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

0256e7c

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

Adds new variation with featureEnabled set to false for feature varia…

01f0288

…ble tests.

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

49802a2

…istener-feature-variables # Conflicts: # packages/optimizely-sdk/lib/optimizely/index.js # packages/optimizely-sdk/lib/tests/test_data.js

Modified data file.

24a61e3

Modified data file.

d66dc1e

Remove quotes.

56a07f1

Return right variable value from variation when featureEnabled is false.

1f4987b

Return right variable value from variation when featureEnabled is false.

473686f

jordangarcia reviewed Mar 25, 2019

View reviewed changes

Fix indentation.

1b191cf

msohailhussain requested a review from jordangarcia March 27, 2019 20:35

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

bb6bfa9

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

mfahadahmed requested a review from aliabbasrizvi March 28, 2019 07:26

mfahadahmed and others added 2 commits March 30, 2019 00:34

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

ea581e0

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

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

69fb7d6

…istener-feature-variables

aliabbasrizvi requested a review from mjc1283 April 2, 2019 20:31

mjc1283 reviewed Apr 2, 2019

View reviewed changes

jordangarcia approved these changes Apr 2, 2019

View reviewed changes

jordangarcia merged commit 3c69a02 into master Apr 2, 2019

aliabbasrizvi deleted the fahad/on-decision-listener-feature-variables branch April 17, 2019 18:42

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

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

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!

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!

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!

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!

aliabbasrizvi commented Mar 26, 2019

Uh oh!

msohailhussain commented Mar 29, 2019

Uh oh!

mjc1283 left a comment

Choose a reason for hiding this comment

Uh oh!

jordangarcia commented Apr 2, 2019

Uh oh!

Uh oh!

coveralls commented Mar 8, 2019 •

edited

Loading