amp-consent metadata values#28187
Conversation
zhouyx
left a comment
There was a problem hiding this 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) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
To follow up: we could consider report this as an error to CMP.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Talked offline: userError for iframe seems like the way to go here.
| */ | ||
| function composeMetadataStoreValue(unused) { | ||
| const storageMetadata = {}; | ||
| // TODO(micajuineho) if (metadata['gdprApplies']) {...} |
There was a problem hiding this comment.
IMO: composeMetadataStoreValue and convertStorageMetadata are a pair of helper methods. What about we implement them together in a separate PR?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Sounds good to me! Correct me, but I think you have convertStorageMetadata() implemented?
There was a problem hiding this comment.
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
| isDirtyEqual = !!infoA['isDirty'] === !!infoB['isDirty']; | ||
| } | ||
| return stateEqual && stringEqual && isDirtyEqual; | ||
| const metadataEqual = isMetadataEqual( |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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
}
There was a problem hiding this comment.
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.
| */ | ||
| function composeMetadataStoreValue(unused) { | ||
| const storageMetadata = {}; | ||
| // TODO(micajuineho) if (metadata['gdprApplies']) {...} |
| export function constructConsentInfo( | ||
| consentState, | ||
| opt_consentString, | ||
| opt_consentMetadata, |
There was a problem hiding this comment.
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.
|
@zhouyx PTAL |
consentMetadatatocheckConsentHrefresponse and request and external iframe consent flow.consentMetadatatoupdateHrefRequestbodyconsentMetadataas part of client info for iframe creationconsentInfoobject (only ifconsentStringis present and valid)METADATAobject ('m': {}) to localstoragesrc/consent#getConsentMetadataInfo()APIgdprAppliesandconsentTypefields (Adding consentStringType to amp-consent metadata #28028 & ✨ amp-consent: Gdpr applies value #27759)