feat(api): OnDecision Notification Listener #240

mfahadahmed · 2019-03-06T15:19:59Z

Summary

Implemented onDecisionListener in following APIs:
isfeatureEnabled
getEnabledFeatures
getFeatureVariableString
getFeatureVariableBoolean
getFeatureVariableInteger
getFeatureVariableDouble

Test plan

Unit testss for OnDecisionListener

jordangarcia · 2019-03-06T19:46:52Z

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

+  ON_DECISION: 'ON_DECISION:type, user_id, attributes, decision_info',
+};
+
+exports.ON_DECISION_NOTIFICATION_TYPES = {


Lets call this DECISION_INFO_TYPES

They aren't really NOTIFICATION_TYPES like the ones above.

jordangarcia

Some changes for the structure of the decisionInfo.

I also echo'd these changes in the design document.

After these changes have been implemented I will review this PR again in more detail.

jordangarcia · 2019-03-06T19:47:25Z

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

+        enums.NOTIFICATION_TYPES.ON_DECISION,
+        {
+          type: ON_DECISION_NOTIFICATION_TYPES.EXPERIMENT_VARIATION,
+          user_id: userId,


lets use camelCase property names to be consistent with other notification events.

If we are using camelCase, then it must be reflected here as well for Activate, Track and ON_DECISION/Decision
https://github.com/optimizely/javascript-sdk/pull/240/files#diff-79c0858a5dc2611f6570afc1265258daR179

jordangarcia · 2019-03-06T19:50:49Z

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

+    }
+
+    var decisionSource = decision.decisionSource || DECISION_SOURCES.ROLLOUT;
+    decisionSource += decisionSource === DECISION_SOURCES.EXPERIMENT ? sprintf(' {%s}', decision.experiment.key) : '';


Lets keep this the same as decision.decisionSource and add a new field to decision_info

{ type: ON_DECISION_NOTIFICATION_TYPES.FEATURE, user_id: userId, attributes: attributes, decisionInfo: { featureKey: featureKey, featureEnabled: featureEnabled, source: 'ROLLOUT', experimentKey: null, }

and

{ type: ON_DECISION_NOTIFICATION_TYPES.FEATURE, user_id: userId, attributes: attributes, decisionInfo: { featureKey: featureKey, featureEnabled: featureEnabled, source: 'EXPERIMENT', experimentKey: 'feature_exp_key', }

jordangarcia · 2019-03-06T19:51:06Z

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

+    var decisionSource = decision.decisionSource || DECISION_SOURCES.ROLLOUT;
+    decisionSource += decisionSource === DECISION_SOURCES.EXPERIMENT ? sprintf(' {%s}', decision.experiment.key) : '';
+
+    console.log(sprintf('Feature: %s - Enabled: %s, Source: %s', featureKey, featureEnabled, decisionSource))


remove this console log.

jordangarcia · 2019-03-06T19:51:54Z

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 {
    variableValue = variable.defaultValue;
    this.logger.log(LOG_LEVEL.INFO, sprintf(LOG_MESSAGES.USER_RECEIVED_DEFAULT_VARIABLE_VALUE, MODULE_NAME, userId, variableKey, featureFlag.key));
  }

-  return projectConfig.getTypeCastValue(variableValue, variableType, this.logger);
+  var decisionSource = decision.decisionSource || DECISION_SOURCES.ROLLOUT;
+  decisionSource += decisionSource === DECISION_SOURCES.EXPERIMENT ? sprintf(' {%s}', decision.experiment.key) : '';


Same thing as above. Let's keep decisionSource unmodified and add experimentKey to decisionInfo

coveralls · 2019-03-07T00:39:54Z

Coverage increased (+0.08%) to 97.729% when pulling e61fb00 on fahad/on-decision-listener into 34ecdcc on master.

aliabbasrizvi · 2019-03-07T10:00:43Z

@mfahadahmed I am going to request you to break this change into 2 separate PRs at least. One for feature enabled and one for feature variable. It will make reviewing go faster.

msohailhussain · 2019-03-07T19:51:38Z

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

@@ -729,22 +729,22 @@ describe('lib/core/decision_service', function() {
                    'variables': [
                      {
                        'id': '4792309476491264',
-                        'value': '10'
+                        'value': '40'


If we are making any change in the existing datafile, please add comments in the PR, why we changed it. Otherwise difficult for reviewers

same for all where you changed true, Buy me Later

msohailhussain · 2019-03-07T19:52:44Z

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

@@ -34,6 +34,7 @@ var LOG_MESSAGES = enums.LOG_MESSAGES;
 var MODULE_NAME = 'OPTIMIZELY';
 var DECISION_SOURCES = enums.DECISION_SOURCES;
 var FEATURE_VARIABLE_TYPES = enums.FEATURE_VARIABLE_TYPES;
+var DECISION_INFO_TYPES = enums.DECISION_INFO_TYPES;


@jordangarcia shouldn't it be DECISION_TYPES?

I think DECISION_INFO_TYPES is fine. It was confusing before when it was DECISION_NOTIFICATION_TYPES

msohailhussain · 2019-03-07T20:10:18Z

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

+};
+
+exports.DECISION_INFO_TYPES = {
+  EXPERIMENT_VARIATION: 'experiment_variation',


Why it's experiment_variation? It should be experiment.

Would you mind checking with @aliabbasrizvi on whether this should be experiment_variation or experiment .

Right now it's still experiment_variation in the design doc

In th example, experiment was used. Added comment in the doc.

msohailhussain · 2019-03-07T20:15:02Z

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

@@ -177,6 +178,13 @@ exports.NODE_CLIENT_VERSION = '3.0.1';
 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, user_id, attributes, decision_info',


It's been rename to Decision?

I dont think this change has been finalized. I'd keep it how it is today for consistency with the other NOTIFICATION_TYPES

msohailhussain · 2019-03-07T20:16:15Z

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

+      this.notificationCenter.sendNotifications(
+        enums.NOTIFICATION_TYPES.ON_DECISION,
+        {
+          type: DECISION_INFO_TYPES.EXPERIMENT_VARIATION,


There is no type EXPERIMENT_VARIATION

I believe @aliabbasrizvi changed this to be just EXPERIMENT in the design doc

msohailhussain · 2019-03-07T20:20:29Z

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

+        enums.NOTIFICATION_TYPES.ON_DECISION,
+        {
+          type: ON_DECISION_NOTIFICATION_TYPES.EXPERIMENT_VARIATION,
+          user_id: userId,


If we are using camelCase, then it must be reflected here as well for Activate, Track and ON_DECISION/Decision
https://github.com/optimizely/javascript-sdk/pull/240/files#diff-79c0858a5dc2611f6570afc1265258daR179

msohailhussain

initial review

…istener

mfahadahmed · 2019-03-08T15:34:29Z

@aliabbasrizvi @jordangarcia @msohailhussain Closing this PR as this change has been split into PR #244 , #245 and #246 now.

mfahadahmed added 6 commits March 4, 2019 18:15

Working on on-decision listener.

272b5e1

Fix unit tests.

337e9ac

Fix unit tests.

96c82c8

code refactoring.

92e37c8

Added unit tests for decision listener.

0e13435

Added unit tests for feature-variable APIs.

38ba13b

mfahadahmed added the WIP label Mar 6, 2019

mfahadahmed changed the title ~~Fahad/on decision listener~~ feat(api): OnDecision Notification Listener Mar 6, 2019

mfahadahmed requested a review from msohailhussain March 6, 2019 15:21

jordangarcia reviewed Mar 6, 2019

View reviewed changes

jordangarcia suggested changes Mar 6, 2019

View reviewed changes

mfahadahmed added 2 commits March 7, 2019 23:09

Implemented new changes in the requirements document.

f3edf5b

Implemented new changes in the requirements document.

dce7a0a

msohailhussain reviewed Mar 7, 2019

View reviewed changes

mfahadahmed added 2 commits March 8, 2019 15:58

Address code review comments.

341a09b

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

e61fb00

…istener

mfahadahmed closed this Mar 8, 2019

mfahadahmed deleted the fahad/on-decision-listener branch May 9, 2019 11:49

feat(api): OnDecision Notification Listener #240

feat(api): OnDecision Notification Listener #240

Uh oh!

Conversation

mfahadahmed commented Mar 6, 2019

Summary

Test plan

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!

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

Uh oh!

Uh oh!

aliabbasrizvi commented Mar 7, 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!

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!

msohailhussain left a comment

Choose a reason for hiding this comment

Uh oh!

mfahadahmed commented Mar 8, 2019

Uh oh!

Uh oh!

coveralls commented Mar 7, 2019 •

edited

Loading