Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Added GetFeatureVariableJSON. #216

Merged
merged 29 commits into from
May 18, 2020

Conversation

mnoman09
Copy link
Contributor

@mnoman09 mnoman09 commented May 7, 2020

Summary

  • Added GetFeatureVariableJson api in optimizely

Test plan

  • Added Unit tests.

Copy link
Contributor

@msohailhussain msohailhussain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please address.

mnoman09 added 2 commits May 11, 2020 20:09
# Conflicts:
#	OptimizelySDK.Tests/OptimizelyJsonTest.cs
@mnoman09 mnoman09 changed the base branch from mnoman/optimizelyJson to master May 11, 2020 15:16
@mnoman09 mnoman09 removed their assignment May 12, 2020
@mnoman09 mnoman09 requested a review from msohailhussain May 12, 2020 14:34
@@ -1624,7 +1704,7 @@ public void TestUnsupportedVariableType()
Assert.IsNull(featureVariableStringRandomType);

var featureVariableStringJsonType = Optimizely.GetFeatureVariableString("unsupported_variabletype", "string_json_key", TestUserId);
Assert.AreEqual(featureVariableStringJsonType, "{\"myvalue\": \"jsonValue\"}");
Assert.IsNull(featureVariableStringJsonType);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why value is changed to IsNULL

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because the subtype of that variable is json and we are changing type from string to json upon initialization. so it will not match and will return null as type of variable will not match

Copy link
Contributor

@msohailhussain msohailhussain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

requested some changes. otherwise looks good.

@mnoman09 mnoman09 self-assigned this May 12, 2020
@mnoman09 mnoman09 requested a review from msohailhussain May 12, 2020 17:13
@mnoman09 mnoman09 removed their assignment May 12, 2020
Copy link
Contributor

@msohailhussain msohailhussain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link

@pawels-optimizely pawels-optimizely left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are there any tests with notification listener ?

Copy link
Contributor

@zashraf1985 zashraf1985 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - Good work

@@ -589,6 +593,13 @@
"key": "string_variable",
"type": "string",
"defaultValue": "wingardium leviosa"
},
Copy link

@pawels-optimizely pawels-optimizely May 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since C# had a problem before with json type, please add

{
					"defaultValue": "{\"int_var\": 5212, \"boolean_key\": true}",
					"type": "json",
					"id": "1",
					"key": "true_json_var"
				}

and test

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the test you asked for. So can you please review if this is what you asked for? link

Copy link

@pawels-optimizely pawels-optimizely left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, ok to merge after one more test I asked

Copy link
Contributor

@msohailhussain msohailhussain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link

@pawels-optimizely pawels-optimizely left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@pawels-optimizely pawels-optimizely merged commit fb8a40f into master May 18, 2020
@mnoman09 mnoman09 deleted the mnoman/GetFeatureVariableJson branch January 6, 2021 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants