Support variable expansion in amp-consent #28700
Conversation
| export function getConsentCID(node) { | ||
| return Services.cidForDoc(node).then((cid) => { | ||
| return cid.get( | ||
| {scope: CID_SCOPE, createCookieIfNotPresent: true}, |
There was a problem hiding this comment.
Note: I fixed the scope of the CLIENT_ID API. Because I don't want the CMP to be able set any scope value to get the tracking id (_ga for example).
The CLIENT_ID here is more like a USER_CONSENT_ID. All CMPs will share the same CID. (In the case where the publisher switch to a different CMP provider).
I could switch to a different variable name given I'm not using the exact CLIENT_ID API to be more specific. (e.g USER_ID). Let me know if you prefer that.
There was a problem hiding this comment.
I think either a comment explaining this distinction or the changing to USER_ID would be sufficient.
There was a problem hiding this comment.
Decided to leave as it is
| export function getConsentCID(node) { | ||
| return Services.cidForDoc(node).then((cid) => { | ||
| return cid.get( | ||
| {scope: CID_SCOPE, createCookieIfNotPresent: true}, |
There was a problem hiding this comment.
I think either a comment explaining this distinction or the changing to USER_ID would be sufficient.
| /** @type {string} */ (this.onUpdateHref_), | ||
| init | ||
| ); | ||
| expandConsentEndpointUrl( |
There was a problem hiding this comment.
Will getConsentId(this.ampdoc_) return a different USER_ID than getConsentId(this.ampdoc_.getBody())?
There was a problem hiding this comment.
this.ampdoc_.getHeadNode() is actually a better method, because the getBody() may not be ready in the case of shadowRoot.
No it should always return the same value. The only reason I need to pass an element here is urlReplacementsForDoc() only takes in element. This is because FIE shares the same ampdoc as the parent doc before the ampdoc-fie experiment is shipped. And we need to use the element to determine if the request is from within the FIE or the ampdoc.
Note <amp-consent> is not supported in FIE, so no need to worry about that.
| it('read cmp config', async () => { | ||
| appendConfigScriptElement(doc, element, dict({})); | ||
| element.setAttribute('type', '_ping_'); | ||
| const consentConfig = new ConsentConfig(element); | ||
| return expect( | ||
| consentConfig.getConsentConfigPromise() | ||
| ).to.eventually.deep.equal( | ||
| // Make a deep copy of the config to avoid error when deleting fields | ||
| const config = JSON.parse( |
There was a problem hiding this comment.
IMO, it would be great if would could fake the ping cmp here, so that we don't have to alter cmps.js.
There was a problem hiding this comment.
It's actually the other way around. I intended to change the fake setting in cmps.js. For manual testing reason, and also to serve as an example to other CMPS. I changed the unit test here only to fix it.
| @@ -205,7 +205,7 @@ export class Services { | |||
| } | |||
|
|
|||
| /** | |||
| * @param {!Element|!./service/ampdoc-impl.AmpDoc} elementOrAmpDoc | |||
| * @param {!Element|!ShadowRoot|!./service/ampdoc-impl.AmpDoc} elementOrAmpDoc | |||
There was a problem hiding this comment.
@jridgewell for approval
There was a problem hiding this comment.
Can shadowRoots have different CIDs? If so, this is broken, since it'll get the shared ampdoc for the page.
There was a problem hiding this comment.
shadowRoot gets the same CID. We haven't get request to create new CID for shadowRoot, afaik content in the shadowRoot are from the same domain thus should get the same id.
I added this to fix type check. Shouldn't affect behavior.
There was a problem hiding this comment.
@jridgewell Does the sound good to you?
* support variable expansion in amp-consent * switch to head * type check
Closes #28588