-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Equativ Bid Adapter: support native bid requests #12566
Conversation
add support of dsa
restore topics
DSA fix for UT
Publish video support
split imp[0] into seperate requests and fix u.t.
Equativ bid adapter: adding support for native
Equativ Bid Adapter: updating unit tests for native requests
modules/equativBidAdapter.js
Outdated
* otherwise. | ||
* Evaluates impressions for validity. The entry evaluated is considered valid if NEITHER of these conditions are met: | ||
* 1) it has a `video` property defined for `mediaTypes.video` which is an empty object | ||
* 2) it has a `native` property defined for `mediatTypes.native` which is an empty object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spelling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here: mediatTypes.native
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just spotted it - thanks!
modules/equativBidAdapter.js
Outdated
if (deepAccess(bid, 'mediaTypes.native')) { | ||
['privacy', 'plcmttype', 'eventtrackers'].forEach(prop => { | ||
if (!bid.mediaTypes.native.ortb[prop]) { | ||
logWarn(`${LOG_PREFIX} Property "${prop}" is missing from request`, bid); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add detail, eg ${LOG_PREFIX} Property "${prop}" is missing from request, ${LOG_PREFIX} will not bid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our product managers want the warnings in there, but do want the bid request to be stopped if these particular properties are missing.
If assets
is missing, they are OK with the bid getting killed, but that is handled already by native.js
.
If not bidding if these other properties are missing is a hard requirement, I can share that back to them. Or if you would like the message further refined in another way (e.g. adding "it is strongly recommended you add ${prop}", etc.) we can do that too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
putting myself in the shoes of someone reading this message, i dont know what the outcome is. Tell the publisher the consequence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, that makes a lot of sense. I'll refine it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpicks
Type of change
Description of change
This work involves adding support for native requests to the new Equativ bid adapter.
Since this code base utilizes the ortbConverter library, there is little in the way of additional business logic updates in the adapter itself (beyond some warning logs). Most changes instead focus on unit tests verifying that bid requests sent out by the adapter conform to the Equativ SSP exchange's expectations of what should be present in the payloads.