Showheroes Bid Adapter : full rework of the adapter #12283
Conversation
FilipStamenkovic
force-pushed
the
showheroes_bidder_update
branch
from
52c1e16
to
489d117
Compare
ChrisHuie
changed the title
|
I thought this was good to go, but it shows conflicts preventing it from merging. Can you resolve the conflicts? @FilipStamenkovic |
FilipStamenkovic
force-pushed
the
showheroes_bidder_update
branch
from
54a8fc1
to
64867ba
Compare
|
@Rothalack Thanks for the review. I resolved the conflicts. On |
modules/showheroes-bsBidAdapter.js
Outdated
| // delete user agent from oRTB, we'll get it from the header | ||
| (req?.device?.ua) && delete req.device['ua']; | ||
| // 'sua' is 2.6 standard, we operate with 2.5 | ||
| (req?.device?.sua) && delete req.device['sua']; |
There was a problem hiding this comment.
do not necessarily need to check can just call delete (either way is fine just pointing out)
delete req?.device?.ua;
delete req?.device?.sua;| code: BIDDER_CODE, | ||
| gvlid: GVLID, | ||
| aliases: ['showheroesBs'], | ||
| supportedMediaTypes: [VIDEO], |
There was a problem hiding this comment.
So this PR is removing BANNER support from this adapter?
I feel like this could be considered a "breaking change" and might have to be put into a "major" prebid version.
Lets ask @patmmccann @bretg @dgirardi @ChrisHuie
There was a problem hiding this comment.
Currently, we don't have anyone using showheroes prebid bidder for banner ads. We plan to do the 'full rework' of banner ads handling next year (probably).
My initial thought was because of that is to temporarily remove the support for BANNER and then later open a new PR where BANNER is re-enabled.
But if this would be a 'breaking change' requiring us to wait for next major release, I can revert my commit and 're-enable' legacy BANNER support.
There was a problem hiding this comment.
I actually do not really care either way but figured it would be good to double check.
I am not aware of us having this use case come up before (where an adapter removes a supported media type)
There was a problem hiding this comment.
let us know this ASAP, @robertrmartinez .
In the future we'll re-add banners, but for the moment they're not working and we thoght we could save some bytes by just removing code that was unnecessary. Releasing this adapter to the OS is critical for the next set of releases. So, if to speed up the process we must reintroduce support for a media type that was not working, we will.
And btw, hello dear PBJS friends :)
modules/showheroes-bsBidAdapter.js
Outdated
| @@ -210,6 +132,7 @@ export const spec = { | |||
| }); | |||
| }); | |||
| } | |||
| logInfo(`found urls to sync:`, syncs); | |||
There was a problem hiding this comment.
just making sure this info log is wanted still
FilipStamenkovic
force-pushed
the
showheroes_bidder_update
branch
from
64867ba
to
1ba2541
Compare
| // 'sua' is 2.6 standard, we operate with 2.5 | ||
| (req?.device?.sua) && delete req.device['sua']; | ||
| return req; | ||
| }, |
There was a problem hiding this comment.
we decided to completely remove this logic instead?
So your server is okay with receiving both ua and sua ??
There was a problem hiding this comment.
Yes, the server is ok with receiving both.
suawe don't support at the moment, but there is a plan to upgrade to 2.6, so at that point, we'll start using and we won't need to open another PR for our bidder.uawe'll use it and as a fallback we'll use a header ifuais not provided.
Type of change
Description of change
Update the
showheroes-bsbidder adapter.PR has the following updates:
prebid-shwith the newopenrtb2/auctionortbConverterto construct a bid requestfpd,floormodule,eids, etc.To test the upgrade just set the openRTB request as a test request:
Other information