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

Feature/widespace adapter #846

Merged
merged 10 commits into from
Dec 16, 2016

Conversation

jonasmattsson1
Copy link
Contributor

@jonasmattsson1 jonasmattsson1 commented Dec 2, 2016

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

  • test parameters for validating bids
{
  bidder: '<bidder name>',
  params: {
    // ...
  }
}
  • contact email of the adapter’s maintainer
  • official adapter submission

Other information

@jonasmattsson1
Copy link
Contributor Author

The contact email of the adapter’s maintainer:
jonas.mattsson@widespace.com

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 new adapter PR. Please add your adapter to the adapters.json file and update your tests to pass. See this page for information about testing Prebid.js if needed, thanks

}

beforeEach(() => {
pbjs._bidsRequested = [];
Copy link
Collaborator

Choose a reason for hiding this comment

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

This causes several other unit tests to fail

@matthewlane matthewlane self-assigned this Dec 7, 2016
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.

Please update according to the notes below.

Also, are these bidder params still valid for testing?

{
  bidder: 'widespace',
  params: {
    sid: 'f666bfaf-69cf-4ed9-9262-08247bb274e4',
    cur: 'EUR'
  }
}

I'm not getting bids back with them.

@@ -35,6 +35,7 @@
"centro",
"roxot",
"vertoz",
'widespace',
Copy link
Collaborator

Choose a reason for hiding this comment

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

JSON requires double quotes, please use them here

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, thank you

let sandbox;

beforeEach(() => {
pbjs._bidsRequested = [];
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can cause other test to fail, please remove

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, thank you

pbjs._bidsRequested.push(BID_REQUEST);
pbjs.widespaceHandleCB(BID_RESPONSE);

console.log(bidManager.addBidResponse.firstCall.args[0]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove console.log statements from tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed, thank you

});

it('should use the CPM returned by the server', () => {
console.log('successfulBid1', successfulBid1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed, thank you


it('should use the CPM returned by the server', () => {
console.log('successfulBid1', successfulBid1);
console.log('successfulBid1 CPM :::', successfulBid1.cpm);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed, thank you


describe('respond with a successful bid', () => {
let successfulBid1,
placementCode;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This variable is never used, please remove if you don't need it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed, thank you

@jonasmattsson1
Copy link
Contributor Author

Thank you, I have updated the tests.

You should now receive test ad´s with
{ bidder: 'widespace', params: { sid: 'f666bfaf-69cf-4ed9-9262-08247bb274e4', cur: 'EUR' } }

@matthewlane
Copy link
Collaborator

Changes look good and tests pass. I'm still getting back noad status responses with those params though, any pointers? Once I can verify a valid status response we should be good to merge

@jonasmattsson1
Copy link
Contributor Author

Hi,

I think you get status = 'noad' because we don´t have 300x250 or 300x600 ad sizes in our system.
I added [300,300] to adunit.sizes. Hope that is okey.

Thank you,
Jonas

@matthewlane
Copy link
Collaborator

Hmm, even with size [300, 300], still seeing a noad response. Could it be a referrer problem? Anything else I need to check for?

@jonasmattsson1
Copy link
Contributor Author

jonasmattsson1 commented Dec 15, 2016

Hi, Can you try to emulate a mobile like iPhone in Chrome forexample.
It´s probably that I think, We only serve ad´s to mobile devices.

@matthewlane
Copy link
Collaborator

No luck with an iPhone or Chrome on mobile for me. Here's a test page I've set up http://client-test.devnxs.net/widespace/. This is using prebid.js built with your adapter. Currently I'm seeing

available:  []
empty or error:  ["widespace"]

in the dev console. For review I check if the adapter is showing up in the available category. Can you check this page as well? Let me know if you are getting a different result, if the params I'm using on the html in that page look wrong, etc. Hopefully this can help track down why I'm not seeing bids. Thanks!

@jonasmattsson1
Copy link
Contributor Author

Thank you for the time to set up the test page. I got noad on your test page aswell but
I see that you are using widespace param sid = "f666bfaf-69cf-4ed9-9262-08247bb274e4", It should be sid = "7b6589bf-95c8-4656-90b9-af9737bb9ad3" that are linked to a demo account.

I get ad´s on your page when I rewrite the sid parameter to the right one in Charles, see the print screen that I made.

https://drive.google.com/file/d/0B2Hq0o69IghYRS0yTXU0eUJ4dEU/view?usp=sharing
You still need to emulate an mobile device.

Thank you,
Jonas

@matthewlane
Copy link
Collaborator

Getting bids now with "7b6589bf-95c8-4656-90b9-af9737bb9ad3" and mobile emulation, thanks

@matthewlane matthewlane merged commit d6ebd87 into prebid:master Dec 16, 2016
mp-12301 pushed a commit to aol/Prebid.js that referenced this pull request Apr 10, 2017
…ebid-official-0.17.0 to release/1.12.0

* commit 'bce975978c2c088d301d120b949d38080ca9d314':
  Add changelog entry.
  Prebid Release 0.17.0
  remove hb_pb targeting key for deal bids with no cpm (prebid#881)
  Fix for bug prebid#866 (prebid#867)
  Allow changing of 'ga' global variable (prebid#832)
  emit auction end event before bidsBackHandler callback (prebid#884)
  Add Widespace adapter (prebid#846)
  prevent rubicon adapter from registering two bids on exceptions (prebid#854)
  Code Refactoring - Upgrading end point. (prebid#826)
  Ignore test html pages (prebid#878)
  Detect browser width instead of the screen width (prebid#837)
  New aardvark adapter with support for aliasing (prebid#875)
  Add Fidelity adapter (prebid#862)
  Adkernel adapter aliasing (prebid#857)
  0.16.1-pre
mp-12301 pushed a commit to aol/Prebid.js that referenced this pull request Apr 10, 2017
…12.0 to master

* commit '728f1763ce2a282a757546e934199313a4771a21':
  Fix failing unit test on CI.
  Add changelog entry.
  Prebid Release 0.17.0
  remove hb_pb targeting key for deal bids with no cpm (prebid#881)
  Fix for bug prebid#866 (prebid#867)
  Allow changing of 'ga' global variable (prebid#832)
  emit auction end event before bidsBackHandler callback (prebid#884)
  Add Widespace adapter (prebid#846)
  prevent rubicon adapter from registering two bids on exceptions (prebid#854)
  Code Refactoring - Upgrading end point. (prebid#826)
  Ignore test html pages (prebid#878)
  Detect browser width instead of the screen width (prebid#837)
  New aardvark adapter with support for aliasing (prebid#875)
  Add Fidelity adapter (prebid#862)
  Adkernel adapter aliasing (prebid#857)
  0.16.1-pre
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