-
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
Support for Native in Pubmatic Adapter #3408
Conversation
Latest Pull
Latest Code
changes to support native in pubmaticbid adapter
documentation for native
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 changes in the files look fine. There are a few things to be reviewed however:
- code coverage > with these changes, the code coverage % for the adapter file dropped from 84.77% to 79.73%. While this is close to the usual preferred threshold (80%), would you be able to re-review the unit tests and see if there's something that can be put together to help close the gap?
- While attempting to test the native changes with the test params in the .md file, I wasn't getting a response from your server (received a 204 No Content). Further below is a copy of the request object that was executed - could you take a look to see if there's something incorrect?
Request's payload
{"id":"1546613828377","at":1,"cur":["USD"],"imp":[{"id":"2c932f813e502d","tagid":"pubmatic_test2","secure":0,"ext":{},"bidfloorcur":"USD","native":{"request":"{\"assets\":[{\"id\":2,\"required\":1,\"img\":{\"type\":3,\"w\":150,\"h\":50}},{\"id\":1,\"required\":1,\"title\":{\"len\":80}},{\"id\":4,\"required\":1,\"data\":{\"type\":1}}]}"}}],"site":{"page":"http://ap.localhost:9999/integrationExamples/gpt/demo_native.html?pbjs_debug=true","ref":"http://ap.localhost:9999/integrationExamples/gpt/demo_native.html?pbjs_debug=true","publisher":{"id":"156209"},"domain":"ap.localhost"},"device":{"ua":"Mozilla/5.0 (Linux; Android 6.0; Nexus 5 Build/MRA58N) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/71.0.3578.80 Mobile Safari/537.36","js":1,"dnt":0,"h":475,"w":1814,"language":"en-US","geo":{}},"user":{"geo":{}},"ext":{"wrapper":{"wv":"prebid_prebid_1.37.0-pre","transactionId":"ed23078a-0994-4340-9e75-4dab515a58e8","wp":"pbjs"}}}
Increased code coverage to 85%, updated pub id for which you will receive bids |
@pm-shashank-jain Thanks for making the updates; I confirmed the code coverage looks good now and the updated test params was returning a bid response. |
* changes to support native in pubmaticbid adapter * Removed port from endpoint * Removed protocol from endpoint * Formatting * Fix request payload * Updated test case * Changed request and response as per ortb spec * Change in request and response * Removed comments and extra code * Code Review comments * Code Review Comments and Test cases for request and response * Removed data type as all data asset types are handled * Code Review Changes * Code Review Comments * Supporting both banner and native and sending 0x0 in case of native * Bug Fixes * Bug response not processed by prebid * Change warning message * Fixed typo * Do not send request in case of invalid native bid * Do not send request in case of invalid native requests * objects converted to strings in log for debug purposes * Fixed logic to check for required parmas * Fixed typo for stringify * documentation for native * Review comments from Prebid * Typo * Typo * Updated pub id for native * Code Review
Type of change
Description of change
Support for native bid request in pubmatic bid adapter
For any changes that affect user-facing APIs or example code documented on http://prebid.org, please provide:
Other information
@pm-harshad-mane @mike-chowla