Advangelists Prebid Server Bidder

[trchandraprakash]

No description provided.

[trchandraprakash]

Hi @mansinahar, Let me know if any changes are needed and when we can make it available for download. Thanks

[mansinahar]

How is Pubid going to be different from PublisherID here?

[trchandraprakash]

Pubid is string.

[mansinahar]

Okay. Is it just a string of numbers or it could be a string of characters or mix of characters and numbers too? Even if that's the case, it might still be easier to have just PublisherID and make that a string rather than have PubID and PublisherID both. We can easily change an integer publisher ID to a string in that case.

[mansinahar]

Ah actually I see from your example json that PubID could be a mix of characters and numbers. In that case as suggested above, could you please rather change PublisherID to type string itself and the integer PublisherID values used before can be converted into string as needed?

[mansinahar]

@trchandraprakash pinging to see if you have had a chance to make the above change

[trchandraprakash]

@mansinahar I am OOO this week. Will update coming monday. Also if I change PublisherID macro to be a string, won't it impact other bidder adapters who are considering it as integer.

[mansinahar]

Well, yeah those values will have to be converted into string before setting them to PublisherID and this PR would have to do that work. From a quick look hopefully it shouldn't be a lot of changes though

[trchandraprakash]

Hi Mansi, Can you please tell what causing checks fail. Same command works on my environment.
./validate.sh --nofmt --cov --race 10

[mansinahar]

Hi @trchandraprakash! Looks like there are formatting issues. I'd suggest running go fmt on your branch and then updating the PR. Specifically the build is complaining about openrtb_ext/imp_advangelists.go. That should hopefully resolve the issue.

[trchandraprakash]

Got it thanks. Let me know if it looks good.

[trchandraprakash]

Hi, Please let me know if it is all good, I have to make it available asap. thanks.

[trchandraprakash]

Hi @guscarreon, I have made requested changes, can you please help in finalizing review, so that i can make it available to our clients asap.

[mansinahar]

@trchandraprakash Thanks for addressing all the feedback. If you could please make the PubID change when you have a chance, we can definitely help merge and close this ASAP.

[trchandraprakash]

Hi @mansinahar, was not sure if i should modify other bidder code, but i guess without that i cannot pass automated check. Have done some modifications here

1. Updated macro PublisherID to be string.
2. Updated my bidder "Advangelists" code to use PublisherID macro instead of PubID.
3. Updated another bidder "adkernelAdn" to accept PublisherID as string.
   Let me know if this is good. Thanks.

[mansinahar]

@trchandraprakash I think this PR should just convert the PublisherID in the "AdKernel" adapter right before the ResolveMacros function is called here: https://github.com/prebid/prebid-server/blob/master/adapters/adkernelAdn/adkernelAdn.go#L203. Something like:

pubIDStr := strconv.Itoa(params.PublisherID)
endpointParams := macros.EndpointTemplateParams{Host: reqHost, PublisherID: pubIDStr}
return macros.ResolveMacros(adapter.EndpointTemplate, endpointParams)

The reason being, the change that you've made is fine but is sort of a big change for their adapter and we would have to get their approval also before merging it in. That will just delay this more for you so we can rather do this simple workaround for now and they can submit another PR changing their adapter to accept a string PublisherID

[trchandraprakash]

@mansinahar I see what you mean, i have made requested changes and reverted back others.

[mansinahar]

LGTM! Thanks @trchandraprakash

[trchandraprakash]

@mansinahar when we can see it available on production for client. Also not sure why i still see status of this PR as changes requested.

[mansinahar]

@trchandraprakash I think those changes requested are from Gus's review earlier which I believe you have already addressed but Github probably doesn't have a way to know that so it's still showing that. We can merge and close once we get a second approval from @guscarreon too.

[guscarreon]

LGTM

[trchandraprakash]

Thanks @mansinahar @guscarreon for approval. Can you please suggest how or when adapter will be available on this page http://prebid.org/dev-docs/prebid-server-bidders.html.

[mansinahar]

@trchandraprakash We are looking to cut a new release on our end tomorrow so it should be available in prod thereafter. We will keep you updated. As for it appearing on http://prebid.org/dev-docs/prebid-server-bidders.html, I think @bretg might be responsible for that or at least should be able to point to the person who is.

[trchandraprakash]

@mansinahar thanks will wait for your update. @bretg can you please help on getting adapter on http://prebid.org/dev-docs/prebid-server-bidders.html.

[mansinahar]

@trchandraprakash FYI: We cut a new tag - 0.76.0 yesterday but unfortunately were having some issues with the release and had to rollback. We're planning to roll it out again on Monday and will keep you posted.

[trchandraprakash]

@mansinahar thanks for update.

[bretg]

For the record, the list of adapters on http://prebid.org/dev-docs/prebid-server-bidders.html is dynamically generated from adapters that are deployed to AppNexus' cluster of servers. So when that deploy succeeds, the adapter will be on the list.

[trchandraprakash]

Thanks @bretg .
Hi @mansinahar, Any updates, can we expect adapter to be available today?

[mansinahar]

@trchandraprakash The adapter is now available in production and on the list of adapters on http://prebid.org/dev-docs/prebid-server-bidders.html

[trchandraprakash]

@mansinahar thanks a lot.