-
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
Ttd Bid adapter: support for transaction id, bcat, and consolidate params #8679
Ttd Bid adapter: support for transaction id, bcat, and consolidate params #8679
Conversation
make placementId parameter optional if GPID is passed pass transactionId through to source.tid read bcat from ortb2
@@ -387,6 +382,10 @@ export const spec = { | |||
ext: getExt(firstPartyData) | |||
} | |||
|
|||
if (firstPartyData && firstPartyData.bcat) { | |||
topLevel.bcat = firstPartyData.bcat; |
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 document if this overwrites or appends existing settings for an account on the backend? I think append is not particularly workable, and overwrite is preferred.
Why not battr and badomain as well?
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.
Sorry, badv
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.
We would also like to see support for battr and badv here.
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.
I'll add battr/badv. We're still discussing behavior internally.
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.
thanks!
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.
We decided to make the blocking additive (docs will reflect this).
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 we discuss? This decision breaks much of the utility of this feature
Generally ad management companies likes Rubicon, Cafemedia, Mediavine, & Freestar have a set of opt outs that are default and quite conservative, that typically match the defaults in GAM sensitive categories on AdX. They then have clients (publishers) opt in to more sensitive categories. If the ad management company, eg Cafemedia, sets up the normal blocking as default blocks on your backend, the opt in is impossible to communicate. In order to use this feature, Cafe and others would have to set up only very minimal blocking on the backend, which might be difficult to transition to logistically.
All that being said, as long as you document it clearly, no concerns from a code perspective and I'm sure we can make it work.
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.
@julian-burger-ttd can you confirm the badv battr changes above are planned for this pr or if they are planned for a follow up?
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.
Confirming for this PR. Haven't had a chance to get to it yet, though. As for overwrite vs. additive we are having some internal discussion as there are multiple use cases to consider.
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.
Updated the code. Docs updates (separate MR anyway) will follow.
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.
comments on imp.ext.tid vs source.tid
Lgtm |
…rams (prebid#8679) * remove siteId parameter make placementId parameter optional if GPID is passed pass transactionId through to source.tid read bcat from ortb2 * enforce check for gpid or placementId * add error message * Support badv/battr, get tid from ext object * Address review feedback Co-authored-by: Minh Daole <[email protected]>
…rams (prebid#8679) * remove siteId parameter make placementId parameter optional if GPID is passed pass transactionId through to source.tid read bcat from ortb2 * enforce check for gpid or placementId * add error message * Support badv/battr, get tid from ext object * Address review feedback Co-authored-by: Minh Daole <[email protected]>
Type of change
Description of change
TTD Adapter Updates
Remove siteId parameter
Make placementId parameter optional if GPID is passed
Pass transactionId through to source.tid
Read bcat from ortb2