feat(OptimizelyConfig): Add new fields to OptimizelyConfig. #322

yasirfolio3 · 2021-08-31T10:19:40Z

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

Dependent PR

https://github.com/optimizely/fullstack-sdk-compatibility-suite/pull/526

Issues

Here's the simple snippet when you will run multiple times, it will give you different result. Maps are randomly picked.

package main

import (
	"fmt"
)

func main() {
	m := map[string]int {
	  "key1": 1,
	  "key6": 6,
	  "key4": 4,
	  "key3": 3,
	  "key7": 7,
	  "key2": 2,					
	}
	
	for key, val := range m {
		fmt.Println(key)
		fmt.Println(val)		
	}
	fmt.Println("Hello, playground")
}

https://stackoverflow.com/questions/55925822/why-are-iterations-over-maps-random

msohailhussain · 2021-08-31T17:12:32Z

pkg/config/datafileprojectconfig/config.go


-	rolloutMap := mappers.MapRollouts(datafile.Rollouts)
+	rollouts, rolloutMap := mappers.MapRollouts(datafile.Rollouts)
+	events := []entities.Event{}


move this code to MapEvents?

msohailhussain · 2021-08-31T17:13:00Z

pkg/config/datafileprojectconfig/config.go

-	featureMap := mappers.MapFeatures(datafile.FeatureFlags, rolloutMap, experimentMap)
+	features, featureMap := mappers.MapFeatures(datafile.FeatureFlags, rolloutMap, experimentMap)
+	attributes := []entities.Attribute{}
+	for _, attribute := range datafile.Attributes {


msohailhussain · 2021-08-31T17:25:20Z

pkg/config/datafileprojectconfig/mappers/condition_trees.go

 var json = jsoniter.ConfigCompatibleWithStandardLibrary

+// GetDefaultOperators returns default conditional operators
+func GetDefaultOperators() []string {


can't you directly assign instead of method.

The-inside-man · 2021-08-31T18:32:11Z

pkg/config/datafileprojectconfig/mappers/audience.go

 			conditionTree, err := buildConditionTree(audience.Conditions)
 			if err != nil {
 				// @TODO: handle error
 				func() {}() // cheat the linters


Should we clean this up maybe, while we are touching the audiences.?

The-inside-man · 2021-08-31T18:38:32Z

pkg/config/optimizely_config.go

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


Should use a constant for the Operands here.

The-inside-man · 2021-08-31T18:40:54Z

pkg/config/optimizely_config.go

+func evaluateSubAudience(subAudience, serializedAudience, cond *string) {
+	// Checks if sub audience is empty or not
+	if *subAudience != "" {
+		if *serializedAudience != "" || *cond == "NOT" {


Same comment here for the operands, then we can reuse the constants.

The-inside-man · 2021-08-31T18:44:13Z

pkg/config/optimizely_config.go

+	if experiment.AudienceConditions == nil {
+		return ""
+	}
+	return getSerializedAudiences(experiment.AudienceConditions, audiencesByID)


if getSerializedAudiences returns an empty string when experiment.AudienceConditions is nil then we dont need the first check here and can just use the getSerializedAudiences function.

pkg/config/datafileprojectconfig/mappers/experiment.go

The-inside-man

Overall LGTM just a few suggestions. Please see comments.

jaeopt

Overall looks great. Some changes suggested.

pkg/config/datafileprojectconfig/config.go

pkg/config/optimizely_config.go

pkg/config/datafileprojectconfig/config.go

dustin-sier

LGTM baring changes discussed

dustin-sier

This will approve it

jaeopt

LGTM with nits

jaeopt · 2021-09-02T16:50:51Z

pkg/config/datafileprojectconfig/mappers/attribute.go

+		tmpAttribute := entities.Attribute{
+			ID:  attribute.ID,
+			Key: attribute.Key,
+		}
 		_, ok := attributeMap[attribute.ID]
 		if !ok {
-			attributeMap[attribute.ID] = entities.Attribute{
-				ID:  attribute.ID,
-				Key: attribute.Key,
-			}
+			attributeMap[attribute.ID] = tmpAttribute


What's the point of this change?

jaeopt · 2021-09-02T17:02:39Z

pkg/config/datafileprojectconfig/mappers/experiment_test.go

 	experimentGroupMap := map[string]string{"11111": "15"}

-	experiments, experimentKeyMap := MapExperiments(rawExperiments, experimentGroupMap)
+	experimentsId, experimentKeyMap := MapExperiments(rawExperiments, experimentGroupMap)


"experimentsId" name misleading. Shouldn't it be experimentMap or experimentIdMap?

jaeopt · 2021-09-02T17:03:29Z

pkg/config/datafileprojectconfig/mappers/experiment_test.go

 	experimentGroupMap := map[string]string{"11111": "15"}

-	experiments, experimentKeyMap := MapExperiments(rawExperiments, experimentGroupMap)
+	experimentsIdMap, experimentKeyMap := MapExperiments(rawExperiments, experimentGroupMap)


experimentIdMap? to be consistent with experimentKeyMap

msohailhussain

lgtm

yasirfolio3 added 4 commits August 27, 2021 19:54

Implementations for config v2.

0d18040

Fixes and unit tests added.

f740d76

cleanup and fixes

a3802de

fixes.

938cb02

optimizely-codereview-bot assigned yasirfolio3 Aug 31, 2021

yasirfolio3 requested a review from msohailhussain August 31, 2021 10:19

msohailhussain reviewed Aug 31, 2021

View reviewed changes

msohailhussain suggested changes Aug 31, 2021

View reviewed changes

msohailhussain unassigned yasirfolio3 Aug 31, 2021

optimizely-codereview-bot requested a review from msohailhussain August 31, 2021 17:28

msohailhussain marked this pull request as ready for review August 31, 2021 17:28

msohailhussain requested a review from a team as a code owner August 31, 2021 17:28

The-inside-man reviewed Aug 31, 2021

View reviewed changes

dustin-sier reviewed Aug 31, 2021

View reviewed changes

pkg/config/datafileprojectconfig/mappers/experiment.go Outdated Show resolved Hide resolved

The-inside-man approved these changes Aug 31, 2021

View reviewed changes

jaeopt requested changes Aug 31, 2021

View reviewed changes

pkg/config/datafileprojectconfig/config.go Show resolved Hide resolved

pkg/config/datafileprojectconfig/config.go Show resolved Hide resolved

pkg/config/datafileprojectconfig/config.go Outdated Show resolved Hide resolved

pkg/config/optimizely_config.go Show resolved Hide resolved

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

msohailhussain reviewed Sep 1, 2021

View reviewed changes

pkg/config/datafileprojectconfig/config.go Show resolved Hide resolved

Suggested changes made.

5a436f2

yasirfolio3 self-assigned this Sep 1, 2021

optimizely-codereview-bot removed the 🤖 4 hrs label Sep 1, 2021

yasirfolio3 added 2 commits September 1, 2021 12:44

Added unit tests to improve coverage.

eb81d24

Added further unit tests to improve coverage.

3c64cc5

msohailhussain requested review from dustin-sier and jaeopt September 1, 2021 17:08

dustin-sier approved these changes Sep 1, 2021 •

edited

Loading

View reviewed changes

dustin-sier requested changes Sep 1, 2021

View reviewed changes

dustin-sier approved these changes Sep 1, 2021

View reviewed changes

Removing lists for audiences, events and attributes.

763ea40

jaeopt approved these changes Sep 2, 2021

View reviewed changes

Suggest changes made.

086624e

msohailhussain closed this Sep 3, 2021

msohailhussain reopened this Sep 3, 2021

msohailhussain approved these changes Sep 3, 2021

View reviewed changes

msohailhussain merged commit 9d97c16 into master Sep 3, 2021

msohailhussain deleted the yasir/config-v2 branch September 3, 2021 18:41

feat(OptimizelyConfig): Add new fields to OptimizelyConfig. #322

feat(OptimizelyConfig): Add new fields to OptimizelyConfig. #322

Uh oh!

Conversation

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

Uh oh!

Summary

Test plan

Dependent PR

Issues

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!

The-inside-man Aug 31, 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!

Uh oh!

The-inside-man left a comment • 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!

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!

dustin-sier left a comment

Choose a reason for hiding this comment

Uh oh!

dustin-sier 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!

msohailhussain left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

yasirfolio3 commented Aug 31, 2021 •

edited

Loading

The-inside-man Aug 31, 2021 •

edited

Loading

The-inside-man left a comment •

edited

Loading