feat(OptimizelyConfig): Add new fields to OptimizelyConfig #698

yasirfolio3 · 2021-08-11T06:12:06Z

Summary

The following new public properties are added to OptimizelyConfig:

sdkKey
environmentKey
attributes
audiences
events
experimentRules and deliveryRules to OptimizelyFeature
audiences to OptimizelyExperiment

Test plan

All FSC tests OPTIMIZELY_CONFIG_V2 tests should pass.
FSC OptimizelyConfig link

coveralls · 2021-08-11T06:23:46Z

Coverage increased (+0.02%) to 97.002% when pulling 47706af on yasir/config-v2 into a37cfb2 on master.

keppel2 · 2021-08-11T06:32:53Z

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


  constructor(configObj: ProjectConfig, datafile: string) {
-    this.experimentsMap = OptimizelyConfig.getExperimentsMap(configObj);
-    this.featuresMap = OptimizelyConfig.getFeaturesMap(configObj, this.experimentsMap);
+    this.sdkKey = configObj.sdkKey ? configObj.sdkKey : '';


You can use configObj.sdkKey ?? ''; if you want.

keppel2 · 2021-08-11T06:49:58Z

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

+      let cond = '';
+      conditions.forEach((item) => {
+        let subAudience = '';
+        // Checks if item is list of conditions means if it is sub audience


// Checks if item is list of conditions means it is sub audience

zashraf1985 · 2021-08-11T14:26:09Z

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

+  Audience,
+  Experiment,
+  FeatureVariable,
  OptimizelyExperimentsMap,
  OptimizelyFeaturesMap,
  OptimizelyVariablesMap,
-  FeatureVariable,
+  OptimizelyAttribute,
+  OptimizelyAudience,
+  OptimizelyEvent,
+  OptimizelyExperiment,
+  OptimizelyVariable,
+  OptimizelyVariation,
+  Rollout,
  VariationVariable,
  Variation,
-  Rollout,


NIT: arrange these in alphabetical order

zashraf1985 · 2021-08-11T14:26:57Z

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

  private datafile: string;
+  private static audienceConditionalOperators: string[] = ['and', 'or', 'not'];


This can probably be moved to ENUMS?

zashraf1985 · 2021-08-11T14:27:12Z

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


  constructor(configObj: ProjectConfig, datafile: string) {
-    this.experimentsMap = OptimizelyConfig.getExperimentsMap(configObj);
-    this.featuresMap = OptimizelyConfig.getFeaturesMap(configObj, this.experimentsMap);
+    this.sdkKey = configObj.sdkKey ? configObj.sdkKey : '';


Suggested change

this.sdkKey = configObj.sdkKey ? configObj.sdkKey : '';

this.sdkKey = configObj.sdkKey || '';

zashraf1985 · 2021-08-11T14:27:45Z

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

-    this.experimentsMap = OptimizelyConfig.getExperimentsMap(configObj);
-    this.featuresMap = OptimizelyConfig.getFeaturesMap(configObj, this.experimentsMap);
+    this.sdkKey = configObj.sdkKey ? configObj.sdkKey : '';
+    this.environmentKey = configObj.environmentKey ? configObj.environmentKey : '';


Suggested change

this.environmentKey = configObj.environmentKey ? configObj.environmentKey : '';

this.environmentKey = configObj.environmentKey || '';

zashraf1985 · 2021-08-11T14:30:03Z

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

+   * @returns     {OptimizelyAudience[]}       Array of unique audiences
+   */
+  static getAudiences(configObj: ProjectConfig): OptimizelyAudience[] {
+    const finalAudiences: OptimizelyAudience[] = [];


why call it finalAudiences? can you simply call it audiences

zashraf1985 · 2021-08-11T14:40:42Z

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

+        if (item instanceof Array) {
+          subAudience = OptimizelyConfig.GetSerializedAudiences(item, audiencesById);
+          subAudience = `(${subAudience})`;
+        } else if (OptimizelyConfig.audienceConditionalOperators.includes(item)) {
+          cond = item.toUpperCase();
+        } else {
+          // Checks if item is audience id
+          const audienceName = audiencesById[item] ? audiencesById[item].name : item;
+          // if audience condition is "NOT" then add "NOT" at start. Otherwise check if there is already audience id in serializedAudience then append condition between serializedAudience and item
+          if (serializedAudience || cond === 'NOT') {
+            cond = cond === '' ? 'OR' : cond;
+            if (serializedAudience === '') {
+              serializedAudience = `${cond} "${audiencesById[item].name}"`;
+            } else {
+              serializedAudience = serializedAudience.concat(` ${cond} "${audienceName}"`);
+            }
+          } else {
+            serializedAudience = `"${audienceName}"`;
+          }
+        } 
+        // Checks if sub audience is empty or not
+        if (subAudience !== '') {
+          if (serializedAudience !== '' || cond === 'NOT') {
+            cond = cond === '' ? 'OR' : cond;
+            if (serializedAudience === '') {
+              serializedAudience = `${cond} ${subAudience}`;
+            } else {
+              serializedAudience = serializedAudience.concat(` ${cond} ${subAudience}`);
+            }
+          } else {
+            serializedAudience = serializedAudience.concat(subAudience);
+          }
+        }
+      });
+    }
+    return serializedAudience;


NOT reviewing this code. will leave it to @msohailhussain and @jaeopt as they have more context about this particular peice

zashraf1985 · 2021-08-11T14:42:57Z

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

+
+    const variableIdMap = OptimizelyConfig.getVariableIdMap(configObj);
+
+    experiments.forEach((experiment) => {


you could use map here and assigned the result directly to deliveryRules?

zashraf1985 · 2021-08-11T14:43:47Z

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

-      },
-      {},
-    )
+    experiments.forEach((experiment) => {


could do reduce here probably?

zashraf1985 · 2021-08-11T14:45:00Z

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

-    datafileCopy.environmentKey = datafile.environmentKey;
-    datafileCopy.sdkKey = datafile.sdkKey;
-  }
+  datafileCopy.environmentKey = datafile.environmentKey ? datafile.environmentKey : '';


Suggested change

datafileCopy.environmentKey = datafile.environmentKey ? datafile.environmentKey : '';

datafileCopy.environmentKey = datafile.environmentKey || '';

zashraf1985 · 2021-08-11T14:45:13Z

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

-    datafileCopy.sdkKey = datafile.sdkKey;
-  }
+  datafileCopy.environmentKey = datafile.environmentKey ? datafile.environmentKey : '';
+  datafileCopy.sdkKey = datafile.sdkKey ? datafile.sdkKey : '';


Suggested change

datafileCopy.sdkKey = datafile.sdkKey ? datafile.sdkKey : '';

datafileCopy.sdkKey = datafile.sdkKey || '';

yavorona · 2021-08-11T22:04:35Z

packages/optimizely-sdk/lib/shared_types.ts

+export type OptimizelyAudience = {
+  id: string;
+  name: string;
+  conditions: unknown[] | string;


It appears that we expect conditions type here to just be string since we JSON.stringify the typedAudience/oldAudience conditions

yavorona · 2021-08-11T22:19:35Z

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

+   * @param       {string}                                featureId
+   * @returns     {[key: string]: Variation}              Variations mapped by key
+   */
+  static GetVariationsMap(


Why this method is also capitalized?

yavorona · 2021-08-11T22:19:46Z

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

+   * @param       {[id: string]: Audience}                   audiencesById
+   * @returns     {string}                                   Serialized audiences condition string
+   */
+  static GetSerializedAudiences(


Why are we capitalizing this method?

yavorona · 2021-08-11T23:05:53Z

packages/optimizely-sdk/lib/shared_types.ts

@@ -248,6 +249,7 @@ export interface OptimizelyOptions {
 export interface OptimizelyExperiment {
  id: string;
  key: string;
+  audiences: unknown[] | string;


Should just be string

yavorona · 2021-08-11T23:17:28Z

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

+    configObj: ProjectConfig,
+    featureVariableIdMap: FeatureVariablesMap,
+    featureId: string,
+    experiments: Experiment[] | undefined


This should not be expected undefined as every rollout object should contain experiments. @msohailhussain please correct if I am wrong.

yavorona · 2021-08-11T23:17:59Z

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

+    experiments: Experiment[] | undefined
+  ): OptimizelyExperiment[] {
+    const deliveryRules: OptimizelyExperiment[] = [];
+    if (!experiments) {


If above is true, this if statement is unnecessary.

yavorona · 2021-08-11T23:25:35Z

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

+    isFeatureEnabled: boolean | undefined
+  ): OptimizelyVariablesMap {
+    let variablesMap: OptimizelyVariablesMap = {};
+    if (!featureId) {


This statement seems unnecessary as we should expect a valid featureId, right?

msohailhussain

please address

msohailhussain · 2021-08-11T23:34:53Z

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

+   */
+  static getAudiences(configObj: ProjectConfig): OptimizelyAudience[] {
+    const finalAudiences: OptimizelyAudience[] = [];
+    const typedAudienceIds: { [id: string]: boolean } = {};


i agree. there is one more existing code that's using the same kv map, we should use .includes and only use Ids arrays.

msohailhussain · 2021-08-11T23:40:19Z

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

@@ -69,150 +332,122 @@ export class OptimizelyConfig {
  static getRolloutExperimentIds(rollouts: Rollout[]): { [key: string]: boolean } {
    return (rollouts || []).reduce((experimentIds: { [key: string]: boolean }, rollout) => {
      rollout.experiments.forEach((e) => {
-        (experimentIds)[e.id] = true;
+        experimentIds[e.id] = true;


same comment, use .includes more cleaner.

yavorona

Looks good except for couple of minor comments and failing browser stack tests.

yavorona · 2021-08-12T21:27:32Z

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

+    featureId: string
+  ): { [key: string]: Variation } {
+    let variationsMap: { [key: string]: OptimizelyVariation } = {};
+    variationsMap = variations.reduce((OptlyVariationsMap: { [key: string]: OptimizelyVariation }, variation) => {


optlyVariationsMap instead of OptlyVariationsMap

yavorona · 2021-08-12T21:29:32Z

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

+    isFeatureEnabled: boolean | undefined
+  ): OptimizelyVariablesMap {
+    const variablesMap = (featureIdVariableMap[featureId] || []).reduce(
+      (OptlyVariablesMap: OptimizelyVariablesMap, featureVariable) => {


optlyVariablesMap instead of OptlyVariablesMap

jaeopt

Overall looks great!

One comment about using OptimizelyConfig types internally.
Also I do not see duplicate-key warning about experimentsMap in OptimizelyConfig and deprecation warning about experimentsMap in OptimizelyFeature.

jaeopt · 2021-08-13T16:15:16Z

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

@@ -81,7 +83,7 @@ export interface ProjectConfig {
  groupIdMap: { [id: string]: Group };
  groups: Group[];
  events: Event[];
-  attributes: Array<{ id: string }>;
+  attributes: OptimizelyAttribute[];


I think it's not a good idea to use "OptimizelyAttribute" or other "OptimizelyXYZ" types in here.
Even if they look identical for now, we should separate them as public types only.

makes sense, thanks.

yasirfolio3 · 2021-08-13T18:50:12Z

Overall looks great!

One comment about using OptimizelyConfig types internally.

Also I do not see duplicate-key warning about experimentsMap in OptimizelyConfig and deprecation warning about experimentsMap in OptimizelyFeature.

Had already added for OptimizelyFeature here: https://github.com/optimizely/javascript-sdk/pull/698/files#diff-2f8a7f1ccf360ce99abd03c811c59f9488fa24db24f8578d808e813e75192acdR330

Also added for OptimizelyConfig Interface here but forgot about the class which i have added now. Comment on the interface can be seen here: https://github.com/optimizely/javascript-sdk/pull/698/files#diff-2f8a7f1ccf360ce99abd03c811c59f9488fa24db24f8578d808e813e75192acdR347

jaeopt

LGTM
nit - add a period (.) at the end of "This experimentsMap is for experiments of legacy projects only" in multiple places.

yasirfolio3 and others added 6 commits August 5, 2021 22:15

Initial commit.

c06fe43

Code for condition serialization added.

d313457

fix optimizely experiments map

24fb82a

improvements

8021a97

cleanup and fixes.

dd5d2b0

linting fixed and unit tests added

082e85b

yasirfolio3 requested a review from a team as a code owner August 11, 2021 06:12

optimizely-codereview-bot assigned yasirfolio3 Aug 11, 2021

keppel2 reviewed Aug 11, 2021

View reviewed changes

zashraf1985 suggested changes Aug 11, 2021

View reviewed changes

yavorona self-requested a review August 11, 2021 21:57

yavorona suggested changes Aug 11, 2021

View reviewed changes

msohailhussain suggested changes Aug 12, 2021

View reviewed changes

Suggested changes made.

6fcc79d

yasirfolio3 removed their assignment Aug 12, 2021

optimizely-codereview-bot requested review from keppel2, msohailhussain, yavorona and zashraf1985 August 12, 2021 10:35

yasirfolio3 added 2 commits August 12, 2021 15:37

nits fixed.

16cc895

nit fixed.

5088dbd

optimizely-codereview-bot added the 🤖 4 hrs label Aug 12, 2021

yavorona reviewed Aug 12, 2021

View reviewed changes

zashraf1985 approved these changes Aug 12, 2021

View reviewed changes

suggested changes made.

b05d049

yasirfolio3 requested a review from yavorona August 13, 2021 05:14

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

jaeopt requested changes Aug 13, 2021

View reviewed changes

optimizely-codereview-bot removed the 🤖 8 hrs label Aug 13, 2021

optimizely-codereview-bot assigned yasirfolio3 Aug 13, 2021

suggested changes made.

76603f2

yasirfolio3 requested a review from jaeopt August 13, 2021 18:54

yavorona approved these changes Aug 13, 2021

View reviewed changes

msohailhussain unassigned yasirfolio3 Aug 13, 2021

msohailhussain approved these changes Aug 13, 2021

View reviewed changes

jaeopt approved these changes Aug 13, 2021

View reviewed changes

added period

47706af

msohailhussain merged commit d32c80f into master Aug 13, 2021

msohailhussain deleted the yasir/config-v2 branch August 13, 2021 22:47

		private datafile: string;
		private static audienceConditionalOperators: string[] = ['and', 'or', 'not'];

	this.sdkKey = configObj.sdkKey ? configObj.sdkKey : '';
	this.sdkKey = configObj.sdkKey \|\| '';

	this.environmentKey = configObj.environmentKey ? configObj.environmentKey : '';
	this.environmentKey = configObj.environmentKey \|\| '';


		const variableIdMap = OptimizelyConfig.getVariableIdMap(configObj);

		experiments.forEach((experiment) => {

	datafileCopy.environmentKey = datafile.environmentKey ? datafile.environmentKey : '';
	datafileCopy.environmentKey = datafile.environmentKey \|\| '';

	datafileCopy.sdkKey = datafile.sdkKey ? datafile.sdkKey : '';
	datafileCopy.sdkKey = datafile.sdkKey \|\| '';

feat(OptimizelyConfig): Add new fields to OptimizelyConfig #698

feat(OptimizelyConfig): Add new fields to OptimizelyConfig #698

Uh oh!

Conversation

yasirfolio3 commented Aug 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

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

Uh oh!

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 Aug 11, 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!

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!

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

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

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yasirfolio3 commented Aug 13, 2021

Uh oh!

jaeopt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yasirfolio3 commented Aug 11, 2021 •

edited

Loading

coveralls commented Aug 11, 2021 •

edited

Loading

zashraf1985 Aug 11, 2021 •

edited

Loading