refactor: moved validated forced decision to decision service#369
refactor: moved validated forced decision to decision service#369msohailhussain merged 3 commits intomasterfrom
Conversation
ozayr-zaviar
changed the title
optimizely/decision_service.py
Outdated
| decide_reasons += rollout_variation_reasons | ||
| return variation, decide_reasons | ||
|
|
||
| def validated_forced_decision(self, decision_context, user_context): |
There was a problem hiding this comment.
can we rename it to something that's more readable and clear? the previous one seemed okay find_validated_forced_decision
|
|
||
| def validated_forced_decision(self, decision_context, user_context): | ||
| """ | ||
| Gets forced decisions based on flag key, rule key and variation. |
There was a problem hiding this comment.
| Gets forced decisions based on flag key, rule key and variation. | |
| Validates and returns forced decision based on flag key, rule key and variation. |
| flag_key = decision_context.flag_key | ||
| rule_key = decision_context.rule_key |
There was a problem hiding this comment.
Can we put these lines inside the if block?
optimizely/decision_service.py
Outdated
| # we use config here so we can use get_flag_variation() function which is defined in project_config | ||
| # otherwise we would us user_context.client instead of config |
There was a problem hiding this comment.
please make it more readable.
| variation = config.get_flag_variation(flag_key, 'key', forced_decision.variation_key) | ||
| if variation: | ||
| if rule_key: | ||
| user_has_forced_decision = enums.ForcedDecisionLogs \ |
There was a problem hiding this comment.
please update this name to something that suggests it is a reason.
user_has_forced_decision
| user_context.logger.info(user_has_forced_decision) | ||
|
|
||
| return variation, reasons | ||
|
|
| reasons.append(user_has_forced_decision) | ||
| user_context.logger.info(user_has_forced_decision) |
There was a problem hiding this comment.
i see the same code at 548-549, can we have it only once?
jaeopt
left a comment
jaeopt
left a comment
There was a problem hiding this comment.
A couple of changes suggested
optimizely/decision_service.py
Outdated
| """ | ||
| reasons = [] | ||
|
|
||
| forced_decision = user_context.find_forced_decision(decision_context) |
There was a problem hiding this comment.
Since we remove extra checking from get_forced_decision, we do not need find_forced_decision. It's identical to get_forced_decision. We can remove it and use get_forced_decision here.
optimizely/decision_service.py
Outdated
| if forced_decision: | ||
| # we use config here so we can use get_flag_variation() function which is defined in project_config | ||
| # otherwise we would us user_context.client instead of config | ||
| config = user_context.client.config_manager.get_config() if user_context.client else None |
There was a problem hiding this comment.
Pass config as a parameter from caller (not from user_context to break a cycle).
msohailhussain
dismissed
yasirfolio3’s stale review
3 approvers already approved and yasir's changes are made.
5 participants
Summary
find_validated_forced_decisionmoved to decision service.Test plan