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.

Sign up for GitHub

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

Jump to bottom

Update serverbid adapter to use smartsync #1324

Merged
merged 1 commit into from
Aug 31, 2017
Merged

Update serverbid adapter to use smartsync #1324

merged 1 commit into from
Aug 31, 2017

Conversation

micha
Copy link
Contributor

@micha micha commented Jun 26, 2017

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

Adds the option to sync cookies with RTB partners (see also #1301).

@micha micha force-pushed the serverbid-smartsync branch 2 times, most recently from d568c33 to 5e85f82 Compare June 26, 2017 17:55
harpere
harpere reviewed Jun 26, 2017
View reviewed changes
Copy link
Collaborator

@harpere harpere left a comment

Choose a reason for hiding this comment

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

no tests for the new sync functionality, when window.SMARTSYNC evaluates to true?

src/adapters/serverbid.js Outdated
const sstimeout = window.SMARTSYNC_TIMEOUT || ((params.timeout || 500) / 2);
setTimeout(function() {
var cb = window[SMARTSYNC_CALLBACK];
window[SMARTSYNC_CALLBACK] = function() {};
Copy link
Collaborator

Choose a reason for hiding this comment

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

minor - as defined earlier on line 54, window[SMARTSYNC_CALLBACK] already redefines itself to an empty function, so doing it here again is redundant. So could just call window[SMARTSYNC_CALLBACK]();

Copy link
Contributor Author

@micha micha Jun 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

The reason for this setTimeout() call is to ensure that bids are fetched in the case that the callback (window[SMARTSYNC_CALLBACK]) is not called by the smartsync script in a timely manner. We also want to make sure that bids aren't fetched more than once. So consider the following cases:

  1. smartsync calls the callback promptly: this means that the timeout on line 63 has not yet fired, and we want to fetch bids and then set the callback to a no-op, so we don't fetch bids again when the timeout on line 63 fires.
  2. smartsync never calls the callback: this means that the timeout on line 63 will fire, fetch bids, and set the callback to a no-op.
  3. smartsync calls the callback after the timeout has fired: in this case we have already fetched bids and we don't want to fetch them again, so the callback must have already been set to a no-op. The code that would have done that is on line 65.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lines 64 and 65 handle the case where the callback is called after the timeout fires, so we don't fetch bids twice.

@micha micha force-pushed the serverbid-smartsync branch 2 times, most recently from 0304647 to 9148f00 Compare June 28, 2017 04:48
@micha
Copy link
Contributor Author

micha commented Jun 28, 2017

Tests added!

harpere
harpere previously approved these changes Jun 28, 2017
View reviewed changes
@harpere harpere added the LGTM label Jun 28, 2017
matthewlane
matthewlane reviewed Jul 13, 2017
View reviewed changes
modules/serverbidBidAdapter.js
} else {
window[SMARTSYNC_CALLBACK] = function() {
window[SMARTSYNC_CALLBACK] = function() {};
_callBids(params);
Copy link
Collaborator

Choose a reason for hiding this comment

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

@micha @harpere checking to make sure I understand this discussion from the previous PR #1301 (comment), what is the mechanism for publishers to opt in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @matthewlane the publisher must set window.SMARTSYNC = true before bids are fetched.

@micha micha force-pushed the serverbid-smartsync branch from cb53657 to 9148f00 Compare August 9, 2017 12:58
@mkendall07
Copy link
Member

@micha it looks like the travis build is failing because of the node version. Can you rebase your branch off master?

@jgrimes jgrimes force-pushed the serverbid-smartsync branch from 9148f00 to 8e5c29d Compare August 29, 2017 14:41
@jgrimes jgrimes force-pushed the serverbid-smartsync branch from 8e5c29d to 84cca96 Compare August 29, 2017 15:08
@jgrimes
Copy link
Contributor

jgrimes commented Aug 30, 2017

@mkendall07 I've rebased and fixed a linter issue (just an unused constant). Should be good to go!

@mkendall07 mkendall07 merged commit 66825d1 into prebid:master Aug 31, 2017
ptomasroos pushed a commit to happypancake/Prebid.js that referenced this pull request Sep 1, 2017
@micha @ptomasroos
Update serverbid adapter to use smartsync (prebid#1324)
539ef41
philipwatson pushed a commit to mbrtargeting/Prebid.js that referenced this pull request Sep 18, 2017
@micha @philipwatson
Update serverbid adapter to use smartsync (prebid#1324)
c51bbdc
outoftime pushed a commit to Genius/Prebid.js that referenced this pull request Sep 18, 2017
Merge tag '0.28.0' of https://github.com/prebid/Prebid.js into prebuilt
da960cc 
* tag '0.28.0' of https://github.com/prebid/Prebid.js: (27 commits)
  Prebid 0.28.0 Release
  Revert "Upgrade sinon to 3.x (prebid#1491)" (prebid#1563)
  add () for correct order of operations in scaling increments for currency (prebid#1559)
  AppnexusAst adapter update: Added source and version to request payload (prebid#1555)
  remove unnecessary spread operator (prebid#1561)
  Adxcg adapter (prebid#1554)
  Upgrade sinon to 3.x (prebid#1491)
  Rename vastPayload to vastXml (prebid#1556)
  Single-size sizes array now can be taken, too (prebid#1535)
  Updated the istanbul-instrumenter-loader (prebid#1550)
  Add AerServ Adapter (prebid#1538)
  Fixed imports and made adform support aliasing (prebid#1518)
  Custom granularity fix (prebid#1546)
  Fix `documentation lint` issues (prebid#1544)
  Yieldbot adunit bidder params slot name usage fix (prebid#1394)
  Update serverbid adapter to use smartsync (prebid#1324)
  Add improvedigitalBidAdapter (prebid#1381)
  Fix prebid#1533 spring server typo (prebid#1542)
  userSync is off by default (prebid#1543)
  currency module (prebid#1374)
  ...
jbAdyoulike pushed a commit to jbAdyoulike/Prebid.js that referenced this pull request Sep 21, 2017
@micha @jbAdyoulike
Update serverbid adapter to use smartsync (prebid#1324)
657549c
vzhukovsky added a commit to aol/Prebid.js that referenced this pull request Oct 12, 2017
@vzhukovsky
Merge pull request prebid#121 in AOLP_ADS_JS/prebid.js from release/1…
e797d03 
….28.0 to aolgithub-master

* commit '4d9ade3df767750743f8888ed9efd6c77f8d0050': (26 commits)
  Added changelog entry.
  Added new aol partners ids.
  Prebid 0.28.0 Release
  Revert "Upgrade sinon to 3.x (prebid#1491)" (prebid#1563)
  add () for correct order of operations in scaling increments for currency (prebid#1559)
  AppnexusAst adapter update: Added source and version to request payload (prebid#1555)
  remove unnecessary spread operator (prebid#1561)
  Adxcg adapter (prebid#1554)
  Upgrade sinon to 3.x (prebid#1491)
  Rename vastPayload to vastXml (prebid#1556)
  Single-size sizes array now can be taken, too (prebid#1535)
  Updated the istanbul-instrumenter-loader (prebid#1550)
  Add AerServ Adapter (prebid#1538)
  Fixed imports and made adform support aliasing (prebid#1518)
  Custom granularity fix (prebid#1546)
  Fix `documentation lint` issues (prebid#1544)
  Yieldbot adunit bidder params slot name usage fix (prebid#1394)
  Update serverbid adapter to use smartsync (prebid#1324)
  Add improvedigitalBidAdapter (prebid#1381)
  Fix prebid#1533 spring server typo (prebid#1542)
  ...
dluxemburg pushed a commit to Genius/Prebid.js that referenced this pull request Jul 17, 2018
@micha @dluxemburg
Update serverbid adapter to use smartsync (prebid#1324)
63a376d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewlane matthewlane matthewlane left review comments

@harpere harpere harpere left review comments

Assignees

@matthewlane matthewlane

Labels
LGTM needs review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@micha @mkendall07 @jgrimes @matthewlane @harpere @dbemiller