feat (audience match types): Update audience evaluator and project config for new audience match types #173

mjc1283 · 2018-10-05T20:10:02Z

Summary

This updates project config and the audience evaluator to finish the implementation of typed audience evaluation:

In audience_evaluator, allow evaluation to continue when no user attributes are passed (previously we were immediately returning false when no attributes were passed)
Update project_config: in createProjectConfig, set up a by-id object for accessing audiences from both audiences and typedAudiences. Also, update getAudiencesForExperiment to first look in projectConfig.typedAudiences, and only look in projectConfig.audiences as a fallback.
Added unit tests checking that all top-level methods that accept user attributes can bucket users into experiments or rollouts that use typed audiences.

Test Plan

New & existing unit tests
Ran compatibility suite tests

projectConfig.typedAudiences first, then fall back to projectConfig.audiences

attributes are passed in

…hods accepting user attributes

mikeproeng37

I haven't yet looked at the test cases. I'll try to get to them soon. In the meantime, here is some feedback.

mikeproeng37 · 2018-10-05T21:16:51Z

packages/optimizely-sdk/lib/core/audience_evaluator/index.js

@@ -30,9 +31,8 @@ module.exports = {
      return true;
    }

-    // if no user attributes specified, return false
    if (!userAttributes) {


I think this should be fine because we do validate that the userAttributes is either an object or null and thus falsy values don't make it here

Yeah, I don't think it needs to be the job of this module to handle other types of falsy values. Gracefully overwriting all falsy values seems reasonable.

@mikeng13 @nchilada I'm not sure if you're agreeing with my change, or requesting that this be left the way it was.

I think we're both agreeing! At least I am. Sorry for the confusion.

mikeproeng37 · 2018-10-05T21:25:07Z

packages/optimizely-sdk/lib/tests/test_data.js

@@ -546,6 +598,73 @@ var configWithFeatures = {
      'id': '594017',
      'name': 'test_audience',
      'conditions': '["and", ["or", ["or", {"type": "custom_attribute", "name": "test_attribute", "value": "test_value"}]]]'
+    },


Just a thought... but at some point maybe we can consolidate the test datafiles that we use across unit and compat tests

Agree. The test datafiles have become large and unwieldy, and this PR is making it worse. Given @nchilada's other test-related comment also, I'm rethinking some of these tests.

mikeproeng37 · 2018-10-05T21:27:16Z

packages/optimizely-sdk/lib/core/audience_evaluator/index.js

@@ -20,8 +20,9 @@ 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
+   * @param  {Object}   [userAttributes]     Hash representing user attributes which will be used in


Please also update the license header in this file and others you have touched that haven't had their license headers updated to this year.

mikeproeng37 · 2018-10-05T21:28:38Z

packages/optimizely-sdk/lib/core/project_config/index.js

+    fns.forEach(projectConfig.typedAudiences, function(audience) {
+      audience.conditions = JSON.parse(audience.conditions);
+    });
+    projectConfig.typedAudienceIdMap = fns.keyBy(projectConfig.typedAudiences, 'id');


Is there any reason to maintain a separate map? Any conflicts in ID's would get overwritten with the typed audience, which is what we want.

+1, a single map would be nice.

And maybe call it audiencesById since it's storing audiences and not IDs? But maybe I'm just parsing it weirdly

mikeproeng37 · 2018-10-05T21:29:35Z

packages/optimizely-sdk/lib/core/project_config/index.js

-    var audiencesInExperiment = fns.filter(projectConfig.audiences, function(audience) {
-      return audienceIds.indexOf(audience.id) !== -1;
+    fns.forEach(audienceIds, function(audienceId) {
+      var typedAudience = projectConfig.typedAudienceIdMap[audienceId];


Haha I dunno why we did the lookup the other way around in the first place. This is so much better :)

mikeproeng37 · 2018-10-05T21:29:55Z

packages/optimizely-sdk/lib/core/project_config/index.js

+        audiencesInExperiment.push(typedAudience);
+        return;
+      }
+      var audience = projectConfig.audienceIdMap[audienceId];


If we have just one consolidated map, we'd just have to look in one place.

nchilada

I'm not sure if we need to test each match type with each API. We just need to make sure that each API does some audience targeting, so we could probably get away with ~2 test cases for each API, one where an experiment's audienceIds collectively pass and one where they collectively fail.

That said, I'm not going to complain too much about thorough tests 😄 LGTM!

P.S. I would also err on the side of whatever's simplest for the moment (either in terms of the smallest change to the codebase or the smallest additional change to this PR): when we tackle audience combinations there'll be another layer of complexity that we'll suddenly have to care about.

…evel methods accepting user attributes" This reverts commit 7bd1c6a.

…iences to the new config.

mikeproeng37

lgtm

mikeproeng37 · 2018-10-09T17:30:34Z

build

Matt Carroll added 3 commits October 5, 2018 11:48

project_config getAudiencesForExperiment: look for audiences in

e41f5c9

projectConfig.typedAudiences first, then fall back to projectConfig.audiences

audience_evaluator: Continue with evaluation even when no user

da3c276

attributes are passed in

optimizely: Add tests for typed audience evaluation for top-level met…

7bd1c6a

…hods accepting user attributes

mjc1283 requested review from nchilada, mikeproeng37, aliabbasrizvi and thomaszurkan-optimizely October 5, 2018 20:10

mikeproeng37 suggested changes Oct 5, 2018

View reviewed changes

nchilada approved these changes Oct 6, 2018

View reviewed changes

Matt Carroll added 5 commits October 8, 2018 11:02

Review fixes

8cfeef9

Revert "optimizely: Add tests for typed audience evaluation for top-l…

2497a3e

…evel methods accepting user attributes" This reverts commit 7bd1c6a.

Restore reverted project_config test

b81b5a5

Restore top-level API tests. Thin out coverage and restrict typed aud…

9eda7f1

…iences to the new config.

Remove unnecessary stub of currentTimestamp

a83c434

mikeproeng37 approved these changes Oct 9, 2018

View reviewed changes

mjc1283 merged commit 0f19fbe into mcarroll/audience-match-types Oct 9, 2018

mjc1283 deleted the mcarroll/audience-match-types-project-config branch October 9, 2018 17:43

mjc1283 mentioned this pull request Oct 9, 2018

feat (audiences): Typed audience evaluation #174

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat (audience match types): Update audience evaluator and project config for new audience match types #173

feat (audience match types): Update audience evaluator and project config for new audience match types #173

Uh oh!

mjc1283 commented Oct 5, 2018 •

edited

Loading

Uh oh!

mikeproeng37 left a comment

Uh oh!

mikeproeng37 Oct 5, 2018

Uh oh!

nchilada Oct 6, 2018

Uh oh!

mjc1283 Oct 8, 2018

Uh oh!

nchilada Oct 8, 2018 •

edited

Loading

Uh oh!

mikeproeng37 Oct 5, 2018

Uh oh!

mjc1283 Oct 8, 2018

Uh oh!

mikeproeng37 Oct 5, 2018

Uh oh!

mikeproeng37 Oct 5, 2018

Uh oh!

nchilada Oct 6, 2018

Uh oh!

mikeproeng37 Oct 5, 2018

Uh oh!

mikeproeng37 Oct 5, 2018

Uh oh!

nchilada left a comment

Uh oh!

mikeproeng37 left a comment

Uh oh!

mikeproeng37 commented Oct 9, 2018

Uh oh!

Uh oh!

feat (audience match types): Update audience evaluator and project config for new audience match types #173

feat (audience match types): Update audience evaluator and project config for new audience match types #173

Uh oh!

Conversation

mjc1283 commented Oct 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

mikeproeng37 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nchilada Oct 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nchilada left a comment

Choose a reason for hiding this comment

Uh oh!

mikeproeng37 left a comment

Choose a reason for hiding this comment

Uh oh!

mikeproeng37 commented Oct 9, 2018

Uh oh!

Uh oh!

mjc1283 commented Oct 5, 2018 •

edited

Loading

nchilada Oct 8, 2018 •

edited

Loading