diff --git a/optimizely/optimizely.py b/optimizely/optimizely.py index 801c1536..76a42783 100644 --- a/optimizely/optimizely.py +++ b/optimizely/optimizely.py @@ -208,12 +208,25 @@ def _get_feature_variable_for_type(self, feature_key, variable_key, variable_typ ) return None + variable_value = variable.defaultValue + 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) + feature_enabled = decision.variation.featureEnabled + if feature_enabled: + variable_value = self.config.get_variable_value_for_variation(variable, decision.variation) + self.logger.info( + 'Got variable value "%s" for variable "%s" of feature flag "%s".' % ( + variable_value, variable_key, feature_key + ) + ) + else: + self.logger.info( + 'Feature "%s" for variation "%s" is not enabled. ' + 'Returning the default variable value "%s".' % (feature_key, decision.variation.key, variable_value) + ) else: - variable_value = variable.defaultValue self.logger.info( 'User "%s" is not in any variation or rollout rule. ' 'Returning default value for variable "%s" of feature flag "%s".' % (user_id, variable_key, feature_key) diff --git a/tests/base.py b/tests/base.py index ba3b5e02..d939db47 100644 --- a/tests/base.py +++ b/tests/base.py @@ -1,4 +1,4 @@ -# Copyright 2016-2018, Optimizely +# Copyright 2016-2019, 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 @@ -307,7 +307,29 @@ def setUp(self, config_dict='config_dict'): 'variations': [{ 'key': '211129', 'id': '211129', - 'featureEnabled': True + 'featureEnabled': True, + 'variables': [{ + 'id': '132', 'value': 'true' + }, { + 'id': '135', 'value': '395' + }, { + 'id': '134', 'value': '39.99' + }, { + 'id': '133', 'value': 'Hello audience' + }] + }, { + 'key': '211229', + 'id': '211229', + 'featureEnabled': False, + 'variables': [{ + 'id': '132', 'value': 'true' + }, { + 'id': '135', 'value': '395' + }, { + 'id': '134', 'value': '39.99' + }, { + 'id': '133', 'value': 'Hello audience' + }] }] }, { 'id': '211137', @@ -379,7 +401,27 @@ def setUp(self, config_dict='config_dict'): 'key': 'test_feature_in_rollout', 'experimentIds': [], 'rolloutId': '211111', - 'variables': [], + 'variables': [{ + 'id': '132', + 'key': 'is_running', + 'defaultValue': 'false', + 'type': 'boolean' + }, { + 'id': '133', + 'key': 'message', + 'defaultValue': 'Hello', + 'type': 'string' + }, { + 'id': '134', + 'key': 'price', + 'defaultValue': '99.99', + 'type': 'double' + }, { + 'id': '135', + 'key': 'count', + 'defaultValue': '999', + 'type': 'integer' + }] }, { 'id': '91113', 'key': 'test_feature_in_group', diff --git a/tests/test_config.py b/tests/test_config.py index 1c40b846..3730bbac 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -1,4 +1,4 @@ -# Copyright 2016-2018, Optimizely +# Copyright 2016-2019, 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 @@ -885,7 +885,19 @@ def test_get_feature_from_key__valid_feature_key(self): opt_obj = optimizely.Optimizely(json.dumps(self.config_dict_with_features)) project_config = opt_obj.config - expected_feature = entities.FeatureFlag('91112', 'test_feature_in_rollout', [], '211111', {}) + expected_feature = entities.FeatureFlag( + '91112', + 'test_feature_in_rollout', + [], + '211111', + { + 'is_running': entities.Variable('132', 'is_running', 'boolean', 'false'), + 'message': entities.Variable('133', 'message', 'string', 'Hello'), + 'price': entities.Variable('134', 'price', 'double', '99.99'), + 'count': entities.Variable('135', 'count', 'integer', '999') + } + ) + self.assertEqual(expected_feature, project_config.get_feature_from_key('test_feature_in_rollout')) def test_get_feature_from_key__invalid_feature_key(self): @@ -916,7 +928,29 @@ def test_get_rollout_from_id__valid_rollout_id(self): 'variations': [{ 'key': '211129', 'id': '211129', - 'featureEnabled': True + 'featureEnabled': True, + 'variables': [{ + 'id': '132', 'value': 'true' + }, { + 'id': '135', 'value': '395' + }, { + 'id': '134', 'value': '39.99' + }, { + 'id': '133', 'value': 'Hello audience' + }] + }, { + 'key': '211229', + 'id': '211229', + 'featureEnabled': False, + 'variables': [{ + 'id': '132', 'value': 'true' + }, { + 'id': '135', 'value': '395' + }, { + 'id': '134', 'value': '39.99' + }, { + 'id': '133', 'value': 'Hello audience' + }] }] }, { 'id': '211137', diff --git a/tests/test_optimizely.py b/tests/test_optimizely.py index 82311620..066c0343 100644 --- a/tests/test_optimizely.py +++ b/tests/test_optimizely.py @@ -2242,6 +2242,122 @@ def test_get_feature_variable__returns_none_if_invalid_variable_key(self): mock.call('Variable with key "invalid_variable" not found in the datafile.') ]) + def test_get_feature_variable__returns_default_value_if_feature_not_enabled(self): + """ Test that get_feature_variable_* returns default value if feature is not enabled for the user. """ + + 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', '111128') + + # 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.object(opt_obj, 'logger') as mock_client_logger: + + self.assertTrue(opt_obj.get_feature_variable_boolean('test_feature_in_experiment', 'is_working', 'test_user')) + + mock_client_logger.info.assert_called_once_with( + 'Feature "test_feature_in_experiment" for variation "control" is not enabled. ' + 'Returning the default variable 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.object(opt_obj, 'logger') as mock_client_logger: + self.assertEqual(10.99, + opt_obj.get_feature_variable_double('test_feature_in_experiment', 'cost', 'test_user')) + + mock_client_logger.info.assert_called_once_with( + 'Feature "test_feature_in_experiment" for variation "control" is not enabled. ' + 'Returning the default variable 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.object(opt_obj, 'logger') as mock_client_logger: + self.assertEqual(999, + opt_obj.get_feature_variable_integer('test_feature_in_experiment', 'count', 'test_user')) + + mock_client_logger.info.assert_called_once_with( + 'Feature "test_feature_in_experiment" for variation "control" is not enabled. ' + 'Returning the default variable 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.object(opt_obj, 'logger') as mock_client_logger: + self.assertEqual('devel', + opt_obj.get_feature_variable_string('test_feature_in_experiment', 'environment', 'test_user')) + + mock_client_logger.info.assert_called_once_with( + 'Feature "test_feature_in_experiment" for variation "control" is not enabled. ' + 'Returning the default variable value "devel".' + ) + + def test_get_feature_variable__returns_default_value_if_feature_not_enabled_in_rollout(self): + """ Test that get_feature_variable_* returns default value if feature is not enabled for the user. """ + + opt_obj = optimizely.Optimizely(json.dumps(self.config_dict_with_features)) + mock_experiment = opt_obj.config.get_experiment_from_key('211127') + mock_variation = opt_obj.config.get_variation_from_id('211127', '211229') + + # 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_ROLLOUT)), \ + mock.patch.object(opt_obj, 'logger') as mock_client_logger: + self.assertFalse(opt_obj.get_feature_variable_boolean('test_feature_in_rollout', 'is_running', 'test_user')) + + mock_client_logger.info.assert_called_once_with( + 'Feature "test_feature_in_rollout" for variation "211229" is not enabled. ' + 'Returning the default variable value "false".' + ) + + # 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_ROLLOUT)), \ + mock.patch.object(opt_obj, 'logger') as mock_client_logger: + self.assertEqual(99.99, + opt_obj.get_feature_variable_double('test_feature_in_rollout', 'price', 'test_user')) + + mock_client_logger.info.assert_called_once_with( + 'Feature "test_feature_in_rollout" for variation "211229" is not enabled. ' + 'Returning the default variable value "99.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_ROLLOUT)), \ + mock.patch.object(opt_obj, 'logger') as mock_client_logger: + self.assertEqual(999, + opt_obj.get_feature_variable_integer('test_feature_in_rollout', 'count', 'test_user')) + + mock_client_logger.info.assert_called_once_with( + 'Feature "test_feature_in_rollout" for variation "211229" is not enabled. ' + 'Returning the default variable 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_ROLLOUT)), \ + mock.patch.object(opt_obj, 'logger') as mock_client_logger: + self.assertEqual('Hello', + opt_obj.get_feature_variable_string('test_feature_in_rollout', 'message', 'test_user')) + mock_client_logger.info.assert_called_once_with( + 'Feature "test_feature_in_rollout" for variation "211229" is not enabled. ' + 'Returning the default variable value "Hello".' + ) + def test_get_feature_variable__returns_none_if_type_mismatch(self): """ Test that get_feature_variable_* returns None if type mismatch. """ @@ -2284,15 +2400,25 @@ def test_get_feature_variable_returns__variable_value__typed_audience_match(self opt_obj = optimizely.Optimizely(json.dumps(self.config_dict_with_typed_audiences)) # Should be included in the feature test via greater-than match audience with id '3468206647' - self.assertEqual( - 'xyz', - opt_obj.get_feature_variable_string('feat_with_var', 'x', 'user1', {'lasers': 71}) + with mock.patch.object(opt_obj, 'logger') as mock_client_logger: + self.assertEqual( + 'xyz', + opt_obj.get_feature_variable_string('feat_with_var', 'x', 'user1', {'lasers': 71}) + ) + + mock_client_logger.info.assert_called_once_with( + 'Got variable value "xyz" for variable "x" of feature flag "feat_with_var".' ) # Should be included in the feature test via exact match boolean audience with id '3468206643' - self.assertEqual( - 'xyz', - opt_obj.get_feature_variable_string('feat_with_var', 'x', 'user1', {'should_do_it': True}) + with mock.patch.object(opt_obj, 'logger') as mock_client_logger: + self.assertEqual( + 'xyz', + opt_obj.get_feature_variable_string('feat_with_var', 'x', 'user1', {'should_do_it': True}) + ) + + mock_client_logger.info.assert_called_once_with( + 'Got variable value "xyz" for variable "x" of feature flag "feat_with_var".' ) def test_get_feature_variable_returns__default_value__typed_audience_match(self):