✨ amp-consent: Gdpr applies value by micajuine-ho · Pull Request #27759 · ampproject/amphtml

micajuine-ho · 2020-04-14T19:47:07Z

Adding support for TCF v2 gdprApplies value.

Allows gdprApplies to be true, false or null (when checkConsentHref is not defined)
~~Stored as g:(0|1|undefined) in local storage~~
If gdprApplies is not sent through checkConsentHref, it will default to remote consentRequired values (never default to local consentRequired).

Future work: cache gdprApplies value, add macro support for gdprApplies, consolidaste src/consent.js methods (gdprApplies, consentString, and consentType, to be sent together, updated together, and retrieved together)

Closes #27190

extensions/amp-consent/0.1/amp-consent.js

extensions/amp-consent/0.1/consent-info.js

zhouyx · 2020-04-14T22:31:01Z

src/consent.js

+ * @param {string} policyId
+ * @return {!Promise<boolean>}
+ */
+export function getGdprAppliesInfo(element, policyId) {


Is the plan to migrate to getConsentPolicyInfo and deprecate the method here?

See comments below

// Return the stored gdpr applies value. doesn't tell what the future plan is.

Oops I was unclear, the plan as discussed offline, is to consolidate the methods in src/consent to return a single consent info object, but for now we will keep this method to unblock the ads teams.

src/consent.js

extensions/amp-consent/0.1/amp-consent.js

zhouyx · 2020-04-14T22:44:29Z

extensions/amp-consent/0.1/amp-consent.js

        consentStateValue,
-        responseConsentString
+        responseConsentString,
+        responseGdprApplies


updating gdprApplies here won't work because it requires consentStateValue to be not null.
A typical workflow is as following

consentHref returns gdprApplies, but no consentStateValue.

Then the consentStateValue is calculated on the client side.

I'd recommend we handle the gdprApplies separate from consentState and consentString. Like what we do with the dirtyBit.
Let me know if you have better ideas.

~~Ok, thanks for addressing this. It was either treat it like consentString/consentStateValue or treat it like dirtyBit. I'm good to treat it like dirtyBit.~~

Edit: Discussed offline, will approach gdprApplies values similar to sharedData: it will only be stored in the Consent Instance and not local storage. As a future optimization, once the value starts to get used more by vendors, we will cache the gdprApplies value in local storage and use that value instead of having to wait for another round trip.

extensions/amp-consent/0.1/consent-state-manager.js

extensions/amp-consent/0.1/test/test-amp-consent.js

zhouyx · 2020-04-16T00:10:32Z

extensions/amp-consent/0.1/test/test-amp-consent.js

+          it('uses given value', async () => {
+            const inlineConfig = {
+              'consentInstanceId': 'abc',
+              'consentRequired': 'remote',


Please also add a test to test the local consentRequired: false or consentRequired: true as well.

Actually it will also work if you decide to limit the gdprApplies with checkConsentHref. But in that case, maybe we should not expose the getGdprAppliesInfo API. That it should only be retrieved with consent string.

Actually it will also work if you decide to limit the gdprApplies with checkConsentHref.

Yes this is currently the case. In the spec, we said that it would default to consentRequired and I interpreted that to mean that we check checkConsentHref first then use consentRequired if it's not given.

That it should only be retrieved with consent string

So are you saying that we should remove getGdprAppliesInfo and getConsentPolicyInfo from src/consent and create a general API (ie: getTcfV2Info())?

Ha the consentRequired defined in different places is confusing. What about the consentRequired in inline config?

Discussed offline: will save consolidation of these APIs for later work (by June), so that we can unblock ads teams that are needing the gdprApplies data.

micajuine-ho · 2020-04-17T20:07:13Z

@zhouyx Ready for review

extensions/amp-consent/0.1/consent-state-manager.js

extensions/amp-consent/0.1/test/test-amp-consent.js

zhouyx · 2020-04-17T23:21:08Z

src/consent.js

+ * @param {string} policyId
+ * @return {!Promise<boolean>}
+ */
+export function getGdprAppliesInfo(element, policyId) {


// Return the stored gdpr applies value. doesn't tell what the future plan is.

extensions/amp-consent/0.1/test/test-amp-consent.js

micajuine-ho · 2020-04-20T20:16:57Z

@zhouyx PTAL

zhouyx

#27759 (comment)

Can you add TODO? Thanks

zhouyx · 2020-04-22T00:36:04Z

extensions/amp-consent/0.1/test/test-amp-consent.js

+          it('defaults correctly when checkConsentHref is not defined', async () => {
+            const inlineConfig = {
+              'consentInstanceId': 'abc',
+              'consentRequired': false,


Does the inline consentRequired value affect the gdprApplies value?
My understanding is it won't, if so do you think we should set the value true instead to make more obvious that gdprApplies value is not related to the inline consentRequired value.

(Sign.... two fields with the same name is soooo confusing)

We use the inline consentRequired value when gdprApplies or checkConsentHref is not supplied. And since consentRequired will always be a valid value after consent-config.js merges it, it's a good fallback.

I've change the test name and values to reflect this.

Updated PR description to reflect or decision (never fallback to local consentRequired)

micajuine-ho · 2020-04-23T16:28:57Z

Can you add TODO

Added

@zhouyx Ready for review :D

zhouyx

LGTM! Thanks for adding the ${gdprApplies} support. cc @Leedehai

micajuine-ho · 2020-04-28T17:51:52Z

Thanks for adding the ${gdprApplies}

Just a note, macro support is not in this PR, but can be future work if anyone requires it :)

* Sync w/ cache from amp-consent * Tests * no caching for now * suggested changes * Never use local consentRequired

Micajuine Ho added 4 commits March 20, 2020 12:17

Sync w/ cache from amp-consent

3c944c6

Implementation

008c6b0

Merge branch 'master' into gdprApplies

1faa14a

Tests

38b8176

micajuine-ho requested a review from zhouyx April 14, 2020 19:47

googlebot added the cla: yes label Apr 14, 2020

micajuine-ho commented Apr 14, 2020

View reviewed changes

extensions/amp-consent/0.1/amp-consent.js Outdated Show resolved Hide resolved

amp-bundle-size bot requested a review from ajwhatson April 14, 2020 20:55

zhouyx suggested changes Apr 14, 2020

View reviewed changes

micajuine-ho force-pushed the gdprApplies branch from 78b443f to 5a4c3d8 Compare April 15, 2020 20:38

no caching for now

b2e9fd4

micajuine-ho force-pushed the gdprApplies branch from 5a4c3d8 to b2e9fd4 Compare April 15, 2020 20:40

micajuine-ho requested a review from zhouyx April 15, 2020 20:42

zhouyx reviewed Apr 16, 2020

View reviewed changes

suggested_changes

026f1b6

micajuine-ho force-pushed the gdprApplies branch from 8ba1a0d to 026f1b6 Compare April 17, 2020 20:05

micajuine-ho requested a review from zhouyx April 17, 2020 20:07

zhouyx reviewed Apr 17, 2020

View reviewed changes

zhouyx reviewed Apr 22, 2020

View reviewed changes

micajuine-ho force-pushed the gdprApplies branch from 58fab1b to 7349144 Compare April 23, 2020 16:31

micajuine-ho requested a review from zhouyx April 23, 2020 17:25

suggested changes

f9e8bd4

micajuine-ho force-pushed the gdprApplies branch 2 times, most recently from 1c03af8 to a68fac0 Compare April 27, 2020 22:04

Never use local consentRequired

3d78a81

micajuine-ho force-pushed the gdprApplies branch from a68fac0 to 3d78a81 Compare April 27, 2020 22:15

micajuine-ho removed the request for review from ajwhatson April 27, 2020 22:17

zhouyx approved these changes Apr 27, 2020

View reviewed changes

micajuine-ho merged commit 8ca8b63 into ampproject:master Apr 28, 2020

micajuine-ho mentioned this pull request May 4, 2020

amp-consent metadata values #28187

Merged

ampprojectbot added the PR use: In Beta / Experimental label May 5, 2020

ldoroshe pushed a commit to ldoroshe/amphtml that referenced this pull request May 8, 2020

✨ amp-consent: gdprApplies value (ampproject#27759)

67ecc54

* Sync w/ cache from amp-consent * Tests * no caching for now * suggested changes * Never use local consentRequired

ampprojectbot added the PR use: In Stable label May 14, 2020

Conversation

micajuine-ho commented Apr 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

micajuine-ho Apr 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhouyx Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

micajuine-ho commented Apr 17, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

micajuine-ho commented Apr 20, 2020

Uh oh!

zhouyx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

micajuine-ho commented Apr 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhouyx left a comment

Choose a reason for hiding this comment

Uh oh!

micajuine-ho commented Apr 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

micajuine-ho commented Apr 14, 2020 •

edited

Loading

micajuine-ho Apr 15, 2020 •

edited

Loading

zhouyx Apr 16, 2020 •

edited

Loading

micajuine-ho commented Apr 23, 2020 •

edited

Loading