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

✨Add Didomi to the CMPs list in the amp-consent extension #21693

Merged
merged 4 commits into from Apr 8, 2019

Conversation

MaximePlancke
Copy link
Contributor

As discussed on this thread #17742, AMP will include CMP integration on his extension amp-consent. This PR integrate Didomi to the list of CMPs compatible with AMP.

You can find an example of integration here: https://sdk-amp.privacy-center.org/demo.html
If you don't see the banner, please run this in your console : AMP.toggleExperiment('amp-consent-v2'). When you click on accept, you will see appearing the picture, originally blocked before a consent was given.

Limitations and improvements:

  • We are currently only supporting and sharing the IAB consentString and returning accepted as the consentState value for every consent given. Indeed, Didomi is not working with a global consent but a consent by vendors. We need to take care of the custom vendors/purposes globally in our SDK to support the consentState correctly.
  • We are waiting on the changes from AMP discussed on I2I: Support CMP Integration with <amp-consent> #17742 to implement a better way to display, or not, the banner from the response of the POST endpoint (e.g: add the consent string in the parameters and call the endpoint every time to determine if the notice needs to be display or not)

Not sure what to reference. Is it the one ? ampproject/wg-monetization#1

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@torch2424
Copy link
Contributor

@MaximePlancke Thank you for this!

Let me know if you need help with CLA, or if you have a corporate CLA signed.

Thanks! 😄

@zhouyx
Copy link
Contributor

zhouyx commented Apr 5, 2019

@MaximePlancke Thank you for the work.
Could you please upload an example page and documentation file. I'm taking vacation, @torch2424 please help with reviewing. Thank you

@MaximePlancke
Copy link
Contributor Author

Hello @torch2424 @zhouyx

Thank you for your replies. We have just signed a CLA for Didomi. It is linked to my Didomi email address. I will push a new commit with it.

Concerning the example + doc, we have an example here: https://sdk-amp.privacy-center.org/demo.html What exactly do you want as a doc? We are going to update our documentation on how to use Didomi as a CMP with AMP for our clients but in this example it is already configured.

Thank you

@MaximePlancke
Copy link
Contributor Author

Hello @torch2424

Seems like it didn't work. If you could help me with that that would be great. The email address is maxime.plancke@didomi.io

Thank you in advance :)

@torch2424
Copy link
Contributor

cc @mrjoro for help with the corporate CLA 😄

For the example / documentation, I think @zhouyx is referring to something we do similar for Ads. Which is something I'll go ahead and whip up now while we sort out your CLA.

Thanks! 👍

@mrjoro
Copy link
Member

mrjoro commented Apr 5, 2019

@MaximePlancke and @torch2424, I don't yet see Didomi in the list of corporate CLAs, though since it is a manual process to approve corporate CLAs it can take a few days. Maxime did Didomi just sign the corp CLA today?

@MaximePlancke
Copy link
Contributor Author

@mrjoro Indeed we just sign it today. I guess I'll try again in a few days :)

Thank you

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Apr 8, 2019
Copy link
Contributor

@torch2424 torch2424 left a comment

Choose a reason for hiding this comment

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

Thanks for the change! 😄

CLA Looks good, merging...

@torch2424 torch2424 merged commit 8da08ea into ampproject:master Apr 8, 2019
@torch2424
Copy link
Contributor

@MaximePlancke

After #21752 is merged, do you mind if I ask you add yourself to our own CMP documentation and to provide an example for our new cmp-vendors example page? The plan would to also have a list of supported vendors on the AMP documentation page so users could find your vendor from our docs 😄

cc @zhouyx

@MaximePlancke
Copy link
Contributor Author

@torch2424

Sure there is no problem. Could you provide more information about what we will have to do? Where in the doc and where in the example folder do we have to add our CMP?

Thank you :)

@torch2424
Copy link
Contributor

@MaximePlancke

Awesome thank you! So yes, once we merge #21752 I will pass that info along. The reason why I don't do that now, as a lot of the team is currently travelling or working towards the upcoming AMP Conf. Once we get back and sync from that, I'll be sure to ping you again. For now, I think we will be fine by just having your users using the documentation provided on your site.

Thanks! 😄

@MaximePlancke
Copy link
Contributor Author

@torch2424

Alright no problem :). Thank you for the explanation

@zhouyx
Copy link
Contributor

zhouyx commented May 15, 2019

@MaximePlancke

We decide that the current consentState value is confusing because it is accepted/rejected/unknown in some cases, but true/false/null in other cases. To fix this and also to be backward compatible we decide to introduce new consentStateValue which always resolve to accepted/rejected/unknown

We are going to deprecate the usage of consentState which pass consent state value to iframe in the name attribute. Instead we will be changing the name to consentStateValue. Please see the updated doc change #22308

Could you please review your implementation and change the usage. We will be deprecating the consentState pretty soon. Thank you very much! Please let me know if there's any question.

@MaximePlancke
Copy link
Contributor Author

Hi @zhouyx

Thank you for the notification. We will spend some time next week to make those changes.

Thank you!

@MaximePlancke
Copy link
Contributor Author

Hi @zhouyx

The changes has been made on our side. Thank you :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants