Adding GumGum server adapter #748
Conversation
cchang320
force-pushed
the
master
branch
4 times, most recently
from
48c31ee to
01c1ed1
Compare
| } | ||
| request.Imp[i].Banner = &bannerCopy | ||
| validImps = append(validImps, request.Imp[i]) | ||
| trackingId = zone |
There was a problem hiding this comment.
You might want to do some checks if people are passing inconsistent values across the imps. Shouldn't happen in theory, but users can end up doing all sorts of crazy things.
There was a problem hiding this comment.
Is it possible for the user to send different values (unless they decide to randomly generate this value in which case none of these values would be valid on GumGum's end)? Since whatever is in the imp.ext object is hard coded when the user sets up the client for the site right?
There was a problem hiding this comment.
There is one imp.ext for each imp, so technically it could be possible to send inconsistent values. It would be a strong indication that there is something seriously wrong with their prebid implementation to generate a mess like that. I am not going to deny this PR if there is no check, just wanted to offer it as an idea to consider.
static/bidder-params/gumgum.json
Outdated
| "description": "A tracking id used to identify GumGum zone.", | ||
| "minLength": 8 | ||
| }, | ||
| "deviceHeight": { |
There was a problem hiding this comment.
Hey, I'm a little confused by deviceWidth and deviceHeight here.
Bidder params are generally defined by Publishers when they set up their page. The device size will change user-by-user... so how would a publisher define them?
Also: OpenRTB already has request.device.w and request.device.h for these... I see you copying it in your adapter. Shouldn't the code on the page (prebid.js or prebid-mobile) just define those when it makes the OpenRTB request? It doesn't seem like they're values that would change between adapters.
There was a problem hiding this comment.
Sure I can clarify. I was testing my adapter with my coworker who created a test page running the Prebid client and we noticed that the device object never got sent in the bid request. We spoke with a publisher that wants to call us through Prebid server and asked if they can send over the device info in the request and they told us that they do not control what gets sent over in the request since they run Prebid.js. So I was under the impression that Prebid.js is not sending the device info in the request.
Regarding how the publisher would set this up, the way my coworker defined the device width and height params was by using the functions window.innerWidth() and window.innerHeight().
Please let me know if there was perhaps some issue with our test page setup and the device info does actually get sent over through Prebid.js.
There was a problem hiding this comment.
Aha! Yeah, that makes sense. Thanks!
Your test page was probably fine... Prebid.js just isn't sending these yet. I opened prebid/Prebid.js#3336 to add them, though.
If you want to test with that branch, it'd be a big help. I hacked it out, but I'm pretty sure it'll fix this problem. I'll test it for real tomorrow, though.
There was a problem hiding this comment.
Gotcha! Will you be merging your changes soon? I’m out of office this week so I can test with your branch on Monday. If I still don’t see the device info included in the request, how should we proceed?
There was a problem hiding this comment.
ugh... so: yes, I plan to merge these changes as soon as possible. They're small, so I can nag the Prebid.js team for a review if necessary.
...but right now the tests don't pass, and I can't even build it locally because of prebid/Prebid.js#3330. The Prebid.js folks are waiting on avevlad/gulp-connect#259, which is outside of our control.
So... I definitely will try to get this in quickly, but can't promise a timeline either.
If you still don't see the device info included in the request, feel free to ping me on it. I do plan on testing myself, though, once the project is buildable again.
There was a problem hiding this comment.
Ah I see. I know you said it’s hard to give a timeline but do you think the issues will be resolved by the end of December? Trying to merge my adapter before the end of Q4 :) Also in the case you think it won’t be merged by the end of December, would it be terrible to merge my adapter as is and get rid of the device width and height in the ext object once your changes have been merged and the device info is being sent over?
There was a problem hiding this comment.
We merged it now :). It's live in version 1.35.0: https://github.com/prebid/Prebid.js/releases/tag/1.35.0
There was a problem hiding this comment.
Awesome news! Okay I will go ahead and remove the device width and height from the extension object and see how it goes. Thanks!
|
|
||
| request.Imp = validImps | ||
|
|
||
| if request.Site != nil { |
There was a problem hiding this comment.
I notice in your static/bidder-info that you also support apps... did you want to do something with the tracking ID for those here too? Maybe in req.App?
It seems a bit weird that the zone info doesn't make it anywhere into the request that goes to your servers on App requests.
There was a problem hiding this comment.
Sorry that's an oversight on my end. GumGum will only support site for now so I'll correct our bidder-info file. Thanks for catching that! If I modify that file will you have to reapprove?
There was a problem hiding this comment.
yeah, but it's fine. We're active :)
* gumgum adapter implementation * removing device info params since newest prebid client started sending over device info * secure endpoints for gumgum * removing app from gumgum bidder info
* gumgum adapter implementation * removing device info params since newest prebid client started sending over device info * secure endpoints for gumgum * removing app from gumgum bidder info
* gumgum adapter implementation * removing device info params since newest prebid client started sending over device info * secure endpoints for gumgum * removing app from gumgum bidder info
No description provided.