Skip to content

DO NOT REVIEW - SEE https://github.com/optimizely/javascript-sdk/pull/288 #216

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

Closed
wants to merge 14 commits into from
Closed

DO NOT REVIEW - SEE https://github.com/optimizely/javascript-sdk/pull/288 #216

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
0738b58
add the ability to provide custom condition evaluators
Feb 12, 2019
2ae49e1
rebase with latest audience editor changes
Feb 12, 2019
0112ff9
updates per code review
Feb 14, 2019
1ce7345
fix merge conflicts
lpappone May 30, 2019
930975c
fix lint command
lpappone Jun 5, 2019
db29de1
update name
lpappone Jun 5, 2019
7411b24
fix tests
lpappone Jun 6, 2019
1102191
noop to rebuild
lpappone Jun 6, 2019
05deac9
remove quotes
lpappone Jun 7, 2019
76d155e
add quotes back in
lpappone Jun 7, 2019
a023b08
regenerate packagelock.json
lpappone Jun 7, 2019
0586dd8
regenerate package-lock.json again
lpappone Jun 7, 2019
f97df78
regenerate another package-lock.json again
lpappone Jun 7, 2019
53f43a6
copy pasta
lpappone Jun 7, 2019
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
112 changes: 73 additions & 39 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 @@ -16,54 +16,88 @@
var conditionTreeEvaluator = require('../condition_tree_evaluator');
var customAttributeConditionEvaluator = require('../custom_attribute_condition_evaluator');
var enums = require('../../utils/enums');
var fns = require('../../utils/fns');
var sprintf = require('sprintf-js').sprintf;

var ERROR_MESSAGES = enums.ERROR_MESSAGES;
var LOG_LEVEL = enums.LOG_LEVEL;
var LOG_MESSAGES = enums.LOG_MESSAGES;
var MODULE_NAME = 'AUDIENCE_EVALUATOR';

module.exports = {
/**
* Determine if the given user attributes satisfy 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
* @param {Object} logger Logger instance.
* @return {Boolean} true if the user attributes match the given audience conditions, false
* otherwise
*/
evaluate: function(audienceConditions, audiencesById, userAttributes, logger) {
// if there are no audiences, return true because that means ALL users are included in the experiment
if (!audienceConditions || audienceConditions.length === 0) {
return true;
}

if (!userAttributes) {
userAttributes = {};
}
/**
* Construct an instance of AudienceEvaluator with a given logger and options
* @param {Logger} logger The Logger instance
* @param {Object=} __exploratoryConditionEvaluators A map of condition evaluators provided by the consumer. This enables matching
* condition types which are not supported natively by the SDK. Note that built in
* Optimizely evaluators cannot be overridden.
* @constructor
*/
function AudienceEvaluator(logger, __exploratoryConditionEvaluators) {
this.logger = logger;
this.typeToEvaluatorMap = fns.assignIn({}, __exploratoryConditionEvaluators, {
'custom_attribute': customAttributeConditionEvaluator
});
}

var evaluateConditionWithUserAttributes = function(condition) {
return customAttributeConditionEvaluator.evaluate(condition, userAttributes, logger);
};
/**
* Determine if the given user attributes satisfy 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
*/
AudienceEvaluator.prototype.evaluate = function(audienceConditions, audiencesById, userAttributes) {
// if there are no audiences, return true because that means ALL users are included in the experiment
if (!audienceConditions || audienceConditions.length === 0) {
return true;
}

var evaluateAudience = function(audienceId) {
var audience = audiencesById[audienceId];
if (audience) {
logger.log(LOG_LEVEL.DEBUG, sprintf(LOG_MESSAGES.EVALUATING_AUDIENCE, MODULE_NAME, audienceId, JSON.stringify(audience.conditions)));
var result = conditionTreeEvaluator.evaluate(audience.conditions, evaluateConditionWithUserAttributes);
var resultText = result === null ? 'UNKNOWN' : result.toString().toUpperCase();
logger.log(LOG_LEVEL.INFO, sprintf(LOG_MESSAGES.AUDIENCE_EVALUATION_RESULT, MODULE_NAME, audienceId, resultText));
return result;
}
if (!userAttributes) {
userAttributes = {};
}

return null;
};
var evaluateAudience = function(audienceId) {
var audience = audiencesById[audienceId];
if (audience) {
this.logger.log(LOG_LEVEL.DEBUG, sprintf(LOG_MESSAGES.EVALUATING_AUDIENCE, MODULE_NAME, audienceId, JSON.stringify(audience.conditions)));
var result = conditionTreeEvaluator.evaluate(audience.conditions, this.evaluateConditionWithUserAttributes.bind(this, userAttributes));
var resultText = result === null ? 'UNKNOWN' : result.toString().toUpperCase();
this.logger.log(LOG_LEVEL.INFO, sprintf(LOG_MESSAGES.AUDIENCE_EVALUATION_RESULT, MODULE_NAME, audienceId, resultText));
return result;
}

return conditionTreeEvaluator.evaluate(audienceConditions, evaluateAudience) || false;
},
return null;
}.bind(this);

return conditionTreeEvaluator.evaluate(audienceConditions, evaluateAudience) || false;
};

/**
* Wrapper around evaluator.evaluate that is passed to the conditionTreeEvaluator.
* Evaluates the condition provided given the user attributes if an evaluator has been defined for the condition type.
* @param {Object} userAttributes A map of user attributes.
* @param {Object} condition A single condition object to evaluate.
* @return {Boolean|null} true if the condition is satisfied, null if a matcher is not found.
*/
AudienceEvaluator.prototype.evaluateConditionWithUserAttributes = function(userAttributes, condition) {
var evaluator = this.typeToEvaluatorMap[condition.type];
if (!evaluator) {
this.logger.log(LOG_LEVEL.WARNING, sprintf(LOG_MESSAGES.UNKNOWN_CONDITION_TYPE, MODULE_NAME, JSON.stringify(condition)));
return null;
}
try {
return evaluator.evaluate(condition, userAttributes, this.logger);
Copy link
Contributor

Choose a reason for hiding this comment

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

if the evaluator we get is not the custom_attribute, we should log a debug message to let the user know that. Might be helpful when trying to debug. Maybe we can even just log which evaluator we ended using (by key)

} catch (err) {
this.logger.log(LOG_LEVEL.ERROR, sprintf(ERROR_MESSAGES.CONDITION_EVALUATOR_ERROR, MODULE_NAME, condition.type, err.message));
}
return null;
};

module.exports = AudienceEvaluator;
45 changes: 24 additions & 21 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 @@ -13,9 +13,8 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
var audienceEvaluator = require('./');
var AudienceEvaluator = require('./');
var chai = require('chai');
var sprintf = require('sprintf-js').sprintf;
var conditionTreeEvaluator = require('../condition_tree_evaluator');
var customAttributeConditionEvaluator = require('../custom_attribute_condition_evaluator');
var sinon = require('sinon');
Expand Down Expand Up @@ -53,10 +52,14 @@ var audiencesById = {
};

describe('lib/core/audience_evaluator', function() {
var audienceEvaluator;
var mockLogger = logger.createLogger({logLevel: LOG_LEVEL.INFO});
beforeEach(function() {
audienceEvaluator = new AudienceEvaluator(mockLogger);
});

describe('APIs', function() {
describe('evaluate', function() {
var mockLogger = logger.createLogger({logLevel: LOG_LEVEL.INFO});

beforeEach(function () {
sinon.stub(mockLogger, 'log');
});
Expand All @@ -66,11 +69,11 @@ describe('lib/core/audience_evaluator', function() {
});

it('should return true if there are no audiences', function() {
assert.isTrue(audienceEvaluator.evaluate([], audiencesById, {}, mockLogger));
assert.isTrue(audienceEvaluator.evaluate([], audiencesById, {}));
});

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

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

assert.isTrue(audienceEvaluator.evaluate(['0', '1'], audiencesById, iphoneUsers, mockLogger));
assert.isTrue(audienceEvaluator.evaluate(['0', '1'], audiencesById, chromeUsers, mockLogger));
assert.isTrue(audienceEvaluator.evaluate(['0', '1'], audiencesById, iphoneChromeUsers, mockLogger));
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 @@ -106,13 +109,13 @@ describe('lib/core/audience_evaluator', function() {
'device_model': 'nexus5',
};

assert.isFalse(audienceEvaluator.evaluate(['0', '1'], audiencesById, nexusUsers, mockLogger));
assert.isFalse(audienceEvaluator.evaluate(['0', '1'], audiencesById, safariUsers, mockLogger));
assert.isFalse(audienceEvaluator.evaluate(['0', '1'], audiencesById, nexusSafariUsers, mockLogger));
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() {
assert.isTrue(audienceEvaluator.evaluate(['2'], audiencesById, null, mockLogger));
assert.isTrue(audienceEvaluator.evaluate(['2'], audiencesById, null));
});

describe('complex audience conditions', function() {
Expand Down Expand Up @@ -199,9 +202,9 @@ describe('lib/core/audience_evaluator', function() {
});
customAttributeConditionEvaluator.evaluate.returns(false);
var userAttributes = { device_model: 'android' };
var result = audienceEvaluator.evaluate(['or', '1'], audiencesById, userAttributes, mockLogger);
var result = audienceEvaluator.evaluate(['or', '1'], audiencesById, userAttributes);
sinon.assert.calledOnce(customAttributeConditionEvaluator.evaluate);
sinon.assert.calledWithExactly(customAttributeConditionEvaluator.evaluate, iphoneUserAudience.conditions[1], userAttributes, mockLogger);
sinon.assert.calledWithExactly(customAttributeConditionEvaluator.evaluate, iphoneUserAudience.conditions[1], userAttributes);
assert.isFalse(result);
});
});
Expand All @@ -224,9 +227,9 @@ describe('lib/core/audience_evaluator', function() {
});
customAttributeConditionEvaluator.evaluate.returns(null);
var userAttributes = { device_model: 5.5 };
var result = audienceEvaluator.evaluate(['or', '1'], audiencesById, userAttributes, mockLogger);
var result = audienceEvaluator.evaluate(['or', '1'], audiencesById, userAttributes);
sinon.assert.calledOnce(customAttributeConditionEvaluator.evaluate);
sinon.assert.calledWithExactly(customAttributeConditionEvaluator.evaluate, iphoneUserAudience.conditions[1], userAttributes, mockLogger);
sinon.assert.calledWithExactly(customAttributeConditionEvaluator.evaluate, iphoneUserAudience.conditions[1], userAttributes);
assert.isFalse(result);
assert.strictEqual(2, mockLogger.log.callCount);
assert.strictEqual(mockLogger.log.args[0][1], 'AUDIENCE_EVALUATOR: Starting to evaluate audience "1" with conditions: ["and",{"name":"device_model","value":"iphone","type":"custom_attribute"}].');
Expand All @@ -239,9 +242,9 @@ describe('lib/core/audience_evaluator', function() {
});
customAttributeConditionEvaluator.evaluate.returns(true);
var userAttributes = { device_model: 'iphone' };
var result = audienceEvaluator.evaluate(['or', '1'], audiencesById, userAttributes, mockLogger);
var result = audienceEvaluator.evaluate(['or', '1'], audiencesById, userAttributes);
sinon.assert.calledOnce(customAttributeConditionEvaluator.evaluate);
sinon.assert.calledWithExactly(customAttributeConditionEvaluator.evaluate, iphoneUserAudience.conditions[1], userAttributes, mockLogger);
sinon.assert.calledWithExactly(customAttributeConditionEvaluator.evaluate, iphoneUserAudience.conditions[1], userAttributes);
assert.isTrue(result);
assert.strictEqual(2, mockLogger.log.callCount);
assert.strictEqual(mockLogger.log.args[0][1], 'AUDIENCE_EVALUATOR: Starting to evaluate audience "1" with conditions: ["and",{"name":"device_model","value":"iphone","type":"custom_attribute"}].');
Expand All @@ -254,9 +257,9 @@ describe('lib/core/audience_evaluator', function() {
});
customAttributeConditionEvaluator.evaluate.returns(false);
var userAttributes = { device_model: 'android' };
var result = audienceEvaluator.evaluate(['or', '1'], audiencesById, userAttributes, mockLogger);
var result = audienceEvaluator.evaluate(['or', '1'], audiencesById, userAttributes);
sinon.assert.calledOnce(customAttributeConditionEvaluator.evaluate);
sinon.assert.calledWithExactly(customAttributeConditionEvaluator.evaluate, iphoneUserAudience.conditions[1], userAttributes, mockLogger);
sinon.assert.calledWithExactly(customAttributeConditionEvaluator.evaluate, iphoneUserAudience.conditions[1], userAttributes);
assert.isFalse(result);
assert.strictEqual(2, mockLogger.log.callCount);
assert.strictEqual(mockLogger.log.args[0][1], 'AUDIENCE_EVALUATOR: Starting to evaluate audience "1" with conditions: ["and",{"name":"device_model","value":"iphone","type":"custom_attribute"}].');
Expand Down
1 change: 0 additions & 1 deletion packages/optimizely-sdk/lib/core/custom_attribute_condition_evaluator/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@ EVALUATORS_BY_MATCH_TYPE[SUBSTRING_MATCH_TYPE] = substringEvaluator;
*/
function evaluate(condition, userAttributes, logger) {
if (condition.type !== CUSTOM_ATTRIBUTE_CONDITION_TYPE) {
logger.log(LOG_LEVEL.WARNING, sprintf(LOG_MESSAGES.UNKNOWN_CONDITION_TYPE, MODULE_NAME, JSON.stringify(condition)));
Copy link

Choose a reason for hiding this comment

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

@jamesopti I see this log message now in AudienceEvaluator.prototype.evaluateConditionWithUserAttributes - what is the purpose of this evaluate function, now?

return null;
}

Expand Down
7 changes: 4 additions & 3 deletions packages/optimizely-sdk/lib/core/decision_service/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
* limitations under the License. *
***************************************************************************/

var audienceEvaluator = require('../audience_evaluator');
var AudienceEvaluator = require('../audience_evaluator');
var bucketer = require('../bucketer');
var enums = require('../../utils/enums');
var fns = require('../../utils/fns');
Expand Down Expand Up @@ -52,6 +52,7 @@ function DecisionService(options) {
this.configObj = options.configObj;
this.userProfileService = options.userProfileService || null;
this.logger = options.logger;
this.audienceEvaluator = new AudienceEvaluator(options.logger, options.__exploratoryConditionEvaluators);
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels like we are polluting our factory method haha but I see no other easy way to integrate into the current setup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean the constructor?

Yea in the current setup, all this state must be threaded through (config, logger instance, etc...)

Would be nice to have a global registration system by instance ID so that this kinda global state can be easily accessed.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is totally fine, by adding configuration to the AudienceEvaluator we must capture that state, so making this an instantiatable class makes sense.

As far as global state, things like audience evaluators should not be considered as global state. If some sort of global registry existed you'd still have to know the instance ID here to resolve the audience evaluators (which would have to be threaded through).

However things like the logger and errorHandler should be global and having to thread them through here is not cool (this is changing soon in another PR)

}

/**
Expand Down Expand Up @@ -168,9 +169,9 @@ DecisionService.prototype.__checkIfUserIsInAudience = function(experimentKey, us
var experimentAudienceConditions = projectConfig.getExperimentAudienceConditions(this.configObj, experimentKey);
var audiencesById = projectConfig.getAudiencesById(this.configObj);
this.logger.log(LOG_LEVEL.DEBUG, sprintf(LOG_MESSAGES.EVALUATING_AUDIENCES_COMBINED, MODULE_NAME, experimentKey, JSON.stringify(experimentAudienceConditions)));
var result = audienceEvaluator.evaluate(experimentAudienceConditions, audiencesById, attributes, this.logger);
var result = this.audienceEvaluator.evaluate(experimentAudienceConditions, audiencesById, attributes);
this.logger.log(LOG_LEVEL.INFO, sprintf(LOG_MESSAGES.AUDIENCE_EVALUATION_RESULT_COMBINED, MODULE_NAME, experimentKey, result.toString().toUpperCase()));

if (!result) {
var userDoesNotMeetConditionsLogMessage = sprintf(LOG_MESSAGES.USER_NOT_IN_EXPERIMENT, MODULE_NAME, userId, experimentKey);
this.logger.log(LOG_LEVEL.INFO, userDoesNotMeetConditionsLogMessage);
Expand Down
4 changes: 2 additions & 2 deletions packages/optimizely-sdk/lib/core/decision_service/index.tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ var sprintf = require('sprintf-js').sprintf;
var testData = require('../../tests/test_data').getTestProjectConfig();
var testDataWithFeatures = require('../../tests/test_data').getTestProjectConfigWithFeatures();
var jsonSchemaValidator = require('../../utils/json_schema_validator');
var audienceEvaluator = require('../audience_evaluator');
var AudienceEvaluator = require('../audience_evaluator');

var chai = require('chai');
var sinon = require('sinon');
Expand Down Expand Up @@ -421,7 +421,7 @@ describe('lib/core/decision_service', function() {
var __audienceEvaluateSpy;

beforeEach(function() {
__audienceEvaluateSpy = sinon.spy(audienceEvaluator, 'evaluate');
__audienceEvaluateSpy = sinon.spy(AudienceEvaluator.prototype, 'evaluate');
});

afterEach(function() {
Expand Down
1 change: 1 addition & 0 deletions packages/optimizely-sdk/lib/optimizely/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ function Optimizely(config) {
configObj: this.configObj,
userProfileService: userProfileService,
logger: this.logger,
__exploratoryConditionEvaluators: config.__exploratoryConditionEvaluators
});

this.notificationCenter = notificationCenter.createNotificationCenter({
Expand Down
6 changes: 5 additions & 1 deletion packages/optimizely-sdk/lib/optimizely/index.tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
***************************************************************************/

var Optimizely = require('./');
var audienceEvaluator = require('../core/audience_evaluator');
var AudienceEvaluator = require('../core/audience_evaluator');
var bluebird = require('bluebird');
var bucketer = require('../core/bucketer');
var enums = require('../utils/enums');
Expand Down Expand Up @@ -238,6 +238,7 @@ describe('lib/optimizely', function() {
configObj: optlyInstance.configObj,
userProfileService: userProfileServiceInstance,
logger: createdLogger,
__exploratoryConditionEvaluators: undefined
Copy link
Contributor

Choose a reason for hiding this comment

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

This is kind of a weird test assertion. It's probably fine, but I also think it makes sense for __exporatoryConditionEvaluators to be required in new Optimizely or Optimizely defaults this to {}

});

// Checking the second log message as the first one just says "Datafile is valid"
Expand All @@ -262,6 +263,7 @@ describe('lib/optimizely', function() {
configObj: optlyInstance.configObj,
userProfileService: null,
logger: createdLogger,
__exploratoryConditionEvaluators: undefined
});

// Checking the second log message as the first one just says "Datafile is valid"
Expand Down Expand Up @@ -3503,6 +3505,7 @@ describe('lib/optimizely', function() {
logToConsole: false,
});
var optlyInstance;
var audienceEvaluator;
beforeEach(function() {
optlyInstance = new Optimizely({
clientEngine: 'node-sdk',
Expand All @@ -3514,6 +3517,7 @@ describe('lib/optimizely', function() {
logger: createdLogger,
isValidInstance: true,
});
audienceEvaluator = AudienceEvaluator.prototype;

sandbox.stub(eventDispatcher, 'dispatchEvent');
sandbox.stub(errorHandler, 'handleError');
Expand Down
Loading