Skip to content
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

Update GetIntent adapter to 1.0 version #1721

Merged
merged 5 commits into from
Oct 30, 2017

Conversation

kprokopchik
Copy link
Contributor

@kprokopchik kprokopchik commented Oct 18, 2017

Type of change

  • Other

Description of change

GetIntent adapter for Prebid.js 1.0 version

  • test parameters for validating bids
{
  bidder: 'getintent',
  params: {
    pid: '7',
    tid: 'test01',
    // ...
  }
}

Other information

@matthewlane matthewlane assigned matthewlane and unassigned ndhimehta Oct 26, 2017
Copy link
Collaborator

@matthewlane matthewlane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dhargo Thanks for the updated adapter, a few changes requested. Also when testing bid responses with the test parameters, I'm receiving a 404 error from your endpoint

http://px.adhigh.net/rtb/direct_banner?pid=12345&tid=9876&known=1&is_video=false&resp_type=JSON&size=300x250& 404 (Not Found)

Please advise so we can validate bid responses, thanks

* @param {BidRequest} bidRequest
* @return {Bid[]} An array of bids which were nested inside the server.
*/
interpretResponse: function(serverResponse, bidRequest) {
Copy link
Collaborator

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 doing something like

serverResponse = serverResponse.body

(or however you'd prefer) in this function should give you the expected behavior

let size = parseSize(serverResponse.size);
let bid = {
requestId: bidRequest.bidId,
bidderCode: spec.code,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bidderCode can be dropped from the bid response object, it'll be set automatically by bidderFactory. Docs just updated, sorry for the confusion

};
if (bidRequest.sizes) {
// TODO: add support for multiple sizes
giBidRequest.size = bidRequest.sizes[0].join('x');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adunit sizes may be defined as [300, 250], in which case sizes[0] would be 300 and cause a TypeError. Be sure to check that bidRequest.sizes[0] is an array before calling join

if (bidRequest.creative_id) bid.creativeId = bidRequest.creative_id;
if (bidRequest.mediaType === 'video') {
bid.vastUrl = serverResponse.vast_url;
bid.descriptionUrl = serverResponse.vast_url;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bid.descriptionUrl is no longer required, bid.vastUrl alone will work now

const bids = [];
if (serverResponse && serverResponse.no_bid !== 1) {
let size = parseSize(serverResponse.size);
let bid = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few of the required bid response fields are missing: ttl, netRevenue, and currency. See the table in this section http://prebid.org/dev-docs/bidder-adapter-1.html#interpreting-the-response for descriptions and examples of each of these fields

@kprokopchik
Copy link
Contributor Author

@matthewlane Thanks for the comments. I've commited fixes. Here is new parameters to use for testing bid response

http://px.adhigh.net/rtb/direct_banner?pid=7&tid=test01&known=1&is_video=false&resp_type=JSON&size=300x250& 

Copy link
Collaborator

@matthewlane matthewlane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, no longer getting the 404 error with those parameters, but still a few issues with requestId and creativeId

height: size[1],
mediaType: bidRequest.mediaType || 'banner'
};
if (bidRequest.creative_id) bid.creativeId = bidRequest.creative_id;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is your endpoint able to always return a creativeId? That's a required parameter, and if it doesn't get set this won't pass validation. It might also be on responseBody rather than bidRequest

if (responseBody && responseBody.no_bid !== 1) {
let size = parseSize(responseBody.size);
let bid = {
requestId: bidRequest.bidId,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bidId didn't appear to be on the bidRequest object during tests I ran. I was able to get bid responses from your endpoint, but because the requestId wasn't getting set, it made it appear that the bid from this adapter was empty

return {
method: 'GET',
url: buildUrl(giBidRequest),
data: giBidRequest,
Copy link
Collaborator

@matthewlane matthewlane Oct 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should set bidId: bidRequest.bidId so that the interpretResponse function has access to it

@kprokopchik
Copy link
Contributor Author

@matthewlane Thank you for the comments. Fixed missing fields (requestId, creativeId).

Have to notice: method signature interpretResponse: function(serverResponse, request) and example in documentation http://prebid.org/dev-docs/bidder-adapter-1.html looks like we always have BidRequest (request parameter) when interpreting server bid response; so it is unclear why do we have to pass requestId to server and back.

@matthewlane
Copy link
Collaborator

Sorry for the confusion about that. The second parameter to interpretResponse is the bid request object which is available for use if needed. This object is whatever was returned the buildRequests function. Bid request and response objects need to have matching ids and one way of doing that is returning the id from buildRequests, but they can also be sent and received from the server as you've done here. In any case I was able to verify bids with your adapter so this is good for merge

@matthewlane matthewlane merged commit ac6775b into prebid:master Oct 30, 2017
Millerrok pushed a commit to Vertamedia/Prebid.js that referenced this pull request Oct 31, 2017
* 'master' of https://github.com/prebid/Prebid.js: (22 commits)
  Update GetIntent adapter to 1.0 version (prebid#1721)
  Add `usePaymentRule` param to AN bidders (prebid#1778)
  New hooks API (replaces monkey-patching for currency) (prebid#1683)
  Change prebidServer to call client user syncs if they exist (prebid#1734)
  Fix Centro adapter to allow requests of the same units (prebid#1746)
  add vastUrl + media type for video bids Prebid Server (prebid#1739)
  Update adxcg adapter for prebid 1.0 (prebid#1741)
  Update yieldmoBid adapter request url (prebid#1771)
  Upgrade Quantcast adapter for Prebid 1.0 (prebid#1753)
  Fidelity Media Adapter update. Prebid v1.0 (prebid#1719)
  Kargo Adapter for Prebid 1.0 (prebid#1729)
  updated for prebid 1.0 api (prebid#1722)
  Add AdOcean adapter (prebid#1735)
  Update Conversant adapter to Prebid 1.0 (prebid#1711)
  Fix test-coverage bug (prebid#1765)
  Migrating TrustX adapter to 1.0 (prebid#1709)
  Update Improve Digital adapter for Prebid 1.0 (prebid#1728)
  Fixed the argument type on getUserSyncs. (prebid#1767)
  nanointeractive bid adapter (prebid#1627)
  Validating bid response params (prebid#1738)
  ...
mattpr pushed a commit to mattpr/Prebid.js that referenced this pull request Oct 31, 2017
* AD-2311: Make GetIntent adapter compatible with Prebid.js 1.0 version

* AD-2311: remove blank line

* Trigger

* GetIntent adapter - added bid response fields: currency, ttl and netRevenue; fixed creative size selector (prebid#1721)

* GetIntent adapter - added bid response fields: bidId, creativeId (prebid#1721)
outoftime pushed a commit to Genius/Prebid.js that referenced this pull request Nov 13, 2017
* tag '0.32.0' of https://github.com/prebid/Prebid.js: (44 commits)
  Prebid 0.32.0 Release
  Commenting out tests that are failing in IE10 (prebid#1710)
  Update dfp.buildVideoUrl to accept adserver url (prebid#1663)
  Update rubicon adapter with new properties and 1.0 changes (prebid#1776)
  Added adUnitCode for compatibility (prebid#1781)
  Remove 'supported' from analytics adapter info (prebid#1780)
  Add TTL parameter to bid (prebid#1784)
  Update GetIntent adapter to 1.0 version (prebid#1721)
  Add `usePaymentRule` param to AN bidders (prebid#1778)
  New hooks API (replaces monkey-patching for currency) (prebid#1683)
  Change prebidServer to call client user syncs if they exist (prebid#1734)
  Fix Centro adapter to allow requests of the same units (prebid#1746)
  add vastUrl + media type for video bids Prebid Server (prebid#1739)
  Update adxcg adapter for prebid 1.0 (prebid#1741)
  Update yieldmoBid adapter request url (prebid#1771)
  Upgrade Quantcast adapter for Prebid 1.0 (prebid#1753)
  Fidelity Media Adapter update. Prebid v1.0 (prebid#1719)
  Kargo Adapter for Prebid 1.0 (prebid#1729)
  updated for prebid 1.0 api (prebid#1722)
  Add AdOcean adapter (prebid#1735)
  ...
vzhukovsky added a commit to aol/Prebid.js that referenced this pull request Dec 28, 2017
….32.0 to aolgithub-master

* commit '7a956351d69ed3a54ec31aaef17a36441be16fbd': (58 commits)
  Fixed wrong contentType and customHeader options.
  Fixed aol adapter bad merge (accepted official changes)
  Fixed issue with invalid webpack module.
  Added partners ids.
  Added changelog entry.
  Refactored interpretResponse tests for Prebid 1.0.
  Prebid 0.32.0 Release
  Commenting out tests that are failing in IE10 (prebid#1710)
  Update dfp.buildVideoUrl to accept adserver url (prebid#1663)
  Update rubicon adapter with new properties and 1.0 changes (prebid#1776)
  Added adUnitCode for compatibility (prebid#1781)
  Remove 'supported' from analytics adapter info (prebid#1780)
  Add TTL parameter to bid (prebid#1784)
  Refactored get userSyncs tests for Prebid 1.0.
  Fixed faining tests for One Mobile endpoint.
  Refactored One Mobile tests for Prebid 1.0
  Refactored Marketplace tests for Prebid 1.0
  Implemented AOL user syncs config via setConfig API.
  Move one mobile post properties to options object.
  Update GetIntent adapter to 1.0 version (prebid#1721)
  ...
dluxemburg pushed a commit to Genius/Prebid.js that referenced this pull request Jul 17, 2018
* AD-2311: Make GetIntent adapter compatible with Prebid.js 1.0 version

* AD-2311: remove blank line

* Trigger

* GetIntent adapter - added bid response fields: currency, ttl and netRevenue; fixed creative size selector (prebid#1721)

* GetIntent adapter - added bid response fields: bidId, creativeId (prebid#1721)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants