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: Adds Decide All, DecideForKeys and track event #251

Merged
merged 53 commits into from
Dec 9, 2020

Conversation

mnoman09
Copy link
Contributor

Summary

  • Added trackevent in OptimizelyUserContext
  • Added decide for keys api
  • Added decideall api
  • Added unit tests of decide, decideAll, DecideForKeys and trackEvent Apis.

Test plan

  • All Unit tests should pass

BugFix of userAttributes
Unit tests fix
BugFix of userAttributes
Unit tests fix
Added Unit tests of above functions
Throwing not implemented exceptions for not implemented functions in optimizelyUserContext
Added detailed documentation of optimizelyUserContext and Decision class
# Conflicts:
#	OptimizelySDK.Tests/OptimizelyUserContextTest.cs
#	OptimizelySDK/OptimizelyUserContext.cs
# Conflicts:
#	OptimizelySDK/Optimizely.cs
# Conflicts:
#	OptimizelySDK/OptimizelyUserContext.cs
@mnoman09 mnoman09 marked this pull request as ready for review November 23, 2020 15:18
Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

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

All changes look good.
See a couple of more change suggestions.

OptimizelySDK/OptimizelyUserContext.cs Outdated Show resolved Hide resolved
Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

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

LGTM

# Conflicts:
#	OptimizelySDK.Tests/OptimizelyDecisions/OptimizelyDecisionTest.cs
#	OptimizelySDK.Tests/OptimizelyUserContextTest.cs
#	OptimizelySDK/Optimizely.cs
#	OptimizelySDK/OptimizelyDecisions/DefaultDecisionReasons.cs
#	OptimizelySDK/OptimizelyUserContext.cs
private OptimizelyDecideOption[] GetAllOptions(OptimizelyDecideOption[] options)
{
OptimizelyDecideOption[] copiedOptions = new OptimizelyDecideOption[DefaultDecideOptions.Length];
Array.Copy(DefaultDecideOptions, copiedOptions, DefaultDecideOptions.Length);
if (options != null)
{
copiedOptions.Concat(options);
copiedOptions = copiedOptions.Concat(options).Concat(DefaultDecideOptions).ToArray();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please try, it's just an improvement.
return = options.Union(DefaultDecideOptions).ToArray();

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.

just have minor nits. please fix before merging. lgtm

Base automatically changed from mnoman/decideapi to master December 9, 2020 00:29
…DecideOptions).ToArray();

with
                copiedOptions = options.Union(DefaultDecideOptions).ToArray();
# Conflicts:
#	OptimizelySDK.Tests/OptimizelyUserContextTest.cs
#	OptimizelySDK/Optimizely.cs
@jaeopt jaeopt merged commit 4fd3067 into master Dec 9, 2020
@jaeopt jaeopt deleted the mnoman/decideAllApi branch December 9, 2020 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants