feat(ForcedDecisions): Add forced-decisions APIs to OptimizelyUserContext #705

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.

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

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

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

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

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

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

jaeopt

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

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

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

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

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

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

+    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

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

+   * @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

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

+    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

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

+      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

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

+    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

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

+    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

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

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

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

+    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

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

@@ -880,7 +884,8 @@ export default class Optimizely {
      return null;
    }

-    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

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

+    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

packages/optimizely-sdk/lib/optimizely_user_context/index.ts Outdated Show resolved Hide resolved

packages/optimizely-sdk/lib/optimizely_user_context/index.ts Outdated Show resolved Hide resolved

packages/optimizely-sdk/lib/optimizely_user_context/index.ts Outdated Show resolved Hide resolved

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ForcedDecisions): Add forced-decisions APIs to OptimizelyUserContext #705

feat(ForcedDecisions): Add forced-decisions APIs to OptimizelyUserContext #705

yavorona commented Sep 23, 2021 •

edited

Loading

coveralls commented Sep 23, 2021 •

edited

Loading

jaeopt left a comment

jaeopt left a comment

jaeopt left a comment

jaeopt left a comment

jaeopt Oct 15, 2021

jaeopt Oct 15, 2021

zashraf1985 Oct 18, 2021

yavorona Oct 19, 2021 •

edited

Loading

zashraf1985 left a comment

zashraf1985 Oct 18, 2021

zashraf1985 Oct 18, 2021

zashraf1985 Oct 18, 2021

yavorona Oct 19, 2021

zashraf1985 Oct 18, 2021

zashraf1985 Oct 18, 2021

zashraf1985 Oct 18, 2021

zashraf1985 Oct 18, 2021

zashraf1985 Oct 18, 2021

yavorona Oct 19, 2021

zashraf1985 left a comment

zashraf1985 Oct 27, 2021

jaeopt left a comment

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

feat(ForcedDecisions): Add forced-decisions APIs to OptimizelyUserContext #705

feat(ForcedDecisions): Add forced-decisions APIs to OptimizelyUserContext #705

Conversation

yavorona commented Sep 23, 2021 • edited Loading

Summary

Test plan

coveralls commented Sep 23, 2021 • edited Loading

jaeopt left a comment

Choose a reason for hiding this comment

jaeopt left a comment

Choose a reason for hiding this comment

jaeopt left a comment

Choose a reason for hiding this comment

jaeopt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yavorona Oct 19, 2021 • edited Loading

Choose a reason for hiding this comment

zashraf1985 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zashraf1985 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaeopt left a comment

Choose a reason for hiding this comment

yavorona commented Sep 23, 2021 •

edited

Loading

coveralls commented Sep 23, 2021 •

edited

Loading

yavorona Oct 19, 2021 •

edited

Loading