-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Ability to statically pass consentData #2636
Ability to statically pass consentData #2636
Conversation
}); | ||
it('results in user settings overriding system defaults', () => { | ||
let staticConfig = { | ||
cmpApi: 'static', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to be part of a consentManagement
object:
pbjs.setConfig({
consentManagement: {
cmpApi: 'iab',
timeout: 8000,
allowAuctionWithoutConsent: false
}
});
This issue exists in another test in this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This setup is working because the setConfig
used in the unit test file is calling the local version of the function that's inside consentManagement.js
; it's not being routed through the config.setConfig()
function that requires config object names.
From an off-line discussion, I plan to rewrite the unit tests to better reflect usage; ideally after this PR is finished.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All right! Cool! Thanks for the clarification
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
In general I'm good with pubs being in charge of passing consent values. Don't have a strong opinion about implementation approach. In any case, there will need to be docs changes to http://prebid.org/dev-docs/modules/consentManagement.html |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ptomasroos In addition to @bretg's suggestions for a documentation update, I found one other point that should be reviewed. Once those are in place, these changes should be fine.
modules/consentManagement.js
Outdated
staticConsentData = config.consentData; | ||
consentTimeout = 0; | ||
} else { | ||
utils.logInfo(`consentManagement config with cmpApi: 'static' did not specify consentData. No consents will be available to adapters.`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should likely be an logWarn
or logError
instead of a logInfo
type message; as this is a negative situation that should be highlighted/easy to spot for a dev troubleshooting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, will fix!
Hey, I'd like to push forward this pull request. @ptomasroos can you help with this? I tried to help by updating the documentation. Here is my branch: I am not sure about usage of the changes you made. Please check if I understand correctly. Also I guess the actual pull request should be opened when this one is ready to be merged. Remaining changes I guess are quite minor, would you have time to deal with them? Thanks. |
I will move on with this next week since I'm currently a bit busy on schedule @polapi |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Hi @protomasroos . You mentioned you'll push the pull request forward last week. Is there anything I can help with to make it happen? |
Hi @protomasroos, It seems you don't have time to work on this. Maybe it would be better to close the pull request so maybe someone else would implement this functionality (instead thinking you're working on this)? |
I'm on vacation, feel free to fork otherwise i will complete this within a few weeks when coming back. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Good news coming up, will fix this during next week when I'm back at work since we need it now as well |
@ptomasroos How is it going? I guess you will again announce you are going to work on this next week? |
@polapi right, its not easy to be CEO, CTO, CMO, COO and CCO. But I will do my best to complete this, since you're having plenty of free time, feel free to press the fork button and complete this? Otherwise I will move on to do this when this is highest on my prio list. Most likely next week |
a7ce552
to
614d4b2
Compare
I've now rebased and fixed the minor @jsnellbaker , created the documentation as requested by @bretg |
The only thing I'm really thinking about here is the format the static data is passed in. Because currently it expects the output from the iab cmp, and when using a lib like the consent-string (from iab) which decodes and encodes the consent string's its returned in another format. It would probably be wise at least that we match the format in some way from these general libraries published from iab. https://github.com/InteractiveAdvertisingBureau/Consent-String-SDK-JS Feels like the consentManagement.js expects the results from a iab __cmp implementation and we should probably try to avoid this. And maybe just pass the consent-string into it instead and let prebid decode? Opinions? |
}); | ||
it('results in user settings overriding system defaults', () => { | ||
let staticConfig = { | ||
cmpApi: 'static', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
@ptomasroos Thanks for putting the additional changes together. In regards to your additional point, currently the module is meant to access the consent data from CMP and pass it along down-stream for other sources (ad-servers, etc) to read/parse. If we were to add responsibility for Prebid to decode/encode the consent values and handle it properly as well as change the type of data we'd be passing to the adapters (if I'm reading that right?), it'd likely require some additional effort that I think falls outside the scope of this PR. If you want to pursue this topic further, can you please open a new issue? There we can have a cleaner (ie not getting bogged down with PR changes in the conversation history) and more focused discussion on the proposed changes and the desired scope of the module. |
* Added static consentData to consentManagement config * Added test for static cmpApi * logError instead of logInfo
* Added static consentData to consentManagement config * Added test for static cmpApi * logError instead of logInfo
* Added static consentData to consentManagement config * Added test for static cmpApi * logError instead of logInfo
* Added static consentData to consentManagement config * Added test for static cmpApi * logError instead of logInfo
* Added static consentData to consentManagement config * Added test for static cmpApi * logError instead of logInfo
* Added static consentData to consentManagement config * Added test for static cmpApi * logError instead of logInfo
* Added static consentData to consentManagement config * Added test for static cmpApi * logError instead of logInfo
Type of change
Description of change
For some services its reasonable to not render the cmp api on every page view when a given user has already given consent and change this in his or hers settings. We only have 100% signed in users on our impressions and it would be very valuable for us to be able to set the consentData staticly on page rendering in order to speed up loading and retrieval of consentData.
Other information
Also reported in #2567 the ability to avoid using cmp, I think its fair to use a CMP to generate the consentData in order for it to be compliant but at least pass it when its not changed or altered through the config