Skip to content

feat (audiences): Audience combinations #175

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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Oct 19, 2018
Merged

feat (audiences): Audience combinations #175

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
066357b
Move custom attribute condition evaluation logic to its own module. Make
Oct 9, 2018
1c4e2dd
Update audience evaluator to handle complex audience conditions.
Oct 10, 2018
4be7e53
Fix doc comments in condition_evaluator
Oct 10, 2018
a861ea6
Rename files to better reflect purposes
Oct 15, 2018
e84a5e1
Tests for top-level APIs using experiments/rollouts that have complex…
Oct 16, 2018
f1dcb2d
Review fixes
Oct 17, 2018
e21e4a6
Test improvements from review
Oct 18, 2018
8a5d595
Remove 2016 from license headers for new files
Oct 18, 2018
34cc8ad
Add assertions that audienceEvaluator.evaluate was called in top-level
Oct 19, 2018
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
41 changes: 25 additions & 16 deletions packages/optimizely-sdk/lib/core/audience_evaluator/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,36 +13,45 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
var conditionEvaluator = require('../condition_evaluator');
var conditionTreeEvaluator = require('../condition_tree_evaluator');
var customAttributeConditionEvaluator = require('../custom_attribute_condition_evaluator');

module.exports = {
/**
* Determine if the given user attributes satisfy the given audience conditions
* @param {Object[]} audiences Audiences to match the user attributes against
* @param {Object[]} audiences.conditions Audience conditions to match the user attributes against
* @param {Object} [userAttributes] Hash representing user attributes which will be used in
* determining if the audience conditions are met. If not
* provided, defaults to an empty object.
* @return {Boolean} True if the user attributes match the given audience conditions
* @param {Array|String|null|undefined} audienceConditions Audience conditions to match the user attributes against - can be an array
* of audience IDs, a nested array of conditions, or a single leaf condition.
* Examples: ["5", "6"], ["and", ["or", "1", "2"], "3"], "1"
* @param {Object} audiencesById Object providing access to full audience objects for audience IDs
* contained in audienceConditions. Keys should be audience IDs, values
* should be full audience objects with conditions properties
* @param {Object} [userAttributes] User attributes which will be used in determining if audience conditions
* are met. If not provided, defaults to an empty object
* @return {Boolean} true if the user attributes match the given audience conditions, false
* otherwise
*/
evaluate: function(audiences, userAttributes) {
evaluate: function(audienceConditions, audiencesById, userAttributes) {
// if there are no audiences, return true because that means ALL users are included in the experiment
if (!audiences || audiences.length === 0) {
if (!audienceConditions || audienceConditions.length === 0) {
return true;
}

if (!userAttributes) {
userAttributes = {};
}

for (var i = 0; i < audiences.length; i++) {
var audience = audiences[i];
var conditions = audience.conditions;
if (conditionEvaluator.evaluate(conditions, userAttributes)) {
return true;
var evaluateConditionWithUserAttributes = function(condition) {
return customAttributeConditionEvaluator.evaluate(condition, userAttributes);
};

var evaluateAudience = function(audienceId) {
var audience = audiencesById[audienceId];
if (audience) {
return conditionTreeEvaluator.evaluate(audience.conditions, evaluateConditionWithUserAttributes);
}
}
return null;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nchilada @mikeng13 Any thoughts on returning null here? Maybe this should throw an error, and project config should reject a datafile as invalid if an experiment has invalid audience ids.

Copy link
Contributor

@nchilada nchilada Oct 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I believe returning null is correct here - this function is being called at activate time so it's probably too late to decide that the datafile is invalid and to stop handling API calls. That said, we could add an additional check for invalid audience references during initialization. If we do so, we reduce this return null to a mere sanity check.

In the design doc I'd written:

If an audience can't be found in [audiences or typedAudiences], the SDK may fail gracefully or spectacularly, during initialization or during experiment activation—whatever it was doing before.

but I don't mind if someone want to propose that we consistently check for invalid audience references during initialization. @mikeng13 @thomaszurkan-optimizely what do you think?

};
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: for symmetry with customAttributeConditionEvaluator.evaluate, I wonder if we should export this functionality as audienceEvaluator.evaluate, and hoist all the other code up to DecisionService.prototype.__checkIfUserIsInAudience?

🔭:architecture_👨‍🚀:🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting. This does feel cleaner. I could see the other code fitting into __checkIfUserIsInAudience. @mikeng13 thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With this approach, audience_evaluator ends up being pretty uninteresting, to the extent that it maybe shouldn't be a module.

Copy link
Contributor

Choose a reason for hiding this comment

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

Up to you. I was just thinking out loud, especially if there was interest in using the _.partial pattern as we do throughout client-js. Doesn't really make a difference


return false;
return conditionTreeEvaluator.evaluate(audienceConditions, evaluateAudience) || false;
},
};
128 changes: 111 additions & 17 deletions packages/optimizely-sdk/lib/core/audience_evaluator/index.tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@
*/
var audienceEvaluator = require('./');
var chai = require('chai');
var conditionTreeEvaluator = require('../condition_tree_evaluator');
var customAttributeConditionEvaluator = require('../custom_attribute_condition_evaluator');
var sinon = require('sinon');

var assert = chai.assert;

var chromeUserAudience = {
Expand All @@ -31,16 +35,29 @@ var iphoneUserAudience = {
type: 'custom_attribute',
}],
};
var conditionsPassingWithNoAttrs = ['not', {
match: 'exists',
name: 'input_value',
type: 'custom_attribute',
}];
var conditionsPassingWithNoAttrsAudience = {
conditions: conditionsPassingWithNoAttrs,
};
var audiencesById = {
0: chromeUserAudience,
1: iphoneUserAudience,
2: conditionsPassingWithNoAttrsAudience,
};

describe('lib/core/audience_evaluator', function() {
describe('APIs', function() {
describe('evaluate', function() {
it('should return true if there are no audiences', function() {
assert.isTrue(audienceEvaluator.evaluate([], {}));
assert.isTrue(audienceEvaluator.evaluate([], audiencesById, {}));
});

it('should return false if there are audiences but no attributes', function() {
assert.isFalse(audienceEvaluator.evaluate([chromeUserAudience], {}));
assert.isFalse(audienceEvaluator.evaluate(['0'], audiencesById, {}));
});

it('should return true if any of the audience conditions are met', function() {
Expand All @@ -57,9 +74,9 @@ describe('lib/core/audience_evaluator', function() {
'device_model': 'iphone',
};

assert.isTrue(audienceEvaluator.evaluate([chromeUserAudience, iphoneUserAudience], iphoneUsers));
assert.isTrue(audienceEvaluator.evaluate([chromeUserAudience, iphoneUserAudience], chromeUsers));
assert.isTrue(audienceEvaluator.evaluate([chromeUserAudience, iphoneUserAudience], iphoneChromeUsers));
assert.isTrue(audienceEvaluator.evaluate(['0', '1'], audiencesById, iphoneUsers));
assert.isTrue(audienceEvaluator.evaluate(['0', '1'], audiencesById, chromeUsers));
assert.isTrue(audienceEvaluator.evaluate(['0', '1'], audiencesById, iphoneChromeUsers));
});

it('should return false if none of the audience conditions are met', function() {
Expand All @@ -76,21 +93,98 @@ describe('lib/core/audience_evaluator', function() {
'device_model': 'nexus5',
};

assert.isFalse(audienceEvaluator.evaluate([chromeUserAudience, iphoneUserAudience], nexusUsers));
assert.isFalse(audienceEvaluator.evaluate([chromeUserAudience, iphoneUserAudience], safariUsers));
assert.isFalse(audienceEvaluator.evaluate([chromeUserAudience, iphoneUserAudience], nexusSafariUsers));
assert.isFalse(audienceEvaluator.evaluate(['0', '1'], audiencesById, nexusUsers));
assert.isFalse(audienceEvaluator.evaluate(['0', '1'], audiencesById, safariUsers));
assert.isFalse(audienceEvaluator.evaluate(['0', '1'], audiencesById, nexusSafariUsers));
});

it('should return true if no attributes are passed and the audience conditions evaluate to true in the absence of attributes', function() {
var conditionsPassingWithNoAttrs = ['not', {
match: 'exists',
name: 'input_value',
type: 'custom_attribute',
}];
var audience = {
conditions: conditionsPassingWithNoAttrs,
};
assert.isTrue(audienceEvaluator.evaluate([audience]));
assert.isTrue(audienceEvaluator.evaluate(['2'], audiencesById));
});

describe('complex audience conditions', function() {
it('should return true if any of the audiences in an "OR" condition pass', function() {
var result = audienceEvaluator.evaluate(
['or', '0', '1'],
audiencesById,
{ browser_type: 'chrome' }
);
assert.isTrue(result);
});

it('should return true if all of the audiences in an "AND" condition pass', function() {
var result = audienceEvaluator.evaluate(
['and', '0', '1'],
audiencesById,
{ browser_type: 'chrome', device_model: 'iphone' }
);
assert.isTrue(result);
});

it('should return true if the audience in a "NOT" condition does not pass', function() {
var result = audienceEvaluator.evaluate(
['not', '1'],
audiencesById,
{ device_model: 'android' }
);
assert.isTrue(result);
});

});

describe('integration with dependencies', function() {
var sandbox = sinon.sandbox.create();

beforeEach(function() {
sandbox.stub(conditionTreeEvaluator, 'evaluate');
sandbox.stub(customAttributeConditionEvaluator, 'evaluate');
});

afterEach(function() {
sandbox.restore();
});

it('returns true if conditionTreeEvaluator.evaluate returns true', function() {
conditionTreeEvaluator.evaluate.returns(true);
var result = audienceEvaluator.evaluate(
['or', '0', '1'],
audiencesById,
{ browser_type: 'chrome' }
);
assert.isTrue(result);
});

it('returns false if conditionTreeEvaluator.evaluate returns false', function() {
conditionTreeEvaluator.evaluate.returns(false);
var result = audienceEvaluator.evaluate(
['or', '0', '1'],
audiencesById,
{ browser_type: 'safari' }
);
assert.isFalse(result);
});

it('returns false if conditionTreeEvaluator.evaluate returns null', function() {
conditionTreeEvaluator.evaluate.returns(null);
var result = audienceEvaluator.evaluate(
['or', '0', '1'],
audiencesById,
{ state: 'California' }
);
assert.isFalse(result);
});

it('calls customAttributeConditionEvaluator.evaluate in the leaf evaluator for audience conditions', function() {
conditionTreeEvaluator.evaluate.callsFake(function(conditions, leafEvaluator) {
return leafEvaluator(conditions[1]);
});
customAttributeConditionEvaluator.evaluate.returns(false);
var userAttributes = { device_model: 'android' };
var result = audienceEvaluator.evaluate(['or', '1'], audiencesById, userAttributes);
sinon.assert.calledOnce(customAttributeConditionEvaluator.evaluate);
sinon.assert.calledWithExactly(customAttributeConditionEvaluator.evaluate, iphoneUserAudience.conditions[1], userAttributes);
assert.isFalse(result);
});
});
});
});
Expand Down
Loading