Fix feature variable accessor default value (#113)

* 🖊️ Return default value when no variable usage found for variation

* Remove sorting

Author: oakbani

optimizely/optimizely.py

@@ -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(
@@ -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 = []
@@ -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):

optimizely/project_config.py

@@ -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
@@ -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():
@@ -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.

tests/test_config.py

@@ -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')
       }

tests/test_optimizely.py

@@ -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. """
 
@@ -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".'
     )
 
   def test_get_feature_variable_double(self):
@@ -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):
@@ -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):
@@ -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))