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

mnoman09 · 2021-06-30T17:07:31Z

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

OptimizelySDK/OptlyConfig/OptimizelyConfigService.cs

msohailhussain · 2021-07-12T19:48:15Z

OptimizelySDK/OptlyConfig/OptimizelyConfigService.cs

            var experimentMap = GetExperimentsMap(projectConfig);
            var featureMap = GetFeaturesMap(projectConfig, experimentMap);
            OptimizelyConfig = new OptimizelyConfig(projectConfig.Revision,
                projectConfig.SDKKey,
                projectConfig.EnvironmentKey,
+                attributes,


won't this break existing constructor?

No actually attributes sdkKey and env key are all in the new constructor.

OptimizelySDK/OptlyConfig/OptimizelyConfigService.cs

msohailhussain · 2021-07-12T19:49:49Z

OptimizelySDK/OptlyConfig/OptimizelyConfigService.cs

+
+        private Audience[] GetAudiences(ProjectConfig projectConfig)
+        {
+            var audiencesArr = Array.FindAll(projectConfig.Audiences, aud => !aud.Id.Equals("$opt_dummy_audience"));


am just thinking, can't we avoid this condition? may be we should remove somewhere else.

msohailhussain · 2021-07-12T19:55:10Z

OptimizelySDK/OptlyConfig/OptimizelyConfigService.cs

            var featureId = projectConfig.GetExperimentFeatureList(experimentId)?.FirstOrDefault();
            var featureIdVariablesMap = GetFeatureIdVariablesMap(projectConfig);
            var variablesMap = new Dictionary<string, OptimizelyVariable>();
+            string featureIdRollout = null;
+            if (rolloutId != null)


lets discuss offline, need to discuss this logic.

I think you add rolloutId checking since "GetExperimentFeatureList" returns a map for experiment-to-feature map not for delivery-rule-to-feature map. What about extend "GetExperimentFeatureList" or similar to cover delivery-rules as well and removing "rolloutId" passing around.

msohailhussain · 2021-07-12T19:58:08Z

OptimizelySDK/OptlyConfig/OptimizelyConfigService.cs

+                    if (item is JArray)
+                    {
+                        subAudience = GetSerializedAudiences(((JArray)item).ToObject<List<object>>(), audienceIdMap);
+                        subAudience = "(" + subAudience + ")";


use StringBuilder.

use append where you are concatenating strings.

OptimizelySDK/OptlyConfig/OptimizelyConfigService.cs

OptimizelySDK.Tests/OptimizelyConfigTests/OptimizelyConfigTest.cs

Added comments

keppel2 · 2021-07-13T17:51:28Z

OptimizelySDK/OptlyConfig/OptimizelyConfig.cs

        public IDictionary<string, OptimizelyExperiment> ExperimentsMap { get; private set; }
        public IDictionary<string, OptimizelyFeature> FeaturesMap { get; private set; }

        private string _datafile;
-        
+


If we have auto formatted code, we won't have to make these kinds of fixes.

OptimizelySDK/OptlyConfig/OptimizelyExperiment.cs

OptimizelySDK/OptlyConfig/OptimizelyConfigService.cs

OptimizelySDK/OptlyConfig/OptimizelyConfig.cs

jaeopt

A few changes and more tests suggested

jaeopt · 2021-07-22T18:14:26Z

OptimizelySDK/OptlyConfig/OptimizelyConfig.cs

+        public Entity.Event[] Events { get; private set; }
+        public OptimizelyAudience[] Audiences { get; private set; }
+        public Attribute[] Attributes { get; private set; }


Why don't we add new public types OptimzelyEvent and OptimizelyAttribute? I understand content-wise they currently look identical, but those are types that we'll open to our customers.

OptimizelySDK.Net35/OptimizelySDK.Net35.csproj

OptimizelySDK.Tests/OptimizelyConfigTests/OptimizelyConfigTest.cs

jaeopt · 2021-07-26T16:42:14Z

OptimizelySDK/OptlyConfig/OptimizelyConfig.cs

        {
            Revision = revision;
            SDKKey = sdkKey;
+            Attributes = attributes;
+            Audiences = audiences;
+            Events = events;
            EnvironmentKey = environmentKey;
            ExperimentsMap = experimentsMap;
            FeaturesMap = featuresMap;


Do we need to have these multiple constructors? This is for creating OptimizelyConfig internally.

These are publicly exposed classes, that's why we added another constructor.

It's expected that clients use OptimizelyConfig object, not building it. Consider not exposing constructor to public if can be controlled.

jaeopt · 2021-07-26T18:20:51Z

OptimizelySDK/OptlyConfig/OptimizelyConfigService.cs

            var featureId = projectConfig.GetExperimentFeatureList(experimentId)?.FirstOrDefault();
            var featureIdVariablesMap = GetFeatureIdVariablesMap(projectConfig);
            var variablesMap = new Dictionary<string, OptimizelyVariable>();
+            string featureIdRollout = null;
+            if (rolloutId != null)


I think you add rolloutId checking since "GetExperimentFeatureList" returns a map for experiment-to-feature map not for delivery-rule-to-feature map. What about extend "GetExperimentFeatureList" or similar to cover delivery-rules as well and removing "rolloutId" passing around.

jaeopt · 2021-07-26T18:21:31Z

OptimizelySDK/OptlyConfig/OptimizelyConfigService.cs


+            featureId = featureId ?? featureIdRollout;


I think this is to cover features which are mapped to delivery-rules. See my comments above.

jaeopt · 2021-07-27T15:59:51Z

OptimizelySDK.Tests/OptimizelyConfigTests/OptimizelyConfigTest.cs

@@ -189,6 +125,7 @@ public void TestGetOptimizelyConfigService()
                    "audience_combinations_experiment", new OptimizelyExperiment (
                        id: "1323241598",
                        key:"audience_combinations_experiment",
+                        audiences: "(\"exactString\" OR \"substringString\") AND (\"exists\" OR \"exactNumber\" OR \"gtNumber\" OR \"ltNumber\" OR \"exactBoolean\")",


Can we add separate test cases to cover 13 cases for audience serialization in TDD?

jaeopt · 2021-07-27T16:12:43Z

OptimizelySDK.Tests/OptimizelyConfigTests/OptimizelyConfigTest.cs

+                    new OptimizelyAudience("3468206647", "gtNumber", new object[] { "and", new object[] { "or", new object[] { "or", new Dictionary<string, object>() { { "name", "lasers" }, { "type", "custom_attribute" }, { "match", "gt" }, { "value", 70 } } } } }),
+                    new OptimizelyAudience("3468206644", "ltNumber", new object[] { "and", new object[] { "or", new object[] { "or", new Dictionary<string, object>() { { "name", "lasers" }, { "type", "custom_attribute" }, { "match", "lt" }, { "value", 1.0 } } } } }),
+                    new OptimizelyAudience("3468206643", "exactBoolean", new object[] { "and", new object[] { "or", new object[] { "or", new Dictionary<string, object>() { { "name", "should_do_it" }, { "type", "custom_attribute" }, { "match", "exact" }, { "value", true } } } } }),
+                    new OptimizelyAudience("3468206648", "notExist", new object[] { "not", new Dictionary<string, object>() { { "name", "input_value" }, { "type", "custom_attribute" }, { "match", "exists" } } }),


Also need validate "$opt_dummy_audience" audience is filtered out. See TDD.

jaeopt · 2021-07-27T16:19:44Z

OptimizelySDK.Tests/OptimizelyConfigTests/OptimizelyConfigTest.cs

+                    new OptimizelyAudience("3468206644", "$$dummyLtNumber", "{\"type\": \"custom_attribute\", \"name\": \"$opt_dummy_attribute\", \"value\": \"impossible_value\"}"),
+                    new OptimizelyAudience("3468206643", "$$dummyExactBoolean", "{\"type\": \"custom_attribute\", \"name\": \"$opt_dummy_attribute\", \"value\": \"impossible_value\"}"),
+                    new OptimizelyAudience("0", "$$dummy", "{\"type\": \"custom_attribute\", \"name\": \"$opt_dummy_attribute\", \"value\": \"impossible_value\"}"),
+                    new OptimizelyAudience("3988293898", "substringString", new object[] { "and", new object[] { "or", new object[] { "or", new Dictionary<string, string>() { { "name", "house" }, { "type", "custom_attribute" }, { "match", "substring" }, { "value", "Slytherin" } } } } }),


Can we modify tests to validate that typedAudiences and audiences filter out same "names"?

msohailhussain · 2021-07-27T20:05:55Z

OptimizelySDK/OptlyConfig/OptimizelyConfigService.cs

+                        {
+                            cond = string.IsNullOrEmpty(cond) ? "OR" : cond;
+                            sAudience = string.IsNullOrEmpty(sAudience.ToString()) ? new StringBuilder(cond + " \"" + audienceIdMap[itemStr]?.Name + "\"") :
+                                        sAudience.Append(" " + cond + " \"" + audienceIdMap[itemStr]?.Name + "\"");


msohailhussain · 2021-07-27T20:06:01Z

OptimizelySDK/OptlyConfig/OptimizelyConfigService.cs

+                        }
+                        else
+                        {
+                            sAudience = new StringBuilder("\"" + audienceIdMap[itemStr]?.Name + "\"");


or use .format or use interpolation string.

…nored in getOptimizelyconfig

jaeopt

Looks good. Suggested to add 2 more test cases for audience serialization.

jaeopt · 2021-07-29T23:27:40Z

OptimizelySDK.Tests/OptimizelyConfigTests/OptimizelyConfigTest.cs

+                new List<object>() { "and", new JArray() { "or", "3468206642", "3988293898" }, "3468206646" },
+                new List<object>() { "and", new JArray() { "or", "3468206642", new JArray() { "and", "3988293898", "3468206646" } }, new JArray() { "and", "3988293899", new JArray() { "or", "3468206647", "3468206643" } } },
+                new List<object>() { "and", "and" },
+                new List<object>() { "not", new JArray() { "and", "3468206642", "3988293898" } }


Let's add 2 more cases from TDD:

empty audiences []

not match id: ["or", "1", "999999999"]

Serialized condition of typedAudience for Optimizelyconfig if key of audience does not exist in map then use the given id as it is. Added unit test of audience as empty and audienceId does not exist

msohailhussain · 2021-08-02T20:03:58Z

FSC passing here.
https://app.travis-ci.com/github/optimizely/fullstack-sdk-compatibility-suite/jobs/528578890

msohailhussain

lgtm

jaeopt

LGTM

Added new Optimizely config fields

98dcba5

optimizely-codereview-bot assigned mnoman09 Jun 30, 2021

mnoman09 added 3 commits July 2, 2021 22:47

Added delivery rules

41e684b

Added optimizelyAudience and rollout featureVariables fix

c6ad489

Added new line at end

9973794

mnoman09 changed the title ~~WIP: Added new Optimizely config fields (DO NOT REVIEW)~~ feat(OptimizelyConfig): Add new fields to OptimizelyConfig Jul 8, 2021

mnoman09 added 2 commits July 8, 2021 22:26

Added audiences and fixed tests

665c553

Net 3.5 deserializing fix

3c04342

msohailhussain marked this pull request as ready for review July 12, 2021 19:44

msohailhussain requested a review from a team as a code owner July 12, 2021 19:44

msohailhussain reviewed Jul 12, 2021

View reviewed changes

OptimizelySDK/OptlyConfig/OptimizelyConfigService.cs Outdated Show resolved Hide resolved

msohailhussain reviewed Jul 12, 2021

View reviewed changes

OptimizelySDK/OptlyConfig/OptimizelyConfigService.cs Outdated Show resolved Hide resolved

msohailhussain reviewed Jul 12, 2021

View reviewed changes

OptimizelySDK/OptlyConfig/OptimizelyConfigService.cs Outdated Show resolved Hide resolved

msohailhussain reviewed Jul 12, 2021

View reviewed changes

OptimizelySDK/OptlyConfig/OptimizelyConfigService.cs Show resolved Hide resolved

msohailhussain reviewed Jul 12, 2021

View reviewed changes

OptimizelySDK/OptlyConfig/OptimizelyConfigService.cs Outdated Show resolved Hide resolved

msohailhussain reviewed Jul 12, 2021

View reviewed changes

OptimizelySDK/OptlyConfig/OptimizelyConfigService.cs Outdated Show resolved Hide resolved

msohailhussain reviewed Jul 12, 2021

View reviewed changes

OptimizelySDK.Tests/OptimizelyConfigTests/OptimizelyConfigTest.cs Outdated Show resolved Hide resolved

mnoman09 added 3 commits July 13, 2021 19:39

Used string builder instead of string

1c0bf1a

Added comments

Added OptimizelyProjectConfigTest file in tests

ef6218b

Moved getEvent After GetFeaturesMap

9fc9d54

mnoman09 requested a review from msohailhussain July 13, 2021 14:52

mnoman09 removed their assignment Jul 13, 2021

keppel2 approved these changes Jul 13, 2021

View reviewed changes

msohailhussain reviewed Jul 13, 2021

View reviewed changes

OptimizelySDK/OptlyConfig/OptimizelyConfigService.cs Outdated Show resolved Hide resolved

Resolved comments and refactored logic

53fbc69

msohailhussain requested a review from jaeopt July 21, 2021 23:36

dustin-sier reviewed Jul 26, 2021

View reviewed changes

OptimizelySDK/OptlyConfig/OptimizelyConfig.cs Outdated Show resolved Hide resolved

mnoman09 added 4 commits July 27, 2021 10:03

removed two additional tests for testing

9ace97f

Merge branch 'master' into mnoman/optimizelyConfigNewFields

621e62c

Audience array merge fix Unit test fix

ce2505b

Condition simplified

b362593

jaeopt requested changes Jul 27, 2021

View reviewed changes

optimizely-codereview-bot assigned mnoman09 Jul 27, 2021

Added OptimizelyEvent and OptimizelyAttribute Class

cb47e3c

msohailhussain reviewed Jul 27, 2021

View reviewed changes

mnoman09 and others added 4 commits July 28, 2021 16:03

Added $opt_dummy_audience in datafile to verify that it is getting ig…

8ba7fe5

…nored in getOptimizelyconfig

Added serializedAudiences Tests cases

62ed97f

refactored little bit.

c685c61

Refactored comments

7351048

msohailhussain unassigned mnoman09 Jul 29, 2021

optimizely-codereview-bot requested review from dustin-sier, jaeopt and msohailhussain July 29, 2021 19:13

added unit tests for multiple experiment and removed sdk and env key.

f1e684f

jaeopt requested changes Jul 29, 2021

View reviewed changes

optimizely-codereview-bot assigned mnoman09 Jul 29, 2021

mnoman09 and others added 2 commits July 30, 2021 14:13

Setting sdkKey and environment key default value as empty string

b5d3383

Serialized condition of typedAudience for Optimizelyconfig if key of audience does not exist in map then use the given id as it is. Added unit test of audience as empty and audienceId does not exist

fixed sorting issue

1e9d40f

msohailhussain approved these changes Aug 2, 2021

View reviewed changes

jaeopt approved these changes Aug 2, 2021

View reviewed changes

corrected sequence of audiences

6a8dafa

jaeopt merged commit db932a5 into master Aug 2, 2021

jaeopt deleted the mnoman/optimizelyConfigNewFields branch August 2, 2021 22:55

yasirfolio3 mentioned this pull request Aug 3, 2021

Config v2 branch shujat333/php-sdk#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mnoman09 commented Jun 30, 2021 •

edited

Loading

msohailhussain Jul 12, 2021

mnoman09 Jul 13, 2021

msohailhussain Jul 12, 2021

msohailhussain Jul 12, 2021

jaeopt Jul 26, 2021

msohailhussain Jul 12, 2021

msohailhussain Jul 22, 2021

keppel2 Jul 13, 2021

jaeopt left a comment

jaeopt Jul 22, 2021

jaeopt Jul 26, 2021

msohailhussain Jul 27, 2021

jaeopt Jul 29, 2021

jaeopt Jul 26, 2021

jaeopt Jul 26, 2021

jaeopt Jul 27, 2021

jaeopt Jul 27, 2021

jaeopt Jul 27, 2021

msohailhussain Jul 27, 2021

msohailhussain Jul 27, 2021

msohailhussain Jul 27, 2021

jaeopt left a comment

jaeopt Jul 29, 2021

msohailhussain commented Aug 2, 2021

msohailhussain left a comment

jaeopt left a comment

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

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

Conversation

mnoman09 commented Jun 30, 2021 • edited Loading

Summary

Test plan

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

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

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

jaeopt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msohailhussain commented Aug 2, 2021

msohailhussain left a comment

Choose a reason for hiding this comment

jaeopt left a comment

Choose a reason for hiding this comment

mnoman09 commented Jun 30, 2021 •

edited

Loading