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

sekindoUM for prebid1.0 #1777

Merged
merged 3 commits into from
Dec 4, 2017
Merged

sekindoUM for prebid1.0 #1777

merged 3 commits into from
Dec 4, 2017

Conversation

nissSK
Copy link
Contributor

@nissSK nissSK commented Oct 30, 2017

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
  • Other

Description of change

sekindoUM for prebid1.0

  • test parameters for validating bids
 var adUnits = [{
      code: 'banner-ad-div',
      sizes: [[300, 250]],
      bids: [{
          bidder: 'sekindoUM',
          params: {
              spaceId: 14071
          }
      }]
    },
        {
      code: 'video-ad-div',
      sizes: [[640, 480]],
      bids: [{
          bidder: 'sekindoUM',
          params: {
              spaceId: 87812,
                          video:{
                                playerWidth:640,
                                playerHeight:480,
                                vid_vastType: 5 //optional
                          }
          }
      }]
    }
        ];

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

  • contact email of the adapter’s maintainer
  • official adapter submission

Other information

@snapwich snapwich self-assigned this Oct 31, 2017
@snapwich snapwich self-requested a review October 31, 2017 20:48
snapwich
snapwich approved these changes Nov 1, 2017
View reviewed changes
Copy link
Collaborator

@snapwich snapwich left a comment

Choose a reason for hiding this comment

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

LGTM

@ndhimehta ndhimehta added the ready label Nov 6, 2017
@snapwich snapwich added the LGTM label Nov 17, 2017
matthewlane
matthewlane requested changes Nov 22, 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.

There was a breaking change added recently that requires an update to the interpretResponse function, and a few other notes for your review

modules/sekindoUMBidAdapter.js Outdated
var bidsCount = bids.length;

import * as utils from 'src/utils';
// import {config} from 'src/config';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Line can be removed if not used

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

modules/sekindoUMBidAdapter.js Outdated
* @param {*} serverResponse A successful response from the server.
* @return {Bid[]} An array of bids which were nested inside the server.
*/
interpretResponse: function(serverResponse, bidRequest) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

#1748 changed the first argument of interpretResponse to:

{
  body: responseBody,
  headers: {
    get: function(header) { /* returns a header from the HTTP response */ }
  }
}

so adding something like

serverResponse = serverResponse.body;

just below this line, or however you'd prefer to grab the body, and updating corresponding tests if needed should get this back to working properly

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

modules/sekindoUMBidAdapter.js Outdated
* @return boolean True if this is a valid bid, and false otherwise.
*/
isBidRequestValid: function(bid) {
if (bid.mediaType == 'video') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

As of #1483, video ad units may be defined with mediaTypes: {video: {}} and would be on this bid parameter, you'll probably want to check for that as well

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

modules/sekindoUMBidAdapter.js Outdated
queryString = utils.tryAppendQueryString(queryString, 'hbcb', '1');/// legasy
queryString = utils.tryAppendQueryString(queryString, 'dcpmflr', bidfloor);
queryString = utils.tryAppendQueryString(queryString, 'protocol', protocol);
if (bidRequest.mediaType === 'video') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

May want to check for mediaTypes.video as well, see other comment for related info

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

@matthewlane matthewlane assigned matthewlane and unassigned snapwich Nov 22, 2017
@jsnellbaker
Copy link
Collaborator

@nissSK I saw your updates to the feedback from @matthewlane, but I don't see that the changes were committed. Are those changes still in the works on your end?

Please confirm when you get the chance. Thanks!

@nissSK
Copy link
Contributor Author

nissSK commented Nov 30, 2017

@jsnellbaker Should be ok now (pushed changes to prebid1.0 instead of master)

@matthewlane
Copy link
Collaborator

@nissSK Changes look good but the 405 file changes are a bit concerning. Looks chmod related, if those can be resolved so there are just the 3 sekindo file changes we should be good for merge

@nissSK
Copy link
Contributor Author

nissSK commented Dec 4, 2017

@matthewlane Sure, didnt notice that.

@matthewlane matthewlane merged commit f93c2dd into prebid:master Dec 4, 2017
Millerrok pushed a commit to Vertamedia/Prebid.js that referenced this pull request Dec 8, 2017
Merge branch 'master' of https://github.com/prebid/Prebid.js
4273734 
* 'master' of https://github.com/prebid/Prebid.js: (43 commits)
  Merge Prebid 1.0 to Master (prebid#1936)
  Prebid.js 0.34.1 release
  Vertamedia adapter outstream support (prebid#1860)
  Expose native image-type asset dimensions on bid response object (prebid#1919)
  Remove for of (prebid#1932)
  Unit-test fix (prebid#1927)
  Remove description_url (prebid#1922)
  Trion Interactive Adapter Bugfix (prebid#1925)
  Remove config setting from Optimatic adapter (prebid#1909)
  IE bug fix (prebid#1930)
  Clarify ad unit media filtering warning (prebid#1903)
  Add ReadPeak Bid Adapter (prebid#1838)
  Change clone function to make deep copies (prebid#1910)
  Serverbid 1.0 (prebid#1802)
  sekindoUM for prebid1.0 (prebid#1777)
  update auctionId to be requestId (prebid#1896)
  bug fixed to populate userSync default values (prebid#1897)
  Increment pre version
  AdkernelAdn analytics adapter (prebid#1868)
  Justpremium Adapter: use `filter` instead of `...new Set` (prebid#1895)
  ...
vzhukovsky added a commit to aol/Prebid.js that referenced this pull request Dec 28, 2017
@vzhukovsky
Merge pull request prebid#134 in AOLP_ADS_JS/prebid.js from release/1…
b736fae 
….34.0 to aolgithub-master

* commit 'f0ba90afa8b52de7a646d43928b2d51ee42e74a1': (53 commits)
  Added changelog entry.
  Added partners ids.
  Added dynamic ttl property for One Display and One Mobile.
  Prebid.js 0.34.1 release
  Vertamedia adapter outstream support (prebid#1860)
  Expose native image-type asset dimensions on bid response object (prebid#1919)
  Remove for of (prebid#1932)
  Unit-test fix (prebid#1927)
  Remove description_url (prebid#1922)
  Trion Interactive Adapter Bugfix (prebid#1925)
  Remove config setting from Optimatic adapter (prebid#1909)
  IE bug fix (prebid#1930)
  Clarify ad unit media filtering warning (prebid#1903)
  Add ReadPeak Bid Adapter (prebid#1838)
  Change clone function to make deep copies (prebid#1910)
  Serverbid 1.0 (prebid#1802)
  sekindoUM for prebid1.0 (prebid#1777)
  update auctionId to be requestId (prebid#1896)
  bug fixed to populate userSync default values (prebid#1897)
  Increment pre version
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewlane matthewlane matthewlane approved these changes

@snapwich snapwich snapwich approved these changes

@jsnellbaker jsnellbaker Awaiting requested review from jsnellbaker

Assignees

@matthewlane matthewlane

@jsnellbaker jsnellbaker

Labels
needs update Prebid 1.0 ready
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@nissSK @jsnellbaker @matthewlane @snapwich @mjacobsonny @ndhimehta