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 #451

Merged
merged 43 commits into from
Nov 4, 2021
Merged

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

merged 43 commits into from
Nov 4, 2021

Conversation

The-inside-man
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

The-inside-man and others added 23 commits September 28, 2021 12:22
@The-inside-man
Addition of ForcedDecisions
cdaa2e0
@The-inside-man
Update constructor to use ? instead of object - reverting
bd41dd1
@The-inside-man
Update constructor to use ? instead of object - reverting
25dfa07
@The-inside-man
Merge branch 'master' into jbrown/forcedDecisions
35ff0ce
@The-inside-man
Update dodgy tests.
00fbc21
@The-inside-man
Add additional test to OptimizelyTest.java
9079018
@The-inside-man
Add tests to OptimizelyUserContextTest.java
f75c04a
@The-inside-man
Update tests for more coverage.
225cd1e
@The-inside-man
Revert javadoc comment test.
2a47e23
@The-inside-man
Update javadocs with titles.
881f802
@The-inside-man
Update deprecations.
5cec109
@The-inside-man
Remove deprecations for set and get forcedVariations.
4964852
@The-inside-man
Simplify the way flagVariationsMap is built
31de5d5
@The-inside-man
Fix removeForcedDecision when no object in map
34cc766
@The-inside-man
cleanup and remove deprecation warnings.
65fe3ad
@The-inside-man
Change forcedDecisions to use a map of Strings to Strings based on a …
8f03cf3 
…new class OptimizelyForcedDecisionsKey which uses the toString to build a key based on a flagKey and ruleKey, where rulekey, if null wil be the string equivelant and then all converted to its hashcode string equivelant.
@The-inside-man
Add license header
a968525
@The-inside-man
Dont append ruleKey to key object toString method if null.
3e415ca
@The-inside-man
Update logic in decide method.
06b12b0
@The-inside-man
Merge branch 'master' into jbrown/forcedDecisions
bf8a6c7
@The-inside-man
Change logic to decide and adjust logic for getVariationForFeature.
1d416c8
@The-inside-man
Update logic for forcedDecisions, add in missing methods for Decision…
443830c 
…Service.java.
@The-inside-man
Update coverage and remove unused class
2161c00
@coveralls
Copy link

coveralls commented Oct 14, 2021 •
edited
Loading

Pull Request Test Coverage Report for Build 1897

  • 207 of 214 (96.73%) changed or added relevant lines in 7 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 90.715%
Changes Missing Coverage Covered Lines Changed/Added Lines %
core-api/src/main/java/com/optimizely/ab/Optimizely.java 21 23 91.3%
core-api/src/main/java/com/optimizely/ab/OptimizelyUserContext.java 53 58 91.38%
Totals Coverage Status
Change from base Build 1862: 0.2%
Covered Lines: 4875
Relevant Lines: 5374
💛 - Coveralls

@The-inside-man The-inside-man removed their assignment Oct 14, 2021
@The-inside-man The-inside-man marked this pull request as ready for review October 14, 2021 17:21
@The-inside-man The-inside-man requested a review from a team as a code owner October 14, 2021 17:21
@The-inside-man
Revert to Map for now.
d0aced2
@The-inside-man
Update OptimizelyUserContext to copy FD, update DecisionService to us…
8bf6faf 
…er SimpleEntry from AbstractMap instead of Pair since no longer supported in Java 11
@The-inside-man
Updated tests and license headers
66cb22f
@The-inside-man
Added test case to confirm no duplicate variations added to the falgs…
743f913 
… variations map per flag and changed implementation to use a Map using variation.id as the key to ensure.
@The-inside-man
Update tests to cover decide with FD and confirm usage.
80b7fb0
@jaeopt jaeopt closed this Nov 1, 2021
@jaeopt jaeopt reopened this Nov 1, 2021
jaeopt
jaeopt requested changes Nov 1, 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 the changes look great!
I suggested a couple of more changes and adding tests for events/notifications.

core-api/src/main/java/com/optimizely/ab/OptimizelyUserContext.java Show resolved Hide resolved
core-api/src/main/java/com/optimizely/ab/OptimizelyUserContext.java Outdated Show resolved Hide resolved
core-api/src/test/java/com/optimizely/ab/OptimizelyUserContextTest.java Show resolved Hide resolved
jaeopt
jaeopt requested changes Nov 1, 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.

One more request

core-api/src/main/java/com/optimizely/ab/OptimizelyDecisionContext.java Show resolved Hide resolved
@The-inside-man The-inside-man requested a review from jaeopt November 3, 2021 14:04
@The-inside-man The-inside-man removed their assignment Nov 3, 2021
jaeopt
jaeopt requested changes Nov 3, 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.

Looks good! One more comment.

core-api/src/main/java/com/optimizely/ab/OptimizelyUserContext.java Outdated Show resolved Hide resolved
@The-inside-man
Copy link
Contributor Author

Looks good! One more comment.

Roger! Committed the change!

jaeopt
jaeopt approved these changes Nov 3, 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 with a nit

core-api/src/main/java/com/optimizely/ab/OptimizelyUserContext.java Outdated Show resolved Hide resolved
dustin-sier
dustin-sier approved these changes Nov 4, 2021
View reviewed changes
Copy link

@dustin-sier dustin-sier left a comment

Choose a reason for hiding this comment

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

LGTM!

@The-inside-man The-inside-man merged commit 04d379a into master Nov 4, 2021
@The-inside-man The-inside-man deleted the jbrown/forcedDecisions branch November 4, 2021 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dustin-sier dustin-sier dustin-sier approved these changes

@jaeopt jaeopt jaeopt approved these changes

Assignees

@The-inside-man The-inside-man

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@The-inside-man @coveralls @jaeopt @dustin-sier @optimizely-codereview-bot