-
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
add adot bidder adapter #4949
add adot bidder adapter #4949
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 @mlequain
Thank you for submitting this new adapter. There are a few things that I observed that need to be initially addressed since they're causing errors or aren't supported by the Prebid.js project.
Can you please take a look when you can?
Thanks!
modules/adotBidAdapter.js
Outdated
const NET_REVENUE = true; | ||
// eslint-disable-next-line no-template-curly-in-string | ||
const AUCTION_PRICE = '${AUCTION_PRICE}'; | ||
const OUTSTREAM_VIDEO_PLAYER_URL = 'http://adserver.adotmob.com/video/player.min.js'; |
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 host path should be secure.
modules/adotBidAdapter.js
Outdated
function isObject(value) { | ||
return (value !== null) && (typeof (value) === 'object') && !isArray(value); | ||
} | ||
|
||
function isArray(value) { | ||
return Array.isArray(value); | ||
} | ||
|
||
function isString(value) { | ||
return (typeof (value) === 'string'); | ||
} | ||
|
||
function isNumber(value) { | ||
return (typeof (value) === 'number'); | ||
} | ||
|
||
function isBoolean(value) { | ||
return (typeof (value) === 'boolean'); | ||
} |
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.
All these functions can be found in the src/utils.js
file under similar names. It may ideal (and save some space in your adapter) to use those instead.
modules/adotBidAdapter.js
Outdated
function groupBy(values, key) { | ||
const groups = values.reduce((acc, value) => { | ||
const groupId = value[key]; | ||
|
||
if (!acc[groupId]) acc[groupId] = []; | ||
acc[groupId].push(value); | ||
|
||
return acc; | ||
}, {}); | ||
|
||
return Object | ||
.entries(groups) | ||
.map(([id, values]) => ({id, key, values})); | ||
} |
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.
There is a similar function available in the src/utils.js
, could you review that function to see if it would meet your needs here?
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.
The function in the src/utils.js
does not format the values the same way unfortunately. Do you want me to rename the method to avoid confusion ?
return isArray(mediaSize) && | ||
(mediaSize.length === 2) && | ||
mediaSize.every(size => (isNumber(size) && size >= 0)); |
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.
There is a function in the src/utils.js
named isArrayOfNums
that I believe basically the same type of check here. Perhaps you could use it instead.
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.
The issue with isArrayOfNums
is that it does not check if the value is superior to 0.
modules/adotBidAdapter.js
Outdated
const base = {bidId, adUnitCode, container: params.video && params.video.container}; | ||
|
||
const imps = Object | ||
.entries(mediaTypes) |
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 property is not supported in IE11. Is there an alternative you can use instead?
modules/adotBidAdapter.js
Outdated
if (!isObject(bid.ext)) return false; | ||
if (!isObject(bid.ext.adot)) return false; | ||
if (!isString(bid.ext.adot.media_type)) return false; | ||
if (!BID_SUPPORTED_MEDIA_TYPES.includes(bid.ext.adot.media_type)) return false; |
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.
Please see earlier note.
modules/adotBidAdapter.js
Outdated
if (!bid.adm && !bid.nurl) return false; | ||
if (bid.adm) { | ||
if (!isString(bid.adm)) return false; | ||
if (!bid.adm.includes(AUCTION_PRICE)) return false; |
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.
Please see earlier note.
modules/adotBidAdapter.js
Outdated
} | ||
if (bid.nurl) { | ||
if (!isString(bid.nurl)) return false; | ||
if (!bid.nurl.includes(AUCTION_PRICE)) return false; |
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.
Please see earlier note.
modules/adotBidAdapter.js
Outdated
const allowedBidderCodes = [this.code]; | ||
|
||
return isObject(adUnit) && | ||
allowedBidderCodes.includes(adUnit.bidder) && |
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.
Please see earlier note.
import { executeRenderer } from 'src/Renderer'; | ||
import * as utils from 'src/utils'; | ||
import { spec } from 'modules/adotBidAdapter'; |
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.
Please include the .js
extension on these imports as well (eg. src/Renderer.js
).
@jsnellbaker I've updated the code to address the issues you mentioned in your previous comment. Let me know if there is anything that I missed or if you want me to change anything else. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
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 @mlequain
Sorry for the delay here; thank you for making the updates. I looked through again, and it looks better. There is another function that needs to be updated because it's not available in IE11. I made a suggestion for an alternative, please see the comment below.
Additionally, when I tried to test the adapter with the params provided in the description - the endpoint didn't appear to be returning a response. The endpoint returned a 204 No Content response.
I copied the request payload in case that may be a factor:
{"id":"1693a3242bca91","imp":[{"id":"2540029fc5df2c_0_0","banner":{"format":[{"w":300,"h":250}],"w":300,"h":250,"pos":0},"pmp":{"deals":[{"id":"adot_prebidjs_integration"}]}},{"id":"2540029fc5df2c_0_1","banner":{"format":[{"w":300,"h":600}],"w":300,"h":600,"pos":0},"pmp":{"deals":[{"id":"adot_prebidjs_integration"}]}}],"site":{"page":"http://test.localhost:9999/integrationExamples/gpt/hello_world.html?pbjs_debug=true","domain":"test.localhost:9999","name":"test.localhost:9999"},"device":{"ua":"Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/80.0.3987.149 Safari/537.36","language":"en-US"},"user":null,"regs":null,"at":1,"ext":{"adot":{"adapter_version":"v1.0.0"},"should_use_gzip":true}}
Can you please take a look? We need to have a valid test bid to verify new adapters to ensure the end-to-end workflow is working properly.
In addition, you have some example test params for native and both video (outstream and instream) that I can use to test?
Please let me know if you have any questions about the above. Thanks!
modules/adotBidAdapter.js
Outdated
const openRTBImpression = serverRequest.data.imp.find(imp => imp.id === impressionId); | ||
const internalImpression = serverRequest._adot_internal.impressions.find(imp => imp.impressionId === impressionId); |
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.
The find
function isn't directly supported in IE11. There is an alternative polyfill you can import to use instead.
The import path would look like:
import find from 'core-js/library/fn/array/find.js';
You can refer to the appnexusBidAdapter.js
to see some examples of how it's used (like here).
Hi @jsnellbaker Sorry for the late reply. I've updated the code to remove the calls to Below you'll find the adUnits you can use to test the video (outstream & instream) as well as the native. Instream adUnitconst adUnit = {
code: 'test-div',
mediaTypes: {
video: {
context: 'instream',
playerSize: [[300, 250]]
}
},
bids: [{
bidder: 'adot',
params: {
video: {
mimes: ['video/mp4'],
minDuration: 5,
maxDuration: 35,
protocols: [2, 3],
instreamContext: 'pre-roll'
},
placementId: 'adot_prebidjs_integration'
}
}]
} Outstream adUnitconst adUnit = {
code: 'test-div',
mediaTypes: {
video: {
context: 'outstream',
playerSize: [[300, 250]]
}
},
bids: [{
bidder: 'adot',
params: {
video: {
mimes: ['video/mp4'],
minDuration: 5,
maxDuration: 35,
protocols: [2, 3]
},
placementId: 'adot_prebidjs_integration'
}
}]
} Native adUnitconst adUnit = {
code: 'test-div',
mediaTypes: {
native: {
image: {
required: false,
sizes: [100, 50]
},
title: {
required: false,
len: 140
},
sponsoredBy: {
required: false
},
clickUrl: {
required: false
},
body: {
required: false
},
icon: {
required: false,
sizes: [50, 50]
}
}
},
bids: [{
bidder: 'adot',
params: {
placementId: 'adot_prebidjs_integration'
}
}]
} |
Hi @mlequain Thanks for providing the updated information. I checked through each of the mediaTypes. Please see below for my feedback:
Can you take a look to see why the native bid isn't returning and about the feedback on the instream? |
…move duplicate code
Hi @jsnellbaker I have updated the code to make sure we either return a The outstream video is working fine on my end. I managed to bid on the native ad using your bid request, let me know if it's working on your end as well. |
Thanks for the follow-up. I retested the video instream and saw it was working. However, I tried the native test again, but I'm still not seeing a bid return. I noticed the sample params had an extra I have copied the updated request payload below, can you take a look and advise?
|
I have updated the adapter documentation and my previous comment to fix the mistake in the native snippet. I have also updated some targeting parameters on my end and I'm seeing bids for the native ad using your bid request. Let me know if everything is working on your end. Thanks a lot |
@mlequain Thanks for the additional update, I am getting a native bid back now. LGTM |
@jsnellbaker Thanks a lot for your help ! |
* add adot bidder adapter * remove promises and other methods to ensure IE11 compatibility and remove duplicate code Co-authored-by: Maxime Lequain <[email protected]>
* add adot bidder adapter * remove promises and other methods to ensure IE11 compatibility and remove duplicate code Co-authored-by: Maxime Lequain <[email protected]>
Type of change
Description of change
Add adot bidder adapter
Be sure to test the integration with your adserver using the Hello World sample page.
For any changes that affect user-facing APIs or example code documented on http://prebid.org, please provide: