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

Add new s2s config option #3643

Merged
merged 3 commits into from
Mar 25, 2019
Merged

Add new s2s config option #3643

merged 3 commits into from
Mar 25, 2019

Conversation

thomas-33across
Copy link
Contributor

@thomas-33across thomas-33across commented Mar 15, 2019

Type of change

  • Feature
  • Does this change affect user-facing APIs or examples documented on http://prebid.org?

Description of change

Adding a new option in the s2s configuration, which allow bidder to modify the sync url before the actual call to the sync endpoint. This enable bidder to have customization over the url, and also enable them to add custom params

  • new s2s configuration options
{
  bidders: ['33across'],
  adapter: 'prebidServer',
  syncEndpoint: 'http://www.mylocal.com:8000/cookie_sync',
  syncUrlModifier: {
    '33across': function(type, url, bidder) {
      const publisherId = '00000123231231'
      url += `&ri=${publisherId}`;

      return url
    }
  }
}

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

@thomas-33across
Copy link
Contributor Author

Hello Prebid team,
I will do the PR for the docs if this propose change can be done.

Thank you

@bretg
Copy link
Collaborator

bretg commented Mar 18, 2019

I'm empathetic with this hook - we had a similar issue resolved by relying on the s2sConfig.accountId, but that doesn't work if we're not the host.

Assigning to @mkendall07 to approve the API change.

* @param {function} done an exit callback; to signify this pixel has either: finished rendering or something went wrong
*/
function doPreBidderSync(type, url, bidder, done) {
if (typeof _s2sConfig.syncUrlModifier[bidder] === 'function') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be testing for syncUrlModifier before attempting to index into bidder?

i.e.

if (_s2sConfig.syncUrlModifier and typeof _s2sConfig.syncUrlModifier[bidder] === 'function')

@mkendall07
Copy link
Member

This has the slight disadvantage of making the s2s config not JSON configurable. If folks are ok with that tradeoff I don't mind approving.

@bretg
Copy link
Collaborator

bretg commented Mar 19, 2019

Rubicon is ok with this approach.

(thomas-33across - thanks for adding the object check)

@bretg bretg requested a review from mkendall07 March 19, 2019 19:38
Copy link
Member

@mkendall07 mkendall07 left a comment

Choose a reason for hiding this comment

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

This LGTM. We just need a documentation PR and then can merge.

@thomas-33across
Copy link
Contributor Author

Hello prebid team
I had create a new PR for the doc and here is the link:
prebid/prebid.github.io#1216

Thank you

@bretg
Copy link
Collaborator

bretg commented Mar 25, 2019

Merging.

@bretg bretg merged commit f77b78c into prebid:master Mar 25, 2019
pycnvr pushed a commit to conversant/Prebid.js that referenced this pull request Apr 4, 2019
* - add new s2s params to do sync url modifier

* add unit test case for s2s config

* - add additional condition check for _s2sConfig.syncUrlModifier
jacekburys-quantcast pushed a commit to jacekburys-quantcast/Prebid.js that referenced this pull request May 15, 2019
* - add new s2s params to do sync url modifier

* add unit test case for s2s config

* - add additional condition check for _s2sConfig.syncUrlModifier
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.

3 participants