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.

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
91 changes: 52 additions & 39 deletions packages/optimizely-sdk/lib/core/audience_evaluator/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,55 +15,68 @@
*/
var conditionTreeEvaluator = require('../condition_tree_evaluator');
var customAttributeConditionEvaluator = require('../custom_attribute_condition_evaluator');
var ERROR_MESSAGES = require('../../utils/enums').ERROR_MESSAGES;
var enums = require('../../utils/enums');
var sprintf = require('sprintf-js').sprintf;

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 = {};
}
function AudienceEvaluator(conditionEvaluators) {
this.typeToEvaluatorMap = Object.assign({
'custom_attribute': customAttributeConditionEvaluator
}, conditionEvaluators);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we just want allow for additional condition types beyond those supported by the javascript-sdk, or do we also want to allow customers to modify how the built-in condition types are evaluated?

If the former, I wonder if we can add some kind of namespacing (as in Optimizely Web audience integrations) to protect custom conditions types from conflicting with (existing and future) built-in condition types.

If the latter, I have serious reservations, since that could make it difficult for us to modify our own condition types without affecting existing customers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yikes, we don't seem to catch errors when constructing the decision service. So if we apply any validation at this stage, or do anything that could throw an error, that error will bubble up into the customer's application.

Maybe we should do something about that! Though for the moment, I'm just pointing this out so that we remain careful not to throw.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the approach of not allowing the built in evaluators to be overridden. If you need to do that, then you'll be able to write your own.

Good call on the error catching. Added an error message for that which is used in a try/catch now.

Copy link
Contributor

Choose a reason for hiding this comment

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

TODO: additional unit tests, eventually

}

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
* @param {Object} logger Logger instance.
* @return {Boolean} true if the user attributes match the given audience conditions, false
* otherwise
*/
AudienceEvaluator.prototype.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;
}

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 evaluateConditionWithUserAttributes = function(condition) {
var evaluator = this.typeToEvaluatorMap[condition.type];
if (!evaluator) {
logger.log(LOG_LEVEL.WARNING, sprintf(ERROR_MESSAGES.AUDIENCE_EVALUATOR_EXPECTED, MODULE_NAME, condition.type));
return false;
}
return evaluator.evaluate(condition, userAttributes, logger);
}.bind(this);

return conditionTreeEvaluator.evaluate(audienceConditions, evaluateAudience) || false;
},
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;
}

return null;
};

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

module.exports = AudienceEvaluator;
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
* 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');
Expand Down Expand Up @@ -53,6 +53,11 @@ var audiencesById = {
};

describe('lib/core/audience_evaluator', function() {
var audienceEvaluator;
beforeEach(function() {
audienceEvaluator = new AudienceEvaluator();
});

describe('APIs', function() {
describe('evaluate', function() {
var mockLogger = logger.createLogger({logLevel: LOG_LEVEL.INFO});
Expand Down
7 changes: 4 additions & 3 deletions packages/optimizely-sdk/lib/core/decision_service/index.js
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.conditionEvaluators);
}

/**
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);
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
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
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,
conditionEvaluators: config.conditionEvaluators
});

this.notificationCenter = notificationCenter.createNotificationCenter({
Expand Down
6 changes: 5 additions & 1 deletion packages/optimizely-sdk/lib/optimizely/index.tests.js
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,
conditionEvaluators: undefined
});

// 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,
conditionEvaluators: 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
1 change: 1 addition & 0 deletions packages/optimizely-sdk/lib/utils/enums/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ exports.LOG_LEVEL = {
};

exports.ERROR_MESSAGES = {
AUDIENCE_EVALUATOR_EXPECTED: '%s: No audience evaluator present for condition type: %s.',
EXPERIMENT_KEY_NOT_IN_DATAFILE: '%s: Experiment key %s is not in datafile.',
FEATURE_NOT_IN_DATAFILE: '%s: Feature key %s is not in datafile.',
IMPROPERLY_FORMATTED_EXPERIMENT: '%s: Experiment key %s is improperly formatted.',
Expand Down