Adding consentStringType to amp-consent metadata by micajuine-ho · Pull Request #28028 · ampproject/amphtml

micajuine-ho · 2020-04-24T22:04:48Z

After adding consentMetadata object in #28187, we are now allowing that object to contain consentStringType field.

consentMetadata is accepted from checkConsentHref and the external iframe consent flow's response. Accepted values of consentStringType are 1, 2, and 3 (mapping will be defined in the amp-consent.md after all changes hit prod. For now see src/consent-state.js).

{
   "consentMetadata": {
      "consentStringType": 1
   }
}

consentMetadata will be sent as a ConsentMetadataDef as part of the checkConsentHref and onUpdateHref requests, as well as part of the ClientInfo sent to external iframe flow.
consentMetadata is only valid if accompanied by a consentString,
consentMetadata will get converted to a ConsentMetadataDef stored within ConsentInfo in memory, and sent to the Consent State Manager.
When the Consent State Manager stores the ConsentInfo to local storage, ConsentMetadataDef will also get stored within it using the METADATA_STORAGE_KEY:

{'s':1, 'r':'consent-string', 'm':{'cst':1}}

ConsentMetadataDef will be accessed internally through src/consent#getConsentMetadata(), via the Consent Policy Manager

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

zhouyx · 2020-04-24T22:56:04Z

Thank you for the PR description. Very helpful.
Before I start review, could you please explain the design around the following points.

consentType is strictly related to consentString. If so how the code address the relationship between consentState, consentString and consentType with code.
consentType always comes with the consentString value. Where and how can it be specified and retrieved? (Endpoint, API)
Is the design backward compatible?

Thank you

micajuine-ho · 2020-04-25T00:08:21Z

@zhouyx Thanks for your questions.

consentType is strictly related to consentString. If so how the code address the relationship between consentState, consentString and consentType with code.

consentType and consentString values are:

gathered from the response from checkConsentHref (and potentially iframe post message, see comment above)
always get updated together via the consent manager
stored together in
- local storage
- in memory within the policy manager for the consent instance
both retrieved from the policy manager

consentType always comes with the consentString value. Where and how can it be specified and retrieved? (Endpoint, API)

Specified from checkConsentHref response (amp-consent#syncRemoteConsentState_() => amp-consent#updateCacheIfNotNull())
Potentially specified from iframe post message (see above)?
Retrieved from src/consent#getPolicyTypeInfo()

Is the design backward compatible?

Yes, we've only added fields, and if the field isn't present, no values will be stored in the policy manager or local storage. Once we consolidate the API to retrieve these values will change, but since they're all inhouse APIs we can make the necessary changes.

zhouyx · 2020-04-29T01:04:05Z

Thanks! Sounds good. One more question, looking at the code one could have consentString undefined but consentType defined. How are we going to handle such corner case?

micajuine-ho · 2020-04-29T17:31:46Z

How are we going to handle such corner case?

@zhouyx As of now, the consentType would get updated, regardless of the undefined consentString. I think it's a fair to add the restriction that if consentString is undefined, then we should treat consentType as undefined as well (and update accordingly).

WDYT?

zhouyx · 2020-04-29T21:52:24Z

I think it's a fair to add the restriction that if consentString is undefined, then we should treat consentType as undefined as well (and update accordingly).

Sounds good. I think the same rule should be applied to gdprApplies as well.

micajuine-ho · 2020-04-29T22:54:13Z

Sounds good. I think the same rule should be applied to gdprApplies as well.

@zhouyx Applied the fix (+ test). PTAL

zhouyx · 2020-04-29T23:07:51Z

To better address the map between consent string. What do you think about renaming the variable consent_string_type?

micajuine-ho · 2020-04-30T18:14:52Z

What do you think about renaming the variable consent_string_type?

I think this will definitely help clarify the mapping.

examples/amp-consent/diy-consent.html

lgtm-com · 2020-05-21T20:23:39Z

This pull request introduces 1 alert when merging 15fe60e into f92fb89 - view on LGTM.com

new alerts:

1 for Invocation of non-function

micajuine-ho · 2020-05-26T23:18:31Z

@zhouyx I have update the PR description so it accurately reflects the PR.

qq: Should src/consent getConsentMetadata() have a real closure type? And if so, do we create a new one in src/metadata-type or can we import from amp-consent/0.1/consent-info?

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

zhouyx · 2020-05-27T20:15:41Z

qq: Should src/consent getConsentMetadata() have a real closure type? And if so, do we create a new one in src/metadata-type or can we import from amp-consent/0.1/consent-info?

My understanding is the real closure type doesn't help much because we need to keep the key name of each field. (You can't minimize 'consentStringType' to 'c' during compilation).
Keeping in within the amp-consent component is reasonable, because it's not a global extern.

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

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

zhouyx · 2020-05-28T20:21:08Z

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

        instance.update(CONSENT_ITEM_STATE.ACCEPTED, 'test', {});
        yield macroTask();
        expect(storageSetSpy).to.be.calledTwice;
+        instance.update(CONSENT_ITEM_STATE.ACCEPTED, 'test');


metadata {} and undefined is different?

Yes, consentMetadata from server response could be undefined and {} resulting in undefined or empty ConsentMetadataDef respectfully. Should we treat these as the same (akaundefined response generates an empty ConsentMetadataDef)?

Depends on if undefined and {} will be treated differently by vendors, and in localStorage.
My understanding is that the two value (and even null) means the same thing.

Ok do you think it's fine if I leave it as is, incase vendor does need that distinction?

zhouyx · 2020-05-28T20:32:27Z

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

        expect(requestBody.consentString).to.equal('old');
        expect(requestBody.consentMetadata).to.be.undefined;
-        yield instance.update(CONSENT_ITEM_STATE.ACCEPTED, 'new', {});
+        yield instance.update(


I'm fine with this since you've already added the metadata to the test.
However I also find it unnecessary with this unit test and many tests above. The test here is to specifically cover consentString change, in the spirit of unit test consentMetadata value should not affect the test.
Ideally the optional value consentMetadata will only be added to related tests to keep the unit tests clean.
To add test coverage: It's great to add more unit tests target consentMetadata functionalities. And add the value to the integration test case.

Good point. I will be more selective in the places where I test future metadata functionality.

It's great to add more unit tests target consentMetadata functionalities

Are there particular functionalities I am missing? I think most of the functionality is handled by test-consent-info.js

And add the value to the integration test case.

In our e2e tests, we check the blocking behavior and but not the local storage values. Should this be changed so that we can check for the correct metadata?

The test coverage on functionalities looks good to me.

Please remind me, I think we didn't check local storage value for a reason.
We can add e2e test coverage once we expose API to get metadata. (or even use analytics variable substitution)

Please remind me, I think we didn't check local storage value for a reason.

We felt that e2e test was not the place to access localstorage, and instead relied upon outcome of test (blocked elements vs non-blocked) as the indicator of localstorage values. We also used analytics substitutions to verify as that was an end behavior of the test.

We have now have the exposed API and once we finish all the metadata value implementations, I will create e2e cases to test (#28634)

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

extensions/amp-consent/amp-consent.md

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

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

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

zhouyx · 2020-05-29T20:45:45Z

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

        expect(requestBody.consentString).to.equal('old');
        expect(requestBody.consentMetadata).to.be.undefined;
-        yield instance.update(CONSENT_ITEM_STATE.ACCEPTED, 'new', {});
+        yield instance.update(


The test coverage on functionalities looks good to me.

Please remind me, I think we didn't check local storage value for a reason.
We can add e2e test coverage once we expose API to get metadata. (or even use analytics variable substitution)

extensions/amp-consent/amp-consent.md

src/consent-state.js

zhouyx · 2020-05-29T20:52:00Z

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

        instance.update(CONSENT_ITEM_STATE.ACCEPTED, 'test', {});
        yield macroTask();
        expect(storageSetSpy).to.be.calledTwice;
+        instance.update(CONSENT_ITEM_STATE.ACCEPTED, 'test');


Depends on if undefined and {} will be treated differently by vendors, and in localStorage.
My understanding is that the two value (and even null) means the same thing.

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

src/consent-state.js

zhouyx · 2020-06-01T19:33:36Z

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

 */

+import {
+  CONSENT_STRING_TYPE, // eslint-disable-line no-unused-vars


nit: You should be able to get rid of this by using {../../../src/consent-state.CONSENT_STRING_TYPE}

hmm I don't see this anywhere in the codebase and I get an error when I do this... Anywhere I can see an example of this?

For example https://github.com/ampproject/amphtml/blob/master/extensions/amp-a4a/0.1/amp-a4a.js#L271

please remove the eslint comment. we no longer need it

zhouyx · 2020-06-01T19:41:58Z

#28028 (comment)

Ok do you think it's fine if I leave it as is, incase vendor does need that distinction?

I can't reply to the origin comment thread 😢 Reply here

Sure if vendor does need the distinction. The tradeoff here is one need to check for the metadata value before accessing any field.

const metaData = getMetaData();
if (metaData && metaData['something'])

I'm good with whatever vendors think works best for them.

micajuine-ho · 2020-06-01T22:06:02Z

I'm good with whatever vendors think works best for them.

Ok, I will follow up, and make changes if necessary.

zhouyx

Thanks Let's see if we can get rid of the eslint-disable-line. LGTM

zhouyx · 2020-06-01T23:41:06Z

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

 */

+import {
+  CONSENT_STRING_TYPE, // eslint-disable-line no-unused-vars


For example https://github.com/ampproject/amphtml/blob/master/extensions/amp-a4a/0.1/amp-a4a.js#L271

zhouyx

LGTM with one nit

micajuine-ho requested a review from zhouyx April 24, 2020 22:04

googlebot added the cla: yes label Apr 24, 2020

micajuine-ho commented Apr 24, 2020

View reviewed changes

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

micajuine-ho force-pushed the consent_type branch from 9e4482a to 0e09747 Compare April 27, 2020 19:23

init

1b6de4d

micajuine-ho force-pushed the consent_type branch from 0e09747 to 1b6de4d Compare April 27, 2020 20:22

Merge branch 'master' into consent_type

71724bb

Consent string undefined case

e47d6ad

better mapping and external consent flow

a319a1b

micajuine-ho force-pushed the consent_type branch from 583fc77 to a319a1b Compare April 30, 2020 20:40

zhouyx reviewed Apr 30, 2020

View reviewed changes

examples/amp-consent/diy-consent.html Outdated Show resolved Hide resolved

micajuine-ho mentioned this pull request May 4, 2020

amp-consent metadata values #28187

Merged

Micajuine Ho added 2 commits May 20, 2020 11:47

Merge branch 'master' into consent_type

315a0cc

Merge branch 'master' into consent_type

15fe60e

micajuine-ho force-pushed the consent_type branch 4 times, most recently from 5f6e131 to b11a080 Compare May 26, 2020 16:28

micajuine-ho changed the title ~~Adding CONSENT_TYPE for TCF_V2 support~~ Adding consentStringType to amp-consent metadata May 26, 2020

micajuine-ho force-pushed the consent_type branch from b11a080 to d2ef3d1 Compare May 26, 2020 23:25

Combining consentStringType into metadata

9de1aab

micajuine-ho force-pushed the consent_type branch from d2ef3d1 to 9de1aab Compare May 26, 2020 23:37

Merge branch 'master' into consent_type

6eccd85

zhouyx reviewed May 27, 2020

View reviewed changes

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

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

micajuine-ho force-pushed the consent_type branch from a54febe to 6505695 Compare May 28, 2020 19:34

micajuine-ho requested a review from zhouyx May 28, 2020 19:35

Remove enum mapping

f1edd2c

micajuine-ho force-pushed the consent_type branch from 6505695 to f1edd2c Compare May 28, 2020 19:36

zhouyx suggested changes May 28, 2020

View reviewed changes

micajuine-ho force-pushed the consent_type branch from 3c753fb to 28a92e2 Compare May 29, 2020 17:22

ampproject deleted a comment from lgtm-com bot May 29, 2020

zhouyx suggested changes May 29, 2020

View reviewed changes

micajuine-ho mentioned this pull request May 29, 2020

Add e2e test for all amp-consent metadata values #28634

Closed

micajuine-ho force-pushed the consent_type branch from 28a92e2 to 7fa3abb Compare May 29, 2020 22:22

micajuine-ho requested a review from zhouyx May 29, 2020 22:25

zhouyx reviewed Jun 1, 2020

View reviewed changes

suggested changes

5280d3b

micajuine-ho force-pushed the consent_type branch from 7fa3abb to 5280d3b Compare June 1, 2020 22:30

zhouyx reviewed Jun 1, 2020

View reviewed changes

zhouyx approved these changes Jun 2, 2020

View reviewed changes

Merge branch 'master' into consent_type

2ac9663

micajuine-ho merged commit b1cdabd into ampproject:master Jun 2, 2020

micajuine-ho mentioned this pull request Jun 12, 2020

[Discussion] <amp-consent> TCFv2 Support #26229

Open

ampprojectbot added the PR use: In Stable label Jun 16, 2020

Conversation

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

Uh oh!

Uh oh!

Uh oh!

zhouyx commented Apr 24, 2020

Uh oh!

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

Uh oh!

Uh oh!

zhouyx commented Apr 29, 2020

Uh oh!

micajuine-ho commented Apr 29, 2020

Uh oh!

zhouyx commented Apr 29, 2020

Uh oh!

micajuine-ho commented Apr 29, 2020

Uh oh!

zhouyx commented Apr 29, 2020

Uh oh!

micajuine-ho commented Apr 30, 2020

Uh oh!

Uh oh!

lgtm-com bot commented May 21, 2020

Uh oh!

micajuine-ho commented May 26, 2020

Uh oh!

Uh oh!

Uh oh!

zhouyx commented May 27, 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!

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!

micajuine-ho May 29, 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!

micajuine-ho May 29, 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

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!

micajuine-ho commented Apr 24, 2020 •

edited

Loading

micajuine-ho commented Apr 25, 2020 •

edited

Loading

zhouyx commented May 27, 2020 •

edited

Loading

micajuine-ho May 29, 2020 •

edited

Loading

micajuine-ho May 29, 2020 •

edited

Loading

micajuine-ho commented Jun 1, 2020 •

edited

Loading

zhouyx left a comment •

edited

Loading