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

Add 1ad4good bidder #4081

Merged
merged 9 commits into from
Nov 11, 2019
Merged

Add 1ad4good bidder #4081

merged 9 commits into from
Nov 11, 2019

Conversation

vladgurgov
Copy link
Contributor

Type of change

Description of change

This is open source bidder for non profit ads that I created this weekend. Could be used to fill unsold inventory for a good causes.

More info about this project can be found here: http://1ad4good.org

I hope it will be net positive for this community as:

open source education about tech and header bidding
always filling (good for some test cases for ad ops
providing some free ad placements to non profits
Thank you!

  • test parameters for validating bids
{
  bidder: '1ad4good',
  params: {
    placementId: 13144370
  }
}

Be sure to test the integration with your adserver using the Hello World sample page. -DONE

  • contact email of the adapter’s maintainer vlad@aaaudi.com
  • [ X] official adapter submission

For any changes that affect user-facing APIs or example code documented on http://prebid.org, please provide:

Other information

updating fork from head
A bidder for non-profit free ads.
more info about this bidder project can be found on project site
http://1ad4good.org
@vladgurgov
Copy link
Contributor Author

@Fawke thank you for looking into it!

It's pretty straightforward, based on appnexus bidder code(for easier adops setup) with some unused code removed. nothing special..

Copy link
Contributor

@Fawke Fawke left a comment

Choose a reason for hiding this comment

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

Hey @vladgurgov,

I've been through the PR and made few observations that needs changes.

  • Your Test coverage is low (77.67%) which is less than our recommended 80% coverage. You can either remove unnecessary code from your Adapter file, or, you can add more test cases to cover more statements.
  • The call to your Ad Server is recommended to go over https. If your server supports it, please consider changing it over.
  • The call to load Instream video ad is not returning any bids. Please find the screenshot of the response.

Screenshot 2019-08-16 at 1 04 24 PM

@vladgurgov
Copy link
Contributor Author

Thanks for your comments @Fawke ! This was my first contribution to this project so I definitely was trying to obey all the rules.

I was developing based on appnexus adapter, so most of the code is the same.
In appnexusBidAdapter.js endpoint is
const URL = '//ib.adnxs.com/ut/v3/prebid';

in rubicon:
export const FASTLANE_ENDPOINT = '//fastlane.rubiconproject.com/a/api/fastlane.json';
export const VIDEO_ENDPOINT = '//prebid-server.rubiconproject.com/openrtb2/auction';

in mine its:
const URL = '//hb.1ad4good.org/prebid';

Should I hardcode https:// in it? I support https, i just wanted to follow the same rules.

Tests are based on appnexusBidAdapter.js which has 75.92% coverage... I will look into adding more tests

I don't bid on video yet, but i will add video campaign on server to make it bid on video as well.

Thanks for your feedback again!

test coverage is improved to >80%
tested for instream video support
@vladgurgov
Copy link
Contributor Author

  • Unused code was removed
  • Test coverages is over >80% now
  • Video campaign added on bidder for testing and tested for compatibility
  • Still waiting if endpoint should be hardcoded to https:// instead of //

Thanks!

@vladgurgov
Copy link
Contributor Author

@Fawke could you please take another look? Thank you!

@vladgurgov
Copy link
Contributor Author

@mkendall07 @Fawke could you take look, thanks!

@stale
Copy link

stale bot commented Sep 9, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Sep 9, 2019
@stale stale bot closed this Sep 16, 2019
@bretg bretg reopened this Oct 18, 2019
@stale stale bot removed the stale label Oct 18, 2019
@bretg
Copy link
Collaborator

bretg commented Oct 18, 2019

re-opening -- ping @Fawke

Copy link
Contributor

@Fawke Fawke left a comment

Choose a reason for hiding this comment

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

@vladgurgov Sorry for the delay in response, I've suggested some changes for you. Let me know if you can incorporate that in your code.

For your question on hardcoding the endpoint to https, yes, you can do that.

modules/1ad4goodBidAdapter.js Outdated Show resolved Hide resolved
modules/1ad4goodBidAdapter.js Outdated Show resolved Hide resolved
modules/1ad4goodBidAdapter.js Outdated Show resolved Hide resolved
modules/1ad4goodBidAdapter.js Outdated Show resolved Hide resolved
modules/1ad4goodBidAdapter.js Show resolved Hide resolved
modules/1ad4goodBidAdapter.js Outdated Show resolved Hide resolved
@stale
Copy link

stale bot commented Nov 5, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Nov 5, 2019
@stale stale bot removed the stale label Nov 8, 2019
@vladgurgov
Copy link
Contributor Author

@Fawke , please take a look again. I removed most of unused code.

Our initial idea with it was that we could simply use this module to test on client side easily by renaming appnexus -> 1ad4good. So that replacing one line immediately let us get fill from us and test code. And we simply wanted to make sure everything is as close to appnexus as possible.

However, I agree with you - since we dont use most of these params it makes sense to remove it. Please take a look. Thanks!

@Fawke
Copy link
Contributor

Fawke commented Nov 8, 2019

@vladgurgov

Thanks for making the required changes. There's just one last change which I suggest you make now, you can hardcode your endpoing URL to https in line:8 in 1ad4goodBidAdapter.js file.

In the next major release of Prebid, we are having all bidder adapters convert to https. Failing to do so will result in them not being included in the next major release.

@vladgurgov
Copy link
Contributor Author

@Fawke
Makes sense. Just updated to https://
Please keep in mind that appnexus module still using //

If this is accepted I can also add some sample pages and docs. I think it could be good beginners "entry-point" for Prebid and it would allow anyone to test and experiment Prebid without any accounts on Appnexus, Rubicon, their ad servers and still get 100% fill.

Let me know if you think it makes sense of you have other ideas for contributing to this project.

Thanks!

@Fawke Fawke merged commit 387f1c9 into prebid:master Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants