feat(ForcedDecisions): Add forced-decisions APIs to OptimizelyUserContext by yavorona · Pull Request #705 · optimizely/javascript-sdk

yavorona · 2021-09-23T01:56:22Z

Summary

Add a set of new APIs for forced-decisions to OptimizelyUserContext:

setForcedDecision
getForcedDecision
removeForcedDecision
removeAllForcedDecisions

Test plan

unit tests for the new APIs
FSC tests with new test cases ( testapp pr: https://github.com/optimizely/javascript-testapp/pull/116)
Custom travis build: https://app.travis-ci.com/github/optimizely/fullstack-sdk-compatibility-suite/jobs/544315539

coveralls · 2021-09-23T02:14:25Z

Coverage increased (+0.04%) to 97.157% when pulling 5bf613c on pnguen/forced-decisions into 6e1f6d6 on master.

jaeopt

It overall looks good. A few changes suggested mostly for OptimizelyUserContext.

jaeopt

Looks good. A few more suggestions on top-level UserContext. We can discuss offline for some of them.

jaeopt

All changes look good. One more suggestion for hashing.

Update decisioService methods to use user instance WIP WIP WIP WIP WIP WIP Finish implementation and start updating unit tests Continue updating exsiting decision service tests Finish updating decision service tests Clean up Fix remaining tests Clean up Start adding new tests Add setForcedDecision and getForcedDecision unit tests Add removeForcedDecision implementation and tests Add removeAllForcedDecisions implementation and tests Add impression event test cases and clean up Add notifications check Add project config tests Add decision getVariationId tests Update OptimizelyUserContext to include forced decision APIs Remove deprecation notes Incorporate Jae comments part 1 Update setForcedDecision signature to use OptimizelyForcedDecisionKey interface Use dictionary for forced decisions instead of array Update unit tests Rebase, fix conficts and update unit tests Update log variables names Include OptimizelyForcedDecisionKey in type interfaces Clea up Apply OptimizelyDecisionKey in getForcedDecision and removeForcedDecision method signatures Consider case when flagKey is empty string Change to using a single forced decisions map

jaeopt

API changes not finalized yet. I just reviewed map hashkey parts.

jaeopt · 2021-10-15T21:17:26Z

+        { flagKey, variationKey } :
+        { flagKey, ruleKey, variationKey };


Do we need to save "flagKey, ruleKey" for decision value? We can consider saving "variationKey" only.

jaeopt · 2021-10-15T21:23:19Z

+    if (this.forcedDecisionsMap.hasOwnProperty(flagKey)) {
+      const forcedDecisionByRuleKey = this.forcedDecisionsMap[flagKey];
+      if (forcedDecisionByRuleKey.hasOwnProperty(ruleKey)) {
+        delete this.forcedDecisionsMap[flagKey][ruleKey];
+        isForcedDecisionRemoved = true;
+      }
+      if (Object.keys(this.forcedDecisionsMap[flagKey]).length === 0) {
+        delete this.forcedDecisionsMap[flagKey];
+      }
+    }


The flat map above will help to simplify this as well -

delete this.forcedDecisionsMap[decisionKey(flagKey, ruleKey)]

The flat map above will help to simplify this as well -

delete this.forcedDecisionsMap[decisionKey(flagKey, ruleKey)]

Looks like @jaeopt is talking about a simple list with composite key instead of 2D. I totally agree with that

I thought we agreed to go with a single nested object here from performance point of view to avoid having to compute the composite key to lookup the forced decision. Wasn't that the case?

zashraf1985

Good Work Overall! Left some comments. Its a big diff so i have half reviewed it so far. Will take another look tomorrow morning and might leave a few more comments

zashraf1985 · 2021-10-18T20:28:54Z

+   * @return    {boolean}                     true if the forced decision has been set successfully.
+   */
+  setForcedDecision(key: OptimizelyDecisionKey, variationKey: string): boolean {
+    if (this.optimizely.getOptimizelyConfig() === null) {


This can be bad performance wise. OptimizelyConfig is not created until the first api call. assuming the scenario where this was not created already, creating a whole config object just to make sure the SDK is initialized is a bit of overkill. it will be better to use any existing flags from optimizely object. there is isValidInstance. you can expose that if needed

zashraf1985 · 2021-10-18T20:33:38Z

+    if (this.forcedDecisionsMap[flagKey]) {
+      this.forcedDecisionsMap[flagKey][ruleKey] = forcedDecision;
+    } else {
+      this.forcedDecisionsMap[flagKey] = {};
+      this.forcedDecisionsMap[flagKey][ruleKey] = forcedDecision;
+    }


Suggested change

if (this.forcedDecisionsMap[flagKey]) {

this.forcedDecisionsMap[flagKey][ruleKey] = forcedDecision;

} else {

this.forcedDecisionsMap[flagKey] = {};

this.forcedDecisionsMap[flagKey][ruleKey] = forcedDecision;

}

if (!this.forcedDecisionsMap[flagKey]) {

this.forcedDecisionsMap[flagKey] = {};

}

this.forcedDecisionsMap[flagKey][ruleKey] = forcedDecision;

zashraf1985 · 2021-10-18T20:40:26Z

+      this.forcedDecisionsMap[flagKey][ruleKey] = forcedDecision;
+    } else {
+      this.forcedDecisionsMap[flagKey] = {};
+      this.forcedDecisionsMap[flagKey][ruleKey] = forcedDecision;


I m not sure about how everyone else is doing it but you could use a simple list with composite keys with concatenation.

In our last meeting, as I remember we decided to go with the nested map here. Let's sync up on it once again

zashraf1985 · 2021-10-18T20:41:26Z

+    if (this.forcedDecisionsMap.hasOwnProperty(flagKey)) {
+      const forcedDecisionByRuleKey = this.forcedDecisionsMap[flagKey];
+      if (forcedDecisionByRuleKey.hasOwnProperty(ruleKey)) {
+        delete this.forcedDecisionsMap[flagKey][ruleKey];
+        isForcedDecisionRemoved = true;
+      }
+      if (Object.keys(this.forcedDecisionsMap[flagKey]).length === 0) {
+        delete this.forcedDecisionsMap[flagKey];
+      }
+    }


The flat map above will help to simplify this as well -

delete this.forcedDecisionsMap[decisionKey(flagKey, ruleKey)]

Looks like @jaeopt is talking about a simple list with composite key instead of 2D. I totally agree with that

zashraf1985 · 2021-10-18T20:43:06Z

+    if (this.forcedDecisionsMap.hasOwnProperty(flagKey)) {
+      const forcedDecisionByRuleKey = this.forcedDecisionsMap[flagKey];
+      if (forcedDecisionByRuleKey.hasOwnProperty(validRuleKey)) {
+        variationKey = forcedDecisionByRuleKey[validRuleKey].variationKey;
+      }
+    }


if you use a single list, this will whole simplify a lot

zashraf1985 · 2021-10-18T20:50:09Z

+    });
+
+    if (Object.keys(this.forcedDecisionsMap).length > 0) {
+      userContext.forcedDecisionsMap = { ...this.forcedDecisionsMap };


May be we should cache the cloned user context. This will make a new clone everytime any API is called. and if there are manyy forced decisions, this can impact performance. May be we can leave it like this for now and circle back to it later.

zashraf1985 · 2021-10-18T20:53:10Z

+    return variationKey;
+  }
+
+  findValidatedForcedDecision(


A comment will be helpful here explaining what exactly it does and how its different from the normal get one

zashraf1985 · 2021-10-18T23:20:42Z

    }

-    const decisionObj = this.decisionService.getVariationForFeature(configObj, featureFlag, userId, attributes).result;
+    const user = this.createUserContext(userId, attributes) as OptimizelyUserContext;


why is explicit casting needed here?

Because getVariationForFeature should expect a user of OptimizelyUserContext type, where createUserContext returns OptimizelyUserContext | null. Validation of userId and attributes happens before we get to create an instance of user context, so having explicit casting seem ok here. Let me know if you think otherwise.

zashraf1985

LGTM! Approved with super NIT.

zashraf1985 · 2021-10-27T18:11:12Z

+    if (!this.optimizely.isValidInstance()) {
+      logger.error(DECISION_MESSAGES.SDK_NOT_READY);
+      return false;
+


NIT: remove this extra line break

jaeopt

LGTM

optimizely-codereview-bot assigned yavorona Sep 23, 2021

yavorona marked this pull request as ready for review October 1, 2021 03:39

yavorona requested a review from a team as a code owner October 1, 2021 03:39

jaeopt requested changes Oct 5, 2021

View reviewed changes

yavorona force-pushed the pnguen/forced-decisions branch from d7f9907 to fc7b87a Compare October 5, 2021 23:44

yavorona requested a review from zashraf1985 October 7, 2021 16:37

yavorona force-pushed the pnguen/forced-decisions branch from 74f7e89 to 2519ef4 Compare October 7, 2021 23:59

yavorona removed their assignment Oct 8, 2021

optimizely-codereview-bot requested a review from jaeopt October 8, 2021 00:16

optimizely-codereview-bot added 🤖 4 hrs 🤖 8 hrs and removed 🤖 4 hrs labels Oct 8, 2021

jaeopt requested changes Oct 11, 2021

View reviewed changes

Comment thread packages/optimizely-sdk/lib/optimizely_user_context/index.ts Outdated

Comment thread packages/optimizely-sdk/lib/optimizely_user_context/index.ts Outdated

Comment thread packages/optimizely-sdk/lib/optimizely_user_context/index.ts Outdated

optimizely-codereview-bot removed the 🤖 8 hrs label Oct 11, 2021

optimizely-codereview-bot assigned yavorona Oct 11, 2021

yavorona force-pushed the pnguen/forced-decisions branch 4 times, most recently from f9ac1f6 to af836c2 Compare October 11, 2021 22:59

yavorona removed their assignment Oct 11, 2021

optimizely-codereview-bot requested a review from jaeopt October 11, 2021 23:05

jaeopt requested changes Oct 12, 2021

View reviewed changes

optimizely-codereview-bot assigned yavorona Oct 12, 2021

yavorona force-pushed the pnguen/forced-decisions branch 2 times, most recently from 1f10508 to 540f0d5 Compare October 14, 2021 18:34

yavorona force-pushed the pnguen/forced-decisions branch from 540f0d5 to 0d0f876 Compare October 14, 2021 22:32

Revert unnecessary changes

2768c82

yavorona removed their assignment Oct 15, 2021

optimizely-codereview-bot requested a review from jaeopt October 15, 2021 00:37

optimizely-codereview-bot added the 🤖 4 hrs label Oct 15, 2021

jaeopt requested changes Oct 15, 2021

View reviewed changes

optimizely-codereview-bot removed the 🤖 4 hrs label Oct 15, 2021

optimizely-codereview-bot assigned yavorona Oct 15, 2021

zashraf1985 suggested changes Oct 18, 2021

View reviewed changes

Address comments

2886188

yavorona force-pushed the pnguen/forced-decisions branch 2 times, most recently from 985950e to 2886188 Compare October 20, 2021 22:54

Update API based on latest TDD changes

f685d3d

yavorona force-pushed the pnguen/forced-decisions branch from fc2ad9e to f685d3d Compare October 21, 2021 00:00

yavorona added 3 commits October 20, 2021 17:09

Expose OptimizelyForcedDecision type

6954be0

Update log message to match FSC

ecc6556

Clean up

6de6fd0

zashraf1985 approved these changes Oct 27, 2021

View reviewed changes

jaeopt approved these changes Oct 27, 2021

View reviewed changes

msohailhussain closed this Oct 27, 2021

msohailhussain reopened this Oct 27, 2021

msohailhussain closed this Oct 27, 2021

msohailhussain reopened this Oct 27, 2021

fixes for fsc to pass.

5bf613c

msohailhussain closed this Oct 28, 2021

msohailhussain reopened this Oct 28, 2021

msohailhussain merged commit 94794ee into master Oct 28, 2021

msohailhussain deleted the pnguen/forced-decisions branch October 28, 2021 21:24

jaeopt mentioned this pull request Jan 6, 2022

chore: prepare for release 4.9.0 #731

Merged

		{ flagKey, variationKey } :
		{ flagKey, ruleKey, variationKey };

Conversation

yavorona commented Sep 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

coveralls commented Sep 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaeopt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jaeopt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jaeopt left a comment

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!

Choose a reason for hiding this comment

Uh oh!

yavorona Oct 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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

Reviewers

Assignees

Labels

Projects

Milestone

yavorona commented Sep 23, 2021 •

edited

Loading

coveralls commented Sep 23, 2021 •

edited

Loading

yavorona Oct 19, 2021 •

edited

Loading