Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Changes from 3 commits
5d9633c
eeb64d4
91474cd
30d9d6b
6d11855
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.