Allow sending RTC with consentState policy adherence #15951
Conversation
| /** | ||
| * Returns whether a given callout object is valid to send an RTC request | ||
| * to, for the given consentState. | ||
| * @param {Object|string} callout |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
build-system/amp.extern.js
Outdated
| @@ -445,3 +445,10 @@ AMP.require; | |||
| * @typedef {function(number, boolean):?|function(number):?} | |||
| */ | |||
| var TransitionDef; | |||
|
|
|||
| const CONSENT_POLICY_STATE = { | |||
There was a problem hiding this comment.
Please add /** @enum {number} ... .
build-system/amp.extern.js
Outdated
| * See src/consent-state.js for more information. | ||
| * @enum {number} | ||
| */ | ||
| const CONSENT_POLICY_STATE = { |
There was a problem hiding this comment.
Given this is now externed can we delete src/consent-state.js (and carry-over documentation)?
| /** | ||
| * Returns whether a given callout object is valid to send an RTC request | ||
| * to, for the given consentState. | ||
| * @param {Object|string} callout |
There was a problem hiding this comment.
calloutConfig sounds like a better name IMO
| * Returns whether a given callout object is valid to send an RTC request | ||
| * to, for the given consentState. | ||
| * @param {Object|string} callout | ||
| * @return {boolean} |
| return false; | ||
| } | ||
|
|
||
| const sendRegardlessOfConsentState = |
There was a problem hiding this comment.
switch (typeof callout.sendRegardlessOfConsentState) {
case 'boolean':
return callout.sendRegardlessOfConsentState;
case 'Array':
return sendRegardlessOfConsentState.includes(this.consentState_);
default:
user().warn(TAG, 'WHAT?! ...');
return false;
}
| * custom URL. | ||
| */ | ||
| modifyRtcConfigForConsentStateSettings() { | ||
| if (!(this.consentState_ == CONSENT_POLICY_STATE.INSUFFICIENT || |
There was a problem hiding this comment.
Duplicate code? Can you just call isValidCalloutForConsentState with the global value? Also what if global sendRegardlessOfConsentState is false but individual callout is true (or array with value)?
There was a problem hiding this comment.
done, and the current setup is that the default is the global is false, and any individual can override.
| // 'macros'. This is for backwards compatability. | ||
| const vendorMacros = | ||
| this.rtcConfig_.vendors[vendor]['macros'] && | ||
| isObject(this.rtcConfig_.vendors[vendor]['macros']) ? |
There was a problem hiding this comment.
Do you need the first check here? I would assume isObject returns false if undefined.
|
This pull request introduces 1 alert when merging 0eda166 into 289a745 - view on LGTM.com new alerts:
Comment posted by LGTM.com |
|
This pull request introduces 1 alert when merging 5f9bd7a into 289a745 - view on LGTM.com new alerts:
Comment posted by LGTM.com |
| customMacros = this.assignMacros(customMacros); | ||
| this.rtcStartTime_ = Date.now(); | ||
| this.handleRtcForCustomUrls(customMacros); | ||
| this.handleRtcForVendorUrls(customMacros); | ||
| return Promise.all(this.promiseArray_); | ||
| return this.promiseArray_.length ? Promise.all(this.promiseArray_) : |
There was a problem hiding this comment.
I would assume that Promise.all empty array is equivalent to Promise.resolve... Why return null?
There was a problem hiding this comment.
I thought the same thing but I was having issues in testing where that was not the case
There was a problem hiding this comment.
This still surprises me... r u sure?
| } | ||
| return false; | ||
| } | ||
| user().warn(TAG, 'Invalid value for sendRegardlessOfConsentState'); |
There was a problem hiding this comment.
Include callout info?
| if (isArray(sendRegardlessOfConsentState)) { | ||
| for (let i = 0; i < sendRegardlessOfConsentState.length; i++) { | ||
| if (this.consentState_ == | ||
| CONSENT_POLICY_STATE[sendRegardlessOfConsentState[i]]) { |
There was a problem hiding this comment.
We could warn here when a constent state given does not exist in the enum
There was a problem hiding this comment.
Could do a switch here:
switch (sendRegardlessOfConsentState[i]) {
case this.consentState_:
return true;
case undefined:
Log... break;
}
| urls.push(this.rtcConfig_.urls[i]); | ||
| } | ||
| } | ||
| this.rtcConfig_.urls = urls; |
There was a problem hiding this comment.
this.rtcConfig_.urls = this.rtcConfig_.urls.filter(url => this.isValidCalloutForConsentState(url));
| return; | ||
| } | ||
|
|
||
| if (this.isValidCalloutForConsentState(this.rtcConfig_)) { |
There was a problem hiding this comment.
So global overrides the value on a per callout/vendor level? Image pub has:
{vendors: {foo: {sendRegardlessOfConsentState: ['unknown']}}, sendRegardlessOfConsentState: true}
If consent state is insufficient should we call out to vendor foo? I would argue no.
| if (isArray(sendRegardlessOfConsentState)) { | ||
| for (let i = 0; i < sendRegardlessOfConsentState.length; i++) { | ||
| if (this.consentState_ == | ||
| CONSENT_POLICY_STATE[sendRegardlessOfConsentState[i]]) { |
There was a problem hiding this comment.
Could do a switch here:
switch (sendRegardlessOfConsentState[i]) {
case this.consentState_:
return true;
case undefined:
Log... break;
}
| * Returns whether a given callout object is valid to send an RTC request | ||
| * to, for the given consentState. | ||
| * @param {Object|string} calloutConfig | ||
| * @param {boolean=} optIsGloballyValid |
There was a problem hiding this comment.
You could make this not optional and just pass false when calling on the global object
| customMacros = this.assignMacros(customMacros); | ||
| this.rtcStartTime_ = Date.now(); | ||
| this.handleRtcForCustomUrls(customMacros); | ||
| this.handleRtcForVendorUrls(customMacros); | ||
| return Promise.all(this.promiseArray_); | ||
| return this.promiseArray_.length ? Promise.all(this.promiseArray_) : |
There was a problem hiding this comment.
This still surprises me... r u sure?
See #15588