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 all commits
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: 63 additions & 5 deletions optimizely/decision_service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# Copyright 2017-2021, Optimizely
# Copyright 2017-2022, Optimizely
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
Expand Down 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(
project_config, 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(
project_config, optimizely_decision_context, user_context)
decide_reasons += reasons_received

if forced_decision_variation:
Expand All @@ -489,3 +489,61 @@ 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, project_config, decision_context, user_context):
"""
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:
project_config: a project config
decision context: a decision context
user_context context: a user context

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

forced_decision = user_context.get_forced_decision(decision_context)

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:
if not project_config:
return None, reasons
variation = project_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
6 changes: 4 additions & 2 deletions optimizely/optimizely.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# Copyright 2016-2021, Optimizely
# Copyright 2016-2022, Optimizely
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
Expand Down Expand Up @@ -1036,7 +1036,9 @@ 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(config,
optimizely_decision_context,
user_context)
variation, decision_reasons = forced_decision_response
reasons += decision_reasons

Expand Down
62 changes: 1 addition & 61 deletions optimizely/optimizely_user_context.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# Copyright 2021, Optimizely and contributors
# Copyright 2021-2022, Optimizely and contributors
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
Expand All @@ -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