-
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
Update AOL adapter for v1.0 #1693
Conversation
Hi everyone. I reviewed new adapter requirements and found only one collision with this PR. It is related to user syncs. In 0.X.X our adapter has following features:
In this PR 2# and 3# are implemented via new Prebid userSyncs module. But I do not see any legal way to implement 1# because it requires globals and some changes in core modules. We would really like to keep all these features in 1.0. Let's discuss a way to do it. |
a49630d
to
7cc2754
Compare
@mjacobsonny @jaiminpanchal27 @ndhimehta Hi everyone. |
@@ -2,6 +2,8 @@ import {expect} from 'chai'; | |||
import * as utils from 'src/utils'; | |||
import AolAdapter from 'modules/aolBidAdapter'; | |||
import bidmanager from 'src/bidmanager'; |
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.
You will need to remove any references to the bidmanager
as it is no longer present in 1.0. You can see how I did this for the rubicon adapter here: https://github.com/prebid/Prebid.js/pull/1671/files
Also, all references to callBids should be removed as well since that API has changed. Tests should be refactored to test against the spec
directly, rather than the adapter interface. The adapter interface is tested in the bidderFactory spec.
src/adapters/bidderFactory.js
Outdated
@@ -180,13 +180,13 @@ export function newBidder(spec) { | |||
// After all the responses have come back, fill up the "no bid" bids and | |||
// register any required usersync pixels. | |||
const responses = []; | |||
function afterAllResponses() { | |||
function afterAllResponses(request) { |
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 going to work correctly. afterAllResponses
is fired once and you are passing in a singular request, which means if there are multiple requests, afterAllResponses
is only going to get the last one.
modules/aolBidAdapter.js
Outdated
} else if (isMarketplaceRequest) { | ||
apiUrl = _buildMarketplaceUrl(bid); | ||
} | ||
if (bidResponse && bidRequest && bidRequest.userSyncOn === constants.EVENTS.BID_RESPONSE) { |
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.
if you are using the bidRequest
just to get at bid.params
, perhaps you could use setConfig
and getConfig
instead in the config module?
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.
It'd probably be best to use the userSync API to check if user syncs should fire. The config is passed as the first paramteter to getUserSyncs
and can be used like this: https://github.com/prebid/Prebid.js/blob/master/modules/appnexusAstBidAdapter.js#L115
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.
Yes, we use it just for fetching the param.
It looks like we can create new namespace via new setConfig API and define it here.
setConfig({aol: { userSyncOn: 'bidResponse'}}});
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.
Yes you could. Is there a reason you want to do your own user sync flag as opposed to the general
pbjs.setConfig({ userSync: {
iframeEnabled: true
}});
and checking the syncConfig that's passed into getUserSyncs
?
src/adapters/bidderFactory.js
Outdated
@@ -249,7 +257,8 @@ export function newBidder(spec) { | |||
typeof request.data === 'string' ? request.data : JSON.stringify(request.data), | |||
{ | |||
method: 'POST', | |||
contentType: 'text/plain', | |||
contentType: request.contentType || 'text/plain', | |||
customHeaders: request.customHeaders || {}, |
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 ability to specify the contentType and customHeaders was already added here as request.options
…ol-adapter-for-1.0 # Conflicts: # src/adapters/bidderFactory.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.
@vzhukovsky Left some comments.
I am not able to validate bids by testing on hello_world
); | ||
} | ||
showCpmAdjustmentWarning = false; // warning is shown at most once | ||
bidRequest.bidderCode = bid.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.
Any reason to add these param to bidRequest
. I don't see its use.
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. Thanks for reviewing it.
We have an analytic adapter that is not part of Prebid official yet. Having "bidderCode" here allows us to break out the reporting between different aliases.
modules/aolBidAdapter.js
Outdated
_addErrorBidResponse(bid, response); | ||
return; | ||
} | ||
function interpretResponse(bidResponse, bidRequest) { |
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.
#1748 changed the first argument of interpretResponse
to:
body: responseBody,
headers: {
get: function(header) { /* returns a header from the HTTP response */ }
}
}```
modules/aolBidAdapter.js
Outdated
return bidder === AOL_BIDDERS_CODES.aol || bidder === AOL_BIDDERS_CODES.onedisplay; | ||
} | ||
return { | ||
bidderCode: bid.bidderCode, |
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.
Some bidResponse params are missing and invalid. Please check http://prebid.org/dev-docs/bidder-adapter-1.html
Please use the following setting for validating bids: |
@jaiminpanchal27 Hi. Comments are fixed and now you can validate the bids. |
@vzhukovsky You missed adding required param Also can you update |
@jaiminpanchal27 Hi. Thank you for reviewing it. I added |
return bidder === AOL_BIDDERS_CODES.aol || bidder === AOL_BIDDERS_CODES.onedisplay; | ||
} | ||
return { | ||
bidderCode: bidRequest.bidderCode, |
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.
Edit: nevermind!
* unstream/master: (36 commits) + Add Optimatic Bid Adapter (prebid#1837) Add Bridgewell adapter (prebid#1825) Kumma adapter updated for Prebid 1.0 (prebid#1766) Touchup add bid response (prebid#1822) Fix skipped test (prebid#1836) Added new size in Rubicon pbjs Adapter (prebid#1842) HuddledMasses header bidding adapter (prebid#1806) Increment pre version Prebid 0.33.0 Release Update AOL adapter for v1.0 (prebid#1693) Sovrn 1.0 compliance (prebid#1796) Platform.io Bidder Adapter update (prebid#1817) Drop non-video bidders from video ad units (prebid#1815) Update renderAd to replace ${AUCTION_PRICE} in adUrl (prebid#1795) Pulsepoint adapter: fixing bid rejection due to missing mandatory bid params. (prebid#1823) Remove require.ensure entirely (prebid#1816) Add custom keyword support for pbs bid adapter (prebid#1763) OpenX Video Adapter update to Prebid v1.0 (prebid#1724) Fix test that hard-coded pbjs global. (prebid#1786) Update Pollux Adapter to v1.0 (prebid#1694) ...
Looks like the Prebid.js PR updating AOL for 1.0 is here: prebid/Prebid.js#1693
….33.0 to aolgithub-master * commit '3e9756098bb20ecbe0314f16eed5298c5675b24c': (32 commits) Wrapped content type in options object. Added partners ids. Added changelog entry. Prebid 0.33.0 Release Update AOL adapter for v1.0 (prebid#1693) Sovrn 1.0 compliance (prebid#1796) Platform.io Bidder Adapter update (prebid#1817) Drop non-video bidders from video ad units (prebid#1815) Update renderAd to replace ${AUCTION_PRICE} in adUrl (prebid#1795) Pulsepoint adapter: fixing bid rejection due to missing mandatory bid params. (prebid#1823) Remove require.ensure entirely (prebid#1816) Add custom keyword support for pbs bid adapter (prebid#1763) OpenX Video Adapter update to Prebid v1.0 (prebid#1724) Fix test that hard-coded pbjs global. (prebid#1786) Update Pollux Adapter to v1.0 (prebid#1694) PubMatic adapter (prebid#1707) Added sizes to Rubicon Adapter (prebid#1818) jsonpFunction name should match the namespace (prebid#1785) Adding 33Across adapter (prebid#1805) Unit test fix (prebid#1812) ...
* Converted AOL bid adapter to Prebid 1.0. * Added description file for AOL Adapter. * Move one mobile post properties to options object. * Implemented AOL user syncs config via setConfig API. * Refactored Marketplace tests for Prebid 1.0. * Refactored One Mobile tests for Prebid 1.0. * Fixed faining tests for One Mobile endpoint. * Refactored get userSyncs tests for Prebid 1.0. * Refactored interpretResponse tests for Prebid 1.0. * Remove outdated tests. * Fixed review comments. * Added ttl support for AOL adapter. * Improved aol adapter docs.
Type of change
Description of change
AOL adapter changes for Prebid 1.0.