Prebid 7: Allow USP module to work without explicit configuration #8229
Conversation
| @@ -289,6 +289,7 @@ function exitModule(errMsg, hookConfig, extraArgs) { | |||
| export function resetConsentData() { | |||
| consentData = undefined; | |||
| consentAPI = undefined; | |||
| consentTimeout = undefined; | |||
There was a problem hiding this comment.
this one stumped me, hat tip to @dgirardi for diagnosing why my 'should disable' test was failing.
modules/consentManagementUsp.js
Outdated
| @@ -298,18 +299,21 @@ export function resetConsentData() { | |||
| */ | |||
| export function setConsentConfig(config) { | |||
| config = config && config.usp; | |||
| if (!config || typeof config !== 'object') { | |||
| logWarn('consentManagement.usp config not defined, exiting usp consent manager'); | |||
| if (config && config.disable === true) { | |||
There was a problem hiding this comment.
this allows a publisher to restore the old behavior (exit immediately) with configuration.
There was a problem hiding this comment.
I don't think this is equivalent - it does not mean "disable" but "ignore". Let's say I do
pbjs.setConfig({
consentManagement: {
gpdr: {...}
}
});
pbjs.setConfig({
consentManagement: {
usp: {
disable: true
}
}
});
the first call would activate USP with the default values, the second would log a warning but otherwise do nothing - USP would still be enabled.
You could resetConsentData (and remove the requestBidsHook), but I can't think of another example where we ask for explicit deactivation. To me it seems cleaner if we either require explicit activation (maybe that means accepting usp: {} to indicate the default), or just make it clear that if you install the module, it's always active.
There was a problem hiding this comment.
if what you say is true, how is this test passing
it('should not run if disable is true', function () {
setConsentConfig({usp: {disable: true}});
expect(consentAPI).to.be.undefined;
expect(consentTimeout).to.be.undefined;
sinon.assert.callCount(utils.logWarn, 1);
});
There was a problem hiding this comment.
That test should be called "should not change settings if disable is true". Add a setConsentConfig({}) in the beginning and it should fail.
patmmccann
added
the
needs 2nd review
patmmccann
removed
needs 2nd review
|
Thanks for your review and changes @dgirardi ; I added docs PR as well |
Part of #7796