diff --git a/modules/id5IdSystem.js b/modules/id5IdSystem.js index 8a8cd25479f..d2d64f52738 100644 --- a/modules/id5IdSystem.js +++ b/modules/id5IdSystem.js @@ -18,7 +18,6 @@ const NB_EXP_DAYS = 30; export const ID5_STORAGE_NAME = 'id5id'; export const ID5_PRIVACY_STORAGE_NAME = `${ID5_STORAGE_NAME}_privacy`; const LOCAL_STORAGE = 'html5'; -const ABTEST_RESOLUTION = 10000; const LOG_PREFIX = 'User ID - ID5 submodule: '; // order the legacy cookie names in reverse priority order so the last @@ -59,22 +58,6 @@ export const id5IdSubmodule = { return undefined; } - // check for A/B testing configuration and hide ID if in Control Group - const abConfig = getAbTestingConfig(config); - const controlGroup = isInControlGroup(universalUid, abConfig.controlGroupPct); - if (abConfig.enabled === true && typeof controlGroup === 'undefined') { - // A/B Testing is enabled, but configured improperly, so skip A/B testing - utils.logError(LOG_PREFIX + 'A/B Testing controlGroupPct must be a number >= 0 and <= 1! Skipping A/B Testing'); - } else if (abConfig.enabled === true && controlGroup === true) { - // A/B Testing is enabled and user is in the Control Group, so do not share the ID5 ID - utils.logInfo(LOG_PREFIX + 'A/B Testing Enabled - user is in the Control Group, so the ID5 ID is NOT exposed'); - universalUid = ''; - linkType = 0; - } else if (abConfig.enabled === true) { - // A/B Testing is enabled but user is not in the Control Group, so ID5 ID is shared - utils.logInfo(LOG_PREFIX + 'A/B Testing Enabled - user is NOT in the Control Group, so the ID5 ID is exposed'); - } - let responseObj = { id5id: { uid: universalUid, @@ -84,8 +67,22 @@ export const id5IdSubmodule = { } }; - if (abConfig.enabled === true) { - utils.deepSetValue(responseObj, 'id5id.ext.abTestingControlGroup', (typeof controlGroup === 'undefined' ? false : controlGroup)); + const abTestingResult = utils.deepAccess(value, 'ab_testing.result'); + switch (abTestingResult) { + case 'control': + // A/B Testing is enabled and user is in the Control Group + utils.logInfo(LOG_PREFIX + 'A/B Testing - user is in the Control Group: ID5 ID is NOT exposed'); + utils.deepSetValue(responseObj, 'id5id.ext.abTestingControlGroup', true); + break; + case 'error': + // A/B Testing is enabled, but configured improperly, so skip A/B testing + utils.logError(LOG_PREFIX + 'A/B Testing ERROR! controlGroupPct must be a number >= 0 and <= 1'); + break; + case 'normal': + // A/B Testing is enabled but user is not in the Control Group, so ID5 ID is shared + utils.logInfo(LOG_PREFIX + 'A/B Testing - user is NOT in the Control Group'); + utils.deepSetValue(responseObj, 'id5id.ext.abTestingControlGroup', false); + break; } utils.logInfo(LOG_PREFIX + 'Decoded ID', responseObj); @@ -139,9 +136,12 @@ export const id5IdSubmodule = { data.provider = config.params.provider; } - // pass in feature flags, if applicable - if (getAbTestingConfig(config).enabled === true) { - utils.deepSetValue(data, 'features.ab', 1); + const abTestingConfig = getAbTestingConfig(config); + if (abTestingConfig.enabled === true) { + data.ab_testing = { + enabled: true, + control_group_pct: abTestingConfig.controlGroupPct // The server validates + }; } const resp = function (callback) { @@ -178,7 +178,7 @@ export const id5IdSubmodule = { utils.logInfo(LOG_PREFIX + 'requesting an ID from the server', data); ajax(url, callbacks, JSON.stringify(data), { method: 'POST', withCredentials: true }); }; - return {callback: resp}; + return { callback: resp }; }, /** @@ -310,37 +310,10 @@ export function storeInLocalStorage(key, value, expDays) { * gets the existing abTesting config or generates a default config with abTesting off * * @param {SubmoduleConfig|undefined} config - * @returns {(Object|undefined)} + * @returns {Object} an object which always contains at least the property "enabled" */ function getAbTestingConfig(config) { - return (config && config.params && config.params.abTesting) || { enabled: false }; -} - -/** - * Return a consistant random number between 0 and ABTEST_RESOLUTION-1 for this user - * Falls back to plain random if no user provided - * @param {string} userId - * @returns {number} - */ -function abTestBucket(userId) { - if (userId) { - return ((utils.cyrb53Hash(userId) % ABTEST_RESOLUTION) + ABTEST_RESOLUTION) % ABTEST_RESOLUTION; - } else { - return Math.floor(Math.random() * ABTEST_RESOLUTION); - } -} - -/** - * Return a consistant boolean if this user is within the control group ratio provided - * @param {string} userId - * @param {number} controlGroupPct - Ratio [0,1] of users expected to be in the control group - * @returns {boolean} - */ -export function isInControlGroup(userId, controlGroupPct) { - if (!utils.isNumber(controlGroupPct) || controlGroupPct < 0 || controlGroupPct > 1) { - return undefined; - } - return abTestBucket(userId) < controlGroupPct * ABTEST_RESOLUTION; + return utils.deepAccess(config, 'params.abTesting', { enabled: false }); } submodule('userId', id5IdSubmodule); diff --git a/test/spec/modules/id5IdSystem_spec.js b/test/spec/modules/id5IdSystem_spec.js index 9ba9aee9c63..debde20e4c0 100644 --- a/test/spec/modules/id5IdSystem_spec.js +++ b/test/spec/modules/id5IdSystem_spec.js @@ -74,9 +74,6 @@ describe('ID5 ID System', function() { } } } - function getFetchCookieConfig() { - return getUserSyncConfig([getId5FetchConfig(ID5_STORAGE_NAME, 'cookie')]); - } function getFetchLocalStorageConfig() { return getUserSyncConfig([getId5FetchConfig(ID5_STORAGE_NAME, 'html5')]); } @@ -239,35 +236,36 @@ describe('ID5 ID System', function() { expect(getNbFromCache(ID5_TEST_PARTNER_ID)).to.be.eq(0); }); - it('should call the ID5 server with ab feature = 1 when abTesting is turned on', function () { + it('should call the ID5 server with ab_testing object when abTesting is turned on', function () { let id5Config = getId5FetchConfig(); - id5Config.params.abTesting = { enabled: true, controlGroupPct: 10 } + id5Config.params.abTesting = { enabled: true, controlGroupPct: 0.234 } let submoduleCallback = id5IdSubmodule.getId(id5Config, undefined, ID5_STORED_OBJ).callback; submoduleCallback(callbackSpy); let request = server.requests[0]; let requestBody = JSON.parse(request.requestBody); - expect(requestBody.features.ab).to.eq(1); + expect(requestBody.ab_testing.enabled).to.eq(true); + expect(requestBody.ab_testing.control_group_pct).to.eq(0.234); request.respond(200, responseHeader, JSON.stringify(ID5_JSON_RESPONSE)); }); - it('should call the ID5 server without ab feature when abTesting is turned off', function () { + it('should call the ID5 server without ab_testing object when abTesting is turned off', function () { let id5Config = getId5FetchConfig(); - id5Config.params.abTesting = { enabled: false, controlGroupPct: 10 } + id5Config.params.abTesting = { enabled: false, controlGroupPct: 0.55 } let submoduleCallback = id5IdSubmodule.getId(id5Config, undefined, ID5_STORED_OBJ).callback; submoduleCallback(callbackSpy); let request = server.requests[0]; let requestBody = JSON.parse(request.requestBody); - expect(requestBody.features).to.be.undefined; + expect(requestBody.ab_testing).to.be.undefined; request.respond(200, responseHeader, JSON.stringify(ID5_JSON_RESPONSE)); }); - it('should call the ID5 server without ab feature when when abTesting is not set', function () { + it('should call the ID5 server without ab_testing when when abTesting is not set', function () { let id5Config = getId5FetchConfig(); let submoduleCallback = id5IdSubmodule.getId(id5Config, undefined, ID5_STORED_OBJ).callback; @@ -275,7 +273,7 @@ describe('ID5 ID System', function() { let request = server.requests[0]; let requestBody = JSON.parse(request.requestBody); - expect(requestBody.features).to.be.undefined; + expect(requestBody.ab_testing).to.be.undefined; request.respond(200, responseHeader, JSON.stringify(ID5_JSON_RESPONSE)); }); @@ -450,105 +448,11 @@ describe('ID5 ID System', function() { const expectedDecodedObjectWithIdAbOff = { id5id: { uid: ID5_STORED_ID, ext: { linkType: ID5_STORED_LINK_TYPE } } }; const expectedDecodedObjectWithIdAbOn = { id5id: { uid: ID5_STORED_ID, ext: { linkType: ID5_STORED_LINK_TYPE, abTestingControlGroup: false } } }; const expectedDecodedObjectWithoutIdAbOn = { id5id: { uid: '', ext: { linkType: 0, abTestingControlGroup: true } } }; - let testConfig; + let testConfig, storedObject; beforeEach(function() { testConfig = getId5FetchConfig(); - }); - - describe('Configuration Validation', function() { - let logErrorSpy; - let logInfoSpy; - - beforeEach(function() { - logErrorSpy = sinon.spy(utils, 'logError'); - logInfoSpy = sinon.spy(utils, 'logInfo'); - }); - afterEach(function() { - logErrorSpy.restore(); - logInfoSpy.restore(); - }); - - // A/B Testing ON, but invalid config - let testInvalidAbTestingConfigsWithError = [ - { enabled: true }, - { enabled: true, controlGroupPct: 2 }, - { enabled: true, controlGroupPct: -1 }, - { enabled: true, controlGroupPct: 'a' }, - { enabled: true, controlGroupPct: true } - ]; - testInvalidAbTestingConfigsWithError.forEach((testAbTestingConfig) => { - it('should be undefined if ratio is invalid', () => { - expect(isInControlGroup('userId', testAbTestingConfig.controlGroupPct)).to.be.undefined; - }); - it('should error if config is invalid, and always return an ID', function () { - testConfig.params.abTesting = testAbTestingConfig; - let decoded = id5IdSubmodule.decode(ID5_STORED_OBJ, testConfig); - expect(decoded).to.deep.equal(expectedDecodedObjectWithIdAbOn); - sinon.assert.calledOnce(logErrorSpy); - }); - }); - - // A/B Testing OFF, with invalid config (ignore) - let testInvalidAbTestingConfigsWithoutError = [ - { enabled: false, controlGroupPct: -1 }, - { enabled: false, controlGroupPct: 2 }, - { enabled: false, controlGroupPct: 'a' }, - { enabled: false, controlGroupPct: true } - ]; - testInvalidAbTestingConfigsWithoutError.forEach((testAbTestingConfig) => { - it('should be undefined if ratio is invalid', () => { - expect(isInControlGroup('userId', testAbTestingConfig.controlGroupPct)).to.be.undefined; - }); - it('should not error if config is invalid but A/B testing is off, and always return an ID', function () { - testConfig.params.abTesting = testAbTestingConfig; - let decoded = id5IdSubmodule.decode(ID5_STORED_OBJ, testConfig); - expect(decoded).to.deep.equal(expectedDecodedObjectWithIdAbOff); - sinon.assert.notCalled(logErrorSpy); - }); - }); - - // A/B Testing ON, with valid config - let testValidConfigs = [ - { enabled: true, controlGroupPct: 0 }, - { enabled: true, controlGroupPct: 0.5 }, - { enabled: true, controlGroupPct: 1 } - ]; - testValidConfigs.forEach((testAbTestingConfig) => { - it('should not be undefined if ratio is valid', () => { - expect(isInControlGroup('userId', testAbTestingConfig.controlGroupPct)).to.not.be.undefined; - }); - it('should not error if config is valid', function () { - testConfig.params.abTesting = testAbTestingConfig; - id5IdSubmodule.decode(ID5_STORED_OBJ, testConfig); - sinon.assert.notCalled(logErrorSpy); - }); - }); - }); - - describe('A/B Testing Config is not Set', function() { - let randStub; - - beforeEach(function() { - randStub = sinon.stub(Math, 'random').callsFake(function() { - return 0; - }); - }); - afterEach(function () { - randStub.restore(); - }); - - it('should expose ID when A/B config is not set', function () { - let decoded = id5IdSubmodule.decode(ID5_STORED_OBJ, testConfig); - expect(decoded).to.deep.equal(expectedDecodedObjectWithIdAbOff); - }); - - it('should expose ID when A/B config is empty', function () { - testConfig.params.abTesting = { }; - - let decoded = id5IdSubmodule.decode(ID5_STORED_OBJ, testConfig); - expect(decoded).to.deep.equal(expectedDecodedObjectWithIdAbOff); - }); + storedObject = utils.deepClone(ID5_STORED_OBJ); }); describe('A/B Testing Config is Set', function() { @@ -563,69 +467,41 @@ describe('ID5 ID System', function() { randStub.restore(); }); - describe('IsInControlGroup', function () { - it('Nobody is in a 0% control group', function () { - expect(isInControlGroup('dsdndskhsdks', 0)).to.be.false; - expect(isInControlGroup('3erfghyuijkm', 0)).to.be.false; - expect(isInControlGroup('', 0)).to.be.false; - expect(isInControlGroup(undefined, 0)).to.be.false; - }); - - it('Everybody is in a 100% control group', function () { - expect(isInControlGroup('dsdndskhsdks', 1)).to.be.true; - expect(isInControlGroup('3erfghyuijkm', 1)).to.be.true; - expect(isInControlGroup('', 1)).to.be.true; - expect(isInControlGroup(undefined, 1)).to.be.true; - }); + describe('Decode', function() { + let logErrorSpy; - it('Being in the control group must be consistant', function () { - const inControlGroup = isInControlGroup('dsdndskhsdks', 0.5); - expect(inControlGroup === isInControlGroup('dsdndskhsdks', 0.5)).to.be.true; - expect(inControlGroup === isInControlGroup('dsdndskhsdks', 0.5)).to.be.true; - expect(inControlGroup === isInControlGroup('dsdndskhsdks', 0.5)).to.be.true; + beforeEach(function() { + logErrorSpy = sinon.spy(utils, 'logError'); }); - - it('Control group ratio must be within a 10% error on a large sample', function () { - let nbInControlGroup = 0; - const sampleSize = 100; - for (let i = 0; i < sampleSize; i++) { - nbInControlGroup = nbInControlGroup + (isInControlGroup('R$*df' + i, 0.5) ? 1 : 0); - } - expect(nbInControlGroup).to.be.greaterThan(sampleSize / 2 - sampleSize / 10); - expect(nbInControlGroup).to.be.lessThan(sampleSize / 2 + sampleSize / 10); + afterEach(function() { + logErrorSpy.restore(); }); - }); - - describe('Decode', function() { - it('should expose ID when A/B testing is off', function () { - testConfig.params.abTesting = { - enabled: false, - controlGroupPct: 0.5 - }; - let decoded = id5IdSubmodule.decode(ID5_STORED_OBJ, testConfig); + it('should not set abTestingControlGroup extension when A/B testing is off', function () { + let decoded = id5IdSubmodule.decode(storedObject, testConfig); expect(decoded).to.deep.equal(expectedDecodedObjectWithIdAbOff); }); - it('should expose ID when no one is in control group', function () { - testConfig.params.abTesting = { - enabled: true, - controlGroupPct: 0 - }; - - let decoded = id5IdSubmodule.decode(ID5_STORED_OBJ, testConfig); + it('should set abTestingControlGroup to false when A/B testing is on but in normal group', function () { + storedObject.ab_testing = { result: 'normal' }; + let decoded = id5IdSubmodule.decode(storedObject, testConfig); expect(decoded).to.deep.equal(expectedDecodedObjectWithIdAbOn); }); it('should not expose ID when everyone is in control group', function () { - testConfig.params.abTesting = { - enabled: true, - controlGroupPct: 1 - }; - - let decoded = id5IdSubmodule.decode(ID5_STORED_OBJ, testConfig); + storedObject.ab_testing = { result: 'control' }; + storedObject.universal_uid = ''; + storedObject.link_type = 0; + let decoded = id5IdSubmodule.decode(storedObject, testConfig); expect(decoded).to.deep.equal(expectedDecodedObjectWithoutIdAbOn); }); + + it('should log A/B testing errors', function () { + storedObject.ab_testing = { result: 'error' }; + let decoded = id5IdSubmodule.decode(storedObject, testConfig); + expect(decoded).to.deep.equal(expectedDecodedObjectWithIdAbOff); + sinon.assert.calledOnce(logErrorSpy); + }); }); }); });