Skip to content

refact: Decide API #314

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.

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

Merged
merged 20 commits into from
Feb 1, 2021
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
16 changes: 6 additions & 10 deletions optimizely/bucketer.py
Original file line number Diff line number Diff line change
Expand Up @@ -72,23 +72,20 @@ def find_bucket(self, project_config, bucketing_id, parent_id, traffic_allocatio

Returns:
Entity ID which may represent experiment or variation and
array of log messages representing decision making.
"""
decide_reasons = []
bucketing_key = BUCKETING_ID_TEMPLATE.format(bucketing_id=bucketing_id, parent_id=parent_id)
bucketing_number = self._generate_bucket_value(bucketing_key)
message = 'Assigned bucket %s to user with bucketing ID "%s".' % (bucketing_number, bucketing_id)
project_config.logger.debug(
message
)
decide_reasons.append(message)

for traffic_allocation in traffic_allocations:
current_end_of_range = traffic_allocation.get('endOfRange')
if bucketing_number < current_end_of_range:
return traffic_allocation.get('entityId'), decide_reasons
return traffic_allocation.get('entityId')

return None, decide_reasons
return None

def bucket(self, project_config, experiment, user_id, bucketing_id):
""" For a given experiment and bucketing ID determines variation to be shown to user.
Expand Down Expand Up @@ -116,10 +113,10 @@ def bucket(self, project_config, experiment, user_id, bucketing_id):
if not group:
return None, decide_reasons

user_experiment_id, find_bucket_reasons = self.find_bucket(
user_experiment_id = self.find_bucket(
project_config, bucketing_id, experiment.groupId, group.trafficAllocation,
)
decide_reasons += find_bucket_reasons

if not user_experiment_id:
message = 'User "%s" is in no experiment.' % user_id
project_config.logger.info(message)
Expand All @@ -142,9 +139,8 @@ def bucket(self, project_config, experiment, user_id, bucketing_id):
decide_reasons.append(message)

# Bucket user if not in white-list and in group (if any)
variation_id, find_bucket_reasons = self.find_bucket(project_config, bucketing_id,
experiment.id, experiment.trafficAllocation)
decide_reasons += find_bucket_reasons
variation_id = self.find_bucket(project_config, bucketing_id,
experiment.id, experiment.trafficAllocation)
if variation_id:
variation = project_config.get_variation_from_id(experiment.key, variation_id)
return variation, decide_reasons
Expand Down
19 changes: 6 additions & 13 deletions optimizely/decision_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,6 @@ def get_forced_variation(self, project_config, experiment_key, user_id):
if user_id not in self.forced_variation_map:
message = 'User "%s" is not in the forced variation map.' % user_id
self.logger.debug(message)
decide_reasons.append(message)
return None, decide_reasons

experiment = project_config.get_experiment_from_key(experiment_key)
Expand All @@ -152,14 +151,12 @@ def get_forced_variation(self, project_config, experiment_key, user_id):
self.logger.debug(
message
)
decide_reasons.append(message)
return None, decide_reasons

variation_id = experiment_to_variation_map.get(experiment.id)
if variation_id is None:
message = 'No variation mapped to experiment "%s" in the forced variation map.' % experiment_key
self.logger.debug(message)
decide_reasons.append(message)
return None, decide_reasons

variation = project_config.get_variation_from_id(experiment_key, variation_id)
Expand Down Expand Up @@ -206,9 +203,8 @@ def get_stored_variation(self, project_config, experiment, user_profile):
user_profile: UserProfile object representing the user's profile.

Returns:
Variation if available. None otherwise And an array of log messages representing decision making.
Variation if available. None otherwise.
"""
decide_reasons = []
user_id = user_profile.user_id
variation_id = user_profile.get_variation_for_experiment(experiment.id)

Expand All @@ -220,13 +216,12 @@ def get_stored_variation(self, project_config, experiment, user_profile):
self.logger.info(
message
)
decide_reasons.append(message)
return variation, decide_reasons
return variation

return None, decide_reasons
return None

def get_variation(
self, project_config, experiment, user_id, attributes, ignore_user_profile=False, decide_options=[]
self, project_config, experiment, user_id, attributes, ignore_user_profile=False
):
""" Top-level function to help determine variation user should be put in.

Expand All @@ -242,7 +237,6 @@ def get_variation(
user_id: ID for user.
attributes: Dict representing user attributes.
ignore_user_profile: True to ignore the user profile lookup. Defaults to False.
decideOptions: Options to customize evaluation.

Returns:
Variation user should see. None if user is not in experiment or experiment is not running
Expand Down Expand Up @@ -279,7 +273,7 @@ def get_variation(

if validator.is_user_profile_valid(retrieved_profile):
user_profile = UserProfile(**retrieved_profile)
variation, reasons_received = self.get_stored_variation(project_config, experiment, user_profile)
variation = self.get_stored_variation(project_config, experiment, user_profile)
decide_reasons += reasons_received
if variation:
message = 'Returning previously activated variation ID "{}" of experiment ' \
Copy link
Contributor

Choose a reason for hiding this comment

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

This message is redundant in reasons (see #223). I think we should remove #223 from reasons and keep this one.

Expand Down Expand Up @@ -433,9 +427,8 @@ def get_experiment_in_group(self, project_config, group, bucketing_id):
and array of log messages representing decision making.
"""
decide_reasons = []
experiment_id, reasons = self.bucketer.find_bucket(
experiment_id = self.bucketer.find_bucket(
project_config, bucketing_id, group.id, group.trafficAllocation)
decide_reasons += reasons
if experiment_id:
experiment = project_config.get_experiment_from_id(experiment_id)
if experiment:
Expand Down
6 changes: 1 addition & 5 deletions optimizely/optimizely.py
Original file line number Diff line number Diff line change
Expand Up @@ -1067,9 +1067,7 @@ def _decide(self, user_context, key, decide_options=None):

all_variables[variable_key] = actual_value

should_include_reasons = False
if OptimizelyDecideOption.INCLUDE_REASONS in decide_options:
should_include_reasons = True
should_include_reasons = OptimizelyDecideOption.INCLUDE_REASONS in decide_options

# Send notification
self.notification_center.send_notifications(
Expand Down Expand Up @@ -1114,10 +1112,8 @@ def _decide_all(self, user_context, decide_options=None):
return {}

config = self.config_manager.get_config()
reasons = []
if not config:
self.logger.error(enums.Errors.INVALID_PROJECT_CONFIG.format('decide'))
reasons.append(OptimizelyDecisionMessage.SDK_NOT_READY)
return {}

keys = []
Expand Down
16 changes: 8 additions & 8 deletions tests/test_decision_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -406,7 +406,7 @@ def test_get_stored_variation__stored_decision_available(self):
with mock.patch.object(
self.decision_service, "logger"
) as mock_decision_service_logging:
variation, _ = self.decision_service.get_stored_variation(
variation = self.decision_service.get_stored_variation(
self.project_config, experiment, profile
)
self.assertEqual(
Expand All @@ -423,7 +423,7 @@ def test_get_stored_variation__no_stored_decision_available(self):

experiment = self.project_config.get_experiment_from_key("test_experiment")
profile = user_profile.UserProfile("test_user")
variation, _ = self.decision_service.get_stored_variation(
variation = self.decision_service.get_stored_variation(
self.project_config, experiment, profile
)
self.assertIsNone(
Expand Down Expand Up @@ -478,7 +478,7 @@ def test_get_variation__bucketing_id_provided(self):
return_value=[None, []],
), mock.patch(
"optimizely.decision_service.DecisionService.get_stored_variation",
return_value=[None, []],
return_value=None,
), mock.patch(
"optimizely.helpers.audience.does_user_meet_audience_conditions", return_value=[True, []]
), mock.patch(
Expand Down Expand Up @@ -545,7 +545,7 @@ def test_get_variation__user_has_stored_decision(self):
return_value=[None, []],
) as mock_get_whitelisted_variation, mock.patch(
"optimizely.decision_service.DecisionService.get_stored_variation",
return_value=[entities.Variation("111128", "control"), []],
return_value=entities.Variation("111128", "control"),
) as mock_get_stored_variation, mock.patch(
"optimizely.helpers.audience.does_user_meet_audience_conditions"
) as mock_audience_check, mock.patch(
Expand Down Expand Up @@ -597,7 +597,7 @@ def test_get_variation__user_bucketed_for_new_experiment__user_profile_service_a
return_value=[None, []],
) as mock_get_whitelisted_variation, mock.patch(
"optimizely.decision_service.DecisionService.get_stored_variation",
return_value=[None, []],
return_value=None,
) as mock_get_stored_variation, mock.patch(
"optimizely.helpers.audience.does_user_meet_audience_conditions", return_value=[True, []]
) as mock_audience_check, mock.patch(
Expand Down Expand Up @@ -706,7 +706,7 @@ def test_get_variation__user_does_not_meet_audience_conditions(self):
return_value=[None, []],
) as mock_get_whitelisted_variation, mock.patch(
"optimizely.decision_service.DecisionService.get_stored_variation",
return_value=[None, []],
return_value=None,
) as mock_get_stored_variation, mock.patch(
"optimizely.helpers.audience.does_user_meet_audience_conditions", return_value=[False, []]
) as mock_audience_check, mock.patch(
Expand Down Expand Up @@ -1447,7 +1447,7 @@ def test_get_experiment_in_group(self):
group = self.project_config.get_group("19228")
experiment = self.project_config.get_experiment_from_id("32222")
with mock.patch(
"optimizely.bucketer.Bucketer.find_bucket", return_value=["32222", []]
"optimizely.bucketer.Bucketer.find_bucket", return_value="32222"
), self.mock_decision_logger as mock_decision_service_logging:
variation_received, _ = self.decision_service.get_experiment_in_group(
self.project_config, group, "test_user"
Expand All @@ -1466,7 +1466,7 @@ def test_get_experiment_in_group__returns_none_if_user_not_in_group(self):

group = self.project_config.get_group("19228")
with mock.patch(
"optimizely.bucketer.Bucketer.find_bucket", return_value=[None, []]
"optimizely.bucketer.Bucketer.find_bucket", return_value=None
), self.mock_decision_logger as mock_decision_service_logging:
variation_received, _ = self.decision_service.get_experiment_in_group(
self.project_config, group, "test_user"
Expand Down
2 changes: 1 addition & 1 deletion tests/test_optimizely.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
from optimizely import optimizely_config
from optimizely import project_config
from optimizely import version
from optimizely.decision.decide_option import DecideOption
from optimizely.decision.optimizely_decide_option import OptimizelyDecideOption as DecideOption
from optimizely.event.event_factory import EventFactory
from optimizely.helpers import enums
from . import base
Expand Down
Loading