✨ Pass previous stored consent info to CMP iframe #20242
Conversation
| return consentStateManager.getConsentInstanceInfo().then(consentInfo => { | ||
| return dict({ | ||
| 'clientConfig': this.clientConfig_, | ||
| 'consentState': consentInfo['consentState'], |
There was a problem hiding this comment.
I decided to use 'consentState' and 'consentString', instead of a 'consentInfo' object because this leaves me more flexibility to change the internal consentInfo object format in the future.
One thing I'd like to get your input is that: what do you think we should pass to the consentState. Right now, it's an enum value which essentially is a number. Do you think that we should keep a map and convert the enum value to a string?
There was a problem hiding this comment.
I very much like your idea of keeping this flexible, and passing parts of the consentInfo, as I am sure consent will evolve in the coming years, and require different information.
For the cosentState going from a number (0, 1, 2) to a string (accept, reject, dismiss), I definitely think we should use a string. It will make the code a lot more clear on their end, and isn't hard for us to do at all. Especially since they can simply build their own ENUMs and logic around a descriptive string, much easier than a number. I also feel like if we change/add new string values, it will probably be caught by an engineer, rather than someone scrolling through the documentation again 😄
There was a problem hiding this comment.
Sounds good. I added a helper method to convert the enum value to string (accepted/rejected/unknown)
This brings another question : )
The API we provide to CMP looks like
action: 'accept/reject/dismiss', but the consent state we pass back looks like 'accepted/rejected/unknown'. Does this look confusing to any of you?
There was a problem hiding this comment.
Ummmm. To me it depends if unknown and dismiss mean the same thing?
If they don't I think that should be fine, because we are defining an action (present tense), and then we report back the state after the action has taken place (past tense).
If they do...Actually, I think distinguishing them still might be better. That way on the engineering side their is less confusion what was the action (request) vs. what is the state (response). Plus it would allow for more flexibility in the future, in case we ever want a state like "accepted, but now rejected" or something like that.
There was a problem hiding this comment.
Talked offline. We agreed it's somewhat confusing in both ways. But dismiss doesn't necessary equal to unknown, because there could be stored value. Because of this, we decided to go with accepted/rejected/unknown
PTAL
| const config = dict({ | ||
| 'promptUISrc': 'https//promptUISrc', | ||
| }); | ||
| consentUI = | ||
| new ConsentUI(mockInstance, config); | ||
| expect(elementByTag(parent, 'iframe')).to.be.null; | ||
| consentUI.show(); | ||
| yield macroTask(); | ||
| yield macroTask(); |
There was a problem hiding this comment.
nit: can you try this with one yield macroTask() ? I've found this works with just one and doesn't need 3? 🤔
There was a problem hiding this comment.
Ahhh! Thank you for catching this. This was my attempt to fix the test, but it didn't work out. I ended up mocking the getConsentInstanceInfo from consent state manager.
test/manual/amp-consent-cmp.html
Outdated
| @@ -241,7 +241,7 @@ <h3>Image that is NOT blocked by consent</h3> | |||
| "consents": {}, | |||
| "postPromptUI": "postPromptUI", | |||
| "clientConfig": { | |||
| "CMP_id": "asdf", | |||
| "CMP_id": "test_id", | |||
| "other_info": "ahhhh" | |||
* pass stored consent to iframe * pass string * nit
This PR implements the feature to pass the previously stored/local consent information to CMP iframe.