Skip to content
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

Deprecate user sync #4241

Merged
merged 6 commits into from
Dec 3, 2019
Merged

Deprecate user sync #4241

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bb77747
deprecate some properties of userSync object
Fawke Sep 23, 2019
57a85d9
change bid adapter to Appnexus
Fawke Sep 25, 2019
17163e1
fix 3 failing test cases
Fawke Sep 25, 2019
733d531
make all unit test cases pass
Fawke Sep 26, 2019
31e7d8d
remove debugger
Fawke Nov 12, 2019
a58e0ea
sync with prebid-3.0 branch
Fawke Dec 3, 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
4 changes: 2 additions & 2 deletions src/adapters/bidderFactory.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,8 +334,8 @@ export function newBidder(spec) {
if (spec.getUserSyncs) {
let filterConfig = config.getConfig('userSync.filterSettings');
let syncs = spec.getUserSyncs({
iframeEnabled: !!(config.getConfig('userSync.iframeEnabled') || (filterConfig && (filterConfig.iframe || filterConfig.all))),
pixelEnabled: !!(config.getConfig('userSync.pixelEnabled') || (filterConfig && (filterConfig.image || filterConfig.all))),
iframeEnabled: !!(filterConfig && (filterConfig.iframe || filterConfig.all)),
pixelEnabled: !!(filterConfig && (filterConfig.image || filterConfig.all)),
}, responses, gdprConsent);
if (syncs) {
if (!Array.isArray(syncs)) {
Expand Down
22 changes: 9 additions & 13 deletions src/userSync.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,12 @@ import includes from 'core-js/library/fn/array/includes';
config.setDefaults({
'userSync': {
syncEnabled: true,
pixelEnabled: true,
filterSettings: {
image: {
bidders: '*',
filter: 'include'
}
},
syncsPerBidder: 5,
syncDelay: 3000,
auctionDelay: 0
Expand All @@ -32,7 +37,7 @@ export function newUserSync(userSyncDependencies) {

// for now - default both to false in case filterSettings config is absent/misconfigured
let permittedPixels = {
image: false,
image: true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This was not mentioned in the PR,

Are we defaulting to enabling image pixels now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi,

Yes, userSync feature had image pixels enabled by default, prior to this change. Reason for this change here is that, we removed certain deprecated fields, as a result, we lost that default setting. We're setting this value to true now to maintain the default setting.

iframe: false
};

Expand Down Expand Up @@ -94,7 +99,7 @@ export function newUserSync(userSyncDependencies) {
* @private
*/
function fireImagePixels() {
if (!(usConfig.pixelEnabled || permittedPixels.image)) {
if (!permittedPixels.image) {
return;
}
forEachFire(queue.image, (sync) => {
Expand All @@ -111,7 +116,7 @@ export function newUserSync(userSyncDependencies) {
* @private
*/
function loadIframes() {
if (!(usConfig.iframeEnabled || permittedPixels.iframe)) {
if (!(permittedPixels.iframe)) {
return;
}
forEachFire(queue.iframe, (sync) => {
Expand Down Expand Up @@ -272,13 +277,6 @@ export function newUserSync(userSyncDependencies) {
if (shouldBidderBeBlocked(type, bidder)) {
return false;
}
// TODO remove this else if code that supports deprecated fields (sometime in 2.x); for now - only run if filterSettings config is not present
} else if (usConfig.enabledBidders && usConfig.enabledBidders.length && usConfig.enabledBidders.indexOf(bidder) < 0) {
return false
} else if (type === 'iframe' && !(usConfig.iframeEnabled || permittedPixels.iframe)) {
return false;
} else if (type === 'image' && !(usConfig.pixelEnabled || permittedPixels.image)) {
return false;
}
return true;
}
Expand All @@ -304,8 +302,6 @@ export const userSync = newUserSync({
*
* @property {boolean} enableOverride
* @property {boolean} syncEnabled
* @property {boolean} pixelEnabled
* @property {boolean} iframeEnabled
* @property {int} syncsPerBidder
* @property {string[]} enabledBidders
* @property {Object} filterSettings
Expand Down
82 changes: 55 additions & 27 deletions test/spec/userSync_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,21 +85,31 @@ describe('user sync', function () {
});

it('should not register pixel URL since it is not supported', function () {
const userSync = newTestUserSync({pixelEnabled: false});
const userSync = newTestUserSync({filterSettings: {
image: {
bidders: '*',
filter: 'exclude'
}
}});
userSync.registerSync('image', 'testBidder', 'http://example.com');
userSync.syncUsers();
expect(triggerPixelStub.getCall(0)).to.be.null;
});

it('should register and load an iframe', function () {
const userSync = newTestUserSync({iframeEnabled: true});
const userSync = newTestUserSync({filterSettings: {
iframe: {
bidders: '*',
filter: 'include'
}
}});
userSync.registerSync('iframe', 'testBidder', 'http://example.com/iframe');
userSync.syncUsers();
expect(insertUserSyncIframeStub.getCall(0).args[0]).to.equal('http://example.com/iframe');
});

it('should only trigger syncs once per page per bidder', function () {
const userSync = newTestUserSync({pixelEnabled: true});
const userSync = newTestUserSync({ pixelEnabled: true });
userSync.registerSync('image', 'testBidder', 'http://example.com/1');
userSync.syncUsers();
userSync.registerSync('image', 'testBidder', 'http://example.com/2');
Expand All @@ -113,7 +123,7 @@ describe('user sync', function () {
});

it('should not fire syncs if cookies are not supported', function () {
const userSync = newTestUserSync({pixelEnabled: true}, true);
const userSync = newTestUserSync({ pixelEnabled: true }, true);
userSync.registerSync('image', 'testBidder', 'http://example.com');
userSync.syncUsers();
expect(triggerPixelStub.getCall(0)).to.be.null;
Expand All @@ -132,14 +142,14 @@ describe('user sync', function () {
userSync.triggerUserSyncs();
expect(syncUsersSpy.notCalled).to.be.true;
// triggerUserSyncs should trigger syncUsers if enableOverride is on
userSync = newTestUserSync({enableOverride: true});
userSync = newTestUserSync({ enableOverride: true });
syncUsersSpy = sinon.spy(userSync, 'syncUsers');
userSync.triggerUserSyncs();
expect(syncUsersSpy.called).to.be.true;
});

it('should limit the number of syncs per bidder', function () {
const userSync = newTestUserSync({syncsPerBidder: 2});
const userSync = newTestUserSync({ syncsPerBidder: 2 });
userSync.registerSync('image', 'testBidder', 'http://example.com/1');
userSync.registerSync('image', 'testBidder', 'http://example.com/2');
userSync.registerSync('image', 'testBidder', 'http://example.com/3');
Expand All @@ -151,8 +161,8 @@ describe('user sync', function () {
expect(triggerPixelStub.getCall(2)).to.be.null;
});

it('should not limit the number of syncs per bidder when set to 0', function() {
const userSync = newTestUserSync({syncsPerBidder: 0});
it('should not limit the number of syncs per bidder when set to 0', function () {
const userSync = newTestUserSync({ syncsPerBidder: 0 });
userSync.registerSync('image', 'testBidder', 'http://example.com/1');
userSync.registerSync('image', 'testBidder', 'http://example.com/2');
userSync.registerSync('image', 'testBidder', 'http://example.com/3');
Expand Down Expand Up @@ -182,15 +192,20 @@ describe('user sync', function () {
});

it('should disable user sync', function () {
const userSync = newTestUserSync({syncEnabled: false});
const userSync = newTestUserSync({ syncEnabled: false });
userSync.registerSync('pixel', 'testBidder', 'http://example.com');
expect(logWarnStub.getCall(0).args[0]).to.exist;
userSync.syncUsers();
expect(triggerPixelStub.getCall(0)).to.be.null;
});

it('should only sync enabled bidders', function () {
const userSync = newTestUserSync({enabledBidders: ['testBidderA']});
const userSync = newTestUserSync({filterSettings: {
image: {
bidders: ['testBidderA'],
filter: 'include'
}
}});
userSync.registerSync('image', 'testBidderA', 'http://example.com/1');
userSync.registerSync('image', 'testBidderB', 'http://example.com/2');
userSync.syncUsers();
Expand All @@ -201,9 +216,9 @@ describe('user sync', function () {

it('should register config set after instantiation', function () {
// start with userSync off
const userSync = newTestUserSync({syncEnabled: false});
const userSync = newTestUserSync({ syncEnabled: false });
// turn it on with setConfig()
config.setConfig({userSync: {syncEnabled: true}});
config.setConfig({ userSync: { syncEnabled: true } });
userSync.registerSync('image', 'testBidder', 'http://example.com');
userSync.syncUsers();
expect(triggerPixelStub.getCall(0)).to.not.be.null;
Expand Down Expand Up @@ -360,8 +375,8 @@ describe('user sync', function () {
});

describe('publicAPI', function () {
describe('canBidderRegisterSync', function() {
describe('with filterSettings', function() {
describe('canBidderRegisterSync', function () {
describe('with filterSettings', function () {
it('should return false if filter settings does not allow it', function () {
const userSync = newUserSync({
config: {
Expand Down Expand Up @@ -397,14 +412,17 @@ describe('user sync', function () {
expect(userSync.canBidderRegisterSync('iframe', 'testBidder')).to.equal(true);
});
});
describe('almost deprecated - without filterSettings', function() {
describe('enabledBidders contains testBidder', function() {
describe('almost deprecated - without filterSettings', function () {
describe('enabledBidders contains testBidder', function () {
it('should return false if type is iframe and iframeEnabled is false', function () {
const userSync = newUserSync({
config: {
pixelEnabled: true,
iframeEnabled: false,
enabledBidders: ['testBidder'],
filterSettings: {
iframe: {
bidders: ['testBidder'],
filter: 'exclude'
}
}
}
});
expect(userSync.canBidderRegisterSync('iframe', 'testBidder')).to.equal(false);
Expand All @@ -424,9 +442,12 @@ describe('user sync', function () {
it('should return false if type is image and pixelEnabled is false', function () {
const userSync = newUserSync({
config: {
pixelEnabled: false,
iframeEnabled: true,
enabledBidders: ['testBidder'],
filterSettings: {
image: {
bidders: ['testBidder'],
filter: 'exclude'
}
}
}
});
expect(userSync.canBidderRegisterSync('image', 'testBidder')).to.equal(false);
Expand All @@ -444,13 +465,20 @@ describe('user sync', function () {
});
});

describe('enabledBidders does not container testBidder', function() {
it('should return false since testBidder is not in enabledBidders', function() {
describe('enabledBidders does not container testBidder', function () {
it('should return false since testBidder is not in enabledBidders', function () {
const userSync = newUserSync({
config: {
pixelEnabled: true,
iframeEnabled: true,
enabledBidders: ['otherTestBidder'],
filterSettings: {
image: {
bidders: ['otherTestBidder'],
filter: 'include'
},
iframe: {
bidders: ['otherTestBidder'],
filter: 'include'
}
}
}
});
expect(userSync.canBidderRegisterSync('iframe', 'testBidder')).to.equal(false);
Expand Down