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

Smartyads Adapter 1.x #2080

Merged
merged 2 commits into from
Mar 6, 2018
Merged

Smartyads Adapter 1.x #2080

merged 2 commits into from
Mar 6, 2018

Conversation

isss1650
Copy link
Contributor

Type of change

  • New bidder adapter

Description of change

Added HB adapter for SmartyAds SSP

  • test parameters for validating bids
{
  bidder: 'smartyads',
  params: {
        placementId: 0
  }
}

  • contact email of the adapter’s maintainer: [email protected]

  • official adapter submission

@isss1650 isss1650 changed the title Smartyads Adapter 1.0 Smartyads Adapter 1.x Jan 29, 2018
@bretg bretg requested a review from pm-harshad-mane January 30, 2018 15:29
@bretg
Copy link
Collaborator

bretg commented Jan 30, 2018

Thanks for the submission @isss1650. While the review is taking place, please note that we will require that a file for this adapter be added to https://github.com/prebid/prebid.github.io/tree/master/dev-docs/bidders

@mkendall07 mkendall07 added this to the Prebid 1.4.0 milestone Feb 12, 2018
@isss1650
Copy link
Contributor Author

Docs
prebid/prebid.github.io#624

@mkendall07 mkendall07 self-requested a review February 20, 2018 19:14
@mkendall07 mkendall07 self-assigned this Feb 20, 2018
@mkendall07 mkendall07 modified the milestones: Prebid 1.4.0, Prebid 1.5.0 Feb 23, 2018
mkendall07
mkendall07 requested changes Feb 26, 2018
View reviewed changes
modules/smartyadsBidAdapter.js Outdated
}
switch (bid['mediaType']) {
case 'banner':
return Boolean(bid['width'] && bid['height'] && bid['ad']);
Copy link
Member

Choose a reason for hiding this comment

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

don't need the Boolean function here. Also prefer dot notation bid.width vs bracket notation for known keys.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dot notation added.
Boolean - disagree. In my opinion, the function must always return data of the same type.
The validation function must return a value in the boolean data type.
Don't you agree with me?

Copy link
Member

Choose a reason for hiding this comment

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

yeah that's fine I'm just use to reviewing code with !! instead of Boolean but it should be functionally equivalent.

modules/smartyadsBidAdapter.js Outdated
return false;
}
switch (bid['mediaType']) {
case 'banner':
Copy link
Member

Choose a reason for hiding this comment

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

we have constants for these types you can use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@mkendall07 mkendall07 removed the request for review from pm-harshad-mane February 26, 2018 18:21
mkendall07
mkendall07 approved these changes Mar 2, 2018
View reviewed changes
Copy link
Member

@mkendall07 mkendall07 left a comment

Choose a reason for hiding this comment

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

LGTM. One comment, your user sync URL http://ssp-nj.webtradehub.com/?c=o&m=cookie returns a 404

@matthewlane matthewlane merged commit b82f852 into prebid:master Mar 6, 2018
mizmaar3 added a commit to widespace-os/Prebid.js that referenced this pull request Mar 8, 2018
@mizmaar3
Merge branch 'master' into Widespace_adapter
ffb52bb 
* master: (27 commits)
  Increment pre version
  Prebid 1.5.0 Release
  Fix cross-platform test failures (prebid#2228)
  Fix uncahced video bids from multi-response array triggering callback early (prebid#2219)
  Add vuble adapter (prebid#2201)
  Update Vidazoo domain (prebid#2223)
  InSkin Bid Adapter: remove referrer field from request body (prebid#2217)
  Gamma Support UserSync Endpoint (prebid#2216)
  Feature/stale bot (prebid#2192)
  33Across Bid Adapter: updated user sync endpoint (prebid#2193)
  Adding PR_REVIEW guideline (prebid#2159)
  Add FairTrade Bid Adapter (prebid#2147)
  Add banner support to Beachfront adapter (prebid#2117)
  Smartyads Adapter 1.x (prebid#2080)
  Audience Network: allow native bids for non-IAB sizes (prebid#2203)
  Update position default value in rubicon (prebid#2214)
  Auctionmanager spec refactor pr (prebid#2155)
  fix mediaType being overwritten by undefined in rubicon bid adapter (prebid#2209)
  Fix lint error (prebid#2208)
  VAST support in adform adapter (prebid#2173)
  ...
dluxemburg pushed a commit to Genius/Prebid.js that referenced this pull request Jul 17, 2018
@isss1650 @dluxemburg
Smartyads Adapter 1.x (prebid#2080)
ad005a0 
* add SmartyAds adapter 1.0

* amendment Smartyads adapter
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkendall07 mkendall07 mkendall07 approved these changes

Assignees

@mkendall07 mkendall07

Labels
LGTM
Projects
None yet
Milestone
Prebid 1.5.0
Development

Successfully merging this pull request may close these issues.
6 participants
@isss1650 @bretg @mkendall07 @matthewlane @pm-harshad-mane @vladis-teqblaze