-
Notifications
You must be signed in to change notification settings - Fork 0
feat (audience match types): Update condition evaluator for new audience match types. #55
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
base: rashid/audience-match-type
Are you sure you want to change the base?
feat (audience match types): Update condition evaluator for new audience match types. #55
Conversation
Pull Request Test Coverage Report for Build 539
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor feedback. Nice work.
# limitations under the License. | ||
# | ||
module Optimizely | ||
class ConditionTreeEvaluator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this a module as it doesn't have state and it's methods can be directly namespaced without creating an object.
class ConditionTreeEvaluator | ||
CUSTOM_ATTRIBUTE_CONDITION_TYPE = 'custom_attribute' | ||
|
||
# Default operator types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit. Remove default
|
||
if conditions.is_a? Array | ||
# Operator to apply is not explicit - assume 'or' | ||
first_operator = EVALUATORS_BY_OPERATOR_TYPE.include?(conditions[0]) ? conditions[0] : OR_CONDITION |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we group both under a single check of EVALUATORS_BY_OPERATOR_TYPE.include?(conditions[0])
# | ||
module Optimizely | ||
class ConditionTreeEvaluator | ||
CUSTOM_ATTRIBUTE_CONDITION_TYPE = 'custom_attribute' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit. is this used within this module?
|
||
condition_match = leaf_condition['match'] | ||
|
||
return nil if !condition_match.nil? && !EVALUATORS_BY_MATCH_TYPE.include?(condition_match) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can avoid checking !condition_match.nil? is you keep the below condition_match = EXACT_MATCH_TYPE if condition_match.nil?
before this
end | ||
|
||
it 'should return nil if there is no user-provided value' do | ||
condition_evaluator = Optimizely::CustomAttributeConditionEvaluator.new('sum' => {}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same. Do not provide anything is attributes. Do this for all others
end | ||
end | ||
|
||
describe 'with a number condition value' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add all the testcases for float as well where the exact_number_conditions has a float value.
end | ||
|
||
it 'should return false if the user-provided value is not greater than the condition value' do | ||
condition_evaluator = Optimizely::CustomAttributeConditionEvaluator.new('input_value' => 8) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
try using 10
end | ||
|
||
it 'should return nil if user-provided value is greater than specified limit' do | ||
condition_evaluator = Optimizely::CustomAttributeConditionEvaluator.new('input_value' => 2.0e+53) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's put this testcase in validator.
|
||
describe 'less than match type' do | ||
before(:context) do | ||
@lt_conditions = {'match' => 'lt', 'name' => 'input_value', 'type' => 'custom_attribute', 'value' => 10} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both for gt and lt add same testcases where condition value is a float. See Python PR optimizely#146
Summary
This adds support for new audience match type conditions to the condition evaluator:
exact, exists, gt, lt, and substring conditions
Abort and return null when appropriate in leaf evaluators
Null handling in and/or/not evaluators
Assume implicit "or" when operator is missing