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

YIELDONE adapter - add buildRequests payload params #3611

Merged
merged 26 commits into from
Mar 11, 2019

Conversation

kusapan
Copy link
Contributor

@kusapan kusapan commented Mar 5, 2019

Type of change

  • Feature

Description of change

  • add buildRequests payload params
    • uc adUnitCode
    • tmax timeout setting by publisher

sample page

contact email of the adapter’s maintainer y1dev@platform-one.co.jp

  • official adapter submission

payload.tmax = window.PREBID_TIMEOUT;
} else if (window.pbjsTimeout) {
payload.tmax = window.pbjsTimeout;
}
Copy link
Collaborator

@robertrmartinez robertrmartinez Mar 5, 2019

Choose a reason for hiding this comment

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

Hello @kusapan

One suggestion is you can utilize the PBJS config in order to also get the bidderTimeout.

This value will always be set.

payload.tmax = config.getConfig('bidderTimeout');

example:

timeout: config.getConfig('bidderTimeout'),

This value is set by the publisher by:

pbjs.setConfig({
   bidderTimeout: 850
});

If it is not explicitly set by the publisher, then it defaults to 3000.

This number is the milliseconds that Prebid gives it's bidders to come back in time for the auction.

So I think it is a good value to send as your tmax.

If there are other reasonings or use cases I am unaware of as to why it would not be included please let me know!

Copy link
Contributor Author

@kusapan kusapan Mar 11, 2019

Choose a reason for hiding this comment

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

Hello @robertrmartinez
Thank you for reviewing.

Our team talks about how to handle tmax.
In this PR, tmax was deleted.

please confirm this.

it('adUnitCode should be sent as uc parameters on any requests', function () {
expect(request[0].data.uc).to.equal('adunit-code1');
expect(request[1].data.uc).to.equal('adunit-code2');
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for adding a unit test!

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.

LGTM

@robertrmartinez robertrmartinez merged commit 16e5e16 into prebid:master Mar 11, 2019
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