From c4bdcb21dbc6ecdb2e65ebb48d7ce036c3110d01 Mon Sep 17 00:00:00 2001 From: Zeeshan Ashraf Date: Tue, 15 Dec 2020 13:45:33 -0800 Subject: [PATCH 01/17] fixed for bucketer --- lib/optimizely/bucketer.rb | 42 ++++++++++++++++++------------ lib/optimizely/decision_service.rb | 9 ++++--- spec/bucketing_spec.rb | 42 ++++++++++++++++++++---------- spec/decision_service_spec.rb | 14 +++++----- 4 files changed, 66 insertions(+), 41 deletions(-) diff --git a/lib/optimizely/bucketer.rb b/lib/optimizely/bucketer.rb index 1a4994f6..1a1e6837 100644 --- 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,48 +57,52 @@ 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 @@ -104,23 +110,25 @@ def find_bucket(bucketing_id, user_id, parent_id, traffic_allocations, decide_re # 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: '#{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 index 77df3635..e7ad6c01 100644 --- 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 index 98a5b711..edc69578 100644 --- 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 +146,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 index 7cdc65b5..27dbf762 100644 --- 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) From 7adb3871cf8df76ccd988db59be2c2bc67b2a0d2 Mon Sep 17 00:00:00 2001 From: Zeeshan Ashraf Date: Tue, 15 Dec 2020 14:14:28 -0800 Subject: [PATCH 02/17] fixed get_bucketing_id --- lib/optimizely/decision_service.rb | 15 ++++++++------- spec/decision_service_spec.rb | 12 ++++++++---- 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/lib/optimizely/decision_service.rb b/lib/optimizely/decision_service.rb index e7ad6c01..491bcc5d 100644 --- a/lib/optimizely/decision_service.rb +++ b/lib/optimizely/decision_service.rb @@ -64,7 +64,8 @@ def get_variation(project_config, experiment_key, user_id, attributes = nil, dec # (nil if experiment is inactive or user does not meet audience conditions) # By default, the bucketing ID should be the user ID - bucketing_id = get_bucketing_id(user_id, attributes, decide_reasons) + bucketing_id, bucketing_id_reasons = get_bucketing_id(user_id, attributes) + decide_reasons&.push(*bucketing_id_reasons) # Check to make sure experiment is active experiment = project_config.get_experiment_from_key(experiment_key) return nil if experiment.nil? @@ -200,7 +201,8 @@ def get_variation_for_feature_rollout(project_config, feature_flag, user_id, att # attributes - Hash representing user attributes # # Returns the Decision struct or nil if not bucketed into any of the targeting rules - bucketing_id = get_bucketing_id(user_id, attributes, decide_reasons) + bucketing_id, bucketing_id_reasons = get_bucketing_id(user_id, attributes) + decide_reasons&.push(*bucketing_id_reasons) rollout_id = feature_flag['rolloutId'] if rollout_id.nil? || rollout_id.empty? feature_flag_key = feature_flag['key'] @@ -459,25 +461,24 @@ def save_user_profile(user_profile, experiment_id, variation_id) end end - def get_bucketing_id(user_id, attributes, decide_reasons = nil) + def get_bucketing_id(user_id, attributes) # Gets the Bucketing Id for Bucketing # # user_id - String user ID # attributes - Hash user attributes # Returns String representing bucketing ID if it is a String type in attributes else return user ID - return user_id unless attributes + return user_id, nil unless attributes bucketing_id = attributes[Optimizely::Helpers::Constants::CONTROL_ATTRIBUTES['BUCKETING_ID']] if bucketing_id - return bucketing_id if bucketing_id.is_a?(String) + return bucketing_id, nil if bucketing_id.is_a?(String) message = 'Bucketing ID attribute is not a string. Defaulted to user ID.' @logger.log(Logger::WARN, message) - decide_reasons&.push(message) end - user_id + [user_id, message] end end end diff --git a/spec/decision_service_spec.rb b/spec/decision_service_spec.rb index 27dbf762..5ce68e97 100644 --- a/spec/decision_service_spec.rb +++ b/spec/decision_service_spec.rb @@ -695,7 +695,8 @@ 'browser_type' => 'firefox', Optimizely::Helpers::Constants::CONTROL_ATTRIBUTES['BUCKETING_ID'] => 5 } - expect(decision_service.send(:get_bucketing_id, 'test_user', user_attributes)).to eq('test_user') + bucketing_id, = decision_service.send(:get_bucketing_id, 'test_user', user_attributes) + expect(bucketing_id).to eq('test_user') expect(spy_logger).to have_received(:log).once.with(Logger::WARN, 'Bucketing ID attribute is not a string. Defaulted to user ID.') end @@ -704,7 +705,8 @@ 'browser_type' => 'firefox', Optimizely::Helpers::Constants::CONTROL_ATTRIBUTES['BUCKETING_ID'] => nil } - expect(decision_service.send(:get_bucketing_id, 'test_user', user_attributes)).to eq('test_user') + bucketing_id, = decision_service.send(:get_bucketing_id, 'test_user', user_attributes) + expect(bucketing_id).to eq('test_user') expect(spy_logger).not_to have_received(:log) end @@ -713,7 +715,8 @@ 'browser_type' => 'firefox', Optimizely::Helpers::Constants::CONTROL_ATTRIBUTES['BUCKETING_ID'] => 'i_am_bucketing_id' } - expect(decision_service.send(:get_bucketing_id, 'test_user', user_attributes)).to eq('i_am_bucketing_id') + bucketing_id, = decision_service.send(:get_bucketing_id, 'test_user', user_attributes) + expect(bucketing_id).to eq('i_am_bucketing_id') expect(spy_logger).not_to have_received(:log) end @@ -722,7 +725,8 @@ 'browser_type' => 'firefox', Optimizely::Helpers::Constants::CONTROL_ATTRIBUTES['BUCKETING_ID'] => '' } - expect(decision_service.send(:get_bucketing_id, 'test_user', user_attributes)).to eq('') + bucketing_id, = decision_service.send(:get_bucketing_id, 'test_user', user_attributes) + expect(bucketing_id).to eq('') expect(spy_logger).not_to have_received(:log) end end From 055ef6f95f03a027e3d4243fc83faf42cb84cc13 Mon Sep 17 00:00:00 2001 From: Zeeshan Ashraf Date: Tue, 15 Dec 2020 14:44:28 -0800 Subject: [PATCH 03/17] fixed get_variation_for_feature_rollout --- lib/optimizely/decision_service.rb | 38 ++++++++++---------- spec/bucketing_spec.rb | 56 +++++++++++++++--------------- spec/decision_service_spec.rb | 25 ++++++++----- 3 files changed, 64 insertions(+), 55 deletions(-) diff --git a/lib/optimizely/decision_service.rb b/lib/optimizely/decision_service.rb index 491bcc5d..39106c67 100644 --- a/lib/optimizely/decision_service.rb +++ b/lib/optimizely/decision_service.rb @@ -141,7 +141,8 @@ def get_variation_for_feature(project_config, feature_flag, user_id, attributes decision = get_variation_for_feature_experiment(project_config, feature_flag, user_id, attributes, decide_options, decide_reasons) return decision unless decision.nil? - decision = get_variation_for_feature_rollout(project_config, feature_flag, user_id, attributes, decide_reasons) + decision, reasons_recieved = get_variation_for_feature_rollout(project_config, feature_flag, user_id, attributes) + decide_reasons&.push(*reasons_recieved) decision end @@ -191,7 +192,7 @@ def get_variation_for_feature_experiment(project_config, feature_flag, user_id, nil end - def get_variation_for_feature_rollout(project_config, feature_flag, user_id, attributes = nil, decide_reasons = nil) + def get_variation_for_feature_rollout(project_config, feature_flag, user_id, attributes = nil) # Determine which variation the user is in for a given rollout. # Returns the variation of the first experiment the user qualifies for. # @@ -201,26 +202,27 @@ def get_variation_for_feature_rollout(project_config, feature_flag, user_id, att # attributes - Hash representing user attributes # # Returns the Decision struct or nil if not bucketed into any of the targeting rules + decide_reasons = [] bucketing_id, bucketing_id_reasons = get_bucketing_id(user_id, attributes) - decide_reasons&.push(*bucketing_id_reasons) + decide_reasons.push(*bucketing_id_reasons) rollout_id = feature_flag['rolloutId'] if rollout_id.nil? || rollout_id.empty? feature_flag_key = feature_flag['key'] message = "Feature flag '#{feature_flag_key}' is not used in a rollout." @logger.log(Logger::DEBUG, message) - decide_reasons&.push(message) - return nil + decide_reasons.push(message) + return nil, decide_reasons end rollout = project_config.get_rollout_from_id(rollout_id) if rollout.nil? message = "Rollout with ID '#{rollout_id}' is not in the datafile '#{feature_flag['key']}'" @logger.log(Logger::DEBUG, message) - decide_reasons&.push(message) - return nil + decide_reasons.push(message) + return nil, decide_reasons end - return nil if rollout['experiments'].empty? + return nil, decide_reasons if rollout['experiments'].empty? rollout_rules = rollout['experiments'] number_of_rules = rollout_rules.length - 1 @@ -234,19 +236,19 @@ def get_variation_for_feature_rollout(project_config, feature_flag, user_id, att unless Audience.user_meets_audience_conditions?(project_config, rollout_rule, attributes, @logger, 'ROLLOUT_AUDIENCE_EVALUATION_LOGS', logging_key) message = "User '#{user_id}' does not meet the audience conditions for targeting rule '#{logging_key}'." @logger.log(Logger::DEBUG, message) - decide_reasons&.push(message) + decide_reasons.push(message) # move onto the next targeting rule next end message = "User '#{user_id}' meets the audience conditions for targeting rule '#{logging_key}'." @logger.log(Logger::DEBUG, message) - decide_reasons&.push(message) + decide_reasons.push(message) # Evaluate if user satisfies the traffic allocation for this rollout rule 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? + decide_reasons.push(*bucket_reasons) + return Decision.new(rollout_rule, variation, DECISION_SOURCES['ROLLOUT']), decide_reasons unless variation.nil? break end @@ -258,19 +260,19 @@ def get_variation_for_feature_rollout(project_config, feature_flag, user_id, att unless Audience.user_meets_audience_conditions?(project_config, everyone_else_experiment, attributes, @logger, 'ROLLOUT_AUDIENCE_EVALUATION_LOGS', logging_key) message = "User '#{user_id}' does not meet the audience conditions for targeting rule '#{logging_key}'." @logger.log(Logger::DEBUG, message) - decide_reasons&.push(message) - return nil + decide_reasons.push(message) + return nil, decide_reasons end message = "User '#{user_id}' meets the audience conditions for targeting rule '#{logging_key}'." @logger.log(Logger::DEBUG, message) - decide_reasons&.push(message) + decide_reasons.push(message) 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? + decide_reasons.push(*bucket_reasons) + return Decision.new(everyone_else_experiment, variation, DECISION_SOURCES['ROLLOUT']), decide_reasons unless variation.nil? - nil + [nil, decide_reasons] end def set_forced_variation(project_config, experiment_key, user_id, variation_key) diff --git a/spec/bucketing_spec.rb b/spec/bucketing_spec.rb index edc69578..98fd65d9 100644 --- a/spec/bucketing_spec.rb +++ b/spec/bucketing_spec.rb @@ -40,17 +40,17 @@ def get_bucketing_key(bucketing_id, entity_id = nil) # Variation 1 expected_variation_1 = config.get_variation_from_id('test_experiment', '111128') - variation_recieved, = bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user') - expect(variation_recieved).to eq(expected_variation_1) + variation_received, = bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user') + expect(variation_received).to eq(expected_variation_1) # Variation 2 expected_variation_2 = config.get_variation_from_id('test_experiment', '111129') - variation_recieved, = bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user') - expect(variation_recieved).to eq(expected_variation_2) + variation_received, = bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user') + expect(variation_received).to eq(expected_variation_2) # No matching variation - variation_recieved, = bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user') - expect(variation_recieved).to be_nil + variation_received, = bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user') + expect(variation_received).to be_nil end it 'should test the output of generate_bucket_value for different inputs' do @@ -69,8 +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') - variation_recieved, = bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user') - expect(variation_recieved).to eq(expected_variation) + variation_received, = bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user') + expect(variation_received).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'.") @@ -82,8 +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') - variation_recieved, = bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user') - expect(variation_recieved).to be_nil + variation_received, = bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user') + expect(variation_received).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) @@ -94,8 +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') - variation_recieved, = bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user') - expect(variation_recieved).to be_nil + variation_received, = bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user') + expect(variation_received).to be_nil expect(spy_logger).to have_received(:log) .with(Logger::INFO, "User 'test_user' is in no experiment.") end @@ -105,8 +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') - variation_recieved, = bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user') - expect(variation_recieved).to eq(expected_variation) + variation_received, = bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user') + expect(variation_received).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'.") @@ -116,8 +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') - variation_recieved, = bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user') - expect(variation_recieved).to be_nil + variation_received, = bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user') + expect(variation_received).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'.") @@ -146,8 +146,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') - variation_recieved, = bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user') - expect(variation_recieved).to be_nil + variation_received, = bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user') + expect(variation_received).to be_nil expect(spy_logger).to have_received(:log) .with(Logger::DEBUG, 'Bucketed into an empty traffic range. Returning nil.') end @@ -160,20 +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') - variation_recieved, = bucketer.bucket(config, experiment, 'test_user', 'test_user') - expect(variation_recieved).to be(expected_variation) + variation_received, = bucketer.bucket(config, experiment, 'test_user', 'test_user') + expect(variation_received).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') - variation_recieved, = bucketer.bucket(config, experiment, 'any_string789', 'test_user') - expect(variation_recieved).to be(expected_variation) + variation_received, = bucketer.bucket(config, experiment, 'any_string789', 'test_user') + expect(variation_received).to be(expected_variation) end # Bucketing with invalid experiment key and bucketing ID it 'should return nil with invalid experiment and bucketing ID' do - variation_recieved, = bucketer.bucket(config, config.get_experiment_from_key('invalid_experiment'), 'some_id', 'test_user') - expect(variation_recieved).to be(nil) + variation_received, = bucketer.bucket(config, config.get_experiment_from_key('invalid_experiment'), 'some_id', 'test_user') + expect(variation_received).to be(nil) end # Bucketing with grouped experiments and bucketing ID @@ -182,12 +182,12 @@ def get_bucketing_key(bucketing_id, entity_id = nil) experiment = config.get_experiment_from_key('group1_exp1') expected_variation = nil - variation_recieved, = bucketer.bucket(config, experiment, 'test_user', 'test_user') - expect(variation_recieved).to be(expected_variation) + variation_received, = bucketer.bucket(config, experiment, 'test_user', 'test_user') + expect(variation_received).to be(expected_variation) expected_variation = config.get_variation_from_id('group1_exp1', '130002') - variation_recieved, = bucketer.bucket(config, experiment, '123456789', 'test_user') - expect(variation_recieved).to be(expected_variation) + variation_received, = bucketer.bucket(config, experiment, '123456789', 'test_user') + expect(variation_received).to be(expected_variation) end end diff --git a/spec/decision_service_spec.rb b/spec/decision_service_spec.rb index 5ce68e97..50b4e88a 100644 --- a/spec/decision_service_spec.rb +++ b/spec/decision_service_spec.rb @@ -482,8 +482,8 @@ describe 'when the feature flag is not associated with a rollout' do it 'should log a message and return nil' do feature_flag = config.feature_flag_key_map['boolean_feature'] - expect(decision_service.get_variation_for_feature_rollout(config, feature_flag, user_id, user_attributes)).to eq(nil) - + variation_received, = decision_service.get_variation_for_feature_rollout(config, feature_flag, user_id, user_attributes) + expect(variation_received).to eq(nil) expect(spy_logger).to have_received(:log).once .with(Logger::DEBUG, "Feature flag '#{feature_flag['key']}' is not used in a rollout.") end @@ -493,7 +493,8 @@ it 'should log a message and return nil' do feature_flag = config.feature_flag_key_map['boolean_feature'].dup feature_flag['rolloutId'] = 'invalid_rollout_id' - expect(decision_service.get_variation_for_feature_rollout(config, feature_flag, user_id, user_attributes)).to eq(nil) + variation_received, = decision_service.get_variation_for_feature_rollout(config, feature_flag, user_id, user_attributes) + expect(variation_received).to eq(nil) expect(spy_logger).to have_received(:log).once .with(Logger::ERROR, "Rollout with ID 'invalid_rollout_id' is not in the datafile.") @@ -506,7 +507,8 @@ experimentless_rollout['experiments'] = [] allow(config).to receive(:get_rollout_from_id).and_return(experimentless_rollout) feature_flag = config.feature_flag_key_map['boolean_single_variable_feature'] - expect(decision_service.get_variation_for_feature_rollout(config, feature_flag, user_id, user_attributes)).to eq(nil) + variation_received, = decision_service.get_variation_for_feature_rollout(config, feature_flag, user_id, user_attributes) + expect(variation_received).to eq(nil) end end @@ -521,7 +523,8 @@ allow(decision_service.bucketer).to receive(:bucket) .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) + variation_received, = decision_service.get_variation_for_feature_rollout(config, feature_flag, user_id, user_attributes) + expect(variation_received).to eq(expected_decision) end end @@ -540,7 +543,8 @@ .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) + variation_received, = decision_service.get_variation_for_feature_rollout(config, feature_flag, user_id, user_attributes) + expect(variation_received).to eq(nil) # make sure we only checked the audience for the first rule expect(Optimizely::Audience).to have_received(:user_meets_audience_conditions?).once @@ -565,7 +569,8 @@ .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) + variation_received, = decision_service.get_variation_for_feature_rollout(config, feature_flag, user_id, user_attributes) + expect(variation_received).to eq(expected_decision) # make sure we only checked the audience for the first rule expect(Optimizely::Audience).to have_received(:user_meets_audience_conditions?).once @@ -593,7 +598,8 @@ .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) + variation_received, = decision_service.get_variation_for_feature_rollout(config, feature_flag, user_id, user_attributes) + expect(variation_received).to eq(expected_decision) # verify we tried to bucket in all targeting rules and the everyone else rule expect(Optimizely::Audience).to have_received(:user_meets_audience_conditions?) @@ -621,7 +627,8 @@ expect(decision_service.bucketer).not_to receive(:bucket) .with(config, everyone_else_experiment, user_id, user_id) - expect(decision_service.get_variation_for_feature_rollout(config, feature_flag, user_id, user_attributes)).to eq(nil) + variation_received, = decision_service.get_variation_for_feature_rollout(config, feature_flag, user_id, user_attributes) + expect(variation_received).to eq(nil) # verify we tried to bucket in all targeting rules and the everyone else rule expect(Optimizely::Audience).to have_received(:user_meets_audience_conditions?).once From 5d118861bef275e3e14af09300b62ce0042adeef Mon Sep 17 00:00:00 2001 From: Zeeshan Ashraf Date: Tue, 15 Dec 2020 17:52:37 -0800 Subject: [PATCH 04/17] fixed get_forced_variation --- lib/optimizely.rb | 2 +- lib/optimizely/decision_service.rb | 26 ++++++++++++++------------ spec/decision_service_spec.rb | 18 ++++++++++-------- 3 files changed, 25 insertions(+), 21 deletions(-) diff --git a/lib/optimizely.rb b/lib/optimizely.rb index 504eeaf7..e97ec4a1 100644 --- a/lib/optimizely.rb +++ b/lib/optimizely.rb @@ -405,7 +405,7 @@ def get_forced_variation(experiment_key, user_id) config = project_config forced_variation_key = nil - forced_variation = @decision_service.get_forced_variation(config, experiment_key, user_id) + forced_variation, = @decision_service.get_forced_variation(config, experiment_key, user_id) forced_variation_key = forced_variation['key'] if forced_variation forced_variation_key diff --git a/lib/optimizely/decision_service.rb b/lib/optimizely/decision_service.rb index 39106c67..b8a6a221 100644 --- a/lib/optimizely/decision_service.rb +++ b/lib/optimizely/decision_service.rb @@ -79,7 +79,8 @@ def get_variation(project_config, experiment_key, user_id, attributes = nil, dec end # Check if a forced variation is set for the user - forced_variation = get_forced_variation(project_config, experiment_key, user_id, decide_reasons) + forced_variation, reasons_received = get_forced_variation(project_config, experiment_key, user_id) + decide_reasons&.push(*reasons_received) return forced_variation['id'] if forced_variation # Check if user is in a white-listed variation @@ -141,8 +142,8 @@ def get_variation_for_feature(project_config, feature_flag, user_id, attributes decision = get_variation_for_feature_experiment(project_config, feature_flag, user_id, attributes, decide_options, decide_reasons) return decision unless decision.nil? - decision, reasons_recieved = get_variation_for_feature_rollout(project_config, feature_flag, user_id, attributes) - decide_reasons&.push(*reasons_recieved) + decision, reasons_received = get_variation_for_feature_rollout(project_config, feature_flag, user_id, attributes) + decide_reasons&.push(*reasons_received) decision end @@ -313,7 +314,7 @@ def set_forced_variation(project_config, experiment_key, user_id, variation_key) true end - def get_forced_variation(project_config, experiment_key, user_id, decide_reasons = nil) + def get_forced_variation(project_config, experiment_key, user_id) # Gets the forced variation for the given user and experiment. # # project_config - Instance of ProjectConfig @@ -322,11 +323,12 @@ def get_forced_variation(project_config, experiment_key, user_id, decide_reasons # # Returns Variation The variation which the given user and experiment should be forced into + decide_reasons = [] unless @forced_variation_map.key? user_id message = "User '#{user_id}' is not in the forced variation map." @logger.log(Logger::DEBUG, message) - decide_reasons&.push(message) - return nil + decide_reasons.push(message) + return nil, decide_reasons end experiment_to_variation_map = @forced_variation_map[user_id] @@ -334,13 +336,13 @@ def get_forced_variation(project_config, experiment_key, user_id, decide_reasons experiment_id = experiment['id'] if experiment # check for nil and empty string experiment ID # this case is logged in get_experiment_from_key - return nil if experiment_id.nil? || experiment_id.empty? + return nil, decide_reasons if experiment_id.nil? || experiment_id.empty? unless experiment_to_variation_map.key? experiment_id message = "No experiment '#{experiment_key}' mapped to user '#{user_id}' in the forced variation map." @logger.log(Logger::DEBUG, message) - decide_reasons&.push(message) - return nil + decide_reasons.push(message) + return nil, decide_reasons end variation_id = experiment_to_variation_map[experiment_id] @@ -350,13 +352,13 @@ def get_forced_variation(project_config, experiment_key, user_id, decide_reasons # check if the variation exists in the datafile # this case is logged in get_variation_from_id - return nil if variation_key.empty? + return nil, decide_reasons if variation_key.empty? message = "Variation '#{variation_key}' is mapped to experiment '#{experiment_key}' and user '#{user_id}' in the forced variation map" @logger.log(Logger::DEBUG, message) - decide_reasons&.push(message) + decide_reasons.push(message) - variation + [variation, decide_reasons] end private diff --git a/spec/decision_service_spec.rb b/spec/decision_service_spec.rb index 50b4e88a..f0bdc249 100644 --- a/spec/decision_service_spec.rb +++ b/spec/decision_service_spec.rb @@ -747,13 +747,15 @@ # User ID is not defined in the forced variation map it 'should log a message and return nil when user is not in forced variation map' do - expect(decision_service.get_forced_variation(config, valid_experiment[:key], user_id)).to eq(nil) + variation_received, = decision_service.get_forced_variation(config, valid_experiment[:key], user_id) + expect(variation_received).to eq(nil) expect(spy_logger).to have_received(:log).with(Logger::DEBUG, "User '#{user_id}' is not in the forced variation map.") end # Experiment key does not exist in the datafile it 'should return nil when experiment key is not in datafile' do - expect(decision_service.get_forced_variation(config, invalid_experiment_key, user_id)).to eq(nil) + variation_received, = decision_service.get_forced_variation(config, invalid_experiment_key, user_id) + expect(variation_received).to eq(nil) end end @@ -787,12 +789,12 @@ # Call set variation with different variations on one user/experiment to confirm that each set is expected. it 'should set and return expected variations when different variations are set and removed for one user/experiment' do expect(decision_service.set_forced_variation(config, valid_experiment[:key], user_id, valid_variation[:key])).to eq(true) - variation = decision_service.get_forced_variation(config, valid_experiment[:key], user_id) + variation, = decision_service.get_forced_variation(config, valid_experiment[:key], user_id) expect(variation['id']).to eq(valid_variation[:id]) expect(variation['key']).to eq(valid_variation[:key]) expect(decision_service.set_forced_variation(config, valid_experiment[:key], user_id, valid_variation_2[:key])).to eq(true) - variation = decision_service.get_forced_variation(config, valid_experiment[:key], user_id) + variation, = decision_service.get_forced_variation(config, valid_experiment[:key], user_id) expect(variation['id']).to eq(valid_variation_2[:id]) expect(variation['key']).to eq(valid_variation_2[:key]) end @@ -800,12 +802,12 @@ # Set variation on multiple experiments for one user. it 'should set and return expected variations when variation is set for multiple experiments for one user' do expect(decision_service.set_forced_variation(config, valid_experiment[:key], user_id, valid_variation[:key])).to eq(true) - variation = decision_service.get_forced_variation(config, valid_experiment[:key], user_id) + variation, = decision_service.get_forced_variation(config, valid_experiment[:key], user_id) expect(variation['id']).to eq(valid_variation[:id]) expect(variation['key']).to eq(valid_variation[:key]) expect(decision_service.set_forced_variation(config, valid_experiment_2[:key], user_id, valid_variation_for_exp_2[:key])).to eq(true) - variation = decision_service.get_forced_variation(config, valid_experiment_2[:key], user_id) + variation, = decision_service.get_forced_variation(config, valid_experiment_2[:key], user_id) expect(variation['id']).to eq(valid_variation_for_exp_2[:id]) expect(variation['key']).to eq(valid_variation_for_exp_2[:key]) end @@ -813,12 +815,12 @@ # Set variations for multiple users. it 'should set and return expected variations when variations are set for multiple users' do expect(decision_service.set_forced_variation(config, valid_experiment[:key], user_id, valid_variation[:key])).to eq(true) - variation = decision_service.get_forced_variation(config, valid_experiment[:key], user_id) + variation, = decision_service.get_forced_variation(config, valid_experiment[:key], user_id) expect(variation['id']).to eq(valid_variation[:id]) expect(variation['key']).to eq(valid_variation[:key]) expect(decision_service.set_forced_variation(config, valid_experiment[:key], user_id_2, valid_variation[:key])).to eq(true) - variation = decision_service.get_forced_variation(config, valid_experiment[:key], user_id_2) + variation, = decision_service.get_forced_variation(config, valid_experiment[:key], user_id_2) expect(variation['id']).to eq(valid_variation[:id]) expect(variation['key']).to eq(valid_variation[:key]) end From 7cbf993c014e6d1fd094644804555d1b1251afc9 Mon Sep 17 00:00:00 2001 From: Zeeshan Ashraf Date: Tue, 15 Dec 2020 18:19:19 -0800 Subject: [PATCH 05/17] fixed a few more methods --- lib/optimizely/decision_service.rb | 40 +++++++++++++++--------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/lib/optimizely/decision_service.rb b/lib/optimizely/decision_service.rb index b8a6a221..8ac2216c 100644 --- a/lib/optimizely/decision_service.rb +++ b/lib/optimizely/decision_service.rb @@ -84,14 +84,17 @@ def get_variation(project_config, experiment_key, user_id, attributes = nil, dec return forced_variation['id'] if forced_variation # Check if user is in a white-listed variation - whitelisted_variation_id = get_whitelisted_variation_id(project_config, experiment_key, user_id, decide_reasons) + whitelisted_variation_id, reasons_received = get_whitelisted_variation_id(project_config, experiment_key, user_id) + decide_reasons&.push(*reasons_received) return whitelisted_variation_id if whitelisted_variation_id should_ignore_user_profile_service = decide_options.include? Optimizely::Decide::OptimizelyDecideOption::IGNORE_USER_PROFILE_SERVICE # Check for saved bucketing decisions if decide_options do not include ignoreUserProfileService unless should_ignore_user_profile_service - user_profile = get_user_profile(user_id, decide_reasons) - saved_variation_id = get_saved_variation_id(project_config, experiment_id, user_profile, decide_reasons) + user_profile, reasons_received = get_user_profile(user_id) + decide_reasons&.push(*reasons_received) + saved_variation_id, reasons_received = get_saved_variation_id(project_config, experiment_id, user_profile) + decide_reasons&.push(*reasons_received) if saved_variation_id message = "Returning previously activated variation ID #{saved_variation_id} of experiment '#{experiment_key}' for user '#{user_id}' from user profile." @logger.log(Logger::INFO, message) @@ -363,7 +366,7 @@ def get_forced_variation(project_config, experiment_key, user_id) private - def get_whitelisted_variation_id(project_config, experiment_key, user_id, decide_reasons = nil) + def get_whitelisted_variation_id(project_config, experiment_key, user_id) # Determine if a user is whitelisted into a variation for the given experiment and return the ID of that variation # # project_config - project_config - Instance of ProjectConfig @@ -374,29 +377,27 @@ def get_whitelisted_variation_id(project_config, experiment_key, user_id, decide whitelisted_variations = project_config.get_whitelisted_variations(experiment_key) - return nil unless whitelisted_variations + return nil, nil unless whitelisted_variations whitelisted_variation_key = whitelisted_variations[user_id] - return nil unless whitelisted_variation_key + return nil, nil unless whitelisted_variation_key whitelisted_variation_id = project_config.get_variation_id_from_key(experiment_key, whitelisted_variation_key) unless whitelisted_variation_id message = "User '#{user_id}' is whitelisted into variation '#{whitelisted_variation_key}', which is not in the datafile." @logger.log(Logger::INFO, message) - decide_reasons&.push(message) - return nil + return nil, message end message = "User '#{user_id}' is whitelisted into variation '#{whitelisted_variation_key}' of experiment '#{experiment_key}'." @logger.log(Logger::INFO, message) - decide_reasons&.push(message) - whitelisted_variation_id + [whitelisted_variation_id, message] end - def get_saved_variation_id(project_config, experiment_id, user_profile, decide_reasons = nil) + def get_saved_variation_id(project_config, experiment_id, user_profile) # Retrieve variation ID of stored bucketing decision for a given experiment from a given user profile # # project_config - project_config - Instance of ProjectConfig @@ -404,22 +405,21 @@ def get_saved_variation_id(project_config, experiment_id, user_profile, decide_r # user_profile - Hash user profile # # Returns string variation ID (nil if no decision is found) - return nil unless user_profile[:experiment_bucket_map] + return nil, nil unless user_profile[:experiment_bucket_map] decision = user_profile[:experiment_bucket_map][experiment_id] - return nil unless decision + return nil, nil unless decision variation_id = decision[:variation_id] - return variation_id if project_config.variation_id_exists?(experiment_id, variation_id) + return variation_id, nil if project_config.variation_id_exists?(experiment_id, variation_id) message = "User '#{user_profile['user_id']}' was previously bucketed into variation ID '#{variation_id}' for experiment '#{experiment_id}', but no matching variation was found. Re-bucketing user." @logger.log(Logger::INFO, message) - decide_reasons&.push(message) - nil + [nil, message] end - def get_user_profile(user_id, decide_reasons = nil) + def get_user_profile(user_id) # Determine if a user is forced into a variation for the given experiment and return the ID of that variation # # user_id - String ID for the user @@ -431,17 +431,17 @@ def get_user_profile(user_id, decide_reasons = nil) experiment_bucket_map: {} } - return user_profile unless @user_profile_service + return user_profile, nil unless @user_profile_service + message = nil begin user_profile = @user_profile_service.lookup(user_id) || user_profile rescue => e message = "Error while looking up user profile for user ID '#{user_id}': #{e}." @logger.log(Logger::ERROR, message) - decide_reasons&.push(message) end - user_profile + [user_profile, message] end def save_user_profile(user_profile, experiment_id, variation_id) From 37bf64dc91641b70fd986fabfd650f771262c0ec Mon Sep 17 00:00:00 2001 From: Zeeshan Ashraf Date: Tue, 15 Dec 2020 19:17:46 -0800 Subject: [PATCH 06/17] fixed get_variatopm --- lib/optimizely.rb | 2 +- lib/optimizely/decision_service.rb | 40 ++++++++------- spec/decision_service_spec.rb | 82 +++++++++++++++++++----------- spec/project_spec.rb | 2 +- 4 files changed, 76 insertions(+), 50 deletions(-) diff --git a/lib/optimizely.rb b/lib/optimizely.rb index e97ec4a1..5ca3be09 100644 --- a/lib/optimizely.rb +++ b/lib/optimizely.rb @@ -878,7 +878,7 @@ def get_variation_with_config(experiment_key, user_id, attributes, config) return nil unless user_inputs_valid?(attributes) - variation_id = @decision_service.get_variation(config, experiment_key, user_id, attributes) + variation_id, = @decision_service.get_variation(config, experiment_key, user_id, attributes) variation = config.get_variation_from_id(experiment_key, variation_id) unless variation_id.nil? variation_key = variation['key'] if variation decision_notification_type = if config.feature_experiment?(experiment['id']) diff --git a/lib/optimizely/decision_service.rb b/lib/optimizely/decision_service.rb index 8ac2216c..b05a7cb6 100644 --- a/lib/optimizely/decision_service.rb +++ b/lib/optimizely/decision_service.rb @@ -52,7 +52,7 @@ def initialize(logger, user_profile_service = nil) @forced_variation_map = {} end - def get_variation(project_config, experiment_key, user_id, attributes = nil, decide_options = [], decide_reasons = nil) + def get_variation(project_config, experiment_key, user_id, attributes = nil, decide_options = []) # Determines variation into which user will be bucketed. # # project_config - project_config - Instance of ProjectConfig @@ -63,43 +63,44 @@ def get_variation(project_config, experiment_key, user_id, attributes = nil, dec # Returns variation ID where visitor will be bucketed # (nil if experiment is inactive or user does not meet audience conditions) + decide_reasons = [] # By default, the bucketing ID should be the user ID bucketing_id, bucketing_id_reasons = get_bucketing_id(user_id, attributes) - decide_reasons&.push(*bucketing_id_reasons) + decide_reasons.push(*bucketing_id_reasons) # Check to make sure experiment is active experiment = project_config.get_experiment_from_key(experiment_key) - return nil if experiment.nil? + return nil, decide_reasons if experiment.nil? experiment_id = experiment['id'] unless project_config.experiment_running?(experiment) message = "Experiment '#{experiment_key}' is not running." @logger.log(Logger::INFO, message) - decide_reasons&.push(message) - return nil + decide_reasons.push(message) + return nil, decide_reasons end # Check if a forced variation is set for the user forced_variation, reasons_received = get_forced_variation(project_config, experiment_key, user_id) - decide_reasons&.push(*reasons_received) - return forced_variation['id'] if forced_variation + decide_reasons.push(*reasons_received) + return forced_variation['id'], decide_reasons if forced_variation # Check if user is in a white-listed variation whitelisted_variation_id, reasons_received = get_whitelisted_variation_id(project_config, experiment_key, user_id) - decide_reasons&.push(*reasons_received) - return whitelisted_variation_id if whitelisted_variation_id + decide_reasons.push(*reasons_received) + return whitelisted_variation_id, decide_reasons if whitelisted_variation_id should_ignore_user_profile_service = decide_options.include? Optimizely::Decide::OptimizelyDecideOption::IGNORE_USER_PROFILE_SERVICE # Check for saved bucketing decisions if decide_options do not include ignoreUserProfileService unless should_ignore_user_profile_service user_profile, reasons_received = get_user_profile(user_id) - decide_reasons&.push(*reasons_received) + decide_reasons.push(*reasons_received) saved_variation_id, reasons_received = get_saved_variation_id(project_config, experiment_id, user_profile) - decide_reasons&.push(*reasons_received) + decide_reasons.push(*reasons_received) if saved_variation_id message = "Returning previously activated variation ID #{saved_variation_id} of experiment '#{experiment_key}' for user '#{user_id}' from user profile." @logger.log(Logger::INFO, message) - decide_reasons&.push(message) - return saved_variation_id + decide_reasons.push(message) + return saved_variation_id, decide_reasons end end @@ -107,13 +108,13 @@ def get_variation(project_config, experiment_key, user_id, attributes = nil, dec unless Audience.user_meets_audience_conditions?(project_config, experiment, attributes, @logger) message = "User '#{user_id}' does not meet the conditions to be in experiment '#{experiment_key}'." @logger.log(Logger::INFO, message) - decide_reasons&.push(message) - return nil + decide_reasons.push(message) + return nil, decide_reasons end # Bucket normally variation, bucket_reasons = @bucketer.bucket(project_config, experiment, bucketing_id, user_id) - decide_reasons&.push(*bucket_reasons) + decide_reasons.push(*bucket_reasons) variation_id = variation ? variation['id'] : nil message = '' @@ -124,11 +125,11 @@ def get_variation(project_config, experiment_key, user_id, attributes = nil, dec message = "User '#{user_id}' is in no variation." end @logger.log(Logger::INFO, message) - decide_reasons&.push(message) + decide_reasons.push(message) # Persist bucketing decision save_user_profile(user_profile, experiment_id, variation_id) unless should_ignore_user_profile_service - variation_id + [variation_id, decide_reasons] end def get_variation_for_feature(project_config, feature_flag, user_id, attributes = nil, decide_options = [], decide_reasons = nil) @@ -180,7 +181,8 @@ def get_variation_for_feature_experiment(project_config, feature_flag, user_id, end experiment_key = experiment['key'] - variation_id = get_variation(project_config, experiment_key, user_id, attributes, decide_options, decide_reasons) + variation_id, reasons_received = get_variation(project_config, experiment_key, user_id, attributes, decide_options) + decide_reasons&.push(*reasons_received) next unless variation_id diff --git a/spec/decision_service_spec.rb b/spec/decision_service_spec.rb index f0bdc249..f01a4ce9 100644 --- a/spec/decision_service_spec.rb +++ b/spec/decision_service_spec.rb @@ -42,7 +42,8 @@ it 'should return the correct variation ID for a given user for whom a variation has been forced' do decision_service.set_forced_variation(config, 'test_experiment', 'test_user', 'variation') - expect(decision_service.get_variation(config, 'test_experiment', 'test_user')).to eq('111129') + variation_received, = decision_service.get_variation(config, 'test_experiment', 'test_user') + expect(variation_received).to eq('111129') # Setting forced variation should short circuit whitelist check, bucketing and audience evaluation expect(decision_service).not_to have_received(:get_whitelisted_variation_id) expect(decision_service.bucketer).not_to have_received(:bucket) @@ -55,7 +56,8 @@ Optimizely::Helpers::Constants::CONTROL_ATTRIBUTES['BUCKETING_ID'] => 'pid' } decision_service.set_forced_variation(config, 'test_experiment_with_audience', 'test_user', 'control_with_audience') - expect(decision_service.get_variation(config, 'test_experiment_with_audience', 'test_user', user_attributes)).to eq('122228') + variation_received, = decision_service.get_variation(config, 'test_experiment_with_audience', 'test_user', user_attributes) + expect(variation_received).to eq('122228') # Setting forced variation should short circuit whitelist check, bucketing and audience evaluation expect(decision_service).not_to have_received(:get_whitelisted_variation_id) expect(decision_service.bucketer).not_to have_received(:bucket) @@ -63,7 +65,8 @@ end it 'should return the correct variation ID for a given user ID and key of a running experiment' do - expect(decision_service.get_variation(config, 'test_experiment', 'test_user')).to eq('111128') + variation_received, = decision_service.get_variation(config, 'test_experiment', 'test_user') + expect(variation_received).to eq('111128') expect(spy_logger).to have_received(:log) .once.with(Logger::INFO, "User 'test_user' is in variation 'control' of experiment 'test_experiment'.") @@ -73,18 +76,21 @@ it 'should return nil when user ID is not bucketed' do allow(decision_service.bucketer).to receive(:bucket).and_return(nil) - expect(decision_service.get_variation(config, 'test_experiment', 'test_user')).to eq(nil) + variation_received, = decision_service.get_variation(config, 'test_experiment', 'test_user') + expect(variation_received).to eq(nil) expect(spy_logger).to have_received(:log) .once.with(Logger::INFO, "User 'test_user' is in no variation.") end it 'should return correct variation ID if user ID is in whitelisted Variations and variation is valid' do - expect(decision_service.get_variation(config, 'test_experiment', 'forced_user1')).to eq('111128') + variation_received, = decision_service.get_variation(config, 'test_experiment', 'forced_user1') + expect(variation_received).to eq('111128') expect(spy_logger).to have_received(:log) .once.with(Logger::INFO, "User 'forced_user1' is whitelisted into variation 'control' of experiment 'test_experiment'.") - expect(decision_service.get_variation(config, 'test_experiment', 'forced_user2')).to eq('111129') + variation_received, = decision_service.get_variation(config, 'test_experiment', 'forced_user2') + expect(variation_received).to eq('111129') expect(spy_logger).to have_received(:log) .once.with(Logger::INFO, "User 'forced_user2' is whitelisted into variation 'variation' of experiment 'test_experiment'.") @@ -99,11 +105,14 @@ 'browser_type' => 'firefox', Optimizely::Helpers::Constants::CONTROL_ATTRIBUTES['BUCKETING_ID'] => 'pid' } - expect(decision_service.get_variation(config, 'test_experiment', 'forced_user1', user_attributes)).to eq('111128') + + variation_received, = decision_service.get_variation(config, 'test_experiment', 'forced_user1', user_attributes) + expect(variation_received).to eq('111128') expect(spy_logger).to have_received(:log) .once.with(Logger::INFO, "User 'forced_user1' is whitelisted into variation 'control' of experiment 'test_experiment'.") - expect(decision_service.get_variation(config, 'test_experiment', 'forced_user2', user_attributes)).to eq('111129') + variation_received, = decision_service.get_variation(config, 'test_experiment', 'forced_user2', user_attributes) + expect(variation_received).to eq('111129') expect(spy_logger).to have_received(:log) .once.with(Logger::INFO, "User 'forced_user2' is whitelisted into variation 'variation' of experiment 'test_experiment'.") @@ -115,7 +124,8 @@ it 'should return the correct variation ID for a user in a whitelisted variation (even when audience conditions do not match)' do user_attributes = {'browser_type' => 'wrong_browser'} - expect(decision_service.get_variation(config, 'test_experiment_with_audience', 'forced_audience_user', user_attributes)).to eq('122229') + variation_received, = decision_service.get_variation(config, 'test_experiment_with_audience', 'forced_audience_user', user_attributes) + expect(variation_received).to eq('122229') expect(spy_logger).to have_received(:log) .once.with( Logger::INFO, @@ -129,7 +139,8 @@ end it 'should return nil if the experiment key is invalid' do - expect(decision_service.get_variation(config, 'totally_invalid_experiment', 'test_user', {})).to eq(nil) + variation_received, = decision_service.get_variation(config, 'totally_invalid_experiment', 'test_user', {}) + expect(variation_received).to eq(nil) expect(spy_logger).to have_received(:log) .once.with(Logger::ERROR, "Experiment key 'totally_invalid_experiment' is not in datafile.") @@ -137,7 +148,8 @@ it 'should return nil if the user does not meet the audience conditions for a given experiment' do user_attributes = {'browser_type' => 'chrome'} - expect(decision_service.get_variation(config, 'test_experiment_with_audience', 'test_user', user_attributes)).to eq(nil) + variation_received, = decision_service.get_variation(config, 'test_experiment_with_audience', 'test_user', user_attributes) + expect(variation_received).to eq(nil) expect(spy_logger).to have_received(:log) .once.with(Logger::INFO, "User 'test_user' does not meet the conditions to be in experiment 'test_experiment_with_audience'.") @@ -148,7 +160,8 @@ end it 'should return nil if the given experiment is not running' do - expect(decision_service.get_variation(config, 'test_experiment_not_started', 'test_user')).to eq(nil) + variation_received, = decision_service.get_variation(config, 'test_experiment_not_started', 'test_user') + expect(variation_received).to eq(nil) expect(spy_logger).to have_received(:log) .once.with(Logger::INFO, "Experiment 'test_experiment_not_started' is not running.") @@ -161,7 +174,8 @@ end it 'should respect forced variations within mutually exclusive grouped experiments' do - expect(decision_service.get_variation(config, 'group1_exp2', 'forced_group_user1')).to eq('130004') + variation_received, = decision_service.get_variation(config, 'group1_exp2', 'forced_group_user1') + expect(variation_received).to eq('130004') expect(spy_logger).to have_received(:log) .once.with(Logger::INFO, "User 'forced_group_user1' is whitelisted into variation 'g1_e2_v2' of experiment 'group1_exp2'.") @@ -172,7 +186,8 @@ end it 'should bucket normally if user is whitelisted into a forced variation that is not in the datafile' do - expect(decision_service.get_variation(config, 'test_experiment', 'forced_user_with_invalid_variation')).to eq('111128') + variation_received, = decision_service.get_variation(config, 'test_experiment', 'forced_user_with_invalid_variation') + expect(variation_received).to eq('111128') expect(spy_logger).to have_received(:log) .once.with( Logger::INFO, @@ -196,7 +211,8 @@ } expect(spy_user_profile_service).to receive(:lookup).once.and_return(nil) - expect(decision_service.get_variation(config, 'test_experiment', 'test_user')).to eq('111128') + variation_received, = decision_service.get_variation(config, 'test_experiment', 'test_user') + expect(variation_received).to eq('111128') # bucketing should have occurred expect(decision_service.bucketer).to have_received(:bucket).once @@ -221,7 +237,8 @@ } expect(spy_user_profile_service).to receive(:lookup).once.and_return(nil) - expect(decision_service.get_variation(config, 'test_experiment', 'test_user', user_attributes)).to eq('111129') + variation_received, = decision_service.get_variation(config, 'test_experiment', 'test_user', user_attributes) + expect(variation_received).to eq('111129') # bucketing should have occurred expect(decision_service.bucketer).to have_received(:bucket).once @@ -243,7 +260,8 @@ expect(spy_user_profile_service).to receive(:lookup) .with('test_user').once.and_return(saved_user_profile) - expect(decision_service.get_variation(config, 'test_experiment', 'test_user')).to eq('111129') + variation_received, = decision_service.get_variation(config, 'test_experiment', 'test_user') + expect(variation_received).to eq('111129') expect(spy_logger).to have_received(:log).once .with(Logger::INFO, "Returning previously activated variation ID 111129 of experiment 'test_experiment' for user 'test_user' from user profile.") @@ -268,7 +286,8 @@ expect(spy_user_profile_service).to receive(:lookup) .once.with('test_user').and_return(saved_user_profile) - expect(decision_service.get_variation(config, 'test_experiment', 'test_user')).to eq('111128') + variation_received, = decision_service.get_variation(config, 'test_experiment', 'test_user') + expect(variation_received).to eq('111128') # bucketing should have occurred expect(decision_service.bucketer).to have_received(:bucket).once @@ -301,7 +320,8 @@ expect(spy_user_profile_service).to receive(:lookup) .once.with('test_user').and_return(saved_user_profile) - expect(decision_service.get_variation(config, 'test_experiment', 'test_user')).to eq('111128') + variation_received, = decision_service.get_variation(config, 'test_experiment', 'test_user') + expect(variation_received).to eq('111128') # bucketing should have occurred expect(decision_service.bucketer).to have_received(:bucket).once @@ -321,7 +341,8 @@ it 'should bucket normally if the user profile service throws an error during lookup' do expect(spy_user_profile_service).to receive(:lookup).once.with('test_user').and_throw(:LookupError) - expect(decision_service.get_variation(config, 'test_experiment', 'test_user')).to eq('111128') + variation_received, = decision_service.get_variation(config, 'test_experiment', 'test_user') + expect(variation_received).to eq('111128') expect(spy_logger).to have_received(:log).once .with(Logger::ERROR, "Error while looking up user profile for user ID 'test_user': uncaught throw :LookupError.") @@ -332,7 +353,8 @@ it 'should log an error if the user profile service throws an error during save' do expect(spy_user_profile_service).to receive(:save).once.and_throw(:SaveError) - expect(decision_service.get_variation(config, 'test_experiment', 'test_user')).to eq('111128') + variation_received, = decision_service.get_variation(config, 'test_experiment', 'test_user') + expect(variation_received).to eq('111128') expect(spy_logger).to have_received(:log).once .with(Logger::ERROR, "Error while saving user profile for user ID 'test_user': uncaught throw :SaveError.") @@ -343,7 +365,8 @@ allow(spy_user_profile_service).to receive(:lookup) .with('test_user').once.and_return(nil) - expect(decision_service.get_variation(config, 'test_experiment', 'test_user', nil, [Optimizely::Decide::OptimizelyDecideOption::IGNORE_USER_PROFILE_SERVICE])).to eq('111128') + variation_received, = decision_service.get_variation(config, 'test_experiment', 'test_user', nil, [Optimizely::Decide::OptimizelyDecideOption::IGNORE_USER_PROFILE_SERVICE]) + expect(variation_received).to eq('111128') expect(decision_service.bucketer).to have_received(:bucket) expect(Optimizely::Audience).to have_received(:user_meets_audience_conditions?) @@ -355,7 +378,8 @@ allow(spy_user_profile_service).to receive(:lookup) .with('test_user').once.and_return(nil) - expect(decision_service.get_variation(config, 'test_experiment', 'test_user')).to eq('111128') + variation_received, = decision_service.get_variation(config, 'test_experiment', 'test_user') + expect(variation_received).to eq('111128') expect(decision_service.bucketer).to have_received(:bucket) expect(Optimizely::Audience).to have_received(:user_meets_audience_conditions?) @@ -398,8 +422,8 @@ # make sure the user is not bucketed into the feature experiment allow(decision_service).to receive(:get_variation) - .with(config, multivariate_experiment['key'], 'user_1', user_attributes, [], nil) - .and_return(nil) + .with(config, multivariate_experiment['key'], 'user_1', user_attributes, []) + .and_return([nil, nil]) end it 'should return nil and log a message' do @@ -457,11 +481,11 @@ mutex_exp = config.experiment_key_map['group1_exp1'] mutex_exp2 = config.experiment_key_map['group1_exp2'] allow(decision_service).to receive(:get_variation) - .with(config, mutex_exp['key'], user_id, user_attributes, [], nil) - .and_return(nil) + .with(config, mutex_exp['key'], user_id, user_attributes, []) + .and_return([nil, nil]) allow(decision_service).to receive(:get_variation) - .with(config, mutex_exp2['key'], user_id, user_attributes, [], nil) - .and_return(nil) + .with(config, mutex_exp2['key'], user_id, user_attributes, []) + .and_return([nil, nil]) end it 'should return nil and log a message' do diff --git a/spec/project_spec.rb b/spec/project_spec.rb index b91685ce..e7be42de 100644 --- a/spec/project_spec.rb +++ b/spec/project_spec.rb @@ -214,7 +214,7 @@ class InvalidErrorHandler; end params = @expected_activate_params variation_to_return = project_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.decision_service.bucketer).to receive(:bucket).and_return([variation_to_return, nil]) allow(project_instance.event_dispatcher).to receive(:dispatch_event).with(instance_of(Optimizely::Event)) allow(project_config).to receive(:get_audience_ids_for_experiment) .with('test_experiment') From ce727a97c53b2509df328a8b9ff5504ffb2de729 Mon Sep 17 00:00:00 2001 From: Zeeshan Ashraf Date: Tue, 15 Dec 2020 19:27:04 -0800 Subject: [PATCH 07/17] fixed get_variation_for_feature_experiment --- lib/optimizely/decision_service.rb | 22 ++++++++++++---------- spec/decision_service_spec.rb | 18 ++++++++++++------ 2 files changed, 24 insertions(+), 16 deletions(-) diff --git a/lib/optimizely/decision_service.rb b/lib/optimizely/decision_service.rb index b05a7cb6..97d50c8d 100644 --- a/lib/optimizely/decision_service.rb +++ b/lib/optimizely/decision_service.rb @@ -143,7 +143,8 @@ def get_variation_for_feature(project_config, feature_flag, user_id, attributes # Returns Decision struct (nil if the user is not bucketed into any of the experiments on the feature) # check if the feature is being experiment on and whether the user is bucketed into the experiment - decision = get_variation_for_feature_experiment(project_config, feature_flag, user_id, attributes, decide_options, decide_reasons) + decision, reasons_received = get_variation_for_feature_experiment(project_config, feature_flag, user_id, attributes, decide_options) + decide_reasons&.push(*reasons_received) return decision unless decision.nil? decision, reasons_received = get_variation_for_feature_rollout(project_config, feature_flag, user_id, attributes) @@ -152,7 +153,7 @@ def get_variation_for_feature(project_config, feature_flag, user_id, attributes decision end - def get_variation_for_feature_experiment(project_config, feature_flag, user_id, attributes = nil, decide_options = [], decide_reasons = nil) + def get_variation_for_feature_experiment(project_config, feature_flag, user_id, attributes = nil, decide_options = []) # Gets the variation the user is bucketed into for the feature flag's experiment. # # project_config - project_config - Instance of ProjectConfig @@ -162,12 +163,13 @@ def get_variation_for_feature_experiment(project_config, feature_flag, user_id, # # Returns Decision struct (nil if the user is not bucketed into any of the experiments on the feature) # or nil if the user is not bucketed into any of the experiments on the feature + decide_reasons = [] feature_flag_key = feature_flag['key'] if feature_flag['experimentIds'].empty? message = "The feature flag '#{feature_flag_key}' is not used in any experiments." @logger.log(Logger::DEBUG, message) - decide_reasons&.push(message) - return nil + decide_reasons.push(message) + return nil, decide_reasons end # Evaluate each experiment and return the first bucketed experiment variation @@ -176,26 +178,26 @@ def get_variation_for_feature_experiment(project_config, feature_flag, user_id, unless experiment message = "Feature flag experiment with ID '#{experiment_id}' is not in the datafile." @logger.log(Logger::DEBUG, message) - decide_reasons&.push(message) - return nil + decide_reasons.push(message) + return nil, decide_reasons end experiment_key = experiment['key'] variation_id, reasons_received = get_variation(project_config, experiment_key, user_id, attributes, decide_options) - decide_reasons&.push(*reasons_received) + decide_reasons.push(*reasons_received) next unless variation_id variation = project_config.variation_id_map[experiment_key][variation_id] - return Decision.new(experiment, variation, DECISION_SOURCES['FEATURE_TEST']) + return Decision.new(experiment, variation, DECISION_SOURCES['FEATURE_TEST']), decide_reasons end message = "The user '#{user_id}' is not bucketed into any of the experiments on the feature '#{feature_flag_key}'." @logger.log(Logger::INFO, message) - decide_reasons&.push(message) + decide_reasons.push(message) - nil + [nil, decide_reasons] end def get_variation_for_feature_rollout(project_config, feature_flag, user_id, attributes = nil) diff --git a/spec/decision_service_spec.rb b/spec/decision_service_spec.rb index f01a4ce9..879944a0 100644 --- a/spec/decision_service_spec.rb +++ b/spec/decision_service_spec.rb @@ -397,7 +397,8 @@ describe 'when the feature flag\'s experiment ids array is empty' do it 'should return nil and log a message' do feature_flag = config.feature_flag_key_map['empty_feature'] - expect(decision_service.get_variation_for_feature_experiment(config, feature_flag, 'user_1', user_attributes)).to eq(nil) + variation_received, = decision_service.get_variation_for_feature_experiment(config, feature_flag, 'user_1', user_attributes) + expect(variation_received).to eq(nil) expect(spy_logger).to have_received(:log).once .with(Logger::DEBUG, "The feature flag 'empty_feature' is not used in any experiments.") @@ -409,7 +410,8 @@ feature_flag = config.feature_flag_key_map['boolean_feature'].dup # any string that is not an experiment id in the data file feature_flag['experimentIds'] = ['1333333337'] - expect(decision_service.get_variation_for_feature_experiment(config, feature_flag, user_id, user_attributes)).to eq(nil) + variation_received, = decision_service.get_variation_for_feature_experiment(config, feature_flag, user_id, user_attributes) + expect(variation_received).to eq(nil) expect(spy_logger).to have_received(:log).once .with(Logger::DEBUG, "Feature flag experiment with ID '1333333337' is not in the datafile.") end @@ -428,7 +430,8 @@ it 'should return nil and log a message' do feature_flag = config.feature_flag_key_map['multi_variate_feature'] - expect(decision_service.get_variation_for_feature_experiment(config, feature_flag, 'user_1', user_attributes, [], nil)).to eq(nil) + variation_received, = decision_service.get_variation_for_feature_experiment(config, feature_flag, 'user_1', user_attributes, []) + expect(variation_received).to eq(nil) expect(spy_logger).to have_received(:log).once .with(Logger::INFO, "The user 'user_1' is not bucketed into any of the experiments on the feature 'multi_variate_feature'.") @@ -449,7 +452,8 @@ config.variation_id_map['test_experiment_multivariate']['122231'], Optimizely::DecisionService::DECISION_SOURCES['FEATURE_TEST'] ) - expect(decision_service.get_variation_for_feature_experiment(config, feature_flag, 'user_1', user_attributes)).to eq(expected_decision) + variation_received, = decision_service.get_variation_for_feature_experiment(config, feature_flag, 'user_1', user_attributes) + expect(variation_received).to eq(expected_decision) end end end @@ -472,7 +476,8 @@ it 'should return the variation the user is bucketed into' do feature_flag = config.feature_flag_key_map['mutex_group_feature'] - expect(decision_service.get_variation_for_feature_experiment(config, feature_flag, user_id, user_attributes)).to eq(expected_decision) + variation_received, = decision_service.get_variation_for_feature_experiment(config, feature_flag, user_id, user_attributes) + expect(variation_received).to eq(expected_decision) end end @@ -490,7 +495,8 @@ it 'should return nil and log a message' do feature_flag = config.feature_flag_key_map['mutex_group_feature'] - expect(decision_service.get_variation_for_feature_experiment(config, feature_flag, user_id, user_attributes)).to eq(nil) + variation_received, = decision_service.get_variation_for_feature_experiment(config, feature_flag, user_id, user_attributes) + expect(variation_received).to eq(nil) expect(spy_logger).to have_received(:log).once .with(Logger::INFO, "The user 'user_1' is not bucketed into any of the experiments on the feature 'mutex_group_feature'.") From 63525e6a4ce83e14d5fdb195b4d4cc9af9883403 Mon Sep 17 00:00:00 2001 From: Zeeshan Ashraf Date: Wed, 16 Dec 2020 11:28:49 -0800 Subject: [PATCH 08/17] fixed get_variation_for_feature --- lib/optimizely.rb | 9 +++++---- lib/optimizely/decision_service.rb | 12 +++++++----- spec/decision_service_spec.rb | 19 +++++++++++-------- spec/project_spec.rb | 6 +++--- 4 files changed, 26 insertions(+), 20 deletions(-) diff --git a/lib/optimizely.rb b/lib/optimizely.rb index 5ca3be09..8e4ec873 100644 --- a/lib/optimizely.rb +++ b/lib/optimizely.rb @@ -199,7 +199,8 @@ def decide(user_context, key, decide_options = []) experiment = nil decision_source = Optimizely::DecisionService::DECISION_SOURCES['ROLLOUT'] - decision = @decision_service.get_variation_for_feature(config, feature_flag, user_id, attributes, decide_options, reasons) + decision, reasons_received = @decision_service.get_variation_for_feature(config, feature_flag, user_id, attributes, decide_options) + reasons.push(*reasons_received) # Send impression event if Decision came from a feature test and decide options doesn't include disableDecisionEvent if decision.is_a?(Optimizely::DecisionService::Decision) @@ -489,7 +490,7 @@ def is_feature_enabled(feature_flag_key, user_id, attributes = nil) return false end - decision = @decision_service.get_variation_for_feature(config, feature_flag, user_id, attributes) + decision, = @decision_service.get_variation_for_feature(config, feature_flag, user_id, attributes) feature_enabled = false source_string = Optimizely::DecisionService::DECISION_SOURCES['ROLLOUT'] @@ -738,7 +739,7 @@ def get_all_feature_variables(feature_flag_key, user_id, attributes = nil) return nil end - decision = @decision_service.get_variation_for_feature(config, feature_flag, user_id, attributes) + decision, = @decision_service.get_variation_for_feature(config, feature_flag, user_id, attributes) variation = decision ? decision['variation'] : nil feature_enabled = variation ? variation['featureEnabled'] : false all_variables = {} @@ -944,7 +945,7 @@ def get_feature_variable_for_type(feature_flag_key, variable_key, variable_type, return nil end - decision = @decision_service.get_variation_for_feature(config, feature_flag, user_id, attributes) + decision, = @decision_service.get_variation_for_feature(config, feature_flag, user_id, attributes) variation = decision ? decision['variation'] : nil feature_enabled = variation ? variation['featureEnabled'] : false diff --git a/lib/optimizely/decision_service.rb b/lib/optimizely/decision_service.rb index 97d50c8d..cc34bc65 100644 --- a/lib/optimizely/decision_service.rb +++ b/lib/optimizely/decision_service.rb @@ -132,7 +132,7 @@ def get_variation(project_config, experiment_key, user_id, attributes = nil, dec [variation_id, decide_reasons] end - def get_variation_for_feature(project_config, feature_flag, user_id, attributes = nil, decide_options = [], decide_reasons = nil) + def get_variation_for_feature(project_config, feature_flag, user_id, attributes = nil, decide_options = []) # Get the variation the user is bucketed into for the given FeatureFlag. # # project_config - project_config - Instance of ProjectConfig @@ -142,15 +142,17 @@ def get_variation_for_feature(project_config, feature_flag, user_id, attributes # # Returns Decision struct (nil if the user is not bucketed into any of the experiments on the feature) + decide_reasons = [] + # check if the feature is being experiment on and whether the user is bucketed into the experiment decision, reasons_received = get_variation_for_feature_experiment(project_config, feature_flag, user_id, attributes, decide_options) - decide_reasons&.push(*reasons_received) - return decision unless decision.nil? + decide_reasons.push(*reasons_received) + return decision, decide_reasons unless decision.nil? decision, reasons_received = get_variation_for_feature_rollout(project_config, feature_flag, user_id, attributes) - decide_reasons&.push(*reasons_received) + decide_reasons.push(*reasons_received) - decision + [decision, decide_reasons] end def get_variation_for_feature_experiment(project_config, feature_flag, user_id, attributes = nil, decide_options = []) diff --git a/spec/decision_service_spec.rb b/spec/decision_service_spec.rb index 879944a0..f066de9d 100644 --- a/spec/decision_service_spec.rb +++ b/spec/decision_service_spec.rb @@ -691,9 +691,10 @@ 'experiment' => expected_experiment, 'variation' => expected_variation } - allow(decision_service).to receive(:get_variation_for_feature_experiment).and_return(expected_decision) + allow(decision_service).to receive(:get_variation_for_feature_experiment).and_return([expected_decision, nil]) - expect(decision_service.get_variation_for_feature(config, feature_flag, user_id, user_attributes)).to eq(expected_decision) + decision_received, = decision_service.get_variation_for_feature(config, feature_flag, user_id, user_attributes) + expect(decision_received).to eq(expected_decision) end end @@ -708,20 +709,22 @@ variation, Optimizely::DecisionService::DECISION_SOURCES['ROLLOUT'] ) - allow(decision_service).to receive(:get_variation_for_feature_experiment).and_return(nil) - allow(decision_service).to receive(:get_variation_for_feature_rollout).and_return(expected_decision) + allow(decision_service).to receive(:get_variation_for_feature_experiment).and_return([nil, nil]) + allow(decision_service).to receive(:get_variation_for_feature_rollout).and_return([expected_decision, nil]) - expect(decision_service.get_variation_for_feature(config, feature_flag, user_id, user_attributes)).to eq(expected_decision) + decision_received, = decision_service.get_variation_for_feature(config, feature_flag, user_id, user_attributes) + expect(decision_received).to eq(expected_decision) end end describe 'and the user is not bucketed into the feature rollout' do it 'should log a message and return nil' do feature_flag = config.feature_flag_key_map['string_single_variable_feature'] - allow(decision_service).to receive(:get_variation_for_feature_experiment).and_return(nil) - allow(decision_service).to receive(:get_variation_for_feature_rollout).and_return(nil) + allow(decision_service).to receive(:get_variation_for_feature_experiment).and_return([nil, nil]) + allow(decision_service).to receive(:get_variation_for_feature_rollout).and_return([nil, nil]) - expect(decision_service.get_variation_for_feature(config, feature_flag, user_id, user_attributes)).to eq(nil) + decision_received, = decision_service.get_variation_for_feature(config, feature_flag, user_id, user_attributes) + expect(decision_received).to eq(nil) end end end diff --git a/spec/project_spec.rb b/spec/project_spec.rb index e7be42de..b907c56b 100644 --- a/spec/project_spec.rb +++ b/spec/project_spec.rb @@ -3974,11 +3974,11 @@ def callback(_args); end user_context = project_instance.create_user_context('user1') expect(project_instance.decision_service).to receive(:get_variation_for_feature) - .with(anything, anything, anything, anything, [], []).once + .with(anything, anything, anything, anything, []).once project_instance.decide(user_context, 'multi_variate_feature') expect(project_instance.decision_service).to receive(:get_variation_for_feature) - .with(anything, anything, anything, anything, [Optimizely::Decide::OptimizelyDecideOption::DISABLE_DECISION_EVENT], []).once + .with(anything, anything, anything, anything, [Optimizely::Decide::OptimizelyDecideOption::DISABLE_DECISION_EVENT]).once project_instance.decide(user_context, 'multi_variate_feature', [Optimizely::Decide::OptimizelyDecideOption::DISABLE_DECISION_EVENT]) expect(project_instance.decision_service).to receive(:get_variation_for_feature) @@ -3989,7 +3989,7 @@ def callback(_args); end Optimizely::Decide::OptimizelyDecideOption::IGNORE_USER_PROFILE_SERVICE, Optimizely::Decide::OptimizelyDecideOption::INCLUDE_REASONS, Optimizely::Decide::OptimizelyDecideOption::EXCLUDE_VARIABLES - ], []).once + ]).once project_instance .decide(user_context, 'multi_variate_feature', [ Optimizely::Decide::OptimizelyDecideOption::DISABLE_DECISION_EVENT, From 514a93d25b3b89223327d2d11007941c27afa16c Mon Sep 17 00:00:00 2001 From: Zeeshan Ashraf Date: Thu, 17 Dec 2020 12:22:01 -0800 Subject: [PATCH 09/17] added assertions for reasons in bucketer --- lib/optimizely/bucketer.rb | 4 ++-- spec/bucketing_spec.rb | 40 +++++++++++++++++++++++++++----------- 2 files changed, 31 insertions(+), 13 deletions(-) diff --git a/lib/optimizely/bucketer.rb b/lib/optimizely/bucketer.rb index 1a1e6837..afa8f89b 100644 --- a/lib/optimizely/bucketer.rb +++ b/lib/optimizely/bucketer.rb @@ -58,7 +58,7 @@ def bucket(project_config, experiment, bucketing_id, user_id) if Helpers::Group.random_policy?(group) traffic_allocations = group.fetch('trafficAllocation') bucketed_experiment_id, find_bucket_reasons = find_bucket(bucketing_id, user_id, group_id, traffic_allocations) - decide_reasons.push(find_bucket_reasons) + decide_reasons.push(*find_bucket_reasons) # return if the user is not bucketed into any experiment unless bucketed_experiment_id @@ -85,7 +85,7 @@ def bucket(project_config, experiment, bucketing_id, user_id) traffic_allocations = experiment['trafficAllocation'] variation_id, find_bucket_reasons = find_bucket(bucketing_id, user_id, experiment_id, traffic_allocations) - decide_reasons.push(find_bucket_reasons) + decide_reasons.push(*find_bucket_reasons) if variation_id && variation_id != '' variation = project_config.get_variation_from_id(experiment_key, variation_id) diff --git a/spec/bucketing_spec.rb b/spec/bucketing_spec.rb index 98fd65d9..8e68cd95 100644 --- a/spec/bucketing_spec.rb +++ b/spec/bucketing_spec.rb @@ -82,8 +82,12 @@ 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') - variation_received, = bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user') + variation_received, reasons = bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user') expect(variation_received).to be_nil + expect(reasons).to eq([ + "Assigned bucket 3000 to user 'test_user' with bucketing ID: 'bucket_id_ignored'.", + "User 'test_user' is not in experiment 'group1_exp2' of group 101." + ]) 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) @@ -91,11 +95,12 @@ def get_bucketing_key(bucketing_id, entity_id = nil) end it 'should return nil when user is not bucketed into any bucket' do - expect(bucketer).to receive(:find_bucket).once.and_return(nil) + expect(bucketer).to receive(:find_bucket).once.and_return([nil, nil]) experiment = config.get_experiment_from_key('group1_exp2') - variation_received, = bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user') + variation_received, reasons = bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user') expect(variation_received).to be_nil + expect(reasons).to eq(["User 'test_user' is in no experiment."]) expect(spy_logger).to have_received(:log) .with(Logger::INFO, "User 'test_user' is in no experiment.") end @@ -105,8 +110,9 @@ 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') - variation_received, = bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user') + variation_received, reasons = bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user') expect(variation_received).to eq(expected_variation) + expect(reasons).to eq(["Assigned bucket 3000 to user 'test_user' with bucketing ID: 'bucket_id_ignored'."]) 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'.") @@ -116,7 +122,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') - variation_received, = bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user') + variation_received, reasons = bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user') + expect(reasons).to eq(["Assigned bucket 50000 to user 'test_user' with bucketing ID: 'bucket_id_ignored'."]) expect(variation_received).to be_nil expect(spy_logger).to have_received(:log).once expect(spy_logger).to have_received(:log) @@ -146,8 +153,9 @@ 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') - variation_received, = bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user') + variation_received, reasons = bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user') expect(variation_received).to be_nil + expect(reasons).to eq(['Bucketed into an empty traffic range. Returning nil.']) expect(spy_logger).to have_received(:log) .with(Logger::DEBUG, 'Bucketed into an empty traffic range. Returning nil.') end @@ -160,20 +168,23 @@ 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') - variation_received, = bucketer.bucket(config, experiment, 'test_user', 'test_user') + variation_received, reasons = bucketer.bucket(config, experiment, 'test_user', 'test_user') expect(variation_received).to be(expected_variation) + expect(reasons).to eq(["Assigned bucket 4577 to user 'test_user' with bucketing ID: 'test_user'."]) # 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') - variation_received, = bucketer.bucket(config, experiment, 'any_string789', 'test_user') + variation_received, reasons = bucketer.bucket(config, experiment, 'any_string789', 'test_user') expect(variation_received).to be(expected_variation) + expect(reasons).to eq(["Assigned bucket 9941 to user 'test_user' with bucketing ID: 'any_string789'."]) end # Bucketing with invalid experiment key and bucketing ID it 'should return nil with invalid experiment and bucketing ID' do - variation_received, = bucketer.bucket(config, config.get_experiment_from_key('invalid_experiment'), 'some_id', 'test_user') + variation_received, reasons = bucketer.bucket(config, config.get_experiment_from_key('invalid_experiment'), 'some_id', 'test_user') expect(variation_received).to be(nil) + expect(reasons).to eq([]) end # Bucketing with grouped experiments and bucketing ID @@ -182,12 +193,19 @@ def get_bucketing_key(bucketing_id, entity_id = nil) experiment = config.get_experiment_from_key('group1_exp1') expected_variation = nil - variation_received, = bucketer.bucket(config, experiment, 'test_user', 'test_user') + variation_received, reasons = bucketer.bucket(config, experiment, 'test_user', 'test_user') expect(variation_received).to be(expected_variation) - + expect(reasons).to eq([ + "Assigned bucket 7543 to user 'test_user' with bucketing ID: 'test_user'.", + "User 'test_user' is not in experiment 'group1_exp1' of group 101." + ]) expected_variation = config.get_variation_from_id('group1_exp1', '130002') variation_received, = bucketer.bucket(config, experiment, '123456789', 'test_user') expect(variation_received).to be(expected_variation) + expect(reasons).to eq([ + "Assigned bucket 7543 to user 'test_user' with bucketing ID: 'test_user'.", + "User 'test_user' is not in experiment 'group1_exp1' of group 101." + ]) end end From f13d88399503fe8d97694abccaf2d97a54d61033 Mon Sep 17 00:00:00 2001 From: Zeeshan Ashraf Date: Thu, 17 Dec 2020 12:47:43 -0800 Subject: [PATCH 10/17] added assertions for reasons for get_variation_for_feature_rollout --- spec/decision_service_spec.rb | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/spec/decision_service_spec.rb b/spec/decision_service_spec.rb index f066de9d..c294b270 100644 --- a/spec/decision_service_spec.rb +++ b/spec/decision_service_spec.rb @@ -512,8 +512,9 @@ describe 'when the feature flag is not associated with a rollout' do it 'should log a message and return nil' do feature_flag = config.feature_flag_key_map['boolean_feature'] - variation_received, = decision_service.get_variation_for_feature_rollout(config, feature_flag, user_id, user_attributes) + variation_received, reasons = decision_service.get_variation_for_feature_rollout(config, feature_flag, user_id, user_attributes) expect(variation_received).to eq(nil) + expect(reasons).to eq(["Feature flag '#{feature_flag['key']}' is not used in a rollout."]) expect(spy_logger).to have_received(:log).once .with(Logger::DEBUG, "Feature flag '#{feature_flag['key']}' is not used in a rollout.") end @@ -523,8 +524,9 @@ it 'should log a message and return nil' do feature_flag = config.feature_flag_key_map['boolean_feature'].dup feature_flag['rolloutId'] = 'invalid_rollout_id' - variation_received, = decision_service.get_variation_for_feature_rollout(config, feature_flag, user_id, user_attributes) + variation_received, reasons = decision_service.get_variation_for_feature_rollout(config, feature_flag, user_id, user_attributes) expect(variation_received).to eq(nil) + expect(reasons).to eq(["Rollout with ID 'invalid_rollout_id' is not in the datafile 'boolean_feature'"]) expect(spy_logger).to have_received(:log).once .with(Logger::ERROR, "Rollout with ID 'invalid_rollout_id' is not in the datafile.") @@ -537,8 +539,9 @@ experimentless_rollout['experiments'] = [] allow(config).to receive(:get_rollout_from_id).and_return(experimentless_rollout) feature_flag = config.feature_flag_key_map['boolean_single_variable_feature'] - variation_received, = decision_service.get_variation_for_feature_rollout(config, feature_flag, user_id, user_attributes) + variation_received, reasons = decision_service.get_variation_for_feature_rollout(config, feature_flag, user_id, user_attributes) expect(variation_received).to eq(nil) + expect(reasons).to eq([]) end end @@ -553,8 +556,9 @@ allow(decision_service.bucketer).to receive(:bucket) .with(config, rollout_experiment, user_id, user_id) .and_return(variation) - variation_received, = decision_service.get_variation_for_feature_rollout(config, feature_flag, user_id, user_attributes) + variation_received, reasons = decision_service.get_variation_for_feature_rollout(config, feature_flag, user_id, user_attributes) expect(variation_received).to eq(expected_decision) + expect(reasons).to eq(["User 'user_1' meets the audience conditions for targeting rule '1'."]) end end @@ -573,8 +577,12 @@ .with(config, everyone_else_experiment, user_id, user_id) .and_return(nil) - variation_received, = decision_service.get_variation_for_feature_rollout(config, feature_flag, user_id, user_attributes) + variation_received, reasons = decision_service.get_variation_for_feature_rollout(config, feature_flag, user_id, user_attributes) expect(variation_received).to eq(nil) + expect(reasons).to eq([ + "User 'user_1' meets the audience conditions for targeting rule '1'.", + "User 'user_1' meets the audience conditions for targeting rule 'Everyone Else'." + ]) # make sure we only checked the audience for the first rule expect(Optimizely::Audience).to have_received(:user_meets_audience_conditions?).once @@ -599,8 +607,9 @@ .with(config, everyone_else_experiment, user_id, user_id) .and_return(variation) - variation_received, = decision_service.get_variation_for_feature_rollout(config, feature_flag, user_id, user_attributes) + variation_received, reasons = decision_service.get_variation_for_feature_rollout(config, feature_flag, user_id, user_attributes) expect(variation_received).to eq(expected_decision) + expect(reasons).to eq(["User 'user_1' meets the audience conditions for targeting rule '1'.", "User 'user_1' meets the audience conditions for targeting rule 'Everyone Else'."]) # make sure we only checked the audience for the first rule expect(Optimizely::Audience).to have_received(:user_meets_audience_conditions?).once @@ -628,8 +637,9 @@ .with(config, everyone_else_experiment, user_id, user_id) .and_return(variation) - variation_received, = decision_service.get_variation_for_feature_rollout(config, feature_flag, user_id, user_attributes) + variation_received, reasons = decision_service.get_variation_for_feature_rollout(config, feature_flag, user_id, user_attributes) expect(variation_received).to eq(expected_decision) + expect(reasons).to eq(["User 'user_1' does not meet the audience conditions for targeting rule '1'.", "User 'user_1' does not meet the audience conditions for targeting rule '2'.", "User 'user_1' meets the audience conditions for targeting rule 'Everyone Else'."]) # verify we tried to bucket in all targeting rules and the everyone else rule expect(Optimizely::Audience).to have_received(:user_meets_audience_conditions?) @@ -657,8 +667,9 @@ expect(decision_service.bucketer).not_to receive(:bucket) .with(config, everyone_else_experiment, user_id, user_id) - variation_received, = decision_service.get_variation_for_feature_rollout(config, feature_flag, user_id, user_attributes) + variation_received, reasons = decision_service.get_variation_for_feature_rollout(config, feature_flag, user_id, user_attributes) expect(variation_received).to eq(nil) + expect(reasons).to eq(["User 'user_1' does not meet the audience conditions for targeting rule '1'.", "User 'user_1' does not meet the audience conditions for targeting rule '2'.", "User 'user_1' does not meet the audience conditions for targeting rule 'Everyone Else'."]) # verify we tried to bucket in all targeting rules and the everyone else rule expect(Optimizely::Audience).to have_received(:user_meets_audience_conditions?).once From 2c6533a422c5197fe8d6a020e73056a687d6ac20 Mon Sep 17 00:00:00 2001 From: Zeeshan Ashraf Date: Thu, 17 Dec 2020 18:19:11 -0800 Subject: [PATCH 11/17] added assertions for reasons everywhere --- spec/decision_service_spec.rb | 203 ++++++++++++++++++++++++++-------- 1 file changed, 159 insertions(+), 44 deletions(-) diff --git a/spec/decision_service_spec.rb b/spec/decision_service_spec.rb index c294b270..21a4ff4b 100644 --- a/spec/decision_service_spec.rb +++ b/spec/decision_service_spec.rb @@ -42,8 +42,9 @@ it 'should return the correct variation ID for a given user for whom a variation has been forced' do decision_service.set_forced_variation(config, 'test_experiment', 'test_user', 'variation') - variation_received, = decision_service.get_variation(config, 'test_experiment', 'test_user') + variation_received, reasons = decision_service.get_variation(config, 'test_experiment', 'test_user') expect(variation_received).to eq('111129') + expect(reasons).to eq(["Variation 'variation' is mapped to experiment 'test_experiment' and user 'test_user' in the forced variation map"]) # Setting forced variation should short circuit whitelist check, bucketing and audience evaluation expect(decision_service).not_to have_received(:get_whitelisted_variation_id) expect(decision_service.bucketer).not_to have_received(:bucket) @@ -56,8 +57,9 @@ Optimizely::Helpers::Constants::CONTROL_ATTRIBUTES['BUCKETING_ID'] => 'pid' } decision_service.set_forced_variation(config, 'test_experiment_with_audience', 'test_user', 'control_with_audience') - variation_received, = decision_service.get_variation(config, 'test_experiment_with_audience', 'test_user', user_attributes) + variation_received, reasons = decision_service.get_variation(config, 'test_experiment_with_audience', 'test_user', user_attributes) expect(variation_received).to eq('122228') + expect(reasons).to eq(["Variation 'control_with_audience' is mapped to experiment 'test_experiment_with_audience' and user 'test_user' in the forced variation map"]) # Setting forced variation should short circuit whitelist check, bucketing and audience evaluation expect(decision_service).not_to have_received(:get_whitelisted_variation_id) expect(decision_service.bucketer).not_to have_received(:bucket) @@ -65,8 +67,13 @@ end it 'should return the correct variation ID for a given user ID and key of a running experiment' do - variation_received, = decision_service.get_variation(config, 'test_experiment', 'test_user') + variation_received, reasons = decision_service.get_variation(config, 'test_experiment', 'test_user') expect(variation_received).to eq('111128') + expect(reasons).to eq([ + "User 'test_user' is not in the forced variation map.", + "Assigned bucket 4577 to user 'test_user' with bucketing ID: 'test_user'.", + "User 'test_user' is in variation 'control' of experiment 'test_experiment'." + ]) expect(spy_logger).to have_received(:log) .once.with(Logger::INFO, "User 'test_user' is in variation 'control' of experiment 'test_experiment'.") @@ -76,21 +83,33 @@ it 'should return nil when user ID is not bucketed' do allow(decision_service.bucketer).to receive(:bucket).and_return(nil) - variation_received, = decision_service.get_variation(config, 'test_experiment', 'test_user') + variation_received, reasons = decision_service.get_variation(config, 'test_experiment', 'test_user') expect(variation_received).to eq(nil) + expect(reasons).to eq([ + "User 'test_user' is not in the forced variation map.", + "User 'test_user' is in no variation." + ]) expect(spy_logger).to have_received(:log) .once.with(Logger::INFO, "User 'test_user' is in no variation.") end it 'should return correct variation ID if user ID is in whitelisted Variations and variation is valid' do - variation_received, = decision_service.get_variation(config, 'test_experiment', 'forced_user1') + variation_received, reasons = decision_service.get_variation(config, 'test_experiment', 'forced_user1') expect(variation_received).to eq('111128') + expect(reasons).to eq([ + "User 'forced_user1' is not in the forced variation map.", + "User 'forced_user1' is whitelisted into variation 'control' of experiment 'test_experiment'." + ]) expect(spy_logger).to have_received(:log) .once.with(Logger::INFO, "User 'forced_user1' is whitelisted into variation 'control' of experiment 'test_experiment'.") - variation_received, = decision_service.get_variation(config, 'test_experiment', 'forced_user2') + variation_received, reasons = decision_service.get_variation(config, 'test_experiment', 'forced_user2') expect(variation_received).to eq('111129') + expect(reasons).to eq([ + "User 'forced_user2' is not in the forced variation map.", + "User 'forced_user2' is whitelisted into variation 'variation' of experiment 'test_experiment'." + ]) expect(spy_logger).to have_received(:log) .once.with(Logger::INFO, "User 'forced_user2' is whitelisted into variation 'variation' of experiment 'test_experiment'.") @@ -106,13 +125,21 @@ Optimizely::Helpers::Constants::CONTROL_ATTRIBUTES['BUCKETING_ID'] => 'pid' } - variation_received, = decision_service.get_variation(config, 'test_experiment', 'forced_user1', user_attributes) + variation_received, reasons = decision_service.get_variation(config, 'test_experiment', 'forced_user1', user_attributes) expect(variation_received).to eq('111128') + expect(reasons).to eq([ + "User 'forced_user1' is not in the forced variation map.", + "User 'forced_user1' is whitelisted into variation 'control' of experiment 'test_experiment'." + ]) expect(spy_logger).to have_received(:log) .once.with(Logger::INFO, "User 'forced_user1' is whitelisted into variation 'control' of experiment 'test_experiment'.") - variation_received, = decision_service.get_variation(config, 'test_experiment', 'forced_user2', user_attributes) + variation_received, reasons = decision_service.get_variation(config, 'test_experiment', 'forced_user2', user_attributes) expect(variation_received).to eq('111129') + expect(reasons).to eq([ + "User 'forced_user2' is not in the forced variation map.", + "User 'forced_user2' is whitelisted into variation 'variation' of experiment 'test_experiment'." + ]) expect(spy_logger).to have_received(:log) .once.with(Logger::INFO, "User 'forced_user2' is whitelisted into variation 'variation' of experiment 'test_experiment'.") @@ -124,8 +151,12 @@ it 'should return the correct variation ID for a user in a whitelisted variation (even when audience conditions do not match)' do user_attributes = {'browser_type' => 'wrong_browser'} - variation_received, = decision_service.get_variation(config, 'test_experiment_with_audience', 'forced_audience_user', user_attributes) + variation_received, reasons = decision_service.get_variation(config, 'test_experiment_with_audience', 'forced_audience_user', user_attributes) expect(variation_received).to eq('122229') + expect(reasons).to eq([ + "User 'forced_audience_user' is not in the forced variation map.", + "User 'forced_audience_user' is whitelisted into variation 'variation_with_audience' of experiment 'test_experiment_with_audience'." + ]) expect(spy_logger).to have_received(:log) .once.with( Logger::INFO, @@ -139,8 +170,9 @@ end it 'should return nil if the experiment key is invalid' do - variation_received, = decision_service.get_variation(config, 'totally_invalid_experiment', 'test_user', {}) + variation_received, reasons = decision_service.get_variation(config, 'totally_invalid_experiment', 'test_user', {}) expect(variation_received).to eq(nil) + expect(reasons).to eq([]) expect(spy_logger).to have_received(:log) .once.with(Logger::ERROR, "Experiment key 'totally_invalid_experiment' is not in datafile.") @@ -148,8 +180,12 @@ it 'should return nil if the user does not meet the audience conditions for a given experiment' do user_attributes = {'browser_type' => 'chrome'} - variation_received, = decision_service.get_variation(config, 'test_experiment_with_audience', 'test_user', user_attributes) + variation_received, reasons = decision_service.get_variation(config, 'test_experiment_with_audience', 'test_user', user_attributes) expect(variation_received).to eq(nil) + expect(reasons).to eq([ + "User 'test_user' is not in the forced variation map.", + "User 'test_user' does not meet the conditions to be in experiment 'test_experiment_with_audience'." + ]) expect(spy_logger).to have_received(:log) .once.with(Logger::INFO, "User 'test_user' does not meet the conditions to be in experiment 'test_experiment_with_audience'.") @@ -160,8 +196,9 @@ end it 'should return nil if the given experiment is not running' do - variation_received, = decision_service.get_variation(config, 'test_experiment_not_started', 'test_user') + variation_received, reasons = decision_service.get_variation(config, 'test_experiment_not_started', 'test_user') expect(variation_received).to eq(nil) + expect(reasons).to eq(["Experiment 'test_experiment_not_started' is not running."]) expect(spy_logger).to have_received(:log) .once.with(Logger::INFO, "Experiment 'test_experiment_not_started' is not running.") @@ -174,8 +211,12 @@ end it 'should respect forced variations within mutually exclusive grouped experiments' do - variation_received, = decision_service.get_variation(config, 'group1_exp2', 'forced_group_user1') + variation_received, reasons = decision_service.get_variation(config, 'group1_exp2', 'forced_group_user1') expect(variation_received).to eq('130004') + expect(reasons).to eq([ + "User 'forced_group_user1' is not in the forced variation map.", + "User 'forced_group_user1' is whitelisted into variation 'g1_e2_v2' of experiment 'group1_exp2'." + ]) expect(spy_logger).to have_received(:log) .once.with(Logger::INFO, "User 'forced_group_user1' is whitelisted into variation 'g1_e2_v2' of experiment 'group1_exp2'.") @@ -186,8 +227,14 @@ end it 'should bucket normally if user is whitelisted into a forced variation that is not in the datafile' do - variation_received, = decision_service.get_variation(config, 'test_experiment', 'forced_user_with_invalid_variation') + variation_received, reasons = decision_service.get_variation(config, 'test_experiment', 'forced_user_with_invalid_variation') expect(variation_received).to eq('111128') + expect(reasons).to eq([ + "User 'forced_user_with_invalid_variation' is not in the forced variation map.", + "User 'forced_user_with_invalid_variation' is whitelisted into variation 'invalid_variation', which is not in the datafile.", + "Assigned bucket 877 to user 'forced_user_with_invalid_variation' with bucketing ID: 'forced_user_with_invalid_variation'.", + "User 'forced_user_with_invalid_variation' is in variation 'control' of experiment 'test_experiment'." + ]) expect(spy_logger).to have_received(:log) .once.with( Logger::INFO, @@ -211,8 +258,13 @@ } expect(spy_user_profile_service).to receive(:lookup).once.and_return(nil) - variation_received, = decision_service.get_variation(config, 'test_experiment', 'test_user') + variation_received, reasons = decision_service.get_variation(config, 'test_experiment', 'test_user') expect(variation_received).to eq('111128') + expect(reasons).to eq([ + "User 'test_user' is not in the forced variation map.", + "Assigned bucket 4577 to user 'test_user' with bucketing ID: 'test_user'.", + "User 'test_user' is in variation 'control' of experiment 'test_experiment'." + ]) # bucketing should have occurred expect(decision_service.bucketer).to have_received(:bucket).once @@ -237,8 +289,13 @@ } expect(spy_user_profile_service).to receive(:lookup).once.and_return(nil) - variation_received, = decision_service.get_variation(config, 'test_experiment', 'test_user', user_attributes) + variation_received, reasons = decision_service.get_variation(config, 'test_experiment', 'test_user', user_attributes) expect(variation_received).to eq('111129') + expect(reasons).to eq([ + "User 'test_user' is not in the forced variation map.", + "Assigned bucket 8933 to user 'test_user' with bucketing ID: 'pid'.", + "User 'test_user' is in variation 'variation' of experiment 'test_experiment'." + ]) # bucketing should have occurred expect(decision_service.bucketer).to have_received(:bucket).once @@ -260,8 +317,12 @@ expect(spy_user_profile_service).to receive(:lookup) .with('test_user').once.and_return(saved_user_profile) - variation_received, = decision_service.get_variation(config, 'test_experiment', 'test_user') + variation_received, reasons = decision_service.get_variation(config, 'test_experiment', 'test_user') expect(variation_received).to eq('111129') + expect(reasons).to eq([ + "User 'test_user' is not in the forced variation map.", + "Returning previously activated variation ID 111129 of experiment 'test_experiment' for user 'test_user' from user profile." + ]) expect(spy_logger).to have_received(:log).once .with(Logger::INFO, "Returning previously activated variation ID 111129 of experiment 'test_experiment' for user 'test_user' from user profile.") @@ -286,8 +347,13 @@ expect(spy_user_profile_service).to receive(:lookup) .once.with('test_user').and_return(saved_user_profile) - variation_received, = decision_service.get_variation(config, 'test_experiment', 'test_user') + variation_received, reasons = decision_service.get_variation(config, 'test_experiment', 'test_user') expect(variation_received).to eq('111128') + expect(reasons).to eq([ + "User 'test_user' is not in the forced variation map.", + "Assigned bucket 4577 to user 'test_user' with bucketing ID: 'test_user'.", + "User 'test_user' is in variation 'control' of experiment 'test_experiment'." + ]) # bucketing should have occurred expect(decision_service.bucketer).to have_received(:bucket).once @@ -320,8 +386,14 @@ expect(spy_user_profile_service).to receive(:lookup) .once.with('test_user').and_return(saved_user_profile) - variation_received, = decision_service.get_variation(config, 'test_experiment', 'test_user') + variation_received, reasons = decision_service.get_variation(config, 'test_experiment', 'test_user') expect(variation_received).to eq('111128') + expect(reasons).to eq([ + "User 'test_user' is not in the forced variation map.", + "User '' was previously bucketed into variation ID '111111' for experiment '111127', but no matching variation was found. Re-bucketing user.", + "Assigned bucket 4577 to user 'test_user' with bucketing ID: 'test_user'.", + "User 'test_user' is in variation 'control' of experiment 'test_experiment'." + ]) # bucketing should have occurred expect(decision_service.bucketer).to have_received(:bucket).once @@ -341,8 +413,14 @@ it 'should bucket normally if the user profile service throws an error during lookup' do expect(spy_user_profile_service).to receive(:lookup).once.with('test_user').and_throw(:LookupError) - variation_received, = decision_service.get_variation(config, 'test_experiment', 'test_user') + variation_received, reasons = decision_service.get_variation(config, 'test_experiment', 'test_user') expect(variation_received).to eq('111128') + expect(reasons).to eq([ + "User 'test_user' is not in the forced variation map.", + "Error while looking up user profile for user ID 'test_user': uncaught throw :LookupError.", + "Assigned bucket 4577 to user 'test_user' with bucketing ID: 'test_user'.", + "User 'test_user' is in variation 'control' of experiment 'test_experiment'." + ]) expect(spy_logger).to have_received(:log).once .with(Logger::ERROR, "Error while looking up user profile for user ID 'test_user': uncaught throw :LookupError.") @@ -353,8 +431,13 @@ it 'should log an error if the user profile service throws an error during save' do expect(spy_user_profile_service).to receive(:save).once.and_throw(:SaveError) - variation_received, = decision_service.get_variation(config, 'test_experiment', 'test_user') + variation_received, reasons = decision_service.get_variation(config, 'test_experiment', 'test_user') expect(variation_received).to eq('111128') + expect(reasons).to eq([ + "User 'test_user' is not in the forced variation map.", + "Assigned bucket 4577 to user 'test_user' with bucketing ID: 'test_user'.", + "User 'test_user' is in variation 'control' of experiment 'test_experiment'." + ]) expect(spy_logger).to have_received(:log).once .with(Logger::ERROR, "Error while saving user profile for user ID 'test_user': uncaught throw :SaveError.") @@ -365,8 +448,13 @@ allow(spy_user_profile_service).to receive(:lookup) .with('test_user').once.and_return(nil) - variation_received, = decision_service.get_variation(config, 'test_experiment', 'test_user', nil, [Optimizely::Decide::OptimizelyDecideOption::IGNORE_USER_PROFILE_SERVICE]) + variation_received, reasons = decision_service.get_variation(config, 'test_experiment', 'test_user', nil, [Optimizely::Decide::OptimizelyDecideOption::IGNORE_USER_PROFILE_SERVICE]) expect(variation_received).to eq('111128') + expect(reasons).to eq([ + "User 'test_user' is not in the forced variation map.", + "Assigned bucket 4577 to user 'test_user' with bucketing ID: 'test_user'.", + "User 'test_user' is in variation 'control' of experiment 'test_experiment'." + ]) expect(decision_service.bucketer).to have_received(:bucket) expect(Optimizely::Audience).to have_received(:user_meets_audience_conditions?) @@ -378,8 +466,13 @@ allow(spy_user_profile_service).to receive(:lookup) .with('test_user').once.and_return(nil) - variation_received, = decision_service.get_variation(config, 'test_experiment', 'test_user') + variation_received, reasons = decision_service.get_variation(config, 'test_experiment', 'test_user') expect(variation_received).to eq('111128') + expect(reasons).to eq([ + "User 'test_user' is not in the forced variation map.", + "Assigned bucket 4577 to user 'test_user' with bucketing ID: 'test_user'.", + "User 'test_user' is in variation 'control' of experiment 'test_experiment'." + ]) expect(decision_service.bucketer).to have_received(:bucket) expect(Optimizely::Audience).to have_received(:user_meets_audience_conditions?) @@ -397,8 +490,9 @@ describe 'when the feature flag\'s experiment ids array is empty' do it 'should return nil and log a message' do feature_flag = config.feature_flag_key_map['empty_feature'] - variation_received, = decision_service.get_variation_for_feature_experiment(config, feature_flag, 'user_1', user_attributes) + variation_received, reasons = decision_service.get_variation_for_feature_experiment(config, feature_flag, 'user_1', user_attributes) expect(variation_received).to eq(nil) + expect(reasons).to eq(["The feature flag 'empty_feature' is not used in any experiments."]) expect(spy_logger).to have_received(:log).once .with(Logger::DEBUG, "The feature flag 'empty_feature' is not used in any experiments.") @@ -410,8 +504,9 @@ feature_flag = config.feature_flag_key_map['boolean_feature'].dup # any string that is not an experiment id in the data file feature_flag['experimentIds'] = ['1333333337'] - variation_received, = decision_service.get_variation_for_feature_experiment(config, feature_flag, user_id, user_attributes) + variation_received, reasons = decision_service.get_variation_for_feature_experiment(config, feature_flag, user_id, user_attributes) expect(variation_received).to eq(nil) + expect(reasons).to eq(["Feature flag experiment with ID '1333333337' is not in the datafile."]) expect(spy_logger).to have_received(:log).once .with(Logger::DEBUG, "Feature flag experiment with ID '1333333337' is not in the datafile.") end @@ -430,8 +525,9 @@ it 'should return nil and log a message' do feature_flag = config.feature_flag_key_map['multi_variate_feature'] - variation_received, = decision_service.get_variation_for_feature_experiment(config, feature_flag, 'user_1', user_attributes, []) + variation_received, reasons = decision_service.get_variation_for_feature_experiment(config, feature_flag, 'user_1', user_attributes, []) expect(variation_received).to eq(nil) + expect(reasons).to eq(["The user 'user_1' is not bucketed into any of the experiments on the feature 'multi_variate_feature'."]) expect(spy_logger).to have_received(:log).once .with(Logger::INFO, "The user 'user_1' is not bucketed into any of the experiments on the feature 'multi_variate_feature'.") @@ -452,8 +548,9 @@ config.variation_id_map['test_experiment_multivariate']['122231'], Optimizely::DecisionService::DECISION_SOURCES['FEATURE_TEST'] ) - variation_received, = decision_service.get_variation_for_feature_experiment(config, feature_flag, 'user_1', user_attributes) + variation_received, reasons = decision_service.get_variation_for_feature_experiment(config, feature_flag, 'user_1', user_attributes) expect(variation_received).to eq(expected_decision) + expect(reasons).to eq([]) end end end @@ -476,8 +573,9 @@ it 'should return the variation the user is bucketed into' do feature_flag = config.feature_flag_key_map['mutex_group_feature'] - variation_received, = decision_service.get_variation_for_feature_experiment(config, feature_flag, user_id, user_attributes) + variation_received, reasons = decision_service.get_variation_for_feature_experiment(config, feature_flag, user_id, user_attributes) expect(variation_received).to eq(expected_decision) + expect(reasons).to eq([]) end end @@ -495,8 +593,9 @@ it 'should return nil and log a message' do feature_flag = config.feature_flag_key_map['mutex_group_feature'] - variation_received, = decision_service.get_variation_for_feature_experiment(config, feature_flag, user_id, user_attributes) + variation_received, reasons = decision_service.get_variation_for_feature_experiment(config, feature_flag, user_id, user_attributes) expect(variation_received).to eq(nil) + expect(reasons).to eq(["The user 'user_1' is not bucketed into any of the experiments on the feature 'mutex_group_feature'."]) expect(spy_logger).to have_received(:log).once .with(Logger::INFO, "The user 'user_1' is not bucketed into any of the experiments on the feature 'mutex_group_feature'.") @@ -704,8 +803,9 @@ } allow(decision_service).to receive(:get_variation_for_feature_experiment).and_return([expected_decision, nil]) - decision_received, = decision_service.get_variation_for_feature(config, feature_flag, user_id, user_attributes) + decision_received, reasons = decision_service.get_variation_for_feature(config, feature_flag, user_id, user_attributes) expect(decision_received).to eq(expected_decision) + expect(reasons).to eq([]) end end @@ -723,8 +823,9 @@ allow(decision_service).to receive(:get_variation_for_feature_experiment).and_return([nil, nil]) allow(decision_service).to receive(:get_variation_for_feature_rollout).and_return([expected_decision, nil]) - decision_received, = decision_service.get_variation_for_feature(config, feature_flag, user_id, user_attributes) + decision_received, reasons = decision_service.get_variation_for_feature(config, feature_flag, user_id, user_attributes) expect(decision_received).to eq(expected_decision) + expect(reasons).to eq([]) end end @@ -734,8 +835,9 @@ allow(decision_service).to receive(:get_variation_for_feature_experiment).and_return([nil, nil]) allow(decision_service).to receive(:get_variation_for_feature_rollout).and_return([nil, nil]) - decision_received, = decision_service.get_variation_for_feature(config, feature_flag, user_id, user_attributes) + decision_received, reasons = decision_service.get_variation_for_feature(config, feature_flag, user_id, user_attributes) expect(decision_received).to eq(nil) + expect(reasons).to eq([]) end end end @@ -746,8 +848,10 @@ 'browser_type' => 'firefox', Optimizely::Helpers::Constants::CONTROL_ATTRIBUTES['BUCKETING_ID'] => 5 } - bucketing_id, = decision_service.send(:get_bucketing_id, 'test_user', user_attributes) + bucketing_id, reason = decision_service.send(:get_bucketing_id, 'test_user', user_attributes) expect(bucketing_id).to eq('test_user') + expect(reason).to eq('Bucketing ID attribute is not a string. Defaulted to user ID.') + expect(spy_logger).to have_received(:log).once.with(Logger::WARN, 'Bucketing ID attribute is not a string. Defaulted to user ID.') end @@ -756,8 +860,9 @@ 'browser_type' => 'firefox', Optimizely::Helpers::Constants::CONTROL_ATTRIBUTES['BUCKETING_ID'] => nil } - bucketing_id, = decision_service.send(:get_bucketing_id, 'test_user', user_attributes) + bucketing_id, reason = decision_service.send(:get_bucketing_id, 'test_user', user_attributes) expect(bucketing_id).to eq('test_user') + expect(reason).to eq(nil) expect(spy_logger).not_to have_received(:log) end @@ -766,8 +871,9 @@ 'browser_type' => 'firefox', Optimizely::Helpers::Constants::CONTROL_ATTRIBUTES['BUCKETING_ID'] => 'i_am_bucketing_id' } - bucketing_id, = decision_service.send(:get_bucketing_id, 'test_user', user_attributes) + bucketing_id, reason = decision_service.send(:get_bucketing_id, 'test_user', user_attributes) expect(bucketing_id).to eq('i_am_bucketing_id') + expect(reason).to eq(nil) expect(spy_logger).not_to have_received(:log) end @@ -776,8 +882,9 @@ 'browser_type' => 'firefox', Optimizely::Helpers::Constants::CONTROL_ATTRIBUTES['BUCKETING_ID'] => '' } - bucketing_id, = decision_service.send(:get_bucketing_id, 'test_user', user_attributes) + bucketing_id, reason = decision_service.send(:get_bucketing_id, 'test_user', user_attributes) expect(bucketing_id).to eq('') + expect(reason).to eq(nil) expect(spy_logger).not_to have_received(:log) end end @@ -791,15 +898,17 @@ # User ID is not defined in the forced variation map it 'should log a message and return nil when user is not in forced variation map' do - variation_received, = decision_service.get_forced_variation(config, valid_experiment[:key], user_id) + variation_received, reasons = decision_service.get_forced_variation(config, valid_experiment[:key], user_id) expect(variation_received).to eq(nil) + expect(reasons).to eq(["User '#{user_id}' is not in the forced variation map."]) expect(spy_logger).to have_received(:log).with(Logger::DEBUG, "User '#{user_id}' is not in the forced variation map.") end # Experiment key does not exist in the datafile it 'should return nil when experiment key is not in datafile' do - variation_received, = decision_service.get_forced_variation(config, invalid_experiment_key, user_id) + variation_received, reasons = decision_service.get_forced_variation(config, invalid_experiment_key, user_id) expect(variation_received).to eq(nil) + expect(reasons).to eq(["User 'test_user' is not in the forced variation map."]) end end @@ -833,40 +942,46 @@ # Call set variation with different variations on one user/experiment to confirm that each set is expected. it 'should set and return expected variations when different variations are set and removed for one user/experiment' do expect(decision_service.set_forced_variation(config, valid_experiment[:key], user_id, valid_variation[:key])).to eq(true) - variation, = decision_service.get_forced_variation(config, valid_experiment[:key], user_id) + variation, reasons = decision_service.get_forced_variation(config, valid_experiment[:key], user_id) expect(variation['id']).to eq(valid_variation[:id]) expect(variation['key']).to eq(valid_variation[:key]) + expect(reasons).to eq(["Variation 'control' is mapped to experiment 'test_experiment' and user 'test_user' in the forced variation map"]) expect(decision_service.set_forced_variation(config, valid_experiment[:key], user_id, valid_variation_2[:key])).to eq(true) - variation, = decision_service.get_forced_variation(config, valid_experiment[:key], user_id) + variation, reasons = decision_service.get_forced_variation(config, valid_experiment[:key], user_id) expect(variation['id']).to eq(valid_variation_2[:id]) expect(variation['key']).to eq(valid_variation_2[:key]) + expect(reasons).to eq(["Variation 'variation' is mapped to experiment 'test_experiment' and user 'test_user' in the forced variation map"]) end # Set variation on multiple experiments for one user. it 'should set and return expected variations when variation is set for multiple experiments for one user' do expect(decision_service.set_forced_variation(config, valid_experiment[:key], user_id, valid_variation[:key])).to eq(true) - variation, = decision_service.get_forced_variation(config, valid_experiment[:key], user_id) + variation, reasons = decision_service.get_forced_variation(config, valid_experiment[:key], user_id) expect(variation['id']).to eq(valid_variation[:id]) expect(variation['key']).to eq(valid_variation[:key]) + expect(reasons).to eq(["Variation 'control' is mapped to experiment 'test_experiment' and user 'test_user' in the forced variation map"]) expect(decision_service.set_forced_variation(config, valid_experiment_2[:key], user_id, valid_variation_for_exp_2[:key])).to eq(true) - variation, = decision_service.get_forced_variation(config, valid_experiment_2[:key], user_id) + variation, reasons = decision_service.get_forced_variation(config, valid_experiment_2[:key], user_id) expect(variation['id']).to eq(valid_variation_for_exp_2[:id]) expect(variation['key']).to eq(valid_variation_for_exp_2[:key]) + expect(reasons).to eq(["Variation 'control_with_audience' is mapped to experiment 'test_experiment_with_audience' and user 'test_user' in the forced variation map"]) end # Set variations for multiple users. it 'should set and return expected variations when variations are set for multiple users' do expect(decision_service.set_forced_variation(config, valid_experiment[:key], user_id, valid_variation[:key])).to eq(true) - variation, = decision_service.get_forced_variation(config, valid_experiment[:key], user_id) + variation, reasons = decision_service.get_forced_variation(config, valid_experiment[:key], user_id) expect(variation['id']).to eq(valid_variation[:id]) expect(variation['key']).to eq(valid_variation[:key]) + expect(reasons).to eq(["Variation 'control' is mapped to experiment 'test_experiment' and user 'test_user' in the forced variation map"]) expect(decision_service.set_forced_variation(config, valid_experiment[:key], user_id_2, valid_variation[:key])).to eq(true) - variation, = decision_service.get_forced_variation(config, valid_experiment[:key], user_id_2) + variation, reasons = decision_service.get_forced_variation(config, valid_experiment[:key], user_id_2) expect(variation['id']).to eq(valid_variation[:id]) expect(variation['key']).to eq(valid_variation[:key]) + expect(reasons).to eq(["Variation 'control' is mapped to experiment 'test_experiment' and user 'test_user_2' in the forced variation map"]) end end end From ef8ddd52db04292ea166a0e9ebc5864ff1fda540 Mon Sep 17 00:00:00 2001 From: Zeeshan Ashraf Date: Fri, 18 Dec 2020 16:26:38 -0800 Subject: [PATCH 12/17] added reasons to another missing method and updated unit tests --- lib/optimizely/audience.rb | 58 ++++++----------- lib/optimizely/decision_service.rb | 13 +++- spec/audience_spec.rb | 99 +++++++++++++++++++----------- spec/decision_service_spec.rb | 27 ++++++++ spec/project_spec.rb | 16 +++++ 5 files changed, 134 insertions(+), 79 deletions(-) diff --git a/lib/optimizely/audience.rb b/lib/optimizely/audience.rb index d809b773..e0794da0 100644 --- a/lib/optimizely/audience.rb +++ b/lib/optimizely/audience.rb @@ -38,6 +38,7 @@ def user_meets_audience_conditions?(config, experiment, attributes, logger, logg # This defaults to experiment['key']. # # Returns boolean representing if user satisfies audience conditions for the audiences or not. + decide_reasons = [] logging_hash ||= 'EXPERIMENT_AUDIENCE_EVALUATION_LOGS' logging_key ||= experiment['key'] @@ -45,26 +46,16 @@ def user_meets_audience_conditions?(config, experiment, attributes, logger, logg audience_conditions = experiment['audienceConditions'] || experiment['audienceIds'] - logger.log( - Logger::DEBUG, - format( - logs_hash['EVALUATING_AUDIENCES_COMBINED'], - logging_key, - audience_conditions - ) - ) + message = format(logs_hash['EVALUATING_AUDIENCES_COMBINED'], logging_key, audience_conditions) + logger.log(Logger::DEBUG, message) + decide_reasons.push(message) # Return true if there are no audiences if audience_conditions.empty? - logger.log( - Logger::INFO, - format( - logs_hash['AUDIENCE_EVALUATION_RESULT_COMBINED'], - logging_key, - 'TRUE' - ) - ) - return true + message = format(logs_hash['AUDIENCE_EVALUATION_RESULT_COMBINED'], logging_key, 'TRUE') + logger.log(Logger::INFO) + decide_reasons.push(message) + return true, decide_reasons end attributes ||= {} @@ -80,39 +71,28 @@ def user_meets_audience_conditions?(config, experiment, attributes, logger, logg return nil unless audience audience_conditions = audience['conditions'] - logger.log( - Logger::DEBUG, - format( - logs_hash['EVALUATING_AUDIENCE'], - audience_id, - audience_conditions - ) - ) + message = format(logs_hash['EVALUATING_AUDIENCE'], audience_id, audience_conditions) + logger.log(Logger::DEBUG, message) + decide_reasons.push(message) audience_conditions = JSON.parse(audience_conditions) if audience_conditions.is_a?(String) result = ConditionTreeEvaluator.evaluate(audience_conditions, evaluate_custom_attr) result_str = result.nil? ? 'UNKNOWN' : result.to_s.upcase - logger.log( - Logger::DEBUG, - format(logs_hash['AUDIENCE_EVALUATION_RESULT'], audience_id, result_str) - ) + message = format(logs_hash['AUDIENCE_EVALUATION_RESULT'], audience_id, result_str) + logger.log(Logger::DEBUG, message) + decide_reasons.push(message) + result end eval_result = ConditionTreeEvaluator.evaluate(audience_conditions, evaluate_audience) - eval_result ||= false - logger.log( - Logger::INFO, - format( - logs_hash['AUDIENCE_EVALUATION_RESULT_COMBINED'], - logging_key, - eval_result.to_s.upcase - ) - ) + message = format(logs_hash['AUDIENCE_EVALUATION_RESULT_COMBINED'], logging_key, eval_result.to_s.upcase) + logger.log(Logger::INFO, message) + decide_reasons.push(message) - eval_result + [eval_result, decide_reasons] end end end diff --git a/lib/optimizely/decision_service.rb b/lib/optimizely/decision_service.rb index cc34bc65..de6f6589 100644 --- a/lib/optimizely/decision_service.rb +++ b/lib/optimizely/decision_service.rb @@ -105,7 +105,9 @@ def get_variation(project_config, experiment_key, user_id, attributes = nil, dec end # Check audience conditions - unless Audience.user_meets_audience_conditions?(project_config, experiment, attributes, @logger) + user_meets_audience_conditions, reasons_received = Audience.user_meets_audience_conditions?(project_config, experiment, attributes, @logger) + decide_reasons.push(*reasons_received) + unless user_meets_audience_conditions message = "User '#{user_id}' does not meet the conditions to be in experiment '#{experiment_key}'." @logger.log(Logger::INFO, message) decide_reasons.push(message) @@ -242,8 +244,10 @@ def get_variation_for_feature_rollout(project_config, feature_flag, user_id, att rollout_rule = rollout_rules[index] logging_key = index + 1 + user_meets_audience_conditions, reasons_received = Audience.user_meets_audience_conditions?(project_config, rollout_rule, attributes, @logger, 'ROLLOUT_AUDIENCE_EVALUATION_LOGS', logging_key) + decide_reasons.push(*reasons_received) # Check that user meets audience conditions for targeting rule - unless Audience.user_meets_audience_conditions?(project_config, rollout_rule, attributes, @logger, 'ROLLOUT_AUDIENCE_EVALUATION_LOGS', logging_key) + unless user_meets_audience_conditions message = "User '#{user_id}' does not meet the audience conditions for targeting rule '#{logging_key}'." @logger.log(Logger::DEBUG, message) decide_reasons.push(message) @@ -266,8 +270,11 @@ def get_variation_for_feature_rollout(project_config, feature_flag, user_id, att # get last rule which is the everyone else rule everyone_else_experiment = rollout_rules[number_of_rules] logging_key = 'Everyone Else' + + user_meets_audience_conditions, reasons_received = Audience.user_meets_audience_conditions?(project_config, everyone_else_experiment, attributes, @logger, 'ROLLOUT_AUDIENCE_EVALUATION_LOGS', logging_key) + decide_reasons.push(*reasons_received) # Check that user meets audience conditions for last rule - unless Audience.user_meets_audience_conditions?(project_config, everyone_else_experiment, attributes, @logger, 'ROLLOUT_AUDIENCE_EVALUATION_LOGS', logging_key) + unless user_meets_audience_conditions message = "User '#{user_id}' does not meet the audience conditions for targeting rule '#{logging_key}'." @logger.log(Logger::DEBUG, message) decide_reasons.push(message) diff --git a/spec/audience_spec.rb b/spec/audience_spec.rb index dd490e35..d353d9f2 100644 --- a/spec/audience_spec.rb +++ b/spec/audience_spec.rb @@ -34,27 +34,25 @@ expect(Optimizely::Audience.user_meets_audience_conditions?(config, experiment, user_attributes, - spy_logger)).to be true + spy_logger)[0]).to be true # Audience Ids exist but Audience Conditions is Empty experiment = config.experiment_key_map['test_experiment'] experiment['audienceIds'] = ['11154'] experiment['audienceConditions'] = [] - expect(Optimizely::Audience.user_meets_audience_conditions?(config, - experiment, - user_attributes, - spy_logger)).to be true + user_meets_audience_conditions, reasons = Optimizely::Audience.user_meets_audience_conditions?(config, experiment, user_attributes, spy_logger) + expect(user_meets_audience_conditions).to be true + expect(reasons).to eq(["Evaluating audiences for experiment 'test_experiment': [].", "Audiences for experiment 'test_experiment' collectively evaluated to TRUE."]) # Audience Ids is Empty and Audience Conditions is nil experiment = config.experiment_key_map['test_experiment'] experiment['audienceIds'] = [] experiment['audienceConditions'] = nil - expect(Optimizely::Audience.user_meets_audience_conditions?(config, - experiment, - user_attributes, - spy_logger)).to be true + user_meets_audience_conditions, reasons = Optimizely::Audience.user_meets_audience_conditions?(config, experiment, user_attributes, spy_logger) + expect(user_meets_audience_conditions).to be true + expect(reasons).to eq(["Evaluating audiences for experiment 'test_experiment': [].", "Audiences for experiment 'test_experiment' collectively evaluated to TRUE."]) end it 'should pass conditions when audience conditions exist else audienceIds are passed' do @@ -85,15 +83,25 @@ allow(Optimizely::CustomAttributeConditionEvaluator).to receive(:new).and_call_original # attributes set to empty dict - expect(Optimizely::Audience.user_meets_audience_conditions?(config, - experiment, - {}, - spy_logger)).to be false + user_meets_audience_conditions, reasons = Optimizely::Audience.user_meets_audience_conditions?(config, experiment, {}, spy_logger) + expect(user_meets_audience_conditions).to be false + expect(reasons).to eq([ + "Evaluating audiences for experiment 'test_experiment_with_audience': [\"11154\"].", + "Starting to evaluate audience '11154' with conditions: [\"and\", [\"or\", [\"or\", {\"name\": \"browser_type\", \"type\": \"custom_attribute\", \"value\": \"firefox\"}]]].", + "Audience '11154' evaluated to UNKNOWN.", + "Audiences for experiment 'test_experiment_with_audience' collectively evaluated to FALSE." + ]) + # attributes set to nil - expect(Optimizely::Audience.user_meets_audience_conditions?(config, - experiment, - nil, - spy_logger)).to be false + user_meets_audience_conditions, reasons = Optimizely::Audience.user_meets_audience_conditions?(config, experiment, nil, spy_logger) + expect(user_meets_audience_conditions).to be false + expect(reasons).to eq([ + "Evaluating audiences for experiment 'test_experiment_with_audience': [\"11154\"].", + "Starting to evaluate audience '11154' with conditions: [\"and\", [\"or\", [\"or\", {\"name\": \"browser_type\", \"type\": \"custom_attribute\", \"value\": \"firefox\"}]]].", + "Audience '11154' evaluated to UNKNOWN.", + "Audiences for experiment 'test_experiment_with_audience' collectively evaluated to FALSE." + ]) + # asserts nil attributes default to empty dict expect(Optimizely::CustomAttributeConditionEvaluator).to have_received(:new).with({}, spy_logger).twice end @@ -104,10 +112,12 @@ 'test_attribute' => 'test_value_1' } allow(Optimizely::ConditionTreeEvaluator).to receive(:evaluate).and_return(true) - expect(Optimizely::Audience.user_meets_audience_conditions?(config, - experiment, - user_attributes, - spy_logger)).to be true + user_meets_audience_conditions, reasons = Optimizely::Audience.user_meets_audience_conditions?(config, experiment, user_attributes, spy_logger) + expect(user_meets_audience_conditions).to be true + expect(reasons).to eq([ + "Evaluating audiences for experiment 'test_experiment': [].", + "Audiences for experiment 'test_experiment' collectively evaluated to TRUE." + ]) end it 'should return false for user_meets_audience_conditions? when condition tree evaluator returns false or nil' do @@ -118,17 +128,22 @@ # condition tree evaluator returns nil allow(Optimizely::ConditionTreeEvaluator).to receive(:evaluate).and_return(nil) - expect(Optimizely::Audience.user_meets_audience_conditions?(config, - experiment, - user_attributes, - spy_logger)).to be false + + user_meets_audience_conditions, reasons = Optimizely::Audience.user_meets_audience_conditions?(config, experiment, user_attributes, spy_logger) + expect(user_meets_audience_conditions).to be false + expect(reasons).to eq([ + "Evaluating audiences for experiment 'test_experiment_with_audience': [\"11154\"].", + "Audiences for experiment 'test_experiment_with_audience' collectively evaluated to FALSE." + ]) # condition tree evaluator returns false allow(Optimizely::ConditionTreeEvaluator).to receive(:evaluate).and_return(false) - expect(Optimizely::Audience.user_meets_audience_conditions?(config, - experiment, - user_attributes, - spy_logger)).to be false + user_meets_audience_conditions, reasons = Optimizely::Audience.user_meets_audience_conditions?(config, experiment, user_attributes, spy_logger) + expect(user_meets_audience_conditions).to be false + expect(reasons).to eq([ + "Evaluating audiences for experiment 'test_experiment_with_audience': [\"11154\"].", + "Audiences for experiment 'test_experiment_with_audience' collectively evaluated to FALSE." + ]) end it 'should correctly evaluate audience Ids and call custom attribute evaluator for leaf nodes' do @@ -213,10 +228,13 @@ } experiment['audienceIds'] = %w[11110] - expect(Optimizely::Audience.user_meets_audience_conditions?(config, - experiment, - user_attributes, - spy_logger)).to be false + user_meets_audience_conditions, reasons = Optimizely::Audience.user_meets_audience_conditions?(config, experiment, user_attributes, spy_logger) + expect(user_meets_audience_conditions).to be false + expect(reasons).to eq([ + "Evaluating audiences for experiment 'test_experiment_with_audience': [\"11110\"].", + "Audiences for experiment 'test_experiment_with_audience' collectively evaluated to FALSE." + ]) + expect(spy_logger).to have_received(:log).once.with( Logger::DEBUG, "Evaluating audiences for experiment 'test_experiment_with_audience': " + '["11110"].' @@ -236,10 +254,17 @@ experiment['audienceIds'] = %w[11154 11155] experiment['audienceConditions'] = nil - expect(Optimizely::Audience.user_meets_audience_conditions?(config, - experiment, - user_attributes, - spy_logger)).to be false + user_meets_audience_conditions, reasons = Optimizely::Audience.user_meets_audience_conditions?(config, experiment, user_attributes, spy_logger) + expect(user_meets_audience_conditions).to be false + expect(reasons).to eq([ + "Evaluating audiences for experiment 'test_experiment_with_audience': [\"11154\", \"11155\"].", + "Starting to evaluate audience '11154' with conditions: [\"and\", [\"or\", [\"or\", {\"name\": \"browser_type\", \"type\": \"custom_attribute\", \"value\": \"firefox\"}]]].", + "Audience '11154' evaluated to UNKNOWN.", + "Starting to evaluate audience '11155' with conditions: [\"and\", [\"or\", [\"or\", {\"name\": \"browser_type\", \"type\": \"custom_attribute\", \"value\": \"chrome\"}]]].", + "Audience '11155' evaluated to UNKNOWN.", + "Audiences for experiment 'test_experiment_with_audience' collectively evaluated to FALSE." + ]) + expect(spy_logger).to have_received(:log).once.with( Logger::DEBUG, "Evaluating audiences for experiment 'test_experiment_with_audience': " + '["11154", "11155"].' diff --git a/spec/decision_service_spec.rb b/spec/decision_service_spec.rb index 21a4ff4b..aaa09264 100644 --- a/spec/decision_service_spec.rb +++ b/spec/decision_service_spec.rb @@ -69,8 +69,11 @@ it 'should return the correct variation ID for a given user ID and key of a running experiment' do variation_received, reasons = decision_service.get_variation(config, 'test_experiment', 'test_user') expect(variation_received).to eq('111128') + expect(reasons).to eq([ "User 'test_user' is not in the forced variation map.", + "Evaluating audiences for experiment 'test_experiment': [].", + "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", "Assigned bucket 4577 to user 'test_user' with bucketing ID: 'test_user'.", "User 'test_user' is in variation 'control' of experiment 'test_experiment'." ]) @@ -87,6 +90,8 @@ expect(variation_received).to eq(nil) expect(reasons).to eq([ "User 'test_user' is not in the forced variation map.", + "Evaluating audiences for experiment 'test_experiment': [].", + "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", "User 'test_user' is in no variation." ]) @@ -184,6 +189,10 @@ expect(variation_received).to eq(nil) expect(reasons).to eq([ "User 'test_user' is not in the forced variation map.", + "Evaluating audiences for experiment 'test_experiment_with_audience': [\"11154\"].", + "Starting to evaluate audience '11154' with conditions: [\"and\", [\"or\", [\"or\", {\"name\": \"browser_type\", \"type\": \"custom_attribute\", \"value\": \"firefox\"}]]].", + "Audience '11154' evaluated to FALSE.", + "Audiences for experiment 'test_experiment_with_audience' collectively evaluated to FALSE.", "User 'test_user' does not meet the conditions to be in experiment 'test_experiment_with_audience'." ]) expect(spy_logger).to have_received(:log) @@ -232,6 +241,8 @@ expect(reasons).to eq([ "User 'forced_user_with_invalid_variation' is not in the forced variation map.", "User 'forced_user_with_invalid_variation' is whitelisted into variation 'invalid_variation', which is not in the datafile.", + "Evaluating audiences for experiment 'test_experiment': [].", + "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", "Assigned bucket 877 to user 'forced_user_with_invalid_variation' with bucketing ID: 'forced_user_with_invalid_variation'.", "User 'forced_user_with_invalid_variation' is in variation 'control' of experiment 'test_experiment'." ]) @@ -262,6 +273,8 @@ expect(variation_received).to eq('111128') expect(reasons).to eq([ "User 'test_user' is not in the forced variation map.", + "Evaluating audiences for experiment 'test_experiment': [].", + "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", "Assigned bucket 4577 to user 'test_user' with bucketing ID: 'test_user'.", "User 'test_user' is in variation 'control' of experiment 'test_experiment'." ]) @@ -293,6 +306,8 @@ expect(variation_received).to eq('111129') expect(reasons).to eq([ "User 'test_user' is not in the forced variation map.", + "Evaluating audiences for experiment 'test_experiment': [].", + "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", "Assigned bucket 8933 to user 'test_user' with bucketing ID: 'pid'.", "User 'test_user' is in variation 'variation' of experiment 'test_experiment'." ]) @@ -351,6 +366,8 @@ expect(variation_received).to eq('111128') expect(reasons).to eq([ "User 'test_user' is not in the forced variation map.", + "Evaluating audiences for experiment 'test_experiment': [].", + "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", "Assigned bucket 4577 to user 'test_user' with bucketing ID: 'test_user'.", "User 'test_user' is in variation 'control' of experiment 'test_experiment'." ]) @@ -391,6 +408,8 @@ expect(reasons).to eq([ "User 'test_user' is not in the forced variation map.", "User '' was previously bucketed into variation ID '111111' for experiment '111127', but no matching variation was found. Re-bucketing user.", + "Evaluating audiences for experiment 'test_experiment': [].", + "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", "Assigned bucket 4577 to user 'test_user' with bucketing ID: 'test_user'.", "User 'test_user' is in variation 'control' of experiment 'test_experiment'." ]) @@ -418,6 +437,8 @@ expect(reasons).to eq([ "User 'test_user' is not in the forced variation map.", "Error while looking up user profile for user ID 'test_user': uncaught throw :LookupError.", + "Evaluating audiences for experiment 'test_experiment': [].", + "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", "Assigned bucket 4577 to user 'test_user' with bucketing ID: 'test_user'.", "User 'test_user' is in variation 'control' of experiment 'test_experiment'." ]) @@ -435,6 +456,8 @@ expect(variation_received).to eq('111128') expect(reasons).to eq([ "User 'test_user' is not in the forced variation map.", + "Evaluating audiences for experiment 'test_experiment': [].", + "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", "Assigned bucket 4577 to user 'test_user' with bucketing ID: 'test_user'.", "User 'test_user' is in variation 'control' of experiment 'test_experiment'." ]) @@ -452,6 +475,8 @@ expect(variation_received).to eq('111128') expect(reasons).to eq([ "User 'test_user' is not in the forced variation map.", + "Evaluating audiences for experiment 'test_experiment': [].", + "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", "Assigned bucket 4577 to user 'test_user' with bucketing ID: 'test_user'.", "User 'test_user' is in variation 'control' of experiment 'test_experiment'." ]) @@ -470,6 +495,8 @@ expect(variation_received).to eq('111128') expect(reasons).to eq([ "User 'test_user' is not in the forced variation map.", + "Evaluating audiences for experiment 'test_experiment': [].", + "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", "Assigned bucket 4577 to user 'test_user' with bucketing ID: 'test_user'.", "User 'test_user' is in variation 'control' of experiment 'test_experiment'." ]) diff --git a/spec/project_spec.rb b/spec/project_spec.rb index b907c56b..b0f22547 100644 --- a/spec/project_spec.rb +++ b/spec/project_spec.rb @@ -3904,6 +3904,10 @@ def callback(_args); end rule_key: nil, reasons: [ "User 'user1' is not in the forced variation map.", + "Evaluating audiences for experiment 'test_experiment_multivariate': [\"11154\"].", + "Starting to evaluate audience '11154' with conditions: [\"and\", [\"or\", [\"or\", {\"name\": \"browser_type\", \"type\": \"custom_attribute\", \"value\": \"firefox\"}]]].", + "Audience '11154' evaluated to UNKNOWN.", + "Audiences for experiment 'test_experiment_multivariate' collectively evaluated to FALSE.", "User 'user1' does not meet the conditions to be in experiment 'test_experiment_multivariate'.", "The user 'user1' is not bucketed into any of the experiments on the feature 'multi_variate_feature'.", "Feature flag 'multi_variate_feature' is not used in a rollout." @@ -3918,6 +3922,10 @@ def callback(_args); end enabled: false, reasons: [ "User 'user1' is not in the forced variation map.", + "Evaluating audiences for experiment 'test_experiment_multivariate': [\"11154\"].", + "Starting to evaluate audience '11154' with conditions: [\"and\", [\"or\", [\"or\", {\"name\": \"browser_type\", \"type\": \"custom_attribute\", \"value\": \"firefox\"}]]].", + "Audience '11154' evaluated to UNKNOWN.", + "Audiences for experiment 'test_experiment_multivariate' collectively evaluated to FALSE.", "User 'user1' does not meet the conditions to be in experiment 'test_experiment_multivariate'.", "The user 'user1' is not bucketed into any of the experiments on the feature 'multi_variate_feature'.", "Feature flag 'multi_variate_feature' is not used in a rollout." @@ -4249,6 +4257,10 @@ def callback(_args); end rule_key: nil, reasons: [ "User 'user1' is not in the forced variation map.", + "Evaluating audiences for experiment 'test_experiment_multivariate': [\"11154\"].", + "Starting to evaluate audience '11154' with conditions: [\"and\", [\"or\", [\"or\", {\"name\": \"browser_type\", \"type\": \"custom_attribute\", \"value\": \"firefox\"}]]].", + "Audience '11154' evaluated to UNKNOWN.", + "Audiences for experiment 'test_experiment_multivariate' collectively evaluated to FALSE.", "User 'user1' does not meet the conditions to be in experiment 'test_experiment_multivariate'.", "The user 'user1' is not bucketed into any of the experiments on the feature 'multi_variate_feature'.", "Feature flag 'multi_variate_feature' is not used in a rollout." @@ -4263,6 +4275,10 @@ def callback(_args); end enabled: false, reasons: [ "User 'user1' is not in the forced variation map.", + "Evaluating audiences for experiment 'test_experiment_multivariate': [\"11154\"].", + "Starting to evaluate audience '11154' with conditions: [\"and\", [\"or\", [\"or\", {\"name\": \"browser_type\", \"type\": \"custom_attribute\", \"value\": \"firefox\"}]]].", + "Audience '11154' evaluated to UNKNOWN.", + "Audiences for experiment 'test_experiment_multivariate' collectively evaluated to FALSE.", "User 'user1' does not meet the conditions to be in experiment 'test_experiment_multivariate'.", "The user 'user1' is not bucketed into any of the experiments on the feature 'multi_variate_feature'.", "Feature flag 'multi_variate_feature' is not used in a rollout." From a71680fb28f338a5ef5907207846379d4cb98c1f Mon Sep 17 00:00:00 2001 From: Zeeshan Ashraf Date: Tue, 22 Dec 2020 11:45:14 -0800 Subject: [PATCH 13/17] fixed an argument error --- lib/optimizely/audience.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/optimizely/audience.rb b/lib/optimizely/audience.rb index e0794da0..b94919a6 100644 --- a/lib/optimizely/audience.rb +++ b/lib/optimizely/audience.rb @@ -53,7 +53,7 @@ def user_meets_audience_conditions?(config, experiment, attributes, logger, logg # Return true if there are no audiences if audience_conditions.empty? message = format(logs_hash['AUDIENCE_EVALUATION_RESULT_COMBINED'], logging_key, 'TRUE') - logger.log(Logger::INFO) + logger.log(Logger::INFO, message) decide_reasons.push(message) return true, decide_reasons end From af181e3d4b1badb108e22a20a7d943218c0af80d Mon Sep 17 00:00:00 2001 From: Zeeshan Ashraf Date: Tue, 22 Dec 2020 14:24:57 -0800 Subject: [PATCH 14/17] removed an unwanted message fron decide reasons --- lib/optimizely/audience.rb | 1 - spec/audience_spec.rb | 11 ++--------- spec/decision_service_spec.rb | 12 ------------ spec/project_spec.rb | 4 ---- 4 files changed, 2 insertions(+), 26 deletions(-) diff --git a/lib/optimizely/audience.rb b/lib/optimizely/audience.rb index b94919a6..0a2a6a2e 100644 --- a/lib/optimizely/audience.rb +++ b/lib/optimizely/audience.rb @@ -48,7 +48,6 @@ def user_meets_audience_conditions?(config, experiment, attributes, logger, logg message = format(logs_hash['EVALUATING_AUDIENCES_COMBINED'], logging_key, audience_conditions) logger.log(Logger::DEBUG, message) - decide_reasons.push(message) # Return true if there are no audiences if audience_conditions.empty? diff --git a/spec/audience_spec.rb b/spec/audience_spec.rb index d353d9f2..1f304a97 100644 --- a/spec/audience_spec.rb +++ b/spec/audience_spec.rb @@ -43,7 +43,7 @@ user_meets_audience_conditions, reasons = Optimizely::Audience.user_meets_audience_conditions?(config, experiment, user_attributes, spy_logger) expect(user_meets_audience_conditions).to be true - expect(reasons).to eq(["Evaluating audiences for experiment 'test_experiment': [].", "Audiences for experiment 'test_experiment' collectively evaluated to TRUE."]) + expect(reasons).to eq(["Audiences for experiment 'test_experiment' collectively evaluated to TRUE."]) # Audience Ids is Empty and Audience Conditions is nil experiment = config.experiment_key_map['test_experiment'] @@ -52,7 +52,7 @@ user_meets_audience_conditions, reasons = Optimizely::Audience.user_meets_audience_conditions?(config, experiment, user_attributes, spy_logger) expect(user_meets_audience_conditions).to be true - expect(reasons).to eq(["Evaluating audiences for experiment 'test_experiment': [].", "Audiences for experiment 'test_experiment' collectively evaluated to TRUE."]) + expect(reasons).to eq(["Audiences for experiment 'test_experiment' collectively evaluated to TRUE."]) end it 'should pass conditions when audience conditions exist else audienceIds are passed' do @@ -86,7 +86,6 @@ user_meets_audience_conditions, reasons = Optimizely::Audience.user_meets_audience_conditions?(config, experiment, {}, spy_logger) expect(user_meets_audience_conditions).to be false expect(reasons).to eq([ - "Evaluating audiences for experiment 'test_experiment_with_audience': [\"11154\"].", "Starting to evaluate audience '11154' with conditions: [\"and\", [\"or\", [\"or\", {\"name\": \"browser_type\", \"type\": \"custom_attribute\", \"value\": \"firefox\"}]]].", "Audience '11154' evaluated to UNKNOWN.", "Audiences for experiment 'test_experiment_with_audience' collectively evaluated to FALSE." @@ -96,7 +95,6 @@ user_meets_audience_conditions, reasons = Optimizely::Audience.user_meets_audience_conditions?(config, experiment, nil, spy_logger) expect(user_meets_audience_conditions).to be false expect(reasons).to eq([ - "Evaluating audiences for experiment 'test_experiment_with_audience': [\"11154\"].", "Starting to evaluate audience '11154' with conditions: [\"and\", [\"or\", [\"or\", {\"name\": \"browser_type\", \"type\": \"custom_attribute\", \"value\": \"firefox\"}]]].", "Audience '11154' evaluated to UNKNOWN.", "Audiences for experiment 'test_experiment_with_audience' collectively evaluated to FALSE." @@ -115,7 +113,6 @@ user_meets_audience_conditions, reasons = Optimizely::Audience.user_meets_audience_conditions?(config, experiment, user_attributes, spy_logger) expect(user_meets_audience_conditions).to be true expect(reasons).to eq([ - "Evaluating audiences for experiment 'test_experiment': [].", "Audiences for experiment 'test_experiment' collectively evaluated to TRUE." ]) end @@ -132,7 +129,6 @@ user_meets_audience_conditions, reasons = Optimizely::Audience.user_meets_audience_conditions?(config, experiment, user_attributes, spy_logger) expect(user_meets_audience_conditions).to be false expect(reasons).to eq([ - "Evaluating audiences for experiment 'test_experiment_with_audience': [\"11154\"].", "Audiences for experiment 'test_experiment_with_audience' collectively evaluated to FALSE." ]) @@ -141,7 +137,6 @@ user_meets_audience_conditions, reasons = Optimizely::Audience.user_meets_audience_conditions?(config, experiment, user_attributes, spy_logger) expect(user_meets_audience_conditions).to be false expect(reasons).to eq([ - "Evaluating audiences for experiment 'test_experiment_with_audience': [\"11154\"].", "Audiences for experiment 'test_experiment_with_audience' collectively evaluated to FALSE." ]) end @@ -231,7 +226,6 @@ user_meets_audience_conditions, reasons = Optimizely::Audience.user_meets_audience_conditions?(config, experiment, user_attributes, spy_logger) expect(user_meets_audience_conditions).to be false expect(reasons).to eq([ - "Evaluating audiences for experiment 'test_experiment_with_audience': [\"11110\"].", "Audiences for experiment 'test_experiment_with_audience' collectively evaluated to FALSE." ]) @@ -257,7 +251,6 @@ user_meets_audience_conditions, reasons = Optimizely::Audience.user_meets_audience_conditions?(config, experiment, user_attributes, spy_logger) expect(user_meets_audience_conditions).to be false expect(reasons).to eq([ - "Evaluating audiences for experiment 'test_experiment_with_audience': [\"11154\", \"11155\"].", "Starting to evaluate audience '11154' with conditions: [\"and\", [\"or\", [\"or\", {\"name\": \"browser_type\", \"type\": \"custom_attribute\", \"value\": \"firefox\"}]]].", "Audience '11154' evaluated to UNKNOWN.", "Starting to evaluate audience '11155' with conditions: [\"and\", [\"or\", [\"or\", {\"name\": \"browser_type\", \"type\": \"custom_attribute\", \"value\": \"chrome\"}]]].", diff --git a/spec/decision_service_spec.rb b/spec/decision_service_spec.rb index aaa09264..52da5dd5 100644 --- a/spec/decision_service_spec.rb +++ b/spec/decision_service_spec.rb @@ -72,7 +72,6 @@ expect(reasons).to eq([ "User 'test_user' is not in the forced variation map.", - "Evaluating audiences for experiment 'test_experiment': [].", "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", "Assigned bucket 4577 to user 'test_user' with bucketing ID: 'test_user'.", "User 'test_user' is in variation 'control' of experiment 'test_experiment'." @@ -90,7 +89,6 @@ expect(variation_received).to eq(nil) expect(reasons).to eq([ "User 'test_user' is not in the forced variation map.", - "Evaluating audiences for experiment 'test_experiment': [].", "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", "User 'test_user' is in no variation." ]) @@ -189,7 +187,6 @@ expect(variation_received).to eq(nil) expect(reasons).to eq([ "User 'test_user' is not in the forced variation map.", - "Evaluating audiences for experiment 'test_experiment_with_audience': [\"11154\"].", "Starting to evaluate audience '11154' with conditions: [\"and\", [\"or\", [\"or\", {\"name\": \"browser_type\", \"type\": \"custom_attribute\", \"value\": \"firefox\"}]]].", "Audience '11154' evaluated to FALSE.", "Audiences for experiment 'test_experiment_with_audience' collectively evaluated to FALSE.", @@ -241,7 +238,6 @@ expect(reasons).to eq([ "User 'forced_user_with_invalid_variation' is not in the forced variation map.", "User 'forced_user_with_invalid_variation' is whitelisted into variation 'invalid_variation', which is not in the datafile.", - "Evaluating audiences for experiment 'test_experiment': [].", "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", "Assigned bucket 877 to user 'forced_user_with_invalid_variation' with bucketing ID: 'forced_user_with_invalid_variation'.", "User 'forced_user_with_invalid_variation' is in variation 'control' of experiment 'test_experiment'." @@ -273,7 +269,6 @@ expect(variation_received).to eq('111128') expect(reasons).to eq([ "User 'test_user' is not in the forced variation map.", - "Evaluating audiences for experiment 'test_experiment': [].", "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", "Assigned bucket 4577 to user 'test_user' with bucketing ID: 'test_user'.", "User 'test_user' is in variation 'control' of experiment 'test_experiment'." @@ -306,7 +301,6 @@ expect(variation_received).to eq('111129') expect(reasons).to eq([ "User 'test_user' is not in the forced variation map.", - "Evaluating audiences for experiment 'test_experiment': [].", "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", "Assigned bucket 8933 to user 'test_user' with bucketing ID: 'pid'.", "User 'test_user' is in variation 'variation' of experiment 'test_experiment'." @@ -366,7 +360,6 @@ expect(variation_received).to eq('111128') expect(reasons).to eq([ "User 'test_user' is not in the forced variation map.", - "Evaluating audiences for experiment 'test_experiment': [].", "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", "Assigned bucket 4577 to user 'test_user' with bucketing ID: 'test_user'.", "User 'test_user' is in variation 'control' of experiment 'test_experiment'." @@ -408,7 +401,6 @@ expect(reasons).to eq([ "User 'test_user' is not in the forced variation map.", "User '' was previously bucketed into variation ID '111111' for experiment '111127', but no matching variation was found. Re-bucketing user.", - "Evaluating audiences for experiment 'test_experiment': [].", "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", "Assigned bucket 4577 to user 'test_user' with bucketing ID: 'test_user'.", "User 'test_user' is in variation 'control' of experiment 'test_experiment'." @@ -437,7 +429,6 @@ expect(reasons).to eq([ "User 'test_user' is not in the forced variation map.", "Error while looking up user profile for user ID 'test_user': uncaught throw :LookupError.", - "Evaluating audiences for experiment 'test_experiment': [].", "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", "Assigned bucket 4577 to user 'test_user' with bucketing ID: 'test_user'.", "User 'test_user' is in variation 'control' of experiment 'test_experiment'." @@ -456,7 +447,6 @@ expect(variation_received).to eq('111128') expect(reasons).to eq([ "User 'test_user' is not in the forced variation map.", - "Evaluating audiences for experiment 'test_experiment': [].", "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", "Assigned bucket 4577 to user 'test_user' with bucketing ID: 'test_user'.", "User 'test_user' is in variation 'control' of experiment 'test_experiment'." @@ -475,7 +465,6 @@ expect(variation_received).to eq('111128') expect(reasons).to eq([ "User 'test_user' is not in the forced variation map.", - "Evaluating audiences for experiment 'test_experiment': [].", "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", "Assigned bucket 4577 to user 'test_user' with bucketing ID: 'test_user'.", "User 'test_user' is in variation 'control' of experiment 'test_experiment'." @@ -495,7 +484,6 @@ expect(variation_received).to eq('111128') expect(reasons).to eq([ "User 'test_user' is not in the forced variation map.", - "Evaluating audiences for experiment 'test_experiment': [].", "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", "Assigned bucket 4577 to user 'test_user' with bucketing ID: 'test_user'.", "User 'test_user' is in variation 'control' of experiment 'test_experiment'." diff --git a/spec/project_spec.rb b/spec/project_spec.rb index b0f22547..80bbfbab 100644 --- a/spec/project_spec.rb +++ b/spec/project_spec.rb @@ -3904,7 +3904,6 @@ def callback(_args); end rule_key: nil, reasons: [ "User 'user1' is not in the forced variation map.", - "Evaluating audiences for experiment 'test_experiment_multivariate': [\"11154\"].", "Starting to evaluate audience '11154' with conditions: [\"and\", [\"or\", [\"or\", {\"name\": \"browser_type\", \"type\": \"custom_attribute\", \"value\": \"firefox\"}]]].", "Audience '11154' evaluated to UNKNOWN.", "Audiences for experiment 'test_experiment_multivariate' collectively evaluated to FALSE.", @@ -3922,7 +3921,6 @@ def callback(_args); end enabled: false, reasons: [ "User 'user1' is not in the forced variation map.", - "Evaluating audiences for experiment 'test_experiment_multivariate': [\"11154\"].", "Starting to evaluate audience '11154' with conditions: [\"and\", [\"or\", [\"or\", {\"name\": \"browser_type\", \"type\": \"custom_attribute\", \"value\": \"firefox\"}]]].", "Audience '11154' evaluated to UNKNOWN.", "Audiences for experiment 'test_experiment_multivariate' collectively evaluated to FALSE.", @@ -4257,7 +4255,6 @@ def callback(_args); end rule_key: nil, reasons: [ "User 'user1' is not in the forced variation map.", - "Evaluating audiences for experiment 'test_experiment_multivariate': [\"11154\"].", "Starting to evaluate audience '11154' with conditions: [\"and\", [\"or\", [\"or\", {\"name\": \"browser_type\", \"type\": \"custom_attribute\", \"value\": \"firefox\"}]]].", "Audience '11154' evaluated to UNKNOWN.", "Audiences for experiment 'test_experiment_multivariate' collectively evaluated to FALSE.", @@ -4275,7 +4272,6 @@ def callback(_args); end enabled: false, reasons: [ "User 'user1' is not in the forced variation map.", - "Evaluating audiences for experiment 'test_experiment_multivariate': [\"11154\"].", "Starting to evaluate audience '11154' with conditions: [\"and\", [\"or\", [\"or\", {\"name\": \"browser_type\", \"type\": \"custom_attribute\", \"value\": \"firefox\"}]]].", "Audience '11154' evaluated to UNKNOWN.", "Audiences for experiment 'test_experiment_multivariate' collectively evaluated to FALSE.", From 5720f831ffb817c9cccad1675cabae0a723dc927 Mon Sep 17 00:00:00 2001 From: Zeeshan Ashraf Date: Tue, 22 Dec 2020 14:33:40 -0800 Subject: [PATCH 15/17] removed another unwanted reason --- lib/optimizely/decision_service.rb | 1 - spec/decision_service_spec.rb | 23 ++--------------------- spec/project_spec.rb | 4 ---- 3 files changed, 2 insertions(+), 26 deletions(-) diff --git a/lib/optimizely/decision_service.rb b/lib/optimizely/decision_service.rb index de6f6589..799543d7 100644 --- a/lib/optimizely/decision_service.rb +++ b/lib/optimizely/decision_service.rb @@ -343,7 +343,6 @@ def get_forced_variation(project_config, experiment_key, user_id) unless @forced_variation_map.key? user_id message = "User '#{user_id}' is not in the forced variation map." @logger.log(Logger::DEBUG, message) - decide_reasons.push(message) return nil, decide_reasons end diff --git a/spec/decision_service_spec.rb b/spec/decision_service_spec.rb index 52da5dd5..14459826 100644 --- a/spec/decision_service_spec.rb +++ b/spec/decision_service_spec.rb @@ -71,7 +71,6 @@ expect(variation_received).to eq('111128') expect(reasons).to eq([ - "User 'test_user' is not in the forced variation map.", "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", "Assigned bucket 4577 to user 'test_user' with bucketing ID: 'test_user'.", "User 'test_user' is in variation 'control' of experiment 'test_experiment'." @@ -88,7 +87,6 @@ variation_received, reasons = decision_service.get_variation(config, 'test_experiment', 'test_user') expect(variation_received).to eq(nil) expect(reasons).to eq([ - "User 'test_user' is not in the forced variation map.", "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", "User 'test_user' is in no variation." ]) @@ -101,7 +99,6 @@ variation_received, reasons = decision_service.get_variation(config, 'test_experiment', 'forced_user1') expect(variation_received).to eq('111128') expect(reasons).to eq([ - "User 'forced_user1' is not in the forced variation map.", "User 'forced_user1' is whitelisted into variation 'control' of experiment 'test_experiment'." ]) expect(spy_logger).to have_received(:log) @@ -110,7 +107,6 @@ variation_received, reasons = decision_service.get_variation(config, 'test_experiment', 'forced_user2') expect(variation_received).to eq('111129') expect(reasons).to eq([ - "User 'forced_user2' is not in the forced variation map.", "User 'forced_user2' is whitelisted into variation 'variation' of experiment 'test_experiment'." ]) expect(spy_logger).to have_received(:log) @@ -131,7 +127,6 @@ variation_received, reasons = decision_service.get_variation(config, 'test_experiment', 'forced_user1', user_attributes) expect(variation_received).to eq('111128') expect(reasons).to eq([ - "User 'forced_user1' is not in the forced variation map.", "User 'forced_user1' is whitelisted into variation 'control' of experiment 'test_experiment'." ]) expect(spy_logger).to have_received(:log) @@ -140,7 +135,6 @@ variation_received, reasons = decision_service.get_variation(config, 'test_experiment', 'forced_user2', user_attributes) expect(variation_received).to eq('111129') expect(reasons).to eq([ - "User 'forced_user2' is not in the forced variation map.", "User 'forced_user2' is whitelisted into variation 'variation' of experiment 'test_experiment'." ]) expect(spy_logger).to have_received(:log) @@ -157,7 +151,6 @@ variation_received, reasons = decision_service.get_variation(config, 'test_experiment_with_audience', 'forced_audience_user', user_attributes) expect(variation_received).to eq('122229') expect(reasons).to eq([ - "User 'forced_audience_user' is not in the forced variation map.", "User 'forced_audience_user' is whitelisted into variation 'variation_with_audience' of experiment 'test_experiment_with_audience'." ]) expect(spy_logger).to have_received(:log) @@ -186,7 +179,6 @@ variation_received, reasons = decision_service.get_variation(config, 'test_experiment_with_audience', 'test_user', user_attributes) expect(variation_received).to eq(nil) expect(reasons).to eq([ - "User 'test_user' is not in the forced variation map.", "Starting to evaluate audience '11154' with conditions: [\"and\", [\"or\", [\"or\", {\"name\": \"browser_type\", \"type\": \"custom_attribute\", \"value\": \"firefox\"}]]].", "Audience '11154' evaluated to FALSE.", "Audiences for experiment 'test_experiment_with_audience' collectively evaluated to FALSE.", @@ -220,7 +212,6 @@ variation_received, reasons = decision_service.get_variation(config, 'group1_exp2', 'forced_group_user1') expect(variation_received).to eq('130004') expect(reasons).to eq([ - "User 'forced_group_user1' is not in the forced variation map.", "User 'forced_group_user1' is whitelisted into variation 'g1_e2_v2' of experiment 'group1_exp2'." ]) expect(spy_logger).to have_received(:log) @@ -236,7 +227,6 @@ variation_received, reasons = decision_service.get_variation(config, 'test_experiment', 'forced_user_with_invalid_variation') expect(variation_received).to eq('111128') expect(reasons).to eq([ - "User 'forced_user_with_invalid_variation' is not in the forced variation map.", "User 'forced_user_with_invalid_variation' is whitelisted into variation 'invalid_variation', which is not in the datafile.", "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", "Assigned bucket 877 to user 'forced_user_with_invalid_variation' with bucketing ID: 'forced_user_with_invalid_variation'.", @@ -268,7 +258,6 @@ variation_received, reasons = decision_service.get_variation(config, 'test_experiment', 'test_user') expect(variation_received).to eq('111128') expect(reasons).to eq([ - "User 'test_user' is not in the forced variation map.", "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", "Assigned bucket 4577 to user 'test_user' with bucketing ID: 'test_user'.", "User 'test_user' is in variation 'control' of experiment 'test_experiment'." @@ -300,7 +289,6 @@ variation_received, reasons = decision_service.get_variation(config, 'test_experiment', 'test_user', user_attributes) expect(variation_received).to eq('111129') expect(reasons).to eq([ - "User 'test_user' is not in the forced variation map.", "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", "Assigned bucket 8933 to user 'test_user' with bucketing ID: 'pid'.", "User 'test_user' is in variation 'variation' of experiment 'test_experiment'." @@ -329,7 +317,6 @@ variation_received, reasons = decision_service.get_variation(config, 'test_experiment', 'test_user') expect(variation_received).to eq('111129') expect(reasons).to eq([ - "User 'test_user' is not in the forced variation map.", "Returning previously activated variation ID 111129 of experiment 'test_experiment' for user 'test_user' from user profile." ]) expect(spy_logger).to have_received(:log).once @@ -359,7 +346,6 @@ variation_received, reasons = decision_service.get_variation(config, 'test_experiment', 'test_user') expect(variation_received).to eq('111128') expect(reasons).to eq([ - "User 'test_user' is not in the forced variation map.", "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", "Assigned bucket 4577 to user 'test_user' with bucketing ID: 'test_user'.", "User 'test_user' is in variation 'control' of experiment 'test_experiment'." @@ -399,7 +385,6 @@ variation_received, reasons = decision_service.get_variation(config, 'test_experiment', 'test_user') expect(variation_received).to eq('111128') expect(reasons).to eq([ - "User 'test_user' is not in the forced variation map.", "User '' was previously bucketed into variation ID '111111' for experiment '111127', but no matching variation was found. Re-bucketing user.", "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", "Assigned bucket 4577 to user 'test_user' with bucketing ID: 'test_user'.", @@ -427,7 +412,6 @@ variation_received, reasons = decision_service.get_variation(config, 'test_experiment', 'test_user') expect(variation_received).to eq('111128') expect(reasons).to eq([ - "User 'test_user' is not in the forced variation map.", "Error while looking up user profile for user ID 'test_user': uncaught throw :LookupError.", "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", "Assigned bucket 4577 to user 'test_user' with bucketing ID: 'test_user'.", @@ -446,7 +430,6 @@ variation_received, reasons = decision_service.get_variation(config, 'test_experiment', 'test_user') expect(variation_received).to eq('111128') expect(reasons).to eq([ - "User 'test_user' is not in the forced variation map.", "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", "Assigned bucket 4577 to user 'test_user' with bucketing ID: 'test_user'.", "User 'test_user' is in variation 'control' of experiment 'test_experiment'." @@ -464,7 +447,6 @@ variation_received, reasons = decision_service.get_variation(config, 'test_experiment', 'test_user', nil, [Optimizely::Decide::OptimizelyDecideOption::IGNORE_USER_PROFILE_SERVICE]) expect(variation_received).to eq('111128') expect(reasons).to eq([ - "User 'test_user' is not in the forced variation map.", "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", "Assigned bucket 4577 to user 'test_user' with bucketing ID: 'test_user'.", "User 'test_user' is in variation 'control' of experiment 'test_experiment'." @@ -483,7 +465,6 @@ variation_received, reasons = decision_service.get_variation(config, 'test_experiment', 'test_user') expect(variation_received).to eq('111128') expect(reasons).to eq([ - "User 'test_user' is not in the forced variation map.", "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", "Assigned bucket 4577 to user 'test_user' with bucketing ID: 'test_user'.", "User 'test_user' is in variation 'control' of experiment 'test_experiment'." @@ -915,7 +896,7 @@ it 'should log a message and return nil when user is not in forced variation map' do variation_received, reasons = decision_service.get_forced_variation(config, valid_experiment[:key], user_id) expect(variation_received).to eq(nil) - expect(reasons).to eq(["User '#{user_id}' is not in the forced variation map."]) + expect(reasons).to eq([]) expect(spy_logger).to have_received(:log).with(Logger::DEBUG, "User '#{user_id}' is not in the forced variation map.") end @@ -923,7 +904,7 @@ it 'should return nil when experiment key is not in datafile' do variation_received, reasons = decision_service.get_forced_variation(config, invalid_experiment_key, user_id) expect(variation_received).to eq(nil) - expect(reasons).to eq(["User 'test_user' is not in the forced variation map."]) + expect(reasons).to eq([]) end end diff --git a/spec/project_spec.rb b/spec/project_spec.rb index 80bbfbab..824d0643 100644 --- a/spec/project_spec.rb +++ b/spec/project_spec.rb @@ -3903,7 +3903,6 @@ def callback(_args); end variation_key: nil, rule_key: nil, reasons: [ - "User 'user1' is not in the forced variation map.", "Starting to evaluate audience '11154' with conditions: [\"and\", [\"or\", [\"or\", {\"name\": \"browser_type\", \"type\": \"custom_attribute\", \"value\": \"firefox\"}]]].", "Audience '11154' evaluated to UNKNOWN.", "Audiences for experiment 'test_experiment_multivariate' collectively evaluated to FALSE.", @@ -3920,7 +3919,6 @@ def callback(_args); end flag_key: 'multi_variate_feature', enabled: false, reasons: [ - "User 'user1' is not in the forced variation map.", "Starting to evaluate audience '11154' with conditions: [\"and\", [\"or\", [\"or\", {\"name\": \"browser_type\", \"type\": \"custom_attribute\", \"value\": \"firefox\"}]]].", "Audience '11154' evaluated to UNKNOWN.", "Audiences for experiment 'test_experiment_multivariate' collectively evaluated to FALSE.", @@ -4254,7 +4252,6 @@ def callback(_args); end variation_key: nil, rule_key: nil, reasons: [ - "User 'user1' is not in the forced variation map.", "Starting to evaluate audience '11154' with conditions: [\"and\", [\"or\", [\"or\", {\"name\": \"browser_type\", \"type\": \"custom_attribute\", \"value\": \"firefox\"}]]].", "Audience '11154' evaluated to UNKNOWN.", "Audiences for experiment 'test_experiment_multivariate' collectively evaluated to FALSE.", @@ -4271,7 +4268,6 @@ def callback(_args); end flag_key: 'multi_variate_feature', enabled: false, reasons: [ - "User 'user1' is not in the forced variation map.", "Starting to evaluate audience '11154' with conditions: [\"and\", [\"or\", [\"or\", {\"name\": \"browser_type\", \"type\": \"custom_attribute\", \"value\": \"firefox\"}]]].", "Audience '11154' evaluated to UNKNOWN.", "Audiences for experiment 'test_experiment_multivariate' collectively evaluated to FALSE.", From dd4add43248d3290eeb5c16907c5d7480531d262 Mon Sep 17 00:00:00 2001 From: Zeeshan Ashraf Date: Tue, 22 Dec 2020 14:42:15 -0800 Subject: [PATCH 16/17] removed unnecessary bucketing reason --- lib/optimizely/bucketer.rb | 1 - spec/bucketing_spec.rb | 11 ++++------- spec/decision_service_spec.rb | 10 ---------- 3 files changed, 4 insertions(+), 18 deletions(-) diff --git a/lib/optimizely/bucketer.rb b/lib/optimizely/bucketer.rb index afa8f89b..0400f354 100644 --- a/lib/optimizely/bucketer.rb +++ b/lib/optimizely/bucketer.rb @@ -118,7 +118,6 @@ def find_bucket(bucketing_id, user_id, parent_id, traffic_allocations) message = "Assigned bucket #{bucket_value} to user '#{user_id}' with bucketing ID: '#{bucketing_id}'." @logger.log(Logger::DEBUG, message) - decide_reasons.push(message) traffic_allocations.each do |traffic_allocation| current_end_of_range = traffic_allocation['endOfRange'] diff --git a/spec/bucketing_spec.rb b/spec/bucketing_spec.rb index 8e68cd95..be3c4e48 100644 --- a/spec/bucketing_spec.rb +++ b/spec/bucketing_spec.rb @@ -85,7 +85,6 @@ def get_bucketing_key(bucketing_id, entity_id = nil) variation_received, reasons = bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user') expect(variation_received).to be_nil expect(reasons).to eq([ - "Assigned bucket 3000 to user 'test_user' with bucketing ID: 'bucket_id_ignored'.", "User 'test_user' is not in experiment 'group1_exp2' of group 101." ]) expect(spy_logger).to have_received(:log) @@ -112,7 +111,7 @@ def get_bucketing_key(bucketing_id, entity_id = nil) expected_variation = config.get_variation_from_id('group2_exp1', '144443') variation_received, reasons = bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user') expect(variation_received).to eq(expected_variation) - expect(reasons).to eq(["Assigned bucket 3000 to user 'test_user' with bucketing ID: 'bucket_id_ignored'."]) + expect(reasons).to eq([]) 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'.") @@ -123,7 +122,7 @@ def get_bucketing_key(bucketing_id, entity_id = nil) experiment = config.get_experiment_from_key('group2_exp1') variation_received, reasons = bucketer.bucket(config, experiment, 'bucket_id_ignored', 'test_user') - expect(reasons).to eq(["Assigned bucket 50000 to user 'test_user' with bucketing ID: 'bucket_id_ignored'."]) + expect(reasons).to eq([]) expect(variation_received).to be_nil expect(spy_logger).to have_received(:log).once expect(spy_logger).to have_received(:log) @@ -170,14 +169,14 @@ def get_bucketing_key(bucketing_id, entity_id = nil) expected_variation = config.get_variation_from_id('test_experiment', '111128') variation_received, reasons = bucketer.bucket(config, experiment, 'test_user', 'test_user') expect(variation_received).to be(expected_variation) - expect(reasons).to eq(["Assigned bucket 4577 to user 'test_user' with bucketing ID: 'test_user'."]) + expect(reasons).to eq([]) # 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') variation_received, reasons = bucketer.bucket(config, experiment, 'any_string789', 'test_user') expect(variation_received).to be(expected_variation) - expect(reasons).to eq(["Assigned bucket 9941 to user 'test_user' with bucketing ID: 'any_string789'."]) + expect(reasons).to eq([]) end # Bucketing with invalid experiment key and bucketing ID @@ -196,14 +195,12 @@ def get_bucketing_key(bucketing_id, entity_id = nil) variation_received, reasons = bucketer.bucket(config, experiment, 'test_user', 'test_user') expect(variation_received).to be(expected_variation) expect(reasons).to eq([ - "Assigned bucket 7543 to user 'test_user' with bucketing ID: 'test_user'.", "User 'test_user' is not in experiment 'group1_exp1' of group 101." ]) expected_variation = config.get_variation_from_id('group1_exp1', '130002') variation_received, = bucketer.bucket(config, experiment, '123456789', 'test_user') expect(variation_received).to be(expected_variation) expect(reasons).to eq([ - "Assigned bucket 7543 to user 'test_user' with bucketing ID: 'test_user'.", "User 'test_user' is not in experiment 'group1_exp1' of group 101." ]) end diff --git a/spec/decision_service_spec.rb b/spec/decision_service_spec.rb index 14459826..13eb8269 100644 --- a/spec/decision_service_spec.rb +++ b/spec/decision_service_spec.rb @@ -72,7 +72,6 @@ expect(reasons).to eq([ "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", - "Assigned bucket 4577 to user 'test_user' with bucketing ID: 'test_user'.", "User 'test_user' is in variation 'control' of experiment 'test_experiment'." ]) @@ -229,7 +228,6 @@ expect(reasons).to eq([ "User 'forced_user_with_invalid_variation' is whitelisted into variation 'invalid_variation', which is not in the datafile.", "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", - "Assigned bucket 877 to user 'forced_user_with_invalid_variation' with bucketing ID: 'forced_user_with_invalid_variation'.", "User 'forced_user_with_invalid_variation' is in variation 'control' of experiment 'test_experiment'." ]) expect(spy_logger).to have_received(:log) @@ -259,7 +257,6 @@ expect(variation_received).to eq('111128') expect(reasons).to eq([ "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", - "Assigned bucket 4577 to user 'test_user' with bucketing ID: 'test_user'.", "User 'test_user' is in variation 'control' of experiment 'test_experiment'." ]) @@ -290,7 +287,6 @@ expect(variation_received).to eq('111129') expect(reasons).to eq([ "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", - "Assigned bucket 8933 to user 'test_user' with bucketing ID: 'pid'.", "User 'test_user' is in variation 'variation' of experiment 'test_experiment'." ]) @@ -347,7 +343,6 @@ expect(variation_received).to eq('111128') expect(reasons).to eq([ "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", - "Assigned bucket 4577 to user 'test_user' with bucketing ID: 'test_user'.", "User 'test_user' is in variation 'control' of experiment 'test_experiment'." ]) @@ -387,7 +382,6 @@ expect(reasons).to eq([ "User '' was previously bucketed into variation ID '111111' for experiment '111127', but no matching variation was found. Re-bucketing user.", "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", - "Assigned bucket 4577 to user 'test_user' with bucketing ID: 'test_user'.", "User 'test_user' is in variation 'control' of experiment 'test_experiment'." ]) @@ -414,7 +408,6 @@ expect(reasons).to eq([ "Error while looking up user profile for user ID 'test_user': uncaught throw :LookupError.", "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", - "Assigned bucket 4577 to user 'test_user' with bucketing ID: 'test_user'.", "User 'test_user' is in variation 'control' of experiment 'test_experiment'." ]) @@ -431,7 +424,6 @@ expect(variation_received).to eq('111128') expect(reasons).to eq([ "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", - "Assigned bucket 4577 to user 'test_user' with bucketing ID: 'test_user'.", "User 'test_user' is in variation 'control' of experiment 'test_experiment'." ]) @@ -448,7 +440,6 @@ expect(variation_received).to eq('111128') expect(reasons).to eq([ "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", - "Assigned bucket 4577 to user 'test_user' with bucketing ID: 'test_user'.", "User 'test_user' is in variation 'control' of experiment 'test_experiment'." ]) @@ -466,7 +457,6 @@ expect(variation_received).to eq('111128') expect(reasons).to eq([ "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", - "Assigned bucket 4577 to user 'test_user' with bucketing ID: 'test_user'.", "User 'test_user' is in variation 'control' of experiment 'test_experiment'." ]) From 87894b3019af41a736aeba541fba2197776433ec Mon Sep 17 00:00:00 2001 From: Zeeshan Ashraf Date: Tue, 22 Dec 2020 14:53:17 -0800 Subject: [PATCH 17/17] fixed a log message --- lib/optimizely/decision_service.rb | 2 +- spec/decision_service_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/optimizely/decision_service.rb b/lib/optimizely/decision_service.rb index 799543d7..ecb5c534 100644 --- a/lib/optimizely/decision_service.rb +++ b/lib/optimizely/decision_service.rb @@ -425,7 +425,7 @@ def get_saved_variation_id(project_config, experiment_id, user_profile) variation_id = decision[:variation_id] return variation_id, nil if project_config.variation_id_exists?(experiment_id, variation_id) - message = "User '#{user_profile['user_id']}' was previously bucketed into variation ID '#{variation_id}' for experiment '#{experiment_id}', but no matching variation was found. Re-bucketing user." + message = "User '#{user_profile[:user_id]}' was previously bucketed into variation ID '#{variation_id}' for experiment '#{experiment_id}', but no matching variation was found. Re-bucketing user." @logger.log(Logger::INFO, message) [nil, message] diff --git a/spec/decision_service_spec.rb b/spec/decision_service_spec.rb index 13eb8269..2f149b06 100644 --- a/spec/decision_service_spec.rb +++ b/spec/decision_service_spec.rb @@ -380,7 +380,7 @@ variation_received, reasons = decision_service.get_variation(config, 'test_experiment', 'test_user') expect(variation_received).to eq('111128') expect(reasons).to eq([ - "User '' was previously bucketed into variation ID '111111' for experiment '111127', but no matching variation was found. Re-bucketing user.", + "User 'test_user' was previously bucketed into variation ID '111111' for experiment '111127', but no matching variation was found. Re-bucketing user.", "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", "User 'test_user' is in variation 'control' of experiment 'test_experiment'." ])