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.

Sign up for GitHub

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

Jump to bottom

feat(ForcedDecisions): add forced-decisions APIs to OptimizelyUserContext #285

Merged
merged 132 commits into from
Dec 7, 2021
Merged

feat(ForcedDecisions): add forced-decisions APIs to OptimizelyUserContext #285

merged 132 commits into from
Dec 7, 2021

Conversation

dustin-sier
Copy link
Contributor

Summary

Add a set of new APIs for forced-decisions to OptimizelyUserContext:

  • setForcedDecision
  • getForcedDecision
  • removeForcedDecision
  • removeAllForcedDecisions

Test plan

  • unit tests for the new APIs
  • FSC tests with new test cases

MiracleManS and others added 19 commits September 24, 2021 10:53
@dustin-sier dustin-sier marked this pull request as ready for review October 14, 2021 17:38
@dustin-sier dustin-sier requested a review from a team as a code owner October 14, 2021 17:38
@dustin-sier dustin-sier removed their assignment Oct 14, 2021
dustin-sier and others added 5 commits November 29, 2021 09:04
@dustin-sier
changedto string.empty
4bca717
@dustin-sier
Updated to handle null
0b669eb
@dustin-sier
copyright fix
6478ed6
@mnoman09 @msohailhussain
Refact: Refactored Forced decision (#287)
d359a0f 
* Refactored and resolved comments given on PR

* some suggested code.

* unit test fixes.

* typo fix

* Removed getVariationFromExperiment as it was extra loop fixed some bugs and other minor issue due to which tests were failing

* Added forcedDecisionStore class

* more refactoring.

* refactored Code

* refactored and removed commented code

* Added forcedDecisionStoreTests

* conditional copy.

* Fixed soure rollout issue

* comment fixed.

Co-authored-by: Sohail Hussain <[email protected]>
@mnoman09
fix(ForcedDecision): remove config-ready check from forced-decision a…
cdf53de 
…pis (#288)
jaeopt
jaeopt requested changes Dec 4, 2021
View reviewed changes
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.

It looks like we still need a few changes.

OptimizelySDK/Event/Builder/EventBuilder.cs Outdated Show resolved Hide resolved
OptimizelySDK/Event/Builder/EventBuilder.cs Show resolved Hide resolved
OptimizelySDK/Event/Builder/EventBuilder.cs Show resolved Hide resolved
OptimizelySDK/OptimizelyUserContext.cs Outdated Show resolved Hide resolved
OptimizelySDK/Bucketing/DecisionService.cs Outdated Show resolved Hide resolved
OptimizelySDK/Bucketing/DecisionService.cs Outdated Show resolved Hide resolved
OptimizelySDK/Bucketing/DecisionService.cs Outdated Show resolved Hide resolved
OptimizelySDK/Bucketing/DecisionService.cs Outdated Show resolved Hide resolved
OptimizelySDK/Bucketing/DecisionService.cs Outdated Show resolved Hide resolved
The-inside-man
The-inside-man suggested changes Dec 4, 2021
View reviewed changes
Copy link

@The-inside-man The-inside-man left a comment

Choose a reason for hiding this comment

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

Just a couple suggestions in addition to Jae's comments.

OptimizelySDK/OptimizelyUserContext.cs Outdated Show resolved Hide resolved
OptimizelySDK/OptimizelyDecisionContext.cs Show resolved Hide resolved
@msohailhussain msohailhussain reopened this Dec 6, 2021
The-inside-man
The-inside-man approved these changes Dec 6, 2021
View reviewed changes
Copy link

@The-inside-man The-inside-man left a comment

Choose a reason for hiding this comment

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

LGTM!

msohailhussain
msohailhussain approved these changes Dec 6, 2021
View reviewed changes
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

@msohailhussain msohailhussain requested a review from jaeopt December 6, 2021 20:11
jaeopt
jaeopt requested changes Dec 6, 2021
View reviewed changes
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. A couple of more changes suggested.

OptimizelySDK/Event/Builder/EventBuilder.cs Outdated Show resolved Hide resolved
OptimizelySDK/Bucketing/DecisionService.cs Show resolved Hide resolved
OptimizelySDK/Optimizely.cs Outdated Show resolved Hide resolved
OptimizelySDK/Optimizely.cs Outdated Show resolved Hide resolved
jaeopt
jaeopt approved these changes Dec 7, 2021
View reviewed changes
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

@msohailhussain msohailhussain reopened this Dec 7, 2021
@msohailhussain msohailhussain merged commit e3fa80f into master Dec 7, 2021
@msohailhussain msohailhussain deleted the dsier/forcedDecisions branch December 7, 2021 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaeopt jaeopt jaeopt approved these changes

@The-inside-man The-inside-man The-inside-man approved these changes

@msohailhussain msohailhussain msohailhussain approved these changes

Assignees

@dustin-sier dustin-sier

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@dustin-sier @The-inside-man @msohailhussain @jaeopt @optimizely-codereview-bot @MiracleManS @dustinsier @mnoman09