Kargo Adapter for Prebid 1.0 by samuelhorwitz · Pull Request #1729 · prebid/Prebid.js

samuelhorwitz · 2017-10-19T18:57:08Z

Description of change

Kargo Adapter migrated to Prebid 1.0

Notes

As a side note, I would like some clarification on currency support. Does the publisher always specify conversion rates? We have multiple currency support being added into our server-side code and I would like to know the best way to configure things so that our rates our used when the pub specifies a compatible non-USD currency. We will be returning our CPMs in the currency the publisher specifies already converted on our end. I'm unsure however of how I should implement this in the adapter. Thank you!

dbemiller · 2017-10-23T20:07:30Z

hey @samuelhorwitz, is there any reason you made this PR against the prebid-1.0 branch? Since you're using the bidderFactory, you should be able to update in master directly.

This helps us with branch maintenance, because otherwise all the patches & bugfixes to your adapter between now and the 1.0 release will become merge conflicts.

Currency

It looks like you're doing the right thing to me. Import the config, read the currency from it in buildRequests, and respond with the bid's currency when you interpretResponse.

@snapwich may want to overrule me here, as he's more familiar with the module... but your code here looks like it should work to me.

samuelhorwitz · 2017-10-23T20:42:12Z

I can change the base of this PR, but should I actually rebase my branch off of master? If I change the base without rebasing, this PR will become much more complicated to review, but if I rebase from master I will be working off of a branch that doesn't include any of the 1.0 updates

dbemiller · 2017-10-23T20:48:11Z

You should create a branch from master, and open a PR against master.

The 1.0 branch doesn't change any adapter-facing APIs in the bidderFactory... and in fact breaking changes to bidderFactory are aimed at master too (see #1748), so if anything prebid-1.0 will be farther out of date.

currency in the config exists in master too... so I don't think your code should change very much (if at all) between the two.

dbemiller · 2017-10-23T20:08:03Z

modules/kargoBidAdapter.js

No need to define this if it's an empty array.... you can just leave it out.

dbemiller · 2017-10-23T20:12:08Z

modules/kargoBidAdapter.js

sync pixels need to be added using the userSync module

You can also implement getUserSyncs, in which case the bidderFactory will put them in the userSync module for you. This would probably be easier for you to test.

Either way, prebid-core will attach them to the page after the auction's over.

When do user syncs fire? We can just leave this off if it's not an immediate thing... we use this tracker to log that an ad unit was received by the page but if it's not fired in a deterministically "soon" fashion then it's better we just leave it off.

Aha... ok. Not a user sync at all, then.

Yeah... there's no real good way to do that. Publishers get annoyed by libraries making extra requests because of the impact it has on page load times.

You could use the user sync module if you want... by default, we fire them 3 seconds after the auction has completed.

However, it still wouldn't be reliable. Since it affects the publishers' page performance, Prebid made this configurable... so they can limit the number that fire, opt out totally, etc.

Ah okay cool, I will remove it then

Sure thing.

If this is a feature you guys really need, I'd recommend opening an issue about it. I don't know how the Prebid.JS leadership feels about supporting this, but that's a good way to get discussion going with some more eyes on it.

I've created a ticket here #1762 but it's not urgent for us to have this support and I've just removed the code

dbemiller · 2017-10-23T20:14:03Z

modules/kargoBidAdapter.js

You can make these truly private if you pull them off the spec object, and just define them as helper functions in the top level of your file.

If they're not exported, they're safe.

For some reason I thought I mocked these more than I did in tests so I made them "private" as opposed to out of closure scope. I don't think I actually do, though. That being said, I don't really mind either way, I added that comment to visually separate the interface functionality from the behind the scenes functionality.

dbemiller · 2017-10-24T12:45:39Z

modules/kargoBidAdapter.js

Our docs are bad here, but... don't send back the bidderCode on bids. The bidderFactory adds it to you, and this won't work quite right if someone aliases your adapter (which can be done by customers at runtime)

mkendall07 · 2017-10-24T13:42:46Z

modules/kargoBidAdapter.js

what's this cookie you are reading? How do you have access to it if you are in the pub domain?

These are first party cookies our pubs are likely to already have on their page from our various other integrations.

ok that's what I thought. Thanks for clarifying.

samuelhorwitz · 2017-10-24T15:08:18Z

So, I just modified this PR to be derived from master/pointing to master. However I will need to re-run some of my own tests to make sure all my changes still work.

dbemiller · 2017-10-24T15:09:35Z

Sounds good, let me know when you're comfortable with it.

samuelhorwitz · 2017-10-24T16:59:58Z

Okay everything seems good from our tests! Thank you

dbemiller

Looks good to me. I left a few non-essential suggestions I just noticed while testing things... but not a big deal.

dbemiller · 2017-10-24T18:59:33Z

modules/kargoBidAdapter.md

Looks like you're missing a comma here.

dbemiller · 2017-10-24T19:02:28Z

modules/kargoBidAdapter.md

You might want to mention something about how this only works on mobile.

I tried this and thought I wasn't getting a bid back... but I put my browser in mobile mode and then it worked. Didn't realize that was a dependency. Others may run into the same problem.

I updated this as well but its below the line commented on so Github didnt dismiss it

dbemiller · 2017-10-24T20:28:19Z

agh... sorry about this, but one more change, because #1742 got merged before we could get this in.

The first argument to interpretResponse now looks like this:

{
  body: responseBody,
  headers: {
    get: function(header) { /* returns a header from the HTTP response */ }
  }
}

You'll have to update the spec so that it looks through that object as well now.

samuelhorwitz · 2017-10-24T22:01:20Z

Alright, done!

samuelhorwitz · 2017-10-25T15:26:57Z

Hey, sorry about this but now I have one last thing I need to add also! It should be a very quick/small change to what we send to our bidder.

dbemiller · 2017-10-25T15:27:35Z

no worries... do what you've gotta do ^^

samuelhorwitz · 2017-10-25T16:04:24Z

Alright finished again!

* '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) ...

* 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) ...

jaiminpanchal27 assigned dbemiller Oct 20, 2017

jaiminpanchal27 added the needs review label Oct 20, 2017

mjacobsonny added the Prebid 1.0 label Oct 20, 2017

dbemiller suggested changes Oct 24, 2017

View reviewed changes

dbemiller reviewed Oct 24, 2017

View reviewed changes

dbemiller added needs update and removed needs review labels Oct 24, 2017

mkendall07 reviewed Oct 24, 2017

View reviewed changes

samuelhorwitz changed the base branch from prebid-1.0 to master October 24, 2017 15:03

samuelhorwitz mentioned this pull request Oct 24, 2017

Tracker extension points for non-usersync trackers #1762

Closed

dbemiller approved these changes Oct 24, 2017

View reviewed changes

dbemiller added LGTM and removed needs update labels Oct 24, 2017

dbemiller added needs update and removed LGTM labels Oct 24, 2017

dbemiller added LGTM and removed needs update labels Oct 25, 2017

Kargo Adapter for Prebid 1.0

bc05f37

dbemiller merged commit ebaec7a into prebid:master Oct 26, 2017

aneuway2 mentioned this pull request Jun 18, 2018

Kargo - Indicate that it is Mobile only demand on bidders.html prebid/prebid.github.io#840

Closed

Conversation

samuelhorwitz commented Oct 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of change

Notes

Uh oh!

dbemiller commented Oct 23, 2017

Currency

Uh oh!

samuelhorwitz commented Oct 23, 2017

Uh oh!

dbemiller commented Oct 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbemiller Oct 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samuelhorwitz Oct 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samuelhorwitz commented Oct 24, 2017

Uh oh!

dbemiller commented Oct 24, 2017

Uh oh!

samuelhorwitz commented Oct 24, 2017

Uh oh!

dbemiller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbemiller commented Oct 24, 2017

Uh oh!

samuelhorwitz commented Oct 24, 2017

Uh oh!

samuelhorwitz commented Oct 25, 2017

Uh oh!

dbemiller commented Oct 25, 2017

Uh oh!

samuelhorwitz commented Oct 25, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Comments

samuelhorwitz commented Oct 19, 2017 •

edited

Loading

dbemiller Oct 23, 2017 •

edited

Loading

samuelhorwitz Oct 24, 2017 •

edited

Loading