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.

Sign up for GitHub

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

Jump to bottom

Added C1X Adapter #1369

Merged
merged 6 commits into from
Aug 4, 2017
Merged

Added C1X Adapter #1369

merged 6 commits into from
Aug 4, 2017

Conversation

CathyHuangtw
Copy link
Contributor

@CathyHuangtw CathyHuangtw commented Jul 11, 2017 •
edited
Loading

Type of change

  • New bidder adapter

Description of change

  • test parameters for validating bids
{
  bidder: 'c1x'
}

Note

  • test ad unit
var adUnits = [
    {
         code: 'div-gpt-ad-1498193081995-0',
         sizes: [[300, 250], [300, 600]],
         bids: [
             {
                      bidder: 'c1x',
                      params: {
                          siteId: '999'
                      }
              }
         ]
    }
];

@CathyHuangtw
Copy link
Contributor Author

Hi, would you be able to do a review on our adapter this week? Please let me know if you have further questions :) Thank you.

@matthewlane matthewlane self-assigned this Jul 18, 2017
matthewlane
matthewlane requested changes Jul 19, 2017
View reviewed changes
Copy link
Collaborator

@matthewlane matthewlane left a comment

Choose a reason for hiding this comment

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

Thanks for the adapter. I was able to verify bid responses and tests passing. A few changes requested below

modules/c1xBidAdapter.js Outdated
},
BIDDER_CODE = 'c1x';

var pbjs = window.pbjs;
Copy link
Collaborator

Choose a reason for hiding this comment

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

The global variable name may be configured by the publisher as something other than pbjs, so here use window.$$PREBID_GLOBAL$$ instead of window.pbjs

modules/c1xBidAdapter.js Outdated
bidmanager.addBidResponse(data.adId, bidObject);
} else {
// no bid
utils.logInfo(LOG_MSG.nobid + data.adId);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Capitalize the b to LOG_MSG.noBid to match the case defined on line 25

modules/c1xBidAdapter.js Outdated
var response = c1xResponse;

if (typeof c1xResponse === CONSTANTS.objectType_string) {
response = JSON.parse(c1xResponse);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please wrap JSON.parse in a try/catch block to catch any parsing exceptions. See appnexusAstBidAdapter or other adapters for examples of this if needed

modules/c1xBidAdapter.js Outdated
var data = response[i],
bidObject = null;
if (data.bid) {
bidObject = bidfactory.createBid(1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Prefer CONSTANTS.STATUS.GOOD in place of 1

modules/c1xBidAdapter.js Outdated
};

function noBidResponse() {
var bidObject = bidfactory.createBid(2);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Prefer CONSTANTS.STATUS.NO_BID in place of 2

matthewlane
matthewlane previously approved these changes Jul 20, 2017
View reviewed changes
@CathyHuangtw CathyHuangtw mentioned this pull request Jul 22, 2017
Merged
@CathyHuangtw
Copy link
Contributor Author

@matthewlane Thank you for your review. I've also added a doc for our bidder to update on Prebid website in this repo. Will our adapter be merged soon and be included in your next release? Thanks!

dbemiller
dbemiller suggested changes Aug 3, 2017
View reviewed changes
Copy link
Contributor

@dbemiller dbemiller left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hey @CathyHuangtw. Code looks good, except for one small issue in the tests.

Some ESLint PRs have been merged since you last pushed, though, which makes our code style a bit more strict than it was when you last pushed. Once you pull & fix them, we can merge it.

test/spec/modules/c1xBidAdapter_spec.js Outdated
};

describe('c1x adapter tests: ', () => {
let pbjs = window.pbjs || {};
Copy link
Contributor

Choose a reason for hiding this comment

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

should use $$PREBID_GLOBAL$$ too.

Copy link
Contributor Author

@CathyHuangtw CathyHuangtw Aug 4, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

thanks for the reminder. Already corrected this for the global naming convention.

@CathyHuangtw
Copy link
Contributor Author

@dbemiller thank you for taking the time to review my adapter code. I have fixed and pushed the changes. Hope it can get merged soon :)

@dbemiller dbemiller merged commit 1a1028c into prebid:master Aug 4, 2017
philipwatson pushed a commit to mbrtargeting/Prebid.js that referenced this pull request Sep 18, 2017
@CathyHuangtw @philipwatson
Added C1X Adapter (prebid#1369)
d491aad 
* Added C1X Adapter and adapter test file

* corrected bidder's params for c1x bidder

* code updates after code review from Prebid team

* add try/catch block to catch any paring exceptions

* fix ESLint errors

* use $$PREBID_GLOBAL$$ for the global naming convention
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dbemiller dbemiller dbemiller approved these changes

@matthewlane matthewlane matthewlane left review comments

Assignees

@matthewlane matthewlane

@dbemiller dbemiller

Labels
LGTM
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@CathyHuangtw @matthewlane @dbemiller