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

fix issue with GDPR module in concurrent auctions #2612

Merged
merged 2 commits into from
May 24, 2018

Conversation

jsnellbaker
Copy link
Collaborator

@jsnellbaker jsnellbaker commented May 24, 2018

Type of change

  • Bugfix

Description of change

To address issue reported in #2586

The current consentManagement module interferes with requests in concurrent auctions (ie multiple requestBids on a page) from firing off/finishing as expected. This is due to the global variables being overwritten and the context of the previous auction instances being lost.

This change stores the variables needed for the consentManagement module in an config object that gets passed along through the workflow of the module to remove the need for global variables. This preserves the needed context for the multiple auctions to complete as expected.

To easily replicate the issue:

  • setup a basic test page that uses a IAB compliant CMP and two adunit objects
  • pass each of these objects into a distinct requestBids call (so there's 1 adunit for each)
  • add the consentManagement config to enable the module
  • delete your euconsent cookie (in case you had consent previously)
  • during next page load when you get prompted for consent, wait a few seconds then give consent

Both ads on the page should render successfully (with consent information included in both). Previously only one of ads would render and nothing would be shown for the other.

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. Thanks!

@YOzaz
Copy link

YOzaz commented May 24, 2018

By any chance, will this be merged soon? We have lots of lazy loading ads...

@jsnellbaker jsnellbaker merged commit 090ae4f into master May 24, 2018
@jsnellbaker
Copy link
Collaborator Author

@YOzaz Now that it's merged, it should be included in the next release (which is scheduled for later today).

@YOzaz
Copy link

YOzaz commented May 24, 2018

@jsnellbaker thank you thank you thank you. We're running out of time, so will probably grab 1.12.0-pre for today as it is, and will update to official release tomorrow.

dluxemburg pushed a commit to Genius/Prebid.js that referenced this pull request Jul 17, 2018
@mkendall07 mkendall07 deleted the gdpr_multi_request_fix branch August 17, 2018 15:13
ghost pushed a commit to devunrulymedia/Prebid.js that referenced this pull request Jan 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants