-
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
Pollux Adapter #1431
Pollux Adapter #1431
Conversation
Added module, test spec and integration example for Pollux Network Bid Adapter
Update Pollux default domain on prebid adapter
On Utils.js make getParameterByName method public
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.
its ok
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.
@polluxnetwork Was able to verify bids back and ad render.
I have left some comments. Please check
@@ -0,0 +1,105 @@ | |||
<!DOCTYPE html> |
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.
Move this example page to integrationExamples/gpt/pollux_zone_728x90.html
modules/polluxBidAdapter.js
Outdated
placementCode = bidObj.placementCode; | ||
} | ||
if (bidObj && response.cpm > 0 && !!response.ad) { | ||
bidObject = bidfactory.createBid(STATUS.GOOD); |
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.
pass bidRequest
as second parameter. This will match request response pairs
modules/polluxBidAdapter.js
Outdated
bidObject.width = response.width; | ||
bidObject.height = response.height; | ||
} else { | ||
bidObject = bidfactory.createBid(STATUS.NO_BID); |
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.
pass bidRequest
as second parameter. This will match request response pairs
sinon.stub(bidManager, 'addBidResponse'); | ||
const adLoaderStub = sinon.stub(adLoader, 'loadScript'); | ||
|
||
describe('callBids', () => { |
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.
Please add more test cases. Your test coverage seems just 54.1 %. Make it atleast >85
To view test coverage run 1) gulp test-coverage
2) gulp view-coverage
Moved zone_728x90.html to integrationExamples/gpt/pollux_zone_728x90.html; Added bidRequest as second parameter to bidfactory.createBid() on Pollux Bid Adapter; Added more test cases to increase test coverage (at least 85%); Review Ref: - prebid#1431 (review)
@jaiminpanchal27 Just pushed the changes you requested. Please review them at your best convenience, and thanks a lot for your help. |
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.
Changes in module are good. I have left some comments for unit tests. Please check.
Thanks.
adUnits.push(unit); | ||
|
||
if (typeof ($$PREBID_GLOBAL$$._bidsRequested) === 'undefined') { | ||
$$PREBID_GLOBAL$$._bidsRequested = [params]; |
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.
Avoid using
For reference you can check these unit tests:
https://github.com/prebid/Prebid.js/blob/master/test/spec/modules/pulsepointBidAdapter_spec.js
}); | ||
|
||
it('should return complete bid response ad (html)', function() { | ||
var stubAddBidResponse = sinon.stub(bidmanager, 'addBidResponse'); |
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.
stub creation and its restore should always be in beforeEach and afterEach hooks. Reason being if some test fails and the next test case is trying to stub the same method than you will get following error from sinon
TypeError: Attempted to wrap method which is already wrapped
- Removed $$PREBID_GLOBAL$$ public vars in unit test; - Moved stubs creation and its restoration to beforeEach and afterEach hooks in unit test; - Exposed polluxHandler method on polluxBidAdapter.
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
This line was added in prebid#1409, removing this then I'll merge
* Added PolluxNetwork Bid Adapter Added module, test spec and integration example for Pollux Network Bid Adapter * Update Pollux domain Update Pollux default domain on prebid adapter * Export getParameterByName method On Utils.js make getParameterByName method public * Executed changes requested by @jaiminpanchal27 on 2017-08-01 Moved zone_728x90.html to integrationExamples/gpt/pollux_zone_728x90.html; Added bidRequest as second parameter to bidfactory.createBid() on Pollux Bid Adapter; Added more test cases to increase test coverage (at least 85%); Review Ref: - prebid#1431 (review) * Fixed Eslint errors on commit f745fe1 * Executed changes requested on PR#1431 review #54993573 - Removed $$PREBID_GLOBAL$$ public vars in unit test; - Moved stubs creation and its restoration to beforeEach and afterEach hooks in unit test; - Exposed polluxHandler method on polluxBidAdapter. * Remove redundant export This line was added in prebid#1409, removing this then I'll merge
* Added PolluxNetwork Bid Adapter Added module, test spec and integration example for Pollux Network Bid Adapter * Update Pollux domain Update Pollux default domain on prebid adapter * Export getParameterByName method On Utils.js make getParameterByName method public * Executed changes requested by @jaiminpanchal27 on 2017-08-01 Moved zone_728x90.html to integrationExamples/gpt/pollux_zone_728x90.html; Added bidRequest as second parameter to bidfactory.createBid() on Pollux Bid Adapter; Added more test cases to increase test coverage (at least 85%); Review Ref: - #1431 (review) * Fixed Eslint errors on commit f745fe1 * Executed changes requested on PR#1431 review #54993573 - Removed $$PREBID_GLOBAL$$ public vars in unit test; - Moved stubs creation and its restoration to beforeEach and afterEach hooks in unit test; - Exposed polluxHandler method on polluxBidAdapter. * Remove redundant export This line was added in #1409, removing this then I'll merge * Update Pollux Adapter to v1.0 * Changes requested on Pollux Adapter pull request #1694 review #74933409 * Changes requested on Pollux Adapter pull request #1694 review #75505070 Rmoved parameter bidderCode from bid responses * Fixed breaking changes to serverResponse in interpretResponse method Parameter serverResponse of method interpretResponse in bid adapter changed from array of bids to an object, where bids are now nested within its parameter body. Plus a refactor of var declaration and log messages. * Fix lint errors on push for commit cc653a
Type of change
Description of change
Adding an adapter for Pollux Network, LLC.
Added module, test spec and integration example for Pollux Network Bid Adapter.
Be sure to test the integration with your adserver using the Hello World sample page.
There is also a pollux test page with the parameters already set.