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 Colombia adapter #2975

Merged
merged 7 commits into from
Sep 18, 2018
Merged

Add Colombia adapter #2975

merged 7 commits into from
Sep 18, 2018

Conversation

ColombiaOnline
Copy link
Contributor

Type of change

  • Bugfix
  • Feature
  • New bidder adapter
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Does this change affect user-facing APIs or examples documented on http://prebid.org?
  • Other

Description of change

Add Colombia adapter for Prebid .
Some test parameters for the bidder:

var adUnits = [{
    code: 'test-ad-div',
    sizes: [[300, 250]],
    bids: [{
    bidder: 'colombia',
      params: { 
        placementId: '44082'
       }
     }]
  }];

Our general contact email address is colombiaonline@timesinteret.in, and for anything related to this bidder, feel free to reach out to me at colombiaonline@timesinteret.in.

Other information

@jsnellbaker
Copy link
Collaborator

jsnellbaker commented Aug 14, 2018

@ColombiaOnline When you get the chance please perform the following:

  • rebase off of master (this should fix the circleci errrors)
  • add unit tests for your adapter (please refer to this page for some additional details and a link to see how other adapters setup their unit tests).
  • create a PR in the docs repo and add your bidder info under here. Feel free to look at how other adapters setup their files if you need a reference (eg appnexus or rubicon).

@idettman
Copy link
Contributor

idettman commented Aug 15, 2018

CircleCI tests are failing, I re-ran but tests failed again.

Copy link
Contributor

@idettman idettman left a comment

Choose a reason for hiding this comment

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

code LGTM

@ColombiaOnline
Copy link
Contributor Author

@idettman
We have fixed the circleci & LGTM errors. please continue to next process for merge.

@bretg
Copy link
Collaborator

bretg commented Aug 23, 2018

docs PR at prebid/prebid.github.io#937

@bretg bretg removed the needs docs label Aug 23, 2018
@stale
Copy link

stale bot commented Sep 6, 2018

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 6, 2018
@ColombiaOnline
Copy link
Contributor Author

we are working on conflict and unit test cases.

@stale stale bot removed the stale label Sep 7, 2018
@ColombiaOnline
Copy link
Contributor Author

We have successfully resolved the conflict.

Copy link
Collaborator

@jsnellbaker jsnellbaker left a comment

Choose a reason for hiding this comment

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

Hi @ColombiaOnline

There are still some items to address in this PR before it can be merged.

Please see below:

  • when I tested the bid params from the .md file in the hello_world.html page, I saw the following response instead of an actual banner ad:
{"cpm":0.0,"width":0,"height":0,"currency":"USD","netRevenue":true,"ttl":0}

Could you look into returning an actual test ad consistently?

  • Additionally the page loaded with a CORS error (see attachment for details of the message).

  • The unit tests for your adapter file are still missing. Can you please put this file together and add it into the PR?

  • See the point in the package.json file; additionally, please try to undo the change that was made in the package-lock.json file.

Please let me know if you have any questions.

package.json Outdated
@@ -104,6 +104,7 @@
"babel-plugin-transform-object-assign": "^6.22.0",
"core-js": "^2.4.1",
"gulp-sourcemaps": "^2.6.0",
"just-clone": "^1.0.2"
"just-clone": "^1.0.2",
"natives": "^1.1.4"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why was this natives package added in? It should be removed if there's no need for it.

@jsnellbaker
Copy link
Collaborator

See screenshot for CORS related error mentioned above:

screen shot 2018-09-12 at 9 37 50 am

@jsnellbaker jsnellbaker assigned jsnellbaker and unassigned idettman Sep 12, 2018
@ColombiaOnline
Copy link
Contributor Author

@jsnellbaker we have updated the code covering all the points pointed by you. Please review.

@jsnellbaker
Copy link
Collaborator

Hi @ColombiaOnline

Thanks for making the various updates. I tested the adapter again and found it working successfully now (no CORS errors).

However, I noticed the natives package is still present in the package.json file. If this is intentional, can you please elaborate on this change? If not, please remove the extra package from the file and we should be good to merge.

Thanks.

@jsnellbaker
Copy link
Collaborator

@ColombiaOnline Actually, I forgot to mention something in the previous update. In regards to the spec.js you added - in the various unit test functions, can you please convert the ,() => { arrow syntax to the standard , function() { syntax?

This styling change was recently implemented across the project in an effort to better comply with Mocha standards. So we'd like new adapters to conform with the change. Please see #2987 for reference on the change.

@ColombiaOnline
Copy link
Contributor Author

@jsnellbaker we have updated the code covering all the points pointed by you. Please review

@jsnellbaker
Copy link
Collaborator

@ColombiaOnline Thanks for the additional changes; LGTM.

@jsnellbaker jsnellbaker merged commit 6d51add into prebid:master Sep 18, 2018
ptomasroos pushed a commit to happypancake/Prebid.js that referenced this pull request Sep 25, 2018
* Add colombia adapter

* Add https in the url

* update files

* add test cases and update md file

* remove native from package.json and update colombia test cases
StefanWallin pushed a commit to mittmedia/Prebid.js that referenced this pull request Sep 28, 2018
* Add colombia adapter

* Add https in the url

* update files

* add test cases and update md file

* remove native from package.json and update colombia test cases
SublimeJeremy pushed a commit to SublimeSkinz/Prebid.js that referenced this pull request Oct 1, 2018
* Add colombia adapter

* Add https in the url

* update files

* add test cases and update md file

* remove native from package.json and update colombia test cases
ghost pushed a commit to devunrulymedia/Prebid.js that referenced this pull request Jan 30, 2019
* Add colombia adapter

* Add https in the url

* update files

* add test cases and update md file

* remove native from package.json and update colombia test cases
pedrolopezmrf pushed a commit to Marfeel/Prebid.js that referenced this pull request Mar 18, 2019
* Add colombia adapter

* Add https in the url

* update files

* add test cases and update md file

* remove native from package.json and update colombia test cases
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants