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

fix(audienceeval): Multiple fixes #355

Merged
merged 10 commits into from
Jan 16, 2019

Conversation

yasirfolio3
Copy link
Contributor

@yasirfolio3 yasirfolio3 commented Jan 10, 2019

  1. For JSON parsing of Leaf condition, mark property 'type' as optional. Return null if leaf condition is evaluated with property 'type' null.
  2. Fixed Audience Conditions Deserialization for empty array.
  3. Fixed Audience Conditions Deserialization for a condition when leaf contains both audience id and a conditions array.
  4. Fixed cases where Audience should be evaluated as null and was instead returning false.
  5. Implemented test-cases for Audience Evaluation to fix issues detected on full-stack.

@coveralls
Copy link

Coverage Status

Coverage increased (+11.6%) to 92.092% when pulling e11377e on yasir/typefix-for-leafcondition into db72170 on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+11.6%) to 92.092% when pulling e11377e on yasir/typefix-for-leafcondition into db72170 on master.

@coveralls
Copy link

coveralls commented Jan 10, 2019

Coverage Status

Coverage increased (+0.03%) to 97.934% when pulling aa70be9 on yasir/typefix-for-leafcondition into 092aa13 on master.

@msohailhussain msohailhussain requested a review from a team January 10, 2019 18:20
Copy link
Contributor

@msohailhussain msohailhussain left a comment

Choose a reason for hiding this comment

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

lgtm

…le audience Ids and no Evaluation operator.
…here leaf node can contain further conditions array.

2. Fixed cases where Audience should be evaluated as null and is transformed to false in the final state.
3. Implemented test-cases for Audience Evaluation to fix issues detected on full-stack.
@yasirfolio3 yasirfolio3 changed the title feat(Audience Evaluation): JSON fix for null type in Leaf condition. feat(Audience Evaluation): JSON Serialization fix for Audience Conditions. Jan 11, 2019
@msohailhussain msohailhussain changed the title feat(Audience Evaluation): JSON Serialization fix for Audience Conditions. fix(audienceeval): Multiple fixes Jan 11, 2019
@msohailhussain
Copy link
Contributor

@thomaszurkan-optimizely we added some more fixes.

Copy link
Contributor

@thomaszurkan-optimizely thomaszurkan-optimizely left a comment

Choose a reason for hiding this comment

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

Please add a unit test that tests for the not condition on Experiment.AudienceConditions where the user passes in nil or empty attributes. In the last round of testing this caused the condition to pass. It should have been nil and therefor false.

@yasirfolio3
Copy link
Contributor Author

Test-cases added for null or empty user attributes in Not condition, please review.

NSArray *notConditionArray = @[@"not", @"3988293898"];
NSArray *conditions = [OPTLYCondition deserializeAudienceConditionsJSONArray:notConditionArray];
OPTLYNotCondition *notCondition = (OPTLYNotCondition *)[conditions firstObject];
XCTAssertNil([notCondition evaluateConditionsWithAttributes:userAttributes projectConfig:self.optimizelyTypedAudience.config]);
Copy link
Contributor

Choose a reason for hiding this comment

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

How does this evaluate if there are no attributes (passing in nil for attributes)? I called activate with nil attributes on a experiment with a audience combination negated. So, match [not "brower_type" has substring "chrome"] and it passed. But, that was before these changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason for this was that Evaluators for Experiment as well as Audiences were returning false if evaluation was null, which was then converted to true by the 'NOT' operator. Now all evaluators can return null aswell, and so when not operator recieves null as evaluation result, it returns null too. See change:

https://github.com/optimizely/objective-c-sdk/pull/355/files#diff-14f1494cb8efebbed033865e5c25e335R58
https://github.com/optimizely/objective-c-sdk/pull/355/files#diff-8749178f3084f0c32b0ea09b5615e095R32
https://github.com/optimizely/objective-c-sdk/pull/355/files#diff-e39e60da2e49e74d868de2fc69c31377R44

I have updated the testcases for activate call with not conditions aswell, please review.

Copy link
Contributor

@thomaszurkan-optimizely thomaszurkan-optimizely left a comment

Choose a reason for hiding this comment

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

LGTM

@thomaszurkan-optimizely thomaszurkan-optimizely merged commit 7f47f68 into master Jan 16, 2019
@yasirfolio3 yasirfolio3 deleted the yasir/typefix-for-leafcondition branch January 17, 2019 05:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants