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

New adapter: Colombia #5158

Merged
merged 21 commits into from
Apr 27, 2020
Merged

New adapter: Colombia #5158

merged 21 commits into from
Apr 27, 2020

Conversation

ColombiaOnline
Copy link
Contributor

Type of change

  • New adapter

Description of change

We have uses some conditions for banner sizes.

@ColombiaOnline
Copy link
Contributor Author

@musikele
Thanks for quick response,
I have fresh commit and resolved all issues. please reviews changes and push the file in prebid repository asap .

@musikele musikele self-requested a review April 23, 2020 14:09
Copy link
Contributor

@musikele musikele left a comment

Choose a reason for hiding this comment

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

I still see 3 issues:

  1. change package-lock.json to match exactly what we have on master
  2. when running npx gulp test, there are two errors related to colombia adapter

package-lock.json Show resolved Hide resolved
'ad': '<div>This is test case for colombia adapter</div>'
}];
let result = spec.interpretResponse(serverResponse, bidRequest[0]);
expect(Object.keys(result)).to.deep.equal(Object.keys(expectedResponse));
Copy link
Contributor

Choose a reason for hiding this comment

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

When Running npx gulp test, I get this error:

✖ should get the correct bid response
        HeadlessChrome 81.0.4044 (Mac OS X 10.15.4)
      TypeError: Cannot convert undefined or null to object
          at keys (<anonymous>)
          at Function.keys (node_modules/es5-shim/es5-shim.js:1145:24)
          at Context.<anonymous> (webpack:///test/spec/modules/colombiaBidAdapter_spec.js:134:21 <- test/test_index.js:113581:67)

}
};
let result = spec.interpretResponse(response, bidRequest[0]);
expect(result.length).to.equal(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

When running npx gulp test, I get this other error:

✖ handles empty bid response
        HeadlessChrome 81.0.4044 (Mac OS X 10.15.4)
      TypeError: Cannot read property 'length' of undefined
          at Context.<anonymous> (webpack:///test/spec/modules/colombiaBidAdapter_spec.js:149:21 <- test/test_index.js:113595:67)

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.

Hi @ColombiaOnline,

  1. Two tests are still failing on the running the command. gulp test (Also the CI is failing because of that). Please find the screenshot for your reference:

Screenshot 2020-04-24 at 4 56 28 PM

  1. In the colombiaBidAdapter.md file, you need to change your example ad unit. With the release of Prebid v3.0 we've removed the property sizes to declare ad unit sizes. Instead, use the mediaTypes property. For example, if you are serving Banner ads, it should look like,
[{
    code: 'test-ad-div',
    mediaTypes: {
       banner: {
         sizes: [[300, 250]]
       }
    },
    bids: [{
    bidder: 'colombia',
      params: { 
        placementId: '307466'
      }
    }]
}];
  1. In colombiaBidAdapter.js file, you are missing this property, supportedMediaTypes, which is an array of media types your bidder supports. Please refer to the appnexusBidAdapter.js for reference.

  2. Is there any particular reason to modify cpm value. I three instances in which I see modifications made to the cpm value.

  1. Docs PR is missing. Please create a PR against this repo, https://github.com/prebid/prebid.github.io. Follow instructions given at this page! http://prebid.org/dev-docs/bidder-adaptor.html#submitting-your-adapter

  2. Your test-coverage is 44%. We require minimum 80% test coverage.

@ColombiaOnline
Copy link
Contributor Author

Hi @Fawke ,
Thanks for suggestions
I have resolved all issues and quick inline comments for your points.
1: Resolved all test cases errors
2: MediaTypes- Property sizes removed and add mediaType objects in colombiaBidAdapter.md file
3: Add supportedMediaTypes variable in colombiaBidAdapter.js
4: It's done to adjust the bid value considering the viewability factor of the adslots.
5: I have already created docs PR.Please find below path
https://github.com/ColombiaOnline/prebid.github.io/blob/master/dev-docs/bidders/colombia.md
6: Please again running the command gulp test

Please reviews changes and push the files in prebid repository asap .

@Fawke Fawke merged commit a883653 into prebid:master Apr 27, 2020
iggyfisk pushed a commit to happypancake/Prebid.js that referenced this pull request Jun 22, 2020
* 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

* Use conditions for the size

* Use conditions for the size and remove test warnings

* remove package.json

* Resolved import related errors

* update referrer method

* remove package json and update referer path

* revert package json

* Resolved all changes

* update version package json

* Resolved test cases issues
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.

3 participants