✨[amp-consent] block buildcallback through granular consent#32990
✨[amp-consent] block buildcallback through granular consent#32990micajuine-ho merged 6 commits intoampproject:masterfrom
Conversation
e3c864e to
35388d7
Compare
| } | ||
| return true; | ||
| }) | ||
| .then((hasPurposeConsents) => { |
There was a problem hiding this comment.
As far as I can tell, nothing in the above .then handler is asynchronous or returning a promise. Rather than extending the promise chain, this chunk could live in the function/handler above
There was a problem hiding this comment.
Just thought it would be cleaning to separate these ideas, but I can fix and also insure clarity.
There was a problem hiding this comment.
Another option if you wanted separation/clarity without extra promises might be defining hasPurposeConsents(purposeConsentsRequired) as its own method and making the full promise handler
.then((purposeConsentsRequired) => {
if (hasPurposeConsents(purposeConsentsRequired) {
this.consentStateManager_.hasAllPurposeConsents();
return true;
}
return false;
});Feel free to ignore this, just offering alternative
| } | ||
| return true; | ||
| }) | ||
| .then((hasPurposeConsents) => { |
842ce95 to
da447c2
Compare
da447c2 to
efad544
Compare
| }); | ||
|
|
||
| afterEach(() => { | ||
| toggleExperiment(win, 'amp-consent-granular-consent', false); |
There was a problem hiding this comment.
FMI: do we explicitly need to toggle experiments off or are they reset each time anyway?
There was a problem hiding this comment.
Explicit is better to avoid memory leaks.
There was a problem hiding this comment.
Any particular reason for doing beforeEach/afterEach instead of beforeAll/afterAll?
There was a problem hiding this comment.
We are not allowed to use beforeAll/afterAll
samouri
left a comment
There was a problem hiding this comment.
Changes to custom-element LGTM
| * Wait for initial consent information to be transmitted, | ||
| * then get consent state for this purpose. | ||
| * | ||
| * Note: Even if we have some intiial consent info, wait until all |
| * when to resolve the promis (via update() or directly). | ||
| * @return {!Promise} | ||
| */ | ||
| whenHasAllPurposeConsents() { |
There was a problem hiding this comment.
nit: rename this method? Perhaps just getHasAllPurposeConsents?
There was a problem hiding this comment.
I agree the naming here can add some confusion (esp. since hasFooBar is frequently used for boolean flags/checks rather than mutation functions). WDYT about something like:
constructor() {
...
this.hasAllPurposeConsentsDeferred = new Deferred();
...
}
get hasAllPurposeConsents() {
return this.hasAllPurposeConsentsDeferred.promise;
}
get hasAllPurposeConsentsResolver() {
return this.hasAllPurposeConsentsDeferred.resolve;
} A bit more verbose, but maybe being explicit could make it clearer?
| }); | ||
|
|
||
| afterEach(() => { | ||
| toggleExperiment(win, 'amp-consent-granular-consent', false); |
There was a problem hiding this comment.
Any particular reason for doing beforeEach/afterEach instead of beforeAll/afterAll?
| * when to resolve the promis (via update() or directly). | ||
| * @return {!Promise} | ||
| */ | ||
| whenHasAllPurposeConsents() { |
There was a problem hiding this comment.
I agree the naming here can add some confusion (esp. since hasFooBar is frequently used for boolean flags/checks rather than mutation functions). WDYT about something like:
constructor() {
...
this.hasAllPurposeConsentsDeferred = new Deferred();
...
}
get hasAllPurposeConsents() {
return this.hasAllPurposeConsentsDeferred.promise;
}
get hasAllPurposeConsentsResolver() {
return this.hasAllPurposeConsentsDeferred.resolve;
} A bit more verbose, but maybe being explicit could make it clearer?
Partial for #31607
Add the runtime check for the
data-block-on-consent-purposesattribute, and block if not all the purpose consents have been collected and are accepted.Since our UI prompts if we don't have all the consents gathered, don't start the check to unblock elements that are blocked by granular consent until the UI closes and we have the purpose consents or we relying on the sync.
For syncing, if we already have all the purpose consents stored, then the UI won't show and we should unblock. If we don't have the consent purposes stored, we will store them through the update, and this will allow the policy manager to signal to runtime which element to unblock.
This notion of waiting until we collect the purpose consents is handled by the consent state manager since it communicates with amp-consent (UI and syncing) and the policy manager.