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

GDPR use getConsentData CMP call to get consentString #2603

Merged
merged 5 commits into from
May 24, 2018

Conversation

jsnellbaker
Copy link
Collaborator

Type of change

  • Bugfix

Description of change

Previously the consentManagement module was using the CMP function getVendorConsents to obtain all aspects of the user's consent information. It was pointed out in #2591 that this function is not the ideal one to use to obtain the user's consent information (specifically the encoded string).

This PR updates the logic of the module to use the getConsentData CMP function to grab the consentString and gdprApplies and then executes the getVendorConsents CMP function to grab the unparsed vendor consent information (to maintain backwards compatibility for that variable we offer to adapters).

@jsnellbaker jsnellbaker removed their assignment May 23, 2018
@mkendall07 mkendall07 self-assigned this May 23, 2018
callIabCMP('getConsentData', function (consentResponse) {
cmpResponse.getConsentData = consentResponse;

callIabCMP('getVendorConsents', function (consentResponse) {
Copy link
Member

Choose a reason for hiding this comment

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

why are you doing this serially and not in parallel ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wanted to only proceed (ie call cmpSuccess()) when I had the responses from both callbacks. This is to collect the results from both and store them into one object to process later. I'm not sure how to best organize having two parallel callbacks meld their results together.

Copy link
Contributor

@dbemiller dbemiller May 23, 2018

Choose a reason for hiding this comment

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

I can chat about this with you if you're interested.

It's much more straightforward with Promise-based APIs... but it can still be done with callback-based ones.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah I think parallel would be ideal, especially if these are making ajax requests. If they're not, it probably doesn't matter.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think these are making ajax requests - so it probably doesn't matter so much.

@jsnellbaker
Copy link
Collaborator Author

I have updated the logic to have the CMP calls execute in a parallel instead of serial as well as resolve the current conflicts with master. Please take another look and let me know of any additional feedback/suggestions.

Thanks.

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.

much cleaner logic now. Thanks!

@jsnellbaker
Copy link
Collaborator Author

Thanks for the approvals - merging in now.

@jsnellbaker jsnellbaker merged commit 8b6ff71 into master May 24, 2018
dluxemburg pushed a commit to Genius/Prebid.js that referenced this pull request Jul 17, 2018
* use getConsentData cmp to get consent info

* add changes for safeframe and iframe workflows

* update logic to execute CMP calls in parallel
@mkendall07 mkendall07 deleted the gdpr_change_cmp_calls branch August 17, 2018 15:13
ghost pushed a commit to devunrulymedia/Prebid.js that referenced this pull request Jan 30, 2019
* use getConsentData cmp to get consent info

* add changes for safeframe and iframe workflows

* update logic to execute CMP calls in parallel
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.

5 participants