Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/optimizely/bucketer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
60 changes: 56 additions & 4 deletions lib/optimizely/config/datafile_project_config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ class DatafileProjectConfig < ProjectConfig
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
Expand Down Expand Up @@ -117,6 +119,8 @@ 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|

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't do it using experiment_map, do it using experiment_id

Expand All @@ -132,9 +136,9 @@ def initialize(datafile, logger, error_handler)
@rollout_experiment_key_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_key_map = @rollout_experiment_key_map.merge(generate_key_map(exps, 'id'))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add all of your logics based on experiment_ids, not on experiment_keys.

end
@all_experiments = @experiment_key_map.merge(@rollout_experiment_key_map)
@all_experiments = @experiment_id_map.merge(@rollout_experiment_key_map)
@all_experiments.each do |key, exp|
variations = exp.fetch('variations')
variations.each do |variation|
Expand All @@ -145,8 +149,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[key] = generate_key_map(variations, 'id')
@variation_key_map_by_experiment_id[key] = generate_key_map(variations, 'key')
end
@feature_flag_key_map = generate_key_map(@feature_flags, 'key')
@experiment_feature_map = {}
Expand Down Expand Up @@ -281,6 +287,52 @@ def get_variation_from_id(experiment_key, variation_id)
nil
end

def get_variation_from_id_by_experiment_id(experiment_id, variation_id)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can variation ids duplicate across experiments? why do we need experiment_id if we have variation_id. shouldn't variation_id be enought to uniquely identify and fetch a variation?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes as far as I know variation Ids are unique but I was following this method def get_variation_from_id(experiment_key, 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_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_by_experiment_id = @variation_key_map_by_experiment_id[experiment_id]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
variation_key_map_by_experiment_id = @variation_key_map_by_experiment_id[experiment_id]
variation_key_map = @variation_key_map_by_experiment_id[experiment_id]

same name as the map will be confusing

if variation_key_map_by_experiment_id
variation = variation_key_map_by_experiment_id[variation_key]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
variation = variation_key_map_by_experiment_id[variation_key]
variation = variation_key_map[variation_key]

return variation 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
#
Expand Down
4 changes: 4 additions & 0 deletions lib/optimizely/project_config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,10 @@ 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_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
Expand Down
2 changes: 1 addition & 1 deletion spec/config/datafile_project_config_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -736,7 +736,7 @@
'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]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this change is needed?

}

expect(project_config.attribute_key_map).to eq(expected_attribute_key_map)
Expand Down