Skip to content

Fix feature variable accessor default value #113

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

Merged
merged 2 commits into from
Apr 5, 2018
Merged

Fix feature variable accessor default value #113

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
05ea76e
:pen: Return default value when no variable usage found for variation
oakbani Apr 5, 2018
9bc9918
Remove sorting
msohailhussain Apr 5, 2018
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
9 changes: 2 additions & 7 deletions optimizely/optimizely.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,11 +230,7 @@ def _get_feature_variable_for_type(self, feature_key, variable_key, variable_typ
decision = self.decision_service.get_variation_for_feature(feature_flag, user_id, attributes)
if decision.variation:
variable_value = self.config.get_variable_value_for_variation(variable, decision.variation)
self.logger.log(
enums.LogLevels.INFO,
'Value for variable "%s" of feature flag "%s" is %s for user "%s".' % (
variable_key, feature_key, variable_value, user_id
))

else:
variable_value = variable.defaultValue
self.logger.log(
Expand Down Expand Up @@ -413,7 +409,7 @@ def get_enabled_features(self, user_id, attributes=None):
attributes: Dict representing user attributes.

Returns:
A sorted list of the keys of the features that are enabled for the user.
A list of the keys of the features that are enabled for the user.
"""

enabled_features = []
Expand All @@ -425,7 +421,6 @@ def get_enabled_features(self, user_id, attributes=None):
if self.is_feature_enabled(feature.key, user_id, attributes):
enabled_features.append(feature.key)

enabled_features.sort()
return enabled_features

def get_feature_variable_boolean(self, feature_key, variable_key, user_id, attributes=None):
Expand Down
32 changes: 24 additions & 8 deletions optimizely/project_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# Copyright 2016-2017, Optimizely
# Copyright 2016-2018, 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 @@ -90,10 +90,9 @@ def __init__(self, datafile, logger, error_handler):
self.variation_id_map[experiment.key] = {}
for variation in self.variation_key_map.get(experiment.key).values():
self.variation_id_map[experiment.key][variation.id] = variation
if variation.variables:
self.variation_variable_usage_map[variation.id] = self._generate_key_map(
variation.variables, 'id', entities.Variation.VariableUsage
)
self.variation_variable_usage_map[variation.id] = self._generate_key_map(
variation.variables, 'id', entities.Variation.VariableUsage
)

self.feature_key_map = self._generate_key_map(self.feature_flags, 'key', entities.FeatureFlag)
for feature in self.feature_key_map.values():
Expand Down Expand Up @@ -439,10 +438,27 @@ def get_variable_value_for_variation(self, variable, variation):
variable_usages = self.variation_variable_usage_map[variation.id]

# Find usage in given variation
variable_usage = variable_usages.get(variable.id)
variable_usage = None
if variable_usages:
variable_usage = variable_usages.get(variable.id)

if variable_usage:
variable_value = variable_usage.value
self.logger.log(
enums.LogLevels.INFO,
'Value for variable "%s" for variation "%s" is "%s".' % (
variable.key, variation.key, variable_value
))

# Return default value in case there is no variable usage for the variable.
return variable_usage.value if variable_usage else variable.defaultValue
else:
variable_value = variable.defaultValue
self.logger.log(
enums.LogLevels.INFO,
'Variable "%s" is not used in variation "%s". Assinging default value "%s".' % (
variable.key, variation.key, variable_value
))

return variable_value

def get_variable_for_feature(self, feature_key, variable_key):
""" Get the variable with the given variable key for the given feature.
Expand Down
2 changes: 2 additions & 0 deletions tests/test_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -616,6 +616,8 @@ def test_init__with_v4_datafile(self):
'129': entities.Variation.VariableUsage('129', '112'),
'130': entities.Variation.VariableUsage('130', '1.211')
},
'28905': {},
'28906': {},
'211113': {
'131': entities.Variation.VariableUsage('131', '15')
}
Expand Down
94 changes: 66 additions & 28 deletions tests/test_optimizely.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1387,29 +1387,6 @@ def side_effect(*args, **kwargs):
mock_is_feature_enabled.assert_any_call('test_feature_in_group', 'user_1', None)
mock_is_feature_enabled.assert_any_call('test_feature_in_experiment_and_rollout', 'user_1', None)

def test_get_enabled_features_returns_a_sorted_list(self):
""" Test that get_enabled_features returns a sorted list of enabled feature keys. """

opt_obj = optimizely.Optimizely(json.dumps(self.config_dict_with_features))

with mock.patch('optimizely.optimizely.Optimizely.is_feature_enabled',
return_value=True) as mock_is_feature_enabled:
received_features = opt_obj.get_enabled_features('user_1')

mock_is_feature_enabled.assert_any_call('test_feature_in_experiment', 'user_1', None)
mock_is_feature_enabled.assert_any_call('test_feature_in_rollout', 'user_1', None)
mock_is_feature_enabled.assert_any_call('test_feature_in_group', 'user_1', None)
mock_is_feature_enabled.assert_any_call('test_feature_in_experiment_and_rollout', 'user_1', None)

expected_sorted_features = [
'test_feature_in_experiment',
'test_feature_in_experiment_and_rollout',
'test_feature_in_group',
'test_feature_in_rollout'
]

self.assertEqual(expected_sorted_features, received_features)

def test_get_enabled_features__invalid_object(self):
""" Test that get_enabled_features returns empty list if Optimizely object is not valid. """

Expand All @@ -1436,7 +1413,7 @@ def test_get_feature_variable_boolean(self):

mock_logger.assert_called_once_with(
enums.LogLevels.INFO,
'Value for variable "is_working" of feature flag "test_feature_in_experiment" is true for user "test_user".'
'Value for variable "is_working" for variation "variation" is "true".'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • feature_flag removed as we don't have feature key reference in the new method where we have moved this log message. Additionally, in logging mode logs mentioning feature keys precede this log so no need of it.
  • user_id same, not available. And the message sort of implied that selection of value somehow depends on the user ID which isn't true.

)

def test_get_feature_variable_double(self):
Expand All @@ -1454,7 +1431,7 @@ def test_get_feature_variable_double(self):

mock_logger.assert_called_once_with(
enums.LogLevels.INFO,
'Value for variable "cost" of feature flag "test_feature_in_experiment" is 10.02 for user "test_user".'
'Value for variable "cost" for variation "variation" is "10.02".'
)

def test_get_feature_variable_integer(self):
Expand All @@ -1472,7 +1449,7 @@ def test_get_feature_variable_integer(self):

mock_logger.assert_called_once_with(
enums.LogLevels.INFO,
'Value for variable "count" of feature flag "test_feature_in_experiment" is 4243 for user "test_user".'
'Value for variable "count" for variation "variation" is "4243".'
)

def test_get_feature_variable_string(self):
Expand All @@ -1491,10 +1468,71 @@ def test_get_feature_variable_string(self):

mock_logger.assert_called_once_with(
enums.LogLevels.INFO,
'Value for variable "environment" of feature flag "test_feature_in_experiment" is staging for user "test_user".'
'Value for variable "environment" for variation "variation" is "staging".'
)

def test_get_feature_variable__returns_default_value_if_variable_usage_not_in_variation(self):
""" Test that get_feature_variable_* returns default value if variable usage not present in variation. """

opt_obj = optimizely.Optimizely(json.dumps(self.config_dict_with_features))
mock_experiment = opt_obj.config.get_experiment_from_key('test_experiment')
mock_variation = opt_obj.config.get_variation_from_id('test_experiment', '111129')

# Empty variable usage map for the mocked variation
opt_obj.config.variation_variable_usage_map['111129'] = None

# Boolean
with mock.patch('optimizely.decision_service.DecisionService.get_variation_for_feature',
return_value=decision_service.Decision(mock_experiment, mock_variation,
decision_service.DECISION_SOURCE_EXPERIMENT)), \
mock.patch('optimizely.logger.NoOpLogger.log') as mock_logger:
self.assertTrue(opt_obj.get_feature_variable_boolean('test_feature_in_experiment', 'is_working', 'test_user'))

mock_logger.assert_called_once_with(
enums.LogLevels.INFO,
'Variable "is_working" is not used in variation "variation". Assinging default value "true".'
)

# Double
with mock.patch('optimizely.decision_service.DecisionService.get_variation_for_feature',
return_value=decision_service.Decision(mock_experiment, mock_variation,
decision_service.DECISION_SOURCE_EXPERIMENT)), \
mock.patch('optimizely.logger.NoOpLogger.log') as mock_logger:
self.assertEqual(10.99,
opt_obj.get_feature_variable_double('test_feature_in_experiment', 'cost', 'test_user'))

mock_logger.assert_called_once_with(
enums.LogLevels.INFO,
'Variable "cost" is not used in variation "variation". Assinging default value "10.99".'
)

# Integer
with mock.patch('optimizely.decision_service.DecisionService.get_variation_for_feature',
return_value=decision_service.Decision(mock_experiment, mock_variation,
decision_service.DECISION_SOURCE_EXPERIMENT)), \
mock.patch('optimizely.logger.NoOpLogger.log') as mock_logger:
self.assertEqual(999,
opt_obj.get_feature_variable_integer('test_feature_in_experiment', 'count', 'test_user'))

mock_logger.assert_called_once_with(
enums.LogLevels.INFO,
'Variable "count" is not used in variation "variation". Assinging default value "999".'
)

# String
with mock.patch('optimizely.decision_service.DecisionService.get_variation_for_feature',
return_value=decision_service.Decision(mock_experiment, mock_variation,
decision_service.DECISION_SOURCE_EXPERIMENT)), \
mock.patch('optimizely.logger.NoOpLogger.log') as mock_logger:
self.assertEqual('devel',
opt_obj.get_feature_variable_string('test_feature_in_experiment', 'environment', 'test_user'))

mock_logger.assert_called_once_with(
enums.LogLevels.INFO,
'Variable "environment" is not used in variation "variation". Assinging default value "devel".'
)

def test_get_feature_variable__returns_default_value(self):
def test_get_feature_variable__returns_default_value_if_no_variation(self):
""" Test that get_feature_variable_* returns default value if no variation. """

opt_obj = optimizely.Optimizely(json.dumps(self.config_dict_with_features))
Expand Down