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
New Adapter: Smile Wanted #1877
New Adapter: Smile Wanted #1877
Changes from all commits
ac8d41d
b10866f
c73a4d1
6d1818c
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.
This is an anti-pattern. Does your bidding server return the media type of the bid in a bid response extension?
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 didn't see changes for this comment. The main problem is ensuring the bid type is correct. If your bidding server sees an impression with a banner and a video object, will it always just make a video 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.
We always attach a Type on the Zone that we create on our UI and the publisher implement this zone in the ZoneId params, So when we receive a new ad request, we know that it is a request for Display or Video in the zone configuration on our end.
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.
Interesting. Would your server return an error if it receives an Imp with both banner and video for a zone that is configured for video?
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.
No but we will force this on publisher side to have only one zone per type of request.
We have already done that with PrebidJS and it's works fine ;)
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.
Ok so i looked at when I make a config with two types (Banner and Video) and we answer on our side with one ad (the most expensive one between Video and Display on our side), so then PrebidJS displays it ;)
It will be an optimization that we can do later to take into consideration several requests of different types and make several responses, but it requires quite a few changes on our side ;)
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.
Since it is not a guaranteed correct identification, one possibility, until you have the infrastructure in place to properly in place to identify bid type, is to set your adapter disabled by default. That will give you a chance to open a dialogue with the PBS host and ensure that the client side has the ability to properly detect and handle the difference between banner and video bids.
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 to clarify : We identify on our end the type, we see in each Bid Request BUT we will answer with only one ad (corresponding to the winning ad on our internal competition between all types).
We support multi request but we do not support multi response, but it's only an optimization for us to have more opportunities to win the competition on publisher's side.
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 think overall the PR LGTM now. Just want to warn you that the above approach of assigning the media type for the bid might lead to the final ad not being rendered properly if the media type wasn't correctly set for a multi-format request. This might happen if for an imp that has both banner and a video object, your server returns a banner bid but then here you assign the media type to be video.
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 have had issues with other bidders in the past. If the final rendering client is not smart enough, it can get confused as to what it is trying to display. Just considering how you can get ahead of it rather than having your publisher clients have a poor experience that needs to be debugged after the fact.