🧪✨ [amp-consent] Allow purposeConsentRequired inline and via endpoint response#32433
Merged
micajuine-ho merged 3 commits intoampproject:masterfrom Feb 5, 2021
Merged
Conversation
10 tasks
rsimha
approved these changes
Feb 4, 2021
Contributor
rsimha
left a comment
There was a problem hiding this comment.
build-system/ change LGTM.
dmanek
approved these changes
Feb 5, 2021
| * @param {ConsentInfoDef} opt_consentInfo | ||
| * @return {!Promise<boolean>} | ||
| */ | ||
| checkGranularConsentRequired_(opt_consentInfo) { |
Contributor
There was a problem hiding this comment.
opt_consentInfolooks unused. Assuming it will be used later?- More of an AMP practice question - instead of using
opt_prefix, can we use default params?
Contributor
Author
There was a problem hiding this comment.
Yup! Unused for now, but we're going to use it in the future to determine if the stored purpose consents match with the publisher defined purposeConsentRequired field. If all the purposes have consents, then we can unblock, otherwise we should should the consent prompt.
More of an AMP practice question - instead of using opt_ prefix, can we use default params?
Both are allowed in the codebase. There's also the unused- prefix.
| /** | ||
| * Get `purposeConsentRequired` from consent config, | ||
| * or from `checkConsentHref` response. | ||
| * @return {!Promise} |
Contributor
There was a problem hiding this comment.
nit: add type like on line 691 below.
| doc.body.appendChild(consentElement); | ||
| ampConsent = new AmpConsent(consentElement); | ||
| await ampConsent.buildCallback(); | ||
| expect(await ampConsent.getPurposeConsentRequired_()).to.undefined; |
Contributor
There was a problem hiding this comment.
- test description say
returns null..., please update to.be.undefined
66fda13 to
120ec1c
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Partial for #31607
Introduce the
amp-consent-granular-consentexperiment for the launch of the granular consent feature.When initializing consent (not the syncing case), if we already have a global consent stored (tcString or consentStateValue of
acceptedorrejected), then collectpurposeConsentRequiredfield from inline config or from the cached remote response.Currently we only validate that it's an array (if it's there), in the future we will compare it to our stored granular consent values to determine if we have collected the necessary consents to bypass showing the consent UI.