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

fix bug related to default usersync config for image-based pixels #4928

Merged
merged 2 commits into from
Mar 6, 2020

Conversation

jsnellbaker
Copy link
Collaborator

Type of change

  • Bugfix

Description of change

Fixes #4864

This fix ensures the default userSync config (which allowed image pixels for all bidders) is properly used when a publisher only defines iframe filterSettings. If the publisher defines their own image or all config in the filterSettings field, then the default image setting is overwritten.

Copy link
Collaborator

@harpere harpere left a comment

Choose a reason for hiding this comment

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

a couple of small things

@@ -45,6 +47,20 @@ export function newUserSync(userSyncDependencies) {
let usConfig = userSyncDependencies.config;
// Update if it's (re)set
config.getConfig('userSync', (conf) => {
// if userSync.filterSettings only contains iframe, merge in default image config to ensure image pixels are fired
Copy link
Collaborator

Choose a reason for hiding this comment

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

might want to include a link to the issue (#4864) to give more detail on why this was done (how it broke backwards compatibility).

src/userSync.js Show resolved Hide resolved
@harpere harpere self-assigned this Mar 5, 2020
@harpere harpere added needs 2nd review Core module updates require two approvals from the core team needs review labels Mar 5, 2020
Copy link
Contributor

@pm-harshad-mane pm-harshad-mane left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@pm-harshad-mane pm-harshad-mane added LGTM and removed needs 2nd review Core module updates require two approvals from the core team needs review labels Mar 6, 2020
@pm-harshad-mane pm-harshad-mane merged commit 6324c93 into master Mar 6, 2020
@jsnellbaker
Copy link
Collaborator Author

@harpere I'll submit a small follow-up PR with the minor changes you asked. I was in the process of making those additions, so I should have it available relatively soon.

jsnellbaker added a commit that referenced this pull request Mar 6, 2020
harpere pushed a commit that referenced this pull request Mar 6, 2020
bmwcmw pushed a commit to criteo-forks/Prebid.js that referenced this pull request Mar 31, 2020
…ebid#4928)

* fix bug related to default usersync config

* fix lint error
bmwcmw pushed a commit to criteo-forks/Prebid.js that referenced this pull request Mar 31, 2020
rjvelicaria pushed a commit to openx/Prebid.js that referenced this pull request Apr 9, 2020
…ebid#4928)

* fix bug related to default usersync config

* fix lint error
rjvelicaria pushed a commit to openx/Prebid.js that referenced this pull request Apr 9, 2020
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 this pull request may close these issues.

Defining userSync.filterSettings.iframe only disables image syncing in v3.7.1
3 participants