Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

amp-consent metadata values #28187

Merged
merged 4 commits into from May 15, 2020
Merged

Conversation

micajuine-ho
Copy link
Contributor

@micajuine-ho micajuine-ho commented May 4, 2020

  • Adds consentMetadata to checkConsentHref response and request and external iframe consent flow.
  • Adds consentMetadata to updateHrefRequest body
  • Adds consentMetadata as part of client info for iframe creation
  • Adds metadata to consentInfo object (only if consentString is present and valid)
  • Adds METADATA object ('m': {}) to localstorage
  • Adds src/consent#getConsentMetadataInfo() API
  • Will add/consolidate gdprApplies and consentType fields (Adding consentStringType to amp-consent metadata #28028 & amp-consent: Gdpr applies value  #27759)

Copy link
Contributor

@zhouyx zhouyx left a comment

Thank you! I like that we organize the additional information to metadata.
One thing I think is missing: You'll also need to pass the metadata to the created CMP iframe.
Please refer to ConsentUI.getClientInfoPromise_()

* @return {Object=}
*/
export function configureMetadataByConsentString(metadata, consentString) {
if (!metadata || typeof metadata != 'object' || !consentString) {
Copy link
Contributor

@zhouyx zhouyx May 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: if (!isObject(metadata) ||!consentString).

Also I found the helper method to not be called a lot. Do you think we can inline the check to the #updateConsentInstanceState() method instead?

Copy link
Contributor

@zhouyx zhouyx May 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To follow up: we could consider report this as an error to CMP.

Copy link
Contributor Author

@micajuine-ho micajuine-ho May 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to move the method.

report this as an error to CMP.

Error reporting to iframe lives in ConsentUI through the post message API format we created. Should we use this?

Copy link
Contributor

@zhouyx zhouyx May 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

postMessage or user error both sounds fine. Caveat is we can report as user error if the data comes from the checkConsentHref endpoint.
Do we report user error with invalid checkConsentHref response today?

Copy link
Contributor Author

@micajuine-ho micajuine-ho May 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Talked offline: userError for iframe seems like the way to go here.

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
*/
function composeMetadataStoreValue(unused) {
const storageMetadata = {};
// TODO(micajuineho) if (metadata['gdprApplies']) {...}
Copy link
Contributor

@zhouyx zhouyx May 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO: composeMetadataStoreValue and convertStorageMetadata are a pair of helper methods. What about we implement them together in a separate PR?

Copy link
Contributor Author

@micajuine-ho micajuine-ho May 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was my idea as well. Just wanted to lay the skeleton out here, and then add implementation as we add gdprApplies and consentStringType to metadata. Is that ok?

Copy link
Contributor

@zhouyx zhouyx May 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me! Correct me, but I think you have convertStorageMetadata() implemented?

Copy link
Contributor Author

@micajuine-ho micajuine-ho May 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, it shouldn't do anything, and I wanted to make sure my ideas were valid by fleshing it out. Should I remove it?

Will remove for now

Copy link
Contributor

@zhouyx zhouyx May 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SG. Thanks

@@ -191,36 +212,55 @@ export function isConsentInfoStoredValueSame(infoA, infoB, opt_isDirty) {
} else {
isDirtyEqual = !!infoA['isDirty'] === !!infoB['isDirty'];
}
return stateEqual && stringEqual && isDirtyEqual;
const metadataEqual = isMetadataEqual(
Copy link
Contributor

@zhouyx zhouyx May 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I introduced this check to help prevent some unnecessary call to the storageAPI.
Now with the stored value getting more complicated, I am no longer sure if the check itself is worth it...

Copy link
Contributor Author

@micajuine-ho micajuine-ho May 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's still worth it, since checking the metadata shouldn't be too much work compared to fetching and parsing info from local storage. Happy to remove the check if you think otherwise.

Copy link
Contributor Author

@micajuine-ho micajuine-ho May 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Talked offline: we will measure performance of this later and determine if it should be removed or not.

* @param {string} policyId
* @return {!Promise<?Object|undefined>}
*/
export function getConsentMetadata(element, policyId) {
Copy link
Contributor

@zhouyx zhouyx May 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the metadata always comes with the consent string. What do you think about returning something like? We don't need to finalize the API in this PR, but providing a thought

{
  string,
  metadata
}

Copy link
Contributor Author

@micajuine-ho micajuine-ho May 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there some situations where we only need the consentString and not the additional metadata?
One use case that comes to mind is the current CONSENT_STRING macro. That would be a reason to keep them separate.

@micajuine-ho micajuine-ho requested a review from zhouyx May 11, 2020
extensions/amp-consent/0.1/amp-consent.js Outdated Show resolved Hide resolved
extensions/amp-consent/0.1/test/test-consent-info.js Outdated Show resolved Hide resolved
*/
function composeMetadataStoreValue(unused) {
const storageMetadata = {};
// TODO(micajuineho) if (metadata['gdprApplies']) {...}
Copy link
Contributor

@zhouyx zhouyx May 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SG. Thanks

* @param {boolean=} opt_isDirty
* @return {!ConsentInfoDef}
*/
export function constructConsentInfo(
consentState,
opt_consentString,
opt_consentMetadata,
Copy link
Contributor

@zhouyx zhouyx May 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand why you want to pass the consent metadata before the isDirty. Please double check that all calling to the #constructConsentInfo() have been updated.

Copy link
Contributor Author

@micajuine-ho micajuine-ho May 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. Done

@micajuine-ho
Copy link
Contributor Author

micajuine-ho commented May 13, 2020

@zhouyx PTAL

@micajuine-ho micajuine-ho merged commit e0b3b17 into ampproject:master May 15, 2020
14 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants