-
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
Justpremium Adapter for 1.0 #1881
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.
Thanks for opening the PR against master. A few changes requested
modules/justpremiumBidAdapter.js
Outdated
import { registerBidder } from 'src/adapters/bidderFactory' | ||
|
||
const BIDDER_CODE = 'justpremium' | ||
const ENDPOINT_URL = top.document.location.protocol + '//pre.ads.justpremium.com/v/2.0/t/xhr' |
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.
top
should generally be in a try/catch block, or use utils.getTopWindowLocation()
to safely get location
here
modules/justpremiumBidAdapter.js
Outdated
return parseInt(b.params.zone) | ||
}))].join(','), | ||
hostname: top.document.location.hostname, | ||
protocol: top.document.location.protocol.replace(':', ''), |
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.
Can also use utils.getTopWindowLocation()
in the above two lines
modules/justpremiumBidAdapter.js
Outdated
sw: window.top.screen.width, | ||
sh: window.top.screen.height, | ||
ww: window.top.innerWidth, | ||
wh: window.top.innerHeight, |
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 above four window.top
references will need to be in a try/catch block, or can set up a variable that access window.top
, place that within a try/catch, and access it here
} | ||
document.cookie = name + '=' + value + expires + '; path=/'; | ||
} | ||
import { expect, assert } from 'chai' |
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.
assert
isn't used, can be removed from import
import { expect, assert } from 'chai' | ||
import { spec } from 'modules/justpremiumBidAdapter' | ||
import { getTopWindowLocation } from 'src/utils' | ||
import { newBidder } from 'src/adapters/bidderFactory' |
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 above two imports aren't used in the test, can be removed
} | ||
}, | ||
|
||
interpretResponse: (serverResponse, bidRequests) => { |
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
modules/justpremiumBidAdapter.js
Outdated
let size = (adUnit.sizes && adUnit.sizes.length && adUnit.sizes[0]) || [] | ||
let bidResponse = { | ||
requestId: bidId, | ||
bidderCode: spec.code, |
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.
bidderCode
will be set by bidderFactory
automatically now, this line can be dropped
modules/justpremiumBidAdapter.js
Outdated
ad: bid.adm, | ||
cpm: bid.price, | ||
currency: bid.currency || 'USD', | ||
ttl: bid.ttl || spec.time |
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.
netRevenue
and creativeId
will also need to be in this object to pass validation, see the table in http://prebid.org/dev-docs/bidder-adapter-1.html#interpreting-the-response for a description of these fields
@matthewlane all requested changes were applied. |
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.
with the most recent changes, LGTM
* Justpremium adapter and unit tests. * Fix test suit. * Performance improvements. * Changes requested in pull request review. * Register justpremium adapter in adaptermanager * pass through bid from request * fix linting errors * Load polyfills for older browsers * Load polyfills if older browser * Remove package-lock.json * Copy new Justpremium adapter from feature/1.0 branch * #1881 Requested changes applied * #1892 Use `filter` instead `...new Set` to get unique values
* Justpremium adapter and unit tests. * Fix test suit. * Performance improvements. * Changes requested in pull request review. * Register justpremium adapter in adaptermanager * pass through bid from request * fix linting errors * Load polyfills for older browsers * Load polyfills if older browser * Remove package-lock.json * Copy new Justpremium adapter from feature/1.0 branch * #1881 Requested changes applied * #1892 Use `filter` instead `...new Set` to get unique values * JSD-2248 update for adapter and tests * JSD-2248 added transactionId in json array * JSD-2248 adapter changes * JSD-2248 adapter changes * Update docs * Remove unnecessary return statement * Support for gdpr_consent in bid adapter * new cookie link and endpoint * back to old endpoint version * sending version of prebid and adapter * sending version of prebid and adapter * without version * update for tests * changes for getUserSyncs method * return gulpfile changes
Type of change
Description of change
@matthewlane @ndhimehta @snapwich I created new pr as after merging latest master to branch used in previous one some files were just deleted causing that tests failed. This one contains exactly the same code, so we will be very grateful for quick approval.