-
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
New bid adapter for OPEN8 #3521
Conversation
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.
Hi @aiichiro
Thanks for submitting this new adapter.
When I was attempting to test both the banner/video outstream test placements - I encountered set of 404/CORS related errors. I copied the text below for reference:
GET http://as.vt.open8.com/v1/control/prebid?slot_key=2ae5a533&imp_id=3l94t3s93z0cigm5&bid_id=2e09c4f8338215& 404 (Not Found)
Access to XMLHttpRequest at 'http://as.vt.open8.com/v1/control/prebid?slot_key=2ae5a533&imp_id=3l94t3s93z0cigm5&bid_id=2e09c4f8338215&' from origin 'http://test.localhost:9999' has been blocked by CORS policy: No 'Access-Control-Allow-Origin' header is present on the requested resource.
Can you please take a look to see what's amiss? If you need any additional details, please let me know. Thanks
Also - can you please put together a docs PR (here) for your bidder? You can refer to the other bidder files for reference. |
@jsnellbaker Thank you for your review. I think we should send PR after our server has been available. Also we create a doc PR. Thanks |
@aiichiro Thanks for clarifying. Do you have a rough ETA for when the server would be available for use? If it's in the relatively near-future, we could just keep this PR open for a bit. If it's longer, we can close for now and resubmit it later. Let me know either way. Thanks. |
@jsnellbaker Thanks for checking. By the way, our video is floating header overlay. so video size depends on window size. 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.
Hi @aiichiro
Thanks for making the changes. I tested both the outstream and banner ads again and they were delivering properly.
While testing, I did observe some warning/errors that were thrown in the browser's console log from both ads. These errors were caught in try/catch blocks so it didn't interfere with the page loading, but I wanted to bring them to your attention in case you wanted to add an extra check in your code in these areas to handle the situation when the fields aren't populated in a bid response (or to confirm that this is what you want).
I noted the two areas in the code that caused the warning/error below; please let me know what you think. Thanks.
modules/open8BidAdapter.js
Outdated
mediaType: BANNER | ||
}); | ||
try { | ||
bannerAd.imps.forEach(impTrackUrl => { |
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.
Could add a check to verify the bannerAd.imps
is populated before running the forEach
.
return syncs; | ||
}, | ||
onBidWon: function(bid) { | ||
const winUrl = bid.nurl.replace( |
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.
Could add a check here to verify the nurl
property is populated in the bid
object.
@jsnellbaker Thanks for your review. |
Thanks for the update; LGTM |
Type of change
Description of change