feat(audiences): Audience combinations #147
Conversation
rashidsp
requested review from
mikeproeng37 and
msohailhussain
and removed request for
mikeproeng37
lib/optimizely/audience.rb
Outdated
| @@ -33,25 +33,29 @@ def user_in_experiment?(config, experiment, attributes) | |||
| # | |||
| # Returns boolean representing if user satisfies audience conditions for any of the audiences or not. | |||
There was a problem hiding this comment.
nit. Update doc. 'any' isn't necessarily true anymore since now we have audiences ANDed as well.
lib/optimizely/audience.rb
Outdated
| custom_attribute_condition_evaluator = CustomAttributeConditionEvaluator.new(attributes) | ||
| return custom_attribute_condition_evaluator.evaluate(condition) | ||
| evaluate_custom_attr = lambda do |condition| | ||
| custom_attr_condition_evaluator = CustomAttributeConditionEvaluator.new(attributes) |
There was a problem hiding this comment.
Can you declare this once outside and reuse it here?
| audience = config.get_audience_from_id(audience_id) | ||
| return nil unless audience | ||
|
|
||
| audience_conditions = audience['conditions'] | ||
| audience_conditions = JSON.parse(audience_conditions) if audience_conditions.is_a?(String) |
There was a problem hiding this comment.
Can we parse this when generating audienceIdMap?
https://github.com/optimizely/ruby-sdk/blob/rashid/audience-match-type-project-config/lib/optimizely/project_config.rb#L106
There was a problem hiding this comment.
Then we will have to update parser tests; would be an irrelevant change.
Leaving it for separate PR.
spec/project_spec.rb
Outdated
| it 'should return nil when complex audience conditions do not match' do | ||
| user_attributes = {'house' => 'Hufflepuff', 'lasers' => 45.5} | ||
| variation_to_return = @project_typed_audience_instance.config.get_variation_from_id('audience_combinations_experiment', '1423767504') | ||
| allow(@project_typed_audience_instance.decision_service.bucketer).to receive(:bucket).and_return(variation_to_return) |
There was a problem hiding this comment.
nit. Can remove the return part
spec/project_spec.rb
Outdated
| }, { | ||
| entity_id: Optimizely::Helpers::Constants::CONTROL_ATTRIBUTES['BOT_FILTERING'], | ||
| key: Optimizely::Helpers::Constants::CONTROL_ATTRIBUTES['BOT_FILTERING'], | ||
| type: 'custom', value: false |
| expect(Optimizely::Audience.user_in_experiment?(@project_instance.config, | ||
| experiment, | ||
| nil)).to be false | ||
| expect(Optimizely::CustomAttributeConditionEvaluator).to have_received(:new).with({}).twice |
There was a problem hiding this comment.
nit. Add a comment that this asserts nil attributes default to {}
There was a problem hiding this comment.
Nice, thanks for updating the license headers as well. It is a bit confusing that we are merging into this branch: audience-match-type-project-config as I see project config as a part of audience-match-types. Anyway, we can follow up offline about which branch to run the integration tests on.
Summary
This adds support for audience combinations on experiments. If
experiment['audienceConditions']is present, it will be used as a condition tree where the leaf conditions are audience Ids.Test plan
Issues