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

Kargo adapter BidID support #2958

Merged
merged 2 commits into from
Aug 14, 2018
Merged

Kargo adapter BidID support #2958

merged 2 commits into from
Aug 14, 2018

Conversation

samuelhorwitz
Copy link
Contributor

Type of change

  • Bugfix
  • Feature
  • New bidder adapter
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Does this change affect user-facing APIs or examples documented on http://prebid.org?
  • Other

Description of change

This updates the adapter to point to a new bid endpoint. The endpoint is versioned so that the old endpoint and payload will still work. The new endpoint accepts placements mapped by prebid bid IDs. This allows multiples of the same placement per request/response.

Other information

This is a fix for the adapter regarding #2873

Copy link
Collaborator

@jsnellbaker jsnellbaker left a comment

Choose a reason for hiding this comment

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

@samuelhorwitz
When attempting to test the changes with the test params in the .md file, I wasn't seeing any content return from the server. Specifically, the server returned an empty object {}.

Below is a copy of the request that was generated from the testpage using the new endpoint:

https://krk.kargo.com/api/v2/bid?json=%7B%22timeout%22%3A3000%2C%22cpmGranularity%22%3A1%2C%22timestamp%22%3A1534167259555%2C%22cpmRange%22%3A%7B%22floor%22%3A0%2C%22ceil%22%3A20%7D%2C%22bidIDs%22%3A%7B%2222346f67ee75e5%22%3A%22_m1Xt2E5dez%22%7D%2C%22userIDs%22%3A%7B%22crbIDs%22%3A%7B%7D%7D%2C%22krux%22%3A%7B%22userID%22%3Anull%2C%22segments%22%3A%5B%5D%7D%2C%22pageURL%22%3A%22http%3A%2F%2Fap.localhost%3A9999%2FintegrationExamples%2Fgpt%2Fhello_world.html%3Fpbjs_debug%3Dtrue%22%2C%22rawCRB%22%3Anull%7D

Does look correct? Do the test params need to be updated in lieu of the new endpoint? Please confirm as we'd like to ensure the adapter is working fine when changes like this are made before merging.

@samuelhorwitz
Copy link
Contributor Author

Sorry, let me get that working. That's because we aren't bidding on that payload but I can get something set up for a working payload. {} is a good sign though, it's not a failure, it's an explicit no bid.

@samuelhorwitz
Copy link
Contributor Author

Alright, try it now. Remember, you have to use a mobile browser user agent or else it won't work, but otherwise it's up and running.

@jsnellbaker
Copy link
Collaborator

jsnellbaker commented Aug 14, 2018

@samuelhorwitz thanks for providing the feedback.

I am seeing something return currently, however I'm encountering a different problem around the currency field. It doesn't seem like the value is being populated correctly in the interpretResponse function.

There are two aspects of the problem I encountered:

  1. When I setup a basic page without the currency module configured, there seemed to be no default currency value within the bid object generated by the interpretResponse function. A 3-digit country code string needs to be specified for the bid to be accepted (see here for table of needed values for bids.) Can you add a fallback value for this scenario?

  2. When I did configure the currency module for a test page, I think the adapter is grabbing the entire currency config object and passing that to the bid, instead of the just actual country code. I was seeing an Warning message like the following:

utils.js:270 WARNING: Returning NO_BID, getCurrencyConversion threw error:  : params :  Error: Specified fromCurrency '[object Object]' not found in the currency rates file
    at getCurrencyConversion (currency.js:243)
    at currency.js:180
    at processBidResponseQueue (currency.js:170)
    at Object.addBidResponseHook (currency.js:164)
    at Object.asyncSeries (hook.js:42)
    at Object.hookedFn (hook.js:77)
    at addBidWithCode (bidderFactory.js:172)
    at addBidUsingRequestMap (bidderFactory.js:309)
    at Array.forEach (<anonymous>)
    at Object.onSuccess [as success] (bidderFactory.js:298)

This happened when it was trying to add the bid to the auction.

In lieu of the above two, could you perhaps modify the currency field in the interpretResponse function's bid object to something like this?
currency: (bidRequest.currency && bidRequest.currency.adServerCurrency) || 'USD'

@samuelhorwitz
Copy link
Contributor Author

Alright I fixed it with 51a1218

This fixes it at the point where I apparently was mishandling it (in the buildRequests function where it will trickle through the rest of the code after pulling from the config). Let me know if that solution makes sense to you.

@jsnellbaker
Copy link
Collaborator

@samuelhorwitz Thanks for the updates; they appear to be working fine now.

@jsnellbaker jsnellbaker merged commit 4ba1d45 into prebid:master Aug 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants