feat(ForcedDecisions): add forced-decisions APIs to OptimizelyUserContext#287
feat(ForcedDecisions): add forced-decisions APIs to OptimizelyUserContext#287msohailhussain merged 15 commits intomasterfrom
Conversation
Co-authored-by: yasirfolio3 <39988750+yasirfolio3@users.noreply.github.com>
| @@ -28,6 +28,7 @@ | |||
| let(:spy_user_profile_service) { spy('user_profile_service') } | |||
There was a problem hiding this comment.
I feel we should have new tests related to forced_decision in this test file as well. Mainly tests related to changes in the decision_service.
| expect(original_attributes).to eq('browser' => 'firefox') | ||
| end | ||
| end | ||
|
|
There was a problem hiding this comment.
I feel we need the following test scenarios:
- Async behaviour when setting and getting forced decisions.
- Async behaviour when getting and removing forced decisions.
- Checking for forced-decision in the fallback-rule. Similarly, for all cases where
FindValidatedForcedDecisionis used. - Validation for empty rule_key when setting forced decision.
jaeopt
left a comment
jaeopt
left a comment
There was a problem hiding this comment.
It overall looks good! Some changes suggested.
| return nil if @forced_decisions.empty? | ||
|
|
||
| forced_decision_key = ForcedDecision.new(flag_key, rule_key) | ||
| variation_key = @forced_decisions[forced_decision_key] |
There was a problem hiding this comment.
Don't we need mutex sync for all forced_decisions accesses (set, get, remove and clone)?
| expect(status).to be true | ||
| status = user_context_obj.remove_all_forced_decision | ||
| expect(status).to be true | ||
| end |
There was a problem hiding this comment.
can we validate both event and notification for the 3 cases? (flagKey), (flagKey, expRule), (flagKey, delRule)
| variations = [] | ||
| get_rules_for_flag(flag).each do |rule| | ||
| rule['variations'].each do |rule_variation| | ||
| variations.push(rule_variation) if variations.select { |variation| variation['id'] == rule_variation['id'] } |
There was a problem hiding this comment.
Does this filter out redundant variations. Can we add a test case for flag_variation_map?
There was a problem hiding this comment.
variations.select can return multiple values but we are using if condition just to check if any value exists.
There was a problem hiding this comment.
I may be confused but the logic looks opposite to me. We need to collect all variations used in rules for a flag (see related discussion here - optimizely/php-sdk#233 (comment)).
We need push variations only if NOT exist already.
Consider to extract this building flag-variations map as a separate function and add a test case.
jaeopt
left a comment
jaeopt
left a comment
There was a problem hiding this comment.
I see some of the key issues not addressed. Can you take a look at those open comments?
Also clean up deprecation messages from PR summary.
| variations = [] | ||
| get_rules_for_flag(flag).each do |rule| | ||
| rule['variations'].each do |rule_variation| | ||
| variations.push(rule_variation) if variations.select { |variation| variation['id'] == rule_variation['id'] } |
There was a problem hiding this comment.
I may be confused but the logic looks opposite to me. We need to collect all variations used in rules for a flag (see related discussion here - optimizely/php-sdk#233 (comment)).
We need push variations only if NOT exist already.
Consider to extract this building flag-variations map as a separate function and add a test case.
|
|
||
| decision, = @decision_service.get_variation_for_feature(config, feature_flag, user_id, attributes) | ||
| user_context = create_user_context(user_id, attributes) | ||
| decision, = @decision_service.get_variation_for_feature(config, feature_flag, user_context) |
There was a problem hiding this comment.
why there is a comma? decision,
6 participants
Summary
Add a set of new APIs for forced-decisions to OptimizelyUserContext:
Test plan