From c6129f72b52764f7dd7f9dd81d19ee28e5cfdc98 Mon Sep 17 00:00:00 2001 From: ozayr-zaviar <54209343+ozayr-zaviar@users.noreply.github.com> Date: Tue, 27 Jul 2021 04:40:56 +0500 Subject: [PATCH 1/2] feat: Duplicate experiment key issue with multi feature flag (#282) --- lib/optimizely.rb | 15 ++- lib/optimizely/bucketer.rb | 4 +- .../config/datafile_project_config.rb | 93 ++++++++++++-- lib/optimizely/decision_service.rb | 40 +++--- lib/optimizely/project_config.rb | 8 +- spec/config/datafile_project_config_spec.rb | 72 ++++++++++- spec/decision_service_spec.rb | 114 +++++++++--------- 7 files changed, 243 insertions(+), 103 deletions(-) diff --git a/lib/optimizely.rb b/lib/optimizely.rb index 8e4ec873..98ebdd39 100644 --- a/lib/optimizely.rb +++ b/lib/optimizely.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true # -# Copyright 2016-2020, Optimizely and contributors +# Copyright 2016-2021, Optimizely and contributors # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -877,12 +877,14 @@ def get_variation_with_config(experiment_key, user_id, attributes, config) experiment = config.get_experiment_from_key(experiment_key) return nil if experiment.nil? + experiment_id = experiment['id'] + 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_id, 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']) + decision_notification_type = if config.feature_experiment?(experiment_id) Helpers::Constants::DECISION_NOTIFICATION_TYPES['FEATURE_TEST'] else Helpers::Constants::DECISION_NOTIFICATION_TYPES['AB_TEST'] @@ -1078,10 +1080,11 @@ def send_impression(config, experiment, variation_key, flag_key, rule_key, enabl } end + experiment_id = experiment['id'] experiment_key = experiment['key'] variation_id = '' - variation_id = config.get_variation_id_from_key(experiment_key, variation_key) if experiment_key != '' + variation_id = config.get_variation_id_from_key_by_experiment_id(experiment_id, variation_key) if experiment_id != '' metadata = { flag_key: flag_key, @@ -1097,9 +1100,9 @@ def send_impression(config, experiment, variation_key, flag_key, rule_key, enabl @logger.log(Logger::INFO, "Activating user '#{user_id}' in experiment '#{experiment_key}'.") - experiment = nil if experiment_key == '' + experiment = nil if experiment_id == '' variation = nil - variation = config.get_variation_from_id(experiment_key, variation_id) unless experiment.nil? + variation = config.get_variation_from_id_by_experiment_id(experiment_id, variation_id) unless experiment.nil? log_event = EventFactory.create_log_event(user_event, @logger) @notification_center.send_notifications( NotificationCenter::NOTIFICATION_TYPES[:ACTIVATE], diff --git a/lib/optimizely/bucketer.rb b/lib/optimizely/bucketer.rb index 0400f354..ba502833 100644 --- a/lib/optimizely/bucketer.rb +++ b/lib/optimizely/bucketer.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true # -# Copyright 2016-2017, 2019-2020 Optimizely and contributors +# Copyright 2016-2017, 2019-2021 Optimizely and contributors # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -88,7 +88,7 @@ def bucket(project_config, experiment, bucketing_id, user_id) decide_reasons.push(*find_bucket_reasons) if variation_id && variation_id != '' - variation = project_config.get_variation_from_id(experiment_key, variation_id) + variation = project_config.get_variation_from_id_by_experiment_id(experiment_id, variation_id) return variation, decide_reasons end diff --git a/lib/optimizely/config/datafile_project_config.rb b/lib/optimizely/config/datafile_project_config.rb index 0babb9e5..2a14a106 100644 --- a/lib/optimizely/config/datafile_project_config.rb +++ b/lib/optimizely/config/datafile_project_config.rb @@ -52,10 +52,12 @@ class DatafileProjectConfig < ProjectConfig attr_reader :feature_variable_key_map attr_reader :group_id_map attr_reader :rollout_id_map - attr_reader :rollout_experiment_key_map + attr_reader :rollout_experiment_id_map attr_reader :variation_id_map attr_reader :variation_id_to_variable_usage_map attr_reader :variation_key_map + attr_reader :variation_id_map_by_experiment_id + attr_reader :variation_key_map_by_experiment_id def initialize(datafile, logger, error_handler) # ProjectConfig init method to fetch and set project config data @@ -113,9 +115,11 @@ def initialize(datafile, logger, error_handler) @audience_id_map = @audience_id_map.merge(generate_key_map(@typed_audiences, 'id')) unless @typed_audiences.empty? @variation_id_map = {} @variation_key_map = {} + @variation_id_map_by_experiment_id = {} + @variation_key_map_by_experiment_id = {} @variation_id_to_variable_usage_map = {} @variation_id_to_experiment_map = {} - @experiment_key_map.each_value do |exp| + @experiment_id_map.each_value do |exp| # Excludes experiments from rollouts variations = exp.fetch('variations') variations.each do |variation| @@ -125,13 +129,13 @@ def initialize(datafile, logger, error_handler) end @rollout_id_map = generate_key_map(@rollouts, 'id') # split out the experiment key map for rollouts - @rollout_experiment_key_map = {} + @rollout_experiment_id_map = {} @rollout_id_map.each_value do |rollout| exps = rollout.fetch('experiments') - @rollout_experiment_key_map = @rollout_experiment_key_map.merge(generate_key_map(exps, 'key')) + @rollout_experiment_id_map = @rollout_experiment_id_map.merge(generate_key_map(exps, 'id')) end - @all_experiments = @experiment_key_map.merge(@rollout_experiment_key_map) - @all_experiments.each do |key, exp| + @all_experiments = @experiment_id_map.merge(@rollout_experiment_id_map) + @all_experiments.each do |id, exp| variations = exp.fetch('variations') variations.each do |variation| variation_id = variation['id'] @@ -141,8 +145,10 @@ def initialize(datafile, logger, error_handler) @variation_id_to_variable_usage_map[variation_id] = generate_key_map(variation_variables, 'id') end - @variation_id_map[key] = generate_key_map(variations, 'id') - @variation_key_map[key] = generate_key_map(variations, 'key') + @variation_id_map[exp['key']] = generate_key_map(variations, 'id') + @variation_key_map[exp['key']] = generate_key_map(variations, 'key') + @variation_id_map_by_experiment_id[id] = generate_key_map(variations, 'id') + @variation_key_map_by_experiment_id[id] = generate_key_map(variations, 'key') end @feature_flag_key_map = generate_key_map(@feature_flags, 'key') @experiment_feature_map = {} @@ -209,6 +215,21 @@ def get_experiment_from_key(experiment_key) nil end + def get_experiment_from_id(experiment_id) + # Retrieves experiment ID for a given key + # + # experiment_id - String id representing the experiment + # + # Returns Experiment or nil if not found + + experiment = @experiment_id_map[experiment_id] + return experiment if experiment + + @logger.log Logger::ERROR, "Experiment id '#{experiment_id}' is not in datafile." + @error_handler.handle_error InvalidExperimentError + nil + end + def get_experiment_key(experiment_id) # Retrieves experiment key for a given ID. # @@ -277,6 +298,52 @@ def get_variation_from_id(experiment_key, variation_id) nil end + def get_variation_from_id_by_experiment_id(experiment_id, variation_id) + # Get variation given experiment ID and variation ID + # + # experiment_id - ID representing parent experiment of variation + # variation_id - ID of the variation + # + # Returns the variation or nil if not found + + variation_id_map_by_experiment_id = @variation_id_map_by_experiment_id[experiment_id] + if variation_id_map_by_experiment_id + variation = variation_id_map_by_experiment_id[variation_id] + return variation if variation + + @logger.log Logger::ERROR, "Variation id '#{variation_id}' is not in datafile." + @error_handler.handle_error InvalidVariationError + return nil + end + + @logger.log Logger::ERROR, "Experiment id '#{experiment_id}' is not in datafile." + @error_handler.handle_error InvalidExperimentError + nil + end + + def get_variation_id_from_key_by_experiment_id(experiment_id, variation_key) + # Get variation given experiment ID and variation key + # + # experiment_id - ID representing parent experiment of variation + # variation_key - Key of the variation + # + # Returns the variation or nil if not found + + variation_key_map = @variation_key_map_by_experiment_id[experiment_id] + if variation_key_map + variation = variation_key_map[variation_key] + return variation['id'] if variation + + @logger.log Logger::ERROR, "Variation key '#{variation_key}' is not in datafile." + @error_handler.handle_error InvalidVariationError + return nil + end + + @logger.log Logger::ERROR, "Experiment id '#{experiment_id}' is not in datafile." + @error_handler.handle_error InvalidExperimentError + nil + end + def get_variation_id_from_key(experiment_key, variation_key) # Get variation ID given experiment key and variation key # @@ -300,17 +367,17 @@ def get_variation_id_from_key(experiment_key, variation_key) nil end - def get_whitelisted_variations(experiment_key) - # Retrieves whitelisted variations for a given experiment Key + def get_whitelisted_variations(experiment_id) + # Retrieves whitelisted variations for a given experiment id # - # experiment_key - String Key representing the experiment + # experiment_id - String id representing the experiment # # Returns whitelisted variations for the experiment or nil - experiment = @experiment_key_map[experiment_key] + experiment = @experiment_id_map[experiment_id] return experiment['forcedVariations'] if experiment - @logger.log Logger::ERROR, "Experiment key '#{experiment_key}' is not in datafile." + @logger.log Logger::ERROR, "Experiment ID '#{experiment_id}' is not in datafile." @error_handler.handle_error InvalidExperimentError end diff --git a/lib/optimizely/decision_service.rb b/lib/optimizely/decision_service.rb index ecb5c534..49df5e32 100644 --- a/lib/optimizely/decision_service.rb +++ b/lib/optimizely/decision_service.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true # -# Copyright 2017-2020, Optimizely and contributors +# Copyright 2017-2021, Optimizely and contributors # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -52,11 +52,11 @@ def initialize(logger, user_profile_service = nil) @forced_variation_map = {} end - def get_variation(project_config, experiment_key, user_id, attributes = nil, decide_options = []) + def get_variation(project_config, experiment_id, user_id, attributes = nil, decide_options = []) # Determines variation into which user will be bucketed. # # project_config - project_config - Instance of ProjectConfig - # experiment_key - Experiment for which visitor variation needs to be determined + # experiment_id - Experiment for which visitor variation needs to be determined # user_id - String ID for user # attributes - Hash representing user attributes # @@ -68,10 +68,10 @@ def get_variation(project_config, experiment_key, user_id, attributes = nil, dec 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) + experiment = project_config.get_experiment_from_id(experiment_id) return nil, decide_reasons if experiment.nil? - experiment_id = experiment['id'] + experiment_key = experiment['key'] unless project_config.experiment_running?(experiment) message = "Experiment '#{experiment_key}' is not running." @logger.log(Logger::INFO, message) @@ -80,12 +80,12 @@ 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, reasons_received = get_forced_variation(project_config, experiment_key, user_id) + forced_variation, reasons_received = get_forced_variation(project_config, experiment['key'], user_id) 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) + whitelisted_variation_id, reasons_received = get_whitelisted_variation_id(project_config, experiment_id, user_id) decide_reasons.push(*reasons_received) return whitelisted_variation_id, decide_reasons if whitelisted_variation_id @@ -122,7 +122,7 @@ def get_variation(project_config, experiment_key, user_id, attributes = nil, dec message = '' if variation_id variation_key = variation['key'] - message = "User '#{user_id}' is in variation '#{variation_key}' of experiment '#{experiment_key}'." + message = "User '#{user_id}' is in variation '#{variation_key}' of experiment '#{experiment_id}'." else message = "User '#{user_id}' is in no variation." end @@ -186,13 +186,13 @@ def get_variation_for_feature_experiment(project_config, feature_flag, user_id, return nil, decide_reasons end - experiment_key = experiment['key'] - variation_id, reasons_received = get_variation(project_config, experiment_key, user_id, attributes, decide_options) + experiment_id = experiment['id'] + variation_id, reasons_received = get_variation(project_config, experiment_id, user_id, attributes, decide_options) decide_reasons.push(*reasons_received) next unless variation_id - variation = project_config.variation_id_map[experiment_key][variation_id] + variation = project_config.get_variation_from_id_by_experiment_id(experiment_id, variation_id) return Decision.new(experiment, variation, DECISION_SOURCES['FEATURE_TEST']), decide_reasons end @@ -315,7 +315,7 @@ def set_forced_variation(project_config, experiment_key, user_id, variation_key) return true end - variation_id = project_config.get_variation_id_from_key(experiment_key, variation_key) + variation_id = project_config.get_variation_id_from_key_by_experiment_id(experiment_id, variation_key) # check if the variation exists in the datafile unless variation_id @@ -334,7 +334,7 @@ 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 - # experiment_key - String Key for experiment + # experiment_key - String key for experiment # user_id - String ID for user # # Returns Variation The variation which the given user and experiment should be forced into @@ -354,7 +354,7 @@ def get_forced_variation(project_config, experiment_key, user_id) 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." + message = "No experiment '#{experiment_id}' mapped to user '#{user_id}' in the forced variation map." @logger.log(Logger::DEBUG, message) decide_reasons.push(message) return nil, decide_reasons @@ -362,14 +362,14 @@ def get_forced_variation(project_config, experiment_key, user_id) variation_id = experiment_to_variation_map[experiment_id] variation_key = '' - variation = project_config.get_variation_from_id(experiment_key, variation_id) + variation = project_config.get_variation_from_id_by_experiment_id(experiment_id, variation_id) variation_key = variation['key'] if variation # check if the variation exists in the datafile # this case is logged in get_variation_from_id 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" + message = "Variation '#{variation_key}' is mapped to experiment '#{experiment_id}' and user '#{user_id}' in the forced variation map" @logger.log(Logger::DEBUG, message) decide_reasons.push(message) @@ -378,7 +378,7 @@ def get_forced_variation(project_config, experiment_key, user_id) private - def get_whitelisted_variation_id(project_config, experiment_key, user_id) + def get_whitelisted_variation_id(project_config, experiment_id, 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 @@ -387,7 +387,7 @@ def get_whitelisted_variation_id(project_config, experiment_key, user_id) # # Returns variation ID into which user_id is whitelisted (nil if no variation) - whitelisted_variations = project_config.get_whitelisted_variations(experiment_key) + whitelisted_variations = project_config.get_whitelisted_variations(experiment_id) return nil, nil unless whitelisted_variations @@ -395,7 +395,7 @@ def get_whitelisted_variation_id(project_config, experiment_key, user_id) return nil, nil unless whitelisted_variation_key - whitelisted_variation_id = project_config.get_variation_id_from_key(experiment_key, whitelisted_variation_key) + whitelisted_variation_id = project_config.get_variation_id_from_key_by_experiment_id(experiment_id, whitelisted_variation_key) unless whitelisted_variation_id message = "User '#{user_id}' is whitelisted into variation '#{whitelisted_variation_key}', which is not in the datafile." @@ -403,7 +403,7 @@ def get_whitelisted_variation_id(project_config, experiment_key, user_id) return nil, message end - message = "User '#{user_id}' is whitelisted into variation '#{whitelisted_variation_key}' of experiment '#{experiment_key}'." + message = "User '#{user_id}' is whitelisted into variation '#{whitelisted_variation_key}' of experiment '#{experiment_id}'." @logger.log(Logger::INFO, message) [whitelisted_variation_id, message] diff --git a/lib/optimizely/project_config.rb b/lib/optimizely/project_config.rb index 5a4e2115..af2d6df7 100644 --- a/lib/optimizely/project_config.rb +++ b/lib/optimizely/project_config.rb @@ -54,6 +54,8 @@ def experiment_running?(experiment); end def get_experiment_from_key(experiment_key); end + def get_experiment_from_id(experiment_id); end + def get_experiment_key(experiment_id); end def get_event_from_key(event_key); end @@ -62,9 +64,13 @@ def get_audience_from_id(audience_id); end def get_variation_from_id(experiment_key, variation_id); end + def get_variation_from_id_by_experiment_id(experiment_id, variation_id); end + + def get_variation_id_from_key_by_experiment_id(experiment_id, variation_key); end + def get_variation_id_from_key(experiment_key, variation_key); end - def get_whitelisted_variations(experiment_key); end + def get_whitelisted_variations(experiment_id); end def get_attribute_id(attribute_key); end diff --git a/spec/config/datafile_project_config_spec.rb b/spec/config/datafile_project_config_spec.rb index 43d7fdac..51cb8dc7 100644 --- a/spec/config/datafile_project_config_spec.rb +++ b/spec/config/datafile_project_config_spec.rb @@ -728,13 +728,13 @@ '166661' => config_body['rollouts'][1] } - expected_rollout_experiment_key_map = { + expected_rollout_experiment_id_map = { '177770' => config_body['rollouts'][0]['experiments'][0], '177772' => config_body['rollouts'][0]['experiments'][1], '177776' => config_body['rollouts'][0]['experiments'][2], '177774' => config_body['rollouts'][1]['experiments'][0], '177779' => config_body['rollouts'][1]['experiments'][1], - 'rollout_exp_with_diff_id_and_key' => config_body['rollouts'][1]['experiments'][2] + '177780' => config_body['rollouts'][1]['experiments'][2] } expect(project_config.attribute_key_map).to eq(expected_attribute_key_map) @@ -748,7 +748,7 @@ expect(project_config.variation_key_map).to eq(expected_variation_key_map) expect(project_config.variation_id_to_variable_usage_map).to eq(expected_variation_id_to_variable_usage_map) expect(project_config.rollout_id_map).to eq(expected_rollout_id_map) - expect(project_config.rollout_experiment_key_map).to eq(expected_rollout_experiment_key_map) + expect(project_config.rollout_experiment_id_map).to eq(expected_rollout_experiment_id_map) end it 'should initialize properties correctly upon creating project with typed audience dict' do @@ -840,19 +840,83 @@ end end + describe 'get_variation_from_id_by_experiment_id' do + it 'should log a message when provided experiment id is invalid' do + config.get_variation_from_id_by_experiment_id('invalid_id', 'some_variation') + expect(spy_logger).to have_received(:log).with(Logger::ERROR, + "Experiment id 'invalid_id' is not in datafile.") + end + it 'should return nil when provided variation id is invalid' do + expect(config.get_variation_from_id_by_experiment_id('111127', 'invalid_variation')).to eq(nil) + expect(spy_logger).to have_received(:log).with(Logger::ERROR, + "Variation id 'invalid_variation' is not in datafile.") + end + + it 'should return variation having featureEnabled false when not provided in the datafile' do + config_body = OptimizelySpec::VALID_CONFIG_BODY + experiment_id = config_body['experiments'][1]['id'] + variation_id = config_body['experiments'][1]['variations'][1]['id'] + + config_body['experiments'][1]['variations'][1][feature_enabled] = nil + + config_body_json = JSON.dump(config_body) + project_config = Optimizely::DatafileProjectConfig.new(config_body_json, logger, error_handler) + + expect(project_config.get_variation_from_id_by_experiment_id(experiment_id, variation_id)[feature_enabled]).to eq(false) + end + end + + describe 'get_variation_id_from_key_by_experiment_id' do + it 'should log a message when provided experiment id is invalid' do + config.get_variation_id_from_key_by_experiment_id('invalid_id', 'some_variation') + expect(spy_logger).to have_received(:log).with(Logger::ERROR, + "Experiment id 'invalid_id' is not in datafile.") + end + it 'should return nil when provided variation key is invalid' do + expect(config.get_variation_id_from_key_by_experiment_id('111127', 'invalid_variation')).to eq(nil) + expect(spy_logger).to have_received(:log).with(Logger::ERROR, + "Variation key 'invalid_variation' is not in datafile.") + end + + it 'should return variation having featureEnabled false when not provided in the datafile' do + config_body = OptimizelySpec::VALID_CONFIG_BODY + experiment_id = config_body['experiments'][1]['id'] + variation_key = config_body['experiments'][1]['variations'][1]['key'] + + config_body['experiments'][1]['variations'][1][feature_enabled] = nil + + config_body_json = JSON.dump(config_body) + project_config = Optimizely::DatafileProjectConfig.new(config_body_json, logger, error_handler) + expect(project_config.get_variation_id_from_key_by_experiment_id(experiment_id, variation_key)).to eq('100029') + end + end + describe 'get_variation_id_from_key' do + config_body = OptimizelySpec::VALID_CONFIG_BODY + experiment_key = config_body['experiments'][1]['key'] + variation_key = config_body['experiments'][1]['variations'][1]['key'] + variation_id = config_body['experiments'][1]['variations'][1]['id'] + it 'should log a message when there is no variation key map for the experiment' do config.get_variation_id_from_key('invalid_key', 'invalid_variation') expect(spy_logger).to have_received(:log).with(Logger::ERROR, "Experiment key 'invalid_key' is not in datafile.") end + it 'should log a message when there is invalid variation key for the experiment' do + expect(config.get_variation_id_from_key(experiment_key, 'invalid_variation')).to eq(nil) + expect(spy_logger).to have_received(:log).with(Logger::ERROR, + "Variation key 'invalid_variation' is not in datafile.") + end + it 'should return variation id for variation key and the experiment key' do + expect(config.get_variation_id_from_key(experiment_key, variation_key)).to eq(variation_id) + end end describe 'get_whitelisted_variations' do it 'should log a message when there is no experiment key map for the experiment' do config.get_whitelisted_variations('invalid_key') expect(spy_logger).to have_received(:log).with(Logger::ERROR, - "Experiment key 'invalid_key' is not in datafile.") + "Experiment ID 'invalid_key' is not in datafile.") end end diff --git a/spec/decision_service_spec.rb b/spec/decision_service_spec.rb index 2f149b06..c0ffb41b 100644 --- a/spec/decision_service_spec.rb +++ b/spec/decision_service_spec.rb @@ -42,9 +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, reasons = decision_service.get_variation(config, 'test_experiment', 'test_user') + variation_received, reasons = decision_service.get_variation(config, '111127', '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"]) + expect(reasons).to eq(["Variation 'variation' is mapped to experiment '111127' 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) @@ -57,9 +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, reasons = decision_service.get_variation(config, 'test_experiment_with_audience', 'test_user', user_attributes) + variation_received, reasons = decision_service.get_variation(config, '122227', '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"]) + expect(reasons).to eq(["Variation 'control_with_audience' is mapped to experiment '122227' 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) @@ -67,23 +67,23 @@ end 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') + variation_received, reasons = decision_service.get_variation(config, '111127', 'test_user') expect(variation_received).to eq('111128') expect(reasons).to eq([ "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", - "User 'test_user' is in variation 'control' of experiment 'test_experiment'." + "User 'test_user' is in variation 'control' of experiment '111127'." ]) expect(spy_logger).to have_received(:log) - .once.with(Logger::INFO, "User 'test_user' is in variation 'control' of experiment 'test_experiment'.") + .once.with(Logger::INFO, "User 'test_user' is in variation 'control' of experiment '111127'.") expect(decision_service).to have_received(:get_whitelisted_variation_id).once expect(decision_service.bucketer).to have_received(:bucket).once end it 'should return nil when user ID is not bucketed' do allow(decision_service.bucketer).to receive(:bucket).and_return(nil) - variation_received, reasons = decision_service.get_variation(config, 'test_experiment', 'test_user') + variation_received, reasons = decision_service.get_variation(config, '111127', 'test_user') expect(variation_received).to eq(nil) expect(reasons).to eq([ "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", @@ -95,21 +95,21 @@ end it 'should return correct variation ID if user ID is in whitelisted Variations and variation is valid' do - variation_received, reasons = decision_service.get_variation(config, 'test_experiment', 'forced_user1') + variation_received, reasons = decision_service.get_variation(config, '111127', 'forced_user1') expect(variation_received).to eq('111128') expect(reasons).to eq([ - "User 'forced_user1' is whitelisted into variation 'control' of experiment 'test_experiment'." + "User 'forced_user1' is whitelisted into variation 'control' of experiment '111127'." ]) expect(spy_logger).to have_received(:log) - .once.with(Logger::INFO, "User 'forced_user1' is whitelisted into variation 'control' of experiment 'test_experiment'.") + .once.with(Logger::INFO, "User 'forced_user1' is whitelisted into variation 'control' of experiment '111127'.") - variation_received, reasons = decision_service.get_variation(config, 'test_experiment', 'forced_user2') + variation_received, reasons = decision_service.get_variation(config, '111127', 'forced_user2') expect(variation_received).to eq('111129') expect(reasons).to eq([ - "User 'forced_user2' is whitelisted into variation 'variation' of experiment 'test_experiment'." + "User 'forced_user2' is whitelisted into variation 'variation' of experiment '111127'." ]) expect(spy_logger).to have_received(:log) - .once.with(Logger::INFO, "User 'forced_user2' is whitelisted into variation 'variation' of experiment 'test_experiment'.") + .once.with(Logger::INFO, "User 'forced_user2' is whitelisted into variation 'variation' of experiment '111127'.") # whitelisted variations should short circuit bucketing expect(decision_service.bucketer).not_to have_received(:bucket) @@ -123,21 +123,21 @@ Optimizely::Helpers::Constants::CONTROL_ATTRIBUTES['BUCKETING_ID'] => 'pid' } - variation_received, reasons = decision_service.get_variation(config, 'test_experiment', 'forced_user1', user_attributes) + variation_received, reasons = decision_service.get_variation(config, '111127', 'forced_user1', user_attributes) expect(variation_received).to eq('111128') expect(reasons).to eq([ - "User 'forced_user1' is whitelisted into variation 'control' of experiment 'test_experiment'." + "User 'forced_user1' is whitelisted into variation 'control' of experiment '111127'." ]) expect(spy_logger).to have_received(:log) - .once.with(Logger::INFO, "User 'forced_user1' is whitelisted into variation 'control' of experiment 'test_experiment'.") + .once.with(Logger::INFO, "User 'forced_user1' is whitelisted into variation 'control' of experiment '111127'.") - variation_received, reasons = decision_service.get_variation(config, 'test_experiment', 'forced_user2', user_attributes) + variation_received, reasons = decision_service.get_variation(config, '111127', 'forced_user2', user_attributes) expect(variation_received).to eq('111129') expect(reasons).to eq([ - "User 'forced_user2' is whitelisted into variation 'variation' of experiment 'test_experiment'." + "User 'forced_user2' is whitelisted into variation 'variation' of experiment '111127'." ]) expect(spy_logger).to have_received(:log) - .once.with(Logger::INFO, "User 'forced_user2' is whitelisted into variation 'variation' of experiment 'test_experiment'.") + .once.with(Logger::INFO, "User 'forced_user2' is whitelisted into variation 'variation' of experiment '111127'.") # whitelisted variations should short circuit bucketing expect(decision_service.bucketer).not_to have_received(:bucket) @@ -147,15 +147,15 @@ 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, reasons = decision_service.get_variation(config, 'test_experiment_with_audience', 'forced_audience_user', user_attributes) + variation_received, reasons = decision_service.get_variation(config, '122227', 'forced_audience_user', user_attributes) expect(variation_received).to eq('122229') expect(reasons).to eq([ - "User 'forced_audience_user' is whitelisted into variation 'variation_with_audience' of experiment 'test_experiment_with_audience'." + "User 'forced_audience_user' is whitelisted into variation 'variation_with_audience' of experiment '122227'." ]) expect(spy_logger).to have_received(:log) .once.with( Logger::INFO, - "User 'forced_audience_user' is whitelisted into variation 'variation_with_audience' of experiment 'test_experiment_with_audience'." + "User 'forced_audience_user' is whitelisted into variation 'variation_with_audience' of experiment '122227'." ) # forced variations should short circuit bucketing @@ -170,12 +170,12 @@ expect(reasons).to eq([]) expect(spy_logger).to have_received(:log) - .once.with(Logger::ERROR, "Experiment key 'totally_invalid_experiment' is not in datafile.") + .once.with(Logger::ERROR, "Experiment id 'totally_invalid_experiment' is not in datafile.") end 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, reasons = decision_service.get_variation(config, 'test_experiment_with_audience', 'test_user', user_attributes) + variation_received, reasons = decision_service.get_variation(config, '122227', 'test_user', user_attributes) expect(variation_received).to eq(nil) expect(reasons).to eq([ "Starting to evaluate audience '11154' with conditions: [\"and\", [\"or\", [\"or\", {\"name\": \"browser_type\", \"type\": \"custom_attribute\", \"value\": \"firefox\"}]]].", @@ -193,7 +193,7 @@ end it 'should return nil if the given experiment is not running' do - variation_received, reasons = decision_service.get_variation(config, 'test_experiment_not_started', 'test_user') + variation_received, reasons = decision_service.get_variation(config, '100027', '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) @@ -208,13 +208,13 @@ end it 'should respect forced variations within mutually exclusive grouped experiments' do - variation_received, reasons = decision_service.get_variation(config, 'group1_exp2', 'forced_group_user1') + variation_received, reasons = decision_service.get_variation(config, '133332', 'forced_group_user1') expect(variation_received).to eq('130004') expect(reasons).to eq([ - "User 'forced_group_user1' is whitelisted into variation 'g1_e2_v2' of experiment 'group1_exp2'." + "User 'forced_group_user1' is whitelisted into variation 'g1_e2_v2' of experiment '133332'." ]) 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'.") + .once.with(Logger::INFO, "User 'forced_group_user1' is whitelisted into variation 'g1_e2_v2' of experiment '133332'.") # forced variations should short circuit bucketing expect(decision_service.bucketer).not_to have_received(:bucket) @@ -223,12 +223,12 @@ end it 'should bucket normally if user is whitelisted into a forced variation that is not in the datafile' do - variation_received, reasons = decision_service.get_variation(config, 'test_experiment', 'forced_user_with_invalid_variation') + variation_received, reasons = decision_service.get_variation(config, '111127', 'forced_user_with_invalid_variation') expect(variation_received).to eq('111128') 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.", - "User 'forced_user_with_invalid_variation' is in variation 'control' of experiment 'test_experiment'." + "User 'forced_user_with_invalid_variation' is in variation 'control' of experiment '111127'." ]) expect(spy_logger).to have_received(:log) .once.with( @@ -253,11 +253,11 @@ } expect(spy_user_profile_service).to receive(:lookup).once.and_return(nil) - variation_received, reasons = decision_service.get_variation(config, 'test_experiment', 'test_user') + variation_received, reasons = decision_service.get_variation(config, '111127', 'test_user') expect(variation_received).to eq('111128') expect(reasons).to eq([ "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", - "User 'test_user' is in variation 'control' of experiment 'test_experiment'." + "User 'test_user' is in variation 'control' of experiment '111127'." ]) # bucketing should have occurred @@ -283,11 +283,11 @@ } expect(spy_user_profile_service).to receive(:lookup).once.and_return(nil) - variation_received, reasons = decision_service.get_variation(config, 'test_experiment', 'test_user', user_attributes) + variation_received, reasons = decision_service.get_variation(config, '111127', 'test_user', user_attributes) expect(variation_received).to eq('111129') expect(reasons).to eq([ "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", - "User 'test_user' is in variation 'variation' of experiment 'test_experiment'." + "User 'test_user' is in variation 'variation' of experiment '111127'." ]) # bucketing should have occurred @@ -310,7 +310,7 @@ expect(spy_user_profile_service).to receive(:lookup) .with('test_user').once.and_return(saved_user_profile) - variation_received, reasons = decision_service.get_variation(config, 'test_experiment', 'test_user') + variation_received, reasons = decision_service.get_variation(config, '111127', 'test_user') expect(variation_received).to eq('111129') expect(reasons).to eq([ "Returning previously activated variation ID 111129 of experiment 'test_experiment' for user 'test_user' from user profile." @@ -339,11 +339,11 @@ expect(spy_user_profile_service).to receive(:lookup) .once.with('test_user').and_return(saved_user_profile) - variation_received, reasons = decision_service.get_variation(config, 'test_experiment', 'test_user') + variation_received, reasons = decision_service.get_variation(config, '111127', 'test_user') expect(variation_received).to eq('111128') expect(reasons).to eq([ "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", - "User 'test_user' is in variation 'control' of experiment 'test_experiment'." + "User 'test_user' is in variation 'control' of experiment '111127'." ]) # bucketing should have occurred @@ -377,12 +377,12 @@ expect(spy_user_profile_service).to receive(:lookup) .once.with('test_user').and_return(saved_user_profile) - variation_received, reasons = decision_service.get_variation(config, 'test_experiment', 'test_user') + variation_received, reasons = decision_service.get_variation(config, '111127', 'test_user') expect(variation_received).to eq('111128') expect(reasons).to eq([ "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'." + "User 'test_user' is in variation 'control' of experiment '111127'." ]) # bucketing should have occurred @@ -403,12 +403,12 @@ 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, reasons = decision_service.get_variation(config, 'test_experiment', 'test_user') + variation_received, reasons = decision_service.get_variation(config, '111127', 'test_user') expect(variation_received).to eq('111128') 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.", - "User 'test_user' is in variation 'control' of experiment 'test_experiment'." + "User 'test_user' is in variation 'control' of experiment '111127'." ]) expect(spy_logger).to have_received(:log).once @@ -420,11 +420,11 @@ 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, reasons = decision_service.get_variation(config, 'test_experiment', 'test_user') + variation_received, reasons = decision_service.get_variation(config, '111127', 'test_user') expect(variation_received).to eq('111128') expect(reasons).to eq([ "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", - "User 'test_user' is in variation 'control' of experiment 'test_experiment'." + "User 'test_user' is in variation 'control' of experiment '111127'." ]) expect(spy_logger).to have_received(:log).once @@ -436,11 +436,11 @@ allow(spy_user_profile_service).to receive(:lookup) .with('test_user').once.and_return(nil) - variation_received, reasons = 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, '111127', 'test_user', nil, [Optimizely::Decide::OptimizelyDecideOption::IGNORE_USER_PROFILE_SERVICE]) expect(variation_received).to eq('111128') expect(reasons).to eq([ "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", - "User 'test_user' is in variation 'control' of experiment 'test_experiment'." + "User 'test_user' is in variation 'control' of experiment '111127'." ]) expect(decision_service.bucketer).to have_received(:bucket) @@ -453,11 +453,11 @@ allow(spy_user_profile_service).to receive(:lookup) .with('test_user').once.and_return(nil) - variation_received, reasons = decision_service.get_variation(config, 'test_experiment', 'test_user') + variation_received, reasons = decision_service.get_variation(config, '111127', 'test_user') expect(variation_received).to eq('111128') expect(reasons).to eq([ "Audiences for experiment 'test_experiment' collectively evaluated to TRUE.", - "User 'test_user' is in variation 'control' of experiment 'test_experiment'." + "User 'test_user' is in variation 'control' of experiment '111127'." ]) expect(decision_service.bucketer).to have_received(:bucket) @@ -505,7 +505,7 @@ # 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, []) + .with(config, multivariate_experiment['id'], 'user_1', user_attributes, []) .and_return([nil, nil]) end @@ -570,10 +570,10 @@ 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, []) + .with(config, mutex_exp['id'], user_id, user_attributes, []) .and_return([nil, nil]) allow(decision_service).to receive(:get_variation) - .with(config, mutex_exp2['key'], user_id, user_attributes, []) + .with(config, mutex_exp2['id'], user_id, user_attributes, []) .and_return([nil, nil]) end @@ -931,13 +931,13 @@ 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(reasons).to eq(["Variation 'control' is mapped to experiment '111127' 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, 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"]) + expect(reasons).to eq(["Variation 'variation' is mapped to experiment '111127' and user 'test_user' in the forced variation map"]) end # Set variation on multiple experiments for one user. @@ -946,13 +946,13 @@ 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(reasons).to eq(["Variation 'control' is mapped to experiment '111127' 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, 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"]) + expect(reasons).to eq(["Variation 'control_with_audience' is mapped to experiment '122227' and user 'test_user' in the forced variation map"]) end # Set variations for multiple users. @@ -961,13 +961,13 @@ 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(reasons).to eq(["Variation 'control' is mapped to experiment '111127' 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, 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"]) + expect(reasons).to eq(["Variation 'control' is mapped to experiment '111127' and user 'test_user_2' in the forced variation map"]) end end end From 07349494050d01f00cc09b0b54a76dca3a0a0f37 Mon Sep 17 00:00:00 2001 From: ozayr-zaviar <54209343+ozayr-zaviar@users.noreply.github.com> Date: Tue, 3 Aug 2021 00:18:51 +0500 Subject: [PATCH 2/2] chore: prepare for release 3.8.1 (#284) ## Summary prepare for release 3.8.1 ## Test plan All Full stack compatibility tests and unit tests pass. --- CHANGELOG.md | 6 ++++++ lib/optimizely/version.rb | 2 +- spec/project_spec.rb | 4 ++-- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1af9316f..1862cb50 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Optimizely Ruby SDK Changelog +## 3.8.1 +August 2nd, 2021 + +### Bug Fixes: +- Fixed duplicate experiment key issue with multiple feature flags. While trying to get variation from the variationKeyMap, it was unable to find because the latest experimentKey was overriding the previous one. [#282](https://github.com/optimizely/ruby-sdk/pull/282) + ## 3.8.0 February 16th, 2021 diff --git a/lib/optimizely/version.rb b/lib/optimizely/version.rb index 6e33fb6a..fcff3388 100644 --- a/lib/optimizely/version.rb +++ b/lib/optimizely/version.rb @@ -17,5 +17,5 @@ # module Optimizely CLIENT_ENGINE = 'ruby-sdk' - VERSION = '3.8.0' + VERSION = '3.8.1' end diff --git a/spec/project_spec.rb b/spec/project_spec.rb index 1a66cfdb..14bfb491 100644 --- a/spec/project_spec.rb +++ b/spec/project_spec.rb @@ -3625,7 +3625,7 @@ def callback(_args); end project_id: '111001', revision: '42', client_name: 'ruby-sdk', - client_version: '3.8.0', + client_version: '3.8.1', anonymize_ip: false, enrich_decisions: true, visitors: [{ @@ -3771,7 +3771,7 @@ def callback(_args); end project_id: '111001', revision: '42', client_name: 'ruby-sdk', - client_version: '3.8.0', + client_version: '3.8.1', anonymize_ip: false, enrich_decisions: true, visitors: [{