-
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
sekindoUM for prebid1.0 #1777
sekindoUM for prebid1.0 #1777
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.
LGTM
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 was a breaking change added recently that requires an update to the interpretResponse
function, and a few other notes for your review
modules/sekindoUMBidAdapter.js
Outdated
var bidsCount = bids.length; | ||
|
||
import * as utils from 'src/utils'; | ||
// import {config} from 'src/config'; |
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.
Line can be removed if not used
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.
done
modules/sekindoUMBidAdapter.js
Outdated
* @param {*} serverResponse A successful response from the server. | ||
* @return {Bid[]} An array of bids which were nested inside the server. | ||
*/ | ||
interpretResponse: function(serverResponse, 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 */ }
}
}
so adding something like
serverResponse = serverResponse.body;
just below this line, or however you'd prefer to grab the body
, and updating corresponding tests if needed should get this back to working properly
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.
done
modules/sekindoUMBidAdapter.js
Outdated
* @return boolean True if this is a valid bid, and false otherwise. | ||
*/ | ||
isBidRequestValid: function(bid) { | ||
if (bid.mediaType == 'video') { |
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.
As of #1483, video ad units may be defined with mediaTypes: {video: {}}
and would be on this bid
parameter, you'll probably want to check for that as well
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.
done
modules/sekindoUMBidAdapter.js
Outdated
queryString = utils.tryAppendQueryString(queryString, 'hbcb', '1');/// legasy | ||
queryString = utils.tryAppendQueryString(queryString, 'dcpmflr', bidfloor); | ||
queryString = utils.tryAppendQueryString(queryString, 'protocol', protocol); | ||
if (bidRequest.mediaType === 'video') { |
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.
May want to check for mediaTypes.video
as well, see other comment for related info
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.
done
@nissSK I saw your updates to the feedback from @matthewlane, but I don't see that the changes were committed. Are those changes still in the works on your end? Please confirm when you get the chance. Thanks! |
@jsnellbaker Should be ok now (pushed changes to prebid1.0 instead of master) |
@nissSK Changes look good but the 405 file changes are a bit concerning. Looks chmod related, if those can be resolved so there are just the 3 sekindo file changes we should be good for merge |
@matthewlane Sure, didnt notice that. |
* 'master' of https://github.com/prebid/Prebid.js: (43 commits) Merge Prebid 1.0 to Master (prebid#1936) Prebid.js 0.34.1 release Vertamedia adapter outstream support (prebid#1860) Expose native image-type asset dimensions on bid response object (prebid#1919) Remove for of (prebid#1932) Unit-test fix (prebid#1927) Remove description_url (prebid#1922) Trion Interactive Adapter Bugfix (prebid#1925) Remove config setting from Optimatic adapter (prebid#1909) IE bug fix (prebid#1930) Clarify ad unit media filtering warning (prebid#1903) Add ReadPeak Bid Adapter (prebid#1838) Change clone function to make deep copies (prebid#1910) Serverbid 1.0 (prebid#1802) sekindoUM for prebid1.0 (prebid#1777) update auctionId to be requestId (prebid#1896) bug fixed to populate userSync default values (prebid#1897) Increment pre version AdkernelAdn analytics adapter (prebid#1868) Justpremium Adapter: use `filter` instead of `...new Set` (prebid#1895) ...
….34.0 to aolgithub-master * commit 'f0ba90afa8b52de7a646d43928b2d51ee42e74a1': (53 commits) Added changelog entry. Added partners ids. Added dynamic ttl property for One Display and One Mobile. Prebid.js 0.34.1 release Vertamedia adapter outstream support (prebid#1860) Expose native image-type asset dimensions on bid response object (prebid#1919) Remove for of (prebid#1932) Unit-test fix (prebid#1927) Remove description_url (prebid#1922) Trion Interactive Adapter Bugfix (prebid#1925) Remove config setting from Optimatic adapter (prebid#1909) IE bug fix (prebid#1930) Clarify ad unit media filtering warning (prebid#1903) Add ReadPeak Bid Adapter (prebid#1838) Change clone function to make deep copies (prebid#1910) Serverbid 1.0 (prebid#1802) sekindoUM for prebid1.0 (prebid#1777) update auctionId to be requestId (prebid#1896) bug fixed to populate userSync default values (prebid#1897) Increment pre version ...
Type of change
Description of change
sekindoUM for prebid1.0
Be sure to test the integration with your adserver using the Hello World sample page.
Other information