feat: implement decide API #634

yavorona · 2020-12-08T18:50:55Z

Summary

Implemented decide API
Implemented Decide Reasons partially

Test

Manual testing + unit tests

Issues

OASIS-6934

coveralls · 2020-12-08T19:04:13Z

Coverage increased (+0.03%) to 96.764% when pulling 27c30a4 on pnguen/implement-decide-api into d658697 on master.

coveralls · 2020-12-08T19:04:14Z

Coverage increased (+0.05%) to 96.761% when pulling 8d98a5d on pnguen/implement-decide-api into dfc1b7f on master.

jaeopt

Looks good. A couple of changes and more tests suggested.

jaeopt · 2020-12-08T19:34:13Z

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

+          var expectedDecision = {
+            variationKey: null,
+            enabled: false,
+            variables: null,


return empty variables (instead of null) on error. Same for other tests.

jaeopt · 2020-12-08T19:44:12Z

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

+      });
+    });
+  });
+


Can we have one more test validating api decide option ("EXCLUDE_VARIABLES") work together with defaultDecideOption ("DISABLE_DISPATCH_EVENT"), so we see both options are on.

jaeopt · 2020-12-08T21:15:14Z

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

+          assert.deepEqual(callArgs[0], expectedImpressionEvent);
+        });
+      });
+


Can we add validation for decision flag notification?

jaeopt · 2020-12-08T21:18:35Z

packages/optimizely-sdk/lib/optimizely_decision/optimizely_decision.ts

+  return {
+    variationKey: null,
+    enabled: false,
+    variables: null,


Variable should be empty {} instead of null

jaeopt · 2020-12-08T21:24:30Z

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

+    });
+  }
+
+  decide(


Can we make sure "decide", "decideAll" and "decideForKey" API in optimizely not open to public?
They should access those APIs via OptimizelyUserContext always.

We can exclude them from the top-level type definitions.

mjc1283 · 2020-12-08T23:02:09Z

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

+          clientEngine: 'node-sdk',
+          datafile: testData.getTestDecideProjectConfig(),
+          errorHandler: errorHandler,
+          eventDispatcher: eventDispatcher,


Using a real event dispatcher could cause event requests to be sent during test runs - we don't want that. We should pass in a fake event dispatcher.

dispatchEvent of eventDispatcher is being stubbed in the outer beforeEach function, so I think we should be fine. @mjc1283 let me know if you think otherwise

mjc1283 · 2020-12-08T23:02:41Z

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

+            clientEngine: 'node-sdk',
+            datafile: testData.getTestDecideProjectConfig(),
+            errorHandler: errorHandler,
+            eventDispatcher: eventDispatcher,


Same comment re: real event dispatcher.

Please see my response above

mjc1283 · 2020-12-08T23:12:06Z

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

+          }
+          assert.deepEqual(decision, expectedDecision);
+          sinon.assert.calledOnce(optlyInstance.eventDispatcher.dispatchEvent);
+          var expectedImpressionEvent = {


IMO, asserting the whole structure of this object is a lot. We could do it one test, and then in the others, we could just check a few properties, or check that dispatchEvent was called.

mjc1283 · 2020-12-08T23:13:45Z

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

+      // Filter out all provided default decide options that are not in OptimizelyDecideOptions[]
+      if (OptimizelyDecideOptions[option]) {
+        defaultDecideOptions[option] = true;
+      }


Perhaps we could log a warning here if we encounter an invalid value - @jaeopt WDYT?

mjc1283 · 2020-12-08T23:15:12Z

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

+   * @return {[key: string]: boolean}             Map of all provided decide options including default decide options
+   */
+  private getAllDecideOptions(options: OptimizelyDecideOptions[]): { [key: string]: boolean } {
+    const allDecideOptions = {...this.defaultDecideOptions};


Looks better! 👍

mjc1283 · 2020-12-08T23:17:23Z

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

+      const featureFlag = projectConfig.getFeatureFromKey(configObj, key, this.logger);
+      featureFlag?.variables.forEach(variable => {


We already have feature declared on 1474. Are these the same? Can we just use that one?

mjc1283 · 2020-12-08T23:19:02Z

packages/optimizely-sdk/lib/optimizely_decision/optimizely_decision.ts

+export interface OptimizelyDecision {
+  variationKey: string | null;
+  enabled: boolean;
+  variables: { [variableKey: string]: unknown } | null;


If following @jaeopt 's suggestion that this is an empty object when there is "no decision", we can remove | null;

mjc1283 · 2020-12-08T23:20:06Z

packages/optimizely-sdk/lib/shared_types.ts

@@ -117,6 +117,15 @@ export interface Rollout {
  experiments: Experiment[];
 }

+//TODO: Move OptimizelyDecideOptions to @optimizely/optimizely-sdk/lib/utils/enums


Why move it there?

Thinking it might be a good idea to expose it for clients use. LMKWYT @mjc1283

Good point 👍

…provided

mjc1283 · 2020-12-09T17:20:00Z

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

 } from '../shared_types';
+import { OptimizelyDecision, newErrorDecision } from '../optimizely_decision/optimizely_decision';


Consider: since there is only one file, we could name it optimizely_decision/index.ts, then it can be imported like import {...} from '../optimizely_decision';

jaeopt

Decide notification format should be changed.

jaeopt · 2020-12-09T18:24:57Z

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

+      type: DECISION_NOTIFICATION_TYPES.FLAG,
+      userId: userId,
+      attributes: attributes,
+      decisionInfo: featureInfo,
+      decisionEventDispatched: decisionEventDispatched,


This notification data is different from the format defined in the TDD. Can you fix it?
Also can we have a test case to validate the notification contents for decide call?

jaeopt

Remove "source" from notification. Other than that, all looks good.

jaeopt · 2020-12-09T21:54:30Z

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

-                featureKey: 'feature_2',
-                featureEnabled: true,
+                flagKey: 'feature_2',
+                enabled: true,
                source: 'feature-test',


remove "source"

jaeopt · 2020-12-09T21:55:41Z

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

+              decisionInfo: {
+                flagKey: 'feature_2',
+                enabled: true,
+                source: 'feature-test',


remove "source"

jaeopt

LGTM

yavorona requested a review from a team as a code owner December 8, 2020 18:50

optimizely-codereview-bot assigned yavorona Dec 8, 2020

yavorona removed their assignment Dec 8, 2020

yavorona requested review from jaeopt and mjc1283 December 8, 2020 18:53

jaeopt requested changes Dec 8, 2020

View reviewed changes

optimizely-codereview-bot assigned yavorona Dec 8, 2020

yavorona added 12 commits December 8, 2020 14:56

Implement createUserContext

ef0dfb6

Clean up

01dda48

Incorporate comments

59bbc9f

Implement decide api

3380ca8

Add decide api unit tests

646feee

add flag to DECISION_NOTIFICATION_TYPES

ea55428

Store defaultDecideOptions as object with boolean values

c856f1c

Start adding decide api unit tests to optimizely client

6c03853

Finish decide api with empty default decide options unit tests

59de6b6

Change alldecideOptions to object instead of an array

a0cb796

Update user context unit tests

b09f217

Fix unit test

ff8745b

yavorona force-pushed the pnguen/implement-decide-api branch from 8d98a5d to ff8745b Compare December 8, 2020 22:57

mjc1283 suggested changes Dec 8, 2020

View reviewed changes

yavorona added 4 commits December 8, 2020 17:10

Address comments

7b58ebe

Add tests when both DISABLE_DECISION_EVENT and EXCLUDE_VARIABLES are …

24f6719

…provided

Log warning when unrecognized decide option provided

023ea81

Remove redundant featureFlag

c2d17b2

yavorona removed their assignment Dec 9, 2020

optimizely-codereview-bot requested review from jaeopt and mjc1283 December 9, 2020 01:50

mjc1283 approved these changes Dec 9, 2020

View reviewed changes

Rename optimizely_decision.ts. to index.ts

8f85715

jaeopt requested changes Dec 9, 2020

View reviewed changes

optimizely-codereview-bot assigned yavorona Dec 9, 2020

Fix notification data object

bacb961

yavorona removed their assignment Dec 9, 2020

optimizely-codereview-bot requested a review from jaeopt December 9, 2020 21:31

jaeopt requested changes Dec 9, 2020

View reviewed changes

optimizely-codereview-bot assigned yavorona Dec 9, 2020

Remove sourse field from decision notification number

27c30a4

yavorona removed their assignment Dec 9, 2020

optimizely-codereview-bot requested a review from jaeopt December 9, 2020 22:13

jaeopt approved these changes Dec 9, 2020

View reviewed changes

yavorona merged commit 132587e into master Dec 9, 2020

yavorona deleted the pnguen/implement-decide-api branch December 9, 2020 22:28

+                  });
+                }
+                decide(

		const featureFlag = projectConfig.getFeatureFromKey(configObj, key, this.logger);
		featureFlag?.variables.forEach(variable => {

		} from '../shared_types';
		import { OptimizelyDecision, newErrorDecision } from '../optimizely_decision/optimizely_decision';

feat: implement decide API #634

feat: implement decide API #634

Uh oh!

Conversation

yavorona commented Dec 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test

Issues

Uh oh!

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

Uh oh!

Uh oh!

coveralls commented Dec 8, 2020

Uh oh!

jaeopt 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!

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!

jaeopt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaeopt 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!

jaeopt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yavorona commented Dec 8, 2020 •

edited

Loading

coveralls commented Dec 8, 2020 •

edited

Loading