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

Sonobi - Add ius param to bid request endpoint #3657

Merged
merged 6 commits into from
Apr 9, 2019

Conversation

JonGoSonobi
Copy link
Contributor

Type of change

  • Feature

Description of change

Sonobi adapter will detect if it can drop iframe pixels and set it as the ius parameter to sonobi's bid request endpoint. If its not ok to drop iframe pixels then the bid request will not respond with iframe pixels.

@JonGoSonobi
Copy link
Contributor Author

Closing this for now. Found an issue with using shoudBidderBeBlocked. Will reopen when we come up with a solution

@JonGoSonobi JonGoSonobi reopened this Mar 20, 2019
@bretg
Copy link
Collaborator

bretg commented Mar 20, 2019

@jsnellbaker - please take a look - I'm not clear why the additional "shouldBidderBeBlocked" object was added to the internal API.

@JonGoSonobi
Copy link
Contributor Author

@bretg @jsnellbaker I decided to try to reuse that function instead of essentially making a copy of it inside our adapter. So I put it on the userSync publicAPI. If you guys prefer a different approach let me know.

@jsnellbaker
Copy link
Collaborator

@JonGoSonobi I'm still thinking over this approach and if we should do something different.

Something to be aware of however are the semi-deprecated userSync settings (like userSync.iframeEnabled and userSync.enabledBidders). These settings still work and there may still be a number of publishers using these settings if they haven't updated their implementation.

If they are using them - the function you imported wouldn't give you an accurate picture.

@JonGoSonobi
Copy link
Contributor Author

JonGoSonobi commented Mar 21, 2019

@jsnellbaker right. We figured we wouldn't worry about those cases since we saw this comment.
"// TODO remove this else if code that supports deprecated fields (sometime in 2.x); for now - only run if filterSettings config is not present". Looks like there is some plans to remove the use of those fields in an upcoming minor version.

Maybe in the mean time to avoid straight up copying this code:

if (usConfig.filterSettings) { if (shouldBidderBeBlocked(type, bidder)) { return utils.logWarn(Bidder '${bidder}' is not permitted to register their userSync ${type} pixels as per filterSettings config.); } // 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 utils.logWarn(Bidder "${bidder}" not permitted to register their userSync pixels.); }

we can move it userSync publicApi similar to what I tried with shouldBidderBeBlocked? Call it like: canBidderRegisterSync

@jsnellbaker
Copy link
Collaborator

@JonGoSonobi That sounds like it could work-out; you'll technically need to incorporate logic to look at the iframeEnabled and pixelEnabled fields as well (since they're not part of that block of code).

If you want to put together the refactored changes, we can take a look at it and work from there.

@JonGoSonobi
Copy link
Contributor Author

@jsnellbaker sounds like a plan, I'll follow up with the changes.

@JonGoSonobi
Copy link
Contributor Author

@jsnellbaker changes committed. Let me know what you think

Copy link
Collaborator

@jsnellbaker jsnellbaker left a comment

Choose a reason for hiding this comment

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

@JonGoSonobi Thanks for putting together the refactor; the changes LGTM. I tested it out with various userSync configs and it seems to work consistently.

FYI - As it's a core-change, we will need a 2nd review before merging.

@jsnellbaker jsnellbaker added the needs 2nd review Core module updates require two approvals from the core team label Mar 26, 2019
@JonGoSonobi
Copy link
Contributor Author

@robertrmartinez any update on this one?

@robertrmartinez
Copy link
Collaborator

@JonGoSonobi Sorry I was out, but this looks good.

I see a test is failing, we should look into why the merge with master has done this.

Copy link
Collaborator

@robertrmartinez robertrmartinez left a comment

Choose a reason for hiding this comment

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

LGTM! @jsnellbaker reran that failure which was just a flakey-fail!
Thanks for this!

@jsnellbaker jsnellbaker merged commit 538d46d into prebid:master Apr 9, 2019
jacekburys-quantcast pushed a commit to jacekburys-quantcast/Prebid.js that referenced this pull request May 15, 2019
* added param to sonobi bid request bid url that lets sonobi know if its ok to drop iframe pixels.

* added case to isFilterConfigValid to check if it is falsey

* fixed eslint issues.

* refactor userSync checking if a bidder can drop a sync pixel to publicAPI function canBidderRegisterSync

* fixed lint error
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs 2nd review Core module updates require two approvals from the core team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants