-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Ventes Bid Adapter: add new bid adapter #7525
Conversation
This pull request introduces 1 alert when merging 2939458 into 80bc6e2 - view on LGTM.com new alerts:
|
Please advise next step to add bid Adapter in prebid.js. Thanks |
@jessoventes can you please add unit testing for this new adapter? |
@jessoventes I see. If device.ip is required for a valid bid, you'll want to add that to your dev docs so users know: https://github.com/prebid/prebid.github.io/pull/3309/files And from the perspective of publishers trying to use your bidder, making IP required is going to limit how many publishers can implement your bidder. Many publishers are not creating their own scripts and are not capable of making them dynamic by page load. That's just my opinion on it at least. |
@Rothalack Even i observed other adapters, i too believe ip will restrict publishers. Let me go through the server side changes to remove ip so that even without ip, we can go ahead. Thanks for the suggestion!! |
@Rothalack @ChrisHuie Required changes to serve request without ip is done at our server, can you please go ahead and check and confirm if everything is fine? |
@ChrisHuie @Rothalack Any update for us here? |
Hey @jessoventes I was able to test this morning. I'm getting a new error when trying to request a bid:
logError @ utils.js:272 |
Can you share the request as well? |
Here's a request that gave the same error: For publisherId, I tried both 5cebea3c9eea646c7b623d5e and VA-062 and both give the same error. Is there other required params other than placementId and publisherId? The other params are listed as optional in your docs. |
@Rothalack Thank you to address about the error!! |
When testing on desktop in browser using |
@Rothalack I tried testing using a html page sample code provided on the official website and used adUtils as below: Also, i checked yesterday's logs i am able to see 4 request to our server was responded with ad. |
I'm getting a different error now. I'm using the hello_world test page in integrationExamples/gpt/hello_world.html Testing with 300x250 size. placementId VA-002-0007-0799, publisherId 5cebea3c9eea646c7b623d5e
The stack trace is: The error object is: The TypeError is showing this;
What I'm thinking is how you've set up device to be interpreted, it's still being "required". |
Fixed the issue.Can you check now? |
I tested again and was only getting a server error from your end. So what I did next was get my ifa id from my personal phone and added device.ifa to the bid request. Once I added that, I started getting ads served. So I can call this good to go and working. It looks like your adapter is in build/dev. I'm honestly unsure how you can build into the prebid file. I don't see why you would need too either. I think you're configured properly. |
|
You will want to make sure your docs over here: prebid/prebid.github.io#3309 are accurate and up to date. I won't hold up this PR for that. It looks like we've missed the cut off for release this week. Apologies for that, I recently reinstalled my OS on this machine and had to get a prebid dev environment set up and running on it today, it never works first try of course. So I will be merging this in shortly and it should go out next week. |
Thanks @Rothalack . Yes, once it is release we could make required changes in case of any issue faced by publishers. Meanwhile, I will confirm the docs and update it if required. |
@Rothalack Just wanted to check, the release date for this merge. Thanks!! |
Normally releases on Weds. Assuming there's no issues that pop up, should be out tomorrow. |
Ok. Thanks. |
@Rothalack In the release of 6.0.0, i am not able to see Bid adapter of "ventes", can you please help me here. |
You're in the previous release |
Ok. Now able to see, Thanks |
* Ventes Avenues initial changes * Ventes Avenues initial changes * Ventes Avenues Solved issues * Ventes Avenues Solved issues * Ventes Avenues Solved issues * Ventes Avenues Solved issues * Ventes Avenues Solved issues * Ventes Avenues Solved issues * Ventes Avenues Solved issues * Ventes Avenues Solved issues * Ventes Avenues Solved issues * Ventes Avenues Solved issues * Ventes Avenues Solved issues * Ventes Avenues Solved issues * Ventes Avenues Solved issues * Ventes Avenues Solved issues * Ventes Avenues Solved issues * Ventes Avenues Solved issues * Ventes Avenues Solved issues * Ventes Avenues Solved issues * Ventes Avenues Solved issues * Ventes Avenues Solved issues * Ventes Avenues Solved issues * Ventes Avenues Solved issues * Ventes Avenues Solved issues * Ventes Avenues Solved issues * Ventes Avenues Solved issues * Ventes Avenues Solved issues * Ventes Avenues Solved issues * Ventes Avenues Solved issues * Ventes Avenues Solved issues * Ventes Avenues Solved issues * Ventes Avenues Solved issues * Ventes Avenues Solved issues for user agent * Added few more info * Ventes Avenues Solved issues for user agent
Type of change
Description of change
Be sure to test the integration with your adserver using the Hello World sample page.
For any changes that affect user-facing APIs or example code documented on http://prebid.org, please provide:
Other information