-
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
Added MobFox Adapter #1312
Added MobFox Adapter #1312
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.
@francoroy Modules are introduced in Prebid with this #1177
So you need to update your Adapter by doing some minor changes. you can check here https://github.com/prebid/Prebid.js/pull/1177/files how you can migrate your code.
Also i left some comments on code.
src/adapters/mobfox.js
Outdated
ajax.ajax(`${BID_REQUEST_BASE_URL}?${queryString}`, { | ||
success(resp, xhr) { | ||
if (xhr.getResponseHeader("Content-Type") == "application/json") | ||
resp = JSON.parse(resp); |
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.
wrap all JSON.parse in try catch block
# Conflicts: # adapters.json
Hi, any progress? |
@francoroy Changes looks good. But not able to validate bids. |
@jaiminpanchal27 Thanks. I can share my hello world page:
|
@francoroy Got it. Thanks. |
I did find a bit of an issue here... If your server sends back a "no bid available" error, the adapter is still trying to add the bid to the bidmanager. This was likely working when you wrote it, but changes in #1277 (in particular, removing this check) cause an undefined reference now. You can reproduce the issue with this:
Since my changes were unintended, I'm replacing the old behavior in #1372... but I don't think this is ideal. If an adapter doesn't have a bid to add, then it shouldn't be calling the |
Hi @dbemiller So I don't understand, should I or should I not call addBidResponse when no bid is available? |
@francoroy There's a |
@dbemiller this is what I already do in my onBidResponseError function: let bidResponse = bidfactory.createBid(CONSTANTS.STATUS.NO_BID, bid); Is it ok then? |
@francoroy You're calling addBidResponse twice, and the second call uses an undefined bid. See the attached screenshot & test page. I set a breakpoint right before the page breaks.
|
@dbemiller Thanks |
@francoroy I'm still getting the exact same error. I've attached a screenshot from the same spot, but looking at your code. Your server is responding with a 200, but the response is It reproduces in your unit tests, too:
|
@dbemiller Thanks, I've fixed it |
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.
Looks good now.
I would recommend expanding your unit tests to catch this case, to prevent future regressions... but I won't require it for merge.
…built * 'master' of https://github.com/prebid/Prebid.js: (95 commits) Specify --browsers when using gulp test --watch (prebid#1420) Added aliases for aol adapter. (prebid#1371) Added MobFox Adapter (prebid#1312) Fixed style error. (prebid#1419) Add native support for Criteo adapter (prebid#999) Update admediaBidAdapter.js (prebid#1395) Increment pre version (prebid#1413) Prebid 0.26.1 Release (prebid#1412) fix prebid#1410 - issue with ie and xhr.timeout (prebid#1411) Lint modules directory (prebid#1404) Set outstream mediaType based on renderer in response (prebid#1391) Fixing the BidAdjustmentEvent fire time (prebid#1399) Fix banner showing up in prebid-core.js (prebid#1388) Mention NodeJS 4.0 dependency in the README (prebid#1386) Increment pre version (prebid#1385) Prebid 0.26.0 Release (prebid#1384) PulsePoint Lite adapter - adding createNew method for aliasing. (prebid#1383) Modernizing build dependencies (prebid#1355) StickyAdsTV bidder adapter update (prebid#1311) Added CPM value as parameter for Vertoz Adapter (prebid#1306) ...
Type of change
Description of change
Be sure to test the integration with your adserver using the Hello World sample page.
Other information