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.

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

Defining userSync.filterSettings.iframe only disables image syncing in v3.7.1 #4864

Closed
Austinb opened this issue Feb 14, 2020 · 4 comments · Fixed by #4928
Closed

Defining userSync.filterSettings.iframe only disables image syncing in v3.7.1 #4864

Austinb opened this issue Feb 14, 2020 · 4 comments · Fixed by #4928
Assignees
Labels

Comments

@Austinb
Copy link

Austinb commented Feb 14, 2020

Type of issue

Bug or undocumented change in default behavior when moving from v2.x to v.3.x

Description

In version 3.7.1 (and 3.3.0) when you specify the userSync.filterSettings it overwrites the defaults rather than being merged with the defaults. If you define the userSync.filterSettings.iframe settings the default userSync.filterSettings.image settings from https://github.com/prebid/Prebid.js/blob/master/src/userSync.js#L9 are lost. This effectively disables image pixel syncing unless you define it explicitly or use the userSync.filterSettings.all property.

In the v2.x series it did not do this (verified in both v2.39.0 and v2.34.0 with currently active deployments).

Steps to reproduce

If you have your pb.js config defined with the following

{
    userSync: {
        filterSettings: {
            iframe: {
                bidders: '*', 
                filter: 'include',
            },
        },
    },
}

The response from the config.getConfig calls in https://github.com/prebid/Prebid.js/blob/master/src/adapters/bidderFactory.js#L335 only contains the defined settings above. As a result the value of pixelEnabled is false a couple of lines down.

Test page

Not available

Expected results

If the image syncing is supposed to be enabled by default then it should always be enabled unless otherwise explicitly disabled in the userSync.filterSettings.image key.

Actual results

If the userSync.filterSettings.image setting is not defined when defining userSync.filterSettings.iframe image syncs are disabled.

Platform details

Building Prebis.js on:
Ubuntu 18.04.3
Node JS v10.17.0
Npm 6.13.6

Other information

Did a quick search and was unable to find any specific change that affected this but I did not spend a lot of time searching either. I did find some comments in https://github.com/prebid/Prebid.js/pull/4241/files referencing that the permittedPixels.image should remain true by default which leads me to believe this is a bug and not an undocumented change in default behavior. If I have some time I will step through and see what change broke the default behavior.

@Austinb
Copy link
Author

Austinb commented Feb 14, 2020

After having dug around a bit I think this issue stems from the removal of the old userSync.pixelEnabled value in #4580. Before it appears the library was always using that value at https://github.com/prebid/Prebid.js/blob/master/src/adapters/bidderFactory.js#L337. Now since that setting is gone the default is not working properly because the Object.assign() at https://github.com/prebid/Prebid.js/blob/master/src/config.js#L298 is (and has been) overwriting the userSync.filterSettings key. That key already exists in the defaults from https://github.com/prebid/Prebid.js/blob/master/src/userSync.js#L6 and is not being merged.

Output of before https://github.com/prebid/Prebid.js/blob/master/src/config.js#L298 I get the following:

{
auctionDelay: 0
​filterSettings: {
​​image: Object { bidders: "*", filter: "include" }
}
​​​syncDelay: 3000
​syncEnabled: true
​syncsPerBidder: 5
}

The value of option after that line is as follows:

{
auctionDelay: 0
​filterSettings: {
​​iframe: Object { bidders: "*", filter: "include" }
}
​​​syncDelay: 3000
​syncEnabled: true
​syncsPerBidder: 5
}

So either there needs to be no more default, a default another way or that code needs to be expanded to check for objects and merge their keys. Please advise as to which is preferable. Personally I think the logic on that line should be changed to a "merge" like deepmerge as there maybe other options/things that need to be merged down the line.

@Austinb Austinb changed the title Defining userSync.filterSettings.iframe only disables pixel syncing in v3.7.1 Defining userSync.filterSettings.iframe only disables image syncing in v3.7.1 Feb 14, 2020
@stale
Copy link

stale bot commented Feb 28, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Feb 28, 2020
@Austinb
Copy link
Author

Austinb commented Feb 28, 2020

Wanted to bump this and make sure it is not over looked.

@stale stale bot removed the stale label Feb 28, 2020
@jsnellbaker
Copy link
Collaborator

@Austinb Thanks for the bump & reporting it - I'll take a look at this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants