fixed for bucketer

optimizely · zashraf1985 · Jan 7, 2021 · Dec 15, 2020 · Dec 15, 2020 · Dec 15, 2020
commit c4bdcb21dbc6ecdb2e65ebb48d7ce036c3110d01
diff --git a/lib/optimizely/bucketer.rb b/lib/optimizely/bucketer.rb
@@ -35,7 +35,7 @@ def initialize(logger)
       @bucket_seed = HASH_SEED
     end
 
-    def bucket(project_config, experiment, bucketing_id, user_id, decide_reasons = nil)
+    def bucket(project_config, experiment, bucketing_id, user_id)
       # Determines ID of variation to be shown for a given experiment key and user ID.
       #
       # project_config - Instance of ProjectConfig
@@ -44,7 +44,9 @@ def bucket(project_config, experiment, bucketing_id, user_id, decide_reasons = n
       # user_id - String ID for user.
       #
       # Returns variation in which visitor with ID user_id has been placed. Nil if no variation.
-      return nil if experiment.nil?
+      return nil, [] if experiment.nil?
+
+      decide_reasons = []
 
       # check if experiment is in a group; if so, check if user is bucketed into specified experiment
       # this will not affect evaluation of rollout rules.
@@ -55,72 +57,78 @@ def bucket(project_config, experiment, bucketing_id, user_id, decide_reasons = n
         group = project_config.group_id_map.fetch(group_id)
         if Helpers::Group.random_policy?(group)
           traffic_allocations = group.fetch('trafficAllocation')
-          bucketed_experiment_id = find_bucket(bucketing_id, user_id, group_id, traffic_allocations)
+          bucketed_experiment_id, find_bucket_reasons = find_bucket(bucketing_id, user_id, group_id, traffic_allocations)
+          decide_reasons.push(find_bucket_reasons)
+
           # return if the user is not bucketed into any experiment
           unless bucketed_experiment_id
             message = "User '#{user_id}' is in no experiment."
             @logger.log(Logger::INFO, message)
-            decide_reasons&.push(message)
-            return nil
+            decide_reasons.push(message)
+            return nil, decide_reasons
           end
 
           # return if the user is bucketed into a different experiment than the one specified
           if bucketed_experiment_id != experiment_id
             message = "User '#{user_id}' is not in experiment '#{experiment_key}' of group #{group_id}."
             @logger.log(Logger::INFO, message)
-            decide_reasons&.push(message)
-            return nil
+            decide_reasons.push(message)
            return nil, decide_reasons
           end
 
           # continue bucketing if the user is bucketed into the experiment specified
           message = "User '#{user_id}' is in experiment '#{experiment_key}' of group #{group_id}."
           @logger.log(Logger::INFO, message)
-          decide_reasons&.push(message)
+          decide_reasons.push(message)
         end
       end
 
       traffic_allocations = experiment['trafficAllocation']
-      variation_id = find_bucket(bucketing_id, user_id, experiment_id, traffic_allocations, decide_reasons)
+      variation_id, find_bucket_reasons = find_bucket(bucketing_id, user_id, experiment_id, traffic_allocations)
+      decide_reasons.push(find_bucket_reasons)
+
       if variation_id && variation_id != ''
         variation = project_config.get_variation_from_id(experiment_key, variation_id)
-        return variation
+        return variation, decide_reasons
       end
 
       # Handle the case when the traffic range is empty due to sticky bucketing
       if variation_id == ''
         message = 'Bucketed into an empty traffic range. Returning nil.'
         @logger.log(Logger::DEBUG, message)
-        decide_reasons&.push(message)
+        decide_reasons.push(message)
       end
 
-      nil
+      [nil, decide_reasons]
     end
 
-    def find_bucket(bucketing_id, user_id, parent_id, traffic_allocations, decide_reasons = nil)
+    def find_bucket(bucketing_id, user_id, parent_id, traffic_allocations)
       # Helper function to find the matching entity ID for a given bucketing value in a list of traffic allocations.
       #
       # bucketing_id - String A customer-assigned value user to generate bucketing key
       # user_id - String ID for user
       # parent_id - String entity ID to use for bucketing ID
       # traffic_allocations - Array of traffic allocations
       #
-      # Returns entity ID corresponding to the provided bucket value or nil if no match is found.
+      # Returns and array of two values where first value is the entity ID corresponding to the provided bucket value
+      # or nil if no match is found. The second value contains the array of reasons stating how the deicision was taken
+      decide_reasons = []
       bucketing_key = format(BUCKETING_ID_TEMPLATE, bucketing_id: bucketing_id, entity_id: parent_id)
       bucket_value = generate_bucket_value(bucketing_key)
 
       message = "Assigned bucket #{bucket_value} to user '#{user_id}' with bucketing ID: '
8000
#{bucketing_id}'."
       @logger.log(Logger::DEBUG, message)
-      decide_reasons&.push(message)
+      decide_reasons.push(message)
 
       traffic_allocations.each do |traffic_allocation|
         current_end_of_range = traffic_allocation['endOfRange']
         if bucket_value < current_end_of_range
           entity_id = traffic_allocation['entityId']
-          return entity_id
+          return entity_id, decide_reasons
         end
       end
 
-      nil
+      [nil, decide_reasons]
     end
 
     private

diff --git a/lib/optimizely/decision_service.rb b/lib/optimizely/decision_service.rb
@@ -107,7 +107,8 @@ def get_variation(project_config, experiment_key, user_id, attributes = nil, dec
       end
 
       # Bucket normally
-      variation = @bucketer.bucket(project_config, experiment, bucketing_id, user_id, decide_reasons)
+      variation, bucket_reasons = @bucketer.bucket(project_config, experiment, bucketing_id, user_id)
+      decide_reasons&.push(*bucket_reasons)
       variation_id = variation ? variation['id'] : nil
 
       message = ''
@@ -241,7 +242,8 @@ def get_variation_for_feature_rollout(project_config, feature_flag, user_id, att
         decide_reasons&.push(message)
 
         # Evaluate if user satisfies the traffic allocation for this rollout rule
-        variation = @bucketer.bucket(project_config, rollout_rule, bucketing_id, user_id, decide_reasons)
+        variation, bucket_reasons = @bucketer.bucket(project_config, rollout_rule, bucketing_id, user_id)
+        decide_reasons&.push(*bucket_reasons)
         return Decision.new(rollout_rule, variation, DECISION_SOURCES['ROLLOUT']) unless variation.nil?
 
         break
@@ -262,7 +264,8 @@ def get_variation_for_feature_rollout(project_config, feature_flag, user_id, att
       @logger.log(Logger::DEBUG, message)
       decide_reasons&.push(message)
 
-      variation = @bucketer.bucket(project_config, everyone_else_experiment, bucketing_id, user_id, decide_reasons)
+      variation, bucket_reasons = @bucketer.bucket(project_config, everyone_else_experiment, bucketing_id, user_id)
+      decide_reasons&.push(*bucket_reasons)
       return Decision.new(everyone_else_experiment, variation, DECISION_SOURCES['ROLLOUT']) unless variation.nil?
 
       nil

diff --git a/spec/bucketing_spec.rb b/spec/bucketing_spec.rb
@@ -40,14 +40,17 @@ def get_bucketing_key(bucketing_id, entity_id = nil)
 
     # Variation 1
     expected_variation_1 = config.get_variation_from_id('test_experiment', '111128')
-    expect(bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user')).to eq(expected_variation_1)
+    variation_recieved, = bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user')
+    expect(variation_recieved).to eq(expected_variation_1)
 
     # Variation 2
     expected_variation_2 = config.get_variation_from_id('test_experiment', '111129')
-    expect(bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user')).to eq(expected_variation_2)
+    variation_recieved, = bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user')
+    expect(variation_recieved).to eq(expected_variation_2)
 
     # No matching variation
-    expect(bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user')).to be_nil
+    variation_recieved, = bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user')
+    expect(variation_recieved).to be_nil
   end
 
   it 'should test the output of generate_bucket_value for different inputs' do
@@ -66,7 +69,8 @@ def get_bucketing_key(bucketing_id, entity_id = nil)
 
     experiment = config.get_experiment_from_key('group1_exp1')
     expected_variation = config.get_variation_from_id('group1_exp1', '130001')
-    expect(bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user')).to eq(expected_variation)
+    variation_recieved, = bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user')
+    expect(variation_recieved).to eq(expected_variation)
     expect(spy_logger).to have_received(:log).exactly(3).times
     expect(spy_logger).to have_received(:log).twice
                                              .with(Logger::DEBUG, "Assigned bucket 3000 to user 'test_user' with bucketing ID: 'bucket_id_ignored'.")
@@ -78,7 +82,8 @@ def get_bucketing_key(bucketing_id, entity_id = nil)
     expect(bucketer).to receive(:generate_bucket_value).once.and_return(3000)
 
     experiment = config.get_experiment_from_key('group1_exp2')
-    expect(bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user')).to be_nil
+    variation_recieved, = bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user')
+    expect(variation_recieved).to be_nil
     expect(spy_logger).to have_received(:log)
       .with(Logger::DEBUG, "Assigned bucket 3000 to user 'test_user' with bucketing ID: 'bucket_id_ignored'.")
     expect(spy_logger).to have_received(:log)
@@ -89,7 +94,8 @@ def get_bucketing_key(bucketing_id, entity_id = nil)
     expect(bucketer).to receive(:find_bucket).once.and_return(nil)
 
     experiment = config.get_experiment_from_key('group1_exp2')
-    expect(bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user')).to be_nil
+    variation_recieved, = bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user')
+    expect(variation_recieved).to be_nil
     expect(spy_logger).to have_received(:log)
       .with(Logger::INFO, "User 'test_user' is in no experiment.")
   end
@@ -99,7 +105,8 @@ def get_bucketing_key(bucketing_id, entity_id = nil)
 
     experiment = config.get_experiment_from_key('group2_exp1')
     expected_variation = config.get_variation_from_id('group2_exp1', '144443')
-    expect(bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user')).to eq(expected_variation)
+    variation_recieved, = bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user')
+    expect(variation_recieved).to eq(expected_variation)
     expect(spy_logger).to have_received(:log).once
     expect(spy_logger).to have_received(:log)
       .with(Logger::DEBUG, "Assigned bucket 3000 to user 'test_user' with bucketing ID: 'bucket_id_ignored'.")
@@ -109,7 +116,8 @@ def get_bucketing_key(bucketing_id, entity_id = nil)
     expect(bucketer).to receive(:generate_bucket_value).and_return(50_000)
 
     experiment = config.get_experiment_from_key('group2_exp1')
-    expect(bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user')).to be_nil
+    variation_recieved, = bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user')
+    expect(variation_recieved).to be_nil
     expect(spy_logger).to have_received(:log).once
     expect(spy_logger).to have_received(:log)
       .with(Logger::DEBUG, "Assigned bucket 50000 to user 'test_user' with bucketing ID: 'bucket_id_ignored'.")
@@ -138,7 +14
9E88
6,8 @@ def get_bucketing_key(bucketing_id, entity_id = nil)
   it 'should return nil when user is in an empty traffic allocation range due to sticky bucketing' do
     expect(bucketer).to receive(:find_bucket).once.and_return('')
     experiment = config.get_experiment_from_key('test_experiment')
-    expect(bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user')).to be_nil
+    variation_recieved, = bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user')
+    expect(variation_recieved).to be_nil
     expect(spy_logger).to have_received(:log)
       .with(Logger::DEBUG, 'Bucketed into an empty traffic range. Returning nil.')
   end
@@ -151,17 +160,20 @@ def get_bucketing_key(bucketing_id, entity_id = nil)
 
       # Bucketing with user id as bucketing id - 'test_user111127' produces bucket value < 5000 thus buckets to control
       expected_variation = config.get_variation_from_id('test_experiment', '111128')
-      expect(bucketer.bucket(config, experiment, 'test_user', 'test_user')).to be(expected_variation)
+      variation_recieved, = bucketer.bucket(config, experiment, 'test_user', 'test_user')
+      expect(variation_recieved).to be(expected_variation)
 
       # Bucketing with bucketing id - 'any_string789111127' produces bucket value btw 5000 to 10,000
       # thus buckets to variation
       expected_variation = config.get_variation_from_id('test_experiment', '111129')
-      expect(bucketer.bucket(config, experiment, 'any_string789', 'test_user')).to be(expected_variation)
+      variation_recieved, = bucketer.bucket(config, experiment, 'any_string789', 'test_user')
+      expect(variation_recieved).to be(expected_variation)
     end
 
     # Bucketing with invalid experiment key and bucketing ID
     it 'should return nil with invalid experiment and bucketing ID' do
-      expect(bucketer.bucket(config, config.get_experiment_from_key('invalid_experiment'), 'some_id', 'test_user')).to be(nil)
+      variation_recieved, = bucketer.bucket(config, config.get_experiment_from_key('invalid_experiment'), 'some_id', 'test_user')
+      expect(variation_recieved).to be(nil)
     end
 
     # Bucketing with grouped experiments and bucketing ID
@@ -170,10 +182,12 @@ def get_bucketing_key(bucketing_id, entity_id = nil)
       experiment = config.get_experiment_from_key('group1_exp1')
 
       expected_variation = nil
-      expect(bucketer.bucket(config, experiment, 'test_user', 'test_user')).to be(expected_variation)
+      variation_recieved, = bucketer.bucket(config, experiment, 'test_user', 'test_user')
+      expect(variation_recieved).to be(expected_variation)
 
       expected_variation = config.get_variation_from_id('group1_exp1', '130002')
-      expect(bucketer.bucket(config, experiment, '123456789', 'test_user')).to be(expected_variation)
+      variation_recieved, = bucketer.bucket(config, experiment, '123456789', 'test_user')
+      expect(variation_recieved).to be(expected_variation)
     end
   end
 

diff --git a/spec/decision_service_spec.rb b/spec/decision_service_spec.rb
@@ -181,7 +181,7 @@
       # bucketing should have occured
       experiment = config.get_experiment_from_key('test_experiment')
       # since we do not pass bucketing id attribute, bucketer will recieve user id as the bucketing id
-      expect(decision_service.bucketer).to have_received(:bucket).once.with(config, experiment, 'forced_user_with_invalid_variation', 'forced_user_with_invalid_variation', nil)
+      expect(decision_service.bucketer).to have_received(:bucket).once.with(config, experiment, 'forced_user_with_invalid_variation', 'forced_user_with_invalid_variation')
     end
 
     describe 'when a UserProfile service is provided' do
@@ -519,7 +519,7 @@
           expected_decision = Optimizely::DecisionService::Decision.new(rollout_experiment, variation, Optimizely::DecisionService::DECISION_SOURCES['ROLLOUT'])
           allow(Optimizely::Audience).to receive(:user_meets_audience_conditions?).and_return(true)
           allow(decision_service.bucketer).to receive(:bucket)
-            .with(config, rollout_experiment, user_id, user_id, nil)
+            .with(config, rollout_experiment, user_id, user_id)
             .and_return(variation)
           expect(decision_service.get_variation_for_feature_rollout(config, feature_flag, user_id, user_attributes, nil)).to eq(expected_decision)
         end
@@ -534,10 +534,10 @@
 
             allow(Optimizely::Audience).to receive(:user_meets_audience_conditions?).and_return(true)
             allow(decision_service.bucketer).to receive(:bucket)
-              .with(config, rollout['experiments'][0], user_id, user_id, nil)
+              .with(config, rollout['experiments'][0], user_id, user_id)
               .and_return(nil)
             allow(decision_service.bucketer).to receive(:bucket)
-              .with(config, everyone_else_experiment, user_id, user_id, nil)
+              .with(config, everyone_else_experiment, user_id, user_id)
               .and_return(nil)
 
             expect(decision_service.get_variation_for_feature_rollout(config, feature_flag, user_id, user_attributes, nil)).to eq(nil)
@@ -559,10 +559,10 @@
             expected_decision = Optimizely::DecisionService::Decision.new(everyone_else_experiment, variation, Optimizely::DecisionService::DECISION_SOURCES['ROLLOUT'])
             allow(Optimizely::Audience).to receive(:user_meets_audience_conditions?).and_return(true)
             allow(decision_service.bucketer).to receive(:bucket)
-              .with(config, rollout['experiments'][0], user_id, user_id, nil)
+              .with(config, rollout['experiments'][0], user_id, user_id)
               .and_return(nil)
             allow(decision_service.bucketer).to receive(:bucket)
-              .with(config, everyone_else_experiment, user_id, user_id, nil)
+              .with(config, everyone_else_experiment, user_id, user_id)
               .and_return(variation)
 
             expect(decision_service.get_variation_for_feature_rollout(config, feature_flag, user_id, user_attributes, nil)).to eq(expected_decision)
@@ -590,7 +590,7 @@
           .with(config, everyone_else_experiment, user_attributes, spy_logger, 'ROLLOUT_AUDIENCE_EVALUATION_LOGS', 'Everyone Else')
           .and_return(true)
         allow(decision_service.bucketer).to receive(:bucket)
-          .with(config, everyone_else_experiment, user_id, user_id, nil)
+          .with(config, everyone_else_experiment, user_id, user_id)
           .and_return(variation)
 
         expect(decision_service.get_variation_for_feature_rollout(config, feature_flag, user_id, user_attributes, nil)).to eq(expected_decision)