fix(audience evaluation): evaluate unknown audience id to null and continue #153
Conversation
|
Verified that this fixes the breaking audience match type tests that are enabled in: Passing test run: |
nchilada
left a comment
nchilada
left a comment
There was a problem hiding this comment.
Thanks for fixing this!
| ) | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
This is a pretty minimalist test case, but I think it's good enough. @aliabbasrizvi thoughts?
There was a problem hiding this comment.
This test is fine, but no way do I know that iPhone San Francisco users are in audience 7718080042.
May be add more comments.
There was a problem hiding this comment.
Ah, yes, we could make it clearer that the experiment activates because the user passes the conditions for the known audience
| $this->_errorHandler->handleError(new InvalidAudienceException('Provided audience is not in datafile.')); | ||
| return new Audience(); | ||
|
|
||
| return null; |
There was a problem hiding this comment.
I think I will keep the previous behavior.
| $conditionTreeEvaluator = new ConditionTreeEvaluator(); | ||
|
|
||
| $audience = $config->getAudience($audienceId); | ||
| if ($audience === null) { |
There was a problem hiding this comment.
I think I have a preference to keep previous behavior and check if $audience->getId equals null.
There was a problem hiding this comment.
Why do you prefer it this way? I don't see any advantage to returning empty entity objects. Instead, they have caused issues. #107
Thus we have replaced returning empty entity by null at multiple places.
There was a problem hiding this comment.
Instead, they have caused issues. #107
I see your point. Like I said it is a preference and I am not married to the idea.
Regarding that issue, I would say that it was a bug and the implementer should have done due diligence.
| ) | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
This test is fine, but no way do I know that iPhone San Francisco users are in audience 7718080042.
May be add more comments.
| $conditionTreeEvaluator = new ConditionTreeEvaluator(); | ||
|
|
||
| $audience = $config->getAudience($audienceId); | ||
| if ($audience === null) { |
There was a problem hiding this comment.
Instead, they have caused issues. #107
I see your point. Like I said it is a preference and I am not married to the idea.
Regarding that issue, I would say that it was a bug and the implementer should have done due diligence.
Summary
In Audience Evaluation, evaluate unknown audience to null and continue evaluation for other audiences.
Though we trust the datafile generation part won't generate such scenario, we still check to sync up with compat suite test here:
https://github.com/optimizely/fullstack-sdk-compatibility-suite/pull/196
Test plan
Ran compatibility suite including audience combination tests.
Issues