-
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
New Adapter: bidglass #3861
New Adapter: bidglass #3861
Conversation
Docs PR prebid/prebid.github.io#1328 |
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 submitting your adaptor. I tested your adaptor on hello-world page and found functionality working fine. But you haven't added any definition to functions like getUserSyncs(), onTimeout(), onBidWon() and onSetTargeting(). Would you want to add something to them, may be just a log.
Also, the unit test coverage is slightly below the required criteria of 80% (its 79.xx %), can you add a few more tests for the same.
Can you please make the minor changes I have mentioned, and let me know if you have any questions.
Thanks.
modules/bidglassBidAdapter.js
Outdated
* @param {ServerResponse[]} serverResponses List of server's responses. | ||
* @return {UserSync[]} The user syncs which should be dropped. | ||
*/ | ||
getUserSyncs: function() {}, |
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 formal params are missing here. Can you please add some code, may be just a log in function definition.
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 taking the time to review our adapter! We've made the updates as requested. One question - is there a gulp (or other) command to check the coverage for an individual bidder's unit tests?
A note regarding the empty function definitions - we don't plan on using getUserSyncs, onTimeout, onBidWon, or onSetTargeting to any practical effect in the near future.
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 the update. You can use gulp test-coverage
, and then gulp view-coverage
to view unit test-coverage of any adaptor file. The view command will open coverage on http://localhost:1999/
, where you can navigate to your file.
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!
- Added formal params to getUserSyncs function definition - getUserSyncs now always returns an array - Improved unit test coverage
Tested the adaptor on hello_world page and all unit tests have passed. Test coverage of the adaptor is 80%. The changes are working well against all parameters listed in https://github.com/prebid/Prebid.js/blob/master/PR_REVIEW.md, hence approving the PR. |
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.
@dliebner Please see comments.
modules/bidglassBidAdapter.js
Outdated
* @param {ServerResponse[]} serverResponses List of server's responses. | ||
* @return {UserSync[]} The user syncs which should be dropped. | ||
*/ | ||
getUserSyncs: function(syncOptions, serverResponses) { |
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.
getUserSyncs, onTimeout, onBidWon and onSetTargeting are all optional. You can remove from the spec if you do not plan to use these features
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.
Got it, thanks.
*/ | ||
|
||
let imps = []; | ||
let getReferer = function() { |
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.
We already pass this information in bidderRequest. Check http://prebid.org/dev-docs/bidder-adaptor.html#referrers
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.
Our rules for determining and reporting referer and origins vary slightly from prebid's built-in methods. We'd prefer to use our own methods for consistency's sake across our platform.
modules/bidglassBidAdapter.js
Outdated
bidResponses.push({ | ||
requestId: bid.requestId, | ||
cpm: parseFloat(bid.cpm), | ||
width: parseInt(bid.width), |
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.
parseInt takes two parameters string and radix. I would suggest to add radix as it does not default to 10 and may return unexpected results.
More info here https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/parseInt and interesting article on parseInt https://medium.com/dailyjs/parseint-mystery-7c4368ef7b21
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, good call.
modules/bidglassBidAdapter.js
Outdated
* @return {Bid[]} An array of bids which were nested inside the server. | ||
*/ | ||
interpretResponse: function(serverResponse) { | ||
// const serverBody = serverResponse.body; |
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.
remove dead 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.
Will do. Commit pending.
- Removed unused methods: getUserSyncs, onTimeout, onBidWon, onSetTargeting - Removed getUserSyncs unit test - Removed "dead code" - Removed some unnecessary comments - Fixed usage of parseInt
Hey, is there anything else you need from us here? |
* Added bidglass adapter + test * PR Review Updates: - Added formal params to getUserSyncs function definition - getUserSyncs now always returns an array - Improved unit test coverage * PR Review Updates: - Removed unused methods: getUserSyncs, onTimeout, onBidWon, onSetTargeting - Removed getUserSyncs unit test - Removed "dead code" - Removed some unnecessary comments - Fixed usage of parseInt
* Added bidglass adapter + test * PR Review Updates: - Added formal params to getUserSyncs function definition - getUserSyncs now always returns an array - Improved unit test coverage * PR Review Updates: - Removed unused methods: getUserSyncs, onTimeout, onBidWon, onSetTargeting - Removed getUserSyncs unit test - Removed "dead code" - Removed some unnecessary comments - Fixed usage of parseInt
Type of change
Description of change
Added the bidglass adapter + tests
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:
Other information