Skip to content
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.

Sign up for GitHub

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

set bid response id to sovrn impid #498

Merged
merged 9 commits into from
Aug 8, 2016
Merged

set bid response id to sovrn impid #498

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c82ceb4
set bid response id to sovrn impid
Jul 29, 2016
1eb9f94
no need to match on "best effort" if `bid.adId` is set
Aug 2, 2016
8d26f91
Sovrn adapter creates bid response with ID of bid request
Aug 4, 2016
e59858d
whitespace fix
Aug 4, 2016
d082eaf
code style fixes
Aug 5, 2016
3ecea76
use sinon fake server request and response
Aug 5, 2016
7c821ed
test bid object and props exist
Aug 8, 2016
62ac5eb
troubleshoot sinon fake request
Aug 8, 2016
83a8844
remove sinon fake request/response approach and just test the bid id …
Aug 8, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/adapters/sovrn.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ var SovrnAdapter = function SovrnAdapter() {

//store bid response
//bid status is good (indicating 1)
bid = bidfactory.createBid(1);
bid = bidfactory.createBid(1, bidObj);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be required/implemented for all adapters? This issue is not isolated to Sovrn, it is just potentially more commonly setup there and was noticed first. aardvark and triplelift are two additional examples where we will run into this issue in our current setup as we setup flex sizes for them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are working towards this approach as a requirement for adapters but opted not to implement by core team as it became too obtrusive to modify adapter code ourselves.

bid.creative_id = sovrnBid.id;
bid.bidderCode = 'sovrn';
bid.cpm = responseCPM;
Expand Down
8 changes: 4 additions & 4 deletions src/bidfactory.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ var utils = require('./utils.js');
dealId,
priceKeyString;
*/
function Bid(statusCode) {
var _bidId = utils.getUniqueIdentifierStr();
function Bid(statusCode, bidRequest) {
var _bidId = bidRequest && bidRequest.bidId || utils.getUniqueIdentifierStr();
var _statusCode = statusCode || 0;

this.bidderCode = '';
Expand Down Expand Up @@ -49,6 +49,6 @@ function Bid(statusCode) {
}

// Bid factory function.
exports.createBid = function (statusCode) {
return new Bid(statusCode);
exports.createBid = function () {
return new Bid(...arguments);
};
9 changes: 6 additions & 3 deletions src/bidmanager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,13 @@ function getBidSetForBidder(bidder) {
exports.addBidResponse = function (adUnitCode, bid) {
if (bid) {
//first lookup bid request and assign it back the bidId if it matches the adUnitCode
let bidRequest = getBidSetForBidder(bid.bidderCode).bids.find(bidRequest => bidRequest.placementCode === adUnitCode);
if(bidRequest && bidRequest.bidId) {
bid.adId = bidRequest.bidId;
if (!bid.adId) {
let bidRequest = getBidSetForBidder(bid.bidderCode).bids.find(bidRequest => bidRequest.placementCode === adUnitCode);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the best effort here, however since this is so critical to the functionality of prebid is there a way we can standardize this? Why would some bidResponses have an adId, but others would not?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bid responses vary and there was initially not much standardization as Prebid is a wrapper around existing header bidding tech. It has since become more evident that there should be more standards to improve header bidding solutions generally, and matching requested bid IDs with bid response IDs is certainly a standard we'd like to see adopted.

if (bidRequest && bidRequest.bidId) {
bid.adId = bidRequest.bidId;
}
}

Object.assign(bid, {
requestId: getBidSetForBidder(bid.bidderCode).requestId,
responseTimestamp: timestamp(),
Expand Down
2 changes: 1 addition & 1 deletion src/prebid.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,7 +375,7 @@ $$PREBID_GLOBAL$$.setTargetingForGPTAsync = function () {
utils.logError('window.googletag is not defined on the page');
return;
}

//first reset any old targeting
getPresetTargeting();
resetPresetTargeting();
Expand Down