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

Create undertone udapter #1850

Merged
merged 16 commits into from
Nov 29, 2017
Merged

Conversation

gilpat
Copy link
Contributor

@gilpat gilpat commented Nov 19, 2017

Type of change

  • [*] New bidder adapter

Description of change

new undertone bidder adapter.

  • test parameters for validating bids
{
   bidder: 'undertone',
          params: {
            placementId: '10433394',
            publisherId: 12345
          }
}

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

Other information

@gilpat
Copy link
Contributor Author

gilpat commented Nov 20, 2017

I saw that this error raised while running 'gulp run-tests'

HeadlessChrome 0.0.0 (Linux 0.0.0) MarsMedia adapter implementation "before each" hook FAILED
Error: Uncaught SyntaxError: Unexpected token < (http://localhost:9876/context.html:1)
HeadlessChrome 0.0.0 (Linux 0.0.0): Executed 859 of 1622 (1 FAILED) (0 secs / 1.241 secs)
HeadlessChrome 0.0.0 (Linux 0.0.0) MarsMedia adapter implementation "before each" hook FAILED
Error: Uncaught SyntaxError: Unexpected token < (http://localhost:9876/context.html:1)
e 0.0.0 (Linux 0.0.0): Executed 859 of 1622 (1 FAILED) (28.686 secs / 1.241 secs)
[15:25:28] 'test-coverage' errored after 3.58 min
[15:25:28] Error: Karma tests failed with exit code 1
at /home/travis/build/prebid/Prebid.js/gulpfile.js:119:12
at removeAllListeners (/home/travis/build/prebid/Prebid.js/node_modules/karma/lib/server.js:380:7)
at Server. (/home/travis/build/prebid/Prebid.js/node_modules/karma/lib/server.js:391:9)
at Server.g (events.js:291:16)
at emitNone (events.js:91:20)
at Server.emit (events.js:185:7)
at emitCloseNT (net.js:1544:8)
at _combinedTickCallback (internal/process/next_tick.js:71:11)
at process._tickCallback (internal/process/next_tick.js:98:9)

I'm not getting this error locally , moreover, our pull request contains undertone adapter only , nothing that is related to Marsmedia .

can you please help?

@mkendall07
Copy link
Member

@gilpat We have some flakky tests. I've restarted the job to see if it passes

@gilpat
Copy link
Contributor Author

gilpat commented Nov 20, 2017

Thanks ,
however the test raises now an error regards to indexExchange adapter

@gilpat
Copy link
Contributor Author

gilpat commented Nov 21, 2017

Hi ,
is there anything we can do ? tests seem to fail for no reason I can see .

@mkendall07
Copy link
Member

@gilpat it's passing now

@danagutkind
Copy link

Hello Matt,
We would like to go live with prebid adaptor before Dec holidays.
Is there something that can be done to push forward the approval process of our adapter?
How many days it usually takes?
We appreciate any help from your side.
Thank you in advance.

Copy link
Collaborator

@jsnellbaker jsnellbaker left a comment

Choose a reason for hiding this comment

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

@gilpat Thanks for submitting these changes. There are a few things that should be reviewed/updated, but mostly it seems good.

Please take a look at the specifics below and let me know if there are any questions. Thanks!

};
},
interpretResponse: function(serverResponse, request) {
const bids = [];
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

if (bidRes.ad && bidRes.cpm > 0) {
const bid = {
requestId: bidRes.bidRequestId,
bidderCode: BIDDER_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 by bidderFactory automatically now, this line can be dropped

const payload = {
'x-ut-hb-params': []
};
const timeout = window.PREBID_TIMEOUT || null;
Copy link
Collaborator

@jsnellbaker jsnellbaker Nov 28, 2017

Choose a reason for hiding this comment

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

Can you clarify why the timeout value is needed for the bid object passed to your system? Is this a reporting metric or actively used by the system?

Prebid data can't be accessed directly through the window object. So we want to understand the need to help figure out some alternatives around supplying this information (if it's really needed).

Choose a reason for hiding this comment

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

Our API enable us to pass the timeout value to the server, so we can response with nobid if we are not able to respond within this time frame. This should improve the user experience and reduce load from our server.
Is there another way to get the timeout you're using? Or maybe there is a constant value you're using? If so, we can just add it here.

@mkendall07
Copy link
Member

@danagutkind
if you can get updates on this in the next 30 minutes we can include in the next release.

@danagutkind
Copy link

@mkendall07
Thank you very much for your comments.
We try to make it on time although 30 minutes is a challenge (our guys are at home at this hour).
When is your next release?

update after code review
@mkendall07
Copy link
Member

Ok np. Just wanted to give you the heads up. Possibly next week depending on things shake out.

@danagutkind
Copy link

@mkendall07
Hi Matt,
Furtunatelly, we managed to push all comments from your side.
We will be grateful if you include out adapter in the closest release.
Thank you for your help and responsiveness. We appreciate it.

import { registerBidder } from 'src/adapters/bidderFactory';

const BIDDER_CODE = 'undertone';
const URL = '//hb.undertone.com/hb'; // //ads.undertone.com/hb
Copy link
Member

Choose a reason for hiding this comment

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

What's //ads.undertone.com/hb ? Could remove this comment

@mkendall07
Copy link
Member

@gilpat We are seeing a CORS related error when testing the adapter. Are you sure the test params are valid and work on the hello world test page?

@jsnellbaker
Copy link
Collaborator

After retesting this morning, we're able to see the test ad with the hello word test page.

LGTM

@jsnellbaker jsnellbaker merged commit ff9cd38 into prebid:master Nov 29, 2017
@danagutkind
Copy link

@mkendall07
Hello Matt,
Would you please tell when is our adapter expected to go live?
We would like to update our publisher.
Many thanks again,
Dana

@danagutkind
Copy link

@mkendall07 Hi Matt
Do you have any estimation when our adapter goes live?
I am asking as we are committed to our publishers.
We would like to reflect timelines as best as we can, so it will be helpful for us to know.
Can you please advice?

Thank you,
Dana

@mkendall07
Copy link
Member

What do you mean live? This is merged and available in the 0.34 release I believe

@danagutkind
Copy link

@mkendall07

Thank you very much.
I see our adapter is merged and available.

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.