-
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
orbidderBidAdapter.js: remove mediaType video to avoid broken requests #11851
orbidderBidAdapter.js: remove mediaType video to avoid broken requests #11851
Conversation
Whoa there, partner! 🌵🤠 We wrangled some duplicated code in your PR:
Reducing code duplication by importing common functions from a library not only makes our code cleaner but also easier to maintain. Please move the common code from both files into a library and import it in each. Keep up the great work! 🚀 |
@@ -93,6 +93,9 @@ export const spec = { | |||
if (bidderRequest && bidderRequest.refererInfo) { | |||
referer = bidderRequest.refererInfo.page || ''; | |||
} | |||
if (bidRequest?.mediaTypes?.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.
I'm confused, on line 4 and 49 this bidder already makes it clear they do not support video
@dgirardi this pr seems to imply a feature request (bug?) in the ortb2 convertor, in which a bidder receives unsupported media types
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 not using ortb converter. maybe it makes sense for core to remove unsupported media types, but on the other hand, adapters do not typically just dump the entirety of their input like this. Any modification of any kind to bid requests might break it for all we know.
@@ -93,6 +93,9 @@ export const spec = { | |||
if (bidderRequest && bidderRequest.refererInfo) { | |||
referer = bidderRequest.refererInfo.page || ''; | |||
} | |||
if (bidRequest?.mediaTypes?.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.
this is not using ortb converter. maybe it makes sense for core to remove unsupported media types, but on the other hand, adapters do not typically just dump the entirety of their input like this. Any modification of any kind to bid requests might break it for all we know.
modules/orbidderBidAdapter.js
Outdated
@@ -93,6 +93,9 @@ export const spec = { | |||
if (bidderRequest && bidderRequest.refererInfo) { | |||
referer = bidderRequest.refererInfo.page || ''; | |||
} | |||
if (bidRequest?.mediaTypes?.video) { | |||
delete bidderRequest.mediaTypes.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.
does this actually work? reading the code I don't think it has any effect on the outgoing request. (also, this bidderRequest
object is not supposed to have a mediaTypes
property).
It would help to have a reproducible issue, and translate that into a unit test.
Sorry, there is indeed a wrong reference for the delete. But, I'm not related to the company behind the bidder. We can see on our sites, that the bidder gets mediaType.video but because they do not support it the bid results in an Error 400. Deleting worked in my tests, I'll provide testpages. We as the auctioner do have no option to define which mediaType we want a bidder to participate, don't we? Maybe this change shouldn't be done but then how would I be able to do multiType for other adapters on the same placement but not for the orbidder? |
I agree that this should be fixed, but it's hard for us to verify it. I understand it's difficult for you as well, hopefully we can get orbidder to chime in (@hendrikiseke1979 ?) As the publisher you can direct only some media types to certain bidders with twin ad units. Basically breaking out orbidder into a duplicate ad unit definition that does not have video. |
Ok, but this will work for us, I can split our adUnits per bidder, I didn't know that's possible. I will try this on monday! |
Whoa there, partner! 🌵🤠 We wrangled some duplicated code in your PR:
Reducing code duplication by importing common functions from a library not only makes our code cleaner but also easier to maintain. Please move the common code from both files into a library and import it in each. Keep up the great work! 🚀 |
prebid#11851) * orbidderBidAdapter.js: remove mediaType video to avoid broken requests * orbidderBidAdapter.js: fix reference
Type of change
Description of change
The bidder seems to not support multiType bidrequests containing video. It then always throws a 400.
Our contact told us, the bidder doesn't support video at all, so we updated the bidder to remove definitions for video on bidRequests.
Other information
Recently changes to the bidder has been done by @arneschulz1984 and @hendrikiseke1979.
To ensure this change suits the bidder it would be great if one of those could do the review please