ampproject / amphtml

The AMP web component framework.
https://amp.dev
Apache License 2.0
14.89k stars 3.89k forks source link

Add e2e test for all amp-consent metadata values #28634

Closed micajuine-ho closed 2 years ago

micajuine-ho commented 4 years ago

Use external API provided to test (or analytics variable substations).

tla-sirdata commented 4 years ago

Hi @micajuine-ho I'm not sure if it's related to this issue but I have the following error in the console since a couple of days on one of our publisher's website. log.js:258 [amp-consent] CMP metadata is invalid or no consent string is found.

It occurs whether there's an existing consent string or not. Can you tell me what is it related to ? It doesn't seem to impact the consent string processing though.

Thanks

MaximePlancke commented 4 years ago

Hi @micajuine-ho

We also have this error since a couple of days on our clients website, either the consent string is present or not. [amp-consent] CMP metadata is invalid or no consent string is found It seems related to the metadata JSON object.

Thank you

micajuine-ho commented 4 years ago

@MaximePlancke @tla-sirdata Thanks for your reports.

The error your clients are seeing is a bug that was resolved in #28623. Fix should be in stable next Monday.

tla-sirdata commented 4 years ago

Hi @micajuine-ho We still have the error in the console when there's no existing consent string. Is it expected ? Thanks

micajuine-ho commented 4 years ago

Hi @tla-sirdata, AMP was on freeze last week due to the holiday schedule. This may be why you're not seeing the error removed.

kemalmustafic commented 4 years ago

Hi @micajuine-ho,

We are still seeing error CMP metadata is invalid or no consent string is found. before the user gives consent. After the consent is given, there is no error message.

Are you planning to remove this error message logging before consent giving?

micajuine-ho commented 4 years ago

Hi @kemalmustafic can I see the a webpage where you are seeing this error?

kemalmustafic commented 4 years ago

Sure @micajuine-ho,

TCFv1 example: https://sandbox.didomi.io/amp.html TCFv2 example: https://sandbox.didomi.io/amp-tcf-v2.html

In both cases, you will see the error message CMP metadata is invalid or no consent string is found. before giving consent.

After you give consent and reload the page, there will be no error message

micajuine-ho commented 4 years ago

Hi @kemalmustafic, this is occuring because the checkConsentHref endpoint is returning metadata but no consentString. We assumed that metadata and consentString would operate in conjunction. Is this case with metadata but no consentString very prevalent?

tla-sirdata commented 4 years ago

Hi @micajuine-ho We always use metadata to transmit the new gdprApplies flag whether there's an existing consent string or not so yes this case is prevalent for us. My understanding is that metadata.gdprApplies is supposed to replace consentRequired in the long term. Is that correct ?

micajuine-ho commented 4 years ago

We always use metadata to transmit the new gdprApplies flag whether there's an existing consent string or not so yes this case is prevalent for us.

In that case I'll remove the requirement for consentString to be present for metadata to be updated.

My understanding is that metadata.gdprApplies is supposed to replace consentRequired in the long term. Is that correct ?

This was not the intention, as we figured that consent may still be required outside of gdpr regulation areas.

tla-sirdata commented 4 years ago

Yes that's why I suggested creating a ccpaApplies flag so anyone can easily identify which regulation is applied but it's probably not a priority at the moment. Thanks !

kemalmustafic commented 4 years ago

Hi @kemalmustafic, this is occuring because the checkConsentHref endpoint is returning metadata but no consentString. We assumed that metadata and consentString would operate in conjunction. Is this case with metadata but no consentString very prevalent?

@micajuine-ho Right, we have a case where metadata is present but the consent string is not.

Is it possible to not display the error message in this case?

micajuine-ho commented 4 years ago

My apologies, this got lost in the weeds. I have submitted a PR (https://github.com/ampproject/amphtml/pull/30465) to fix this.

micajuine-ho commented 4 years ago

@kemalmustafic @tla-sirdata

When you are sending in new metadata, is there a reason why you are not sending a consent string?

If we were to remove the restriction right now as it is, when new metadata would be sent without a consent string, the existing consent string in storage would be erased. Is this the behavior you would expect?

tla-sirdata commented 4 years ago

@micajuine-ho The reasons why the consent string is not sent can be :

Most of the time, that means there's no existing consent string. If we want to erase a consent string, we send expireCache=true but the consent string is still sent with it.

micajuine-ho commented 4 years ago

Got it. Sounds like there is a good use case for this behavior!

micajuine-ho commented 4 years ago

I believe this change will be in before the holiday freeze.

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.