Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(OptimizelyConfig): Add new fields to OptimizelyConfig #285

Merged
merged 21 commits into from
Aug 12, 2021
Merged
Show file tree
Hide file tree
Changes from 10 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: 2 additions & 0 deletions lib/optimizely/condition_tree_evaluator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ module ConditionTreeEvaluator
NOT_CONDITION => :not_evaluator
}.freeze

OPERATORS = [AND_CONDITION, OR_CONDITION, NOT_CONDITION].freeze

module_function

def evaluate(conditions, leaf_evaluator)
Expand Down
4 changes: 2 additions & 2 deletions lib/optimizely/config/datafile_project_config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,8 @@ def initialize(datafile, logger, error_handler)
@anonymize_ip = config.key?('anonymizeIP') ? config['anonymizeIP'] : false
@bot_filtering = config['botFiltering']
@revision = config['revision']
@sdk_key = config.fetch('sdkKey', nil)
@environment_key = config.fetch('environmentKey', nil)
@sdk_key = config.fetch('sdkKey', '')
@environment_key = config.fetch('environmentKey', '')
@rollouts = config.fetch('rollouts', [])
@send_flag_decisions = config.fetch('sendFlagDecisions', false)

Expand Down
204 changes: 179 additions & 25 deletions lib/optimizely/optimizely_config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,56 +16,111 @@
#

module Optimizely
require 'json'
class OptimizelyConfig
include Optimizely::ConditionTreeEvaluator
def initialize(project_config)
@project_config = project_config
@rollouts = @project_config.rollouts
@audiences = []
type_audiences = @project_config.typed_audiences
optly_typed_audiences = []
Copy link
Contributor

@zashraf1985 zashraf1985 Aug 5, 2021

Choose a reason for hiding this comment

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

whats the difference between type_audiences and optly_typed_audiences. It should be clear from the variable names that why there are two arrays being used. Actually you dont even need to use type_audiences one. You can use #project_config.typed_audiences directly to loop below. optly_typed_audiences does not sound consistent with others as no other variable are prefixed with optly. You should use typed_audiences to hold temp audiences and directly reference @project_config.typed_audiences to loop through the original array.

id_lookup_dict = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume it holds audiences only, if yes, the name should reflect that.


type_audiences.each do |type_audience|
optly_typed_audiences.push(
'id' => type_audience['id'],
'name' => type_audience['name'],
'conditions' => type_audience['conditions'].to_json
)
id_lookup_dict[type_audience['id']] = type_audience['id']
end

@project_config.audiences.each do |old_audience|
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: Why name it old_audience. why cant you just call it audience?

next unless !id_lookup_dict.key?(old_audience['id']) && (old_audience['id'] != '$opt_dummy_audience')

optly_typed_audiences.push(
'id' => old_audience['id'],
'name' => old_audience['name'],
'conditions' => old_audience['conditions']
)
end

@audiences = optly_typed_audiences
end

def config
experiments_map_object = experiments_map
features_map = get_features_map(experiments_map_object)
features_map = get_features_map(experiments_id_map)
config = {
'sdkKey' => @project_config.sdk_key,
'datafile' => @project_config.datafile,
'experimentsMap' => experiments_map_object,
Copy link
Contributor

@jaeopt jaeopt Aug 11, 2021

Choose a reason for hiding this comment

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

Can we add a note about key-conflict issue on top of 'experimentsMap':

// This experimentsMap is for experiments of legacy projects only.
// For flag projects, experiment keys are not guaranteed to be unique
// across multiple flags, so this map may not include all experiments
// when keys conflict.

Copy link
Contributor

Choose a reason for hiding this comment

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

Need a period at the end of the first sentence - "This experimentsMap is for experiments of legacy projects only"

'featuresMap' => features_map,
'revision' => @project_config.revision
'revision' => @project_config.revision,
'attributes' => get_attributes_list(@project_config.attributes),
'audiences' => @audiences,
'events' => get_events_list(@project_config.events),
'environmentKey' => @project_config.environment_key
}
config['sdkKey'] = @project_config.sdk_key if @project_config.sdk_key
config['environmentKey'] = @project_config.environment_key if @project_config.environment_key
config
end

private

def experiments_map
feature_variables_map = @project_config.feature_flags.reduce({}) do |result_map, feature|
result_map.update(feature['id'] => feature['variables'])
end
def experiments_id_map
feature_variables_map = feature_variable_map
audiences_id_map = audiences_map
@project_config.experiments.reduce({}) do |experiments_map, experiment|
feature_id = @project_config.experiment_feature_map.fetch(experiment['id'], []).first
experiments_map.update(
experiment['key'] => {
experiment['id'] => {
'id' => experiment['id'],
'key' => experiment['key'],
'variationsMap' => experiment['variations'].reduce({}) do |variations_map, variation|
variation_object = {
'id' => variation['id'],
'key' => variation['key'],
'variablesMap' => get_merged_variables_map(variation, experiment['id'], feature_variables_map)
}
variation_object['featureEnabled'] = variation['featureEnabled'] if @project_config.feature_experiment?(experiment['id'])
variations_map.update(variation['key'] => variation_object)
end
'variationsMap' => get_variation_map(feature_id, experiment, feature_variables_map),
'audiences' => replace_ids_with_names(experiment.fetch('audienceConditions', []), audiences_id_map) || ''
}
)
end
end

def audiences_map
@audiences.reduce({}) do |audiences_map, optly_audience|
audiences_map.update(optly_audience['id'] => optly_audience['name'])
end
end

def experiments_map
experiments_key_map = {}
experiments_id_map.each do |_, experiment|
experiments_key_map[experiment['key']] = experiment
end
experiments_key_map
Copy link
Contributor

@zashraf1985 zashraf1985 Aug 5, 2021

Choose a reason for hiding this comment

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

You could probably use reduce here?

end

def feature_variable_map
@project_config.feature_flags.reduce({}) do |result_map, feature|
result_map.update(feature['id'] => feature['variables'])
end
end

def get_variation_map(feature_id, experiment, feature_variables_map)
experiment['variations'].reduce({}) do |variations_map, variation|
variation_object = {
'id' => variation['id'],
'key' => variation['key'],
'variablesMap' => get_merged_variables_map(variation, feature_id, feature_variables_map)
}
variation_object['featureEnabled'] = variation['featureEnabled'] if @project_config.feature_experiment?(experiment['id'])
variations_map.update(variation['key'] => variation_object)
end
end

# Merges feature key and type from feature variables to variation variables.
def get_merged_variables_map(variation, experiment_id, feature_variables_map)
feature_ids = @project_config.experiment_feature_map[experiment_id]
return {} unless feature_ids
def get_merged_variables_map(variation, feature_id, feature_variables_map)
return {} unless feature_id

experiment_feature_variables = feature_variables_map[feature_ids[0]]
feature_variables = feature_variables_map[feature_id]
# temporary variation variables map to get values to merge.
temp_variables_id_map = {}
if variation['variables']
Expand All @@ -78,7 +133,7 @@ def get_merged_variables_map(variation, experiment_id, feature_variables_map)
)
end
end
experiment_feature_variables.reduce({}) do |variables_map, feature_variable|
feature_variables.reduce({}) do |variables_map, feature_variable|
variation_variable = temp_variables_id_map[feature_variable['id']]
variable_value = variation['featureEnabled'] && variation_variable ? variation_variable['value'] : feature_variable['defaultValue']
variables_map.update(
Expand All @@ -94,13 +149,14 @@ def get_merged_variables_map(variation, experiment_id, feature_variables_map)

def get_features_map(all_experiments_map)
@project_config.feature_flags.reduce({}) do |features_map, feature|
delivery_rules = get_delivery_rules(@rollouts, feature['rolloutId'], feature['id'])
features_map.update(
feature['key'] => {
'id' => feature['id'],
'key' => feature['key'],
'experimentsMap' => feature['experimentIds'].reduce({}) do |experiments_map, experiment_id|
experiment_key = @project_config.experiment_id_map[experiment_id]['key']
experiments_map.update(experiment_key => all_experiments_map[experiment_key])
experiments_map.update(experiment_key => experiments_id_map[experiment_id])
jaeopt marked this conversation as resolved.
Show resolved Hide resolved
end,
'variablesMap' => feature['variables'].reduce({}) do |variables, variable|
variables.update(
Expand All @@ -111,10 +167,108 @@ def get_features_map(all_experiments_map)
'value' => variable['defaultValue']
}
)
end
end,
'experimentRules' => feature['experimentIds'].reduce([]) do |experiments_map, experiment_id|
Copy link
Contributor

Choose a reason for hiding this comment

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

use map here. you are transforming array into an array here, not reducing to a value or object.

experiments_map.push(all_experiments_map[experiment_id])
end,
'deliveryRules' => delivery_rules
}
)
end
end

def get_attributes_list(attributes)
attributes.reduce([]) do |attributes_list, attribute|
Copy link
Contributor

Choose a reason for hiding this comment

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

map makes more sense here as you are transforming an array to another array

attributes_list.push(
'id' => attribute['id'],
'key' => attribute['key']
)
end
end

def get_events_list(events)
events.reduce([]) do |events_list, event|
Copy link
Contributor

Choose a reason for hiding this comment

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

use map here instead of reduce

events_list.push(
'id' => event['id'],
'key' => event['key'],
'experimentIds' => event['experimentIds']
)
end
end

def lookup_name_from_id(audience_id, audiences_map)
name = audiences_map[audience_id] || audience_id
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you check if only audiences_map[audience_id] || audience_id works?

name
end

def stringify_conditions(conditions, audiences_map)
Copy link
Contributor

Choose a reason for hiding this comment

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

This code looks complex. It will be great if you can add detailed comments inside the method with each branch condition.

operand = 'OR'
conditions_str = ''
length = conditions.length()
return '' if length.zero?
return '"' + lookup_name_from_id(conditions[0], audiences_map) + '"' if length == 1 && !OPERATORS.include?(conditions[0])

if length == 2 && OPERATORS.include?(conditions[0]) && !conditions[1].is_a?(Array) && !OPERATORS.include?(conditions[1])
return '"' + lookup_name_from_id(conditions[1], audiences_map) + '"' if conditions[0] != 'not'

return conditions[0].upcase + ' "' + lookup_name_from_id(conditions[1], audiences_map) + '"'

end
if length > 1
(0..length - 1).each do |n|
if OPERATORS.include?(conditions[n])
operand = conditions[n].upcase
elsif conditions[n].is_a?(Array)
conditions_str += if n + 1 < length
'(' + stringify_conditions(conditions[n], audiences_map) + ') '
else
operand + ' (' + stringify_conditions(conditions[n], audiences_map) + ')'
end
else
audience_name = lookup_name_from_id(conditions[n], audiences_map)
unless audience_name.nil?
conditions_str += if n + 1 < length - 1
'"' + audience_name + '" ' + operand + ' '
elsif n + 1 == length
operand + ' "' + audience_name + '"'
else
'"' + audience_name + '" '
end
end
end
end
end
conditions_str || ''
end

def replace_ids_with_names(conditions, audiences_map)
if !conditions.empty?
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT! Could use ternary operator here?

stringify_conditions(conditions, audiences_map)
else
''
end
end

def get_delivery_rules(rollouts, rollout_id, feature_id)
delivery_rules = []
audiences_id_map = audiences_map
feature_variables_map = feature_variable_map
rollout = rollouts.select { |selected_rollout| selected_rollout['id'] == rollout_id }
if rollout.any?
rollout = rollout[0]
experiments = rollout['experiments']
experiments.each do |experiment|
optly_exp = {
'id' => experiment['id'],
'key' => experiment['key'],
'variationsMap' => get_variation_map(feature_id, experiment, feature_variables_map),
'audiences' => replace_ids_with_names(experiment.fetch('audienceConditions', []), audiences_id_map) || ''
}
delivery_rules.push(optly_exp)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

could use experiment.map and directly assign to delivery_rules


end
delivery_rules
end
end
end
Loading