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

Sovrn 1.0 compliance #1796

Merged
merged 24 commits into from
Nov 14, 2017
Merged

Sovrn 1.0 compliance #1796

merged 24 commits into from
Nov 14, 2017

Conversation

tedrand
Copy link
Contributor

@tedrand tedrand commented Nov 2, 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

Adjustments to Sovrn bid adapter to ensure compliance with Prebid 1.0.0.

Rachel Joyce and others added 17 commits July 26, 2017 17:06
…sReceived in Sovrn adapter and Sovrn tests.
… this is the stated standard for Prebid 1.0.0.
…son', as this is the stated standard for Prebid 1.0.0."

This reverts commit 0338ce7.
…ig for GET requests. Added test for this second change and removed tests for passing null or undefined member variables into the request.options object.
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 updated adapter. Please also add your .md file in this PR: http://prebid.org/dev-docs/bidder-adapter-1.html#planning-your-adapter

@tedrand
Copy link
Contributor Author

tedrand commented Nov 6, 2017

Thanks, I'll get that put together. As far as the test ad unit that returns test creatives, does this need to be able to serve on any site?

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.

@tedrand We run test from localhost so if the test creatives serve from there that's fine. We can also rewrite localhost to a specific domain if needed, if that's the case just make sure to add a note about this to the .md file. In any case I was able to receive bids with my usual setup and your test parameters so that part is good. A few changes requested before merging:

Sovrn's adapter integration to the Prebid library. Posts plain-text JSON to the /rtb/bid endpoint.

# Test Parameters

Copy link
Collaborator

Choose a reason for hiding this comment

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

This line and line 46 should mark the code with ```

import { expect } from 'chai';
import { spec } from 'modules/sovrnBidAdapter';
import { newBidder } from 'src/adapters/bidderFactory';
import { REPO_AND_VERSION } from 'src/constants';
import bidmanager from 'src/bidmanager';
import adloader from 'src/adloader';
var utils = require('src/utils');
Copy link
Collaborator

Choose a reason for hiding this comment

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

bidmanager, adloader, and utils modules aren't used in the new test, can be removed

seatbid[0].bid.map(sovrnBid => {
sovrnBidResponses.push({
requestId: sovrnBid.impid,
bidderCode: spec.code,
Copy link
Collaborator

Choose a reason for hiding this comment

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

bidderCode will be set automatically by bidderFactory now, this line can be dropped

* @param {id, seatbid} sovrnResponse A successful response from Sovrn.
* @return {Bid[]} An array of formatted bids.
*/
interpretResponse: function({id, seatbid}) {
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 destructuring like

{body: {id, seatbid}}

or however you'd prefer to grab the body, and updating corresponding tests 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.

Thanks @matthewlane for the info here. Just so I am on the same page, destructuring as you have suggested will lead to the same functionality as what we currently have implemented? I am just making sure that we do not have to change the bid response's JSON structure on our end.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's right, updating in this way will give the same functionality as before #1748 was merged to master, and there are no changes required to the JSON structure sent from your endpoint.

For a (non-destructuring) example, appnexusAst was updated here to pick the response data off the body property. Before #1748 we used the first parameter directly. This update lets us keep working properly and no changes were made to our endpoint because behind the scenes it's the same data coming through, just on the body property of the first parameter rather than the first parameter itself now.

Let me know if you have more questions, and I'll verify the change works whenever the PR is updated

@@ -66,6 +66,7 @@ import { logWarn, logError, parseQueryStringParameters, delayExecution } from 's
* @property {('GET'|'POST')} method The type of request which this is.
* @property {string} url The endpoint for the request. For example, "//bids.example.com".
* @property {string|object} data Data to be sent in the request.
* @property {object} options Content-Type set in the header of the bid request, overrides default 'text/plain'.
Copy link
Collaborator

@matthewlane matthewlane Nov 8, 2017

Choose a reason for hiding this comment

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

Proposed changes to core modules are welcome but should be made in a separate PR. Also calls from 1.0 bidders will have contentType: 'text/plain' set by default, so needing to set options: {contentType: 'text/plain'} on the object returned by your buildRequests function is unnecessary I believe?

Edit: oh this is already in master via #1681? GitHub shows these files as changed for some reason

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah sorry I couldn't figure out why that was happening either.

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.

Changes look good. The package.json and package-lock.json changes should be dropped from the PR though then we're good for merge

@@ -0,0 +1,16925 @@
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

File should be dropped from PR

package.json Outdated
@@ -43,7 +43,7 @@
"eslint-plugin-standard": "^3.0.1",
"faker": "^3.1.0",
"fs.extra": "^1.3.2",
"gulp": "^3.8.7",
"gulp": "^3.9.1",
Copy link
Collaborator

Choose a reason for hiding this comment

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

We'll need to check the version upgrade in a separate PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

darn sorry! i will roll those back

@tedrand
Copy link
Contributor Author

tedrand commented Nov 10, 2017

this should be good now, I think

@matthewlane
Copy link
Collaborator

matthewlane commented Nov 14, 2017

@tedrand Can you delete the package-lock.json entirely, not just make it an empty file? After that, the tab at the top of this PR for Files changed should show 3.

If you're able to do this within the next three hours that'd be great as we're releasing today, if not I'll do a manual merge to still get it in, but would prefer doing it from GitHub

@matthewlane matthewlane merged commit 02f07ee into prebid:master Nov 14, 2017
Millerrok pushed a commit to Vertamedia/Prebid.js that referenced this pull request Nov 21, 2017
* unstream/master: (36 commits)
  + Add Optimatic Bid Adapter (prebid#1837)
  Add Bridgewell adapter (prebid#1825)
  Kumma adapter updated for Prebid 1.0 (prebid#1766)
  Touchup add bid response (prebid#1822)
  Fix skipped test (prebid#1836)
  Added new size in Rubicon pbjs Adapter (prebid#1842)
  HuddledMasses header bidding adapter (prebid#1806)
  Increment pre version
  Prebid 0.33.0 Release
  Update AOL adapter for v1.0  (prebid#1693)
  Sovrn 1.0 compliance (prebid#1796)
  Platform.io Bidder Adapter update (prebid#1817)
  Drop non-video bidders from video ad units (prebid#1815)
  Update renderAd to replace ${AUCTION_PRICE} in adUrl (prebid#1795)
  Pulsepoint adapter: fixing bid rejection due to missing mandatory bid params. (prebid#1823)
  Remove require.ensure entirely (prebid#1816)
  Add custom keyword support for pbs bid adapter (prebid#1763)
  OpenX Video Adapter update to Prebid v1.0 (prebid#1724)
  Fix test that hard-coded pbjs global. (prebid#1786)
  Update Pollux Adapter to v1.0 (prebid#1694)
  ...
vzhukovsky added a commit to aol/Prebid.js that referenced this pull request Dec 28, 2017
….33.0 to aolgithub-master

* commit '3e9756098bb20ecbe0314f16eed5298c5675b24c': (32 commits)
  Wrapped content type in options object.
  Added partners ids.
  Added changelog entry.
  Prebid 0.33.0 Release
  Update AOL adapter for v1.0  (prebid#1693)
  Sovrn 1.0 compliance (prebid#1796)
  Platform.io Bidder Adapter update (prebid#1817)
  Drop non-video bidders from video ad units (prebid#1815)
  Update renderAd to replace ${AUCTION_PRICE} in adUrl (prebid#1795)
  Pulsepoint adapter: fixing bid rejection due to missing mandatory bid params. (prebid#1823)
  Remove require.ensure entirely (prebid#1816)
  Add custom keyword support for pbs bid adapter (prebid#1763)
  OpenX Video Adapter update to Prebid v1.0 (prebid#1724)
  Fix test that hard-coded pbjs global. (prebid#1786)
  Update Pollux Adapter to v1.0 (prebid#1694)
  PubMatic adapter (prebid#1707)
  Added sizes to Rubicon Adapter (prebid#1818)
  jsonpFunction name should match the namespace (prebid#1785)
  Adding 33Across adapter (prebid#1805)
  Unit test fix (prebid#1812)
  ...
sovrnImps.push(imp);
sovrnImps.push({
id: bid.bidId,
banner: { w: 1, h: 1 },
Copy link

Choose a reason for hiding this comment

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

@tedrand Is this hardcoded as 1x1 because Sovrn knows the size based on the ID? It seems like the bid.sizes and bid.params.sizes properties will be totally ignored by the adapter. Is that correct?

@mkendall07 If that’s the case, then https://github.com/prebid/Prebid.js/blob/master/modules/sovrnBidAdapter.md should be updated to omit the sizes fields, right?

@s-nikulin-lineate s-nikulin-lineate deleted the sovrn-1.0-compliance branch June 20, 2022 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants