refactor: moved validated forced decision to decision service. #293
Conversation
lib/optimizely.rb
Outdated
| @@ -199,7 +199,7 @@ def decide(user_context, key, decide_options = []) | |||
| experiment = nil | |||
| decision_source = Optimizely::DecisionService::DECISION_SOURCES['ROLLOUT'] | |||
| context = Optimizely::OptimizelyUserContext::OptimizelyDecisionContext.new(key, nil) | |||
| variation, reasons_received = user_context.find_validated_forced_decision(context) | |||
| variation, reasons_received = @decision_service.validated_forced_decision(context, user_context) | |||
There was a problem hiding this comment.
please keep the method name find_validated_forced_decision
| flag_key = context[:flag_key] | ||
| rule_key = context[:rule_key] |
There was a problem hiding this comment.
can we move this code inside the if block since it is only used there?
| reasons.push(reason) | ||
| return variation, reasons |
There was a problem hiding this comment.
this code can be moved to line 436, that way we won't have reasons.push(reason) twice.
| @@ -26,6 +26,7 @@ class OptimizelyUserContext | |||
| attr_reader :forced_decisions | |||
| attr_reader :OptimizelyDecisionContext | |||
| attr_reader :OptimizelyForcedDecision | |||
| attr_reader :optimizely_client | |||
There was a problem hiding this comment.
why is this needed when we are only removing a method here?
There was a problem hiding this comment.
Also update the Copyright header with 2020-2021
| @@ -26,6 +26,7 @@ class OptimizelyUserContext | |||
| attr_reader :forced_decisions | |||
| attr_reader :OptimizelyDecisionContext | |||
| attr_reader :OptimizelyForcedDecision | |||
| attr_reader :optimizely_client | |||
There was a problem hiding this comment.
Also update the Copyright header with 2020-2021
lib/optimizely/decision_service.rb
Outdated
| @@ -417,6 +417,28 @@ def get_forced_variation(project_config, experiment_key, user_id) | |||
| [variation, decide_reasons] | |||
| end | |||
|
|
|||
| def validated_forced_decision(context, user_context) | |||
| decision = user_context.find_forced_decision(context) | |||
There was a problem hiding this comment.
Remove find_forced_decision and use get_forced_decision. Since we remove sanity checking, they're now identical.
lib/optimizely/decision_service.rb
Outdated
| reasons = [] | ||
| target = rule_key ? "flag (#{flag_key}), rule (#{rule_key})" : "flag (#{flag_key})" | ||
| if variation_key | ||
| variation = user_context.optimizely_client.get_flag_variation(flag_key, variation_key, 'key') |
There was a problem hiding this comment.
I don't know what happened to my previous comments on this line. github bug? :)
We need to change this line to same as other SDKs - pass 'config' as a parameter and move get_flag_variation to config, so we can remove dependency to user_context.optimizely_client here.
Summary
find_validated_forced_decisionmoved to decision service.Test plan