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 Prebid1.0 adapter for SpotX #3472

Merged
merged 5 commits into from
Jan 31, 2019
Merged

Conversation

strider820
Copy link
Contributor

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
  • Does this change affect user-facing APIs or examples documented on http://prebid.org?
  • Other

Description of change

Creating a new SpotX Bidder Adapter

  • test parameters for validating bids
{
  bidder: 'spotx',
  params: {
    channel_id: '85394',
    ad_unit: 'outstream',
    outstream_options: {
      slot: 'div-gpt-ad-123445678-1',
      content_width:  '300',
      content_height: '250',
    }
  }
}

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

For any changes that affect user-facing APIs or example code documented on http://prebid.org, please provide:

import * as utils from 'src/utils';
import { Renderer } from 'src/Renderer';
import { registerBidder } from 'src/adapters/bidderFactory';
import { VIDEO } from 'src/mediaTypes';
Copy link
Collaborator

@robertrmartinez robertrmartinez Jan 29, 2019

Choose a reason for hiding this comment

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

Imports like these need to be relative after 1.38.0

See: #3435

so add ../ before each src

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

Copy link
Collaborator

@robertrmartinez robertrmartinez left a comment

Choose a reason for hiding this comment

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

Just minor nit pick about unused imports in spec file!

Otherwise this LGTM!

One note is you are juuuuust above the 80% Test Coverage threshold.

Maybe in future PRs we can get that number up!

gulp test-coverage and you can pick out a few lines to cover next time!

Thanks, once those unused imports are fixed we can merge for next Tuesdays Release!


// Add GDPR flag and consent string
if (bidderRequest && bidderRequest.gdprConsent) {
userExt.consent = bidderRequest.gdprConsent.consentString;
Copy link
Collaborator

Choose a reason for hiding this comment

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

If a pub enables the consent modul, but for some reason the consent information cannot be found,
then prebid passes an object like so to the bidders:

{
   gdprConsent: {
      consentString: undefined
      gdprApplies: undefined
      vendorData: undefined
  }
}

So technically your userExt would be:

{
   consent: undefined
}

at this point, but it gets pruned before the request eventually gets sent by prebid so no big deal, just though I would make aware that Prebid passes those undefined values in some instances!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Our ad server should handle undefined in those requests anyways, but that's good to know!

import {config} from 'src/config';
import * as utils from 'src/utils';
import find from 'core-js/library/fn/array/find';

Copy link
Collaborator

Choose a reason for hiding this comment

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

Almost all of these imports are never used besides expect and spec

Can you remove the unused ones?

Both CONSTANTS as well as INTEGRATION below here are not used either.

Let me know if I missed something and there is a reason for keeping them!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. I went ahead and wrote a few more tests while I was in there to bump that coverage up a bit.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Awesome!! Thanks for that.

Copy link
Collaborator

@robertrmartinez robertrmartinez left a comment

Choose a reason for hiding this comment

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

Great stuff! Thanks!

@robertrmartinez robertrmartinez merged commit cbfe312 into prebid:master Jan 31, 2019
jacekburys-quantcast pushed a commit to jacekburys-quantcast/Prebid.js that referenced this pull request May 15, 2019
* Add SpotXBidAdapter

* Minor updates

* Undo testing changes to shared files

* Fix relative imports

* Remove superfluous imports and write a few more tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants