Refactor bucketer return value and fix FF decision bug (#59)

Author: mikeproeng37

CHANGELOG

@@ -1,4 +1,9 @@
 -------------------------------------------------------------------------------
+2.0.0.beta
+* Introduce feature flags and feature rollouts support.
+* Introduce variable support via feature flags.
+-------------------------------------------------------------------------------
+-------------------------------------------------------------------------------
 1.3.0
 * Introduce user profile service support.
 -------------------------------------------------------------------------------

lib/optimizely.rb

@@ -139,7 +139,10 @@ def get_variation(experiment_key, user_id, attributes = nil)
       variation_id = @decision_service.get_variation(experiment_key, user_id, attributes)
 
       unless variation_id.nil?
-        return @config.get_variation_key_from_id(experiment_key, variation_id)
+        variation = @config.get_variation_from_id(experiment_key, variation_id)
+        if variation
+          return variation['key']
+        end
       end
       nil
     end
@@ -361,7 +364,7 @@ def get_feature_variable_for_type(feature_flag_key, variable_key, variable_type,
           variation = decision['variation']
           variation_variable_usages = @config.variation_id_to_variable_usage_map[variation['id']]
           variable_id = variable['id']
-          unless variation_variable_usages.key?(variable_id)
+          unless variation_variable_usages and variation_variable_usages.key?(variable_id)
             variation_key = variation['key']
             @logger.log(Logger::DEBUG,
               "Variable '#{variable_key}' is not used in variation '#{variation_key}'. Returning the default variable value '#{variable_value}'."

lib/optimizely/bucketer.rb

@@ -41,7 +41,7 @@ def bucket(experiment, user_id)
       # experiment - Experiment for which visitor is to be bucketed.
       # user_id - String ID for user.
       #
-      # Returns String variation ID in which visitor with ID user_id has been placed. Nil if no variation.
+      # Returns variation in which visitor with ID user_id has been placed. Nil if no variation.
 
       # check if experiment is in a group; if so, check if user is bucketed into specified experiment
       experiment_id = experiment['id']
@@ -78,12 +78,13 @@ def bucket(experiment, user_id)
       traffic_allocations = experiment['trafficAllocation']
       variation_id = find_bucket(user_id, experiment_id, traffic_allocations)
       if variation_id && variation_id != ''
-        variation_key = @config.get_variation_key_from_id(experiment_key, variation_id)
+        variation = @config.get_variation_from_id(experiment_key, variation_id)
+        variation_key = variation ? variation['key'] : nil
         @config.logger.log(
           Logger::INFO,
           "User '#{user_id}' is in variation '#{variation_key}' of experiment '#{experiment_key}'."
         )
-        return variation_id
+        return variation
       end
 
       # Handle the case when the traffic range is empty due to sticky bucketing

lib/optimizely/decision_service.rb

@@ -83,7 +83,8 @@ def get_variation(experiment_key, user_id, attributes = nil)
       end
 
       # Bucket normally
-      variation_id = @bucketer.bucket(experiment, user_id)
+      variation = @bucketer.bucket(experiment, user_id)
+      variation_id = variation ? variation['id'] : nil
 
       # Persist bucketing decision
       save_user_profile(user_profile, experiment_id, variation_id)
@@ -244,10 +245,6 @@ def get_variation_for_feature_rollout(feature_flag, user_id, attributes = nil)
           variation = @bucketer.bucket(experiment, user_id)
           unless variation.nil?
             variation_key = variation['key']
-            @config.logger.log(
-              Logger::DEBUG,
-              "User '#{user_id}' is in variation '#{variation_key}' of experiment '#{experiment_key}'."
-            )
             return variation
           end
 

lib/optimizely/project_config.rb

@@ -202,18 +202,18 @@ def get_audience_conditions_from_id(audience_id)
       nil
     end
 
-    def get_variation_key_from_id(experiment_key, variation_id)
-      # Get variation key given experiment key and variation ID
+    def get_variation_from_id(experiment_key, variation_id)
+      # Get variation given experiment key and variation ID
       #
       # experiment_key - Key representing parent experiment of variation
       # variation_id - ID of the variation
       #
-      # Returns key of the variation
+      # Returns the variation or nil if not found
 
       variation_id_map = @variation_id_map[experiment_key]
       if variation_id_map
         variation = variation_id_map[variation_id]
-        return variation['key'] if variation
+        return variation if variation
         @logger.log Logger::ERROR, "Variation id '#{variation_id}' is not in datafile."
         @error_handler.handle_error InvalidVariationError
         return nil

lib/optimizely/version.rb

@@ -14,5 +14,5 @@
 #    limitations under the License.
 #
 module Optimizely
-  VERSION = '1.3.0'.freeze
+  VERSION = '2.0.0.beta'.freeze
 end

spec/bucketing_spec.rb

@@ -36,10 +36,12 @@ def get_bucketing_id(user_id, entity_id=nil)
     experiment = config.get_experiment_from_key('test_experiment')
 
     # Variation 1
-    expect(bucketer.bucket(experiment, 'test_user')).to eq('111128')
+    expected_variation_1 = config.get_variation_from_id('test_experiment', '111128')
+    expect(bucketer.bucket(experiment, 'test_user')).to eq(expected_variation_1)
 
     # Variation 2
-    expect(bucketer.bucket(experiment, 'test_user')).to eq('111129')
+    expected_variation_2 = config.get_variation_from_id('test_experiment','111129')
+    expect(bucketer.bucket(experiment, 'test_user')).to eq(expected_variation_2)
 
     # No matching variation
     expect(bucketer.bucket(experiment, 'test_user')).to be_nil
@@ -58,7 +60,8 @@ def get_bucketing_id(user_id, entity_id=nil)
     expect(bucketer).to receive(:generate_bucket_value).twice.and_return(3000)
 
     experiment = config.get_experiment_from_key('group1_exp1')
-    expect(bucketer.bucket(experiment, 'test_user')).to eq('130001')
+    expected_variation = config.get_variation_from_id('group1_exp1','130001')
+    expect(bucketer.bucket(experiment, 'test_user')).to eq(expected_variation)
     expect(spy_logger).to have_received(:log).exactly(4).times
     expect(spy_logger).to have_received(:log).twice
                       .with(Logger::DEBUG, "Assigned bucket 3000 to user 'test_user'.")
@@ -92,7 +95,8 @@ def get_bucketing_id(user_id, entity_id=nil)
     expect(bucketer).to receive(:generate_bucket_value).once.and_return(3000)
 
     experiment = config.get_experiment_from_key('group2_exp1')
-    expect(bucketer.bucket(experiment, 'test_user')).to eq('144443')
+    expected_variation = config.get_variation_from_id('group2_exp1','144443')
+    expect(bucketer.bucket(experiment, 'test_user')).to eq(expected_variation)
     expect(spy_logger).to have_received(:log).twice
     expect(spy_logger).to have_received(:log)
       .with(Logger::DEBUG, "Assigned bucket 3000 to user 'test_user'.")

spec/decision_service_spec.rb

@@ -429,8 +429,6 @@
 
           expect(spy_logger).to have_received(:log).once
                           .with(Logger::DEBUG, "User 'user_1' meets conditions for targeting rule '1'.")
-          expect(spy_logger).to have_received(:log).once
-                          .with(Logger::DEBUG, "User 'user_1' is in variation '177771' of experiment '177770'.")
         end
       end
 

spec/project_config_spec.rb

@@ -662,14 +662,14 @@
       end
     end
 
-    describe 'get_variation_key_from_id' do
+    describe 'get_variation_from_id' do
       it 'should log a message when provided experiment key is invalid' do
-        config.get_variation_key_from_id('invalid_key', 'some_variation')
+        config.get_variation_from_id('invalid_key', 'some_variation')
         expect(spy_logger).to have_received(:log).with(Logger::ERROR,
                                                        "Experiment key 'invalid_key' is not in datafile.")
       end
       it 'should return nil when provided variation key is invalid' do
-        expect(config.get_variation_key_from_id('test_experiment', 'invalid_variation')).to eq(nil)
+        expect(config.get_variation_from_id('test_experiment', 'invalid_variation')).to eq(nil)
       end
     end
 
@@ -738,16 +738,16 @@
       end
     end
 
-    describe 'get_variation_key_from_id' do
+    describe 'get_variation_from_id' do
       it 'should raise an error when provided experiment key is invalid' do
-        expect { config.get_variation_key_from_id('invalid_key', 'some_variation') }
+        expect { config.get_variation_from_id('invalid_key', 'some_variation') }
                .to raise_error(Optimizely::InvalidExperimentError)
       end
     end
 
-    describe 'get_variation_key_from_id' do
+    describe 'get_variation_from_id' do
       it 'should raise an error when provided variation key is invalid' do
-        expect { config.get_variation_key_from_id('test_experiment', 'invalid_variation') }
+        expect { config.get_variation_from_id('test_experiment', 'invalid_variation') }
                .to raise_error(Optimizely::InvalidVariationError)
       end
     end

spec/project_spec.rb

@@ -20,7 +20,7 @@
 require 'optimizely/exceptions'
 require 'optimizely/version'
 
-describe 'OptimizelyV2' do
+describe 'Optimizely' do
   let(:config_body) { OptimizelySpec::VALID_CONFIG_BODY }
   let(:config_body_JSON) { OptimizelySpec::VALID_CONFIG_BODY_JSON }
   let(:config_body_invalid_JSON) { OptimizelySpec::INVALID_CONFIG_BODY_JSON }
@@ -136,7 +136,8 @@ class InvalidErrorHandler; end
         }
       }
 
-      allow(project_instance.decision_service.bucketer).to receive(:bucket).and_return('111128')
+      variation_to_return = project_instance.config.get_variation_from_id('test_experiment', '111128')
+      allow(project_instance.decision_service.bucketer).to receive(:bucket).and_return(variation_to_return)
       allow(project_instance.event_dispatcher).to receive(:dispatch_event).with(instance_of(Optimizely::Event))
       allow(project_instance.config).to receive(:get_audience_ids_for_experiment)
                                        .with('test_experiment')
@@ -175,7 +176,8 @@ class InvalidErrorHandler; end
         }
       }
 
-      allow(project_instance.decision_service.bucketer).to receive(:bucket).and_return('122228')
+      variation_to_return = project_instance.config.get_variation_from_id('test_experiment_with_audience', '122228')
+      allow(project_instance.decision_service.bucketer).to receive(:bucket).and_return(variation_to_return)
       allow(project_instance.event_dispatcher).to receive(:dispatch_event).with(instance_of(Optimizely::Event))
 
       expect(project_instance.activate('test_experiment_with_audience', 'test_user', 'browser_type' => 'firefox'))
@@ -226,7 +228,8 @@ class InvalidErrorHandler; end
         }
       }
 
-      allow(project_instance.decision_service.bucketer).to receive(:bucket).and_return('111128')
+      variation_to_return = project_instance.config.get_variation_from_id('test_experiment', '111128')
+      allow(project_instance.decision_service.bucketer).to receive(:bucket).and_return(variation_to_return)
       allow(project_instance.event_dispatcher).to receive(:dispatch_event).with(instance_of(Optimizely::Event))
       allow(project_instance.config).to receive(:get_audience_ids_for_experiment)
                                         .with('test_experiment')
@@ -237,7 +240,8 @@ class InvalidErrorHandler; end
     end
 
     it 'should log when an exception has occurred during dispatching the impression event' do
-      allow(project_instance.decision_service.bucketer).to receive(:bucket).and_return('111128')
+      variation_to_return = project_instance.config.get_variation_from_id('test_experiment', '111128')
+      allow(project_instance.decision_service.bucketer).to receive(:bucket).and_return(variation_to_return)
       allow(project_instance.event_dispatcher).to receive(:dispatch_event).with(any_args).and_raise(RuntimeError)
       project_instance.activate('test_experiment', 'test_user')
       expect(spy_logger).to have_received(:log).once.with(Logger::ERROR, "Unable to dispatch impression event. Error: RuntimeError")