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

refactor: moved validated forced decision to decision service #369

Merged
merged 3 commits into from
Jan 6, 2022
Merged

refactor: moved validated forced decision to decision service #369

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6673021
moved validate forced decision
ozayr-zaviar Dec 22, 2021
0099740
headers updated
ozayr-zaviar Jan 4, 2022
e6d39af
comments addressed
ozayr-zaviar Jan 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 64 additions & 4 deletions optimizely/decision_service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,8 +378,8 @@ def get_variation_for_rollout(self, project_config, feature, user):
# check forced decision first
rule = rollout_rules[index]
optimizely_decision_context = OptimizelyUserContext.OptimizelyDecisionContext(feature.key, rule.key)
forced_decision_variation, reasons_received = user.find_validated_forced_decision(
optimizely_decision_context)
forced_decision_variation, reasons_received = self.validated_forced_decision(
optimizely_decision_context, user)
decide_reasons += reasons_received

if forced_decision_variation:
Expand Down Expand Up @@ -464,8 +464,8 @@ def get_variation_for_feature(self, project_config, feature, user_context, optio
optimizely_decision_context = OptimizelyUserContext.OptimizelyDecisionContext(feature.key,
experiment.key)

forced_decision_variation, reasons_received = user_context.find_validated_forced_decision(
optimizely_decision_context)
forced_decision_variation, reasons_received = self.validated_forced_decision(
optimizely_decision_context, user_context)
decide_reasons += reasons_received

if forced_decision_variation:
Expand All @@ -489,3 +489,63 @@ def get_variation_for_feature(self, project_config, feature, user_context, optio
if rollout_variation_reasons:
decide_reasons += rollout_variation_reasons
return variation, decide_reasons

def validated_forced_decision(self, decision_context, user_context):
Copy link
Contributor

@yasirfolio3 yasirfolio3 Dec 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

can we rename it to something that's more readable and clear? the previous one seemed okay find_validated_forced_decision

"""
Gets forced decisions based on flag key, rule key and variation.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Gets forced decisions based on flag key, rule key and variation.
Validates and returns forced decision based on flag key, rule key and variation.


Args:
decision context: a decision context
user_context context: a user context

Returns:
Variation of the forced decision.
"""
reasons = []

forced_decision = user_context.find_forced_decision(decision_context)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


flag_key = decision_context.flag_key
rule_key = decision_context.rule_key
Comment on lines +509 to +510
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we put these lines inside the if block?


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
Copy link
Contributor

Choose a reason for hiding this comment

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

please make it more readable.

config = user_context.client.config_manager.get_config() if user_context.client else None
Copy link
Contributor

Choose a reason for hiding this comment

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

Pass config as a parameter from caller (not from user_context to break a cycle).

if not config:
return None, reasons
variation = config.get_flag_variation(flag_key, 'key', forced_decision.variation_key)
if variation:
if rule_key:
user_has_forced_decision = enums.ForcedDecisionLogs \
Copy link
Contributor

Choose a reason for hiding this comment

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

please update this name to something that suggests it is a reason.
user_has_forced_decision

.USER_HAS_FORCED_DECISION_WITH_RULE_SPECIFIED.format(forced_decision.variation_key,
flag_key,
rule_key,
user_context.user_id)

else:
user_has_forced_decision = enums.ForcedDecisionLogs \
.USER_HAS_FORCED_DECISION_WITHOUT_RULE_SPECIFIED.format(forced_decision.variation_key,
flag_key,
user_context.user_id)

reasons.append(user_has_forced_decision)
user_context.logger.info(user_has_forced_decision)
Comment on lines +530 to +531
Copy link
Contributor

Choose a reason for hiding this comment

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

i see the same code at 548-549, can we have it only once?


return variation, reasons

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

else:
if rule_key:
user_has_forced_decision_but_invalid = enums.ForcedDecisionLogs \
.USER_HAS_FORCED_DECISION_WITH_RULE_SPECIFIED_BUT_INVALID.format(flag_key,
rule_key,
user_context.user_id)
else:
user_has_forced_decision_but_invalid = enums.ForcedDecisionLogs \
.USER_HAS_FORCED_DECISION_WITHOUT_RULE_SPECIFIED_BUT_INVALID.format(flag_key,
user_context.user_id)

reasons.append(user_has_forced_decision_but_invalid)
user_context.logger.info(user_has_forced_decision_but_invalid)

return None, reasons
3 changes: 2 additions & 1 deletion optimizely/optimizely.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1036,7 +1036,8 @@ def _decide(self, user_context, key, decide_options=None):

# Check forced decisions first
optimizely_decision_context = OptimizelyUserContext.OptimizelyDecisionContext(flag_key=key, rule_key=rule_key)
forced_decision_response = user_context.find_validated_forced_decision(optimizely_decision_context)
forced_decision_response = self.decision_service.validated_forced_decision(optimizely_decision_context,
user_context)
variation, decision_reasons = forced_decision_response
reasons += decision_reasons

Expand Down
60 changes: 0 additions & 60 deletions optimizely/optimizely_user_context.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@
import copy
import threading

from .helpers import enums


class OptimizelyUserContext(object):
"""
Expand Down Expand Up @@ -225,61 +223,3 @@ def find_forced_decision(self, decision_context):

# must allow None to be returned for the Flags only case
return self.forced_decisions_map.get(decision_context)

def find_validated_forced_decision(self, decision_context):
"""
Gets forced decisions based on flag key, rule key and variation.

Args:
decision context: a decision context

Returns:
Variation of the forced decision.
"""
reasons = []

forced_decision = self.find_forced_decision(decision_context)

flag_key = decision_context.flag_key
rule_key = decision_context.rule_key

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 self.client instead of config
config = self.client.config_manager.get_config() if self.client else None
if not config:
return None, reasons
variation = config.get_flag_variation(flag_key, 'key', forced_decision.variation_key)
if variation:
if rule_key:
user_has_forced_decision = enums.ForcedDecisionLogs \
.USER_HAS_FORCED_DECISION_WITH_RULE_SPECIFIED.format(forced_decision.variation_key,
flag_key,
rule_key,
self.user_id)

else:
user_has_forced_decision = enums.ForcedDecisionLogs \
.USER_HAS_FORCED_DECISION_WITHOUT_RULE_SPECIFIED.format(forced_decision.variation_key,
flag_key,
self.user_id)

reasons.append(user_has_forced_decision)
self.logger.info(user_has_forced_decision)

return variation, reasons

else:
if rule_key:
user_has_forced_decision_but_invalid = enums.ForcedDecisionLogs \
.USER_HAS_FORCED_DECISION_WITH_RULE_SPECIFIED_BUT_INVALID.format(flag_key,
rule_key,
self.user_id)
else:
user_has_forced_decision_but_invalid = enums.ForcedDecisionLogs \
.USER_HAS_FORCED_DECISION_WITHOUT_RULE_SPECIFIED_BUT_INVALID.format(flag_key, self.user_id)

reasons.append(user_has_forced_decision_but_invalid)
self.logger.info(user_has_forced_decision_but_invalid)

return None, reasons