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

fixed PBS cookie syncs #1637

Merged
merged 2 commits into from
Oct 3, 2017
Merged

Conversation

harpere
Copy link
Collaborator

@harpere harpere commented Sep 28, 2017

Type of change

  • [x ] 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: {
    // ...
  }
}

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

  • contact email of the adapter’s maintainer
  • official adapter submission

Other information

@snapwich
Copy link
Collaborator

Sorry I'm late to the game here, but it seems like it would be ideal to have this sync code for the prebidServer adapter continue using the userSync.js module. If the issue is priority (it needs to run immediately) it seems like we should just code that use-case into userSync.js.

In all likelihood it's probably going to be a one-off, but if we need to make a one-off I think it's advantageous to put it in the place where you'd expect to find it, w/ the rest of the user sync code. If it ends up not being a one-off, now it's in a place that is reusable. Thoughts?

@mkendall07
Copy link
Member

@snapwich I'd prefer not to have this patter in userSync.js to be honest. I think this is a good case for a one off.

@harpere
Copy link
Collaborator Author

harpere commented Oct 2, 2017

@snapwich @mkendall07 I thought about adding this use case to userSync.registerSync(), but didn't think it was worth the added complication for a one-off. Also considered simply adding the new doBidderSync() function to userSync instead, but thought that might encourage bidders to use that instead of registerSync().

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.

LGTM

@snapwich snapwich self-requested a review October 3, 2017 17:07
Copy link
Collaborator

@snapwich snapwich left a comment

Choose a reason for hiding this comment

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

LGTM

@harpere harpere merged commit 6b6a1fb into prebid:master Oct 3, 2017
vzhukovsky added a commit to aol/Prebid.js that referenced this pull request Oct 25, 2017
….30.0 to aolgithub-master

* commit '5a8d2bf93ee15071a78e24ac976103cacf3c6021': (35 commits)
  Added changelog entry.
  Prebid 0.30.1 Release
  Remove undefined variable usage (prebid#1662)
  fixes bug for IE when invalid value passed to parse (prebid#1657)
  Aliasbidder fix (prebid#1652)
  prebidAdapter secure support (prebid#1655)
  Increment pre version
  Prebid 0.30.0 Release
  Add native param support to mediaTypes (prebid#1625)
  PulsePoint Lite adpater changes (prebid#1630)
  Appnexus ast unittest updates (prebid#1654)
  Support aspect ratio specification for native images (prebid#1634)
  Revert changes for switch between client side and server side. (prebid#1653)
  rubicon converted to bidderFactory (prebid#1624)
  Add JSDoc for `pbjs.getAllWinningBids` (prebid#1566)
  Add ignore-loader to handle .md files (prebid#1646)
  fixed PBS cookie syncs (prebid#1637)
  Add placementId request param to Yieldmo bid adapter (prebid#1632)
  Adxcg analytics adapter (prebid#1599)
  Add publisher sub-id support to the Criteo adapter (prebid#1629)
  ...
@qrilka
Copy link

qrilka commented Oct 26, 2017

@harpere could you explain a bit the fact that in this PR you remove the only use of registerSync in the repo?
Do I understand it correctly that this function should be called but nobody does it?

@mkendall07
Copy link
Member

@qrilka
The new pattern is to create a spec file and register it with the bidder factory. The spec should contain a getUserSyncs function which will invoke registerSync underneath.
Example:
https://github.com/prebid/Prebid.js/blob/master/modules/appnexusAstBidAdapter.js#L114
and
https://github.com/prebid/Prebid.js/blob/master/src/adapters/bidderFactory.js#L205

@qrilka
Copy link

qrilka commented Oct 26, 2017

thanks for the info @mkendall07
so the current docs are not correct talking about using both functions in http://prebid.org/dev-docs/bidder-adapter-1.html#register-user-syncs ? Also getUserSyncs on that page uses only 1 parameter while in the code I see responses as the 2nd one.

dluxemburg pushed a commit to Genius/Prebid.js that referenced this pull request Jul 17, 2018
* fixed PBS cookie syncs

* added comment
@robertrmartinez robertrmartinez deleted the bugfix/s2s-user-sync branch July 5, 2023 19:45
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.

4 participants